Added check for aa/bw rect merging

http://codereview.appspot.com/6449079/

git-svn-id: http://skia.googlecode.com/svn/trunk@4907 2bbb7eff-a529-9590-31e7-b0007b416f81

diff --git a/src/core/SkClipStack.cpp b/src/core/SkClipStack.cpp
index e9a02ec..38856e5 100644 (file)
--- a/src/core/SkClipStack.cpp
+++ b/src/core/SkClipStack.cpp
@@ -58,6 +58,13 @@ struct SkClipStack::Rec {
         // bounding box members are updated in a following updateBound call
     }
 
+    void setEmpty() {
+        fState = kEmpty_State;
+        fFiniteBound.setEmpty();
+        fFiniteBoundType = kNormal_BoundsType;
+        fIsIntersectionOfRects = false;
+    }
+
     bool operator==(const Rec& b) const {
         if (fSaveCount != b.fSaveCount || fOp != b.fOp || fState != b.fState ||
                 fDoAA != b.fDoAA) {
@@ -82,17 +89,50 @@ struct SkClipStack::Rec {
     /**
      *  Returns true if this Rec can be intersected in place with a new clip
      */
-    bool canBeIntersected(int saveCount, SkRegion::Op op) const {
+    bool canBeIntersectedInPlace(int saveCount, SkRegion::Op op) const {
         if (kEmpty_State == fState && (
                     SkRegion::kDifference_Op == op ||
                     SkRegion::kIntersect_Op == op)) {
             return true;
         }
-        return  fSaveCount == saveCount &&
-                SkRegion::kIntersect_Op == fOp &&
-                SkRegion::kIntersect_Op == op;
+        return  fSaveCount == saveCount && 
+                SkRegion::kIntersect_Op == op &&
+                (SkRegion::kIntersect_Op == fOp || SkRegion::kReplace_Op == fOp);
+    }
+
+    /**
+     * This method checks to see if two rect clips can be safely merged into
+     * one. The issue here is that to be strictly correct all the edges of
+     * the resulting rect must have the same anti-aliasing.
+     */
+    bool rectRectIntersectAllowed(const SkRect& newR, bool newAA) const {
+        SkASSERT(kRect_State == fState);
+
+        if (fDoAA == newAA) {
+            // if the AA setting is the same there is no issue
+            return true;
+        }
+
+        if (!SkRect::Intersects(fRect, newR)) {
+            // The calling code will correctly set the result to the empty clip
+            return true;
+        }
+
+        if (fRect.contains(newR)) {
+            // if the new rect carves out a portion of the old one there is no
+            // issue
+            return true;
+        }
+
+        // So either the two overlap in some complex manner or newR contains oldR.
+        // In the first, case the edges will require different AA. In the second,
+        // the AA setting that would be carried forward is incorrect (e.g., oldR 
+        // is AA while newR is BW but since newR contains oldR, oldR will be 
+        // drawn BW) since the new AA setting will predominate.
+        return false;
     }
 
+
     /**
      * The different combination of fill & inverse fill when combining
      * bounding boxes
@@ -283,7 +323,8 @@ struct SkClipStack::Rec {
 
             if (SkRegion::kReplace_Op == fOp ||
                 (SkRegion::kIntersect_Op == fOp && NULL == prior) || 
-                (SkRegion::kIntersect_Op == fOp && prior->fIsIntersectionOfRects)) {
+                (SkRegion::kIntersect_Op == fOp && prior->fIsIntersectionOfRects &&
+                 prior->rectRectIntersectAllowed(fRect, fDoAA))) {
                 fIsIntersectionOfRects = true;
             }
 
@@ -486,32 +527,29 @@ void SkClipStack::clipDevRect(const SkRect& rect, SkRegion::Op op, bool doAA) {
     SkDeque::Iter iter(fDeque, SkDeque::Iter::kBack_IterStart);
     Rec* rec = (Rec*) iter.prev();
 
-    if (rec && rec->canBeIntersected(fSaveCount, op)) {
+    if (rec && rec->canBeIntersectedInPlace(fSaveCount, op)) {
         switch (rec->fState) {
             case Rec::kEmpty_State:
                 SkASSERT(rec->fFiniteBound.isEmpty());
                 SkASSERT(kNormal_BoundsType == rec->fFiniteBoundType);
                 SkASSERT(!rec->fIsIntersectionOfRects);
                 return;
-            case Rec::kRect_State: {
-                if (!rec->fRect.intersect(rect)) {
-                    rec->fState = Rec::kEmpty_State;
-                    rec->fFiniteBound.setEmpty();
-                    rec->fFiniteBoundType = kNormal_BoundsType;
-                    rec->fIsIntersectionOfRects = false;
+            case Rec::kRect_State:
+                if (rec->rectRectIntersectAllowed(rect, doAA)) {
+                    if (!rec->fRect.intersect(rect)) {
+                        rec->setEmpty();
+                        return;
+                    }
+
+                    rec->fDoAA = doAA;
+                    Rec* prev = (Rec*) iter.prev();
+                    rec->updateBound(prev);
                     return;
                 }
-
-                Rec* prev = (Rec*) iter.prev();
-                rec->updateBound(prev);
-                return;
-            }
+                break;
             case Rec::kPath_State:
                 if (!SkRect::Intersects(rec->fPath.getBounds(), rect)) {
-                    rec->fState = Rec::kEmpty_State;
-                    rec->fFiniteBound.setEmpty();
-                    rec->fFiniteBoundType = kNormal_BoundsType;
-                    rec->fIsIntersectionOfRects = false;
+                    rec->setEmpty();
                     return;
                 }
                 break;
@@ -527,7 +565,7 @@ void SkClipStack::clipDevPath(const SkPath& path, SkRegion::Op op, bool doAA) {
         return this->clipDevRect(alt, op, doAA);
     }
     Rec* rec = (Rec*)fDeque.back();
-    if (rec && rec->canBeIntersected(fSaveCount, op)) {
+    if (rec && rec->canBeIntersectedInPlace(fSaveCount, op)) {
         const SkRect& pathBounds = path.getBounds();
         switch (rec->fState) {
             case Rec::kEmpty_State:
@@ -537,19 +575,13 @@ void SkClipStack::clipDevPath(const SkPath& path, SkRegion::Op op, bool doAA) {
                 return;
             case Rec::kRect_State:
                 if (!SkRect::Intersects(rec->fRect, pathBounds)) {
-                    rec->fState = Rec::kEmpty_State;
-                    rec->fFiniteBound.setEmpty();
-                    rec->fFiniteBoundType = kNormal_BoundsType;
-                    rec->fIsIntersectionOfRects = false;
+                    rec->setEmpty();
                     return;
                 }
                 break;
             case Rec::kPath_State:
                 if (!SkRect::Intersects(rec->fPath.getBounds(), pathBounds)) {
-                    rec->fState = Rec::kEmpty_State;
-                    rec->fFiniteBound.setEmpty();
-                    rec->fFiniteBoundType = kNormal_BoundsType;
-                    rec->fIsIntersectionOfRects = false;
+                    rec->setEmpty();
                     return;
                 }
                 break;

diff --git a/tests/ClipStackTest.cpp b/tests/ClipStackTest.cpp
index afbbd7e..1def9b4 100644 (file)
--- a/tests/ClipStackTest.cpp
+++ b/tests/ClipStackTest.cpp
@@ -127,6 +127,8 @@ static void assert_count(skiatest::Reporter* reporter, const SkClipStack& stack,
     REPORTER_ASSERT(reporter, count == counter);
 }
 
+// Exercise the SkClipStack's bottom to top and bidirectional iterators
+// (including the skipToTopmost functionality)
 static void test_iterators(skiatest::Reporter* reporter) {
     SkClipStack stack;
 
@@ -183,6 +185,7 @@ static void test_iterators(skiatest::Reporter* reporter) {
     }
 }
 
+// Exercise the SkClipStack's getConservativeBounds computation
 static void test_bounds(skiatest::Reporter* reporter, bool useRects) {
 
     static const int gNumCases = 20;
@@ -351,6 +354,124 @@ static void test_isWideOpen(skiatest::Reporter* reporter) {
     }
 }
 
+int count(const SkClipStack& stack) {
+
+    SkClipStack::Iter iter(stack, SkClipStack::Iter::kTop_IterStart);
+
+    const SkClipStack::Iter::Clip* clip = NULL;
+    int count = 0;
+
+    for (clip = iter.prev(); clip; clip = iter.prev(), ++count) {
+        ;
+    }
+
+    return count;
+}
+
+// Test out SkClipStack's merging of rect clips. In particular exercise
+// merging of aa vs. bw rects.
+static void test_rect_merging(skiatest::Reporter* reporter) {
+
+    SkRect overlapLeft  = SkRect::MakeLTRB(10, 10, 50, 50);
+    SkRect overlapRight = SkRect::MakeLTRB(40, 40, 80, 80);
+
+    SkRect nestedParent = SkRect::MakeLTRB(10, 10, 90, 90);
+    SkRect nestedChild  = SkRect::MakeLTRB(40, 40, 60, 60);
+
+    SkRect bound;
+    SkClipStack::BoundsType type;
+    bool isIntersectionOfRects;
+
+    // all bw overlapping - should merge
+    {
+        SkClipStack stack;
+
+        stack.clipDevRect(overlapLeft, SkRegion::kReplace_Op, false);
+
+        stack.clipDevRect(overlapRight, SkRegion::kIntersect_Op, false);
+
+        REPORTER_ASSERT(reporter, 1 == count(stack));
+
+        stack.getBounds(&bound, &type, &isIntersectionOfRects);
+
+        REPORTER_ASSERT(reporter, isIntersectionOfRects);
+    }
+
+    // all aa overlapping - should merge
+    {
+        SkClipStack stack;
+
+        stack.clipDevRect(overlapLeft, SkRegion::kReplace_Op, true);
+
+        stack.clipDevRect(overlapRight, SkRegion::kIntersect_Op, true);
+
+        REPORTER_ASSERT(reporter, 1 == count(stack));
+
+        stack.getBounds(&bound, &type, &isIntersectionOfRects);
+
+        REPORTER_ASSERT(reporter, isIntersectionOfRects);
+    }
+
+    // mixed overlapping - should _not_ merge
+    {
+        SkClipStack stack;
+
+        stack.clipDevRect(overlapLeft, SkRegion::kReplace_Op, true);
+
+        stack.clipDevRect(overlapRight, SkRegion::kIntersect_Op, false);
+
+        REPORTER_ASSERT(reporter, 2 == count(stack));
+
+        stack.getBounds(&bound, &type, &isIntersectionOfRects);
+
+        REPORTER_ASSERT(reporter, !isIntersectionOfRects);
+    }
+
+    // mixed nested (bw inside aa) - should merge
+    {
+        SkClipStack stack;
+
+        stack.clipDevRect(nestedParent, SkRegion::kReplace_Op, true);
+
+        stack.clipDevRect(nestedChild, SkRegion::kIntersect_Op, false);
+
+        REPORTER_ASSERT(reporter, 1 == count(stack));
+
+        stack.getBounds(&bound, &type, &isIntersectionOfRects);
+
+        REPORTER_ASSERT(reporter, isIntersectionOfRects);
+    }
+
+    // mixed nested (aa inside bw) - should merge
+    {
+        SkClipStack stack;
+
+        stack.clipDevRect(nestedParent, SkRegion::kReplace_Op, false);
+
+        stack.clipDevRect(nestedChild, SkRegion::kIntersect_Op, true);
+
+        REPORTER_ASSERT(reporter, 1 == count(stack));
+
+        stack.getBounds(&bound, &type, &isIntersectionOfRects);
+
+        REPORTER_ASSERT(reporter, isIntersectionOfRects);
+    }
+
+    // reverse nested (aa inside bw) - should _not_ merge
+    {
+        SkClipStack stack;
+
+        stack.clipDevRect(nestedChild, SkRegion::kReplace_Op, false);
+
+        stack.clipDevRect(nestedParent, SkRegion::kIntersect_Op, true);
+
+        REPORTER_ASSERT(reporter, 2 == count(stack));
+
+        stack.getBounds(&bound, &type, &isIntersectionOfRects);
+
+        REPORTER_ASSERT(reporter, !isIntersectionOfRects);
+    }
+}
 
 static void TestClipStack(skiatest::Reporter* reporter) {
     SkClipStack stack;
@@ -388,9 +509,10 @@ static void TestClipStack(skiatest::Reporter* reporter) {
 
     test_assign_and_comparison(reporter);
     test_iterators(reporter);
-    test_bounds(reporter, true);
-    test_bounds(reporter, false);
+    test_bounds(reporter, true);        // once with rects
+    test_bounds(reporter, false);       // once with paths
     test_isWideOpen(reporter);
+    test_rect_merging(reporter);
 }
 
 #include "TestClassDef.h"