Handle inverted rects in SkRRect creation methods

An alternative way of addressing this is to alter SkCanvas::drawRoundRect to just reject isEmpty (i.e., un-sorted or truly empty) input rects.

BUG=skia:3786

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

4 files changed, 126 insertions, 42 deletions

diff --git a/gm/roundrects.cpp b/gm/roundrects.cpp
index ab7ee696c2..cf3c5e234d 100644
--- a/gm/roundrects.cpp
+++ b/gm/roundrects.cpp
@@ -18,6 +18,15 @@
 
 namespace skiagm {
 
+static SkColor gen_color(SkRandom* rand) {
+    SkScalar hsv[3];
+    hsv[0] = rand->nextRangeF(0.0f, 360.0f);
+    hsv[1] = rand->nextRangeF(0.75f, 1.0f);
+    hsv[2] = rand->nextRangeF(0.75f, 1.0f);
+
+    return sk_tool_utils::color_to_565(SkHSVToColor(hsv));
+}
+
 class RoundRectGM : public GM {
 public:
     RoundRectGM() {
@@ -127,19 +136,10 @@ protected:
         }
     }
 
-    SkColor genColor(SkRandom* rand) {
-        SkScalar hsv[3];
-        hsv[0] = rand->nextRangeF(0.0f, 360.0f);
-        hsv[1] = rand->nextRangeF(0.75f, 1.0f);
-        hsv[2] = rand->nextRangeF(0.75f, 1.0f);
-
-        return sk_tool_utils::color_to_565(SkHSVToColor(hsv));
-    }
-
     void onDraw(SkCanvas* canvas) override {
         SkRandom rand(1);
         canvas->translate(20 * SK_Scalar1, 20 * SK_Scalar1);
-        SkRect rect = SkRect::MakeLTRB(-20, -30, 20, 30);
+        const SkRect rect = SkRect::MakeLTRB(-20, -30, 20, 30);
         SkRRect circleRect;
         circleRect.setRectXY(rect, 5, 5);
 
@@ -167,7 +167,7 @@ protected:
                                   3 * SK_Scalar1 / 4);
                 canvas->concat(mat);
 
-                SkColor color = genColor(&rand);
+                SkColor color = gen_color(&rand);
                 fPaints[i].setColor(color);
 
                 canvas->drawRect(rect, rectPaint);
@@ -192,7 +192,7 @@ protected:
             canvas->translate(kXStart + SK_Scalar1 * kXStep * 2.55f + SK_Scalar1 / 4,
                               kYStart + SK_Scalar1 * kYStep * i + 3 * SK_Scalar1 / 4);
 
-            SkColor color = genColor(&rand);
+            SkColor color = gen_color(&rand);
             fPaints[i].setColor(color);
 
             canvas->drawRect(rect, rectPaint);
@@ -212,7 +212,7 @@ protected:
                               kYStart + SK_Scalar1 * kYStep * i + 3 * SK_Scalar1 / 4 +
                               SK_ScalarHalf * kYStep);
 
-            SkColor color = genColor(&rand);
+            SkColor color = gen_color(&rand);
             fPaints[i].setColor(color);
 
             canvas->drawRect(rect, rectPaint);
@@ -231,7 +231,7 @@ protected:
             canvas->translate(kXStart + SK_Scalar1 * kXStep * 3.25f + SK_Scalar1 / 4,
                               kYStart + SK_Scalar1 * kYStep * i + 3 * SK_Scalar1 / 4);
 
-            SkColor color = genColor(&rand);
+            SkColor color = gen_color(&rand);
             fPaints[i].setColor(color);
 
             canvas->drawRRect(circleRect, fPaints[i]);
@@ -250,7 +250,7 @@ protected:
                               kYStart + SK_Scalar1 * kYStep * i + 3 * SK_Scalar1 / 4 +
                               SK_ScalarHalf * kYStep);
 
-            SkColor color = genColor(&rand);
+            SkColor color = gen_color(&rand);
             fPaints[i].setColor(color);
 
             canvas->drawRRect(circleRect, fPaints[i]);
@@ -275,7 +275,7 @@ protected:
                               kYStart + SK_Scalar1 * kYStep * i + 3 * SK_Scalar1 / 4 +
                               SK_ScalarHalf * kYStep);
 
-            SkColor color = genColor(&rand);
+            SkColor color = gen_color(&rand);
             fPaints[i].setColor(color);
             fPaints[i].setShader(shader);
 
@@ -310,7 +310,7 @@ protected:
                                   kYStart + SK_Scalar1 * kYStep * i + 3 * SK_Scalar1 / 4 +
                                   SK_ScalarHalf * kYStep);
 
-                SkColor color = genColor(&rand);
+                SkColor color = gen_color(&rand);
 
                 SkPaint p;
                 p.setAntiAlias(true);
@@ -323,6 +323,24 @@ protected:
             }
         }
 
+        // test old entry point (skbug.com/3786)
+        {
+            canvas->save();
+
+            canvas->translate(kXStart + SK_Scalar1 * kXStep * 5 + SK_Scalar1 / 4,
+                              kYStart + SK_Scalar1 * kYStep * 4 + SK_Scalar1 / 4 +
+                              SK_ScalarHalf * kYStep);
+
+            const SkColor color = gen_color(&rand);
+
+            SkPaint p;
+            p.setColor(color);
+
+            const SkRect oooRect = { 20, 30, -20, -30 };     // intentionally out of order
+            canvas->drawRoundRect(oooRect, 10, 10, p);
+
+            canvas->restore();
+        }
     }
 
 private:
diff --git a/include/core/SkRRect.h b/include/core/SkRRect.h
index 064e7be8e4..8d8cdbf02c 100644
--- a/include/core/SkRRect.h
+++ b/include/core/SkRRect.h
@@ -127,12 +127,14 @@ public:
      * Set this RR to match the supplied rect. All radii will be 0.
      */
     void setRect(const SkRect& rect) {
-        if (rect.isEmpty()) {
+        fRect = rect;
+        fRect.sort();
+
+        if (fRect.isEmpty()) {
             this->setEmpty();
             return;
         }
 
-        fRect = rect;
         memset(fRadii, 0, sizeof(fRadii));
         fType = kRect_Type;
 
@@ -156,15 +158,17 @@ public:
      * width and all y radii will equal half the height.
      */
     void setOval(const SkRect& oval) {
-        if (oval.isEmpty()) {
+        fRect = oval;
+        fRect.sort();
+
+        if (fRect.isEmpty()) {
             this->setEmpty();
             return;
         }
 
-        SkScalar xRad = SkScalarHalf(oval.width());
-        SkScalar yRad = SkScalarHalf(oval.height());
+        SkScalar xRad = SkScalarHalf(fRect.width());
+        SkScalar yRad = SkScalarHalf(fRect.height());
 
-        fRect = oval;
         for (int i = 0; i < 4; ++i) {
             fRadii[i].set(xRad, yRad);
         }
diff --git a/src/core/SkRRect.cpp b/src/core/SkRRect.cpp
index ef811215f4..2f576fa95a 100644
--- a/src/core/SkRRect.cpp
+++ b/src/core/SkRRect.cpp
@@ -11,7 +11,10 @@
 ///////////////////////////////////////////////////////////////////////////////
 
 void SkRRect::setRectXY(const SkRect& rect, SkScalar xRad, SkScalar yRad) {
-    if (rect.isEmpty() || !rect.isFinite()) {
+    fRect = rect;
+    fRect.sort();
+
+    if (fRect.isEmpty() || !fRect.isFinite()) {
         this->setEmpty();
         return;
     }
@@ -25,14 +28,13 @@ void SkRRect::setRectXY(const SkRect& rect, SkScalar xRad, SkScalar yRad) {
         return;
     }
 
-    if (rect.width() < xRad+xRad || rect.height() < yRad+yRad) {
-        SkScalar scale = SkMinScalar(rect.width() / (xRad + xRad), rect.height() / (yRad + yRad));
+    if (fRect.width() < xRad+xRad || fRect.height() < yRad+yRad) {
+        SkScalar scale = SkMinScalar(fRect.width() / (xRad + xRad), fRect.height() / (yRad + yRad));
         SkASSERT(scale < SK_Scalar1);
         xRad = SkScalarMul(xRad, scale);
         yRad = SkScalarMul(yRad, scale);
     }
 
-    fRect = rect;
     for (int i = 0; i < 4; ++i) {
         fRadii[i].set(xRad, yRad);
     }
@@ -47,7 +49,10 @@ void SkRRect::setRectXY(const SkRect& rect, SkScalar xRad, SkScalar yRad) {
 
 void SkRRect::setNinePatch(const SkRect& rect, SkScalar leftRad, SkScalar topRad,
                            SkScalar rightRad, SkScalar bottomRad) {
-    if (rect.isEmpty() || !rect.isFinite()) {
+    fRect = rect;
+    fRect.sort();
+
+    if (fRect.isEmpty() || !fRect.isFinite()) {
         this->setEmpty();
         return;
     }
@@ -64,11 +69,11 @@ void SkRRect::setNinePatch(const SkRect& rect, SkScalar leftRad, SkScalar topRad
     bottomRad = SkMaxScalar(bottomRad, 0);
 
     SkScalar scale = SK_Scalar1;
-    if (leftRad + rightRad > rect.width()) {
-        scale = rect.width() / (leftRad + rightRad);
+    if (leftRad + rightRad > fRect.width()) {
+        scale = fRect.width() / (leftRad + rightRad);
     }
-    if (topRad + bottomRad > rect.height()) {
-        scale = SkMinScalar(scale, rect.height() / (topRad + bottomRad));
+    if (topRad + bottomRad > fRect.height()) {
+        scale = SkMinScalar(scale, fRect.height() / (topRad + bottomRad));
     }
 
     if (scale < SK_Scalar1) {
@@ -79,7 +84,7 @@ void SkRRect::setNinePatch(const SkRect& rect, SkScalar leftRad, SkScalar topRad
     }
 
     if (leftRad == rightRad && topRad == bottomRad) {
-        if (leftRad >= SkScalarHalf(rect.width()) && topRad >= SkScalarHalf(rect.height())) {
+        if (leftRad >= SkScalarHalf(fRect.width()) && topRad >= SkScalarHalf(fRect.height())) {
             fType = kOval_Type;
         } else if (0 == leftRad || 0 == topRad) {
             // If the left and (by equality check above) right radii are zero then it is a rect.
@@ -96,7 +101,6 @@ void SkRRect::setNinePatch(const SkRect& rect, SkScalar leftRad, SkScalar topRad
         fType = kNinePatch_Type;
     }
 
-    fRect = rect;
     fRadii[kUpperLeft_Corner].set(leftRad, topRad);
     fRadii[kUpperRight_Corner].set(rightRad, topRad);
     fRadii[kLowerRight_Corner].set(rightRad, bottomRad);
@@ -138,7 +142,10 @@ static double compute_min_scale(double rad1, double rad2, double limit, double c
 }
 
 void SkRRect::setRectRadii(const SkRect& rect, const SkVector radii[4]) {
-    if (rect.isEmpty() || !rect.isFinite()) {
+    fRect = rect;
+    fRect.sort();
+
+    if (fRect.isEmpty() || !fRect.isFinite()) {
         this->setEmpty();
         return;
     }
@@ -148,7 +155,6 @@ void SkRRect::setRectRadii(const SkRect& rect, const SkVector radii[4]) {
         return;
     }
 
-    fRect = rect;
     memcpy(fRadii, radii, sizeof(fRadii));
 
     bool allCornersSquare = true;
@@ -184,10 +190,10 @@ void SkRRect::setRectRadii(const SkRect& rect, const SkVector radii[4]) {
     // If f < 1, then all corner radii are reduced by multiplying them by f."
     double scale = 1.0;
 
-    scale = compute_min_scale(fRadii[0].fX, fRadii[1].fX, rect.width(),  scale);
-    scale = compute_min_scale(fRadii[1].fY, fRadii[2].fY, rect.height(), scale);
-    scale = compute_min_scale(fRadii[2].fX, fRadii[3].fX, rect.width(),  scale);
-    scale = compute_min_scale(fRadii[3].fY, fRadii[0].fY, rect.height(), scale);
+    scale = compute_min_scale(fRadii[0].fX, fRadii[1].fX, fRect.width(),  scale);
+    scale = compute_min_scale(fRadii[1].fY, fRadii[2].fY, fRect.height(), scale);
+    scale = compute_min_scale(fRadii[2].fX, fRadii[3].fX, fRect.width(),  scale);
+    scale = compute_min_scale(fRadii[3].fY, fRadii[0].fY, fRect.height(), scale);
 
     if (scale < 1.0) {
         for (int i = 0; i < 4; ++i) {
@@ -204,8 +210,8 @@ void SkRRect::setRectRadii(const SkRect& rect, const SkVector radii[4]) {
     //     path.addRRect(rrect);
     //     rrect.rect() != path.getBounds()
     for (int i = 0; i < 4; ++i) {
-        fRadii[i].fX = clamp_radius_check_predicates(fRadii[i].fX, rect.fLeft, rect.fRight);
-        fRadii[i].fY = clamp_radius_check_predicates(fRadii[i].fY, rect.fTop, rect.fBottom);
+        fRadii[i].fX = clamp_radius_check_predicates(fRadii[i].fX, fRect.fLeft, fRect.fRight);
+        fRadii[i].fY = clamp_radius_check_predicates(fRadii[i].fY, fRect.fTop, fRect.fBottom);
     }
     // At this point we're either oval, simple, or complex (not empty or rect).
     this->computeType();
diff --git a/tests/RoundRectTest.cpp b/tests/RoundRectTest.cpp
index 8d8f76fbf0..02dad22e08 100644
--- a/tests/RoundRectTest.cpp
+++ b/tests/RoundRectTest.cpp
@@ -47,6 +47,61 @@ static void test_empty_crbug_458524(skiatest::Reporter* reporter) {
     REPORTER_ASSERT(reporter, SkRRect::kEmpty_Type == other.getType());
 }
 
+// Test that all the SkRRect entry points correctly handle un-sorted and
+// zero-sized input rects
+static void test_empty(skiatest::Reporter* reporter) {
+    static const SkRect oooRects[] = {  // out of order
+        { 100, 0, 0, 100 },  // ooo horizontal
+        { 0, 100, 100, 0 },  // ooo vertical
+        { 100, 100, 0, 0 },  // ooo both
+    };
+
+    static const SkRect emptyRects[] = {
+        { 100, 100, 100, 200 }, // empty horizontal
+        { 100, 100, 200, 100 }, // empty vertical
+        { 100, 100, 100, 100 }, // empty both
+        { 0, 0, 0, 0 }          // setEmpty-empty
+    };
+
+    static const SkVector radii[4] = { { 0, 1 }, { 2, 3 }, { 4, 5 }, { 6, 7 } };
+
+    SkRRect r;
+
+    for (size_t i = 0; i < SK_ARRAY_COUNT(oooRects); ++i) {
+        r.setRect(oooRects[i]);
+        REPORTER_ASSERT(reporter, !r.isEmpty());
+
+        r.setOval(oooRects[i]);
+        REPORTER_ASSERT(reporter, !r.isEmpty());
+
+        r.setRectXY(oooRects[i], 1, 2);
+        REPORTER_ASSERT(reporter, !r.isEmpty());
+
+        r.setNinePatch(oooRects[i], 0, 1, 2, 3);
+        REPORTER_ASSERT(reporter, !r.isEmpty());
+
+        r.setRectRadii(oooRects[i], radii);
+        REPORTER_ASSERT(reporter, !r.isEmpty());
+    }
+
+    for (size_t i = 0; i < SK_ARRAY_COUNT(emptyRects); ++i) {
+        r.setRect(emptyRects[i]);
+        REPORTER_ASSERT(reporter, r.isEmpty());
+
+        r.setOval(emptyRects[i]);
+        REPORTER_ASSERT(reporter, r.isEmpty());
+
+        r.setRectXY(emptyRects[i], 1, 2);
+        REPORTER_ASSERT(reporter, r.isEmpty());
+
+        r.setNinePatch(emptyRects[i], 0, 1, 2, 3);
+        REPORTER_ASSERT(reporter, r.isEmpty());
+
+        r.setRectRadii(emptyRects[i], radii);
+        REPORTER_ASSERT(reporter, r.isEmpty());
+    }
+}
+
 static const SkScalar kWidth = 100.0f;
 static const SkScalar kHeight = 100.0f;
 
@@ -679,4 +734,5 @@ DEF_TEST(RoundRect, reporter) {
     test_issue_2696(reporter);
     test_tricky_radii(reporter);
     test_empty_crbug_458524(reporter);
+    test_empty(reporter);
 }