Fix error revealed by Android unit test

src/core/SkPicturePlayback.cpp

Index: src/core/SkPicturePlayback.cpp
diff --git a/src/core/SkPicturePlayback.cpp b/src/core/SkPicturePlayback.cpp
index 63f2d7e8f93b943d4ed8fc4d5d6b9304a4ba0117..3f83b6a9cccc7080ba3dd36555141e164e7b0bbd 100644
--- a/src/core/SkPicturePlayback.cpp
+++ b/src/core/SkPicturePlayback.cpp
@@ -59,7 +59,7 @@ SkPicturePlayback::SkPicturePlayback(const SkPicture* picture, const SkPictInfo&
 SkPicturePlayback::SkPicturePlayback(const SkPicture* picture,
                                      const SkPictureRecord& record,
                                      const SkPictInfo& info,
-                                     bool deepCopy)
+                                     bool deepCopyOps)
     : fPicture(picture)
     , fInfo(info) {
 #ifdef SK_DEBUG_SIZE
@@ -107,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());

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

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);

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

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.

+    } else {
+        fOpData = writer.snapshotAsData();
+    }
 
     fBoundingHierarchy = record.fBoundingHierarchy;
     fStateTree = record.fStateTree;
@@ -134,12 +139,8 @@ SkPicturePlayback::SkPicturePlayback(const SkPicture* picture,
     if (fPictureCount > 0) {
         fPictureRefs = SkNEW_ARRAY(const SkPicture*, fPictureCount);
         for (int i = 0; i < fPictureCount; i++) {
-            if (deepCopy) {
-                fPictureRefs[i] = pictures[i]->clone();

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

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.)

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

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.

-            } else {
-                fPictureRefs[i] = pictures[i];
-                fPictureRefs[i]->ref();
-            }
+            fPictureRefs[i] = pictures[i];
+            fPictureRefs[i]->ref();
         }
     }