From d0f1a4fb28057de42d116005d82166b2302d28e9 Mon Sep 17 00:00:00 2001
From: "fmalita@google.com"
 <fmalita@google.com@2bbb7eff-a529-9590-31e7-b0007b416f81>
Date: Tue, 27 Aug 2013 15:50:19 +0000
Subject: [PATCH] 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
---
 src/core/SkPictureRecord.cpp | 18 +++++++++-----
 tests/PictureTest.cpp        | 58 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 6 deletions(-)

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 @@ void SkPictureRecord::recordRestoreOffsetPlaceholder(SkRegion::Op op) {
         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 @@ static void test_clip_bound_opt(skiatest::Reporter* reporter) {
     }
 }
 
+/**
+ * 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 @@ static void TestPicture(skiatest::Reporter* reporter) {
     test_bitmap_with_encoded_data(reporter);
     test_clone_empty(reporter);
     test_clip_bound_opt(reporter);
+    test_clip_expansion(reporter);
 }
 
 #include "TestClassDef.h"
-- 
2.7.4