more path is rect bugs

More edge cases found; clean up the logic a bit
to make more clear where the rectangle points
start and stop.

R=robertphillips@google.com,brianosman@google.com
Bug: 824145,skia:7792
Change-Id: Ie24dfd1519f30875f44ffac68e20d777490b00b9
Reviewed-on: https://skia-review.googlesource.com/120422
Commit-Queue: Cary Clark <caryclark@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>

3 files changed, 164 insertions, 30 deletions

diff --git a/gm/pathfill.cpp b/gm/pathfill.cpp
index d96f356bef..af91b8409d 100644
--- a/gm/pathfill.cpp
+++ b/gm/pathfill.cpp
@@ -432,3 +432,70 @@ DEF_SIMPLE_GM(rotatedcubicpath, canvas, 200, 200) {
 
 DEF_GM( return new PathFillGM; )
 DEF_GM( return new PathInverseFillGM; )
+
+DEF_SIMPLE_GM(bug7792, canvas, 800, 200) {
+    // from skbug.com/7792 bug description
+    SkPaint p;
+    SkPath path;
+    path.moveTo(10, 10);
+    path.moveTo(75, 75);
+    path.lineTo(150, 75);
+    path.lineTo(150, 150);
+    path.lineTo(75, 150);
+    canvas->drawPath(path, p);
+    // from skbug.com/7792 comment 3
+    canvas->translate(200, 0);
+    path.reset();
+    path.moveTo(75, 50);
+    path.moveTo(100, 75);
+    path.lineTo(150, 75);
+    path.lineTo(150, 150);
+    path.lineTo(75, 150);
+    path.lineTo(75, 50);
+    path.close();
+    canvas->drawPath(path, p);
+    // from skbug.com/7792 comment 9
+    canvas->translate(200, 0);
+    path.reset();
+    path.moveTo(10, 10);
+    path.moveTo(75, 75);
+    path.lineTo(150, 75);
+    path.lineTo(150, 150);
+    path.lineTo(75, 150);
+    path.close();
+    canvas->drawPath(path, p);
+    // from skbug.com/7792 comment 11
+    canvas->translate(-200 * 2, 200);
+    path.reset();
+    path.moveTo(75, 150);
+    path.lineTo(75, 75);
+    path.lineTo(150, 75);
+    path.lineTo(150, 150);
+    path.lineTo(75, 150);
+    path.moveTo(75, 150);
+    canvas->drawPath(path, p);
+    // from skbug.com/7792 comment 14
+    canvas->translate(200, 0);
+    path.reset();
+    path.moveTo(250, 75);
+    path.moveTo(250, 75);
+    path.moveTo(250, 75);
+    path.moveTo(100, 75);
+    path.lineTo(150, 75);
+    path.lineTo(150, 150);
+    path.lineTo(75, 150);
+    path.lineTo(75, 75);
+    path.close();
+    path.lineTo(0, 0);
+    path.close();
+    canvas->drawPath(path, p);
+    // from skbug.com/7792 comment 15
+    canvas->translate(200, 0);
+    path.reset();
+    path.moveTo(75, 75);
+    path.lineTo(150, 75);
+    path.lineTo(150, 150);
+    path.lineTo(75, 150);
+    path.moveTo(250, 75);
+    canvas->drawPath(path, p);
+}
diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp
index 284a6a716c..9e242376c0 100644
--- a/src/core/SkPath.cpp
+++ b/src/core/SkPath.cpp
@@ -447,18 +447,20 @@ static int rect_make_dir(SkScalar dx, SkScalar dy) {
 bool SkPath::isRectContour(bool allowPartial, int* currVerb, const SkPoint** ptsPtr,
         bool* isClosed, Direction* direction, SkRect* rect) const {
     int corners = 0;
-    SkPoint first, last;
-    const SkPoint* firstPt = nullptr;
+    SkPoint previous;  // used to construct line from previous point
+    const SkPoint* firstPt = nullptr; // first point in the rect (last of first moves)
+    const SkPoint* lastPt = nullptr;  // last point in the rect (last of lines or first if closed)
     const SkPoint* pts = *ptsPtr;
-    const SkPoint* savePts = nullptr;
-    first.set(0, 0);
-    last.set(0, 0);
+    const SkPoint* savePts = nullptr; // used to allow caller to iterate through a pair of rects
+    previous.set(0, 0);
     int firstDirection = 0;
     int lastDirection = 0;
     int nextDirection = 0;
     bool closedOrMoved = false;
+    bool addedLine = false;
     bool autoClose = false;
     bool insertClose = false;
+    bool accumulatingRect = false;
     int verbCnt = fPathRef->countVerbs();
     while (*currVerb < verbCnt && (!allowPartial || !autoClose)) {
         uint8_t verb = insertClose ? (uint8_t) kClose_Verb : fPathRef->atVerb(*currVerb);
@@ -468,23 +470,27 @@ bool SkPath::isRectContour(bool allowPartial, int* currVerb, const SkPoint** pts
                 pts = *ptsPtr;
                 autoClose = true;
                 insertClose = false;
+                accumulatingRect = false;
             case kLine_Verb: {
-                SkScalar left = last.fX;
-                SkScalar top = last.fY;
+                SkScalar left = previous.fX;
+                SkScalar top = previous.fY;
                 SkScalar right = pts->fX;
                 SkScalar bottom = pts->fY;
+                if (accumulatingRect) {
+                    lastPt = pts;
+                }
                 ++pts;
                 if (left != right && top != bottom) {
                     return false; // diagonal
                 }
+                addedLine = true;
                 if (left == right && top == bottom) {
                     break; // single point on side OK
                 }
                 nextDirection = rect_make_dir(right - left, bottom - top);
                 if (0 == corners) {
                     firstDirection = nextDirection;
-                    first = last;
-                    last = pts[-1];
+                    previous = pts[-1];
                     corners = 1;
                     closedOrMoved = false;
                     break;
@@ -501,7 +507,7 @@ bool SkPath::isRectContour(bool allowPartial, int* currVerb, const SkPoint** pts
                         return false; // too many direction changes
                     }
                 }
-                last = pts[-1];
+                previous = pts[-1];
                 if (lastDirection == nextDirection) {
                     break; // colinear segment
                 }
@@ -525,8 +531,13 @@ bool SkPath::isRectContour(bool allowPartial, int* currVerb, const SkPoint** pts
                     *currVerb -= 1;  // try move again afterwards
                     goto addMissingClose;
                 }
-                firstPt = pts;
-                last = *pts++;
+                if (!addedLine) {
+                    firstPt = pts;
+                    accumulatingRect = true;
+                } else {
+                    accumulatingRect = false;
+                }
+                previous = *pts++;
                 closedOrMoved = true;
                 break;
             default:
@@ -539,10 +550,12 @@ addMissingClose:
         ;
     }
     // Success if 4 corners and first point equals last
-    SkScalar closeX = first.x() - last.x();
-    SkScalar closeY = first.y() - last.y();
+    if (corners < 3 || corners > 4) {
+        return false;
+    }
+    SkPoint closeXY = *firstPt - *lastPt;
     // If autoClose, check if close generates diagonal
-    bool result = 4 == corners && (first == last || (autoClose && (!closeX || !closeY)));
+    bool result = 4 == corners && (closeXY.isZero() || (autoClose && (!closeXY.fX || !closeXY.fY)));
     if (!result) {
         // check if we are just an incomplete rectangle, in which case we can
         // return true, but not claim to be closed.
@@ -550,12 +563,12 @@ addMissingClose:
         //    3 sided rectangle
         //    4 sided but the last edge is not long enough to reach the start
         //
-        if (closeX && closeY) {
+        if (closeXY.fX && closeXY.fY) {
             return false;   // we're diagonal, abort (can we ever reach this?)
         }
-        int closeDirection = rect_make_dir(closeX, closeY);
+        int closeDirection = rect_make_dir(closeXY.fX, closeXY.fY);
         // make sure the close-segment doesn't double-back on itself
-        if (3 == corners || (4 == corners && closeDirection == lastDirection)) {
+        if (3 == corners || closeDirection == lastDirection) {
             result = true;
             autoClose = false;  // we are not closed
         }
@@ -564,7 +577,7 @@ addMissingClose:
         *ptsPtr = savePts;
     }
     if (result && rect) {
-        ptrdiff_t count = (savePts ? savePts : pts) - firstPt;
+        ptrdiff_t count = lastPt - firstPt + 1;
         rect->set(firstPt, (int) count);
     }
     if (result && isClosed) {
diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp
index 85b307a903..ae6568f411 100644
--- a/tests/PathTest.cpp
+++ b/tests/PathTest.cpp
@@ -4901,21 +4901,75 @@ DEF_TEST(ClipPath_nonfinite, reporter) {
 
 // skbug.com/7792
 DEF_TEST(Path_isRect, reporter) {
-    SkPath path;
-    SkPoint points[] = { {10, 10}, {75, 75}, {150, 75}, {150, 150}, {75, 150} };
-    for (size_t index = 0; index < SK_ARRAY_COUNT(points); ++index) {
-        index < 2 ? path.moveTo(points[index]) : path.lineTo(points[index]);
-    }
+    auto makePath = [](const SkPoint* points, size_t count, bool close) -> SkPath {
+        SkPath path;
+        for (size_t index = 0; index < count; ++index) {
+            index < 2 ? path.moveTo(points[index]) : path.lineTo(points[index]);
+        }
+        return path;
+    };
+    auto makePath2 = [](const SkPoint* points, const SkPath::Verb* verbs, size_t count) -> SkPath {
+        SkPath path;
+        for (size_t index = 0; index < count; ++index) {
+            switch (verbs[index]) {
+                case SkPath::kMove_Verb:
+                    path.moveTo(*points++);
+                    break;
+                case SkPath::kLine_Verb:
+                    path.lineTo(*points++);
+                    break;
+                case SkPath::kClose_Verb:
+                    path.close();
+                    break;
+                default:
+                    SkASSERT(0);
+            }
+        }
+        return path;
+    };
+    // isolated from skbug.com/7792 bug description
     SkRect rect;
+    SkPoint points[] = { {10, 10}, {75, 75}, {150, 75}, {150, 150}, {75, 150} };
+    SkPath path = makePath(points, SK_ARRAY_COUNT(points), false);
     REPORTER_ASSERT(reporter, path.isRect(&rect, nullptr, nullptr));
     SkRect compare;
     compare.set(&points[1], SK_ARRAY_COUNT(points) - 1);
     REPORTER_ASSERT(reporter, rect == compare);
-    path.reset();
-    SkPoint points2[] = { {75, 50}, {100, 75}, {150, 75}, {150, 150}, {75, 150}, {75, 50} };
-    for (size_t index = 0; index < SK_ARRAY_COUNT(points); ++index) {
-        index < 2 ? path.moveTo(points2[index]) : path.lineTo(points2[index]);
-    }
-    path.close();
+    // isolated from skbug.com/7792 comment 3
+    SkPoint points3[] = { {75, 50}, {100, 75}, {150, 75}, {150, 150}, {75, 150}, {75, 50} };
+    path = makePath(points3, SK_ARRAY_COUNT(points3), true);
     REPORTER_ASSERT(reporter, !path.isRect(&rect, nullptr, nullptr));
+    // isolated from skbug.com/7792 comment 9
+    SkPoint points9[] = { {10, 10}, {75, 75}, {150, 75}, {150, 150}, {75, 150} };
+    path = makePath(points9, SK_ARRAY_COUNT(points9), true);
+    REPORTER_ASSERT(reporter, path.isRect(&rect, nullptr, nullptr));
+    compare.set(&points9[1], SK_ARRAY_COUNT(points9) - 1);
+    REPORTER_ASSERT(reporter, rect == compare);
+    // isolated from skbug.com/7792 comment 11
+    SkPath::Verb verbs11[] = { SkPath::kMove_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb,
+                               SkPath::kLine_Verb, SkPath::kLine_Verb, SkPath::kMove_Verb };
+    SkPoint points11[] = { {75, 150}, {75, 75}, {150, 75}, {150, 150}, {75, 150}, {75, 150} };
+    path = makePath2(points11, verbs11, SK_ARRAY_COUNT(verbs11));
+    REPORTER_ASSERT(reporter, path.isRect(&rect, nullptr, nullptr));
+    compare.set(&points11[0], SK_ARRAY_COUNT(points11));
+    REPORTER_ASSERT(reporter, rect == compare);
+    // isolated from skbug.com/7792 comment 14
+    SkPath::Verb verbs14[] = { SkPath::kMove_Verb, SkPath::kMove_Verb, SkPath::kMove_Verb,
+                               SkPath::kMove_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb,
+                               SkPath::kLine_Verb, SkPath::kLine_Verb, SkPath::kClose_Verb,
+                               SkPath::kLine_Verb, SkPath::kClose_Verb };
+    SkPoint points14[] = { {250, 75}, {250, 75}, {250, 75}, {100, 75},
+                           {150, 75}, {150, 150}, {75, 150}, {75, 75}, {0, 0} };
+    path = makePath2(points14, verbs14, SK_ARRAY_COUNT(verbs14));
+    REPORTER_ASSERT(reporter, path.isRect(&rect, nullptr, nullptr));
+    compare.set(&points14[3], 5);
+    REPORTER_ASSERT(reporter, rect == compare);
+    // isolated from skbug.com/7792 comment 15
+    SkPath::Verb verbs15[] = { SkPath::kMove_Verb, SkPath::kLine_Verb, SkPath::kLine_Verb,
+                               SkPath::kLine_Verb, SkPath::kMove_Verb };
+    SkPoint points15[] = { {75, 75}, {150, 75}, {150, 150}, {75, 150}, {250, 75} };
+    path = makePath2(points15, verbs15, SK_ARRAY_COUNT(verbs15));
+    REPORTER_ASSERT(reporter, path.isRect(&rect, nullptr, nullptr));
+    compare.set(&points15[0], SK_ARRAY_COUNT(points15) - 1);
+    REPORTER_ASSERT(reporter, rect == compare);
 }