From 9058d602d0303c4d601bea260929367a8be37f04 Mon Sep 17 00:00:00 2001 From: robertphillips Date: Tue, 10 Jun 2014 11:45:46 -0700 Subject: [PATCH] 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 | 8 ++++- src/core/SkPicturePlayback.cpp | 10 ++++-- src/core/SkPicturePlayback.h | 3 +- src/core/SkPictureRecorder.cpp | 3 +- tests/PictureTest.cpp | 58 ++++++++++++++++++++++++++++++++++ 5 files changed, 77 insertions(+), 5 deletions(-) diff --git a/src/core/SkPicture.cpp b/src/core/SkPicture.cpp index c2c8aaf572..7120f61179 100644 --- a/src/core/SkPicture.cpp +++ b/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) diff --git a/src/core/SkPicturePlayback.cpp b/src/core/SkPicturePlayback.cpp index d57e30eeb1..3f83b6a9cc 100644 --- a/src/core/SkPicturePlayback.cpp +++ b/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; diff --git a/src/core/SkPicturePlayback.h b/src/core/SkPicturePlayback.h index 91429dbd07..9d4c996653 100644 --- a/src/core/SkPicturePlayback.h +++ b/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&, diff --git a/src/core/SkPictureRecorder.cpp b/src/core/SkPictureRecorder.cpp index a289f956fd..863dbcda90 100644 --- a/src/core/SkPictureRecorder.cpp +++ b/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); diff --git a/tests/PictureTest.cpp b/tests/PictureTest.cpp index 618ccfc4c0..6969524bd7 100644 --- a/tests/PictureTest.cpp +++ b/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 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 final(recorder.endRecording()); + } } static void test_unbalanced_save_restores(skiatest::Reporter* reporter) {