Fix rrects that are large enough that we lose/gain a bit when we add the radius to a bounds coordinate.

add test that triggers assert in addRRect

BUG=skia:3239

Review URL: https://codereview.chromium.org/803153003

4 files changed, 83 insertions, 4 deletions

diff --git a/include/core/SkPathRef.h b/include/core/SkPathRef.h
index 78c6cd5af7..ded8e3fb64 100644
--- a/include/core/SkPathRef.h
+++ b/include/core/SkPathRef.h
@@ -253,6 +253,8 @@ public:
      */
     uint32_t genID() const;
 
+    SkDEBUGCODE(void validate() const;)
+
 private:
     enum SerializationOffsets {
         kIsFinite_SerializationShift = 25,  // requires 1 bit
@@ -415,8 +417,6 @@ private:
         return reinterpret_cast<intptr_t>(fVerbs) - reinterpret_cast<intptr_t>(fPoints);
     }
 
-    SkDEBUGCODE(void validate() const;)
-
     /**
      * Called the first time someone calls CreateEmpty to actually create the singleton.
      */
diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp
index f8280bd6d2..6dc056e81f 100644
--- a/src/core/SkPath.cpp
+++ b/src/core/SkPath.cpp
@@ -1118,6 +1118,7 @@ void SkPath::addRRect(const SkRRect& rrect, Direction dir) {
         }
         this->close();
     }
+    SkDEBUGCODE(fPathRef->validate();)
 }
 
 bool SkPath::hasOnlyMoveTos() const {
diff --git a/src/core/SkRRect.cpp b/src/core/SkRRect.cpp
index d3a571fe99..58e7de7910 100644
--- a/src/core/SkRRect.cpp
+++ b/src/core/SkRRect.cpp
@@ -97,6 +97,32 @@ void SkRRect::setNinePatch(const SkRect& rect, SkScalar leftRad, SkScalar topRad
     SkDEBUGCODE(this->validate();)
 }
 
+/*
+ *  TODO: clean this guy up and possibly add to SkScalar.h
+ */
+static inline SkScalar SkScalarDecULP(SkScalar value) {
+#if SK_SCALAR_IS_FLOAT
+        return SkBits2Float(SkFloat2Bits(value) - 1);
+#else
+    #error "need impl for doubles"
+#endif
+}
+
+static SkScalar clamp_radius_add(SkScalar rad, SkScalar min, SkScalar max) {
+    SkASSERT(rad <= max - min);
+    if (min + rad > max) {
+        rad = SkScalarDecULP(rad);
+    }
+    return rad;
+}
+
+static SkScalar clamp_radius_sub(SkScalar rad, SkScalar min, SkScalar max) {
+    SkASSERT(rad <= max - min);
+    if (max - rad < min) {
+        rad = SkScalarDecULP(rad);
+    }
+    return rad;
+}
 
 void SkRRect::setRectRadii(const SkRect& rect, const SkVector radii[4]) {
     if (rect.isEmpty()) {
@@ -159,11 +185,27 @@ void SkRRect::setRectRadii(const SkRect& rect, const SkVector radii[4]) {
 
     if (scale < SK_Scalar1) {
         for (int i = 0; i < 4; ++i) {
-            fRadii[i].fX = SkScalarMul(fRadii[i].fX, scale);
-            fRadii[i].fY = SkScalarMul(fRadii[i].fY, scale);
+            fRadii[i].fX *= scale;
+            fRadii[i].fY *= scale;
         }
     }
 
+    // skbug.com/3239 -- its possible that we can hit the following inconsistency:
+    //     rad == bounds.bottom - bounds.top
+    //     bounds.bottom - radius < bounds.top
+    //     YIKES
+    // We need to detect and "fix" this now, otherwise we can have the following wackiness:
+    //     path.addRRect(rrect);
+    //     rrect.rect() != path.getBounds()
+    fRadii[0].fX = clamp_radius_add(fRadii[0].fX, rect.fLeft, rect.fRight);
+    fRadii[0].fY = clamp_radius_add(fRadii[0].fY, rect.fTop, rect.fBottom);
+    fRadii[1].fX = clamp_radius_sub(fRadii[1].fX, rect.fLeft, rect.fRight);
+    fRadii[1].fY = clamp_radius_add(fRadii[1].fY, rect.fTop, rect.fBottom);
+    fRadii[2].fX = clamp_radius_sub(fRadii[2].fX, rect.fLeft, rect.fRight);
+    fRadii[2].fY = clamp_radius_sub(fRadii[2].fY, rect.fTop, rect.fBottom);
+    fRadii[3].fX = clamp_radius_add(fRadii[3].fX, rect.fLeft, rect.fRight);
+    fRadii[3].fY = clamp_radius_sub(fRadii[3].fY, rect.fTop, rect.fBottom);
+
     // At this point we're either oval, simple, or complex (not empty or rect)
     // but we lazily resolve the type to avoid the work if the information
     // isn't required.
diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp
index 1a74682e54..4bd4538f1a 100644
--- a/tests/PathTest.cpp
+++ b/tests/PathTest.cpp
@@ -21,6 +21,41 @@
 #include "SkWriter32.h"
 #include "Test.h"
 
+static void set_radii(SkVector radii[4], int index, float rad) {
+    sk_bzero(radii, sizeof(SkVector) * 4);
+    radii[index].set(rad, rad);
+}
+
+static void test_add_rrect(skiatest::Reporter* reporter, const SkRect& bounds,
+                           const SkVector radii[4]) {
+    SkRRect rrect;
+    rrect.setRectRadii(bounds, radii);
+    REPORTER_ASSERT(reporter, bounds == rrect.rect());
+
+    SkPath path;
+    // this line should not assert in the debug build (from validate)
+    path.addRRect(rrect);
+    REPORTER_ASSERT(reporter, bounds == path.getBounds());
+}
+
+static void test_skbug_3239(skiatest::Reporter* reporter) {
+    const float min = SkBits2Float(0xcb7f16c8); /* -16717512.000000 */
+    const float max = SkBits2Float(0x4b7f1c1d); /*  16718877.000000 */
+    const float big = SkBits2Float(0x4b7f1bd7); /*  16718807.000000 */
+
+    const float rad = 33436320;
+
+    const SkRect rectx = SkRect::MakeLTRB(min, min, max, big);
+    const SkRect recty = SkRect::MakeLTRB(min, min, big, max);
+
+    SkVector radii[4];
+    for (int i = 0; i < 4; ++i) {
+        set_radii(radii, i, rad);
+        test_add_rrect(reporter, rectx, radii);
+        test_add_rrect(reporter, recty, radii);
+    }
+}
+
 static void make_path_crbug364224(SkPath* path) {
     path->reset();
     path->moveTo(3.747501373f, 2.724499941f);
@@ -3729,4 +3764,5 @@ DEF_TEST(Paths, reporter) {
     PathRefTest_Private::TestPathRef(reporter);
     test_dump(reporter);
     test_path_crbugskia2820(reporter);
+    test_skbug_3239(reporter);
 }