Fix behavior of SkPicture::kUsePathBoundsForClip_RecordingFlag to handle inverse fills and all clip ops correctly.

BUG=crbug.com/229011
TEST=Picture unit test + complexclip* GMs

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

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

src/core/SkPictureRecord.cpp

@@ -10,6 +10,7 @@
 #include "SkPixelRef.h"
 #include "SkRRect.h"
 #include "SkBBoxHierarchy.h"
+#include "SkDevice.h"
 #include "SkPictureStateTree.h"
 
 #define MIN_WRITER_SIZE 16384
@@ -755,7 +756,7 @@ bool SkPictureRecord::clipRRect(const SkRRect& rrect, SkRegion::Op op, bool doAA
     validate(initialOffset, size);
 
     if (fRecordFlags & SkPicture::kUsePathBoundsForClip_RecordingFlag) {
-        return this->INHERITED::clipRect(rrect.getBounds(), op, doAA);
+        return this->updateClipConservativelyUsingBounds(rrect.getBounds(), op, doAA, false);
     } else {
         return this->INHERITED::clipRRect(rrect, op, doAA);
     }
@@ -783,12 +784,83 @@ bool SkPictureRecord::clipPath(const SkPath& path, SkRegion::Op op, bool doAA) {
     validate(initialOffset, size);
 
     if (fRecordFlags & SkPicture::kUsePathBoundsForClip_RecordingFlag) {
-        return this->INHERITED::clipRect(path.getBounds(), op, doAA);
+        return this->updateClipConservativelyUsingBounds(path.getBounds(), op, doAA,
+                                                         path.isInverseFillType());
     } else {
         return this->INHERITED::clipPath(path, op, doAA);
     }
 }
 
+bool SkPictureRecord::updateClipConservativelyUsingBounds(const SkRect& bounds, SkRegion::Op op,
+                                                          bool doAA, bool inverseFilled) {
+    // This is for updating the clip when kUsePathBoundsForClip_RecordingFlag
+    // is set. The current clip of the recording canvas is used for quick
+    // culling of clipped-out primitives, which must not yield any false
+    // positives, while still rejecting as many as possible.
+    // Contract: 
+    //    The current clip must contain the true clip. The true
+    //    clip is the clip that would have been computed with 
+    //    kUsePathBoundsForClip_RecordingFlag disabled. 
+    // Objective:
+    //    Keep the current clip as small as possible without
+    //    breaking the contract, using only clip bounding rectangles
+    //    (for performance).
+    if (inverseFilled) {
+        switch (op) {
+            case SkRegion::kIntersect_Op:
+            case SkRegion::kDifference_Op:
+                // These ops can only shrink the current clip. So leaving
+                // the clip unchanges conservatively respects the contract.
+                return this->getClipDeviceBounds(NULL);
+            case SkRegion::kUnion_Op:
+            case SkRegion::kReplace_Op:
+            case SkRegion::kReverseDifference_Op:
+            case SkRegion::kXOR_Op:
+                {
+                    // These ops can grow the current clip up to the extents of
+                    // the input clip, which is inverse filled, so we just set
+                    // the current clip to the device bounds.
+                    SkRect deviceBounds;
+                    SkIRect deviceIBounds;
+                    this->getDevice()->getGlobalBounds(&deviceIBounds);
+                    deviceBounds = SkRect::MakeFromIRect(deviceIBounds);
+                    this->INHERITED::save(SkCanvas::kMatrix_SaveFlag);
+                    // set the clip in device space
+                    this->INHERITED::setMatrix(SkMatrix::I());
+                    bool result = this->INHERITED::clipRect(deviceBounds,
+                        SkRegion::kReplace_Op, doAA);
+                    this->INHERITED::restore(); //pop the matrix, but keep the clip
+                    return result;
+                }
+            default:
+                SkASSERT(0); // unhandled op?
+        }
+    } else {
+        // Not inverse filled
+        switch (op) {
+            case SkRegion::kIntersect_Op:
+            case SkRegion::kUnion_Op:
+            case SkRegion::kReplace_Op:
+                return this->INHERITED::clipRect(bounds, op, doAA);
+            case SkRegion::kDifference_Op:
+                // Difference can only shrink the current clip.
+                // Leaving clip unchanged conservatively fullfills the contract.
+                return this->getClipDeviceBounds(NULL);
+            case SkRegion::kReverseDifference_Op:
+                // To reverse, we swap in the bounds with a replace op.
+                // As with difference, leave it unchanged.
+                return this->INHERITED::clipRect(bounds, SkRegion::kReplace_Op, doAA);
+            case SkRegion::kXOR_Op:
+                // Be conservative, based on (A XOR B) always included in (A union B),
+                // which is always included in (bounds(A) union bounds(B))
+                return this->INHERITED::clipRect(bounds, SkRegion::kUnion_Op, doAA);
+            default:
+                SkASSERT(0); // unhandled op?
+        }
+    }    
+    return true;
+}
+
 bool SkPictureRecord::clipRegion(const SkRegion& region, SkRegion::Op op) {
     // op + region index + clip params
     uint32_t size = 3 * kUInt32Size;

src/core/SkPictureRecord.h

@@ -103,6 +103,8 @@ public:
     void endRecording();
 
 private:
+    bool updateClipConservativelyUsingBounds(const SkRect&, SkRegion::Op,
+                                             bool doAA, bool inverseFilled);
     void handleOptimization(int opt);
     void recordRestoreOffsetPlaceholder(SkRegion::Op);
     void fillRestoreOffsetPlaceholdersForCurrentStackLevel(

tests/PictureTest.cpp

@@ -449,6 +449,105 @@ static void test_clone_empty(skiatest::Reporter* reporter) {
     }
 }
 
+static void test_clip_bound_opt(skiatest::Reporter* reporter) {
+    // Test for crbug.com/229011
+    SkRect rect1 = SkRect::MakeXYWH(SkIntToScalar(4), SkIntToScalar(4),
+                                    SkIntToScalar(2), SkIntToScalar(2));
+    SkRect rect2 = SkRect::MakeXYWH(SkIntToScalar(7), SkIntToScalar(7),
+                                    SkIntToScalar(1), SkIntToScalar(1));
+    SkRect rect3 = SkRect::MakeXYWH(SkIntToScalar(6), SkIntToScalar(6),
+                                    SkIntToScalar(1), SkIntToScalar(1));
+
+    SkPath invPath;
+    invPath.addOval(rect1);
+    invPath.setFillType(SkPath::kInverseEvenOdd_FillType);
+    SkPath path;
+    path.addOval(rect2);
+    SkPath path2;
+    path2.addOval(rect3);
+    SkIRect clipBounds;
+    // Minimalist test set for 100% code coverage of
+    // SkPictureRecord::updateClipConservativelyUsingBounds
+    {
+        SkPicture picture;
+        SkCanvas* canvas = picture.beginRecording(10, 10,
+            SkPicture::kUsePathBoundsForClip_RecordingFlag);
+        canvas->clipPath(invPath, SkRegion::kIntersect_Op);
+        bool nonEmpty = canvas->getClipDeviceBounds(&clipBounds);
+        REPORTER_ASSERT(reporter, true == nonEmpty);
+        REPORTER_ASSERT(reporter, 0 == clipBounds.fLeft);
+        REPORTER_ASSERT(reporter, 0 == clipBounds.fTop);
+        REPORTER_ASSERT(reporter, 10 == clipBounds.fBottom);
+        REPORTER_ASSERT(reporter, 10 == clipBounds.fRight);
+    }
+    {
+        SkPicture picture;
+        SkCanvas* canvas = picture.beginRecording(10, 10,
+            SkPicture::kUsePathBoundsForClip_RecordingFlag);
+        canvas->clipPath(path, SkRegion::kIntersect_Op);
+        canvas->clipPath(invPath, SkRegion::kIntersect_Op);
+        bool nonEmpty = canvas->getClipDeviceBounds(&clipBounds);
+        REPORTER_ASSERT(reporter, true == nonEmpty);
+        REPORTER_ASSERT(reporter, 7 == clipBounds.fLeft);
+        REPORTER_ASSERT(reporter, 7 == clipBounds.fTop);
+        REPORTER_ASSERT(reporter, 8 == clipBounds.fBottom);
+        REPORTER_ASSERT(reporter, 8 == clipBounds.fRight);
+    }
+    {
+        SkPicture picture;
+        SkCanvas* canvas = picture.beginRecording(10, 10,
+            SkPicture::kUsePathBoundsForClip_RecordingFlag);
+        canvas->clipPath(path, SkRegion::kIntersect_Op);
+        canvas->clipPath(invPath, SkRegion::kUnion_Op);
+        bool nonEmpty = canvas->getClipDeviceBounds(&clipBounds);
+        REPORTER_ASSERT(reporter, true == nonEmpty);
+        REPORTER_ASSERT(reporter, 0 == clipBounds.fLeft);
+        REPORTER_ASSERT(reporter, 0 == clipBounds.fTop);
+        REPORTER_ASSERT(reporter, 10 == clipBounds.fBottom);
+        REPORTER_ASSERT(reporter, 10 == clipBounds.fRight);
+    }
+    {
+        SkPicture picture;
+        SkCanvas* canvas = picture.beginRecording(10, 10,
+            SkPicture::kUsePathBoundsForClip_RecordingFlag);
+        canvas->clipPath(path, SkRegion::kDifference_Op);
+        bool nonEmpty = canvas->getClipDeviceBounds(&clipBounds);
+        REPORTER_ASSERT(reporter, true == nonEmpty);
+        REPORTER_ASSERT(reporter, 0 == clipBounds.fLeft);
+        REPORTER_ASSERT(reporter, 0 == clipBounds.fTop);
+        REPORTER_ASSERT(reporter, 10 == clipBounds.fBottom);
+        REPORTER_ASSERT(reporter, 10 == clipBounds.fRight);
+    }
+    {
+        SkPicture picture;
+        SkCanvas* canvas = picture.beginRecording(10, 10,
+            SkPicture::kUsePathBoundsForClip_RecordingFlag);
+        canvas->clipPath(path, SkRegion::kReverseDifference_Op);
+        bool nonEmpty = canvas->getClipDeviceBounds(&clipBounds);
+        // True clip is actually empty in this case, but the best
+        // determination we can make using only bounds as input is that the
+        // clip is included in the bounds of 'path'.
+        REPORTER_ASSERT(reporter, true == nonEmpty);
+        REPORTER_ASSERT(reporter, 7 == clipBounds.fLeft);
+        REPORTER_ASSERT(reporter, 7 == clipBounds.fTop);
+        REPORTER_ASSERT(reporter, 8 == clipBounds.fBottom);
+        REPORTER_ASSERT(reporter, 8 == clipBounds.fRight);
+    }
+    {
+        SkPicture picture;
+        SkCanvas* canvas = picture.beginRecording(10, 10,
+            SkPicture::kUsePathBoundsForClip_RecordingFlag);
+        canvas->clipPath(path, SkRegion::kIntersect_Op);
+        canvas->clipPath(path2, SkRegion::kXOR_Op);
+        bool nonEmpty = canvas->getClipDeviceBounds(&clipBounds);
+        REPORTER_ASSERT(reporter, true == nonEmpty);
+        REPORTER_ASSERT(reporter, 6 == clipBounds.fLeft);
+        REPORTER_ASSERT(reporter, 6 == clipBounds.fTop);
+        REPORTER_ASSERT(reporter, 8 == clipBounds.fBottom);
+        REPORTER_ASSERT(reporter, 8 == clipBounds.fRight);
+    }
+}
+
 static void TestPicture(skiatest::Reporter* reporter) {
 #ifdef SK_DEBUG
     test_deleting_empty_playback();
@@ -460,6 +559,7 @@ static void TestPicture(skiatest::Reporter* reporter) {
     test_gatherpixelrefs(reporter);
     test_bitmap_with_encoded_data(reporter);
     test_clone_empty(reporter);
+    test_clip_bound_opt(reporter);
 }
 
 #include "TestClassDef.h"