Remove Skia's use of the default SkPicture constructor and multi-clone

This cannot be landed until (Chrome: Switch to one-at-a-time SkPicture::clone interface - https://codereview.chromium.org/380323002/) has landed.

R=mtklein@google.com
TBR=reed@google.com

Review URL: https://codereview.chromium.org/388833003

diff --git a/gyp/skia_for_android_framework_defines.gypi b/gyp/skia_for_android_framework_defines.gypi
index 93d80ea..c0a9a69 100644 (file)
--- a/gyp/skia_for_android_framework_defines.gypi
+++ b/gyp/skia_for_android_framework_defines.gypi
@@ -19,6 +19,7 @@
       'SK_SUPPORT_LEGACY_CLIPTOLAYERFLAG',
       # Transitional, for deprecated SkCanvas::SaveFlags methods.
       'SK_ATTR_DEPRECATED=SK_NOTHING_ARG1',
+      'SK_SUPPORT_LEGACY_DEFAULT_PICTURE_CTOR',
     ],
   },
 }

diff --git a/include/core/SkPicture.h b/include/core/SkPicture.h
index 362f806..db996e0 100644 (file)
--- a/include/core/SkPicture.h
+++ b/include/core/SkPicture.h
@@ -63,7 +63,9 @@ public:
         typedef SkRefCnt INHERITED;
     };
 
+#ifdef SK_SUPPORT_LEGACY_DEFAULT_PICTURE_CTOR
     SkPicture();
+#endif
 
     /**  PRIVATE / EXPERIMENTAL -- do not call */
     void EXPERIMENTAL_addAccelData(const AccelData*) const;
@@ -112,13 +114,6 @@ public:
      *  Creates a thread-safe clone of the picture that is ready for playback.
      */
     SkPicture* clone() const;
-
-    /**
-     * Creates multiple thread-safe clones of this picture that are ready for
-     * playback. The resulting clones are stored in the provided array of
-     * SkPictures.
-     */
-    void clone(SkPicture* pictures, int count) const;
 #endif
 
     /** Replays the drawing commands on the specified canvas.
@@ -244,8 +239,8 @@ private:
 
     void needsNewGenID() { fUniqueID = SK_InvalidGenID; }
 
-    // Create a new SkPicture from an existing SkPictureData. Ref count of
-    // data is unchanged.
+    // Create a new SkPicture from an existing SkPictureData. The new picture
+    // takes ownership of 'data'.
     SkPicture(SkPictureData* data, int width, int height);
 
     SkPicture(int width, int height, const SkPictureRecord& record, bool deepCopyOps);

diff --git a/src/core/SkPicture.cpp b/src/core/SkPicture.cpp
index b96caf9..d57de97 100644 (file)
--- a/src/core/SkPicture.cpp
+++ b/src/core/SkPicture.cpp
@@ -111,6 +111,7 @@ const char* DrawTypeToString(DrawType drawType) {
 
 ///////////////////////////////////////////////////////////////////////////////
 
+#ifdef SK_SUPPORT_LEGACY_DEFAULT_PICTURE_CTOR
 // fRecord OK
 SkPicture::SkPicture()
     : fWidth(0)
@@ -118,6 +119,7 @@ SkPicture::SkPicture()
     , fRecordWillPlayBackBitmaps(false) {
     this->needsNewGenID();
 }
+#endif
 
 // fRecord OK
 SkPicture::SkPicture(int width, int height,
@@ -147,75 +149,59 @@ SkPicture::~SkPicture() {}
 #ifdef SK_SUPPORT_LEGACY_PICTURE_CLONE
 // fRecord TODO, fix by deleting this method
 SkPicture* SkPicture::clone() const {
-    SkPicture* clonedPicture = SkNEW(SkPicture);
-    this->clone(clonedPicture, 1);
-    return clonedPicture;
-}
 
-// fRecord TODO, fix by deleting this method
-void SkPicture::clone(SkPicture* pictures, int count) const {
-    SkPictCopyInfo copyInfo;
+    SkAutoTDelete<SkPictureData> newData;
 
-    for (int i = 0; i < count; i++) {
-        SkPicture* clone = &pictures[i];
+    if (fData.get()) {
+        SkPictCopyInfo copyInfo;
 
-        clone->needsNewGenID();
-        clone->fWidth = fWidth;
-        clone->fHeight = fHeight;
-        clone->fData.reset(NULL);
-        clone->fRecordWillPlayBackBitmaps = fRecordWillPlayBackBitmaps;
+        int paintCount = SafeCount(fData->fPaints);
 
-        /*  We want to copy the src's playback. However, if that hasn't been built
-            yet, we need to fake a call to endRecording() without actually calling
-            it (since it is destructive, and we don't want to change src).
+        /* The alternative to doing this is to have a clone method on the paint and have it
+         * make the deep copy of its internal structures as needed. The holdup to doing
+         * that is at this point we would need to pass the SkBitmapHeap so that we don't
+         * unnecessarily flatten the pixels in a bitmap shader.
          */
-        if (fData.get()) {
-            if (!copyInfo.initialized) {
-                int paintCount = SafeCount(fData->fPaints);
-
-                /* The alternative to doing this is to have a clone method on the paint and have it
-                 * make the deep copy of its internal structures as needed. The holdup to doing
-                 * that is at this point we would need to pass the SkBitmapHeap so that we don't
-                 * unnecessarily flatten the pixels in a bitmap shader.
-                 */
-                copyInfo.paintData.setCount(paintCount);
-
-                /* Use an SkBitmapHeap to avoid flattening bitmaps in shaders. If there already is
-                 * one, use it. If this SkPictureData was created from a stream, fBitmapHeap
-                 * will be NULL, so create a new one.
-                 */
-                if (fData->fBitmapHeap.get() == NULL) {
-                    // FIXME: Put this on the stack inside SkPicture::clone.
-                    SkBitmapHeap* heap = SkNEW(SkBitmapHeap);
-                    copyInfo.controller.setBitmapStorage(heap);
-                    heap->unref();
-                } else {
-                    copyInfo.controller.setBitmapStorage(fData->fBitmapHeap);
-                }
-
-                SkDEBUGCODE(int heapSize = SafeCount(fData->fBitmapHeap.get());)
-                for (int i = 0; i < paintCount; i++) {
-                    if (NeedsDeepCopy(fData->fPaints->at(i))) {
-                        copyInfo.paintData[i] =
-                            SkFlatData::Create<SkPaint::FlatteningTraits>(&copyInfo.controller,
-                                                              fData->fPaints->at(i), 0);
-
-                    } else {
-                        // this is our sentinel, which we use in the unflatten loop
-                        copyInfo.paintData[i] = NULL;
-                    }
-                }
-                SkASSERT(SafeCount(fData->fBitmapHeap.get()) == heapSize);
-
-                // needed to create typeface playback
-                copyInfo.controller.setupPlaybacks();
-                copyInfo.initialized = true;
-            }
+        copyInfo.paintData.setCount(paintCount);
 
-            clone->fData.reset(SkNEW_ARGS(SkPictureData, (*fData, &copyInfo)));
-            clone->fUniqueID = this->uniqueID(); // need to call method to ensure != 0
+        /* Use an SkBitmapHeap to avoid flattening bitmaps in shaders. If there already is
+         * one, use it. If this SkPictureData was created from a stream, fBitmapHeap
+         * will be NULL, so create a new one.
+         */
+        if (fData->fBitmapHeap.get() == NULL) {
+            // FIXME: Put this on the stack inside SkPicture::clone.
+            SkBitmapHeap* heap = SkNEW(SkBitmapHeap);
+            copyInfo.controller.setBitmapStorage(heap);
+            heap->unref();
+        } else {
+            copyInfo.controller.setBitmapStorage(fData->fBitmapHeap);
         }
+
+        SkDEBUGCODE(int heapSize = SafeCount(fData->fBitmapHeap.get());)
+        for (int i = 0; i < paintCount; i++) {
+            if (NeedsDeepCopy(fData->fPaints->at(i))) {
+                copyInfo.paintData[i] =
+                    SkFlatData::Create<SkPaint::FlatteningTraits>(&copyInfo.controller,
+                    fData->fPaints->at(i), 0);
+
+            } else {
+                // this is our sentinel, which we use in the unflatten loop
+                copyInfo.paintData[i] = NULL;
+            }
+        }
+        SkASSERT(SafeCount(fData->fBitmapHeap.get()) == heapSize);
+
+        // needed to create typeface playback
+        copyInfo.controller.setupPlaybacks();
+
+        newData.reset(SkNEW_ARGS(SkPictureData, (*fData, &copyInfo)));
     }
+
+    SkPicture* clone = SkNEW_ARGS(SkPicture, (newData.detach(), fWidth, fHeight));
+    clone->fRecordWillPlayBackBitmaps = fRecordWillPlayBackBitmaps;
+    clone->fUniqueID = this->uniqueID(); // need to call method to ensure != 0
+
+    return clone;
 }
 #endif//SK_SUPPORT_LEGACY_PICTURE_CLONE
 

diff --git a/src/core/SkPictureData.cpp b/src/core/SkPictureData.cpp
index 746250d..1553462 100644 (file)
--- a/src/core/SkPictureData.cpp
+++ b/src/core/SkPictureData.cpp
@@ -153,8 +153,6 @@ SkPictureData::SkPictureData(const SkPictureData& src, SkPictCopyInfo* deepCopyI
     SkSafeRef(fStateTree);
 
     if (deepCopyInfo) {
-        SkASSERT(deepCopyInfo->initialized);
-
         int paintCount = SafeCount(src.fPaints);
 
         if (src.fBitmaps) {

diff --git a/src/core/SkPictureData.h b/src/core/SkPictureData.h
index 15ea37b..6ea0b18 100644 (file)
--- a/src/core/SkPictureData.h
+++ b/src/core/SkPictureData.h
@@ -120,9 +120,8 @@ private:
  * enables the data to be generated once and reused for subsequent copies.
  */
 struct SkPictCopyInfo {
-    SkPictCopyInfo() : initialized(false), controller(1024) {}
+    SkPictCopyInfo() : controller(1024) {}
 
-    bool initialized;
     SkChunkFlatController controller;
     SkTDArray<SkFlatData*> paintData;
 };

diff --git a/tests/GpuLayerCacheTest.cpp b/tests/GpuLayerCacheTest.cpp
index d6371e1..02917e4 100644 (file)
--- a/tests/GpuLayerCacheTest.cpp
+++ b/tests/GpuLayerCacheTest.cpp
@@ -10,6 +10,7 @@
 #include "GrContext.h"
 #include "GrContextFactory.h"
 #include "GrLayerCache.h"
+#include "SkPictureRecorder.h"
 #include "Test.h"
 
 static const int kNumLayers = 5;
@@ -54,11 +55,13 @@ DEF_GPUTEST(GpuLayerCache, reporter, factory) {
         return;
     }
 
-    SkPicture picture;
+    SkPictureRecorder recorder;
+    recorder.beginRecording(1, 1);
+    SkAutoTUnref<const SkPicture> picture(recorder.endRecording());
 
     GrLayerCache cache(context);
 
-    create_layers(reporter, &cache, picture);
+    create_layers(reporter, &cache, *picture);
 
     // Lock the layers making them all 512x512
     GrTextureDesc desc;
@@ -67,7 +70,7 @@ DEF_GPUTEST(GpuLayerCache, reporter, factory) {
     desc.fConfig = kSkia8888_GrPixelConfig;
 
     for (int i = 0; i < kNumLayers; ++i) {
-        GrCachedLayer* layer = cache.findLayer(&picture, i);
+        GrCachedLayer* layer = cache.findLayer(picture, i);
         REPORTER_ASSERT(reporter, NULL != layer);
 
         bool foundInCache = cache.lock(layer, desc);
@@ -91,14 +94,14 @@ DEF_GPUTEST(GpuLayerCache, reporter, factory) {
 
     // Unlock the textures
     for (int i = 0; i < kNumLayers; ++i) {
-        GrCachedLayer* layer = cache.findLayer(&picture, i);
+        GrCachedLayer* layer = cache.findLayer(picture, i);
         REPORTER_ASSERT(reporter, NULL != layer);
 
         cache.unlock(layer);
     }
 
     for (int i = 0; i < kNumLayers; ++i) {
-        GrCachedLayer* layer = cache.findLayer(&picture, i);
+        GrCachedLayer* layer = cache.findLayer(picture, i);
         REPORTER_ASSERT(reporter, NULL != layer);
 
 #if USE_ATLAS
@@ -118,7 +121,7 @@ DEF_GPUTEST(GpuLayerCache, reporter, factory) {
 
     // Free them all SkGpuDevice-style. This will not free up the
     // atlas' texture but will eliminate all the layers.
-    cache.purge(&picture);
+    cache.purge(picture);
 
     REPORTER_ASSERT(reporter, GetNumLayers::NumLayers(&cache) == 0);
     // TODO: add VRAM/resource cache check here

diff --git a/tests/PictureTest.cpp b/tests/PictureTest.cpp
index a4dc7d7..cc5d880 100644 (file)
--- a/tests/PictureTest.cpp
+++ b/tests/PictureTest.cpp
@@ -1521,12 +1521,12 @@ static void test_hierarchical(skiatest::Reporter* reporter) {
 
 static void test_gen_id(skiatest::Reporter* reporter) {
 
-    SkPicture empty;
+    SkPictureRecorder recorder;
+    recorder.beginRecording(0, 0);
+    SkAutoTUnref<SkPicture> empty(recorder.endRecording());
 
     // Empty pictures should still have a valid ID
-    REPORTER_ASSERT(reporter, empty.uniqueID() != SK_InvalidGenID);
-
-    SkPictureRecorder recorder;
+    REPORTER_ASSERT(reporter, empty->uniqueID() != SK_InvalidGenID);
 
     SkCanvas* canvas = recorder.beginRecording(1, 1);
     canvas->drawARGB(255, 255, 255, 255);
@@ -1535,7 +1535,7 @@ static void test_gen_id(skiatest::Reporter* reporter) {
     REPORTER_ASSERT(reporter, hasData->uniqueID() != SK_InvalidGenID);
 
     // both pictures should have different ids
-    REPORTER_ASSERT(reporter, hasData->uniqueID() != empty.uniqueID());
+    REPORTER_ASSERT(reporter, hasData->uniqueID() != empty->uniqueID());
 }
 
 DEF_TEST(Picture, reporter) {