Fix error revealed by Android unit test

src/core/SkPicturePlayback.cpp

 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 #include <new>
 #include "SkBBoxHierarchy.h"
 #include "SkPicturePlayback.h"
 #include "SkPictureRecord.h"
@@ skipping 41 matching lines @@
 
 SkPicturePlayback::SkPicturePlayback(const SkPicture* picture, const SkPictInfo& info)
     : fPicture(picture)
     , fInfo(info) {
     this->init();
 }
 
 SkPicturePlayback::SkPicturePlayback(const SkPicture* picture,
                                      const SkPictureRecord& record,
                                      const SkPictInfo& info,
-                                     bool deepCopy)
+                                     bool deepCopyOps)
     : fPicture(picture)
     , fInfo(info) {
 #ifdef SK_DEBUG_SIZE
     size_t overallBytes, bitmapBytes, matricesBytes,
     paintBytes, pathBytes, pictureBytes, regionBytes;
     int bitmaps = record.bitmaps(&bitmapBytes);
     int matrices = record.matrices(&matricesBytes);
     int paints = record.paints(&paintBytes);
     int paths = record.paths(&pathBytes);
     int pictures = record.pictures(&pictureBytes);
@@ skipping 27 matching lines @@
 #endif
 
     record.validate(record.writeStream().bytesWritten(), 0);
     const SkWriter32& writer = record.writeStream();
     this->init();
     SkASSERT(!fOpData);
     if (writer.bytesWritten() == 0) {
         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;
 
     SkSafeRef(fBoundingHierarchy);
     SkSafeRef(fStateTree);
 
     if (NULL != fBoundingHierarchy) {
         fBoundingHierarchy->flushDeferredInserts();
     }
 
     // copy over the refcnt dictionary to our reader
     record.fFlattenableHeap.setupPlaybacks();
 
     fBitmaps = record.fBitmapHeap->extractBitmaps();
     fPaints = record.fPaints.unflattenToArray();
 
     fBitmapHeap.reset(SkSafeRef(record.fBitmapHeap));
 
     picture->initForPlayback();
 
     const SkTDArray<const SkPicture* >& pictures = record.getPictureRefs();
     fPictureCount = pictures.count();
     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();
         }
     }
 
 #ifdef SK_DEBUG_SIZE
     int overall = fPlayback->size(&overallBytes);
     bitmaps = fPlayback->bitmaps(&bitmapBytes);
     paints = fPlayback->paints(&paintBytes);
     paths = fPlayback->paths(&pathBytes);
     pictures = fPlayback->pictures(&pictureBytes);
     regions = fPlayback->regions(&regionBytes);
@@ skipping 1685 matching lines @@
     for (index = 0; index < fPictureCount; index++)
         bufferPtr += snprintf(bufferPtr, DUMP_BUFFER_SIZE - (bufferPtr - pBuffer),
             "picture%p, ", fPictureRefs[index]);
     if (fPictureCount > 0)
         SkDebugf("%s0};\n", pBuffer);
 
     const_cast<SkPicturePlayback*>(this)->dumpStream();
 }
 
 #endif