Fix clip expansion in SkPictureRecord::recordRestoreOffsetPlaceholder()
For operations which can expand the region, zeroing previous clip ops' restore offsets is not enough: we need to also break the chain - otherwise the next restore() will simply traverse back and reset the skip offsets.
R=robertphillips@google.com
Review URL: https://codereview.chromium.org/22987003
git-svn-id: http://skia.googlecode.com/svn/trunk@10934 2bbb7eff-a529-9590-31e7-b0007b416f81
diff --git a/src/core/SkPictureRecord.cpp b/src/core/SkPictureRecord.cpp
index 01478bb..4134f8d 100644
--- a/src/core/SkPictureRecord.cpp
+++ b/src/core/SkPictureRecord.cpp
@@ -715,21 +715,27 @@
return;
}
+ // The RestoreOffset field is initially filled with a placeholder
+ // value that points to the offset of the previous RestoreOffset
+ // in the current stack level, thus forming a linked list so that
+ // the restore offsets can be filled in when the corresponding
+ // restore command is recorded.
+ int32_t prevOffset = fRestoreOffsetStack.top();
+
if (regionOpExpands(op)) {
// Run back through any previous clip ops, and mark their offset to
// be 0, disabling their ability to trigger a jump-to-restore, otherwise
// they could hide this clips ability to expand the clip (i.e. go from
// empty to non-empty).
fillRestoreOffsetPlaceholdersForCurrentStackLevel(0);
+
+ // Reset the pointer back to the previous clip so that subsequent
+ // restores don't overwrite the offsets we just cleared.
+ prevOffset = 0;
}
size_t offset = fWriter.size();
- // The RestoreOffset field is initially filled with a placeholder
- // value that points to the offset of the previous RestoreOffset
- // in the current stack level, thus forming a linked list so that
- // the restore offsets can be filled in when the corresponding
- // restore command is recorded.
- addInt(fRestoreOffsetStack.top());
+ addInt(prevOffset);
fRestoreOffsetStack.top() = offset;
}
diff --git a/tests/PictureTest.cpp b/tests/PictureTest.cpp
index 1827fdc..f823806 100644
--- a/tests/PictureTest.cpp
+++ b/tests/PictureTest.cpp
@@ -8,6 +8,7 @@
#include "SkCanvas.h"
#include "SkColorPriv.h"
#include "SkData.h"
+#include "SkDevice.h"
#include "SkError.h"
#include "SkPaint.h"
#include "SkPicture.h"
@@ -550,6 +551,62 @@
}
}
+/**
+ * A canvas that records the number of clip commands.
+ */
+class ClipCountingCanvas : public SkCanvas {
+public:
+ explicit ClipCountingCanvas(SkDevice* device)
+ : SkCanvas(device)
+ , fClipCount(0){
+ }
+
+ virtual bool clipRect(const SkRect& r, SkRegion::Op op, bool doAA)
+ SK_OVERRIDE {
+ fClipCount += 1;
+ return this->INHERITED::clipRect(r, op, doAA);
+ }
+
+ virtual bool clipRRect(const SkRRect& rrect, SkRegion::Op op, bool doAA)
+ SK_OVERRIDE {
+ fClipCount += 1;
+ return this->INHERITED::clipRRect(rrect, op, doAA);
+ }
+
+ virtual bool clipPath(const SkPath& path, SkRegion::Op op, bool doAA)
+ SK_OVERRIDE {
+ fClipCount += 1;
+ return this->INHERITED::clipPath(path, op, doAA);
+ }
+
+ unsigned getClipCount() const { return fClipCount; }
+
+private:
+ unsigned fClipCount;
+
+ typedef SkCanvas INHERITED;
+};
+
+static void test_clip_expansion(skiatest::Reporter* reporter) {
+ SkPicture picture;
+ SkCanvas* canvas = picture.beginRecording(10, 10, 0);
+
+ canvas->clipRect(SkRect::MakeEmpty(), SkRegion::kReplace_Op);
+ // The following expanding clip should not be skipped.
+ canvas->clipRect(SkRect::MakeXYWH(4, 4, 3, 3), SkRegion::kUnion_Op);
+ // Draw something so the optimizer doesn't just fold the world.
+ SkPaint p;
+ p.setColor(SK_ColorBLUE);
+ canvas->drawPaint(p);
+
+ SkDevice testDevice(SkBitmap::kNo_Config, 10, 10);
+ ClipCountingCanvas testCanvas(&testDevice);
+ picture.draw(&testCanvas);
+
+ // Both clips should be present on playback.
+ REPORTER_ASSERT(reporter, testCanvas.getClipCount() == 2);
+}
+
static void TestPicture(skiatest::Reporter* reporter) {
#ifdef SK_DEBUG
test_deleting_empty_playback();
@@ -562,6 +619,7 @@
test_bitmap_with_encoded_data(reporter);
test_clone_empty(reporter);
test_clip_bound_opt(reporter);
+ test_clip_expansion(reporter);
}
#include "TestClassDef.h"