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
diff --git a/src/core/SkPictureRecord.cpp b/src/core/SkPictureRecord.cpp
index 375bf42..f1e3b41 100644
--- a/src/core/SkPictureRecord.cpp
+++ b/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 @@
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 @@
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;
diff --git a/src/core/SkPictureRecord.h b/src/core/SkPictureRecord.h
index 88ff302..6245b78 100644
--- a/src/core/SkPictureRecord.h
+++ b/src/core/SkPictureRecord.h
@@ -103,6 +103,8 @@
void endRecording();
private:
+ bool updateClipConservativelyUsingBounds(const SkRect&, SkRegion::Op,
+ bool doAA, bool inverseFilled);
void handleOptimization(int opt);
void recordRestoreOffsetPlaceholder(SkRegion::Op);
void fillRestoreOffsetPlaceholdersForCurrentStackLevel(
diff --git a/tests/PictureTest.cpp b/tests/PictureTest.cpp
index c702e94..20226e7 100644
--- a/tests/PictureTest.cpp
+++ b/tests/PictureTest.cpp
@@ -449,6 +449,105 @@
}
}
+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 @@
test_gatherpixelrefs(reporter);
test_bitmap_with_encoded_data(reporter);
test_clone_empty(reporter);
+ test_clip_bound_opt(reporter);
}
#include "TestClassDef.h"