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
diff --git a/src/core/SkPicture.cpp b/src/core/SkPicture.cpp
index c2c8aaf..7120f61 100644
--- a/src/core/SkPicture.cpp
+++ b/src/core/SkPicture.cpp
@@ -137,7 +137,13 @@
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)
diff --git a/src/core/SkPicturePlayback.cpp b/src/core/SkPicturePlayback.cpp
index d57e30e..3f83b6a 100644
--- a/src/core/SkPicturePlayback.cpp
+++ b/src/core/SkPicturePlayback.cpp
@@ -58,7 +58,8 @@
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 @@
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;
diff --git a/src/core/SkPicturePlayback.h b/src/core/SkPicturePlayback.h
index 91429db..9d4c996 100644
--- a/src/core/SkPicturePlayback.h
+++ b/src/core/SkPicturePlayback.h
@@ -78,7 +78,8 @@
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&,
diff --git a/src/core/SkPictureRecorder.cpp b/src/core/SkPictureRecorder.cpp
index a289f95..863dbcd 100644
--- a/src/core/SkPictureRecorder.cpp
+++ b/src/core/SkPictureRecorder.cpp
@@ -55,7 +55,8 @@
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);
diff --git a/tests/PictureTest.cpp b/tests/PictureTest.cpp
index 618ccfc..6969524 100644
--- a/tests/PictureTest.cpp
+++ b/tests/PictureTest.cpp
@@ -986,6 +986,45 @@
}
};
+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 @@
// 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) {