Fix error revealed by Android unit test

The issue is/was that the original Picture/PictureRecorder that is being partially replayed is not guaranteed to issue any more commands before attempting to modify the existing data. Such modification is prohibited if there is a extant copy-on-write snapshot. Rather then further complicate the SkWriter32::snapshot capability for a dis-preferred use case, this CL simply copies the operation data.

R=scroggo@google.com, reed@google.com

Author: robertphillips@google.com

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

src/core/SkPicture.cpp

@@ -137,7 +137,13 @@ SkPicturePlayback* SkPicture::FakeEndRecording(const SkPicture* resourceSrc,
                                                const SkPictureRecord& record) {
     SkPictInfo info;
     resourceSrc->createHeader(&info);
-    return SkNEW_ARGS(SkPicturePlayback, (resourceSrc, record, info));
+
+    // FakeEndRecording is only called from partialReplay. For that use case
+    // we cannot be certain that the next call to SkWriter32::overwriteTAt
+    // will be preceded by an append (i.e., that the required copy on write
+    // will occur). In this case just force a deep copy of the operations.
+    const bool deepCopyOps = true;
+    return SkNEW_ARGS(SkPicturePlayback, (resourceSrc, record, info, deepCopyOps));
 }
 
 SkPicture::SkPicture(const SkPicture& src)

src/core/SkPicturePlayback.cpp

@@ -58,7 +58,8 @@ SkPicturePlayback::SkPicturePlayback(const SkPicture* picture, const SkPictInfo&
 
 SkPicturePlayback::SkPicturePlayback(const SkPicture* picture,
                                      const SkPictureRecord& record,
-                                     const SkPictInfo& info)
+                                     const SkPictInfo& info,
+                                     bool deepCopyOps)
     : fPicture(picture)
     , fInfo(info) {
 #ifdef SK_DEBUG_SIZE
@@ -106,7 +107,12 @@ SkPicturePlayback::SkPicturePlayback(const SkPicture* picture,
         fOpData = SkData::NewEmpty();
         return;
     }
-    fOpData = writer.snapshotAsData();
+    if (deepCopyOps) {
+        // Don't try to do anything clever w.r.t. copy on write
+        fOpData = SkData::NewWithCopy(writer.contiguousArray(), writer.bytesWritten());
+    } else {
+        fOpData = writer.snapshotAsData();
+    }
 
     fBoundingHierarchy = record.fBoundingHierarchy;
     fStateTree = record.fStateTree;

src/core/SkPicturePlayback.h

@@ -78,7 +78,8 @@ class SkPicturePlayback {
 public:
     SkPicturePlayback(const SkPicture* picture, const SkPicturePlayback& src,
                       SkPictCopyInfo* deepCopyInfo = NULL);
-    SkPicturePlayback(const SkPicture* picture, const SkPictureRecord& record, const SkPictInfo&);
+    SkPicturePlayback(const SkPicture* picture, const SkPictureRecord& record, const SkPictInfo&,
+                      bool deepCopyOps);
     static SkPicturePlayback* CreateFromStream(SkPicture* picture,
                                                SkStream*,
                                                const SkPictInfo&,

src/core/SkPictureRecorder.cpp

@@ -55,7 +55,8 @@ SkPicture* SkPictureRecorder::endRecording() {
 
     SkPictInfo info;
     fPicture->createHeader(&info);
-    fPicture->fPlayback = SkNEW_ARGS(SkPicturePlayback, (fPicture, *fCanvas, info));
+    const bool deepCopyOps = false;
+    fPicture->fPlayback = SkNEW_ARGS(SkPicturePlayback, (fPicture, *fCanvas, info, deepCopyOps));
 
     SkSafeSetNull(fCanvas);
 

tests/PictureTest.cpp

@@ -986,6 +986,45 @@ public:
     }
 };
 
+static void create_imbalance(SkCanvas* canvas) {
+    SkRect clipRect = SkRect::MakeWH(2, 2);
+    SkRect drawRect = SkRect::MakeWH(10, 10);
+    canvas->save();
+        canvas->clipRect(clipRect, SkRegion::kReplace_Op);
+        canvas->translate(1.0f, 1.0f);
+        SkPaint p;
+        p.setColor(SK_ColorGREEN);
+        canvas->drawRect(drawRect, p);
+    // no restore
+}
+
+// This tests that replaying a potentially unbalanced picture into a canvas
+// doesn't affect the canvas' save count or matrix/clip state.
+static void check_balance(skiatest::Reporter* reporter, SkPicture* picture) {
+    SkBitmap bm;
+    bm.allocN32Pixels(4, 3);
+    SkCanvas canvas(bm);
+
+    int beforeSaveCount = canvas.getSaveCount();
+
+    SkMatrix beforeMatrix = canvas.getTotalMatrix();
+
+    SkRect beforeClip;
+
+    canvas.getClipBounds(&beforeClip);
+
+    canvas.drawPicture(picture);
+
+    REPORTER_ASSERT(reporter, beforeSaveCount == canvas.getSaveCount());
+    REPORTER_ASSERT(reporter, beforeMatrix == canvas.getTotalMatrix());
+
+    SkRect afterClip;
+
+    canvas.getClipBounds(&afterClip);
+
+    REPORTER_ASSERT(reporter, afterClip == beforeClip);
+}
+
 // Test out SkPictureRecorder::partialReplay
 DEF_TEST(PictureRecorder_replay, reporter) {
     // check save/saveLayer state
@@ -1040,6 +1079,25 @@ DEF_TEST(PictureRecorder_replay, reporter) {
         // The snapshot shouldn't pick up any operations added after it was made
         REPORTER_ASSERT(reporter, !copy->willPlayBackBitmaps());
     }
+
+    // Recreate the Android partialReplay test case
+    {
+        SkPictureRecorder recorder;
+
+        SkCanvas* canvas = recorder.beginRecording(4, 3, NULL, 0);
+        create_imbalance(canvas);
+
+        int expectedSaveCount = canvas->getSaveCount();
+
+        SkAutoTUnref<SkPicture> copy(SkPictureRecorderReplayTester::Copy(&recorder));
+        check_balance(reporter, copy);
+
+        REPORTER_ASSERT(reporter, expectedSaveCount = canvas->getSaveCount());
+
+        // End the recording of source to test the picture finalization
+        // process isn't complicated by the partialReplay step
+        SkAutoTUnref<SkPicture> final(recorder.endRecording());
+    }
 }
 
 static void test_unbalanced_save_restores(skiatest::Reporter* reporter) {