Remove SkPicture pointer from SkPicturePlayback

This CL simplifies the relationship between SkPicture and SkPicturePlayback by moving the path heap into SkPicturePlayback and removing SkPicturePlayback's SkPicture pointer.

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

Author: robertphillips@google.com

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

debugger/QT/SkDebuggerGUI.cpp

@@ -160,23 +160,21 @@ void SkDebuggerGUI::showDeletes() {
 // offsets to individual commands.
 class SkTimedPicturePlayback : public SkPicturePlayback {
 public:
-    static SkTimedPicturePlayback* CreateFromStream(SkPicture* picture,
-                                                    SkStream* stream, const SkPictInfo& info,
+    static SkTimedPicturePlayback* CreateFromStream(SkStream* stream, const SkPictInfo& info,
                                                     SkPicture::InstallPixelRefProc proc,
                                                     const SkTDArray<bool>& deletedCommands) {
         // Mimics SkPicturePlayback::CreateFromStream
         SkAutoTDelete<SkTimedPicturePlayback> playback(SkNEW_ARGS(SkTimedPicturePlayback,
-                                                               (picture, deletedCommands, info)));
-        if (!playback->parseStream(picture, stream, proc)) {
+                                                               (deletedCommands, info)));
+        if (!playback->parseStream(stream, proc)) {
             return NULL; // we're invalid
         }
         return playback.detach();
     }
 
-    SkTimedPicturePlayback(SkPicture* picture,
-                           const SkTDArray<bool>& deletedCommands,
+    SkTimedPicturePlayback(const SkTDArray<bool>& deletedCommands,
                            const SkPictInfo& info)
-        : INHERITED(picture, info)
+        : INHERITED(info)
         , fSkipCommands(deletedCommands)
         , fTot(0.0)
         , fCurCommand(0) {
@@ -272,21 +270,20 @@ public:
             return NULL;
         }
 
-        SkTimedPicture* newPict = SkNEW_ARGS(SkTimedPicture, (NULL, info.fWidth, info.fHeight));
         // Check to see if there is a playback to recreate.
         if (stream->readBool()) {
             SkTimedPicturePlayback* playback = SkTimedPicturePlayback::CreateFromStream(
-                                                                newPict, stream,
+                                                                stream,
                                                                 info, proc,
                                                                 deletedCommands);
             if (NULL == playback) {
-                SkDELETE(newPict);
                 return NULL;
             }
-            newPict->fPlayback = playback;
+
+            return SkNEW_ARGS(SkTimedPicture, (playback, info.fWidth, info.fHeight));
         }
 
-        return newPict;
+        return NULL;
     }
 
     void resetTimes() { ((SkTimedPicturePlayback*) fPlayback)->resetTimes(); }

include/core/SkPicture.h

@@ -23,7 +23,6 @@ class SkBBoxHierarchy;
 class SkCanvas;
 class SkDrawPictureCallback;
 class SkData;
-class SkPathHeap;
 class SkPicturePlayback;
 class SkPictureRecord;
 class SkStream;
@@ -286,23 +285,12 @@ protected:
     // playback is unchanged.
     SkPicture(SkPicturePlayback*, int width, int height);
 
-    SkPicture(int width, int height, SkPictureRecord& record, bool deepCopyOps);
+    SkPicture(int width, int height, const SkPictureRecord& record, bool deepCopyOps);
 
 private:
-    SkAutoTUnref<SkPathHeap> fPathHeap;  // reference counted
-
-    const SkPath& getPath(int index) const;
-    int addPathToHeap(const SkPath& path);
-
-    void flattenToBuffer(SkWriteBuffer& buffer) const;
-    bool parseBufferTag(SkReadBuffer& buffer, uint32_t tag, uint32_t size);
-
     static void WriteTagSize(SkWriteBuffer& buffer, uint32_t tag, size_t size);
     static void WriteTagSize(SkWStream* stream, uint32_t tag, size_t size);
 
-    void initForPlayback() const;
-    void dumpSize() const;
-
     // An OperationList encapsulates a set of operation offsets into the picture byte
     // stream along with the CTMs needed for those operation.
     class OperationList : ::SkNoncopyable {

src/core/SkPicture.cpp

@@ -131,18 +131,16 @@ SkPicture::SkPicture()
 }
 
 SkPicture::SkPicture(int width, int height,
-                     SkPictureRecord& record,
+                     const SkPictureRecord& record,
                      bool deepCopyOps)
     : fWidth(width)
     , fHeight(height)
     , fAccelData(NULL) {
     this->needsNewGenID();
 
-    fPathHeap.reset(SkSafeRef(record.pathHeap()));
-
     SkPictInfo info;
     this->createHeader(&info);
-    fPlayback = SkNEW_ARGS(SkPicturePlayback, (this, record, info, deepCopyOps));
+    fPlayback = SkNEW_ARGS(SkPicturePlayback, (record, info, deepCopyOps));
 }
 
 SkPicture::SkPicture(const SkPicture& src)
@@ -152,47 +150,12 @@ SkPicture::SkPicture(const SkPicture& src)
     fWidth = src.fWidth;
     fHeight = src.fHeight;
 
-    /*  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).
-     */
     if (src.fPlayback) {
-        fPlayback = SkNEW_ARGS(SkPicturePlayback, (this, *src.fPlayback));
+        fPlayback = SkNEW_ARGS(SkPicturePlayback, (*src.fPlayback));
         fUniqueID = src.uniqueID();     // need to call method to ensure != 0
     } else {
         fPlayback = NULL;
     }
-
-    fPathHeap.reset(SkSafeRef(src.fPathHeap.get()));
-}
-
-const SkPath& SkPicture::getPath(int index) const {
-    return (*fPathHeap.get())[index];
-}
-
-int SkPicture::addPathToHeap(const SkPath& path) {
-    if (NULL == fPathHeap) {
-        fPathHeap.reset(SkNEW(SkPathHeap));
-    }
-#ifdef SK_DEDUP_PICTURE_PATHS
-    return fPathHeap->insert(path);
-#else
-    return fPathHeap->append(path);
-#endif
-}
-
-void SkPicture::initForPlayback() const {
-    // ensure that the paths bounds are pre-computed
-    if (NULL != fPathHeap.get()) {
-        for (int i = 0; i < fPathHeap->count(); i++) {
-            (*fPathHeap.get())[i].updateBoundsCache();
-        }
-    }
-}
-
-void SkPicture::dumpSize() const {
-    SkDebugf("--- picture size: paths=%d\n",
-             SafeCount(fPathHeap.get()));
 }
 
 SkPicture::~SkPicture() {
@@ -206,7 +169,6 @@ void SkPicture::swap(SkPicture& other) {
     SkTSwap(fAccelData, other.fAccelData);
     SkTSwap(fWidth, other.fWidth);
     SkTSwap(fHeight, other.fHeight);
-    fPathHeap.swap(&other.fPathHeap);
 }
 
 SkPicture* SkPicture::clone() const {
@@ -273,13 +235,11 @@ void SkPicture::clone(SkPicture* pictures, int count) const {
                 copyInfo.initialized = true;
             }
 
-            clone->fPlayback = SkNEW_ARGS(SkPicturePlayback, (clone, *fPlayback, &copyInfo));
+            clone->fPlayback = SkNEW_ARGS(SkPicturePlayback, (*fPlayback, &copyInfo));
             clone->fUniqueID = this->uniqueID(); // need to call method to ensure != 0
         } else {
             clone->fPlayback = NULL;
         }
-
-        clone->fPathHeap.reset(SkSafeRef(fPathHeap.get()));
     }
 }
 
@@ -389,20 +349,17 @@ SkPicture* SkPicture::CreateFromStream(SkStream* stream, InstallPixelRefProc pro
         return NULL;
     }
 
-    SkPicture* newPict = SkNEW_ARGS(SkPicture, (NULL, info.fWidth, info.fHeight));
-
     // Check to see if there is a playback to recreate.
     if (stream->readBool()) {
-        SkPicturePlayback* playback = SkPicturePlayback::CreateFromStream(newPict, stream,
-                                                                          info, proc);
+        SkPicturePlayback* playback = SkPicturePlayback::CreateFromStream(stream, info, proc);
         if (NULL == playback) {
-            SkDELETE(newPict);
             return NULL;
         }
-        newPict->fPlayback = playback;
+
+        return SkNEW_ARGS(SkPicture, (playback, info.fWidth, info.fHeight));
     }
 
-    return newPict;
+    return NULL;
 }
 
 SkPicture* SkPicture::CreateFromBuffer(SkReadBuffer& buffer) {
@@ -412,19 +369,17 @@ SkPicture* SkPicture::CreateFromBuffer(SkReadBuffer& buffer) {
         return NULL;
     }
 
-    SkPicture* newPict = SkNEW_ARGS(SkPicture, (NULL, info.fWidth, info.fHeight));
-
     // Check to see if there is a playback to recreate.
     if (buffer.readBool()) {
-        SkPicturePlayback* playback = SkPicturePlayback::CreateFromBuffer(newPict, buffer, info);
+        SkPicturePlayback* playback = SkPicturePlayback::CreateFromBuffer(buffer, info);
         if (NULL == playback) {
-            SkDELETE(newPict);
             return NULL;
         }
-        newPict->fPlayback = playback;
+
+        return SkNEW_ARGS(SkPicture, (playback, info.fWidth, info.fHeight));
     }
 
-    return newPict;
+    return NULL;
 }
 
 void SkPicture::createHeader(SkPictInfo* info) const {
@@ -474,32 +429,6 @@ void SkPicture::WriteTagSize(SkWStream* stream, uint32_t tag,  size_t size) {
     stream->write32(SkToU32(size));
 }
 
-bool SkPicture::parseBufferTag(SkReadBuffer& buffer,
-                               uint32_t tag,
-                               uint32_t size) {
-    switch (tag) {
-        case SK_PICT_PATH_BUFFER_TAG:
-            if (size > 0) {
-                fPathHeap.reset(SkNEW_ARGS(SkPathHeap, (buffer)));
-            }
-            break;
-        default:
-            // The tag was invalid.
-            return false;
-    }
-
-    return true;    // success
-}
-
-void SkPicture::flattenToBuffer(SkWriteBuffer& buffer) const {
-    int n;
-
-    if ((n = SafeCount(fPathHeap.get())) > 0) {
-        WriteTagSize(buffer, SK_PICT_PATH_BUFFER_TAG, n);
-        fPathHeap->flatten(buffer);
-    }
-}
-
 void SkPicture::flatten(SkWriteBuffer& buffer) const {
     SkPicturePlayback* playback = fPlayback;
 

src/core/SkPicturePlayback.cpp

@@ -50,18 +50,24 @@ void SkPicturePlayback::PlaybackReplacements::validate() const {
 }
 #endif
 
-SkPicturePlayback::SkPicturePlayback(const SkPicture* picture, const SkPictInfo& info)
-    : fPicture(picture)
-    , fInfo(info) {
+SkPicturePlayback::SkPicturePlayback(const SkPictInfo& info)
+    : fInfo(info) {
     this->init();
 }
 
-SkPicturePlayback::SkPicturePlayback(const SkPicture* picture,
-                                     const SkPictureRecord& record,
+void SkPicturePlayback::initForPlayback() const {
+    // ensure that the paths bounds are pre-computed
+    if (NULL != fPathHeap.get()) {
+        for (int i = 0; i < fPathHeap->count(); i++) {
+            (*fPathHeap.get())[i].updateBoundsCache();
+        }
+    }
+}
+
+SkPicturePlayback::SkPicturePlayback(const SkPictureRecord& record,
                                      const SkPictInfo& info,
                                      bool deepCopyOps)
-    : fPicture(picture)
-    , fInfo(info) {
+    : fInfo(info) {
 #ifdef SK_DEBUG_SIZE
     size_t overallBytes, bitmapBytes, matricesBytes,
     paintBytes, pathBytes, pictureBytes, regionBytes;
@@ -121,8 +127,9 @@ SkPicturePlayback::SkPicturePlayback(const SkPicture* picture,
     fPaints = record.fPaints.unflattenToArray();
 
     fBitmapHeap.reset(SkSafeRef(record.fBitmapHeap));
+    fPathHeap.reset(SkSafeRef(record.pathHeap()));
 
-    picture->initForPlayback();
+    this->initForPlayback();
 
     const SkTDArray<const SkPicture* >& pictures = record.getPictureRefs();
     fPictureCount = pictures.count();
@@ -156,14 +163,12 @@ SkPicturePlayback::SkPicturePlayback(const SkPicture* picture,
 #endif
 }
 
-SkPicturePlayback::SkPicturePlayback(const SkPicture* picture, 
-                                     const SkPicturePlayback& src,
-                                     SkPictCopyInfo* deepCopyInfo)
-    : fPicture(picture)
-    , fInfo(src.fInfo) {
+SkPicturePlayback::SkPicturePlayback(const SkPicturePlayback& src, SkPictCopyInfo* deepCopyInfo)
+    : fInfo(src.fInfo) {
     this->init();
 
     fBitmapHeap.reset(SkSafeRef(src.fBitmapHeap.get()));
+    fPathHeap.reset(SkSafeRef(src.fPathHeap.get()));
 
     fOpData = SkSafeRef(src.fOpData);
 
@@ -254,7 +259,8 @@ void SkPicturePlayback::dumpSize() const {
              fOpData->size(),
              SafeCount(fBitmaps), SafeCount(fBitmaps) * sizeof(SkBitmap),
              SafeCount(fPaints), SafeCount(fPaints) * sizeof(SkPaint));
-    fPicture->dumpSize();
+    SkDebugf("--- picture size: paths=%d\n",
+             SafeCount(fPathHeap.get()));
 }
 
 bool SkPicturePlayback::containsBitmaps() const {
@@ -351,7 +357,10 @@ void SkPicturePlayback::flattenToBuffer(SkWriteBuffer& buffer) const {
         }
     }
 
-    fPicture->flattenToBuffer(buffer);
+    if ((n = SafeCount(fPathHeap.get())) > 0) {
+        SkPicture::WriteTagSize(buffer, SK_PICT_PATH_BUFFER_TAG, n);
+        fPathHeap->flatten(buffer);
+    }
 }
 
 void SkPicturePlayback::serialize(SkWStream* stream,
@@ -433,8 +442,7 @@ static uint32_t pictInfoFlagsToReadBufferFlags(uint32_t pictInfoFlags) {
     return rbMask;
 }
 
-bool SkPicturePlayback::parseStreamTag(SkPicture* picture,
-                                       SkStream* stream,
+bool SkPicturePlayback::parseStreamTag(SkStream* stream,
                                        uint32_t tag,
                                        uint32_t size,
                                        SkPicture::InstallPixelRefProc proc) {
@@ -536,7 +544,7 @@ bool SkPicturePlayback::parseStreamTag(SkPicture* picture,
             while (!buffer.eof()) {
                 tag = buffer.readUInt();
                 size = buffer.readUInt();
-                if (!this->parseBufferTag(picture, buffer, tag, size)) {
+                if (!this->parseBufferTag(buffer, tag, size)) {
                     return false;
                 }
             }
@@ -546,8 +554,7 @@ bool SkPicturePlayback::parseStreamTag(SkPicture* picture,
     return true;    // success
 }
 
-bool SkPicturePlayback::parseBufferTag(SkPicture* picture,
-                                       SkReadBuffer& buffer,
+bool SkPicturePlayback::parseBufferTag(SkReadBuffer& buffer,
                                        uint32_t tag, uint32_t size) {
     switch (tag) {
         case SK_PICT_BITMAP_BUFFER_TAG: {
@@ -567,7 +574,9 @@ bool SkPicturePlayback::parseBufferTag(SkPicture* picture,
             }
         } break;
         case SK_PICT_PATH_BUFFER_TAG:
-            picture->parseBufferTag(buffer, tag, size);
+            if (size > 0) {
+                fPathHeap.reset(SkNEW_ARGS(SkPathHeap, (buffer)));
+            }
             break;
         case SK_PICT_READER_TAG: {
             SkAutoMalloc storage(size);
@@ -611,32 +620,29 @@ bool SkPicturePlayback::parseBufferTag(SkPicture* picture,
     return true;    // success
 }
 
-SkPicturePlayback* SkPicturePlayback::CreateFromStream(SkPicture* picture,
-                                                       SkStream* stream,
+SkPicturePlayback* SkPicturePlayback::CreateFromStream(SkStream* stream,
                                                        const SkPictInfo& info,
                                                        SkPicture::InstallPixelRefProc proc) {
-    SkAutoTDelete<SkPicturePlayback> playback(SkNEW_ARGS(SkPicturePlayback, (picture, info)));
+    SkAutoTDelete<SkPicturePlayback> playback(SkNEW_ARGS(SkPicturePlayback, (info)));
 
-    if (!playback->parseStream(picture, stream, proc)) {
+    if (!playback->parseStream(stream, proc)) {
         return NULL;
     }
     return playback.detach();
 }
 
-SkPicturePlayback* SkPicturePlayback::CreateFromBuffer(SkPicture* picture,
-                                                       SkReadBuffer& buffer,
+SkPicturePlayback* SkPicturePlayback::CreateFromBuffer(SkReadBuffer& buffer,
                                                        const SkPictInfo& info) {
-    SkAutoTDelete<SkPicturePlayback> playback(SkNEW_ARGS(SkPicturePlayback, (picture, info)));
+    SkAutoTDelete<SkPicturePlayback> playback(SkNEW_ARGS(SkPicturePlayback, (info)));
     buffer.setVersion(info.fVersion);
 
-    if (!playback->parseBuffer(picture, buffer)) {
+    if (!playback->parseBuffer(buffer)) {
         return NULL;
     }
     return playback.detach();
 }
 
-bool SkPicturePlayback::parseStream(SkPicture* picture,
-                                    SkStream* stream,
+bool SkPicturePlayback::parseStream(SkStream* stream,
                                     SkPicture::InstallPixelRefProc proc) {
     for (;;) {
         uint32_t tag = stream->readU32();
@@ -645,14 +651,14 @@ bool SkPicturePlayback::parseStream(SkPicture* picture,
         }
 
         uint32_t size = stream->readU32();
-        if (!this->parseStreamTag(picture, stream, tag, size, proc)) {
+        if (!this->parseStreamTag(stream, tag, size, proc)) {
             return false; // we're invalid
         }
     }
     return true;
 }
 
-bool SkPicturePlayback::parseBuffer(SkPicture* picture, SkReadBuffer& buffer) {
+bool SkPicturePlayback::parseBuffer(SkReadBuffer& buffer) {
     for (;;) {
         uint32_t tag = buffer.readUInt();
         if (SK_PICT_EOF_TAG == tag) {
@@ -660,7 +666,7 @@ bool SkPicturePlayback::parseBuffer(SkPicture* picture, SkReadBuffer& buffer) {
         }
 
         uint32_t size = buffer.readUInt();
-        if (!this->parseBufferTag(picture, buffer, tag, size)) {
+        if (!this->parseBufferTag(buffer, tag, size)) {
             return false; // we're invalid
         }
     }

src/core/SkPicturePlayback.h

@@ -9,29 +9,27 @@
 #ifndef SkPicturePlayback_DEFINED
 #define SkPicturePlayback_DEFINED
 
-#include "SkPicture.h"
-#include "SkReader32.h"
-
 #include "SkBitmap.h"
-#include "SkData.h"
-#include "SkMatrix.h"
-#include "SkReadBuffer.h"
-#include "SkPaint.h"
-#include "SkPath.h"
 #include "SkPathHeap.h"
-#include "SkRegion.h"
-#include "SkRRect.h"
+#include "SkPicture.h"
 #include "SkPictureFlat.h"
 
 #ifdef SK_BUILD_FOR_ANDROID
 #include "SkThread.h"
 #endif
 
+class SkData;
 class SkPictureRecord;
+class SkReader32;
 class SkStream;
 class SkWStream;
 class SkBBoxHierarchy;
+class SkMatrix;
+class SkPaint;
+class SkPath;
 class SkPictureStateTree;
+class SkReadBuffer;
+class SkRegion;
 
 struct SkPictInfo {
     enum Flags {
@@ -126,16 +124,13 @@ struct SkPictCopyInfo {
 
 class SkPicturePlayback {
 public:
-    SkPicturePlayback(const SkPicture* picture, const SkPicturePlayback& src,
+    SkPicturePlayback(const SkPicturePlayback& src,
                       SkPictCopyInfo* deepCopyInfo = NULL);
-    SkPicturePlayback(const SkPicture* picture, const SkPictureRecord& record, 
-                      const SkPictInfo&, bool deepCopyOps);
-    static SkPicturePlayback* CreateFromStream(SkPicture* picture,
-                                               SkStream*,
+    SkPicturePlayback(const SkPictureRecord& record, const SkPictInfo&, bool deepCopyOps);
+    static SkPicturePlayback* CreateFromStream(SkStream*,
                                                const SkPictInfo&,
                                                SkPicture::InstallPixelRefProc);
-    static SkPicturePlayback* CreateFromBuffer(SkPicture* picture,
-                                               SkReadBuffer&,
+    static SkPicturePlayback* CreateFromBuffer(SkReadBuffer&,
                                                const SkPictInfo&);
 
     virtual ~SkPicturePlayback();
@@ -163,10 +158,10 @@ public:
     void resetOpID() { fCurOffset = 0; }
 
 protected:
-    explicit SkPicturePlayback(const SkPicture* picture, const SkPictInfo& info);
+    explicit SkPicturePlayback(const SkPictInfo& info);
 
-    bool parseStream(SkPicture* picture, SkStream*, SkPicture::InstallPixelRefProc);
-    bool parseBuffer(SkPicture* picture, SkReadBuffer& buffer);
+    bool parseStream(SkStream*, SkPicture::InstallPixelRefProc);
+    bool parseBuffer(SkReadBuffer& buffer);
 #ifdef SK_DEVELOPER
     virtual bool preDraw(int opIndex, int type);
     virtual void postDraw(int opIndex);
@@ -197,7 +192,8 @@ private:
     }
 
     const SkPath& getPath(SkReader32& reader) {
-        return fPicture->getPath(reader.readInt() - 1);
+        int index = reader.readInt() - 1;
+        return (*fPathHeap.get())[index];
     }
 
     const SkPicture* getPicture(SkReader32& reader) {
@@ -277,18 +273,14 @@ public:
 #endif
 
 private:    // these help us with reading/writing
-    bool parseStreamTag(SkPicture* picture, SkStream*, uint32_t tag, uint32_t size,
-                        SkPicture::InstallPixelRefProc);
-    bool parseBufferTag(SkPicture* picture, SkReadBuffer&, uint32_t tag, uint32_t size);
+    bool parseStreamTag(SkStream*, uint32_t tag, uint32_t size, SkPicture::InstallPixelRefProc);
+    bool parseBufferTag(SkReadBuffer&, uint32_t tag, uint32_t size);
     void flattenToBuffer(SkWriteBuffer&) const;
 
 private:
     friend class SkPicture;
     friend class SkGpuDevice;   // for access to setDrawLimits & setReplacements
 
-    // The picture that owns this SkPicturePlayback object
-    const SkPicture* fPicture;
-
     // Only used by getBitmap() if the passed in index is SkBitmapHeap::INVALID_SLOT. This empty
     // bitmap allows playback to draw nothing and move on.
     SkBitmap fBadBitmap;
@@ -300,6 +292,8 @@ private:
 
     SkData* fOpData;    // opcodes and parameters
 
+    SkAutoTUnref<const SkPathHeap> fPathHeap;  // reference counted
+
     const SkPicture** fPictureRefs;
     int fPictureCount;
 
@@ -403,6 +397,8 @@ private:
     static void WriteFactories(SkWStream* stream, const SkFactorySet& rec);
     static void WriteTypefaces(SkWStream* stream, const SkRefCntSet& rec);
 
+    void initForPlayback() const;
+
 #ifdef SK_BUILD_FOR_ANDROID
     SkMutex fDrawMutex;
     bool fAbortCurrentPlayback;

src/core/SkPictureRecord.h

@@ -88,8 +88,8 @@ public:
         return fWriter.snapshotAsData();
     }
 
-    SkPathHeap* pathHeap() {
-        return fPathHeap;
+    const SkPathHeap* pathHeap() const {
+        return fPathHeap.get();
     }
 
     const SkPictureContentInfo& contentInfo() const {