Fix error revealed by Android unit test

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.

Committed: https://skia.googlesource.com/skia/+/9058d602d0303c4d601bea260929367a8be37f04

[reed1, 2014-06-10 12:29:03]

lgtm w/ crazy suggestions

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicture.cpp#newco...
src/core/SkPicture.cpp:144: // will occur). In this case just force a deep copy
of the operations.
just a suggestion to help document...

const bool deepCopy = true;
return SkNEW.... (..., deepCopy));

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:112: fOpData =
SkData::NewWithCopy(writer.contiguousArray(), writer.bytesWritten());
I presume this continuousArray() call can mutate the writer.

It does seem like we could perform this nearly as efficiently w/o having to do
that... (just an idea)

len = writer.len;
block = malloc(len)
writer.copyInto(block, len);

Now the writer can walk a link-list (as needed) to copy into block

data = SkData::NewWithMalloc(block, len);

https://codereview.chromium.org/316063005/diff/1/src/core/SkPictureRecorder.cpp
File src/core/SkPictureRecorder.cpp (right):

https://codereview.chromium.org/316063005/diff/1/src/core/SkPictureRecorder.c...
src/core/SkPictureRecorder.cpp:57: fPicture->createHeader(&info);
? const bool deepCopyOps = false;
...

[scroggo, 2014-06-10 13:31:25]

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicture.cpp#newco...
src/core/SkPicture.cpp:141: // FakeEndRecording is only called from
partialReplay. For that use case
Originally it had multiple callers. Does it make sense to move this code inside
partialReplay?

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (left):

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:138: fPictureRefs[i] = pictures[i]->clone();
Why this change? deepCopy is used when we are cloning so we can have thread safe
pictures. If each of those pictures points to the same subpicture, we are no
longer threadsafe.

(I do think we're close to a place where an SkPicture is threadsafe, but as far
as I'm aware, we are not there yet.)

https://codereview.chromium.org/316063005/diff/1/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/316063005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:1001: static void check_balance(skiatest::Reporter*
reporter, SkPicture* picture) {
Some comments here would be nice. e.g.

// Test that playing an SkPicture into an SkCanvas does not modify the 
// SkCanvas' save count.

https://codereview.chromium.org/316063005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:1020: REPORTER_ASSERT(reporter, afterClip.height() == 4);
How did the height end up 4?

https://codereview.chromium.org/316063005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:1092: SkAutoTUnref<SkPicture>
final(recorder.endRecording());
What does this test?

[robertphillips, 2014-06-10 17:26:28]

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicture.cpp#newco...
src/core/SkPicture.cpp:141: // FakeEndRecording is only called from
partialReplay. For that use case
It will go away entirely in a later CL.

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicture.cpp#newco...
src/core/SkPicture.cpp:144: // will occur). In this case just force a deep copy
of the operations.
On 2014/06/10 12:29:03, reed1 wrote:
> just a suggestion to help document...
> 
> const bool deepCopy = true;
> return SkNEW.... (..., deepCopy));

Done.

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (left):

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:138: fPictureRefs[i] = pictures[i]->clone();
TL;DR - this change has been split off into: (Remove unused 'deepCopy' parameter
- https://codereview.chromium.org/324093003/)

The 'deepCopy' flag is/was unused in the new world of picture recording. The
SkPicturePlayback copy constructor is used when creating thread-safe pictures.
This constructor is only used by SkPictureRecorder when "finalizing" an
SkPicture so it need not be concerned about thread-safety issues.

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/316063005/diff/1/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:112: fOpData =
SkData::NewWithCopy(writer.contiguousArray(), writer.bytesWritten());
Right now SkWriter32 does store its data contiguously internally (so this call
isn't creating havoc).

I think, in the distant future where SkData doesn't need to be contiguous, we
would need something like SkWriter32::deepCopyAsData.

I would prefer to leave these changes to future generations.

https://codereview.chromium.org/316063005/diff/1/src/core/SkPictureRecorder.cpp
File src/core/SkPictureRecorder.cpp (right):

https://codereview.chromium.org/316063005/diff/1/src/core/SkPictureRecorder.c...
src/core/SkPictureRecorder.cpp:57: fPicture->createHeader(&info);
On 2014/06/10 12:29:03, reed1 wrote:
> ? const bool deepCopyOps = false;
> ...

Done.

https://codereview.chromium.org/316063005/diff/1/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/316063005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:1001: static void check_balance(skiatest::Reporter*
reporter, SkPicture* picture) {
On 2014/06/10 13:31:25, scroggo wrote:
> Some comments here would be nice. e.g.
> 
> // Test that playing an SkPicture into an SkCanvas does not modify the 
> // SkCanvas' save count.

Done.

https://codereview.chromium.org/316063005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:1092: SkAutoTUnref<SkPicture>
final(recorder.endRecording());
On 2014/06/10 13:31:25, scroggo wrote:
> What does this test?

Done - added comment.

[robertphillips, 2014-06-10 17:27:24]

https://codereview.chromium.org/316063005/diff/1/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/316063005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:1020: REPORTER_ASSERT(reporter, afterClip.height() == 4);
Switch to storing a beforeClip so things are clearer.

[scroggo, 2014-06-10 17:39:40]

lgtm

https://codereview.chromium.org/316063005/diff/20001/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/316063005/diff/20001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1002: // doesn't effect the canvas' save count or
matrix/clip state.
Shouldn't this be "affect"?

[robertphillips, 2014-06-10 17:43:06]

https://codereview.chromium.org/316063005/diff/20001/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/316063005/diff/20001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1002: // doesn't effect the canvas' save count or
matrix/clip state.
On 2014/06/10 17:39:40, scroggo wrote:
> Shouldn't this be "affect"?

Done.

[robertphillips, 2014-06-10 17:44:46]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-06-10 17:45:12]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/robertphillips@google.com/316063005/4...

[commit-bot: I haz the power, 2014-06-10 18:45:53]

Change committed as 9058d602d0303c4d601bea260929367a8be37f04