Fix SkClipStack bug with inverse-filled difference elements

Previously, SkClipStack would call "setEmpty" on itself when an
inverse-filled difference element made the stack empty. This was
a problem because setEmpty would forget the element had an inverse
fill, yet leave the op as "difference". This change modifies it to
manually update the clip bounds and set the gen-ID to kEmptyGenID,
rather than calling setEmpty.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2175493002

Review-Url: https://codereview.chromium.org/2175493002

diff --git a/src/core/SkClipStack.cpp b/src/core/SkClipStack.cpp
index e39aeee..06418b4 100644 (file)
--- a/src/core/SkClipStack.cpp
+++ b/src/core/SkClipStack.cpp
@@ -237,10 +237,10 @@ void SkClipStack::Element::combineBoundsDiff(FillCombo combination, const SkRect
             // is erased, so the only pixels that can remain set
             // occur w/in the intersection of the two finite bounds
             if (!fFiniteBound.intersect(prevFinite)) {
-                this->setEmpty();
-            } else {
-                fFiniteBoundType = kNormal_BoundsType;
+                fFiniteBound.setEmpty();
+                fGenID = kEmptyGenID;
             }
+            fFiniteBoundType = kNormal_BoundsType;
             break;
         case kPrev_Cur_FillCombo:
             // The most conservative result bound is that of the

diff --git a/tests/ClipStackTest.cpp b/tests/ClipStackTest.cpp
index 27753c4..4d375ee 100644 (file)
--- a/tests/ClipStackTest.cpp
+++ b/tests/ClipStackTest.cpp
@@ -794,6 +794,51 @@ static void test_quickContains(skiatest::Reporter* reporter) {
     }
 }
 
+static void set_region_to_stack(const SkClipStack& stack, const SkIRect& bounds, SkRegion* region) {
+    region->setRect(bounds);
+    SkClipStack::Iter iter(stack, SkClipStack::Iter::kBottom_IterStart);
+    while (const SkClipStack::Element *element = iter.next()) {
+        SkRegion elemRegion;
+        SkRegion boundsRgn(bounds);
+        SkPath path;
+
+        switch (element->getType()) {
+            case SkClipStack::Element::kEmpty_Type:
+                elemRegion.setEmpty();
+                break;
+            default:
+                element->asPath(&path);
+                elemRegion.setPath(path, boundsRgn);
+                break;
+        }
+        region->op(elemRegion, element->getOp());
+    }
+}
+
+static void test_invfill_diff_bug(skiatest::Reporter* reporter) {
+    SkClipStack stack;
+    stack.clipDevRect({10, 10, 20, 20}, SkRegion::kIntersect_Op, false);
+
+    SkPath path;
+    path.addRect({30, 10, 40, 20});
+    path.setFillType(SkPath::kInverseWinding_FillType);
+    stack.clipDevPath(path, SkRegion::kDifference_Op, false);
+
+    REPORTER_ASSERT(reporter, SkClipStack::kEmptyGenID == stack.getTopmostGenID());
+
+    SkRect stackBounds;
+    SkClipStack::BoundsType stackBoundsType;
+    stack.getBounds(&stackBounds, &stackBoundsType);
+
+    REPORTER_ASSERT(reporter, stackBounds.isEmpty());
+    REPORTER_ASSERT(reporter, SkClipStack::kNormal_BoundsType == stackBoundsType);
+
+    SkRegion region;
+    set_region_to_stack(stack, {0, 0, 50, 30}, &region);
+
+    REPORTER_ASSERT(reporter, region.isEmpty());
+}
+
 ///////////////////////////////////////////////////////////////////////////////////////////////////
 
 #if SK_SUPPORT_GPU
@@ -859,25 +904,6 @@ static void add_elem_to_stack(const SkClipStack::Element& element, SkClipStack*
     }
 }
 
-static void add_elem_to_region(const SkClipStack::Element& element,
-                               const SkIRect& bounds,
-                               SkRegion* region) {
-    SkRegion elemRegion;
-    SkRegion boundsRgn(bounds);
-    SkPath path;
-
-    switch (element.getType()) {
-        case SkClipStack::Element::kEmpty_Type:
-            elemRegion.setEmpty();
-            break;
-        default:
-            element.asPath(&path);
-            elemRegion.setPath(path, boundsRgn);
-            break;
-    }
-    region->op(elemRegion, element.getOp());
-}
-
 static void test_reduced_clip_stack(skiatest::Reporter* reporter) {
     // We construct random clip stacks, reduce them, and then rasterize both versions to verify that
     // they are equal.
@@ -991,20 +1017,11 @@ static void test_reduced_clip_stack(skiatest::Reporter* reporter) {
 
         // convert both the original stack and reduced stack to SkRegions and see if they're equal
         SkRegion region;
-        SkRegion reducedRegion;
+        set_region_to_stack(stack, inflatedIBounds, &region);
 
-        region.setRect(inflatedIBounds);
-        const SkClipStack::Element* element;
-        SkClipStack::Iter iter(stack, SkClipStack::Iter::kBottom_IterStart);
-        while ((element = iter.next())) {
-            add_elem_to_region(*element, inflatedIBounds, &region);
-        }
+        SkRegion reducedRegion;
+        set_region_to_stack(reducedStack, inflatedIBounds, &reducedRegion);
 
-        reducedRegion.setRect(inflatedIBounds);
-        iter.reset(reducedStack, SkClipStack::Iter::kBottom_IterStart);
-        while ((element = iter.next())) {
-            add_elem_to_region(*element, inflatedIBounds, &reducedRegion);
-        }
         SkString testCase;
         testCase.printf("Iteration %d", i);
         REPORTER_ASSERT_MESSAGE(reporter, region == reducedRegion, testCase.c_str());
@@ -1204,6 +1221,7 @@ DEF_TEST(ClipStack, reporter) {
     test_rect_inverse_fill(reporter);
     test_path_replace(reporter);
     test_quickContains(reporter);
+    test_invfill_diff_bug(reporter);
 #if SK_SUPPORT_GPU
     test_reduced_clip_stack(reporter);
     test_reduced_clip_stack_genid(reporter);