Remove picture pre-allocation from SkPictureRecorder

This CL improves the separation of the SkPicture and SkPictureRecord classes. It delays creation of the SkPicture (in SkPictureRecorder) until recording is actually completed. To accomplish this the SkRecord-derived classes now get SkPathHeap and SkPictureContentInfo members that are absorbed by the SkPicture when it is constructed.

As an ancillary change, this CL also moves the SkPictureContentInfo object from SkPicture to SkPicturePlayback. This is intended to centralize all the data in the SkPicturePlayback object.

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

Author: robertphillips@google.com

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

9 files changed, 166 insertions, 87 deletions

diff --git a/src/core/SkBBoxHierarchyRecord.cpp b/src/core/SkBBoxHierarchyRecord.cpp
index d6fbd21239..96e6cdf102 100644
--- a/src/core/SkBBoxHierarchyRecord.cpp
+++ b/src/core/SkBBoxHierarchyRecord.cpp
@@ -9,11 +9,10 @@
 #include "SkBBoxHierarchyRecord.h"
 #include "SkPictureStateTree.h"
 
-SkBBoxHierarchyRecord::SkBBoxHierarchyRecord(SkPicture* picture,
-                                             const SkISize& size,
+SkBBoxHierarchyRecord::SkBBoxHierarchyRecord(const SkISize& size,
                                              uint32_t recordFlags,
                                              SkBBoxHierarchy* h)
-    : INHERITED(picture, size, recordFlags) {
+    : INHERITED(size, recordFlags) {
     fStateTree = SkNEW(SkPictureStateTree);
     fBoundingHierarchy = h;
     fBoundingHierarchy->ref();
diff --git a/src/core/SkBBoxHierarchyRecord.h b/src/core/SkBBoxHierarchyRecord.h
index f6fac18018..51fce0d8bc 100644
--- a/src/core/SkBBoxHierarchyRecord.h
+++ b/src/core/SkBBoxHierarchyRecord.h
@@ -19,8 +19,7 @@
 class SkBBoxHierarchyRecord : public SkBBoxRecord, public SkBBoxHierarchyClient {
 public:
     /** This will take a ref of h */
-    SkBBoxHierarchyRecord(SkPicture* picture, const SkISize& size,
-                          uint32_t recordFlags, SkBBoxHierarchy* h);
+    SkBBoxHierarchyRecord(const SkISize& size, uint32_t recordFlags, SkBBoxHierarchy* h);
 
     virtual void handleBBox(const SkRect& bounds) SK_OVERRIDE;
 
diff --git a/src/core/SkBBoxRecord.h b/src/core/SkBBoxRecord.h
index 123a91f1ec..f3d72b0448 100644
--- a/src/core/SkBBoxRecord.h
+++ b/src/core/SkBBoxRecord.h
@@ -19,8 +19,8 @@
 class SkBBoxRecord : public SkPictureRecord {
 public:
 
-    SkBBoxRecord(SkPicture* picture, const SkISize& size, uint32_t recordFlags)
-        : INHERITED(picture, size, recordFlags) {
+    SkBBoxRecord(const SkISize& size, uint32_t recordFlags)
+        : INHERITED(size, recordFlags) {
     }
     virtual ~SkBBoxRecord() { }
 
diff --git a/src/core/SkPicture.cpp b/src/core/SkPicture.cpp
index 7120f61179..9b7177870d 100644
--- a/src/core/SkPicture.cpp
+++ b/src/core/SkPicture.cpp
@@ -130,26 +130,24 @@ SkPicture::SkPicture()
     fWidth = fHeight = 0;
 }
 
-// This method makes a SkPicturePlayback object from an in-progress recording.
-// Unfortunately, it does not include the restoreToCount of a real endRecording
-// call.
-SkPicturePlayback* SkPicture::FakeEndRecording(const SkPicture* resourceSrc,
-                                               const SkPictureRecord& record) {
-    SkPictInfo info;
-    resourceSrc->createHeader(&info);
+SkPicture::SkPicture(int width, int height,
+                     SkPictureRecord& record,
+                     bool deepCopyOps)
+    : fWidth(width)
+    , fHeight(height)
+    , fAccelData(NULL) {
+    this->needsNewGenID();
+
+    fPathHeap.reset(SkSafeRef(record.pathHeap()));
 
-    // FakeEndRecording is only called from partialReplay. For that use case
-    // we cannot be certain that the next call to SkWriter32::overwriteTAt
-    // will be preceded by an append (i.e., that the required copy on write
-    // will occur). In this case just force a deep copy of the operations.
-    const bool deepCopyOps = true;
-    return SkNEW_ARGS(SkPicturePlayback, (resourceSrc, record, info, deepCopyOps));
+    SkPictInfo info;
+    this->createHeader(&info);
+    fPlayback = SkNEW_ARGS(SkPicturePlayback, (this, record, info, deepCopyOps));
 }
 
 SkPicture::SkPicture(const SkPicture& src)
     : INHERITED()
-    , fAccelData(NULL)
-    , fContentInfo(src.fContentInfo) {
+    , fAccelData(NULL) {
     this->needsNewGenID();
     fWidth = src.fWidth;
     fHeight = src.fHeight;
@@ -209,7 +207,6 @@ void SkPicture::swap(SkPicture& other) {
     SkTSwap(fWidth, other.fWidth);
     SkTSwap(fHeight, other.fHeight);
     fPathHeap.swap(&other.fPathHeap);
-    fContentInfo.swap(&other.fContentInfo);
 }
 
 SkPicture* SkPicture::clone() const {
@@ -228,7 +225,6 @@ void SkPicture::clone(SkPicture* pictures, int count) const {
         clone->fWidth = fWidth;
         clone->fHeight = fHeight;
         SkDELETE(clone->fPlayback);
-        clone->fContentInfo.set(fContentInfo);
 
         /*  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
@@ -524,23 +520,14 @@ void SkPicture::flatten(SkWriteBuffer& buffer) const {
 
 #if SK_SUPPORT_GPU
 bool SkPicture::suitableForGpuRasterization(GrContext* context, const char **reason) const {
-    // TODO: the heuristic used here needs to be refined
-    static const int kNumPaintWithPathEffectUsesTol = 1;
-    static const int kNumAAConcavePaths = 5;
-
-    SkASSERT(this->numAAHairlineConcavePaths() <= this->numAAConcavePaths());
-
-    bool ret = this->numPaintWithPathEffectUses() < kNumPaintWithPathEffectUsesTol &&
-               (this->numAAConcavePaths()-this->numAAHairlineConcavePaths()) < kNumAAConcavePaths;
-    if (!ret && reason) {
-        if (this->numPaintWithPathEffectUses() >= kNumPaintWithPathEffectUsesTol)
-            *reason = "Too many path effects.";
-        else if ((this->numAAConcavePaths()-this->numAAHairlineConcavePaths()) >= kNumAAConcavePaths)
-            *reason = "Too many anti-aliased concave paths.";
-        else
-            *reason = "Unknown reason for GPU unsuitability.";
+    if (NULL == fPlayback) {
+        if (NULL != reason) {
+            *reason = "Missing playback object.";
+        }
+        return false;
     }
-    return ret;
+
+    return fPlayback->suitableForGpuRasterization(context, reason);
 }
 #endif
 
diff --git a/src/core/SkPicturePlayback.cpp b/src/core/SkPicturePlayback.cpp
index 3f83b6a9cc..aa2b021928 100644
--- a/src/core/SkPicturePlayback.cpp
+++ b/src/core/SkPicturePlayback.cpp
@@ -99,26 +99,16 @@ SkPicturePlayback::SkPicturePlayback(const SkPicture* picture,
     record.dumpPaints();
 #endif
 
-    record.validate(record.writeStream().bytesWritten(), 0);
-    const SkWriter32& writer = record.writeStream();
     this->init();
-    SkASSERT(!fOpData);
-    if (writer.bytesWritten() == 0) {
-        fOpData = SkData::NewEmpty();
-        return;
-    }
-    if (deepCopyOps) {
-        // Don't try to do anything clever w.r.t. copy on write
-        fOpData = SkData::NewWithCopy(writer.contiguousArray(), writer.bytesWritten());
-    } else {
-        fOpData = writer.snapshotAsData();
-    }
+
+    fOpData = record.opData(deepCopyOps);
 
     fBoundingHierarchy = record.fBoundingHierarchy;
     fStateTree = record.fStateTree;
 
     SkSafeRef(fBoundingHierarchy);
     SkSafeRef(fStateTree);
+    fContentInfo.set(record.fContentInfo);
 
     if (NULL != fBoundingHierarchy) {
         fBoundingHierarchy->flushDeferredInserts();
@@ -166,7 +156,8 @@ SkPicturePlayback::SkPicturePlayback(const SkPicture* picture,
 #endif
 }
 
-SkPicturePlayback::SkPicturePlayback(const SkPicture* picture, const SkPicturePlayback& src,
+SkPicturePlayback::SkPicturePlayback(const SkPicture* picture, 
+                                     const SkPicturePlayback& src,
                                      SkPictCopyInfo* deepCopyInfo)
     : fPicture(picture)
     , fInfo(src.fInfo) {
@@ -178,6 +169,7 @@ SkPicturePlayback::SkPicturePlayback(const SkPicture* picture, const SkPicturePl
 
     fBoundingHierarchy = src.fBoundingHierarchy;
     fStateTree = src.fStateTree;
+    fContentInfo.set(src.fContentInfo);
 
     SkSafeRef(fBoundingHierarchy);
     SkSafeRef(fStateTree);
@@ -1335,6 +1327,30 @@ void SkPicturePlayback::draw(SkCanvas& canvas, SkDrawPictureCallback* callback)
 //    this->dumpSize();
 }
 
+
+#if SK_SUPPORT_GPU
+bool SkPicturePlayback::suitableForGpuRasterization(GrContext* context, const char **reason) const {
+    // TODO: the heuristic used here needs to be refined
+    static const int kNumPaintWithPathEffectUsesTol = 1;
+    static const int kNumAAConcavePaths = 5;
+
+    SkASSERT(fContentInfo.numAAHairlineConcavePaths() <= fContentInfo.numAAConcavePaths());
+
+    bool ret = fContentInfo.numPaintWithPathEffectUses() < kNumPaintWithPathEffectUsesTol &&
+                    (fContentInfo.numAAConcavePaths() - fContentInfo.numAAHairlineConcavePaths()) 
+                    < kNumAAConcavePaths;
+    if (!ret && NULL != reason) {
+        if (fContentInfo.numPaintWithPathEffectUses() >= kNumPaintWithPathEffectUsesTol)
+            *reason = "Too many path effects.";
+        else if ((fContentInfo.numAAConcavePaths() - fContentInfo.numAAHairlineConcavePaths()) 
+                 >= kNumAAConcavePaths)
+            *reason = "Too many anti-aliased concave paths.";
+        else
+            *reason = "Unknown reason for GPU unsuitability.";
+    }
+    return ret;
+}
+#endif
 ///////////////////////////////////////////////////////////////////////////////
 
 #ifdef SK_DEBUG_SIZE
diff --git a/src/core/SkPicturePlayback.h b/src/core/SkPicturePlayback.h
index 9d4c996653..9751e3a5e7 100644
--- a/src/core/SkPicturePlayback.h
+++ b/src/core/SkPicturePlayback.h
@@ -62,6 +62,56 @@ struct SkPictInfo {
 // Always write this guy last (with no length field afterwards)
 #define SK_PICT_EOF_TAG     SkSetFourByteTag('e', 'o', 'f', ' ')
 
+// SkPictureContentInfo is not serialized! It is intended solely for use
+// with suitableForGpuRasterization.
+class SkPictureContentInfo {
+public:
+    SkPictureContentInfo() { this->reset(); }
+
+    SkPictureContentInfo(const SkPictureContentInfo& src) { this->set(src); }
+
+    void set(const SkPictureContentInfo& src) {
+        fNumPaintWithPathEffectUses = src.fNumPaintWithPathEffectUses;
+        fNumAAConcavePaths = src.fNumAAConcavePaths;
+        fNumAAHairlineConcavePaths = src.fNumAAHairlineConcavePaths;
+    }
+
+    void reset() {
+        fNumPaintWithPathEffectUses = 0;
+        fNumAAConcavePaths = 0;
+        fNumAAHairlineConcavePaths = 0;
+    }
+
+    void swap(SkPictureContentInfo* other) {
+        SkTSwap(fNumPaintWithPathEffectUses, other->fNumPaintWithPathEffectUses);
+        SkTSwap(fNumAAConcavePaths, other->fNumAAConcavePaths);
+        SkTSwap(fNumAAHairlineConcavePaths, other->fNumAAHairlineConcavePaths);
+    }
+
+    void incPaintWithPathEffectUses() { ++fNumPaintWithPathEffectUses; }
+    int numPaintWithPathEffectUses() const { return fNumPaintWithPathEffectUses; }
+
+    void incAAConcavePaths() { ++fNumAAConcavePaths; }
+    int numAAConcavePaths() const { return fNumAAConcavePaths; }
+
+    void incAAHairlineConcavePaths() {
+        ++fNumAAHairlineConcavePaths;
+        SkASSERT(fNumAAHairlineConcavePaths <= fNumAAConcavePaths);
+    }
+    int numAAHairlineConcavePaths() const { return fNumAAHairlineConcavePaths; }
+
+private:
+    // This field is incremented every time a paint with a path effect is
+    // used (i.e., it is not a de-duplicated count)
+    int fNumPaintWithPathEffectUses;
+    // This field is incremented every time an anti-aliased drawPath call is
+    // issued with a concave path
+    int fNumAAConcavePaths;
+    // This field is incremented every time a drawPath call is
+    // issued for a hairline stroked concave path.
+    int fNumAAHairlineConcavePaths;
+};
+
 /**
  * Container for data that is needed to deep copy a SkPicture. The container
  * enables the data to be generated once and reused for subsequent copies.
@@ -78,8 +128,8 @@ class SkPicturePlayback {
 public:
     SkPicturePlayback(const SkPicture* picture, const SkPicturePlayback& src,
                       SkPictCopyInfo* deepCopyInfo = NULL);
-    SkPicturePlayback(const SkPicture* picture, const SkPictureRecord& record, const SkPictInfo&,
-                      bool deepCopyOps);
+    SkPicturePlayback(const SkPicture* picture, const SkPictureRecord& record, 
+                      const SkPictInfo&, bool deepCopyOps);
     static SkPicturePlayback* CreateFromStream(SkPicture* picture,
                                                SkStream*,
                                                const SkPictInfo&,
@@ -222,6 +272,10 @@ public:
     void dump() const;
 #endif
 
+#if SK_SUPPORT_GPU
+    bool suitableForGpuRasterization(GrContext* context, const char **reason) const;
+#endif
+
 private:    // these help us with reading/writing
     bool parseStreamTag(SkPicture* picture, SkStream*, uint32_t tag, uint32_t size,
                         SkPicture::InstallPixelRefProc);
@@ -252,6 +306,8 @@ private:
     SkBBoxHierarchy* fBoundingHierarchy;
     SkPictureStateTree* fStateTree;
 
+    SkPictureContentInfo fContentInfo;
+
     // Limit the opcode playback to be between the offsets 'start' and 'stop'.
     // The opcode at 'start' should be a saveLayer while the opcode at
     // 'stop' should be a restore. Neither of those commands will be issued.
diff --git a/src/core/SkPictureRecord.cpp b/src/core/SkPictureRecord.cpp
index 1a1aecc600..5d2eca7a55 100644
--- a/src/core/SkPictureRecord.cpp
+++ b/src/core/SkPictureRecord.cpp
@@ -36,7 +36,7 @@ static const uint32_t kSaveSize = 2 * kUInt32Size;
 static const uint32_t kSaveLayerNoBoundsSize = 4 * kUInt32Size;
 static const uint32_t kSaveLayerWithBoundsSize = 4 * kUInt32Size + sizeof(SkRect);
 
-SkPictureRecord::SkPictureRecord(SkPicture* picture, const SkISize& dimensions, uint32_t flags)
+SkPictureRecord::SkPictureRecord(const SkISize& dimensions, uint32_t flags)
     : INHERITED(dimensions.width(), dimensions.height())
     , fBoundingHierarchy(NULL)
     , fStateTree(NULL)
@@ -49,7 +49,6 @@ SkPictureRecord::SkPictureRecord(SkPicture* picture, const SkISize& dimensions,
     fPointWrites = fRectWrites = fTextWrites = 0;
 #endif
 
-    fPicture = picture;
     fBitmapHeap = SkNEW(SkBitmapHeap);
     fFlattenableHeap.setBitmapStorage(fBitmapHeap);
 
@@ -1066,11 +1065,11 @@ void SkPictureRecord::onDrawDRRect(const SkRRect& outer, const SkRRect& inner,
 void SkPictureRecord::drawPath(const SkPath& path, const SkPaint& paint) {
 
     if (paint.isAntiAlias() && !path.isConvex()) {
-        fPicture->incAAConcavePaths();
+        fContentInfo.incAAConcavePaths();
 
         if (SkPaint::kStroke_Style == paint.getStyle() &&
             0 == paint.getStrokeWidth()) {
-            fPicture->incAAHairlineConcavePaths();
+            fContentInfo.incAAHairlineConcavePaths();
         }
     }
 
@@ -1589,7 +1588,7 @@ const SkFlatData* SkPictureRecord::getFlatPaintData(const SkPaint& paint) {
 
 const SkFlatData* SkPictureRecord::addPaintPtr(const SkPaint* paint) {
     if (NULL != paint && NULL != paint->getPathEffect()) {
-        fPicture->incPaintWithPathEffectUses();
+        fContentInfo.incPaintWithPathEffectUses();
     }
 
     const SkFlatData* data = paint ? getFlatPaintData(*paint) : NULL;
@@ -1603,7 +1602,14 @@ void SkPictureRecord::addFlatPaint(const SkFlatData* flatPaint) {
 }
 
 int SkPictureRecord::addPathToHeap(const SkPath& path) {
-    return fPicture->addPathToHeap(path);
+    if (NULL == fPathHeap) {
+        fPathHeap.reset(SkNEW(SkPathHeap));
+    }
+#ifdef SK_DEDUP_PICTURE_PATHS
+    return fPathHeap->insert(path);
+#else
+    return fPathHeap->append(path);
+#endif
 }
 
 void SkPictureRecord::addPath(const SkPath& path) {
diff --git a/src/core/SkPictureRecord.h b/src/core/SkPictureRecord.h
index d6cdf05f28..b4920e37bb 100644
--- a/src/core/SkPictureRecord.h
+++ b/src/core/SkPictureRecord.h
@@ -15,6 +15,7 @@
 #endif
 #include "SkPathHeap.h"
 #include "SkPicture.h"
+#include "SkPicturePlayback.h"
 #include "SkPictureFlat.h"
 #include "SkTemplates.h"
 #include "SkWriter32.h"
@@ -33,7 +34,7 @@ class SkPictureStateTree;
 
 class SkPictureRecord : public SkCanvas {
 public:
-    SkPictureRecord(SkPicture* picture, const SkISize& dimensions, uint32_t recordFlags);
+    SkPictureRecord(const SkISize& dimensions, uint32_t recordFlags);
     virtual ~SkPictureRecord();
 
     virtual void clear(SkColor) SK_OVERRIDE;
@@ -73,6 +74,28 @@ public:
         return fPictureRefs;
     }
 
+    SkData* opData(bool deepCopy) const {
+        this->validate(fWriter.bytesWritten(), 0);
+
+        if (fWriter.bytesWritten() == 0) {
+            return SkData::NewEmpty();
+        }
+
+        if (deepCopy) {
+            return SkData::NewWithCopy(fWriter.contiguousArray(), fWriter.bytesWritten());
+        }
+
+        return fWriter.snapshotAsData();
+    }
+
+    SkPathHeap* pathHeap() {
+        return fPathHeap;
+    }
+
+    const SkPictureContentInfo& contentInfo() const {
+        return fContentInfo;
+    }
+
     void setFlags(uint32_t recordFlags) {
         fRecordFlags = recordFlags;
     }
@@ -283,12 +306,12 @@ protected:
     SkBitmapHeap* fBitmapHeap;
 
 private:
-    // The owning SkPicture
-    SkPicture* fPicture;
-
     friend class MatrixClipState; // for access to *Impl methods
     friend class SkMatrixClipStateMgr; // for access to *Impl methods
 
+    SkPictureContentInfo fContentInfo;
+    SkAutoTUnref<SkPathHeap> fPathHeap;
+
     SkChunkFlatController fFlattenableHeap;
 
     SkPaintDictionary fPaints;
diff --git a/src/core/SkPictureRecorder.cpp b/src/core/SkPictureRecorder.cpp
index 863dbcda90..3abd08e1b0 100644
--- a/src/core/SkPictureRecorder.cpp
+++ b/src/core/SkPictureRecorder.cpp
@@ -18,20 +18,19 @@ SkPictureRecorder::~SkPictureRecorder() {
 SkCanvas* SkPictureRecorder::beginRecording(int width, int height,
                                             SkBBHFactory* bbhFactory /* = NULL */,
                                             uint32_t recordFlags /* = 0 */) {
-    SkSafeSetNull(fCanvas);
-    fPicture.reset(SkNEW(SkPicture));
+    SkSafeSetNull(fCanvas); // terminate any prior recording(s)
 
-    fPicture->fWidth = width;
-    fPicture->fHeight = height;
+    fWidth = width;
+    fHeight = height;
 
     const SkISize size = SkISize::Make(width, height);
 
     if (NULL != bbhFactory) {
         SkAutoTUnref<SkBBoxHierarchy> tree((*bbhFactory)(width, height));
         SkASSERT(NULL != tree);
-        fCanvas = SkNEW_ARGS(SkBBoxHierarchyRecord, (fPicture, size, recordFlags, tree.get()));
+        fCanvas = SkNEW_ARGS(SkBBoxHierarchyRecord, (size, recordFlags, tree.get()));
     } else {
-        fCanvas = SkNEW_ARGS(SkPictureRecord, (fPicture, size, recordFlags));
+        fCanvas = SkNEW_ARGS(SkPictureRecord, (size, recordFlags));
     }
 
     fCanvas->beginRecording();
@@ -44,23 +43,18 @@ SkCanvas* SkPictureRecorder::getRecordingCanvas() {
 }
 
 SkPicture* SkPictureRecorder::endRecording() {
-    if (NULL == fPicture.get()) {
+    if (NULL == fCanvas) {
         return NULL;
     }
 
-    SkASSERT(NULL == fPicture->fPlayback);
-    SkASSERT(NULL != fCanvas);
-
     fCanvas->endRecording();
 
-    SkPictInfo info;
-    fPicture->createHeader(&info);
     const bool deepCopyOps = false;
-    fPicture->fPlayback = SkNEW_ARGS(SkPicturePlayback, (fPicture, *fCanvas, info, deepCopyOps));
-
+    SkAutoTUnref<SkPicture> picture(SkNEW_ARGS(SkPicture, (fWidth, fHeight, 
+                                                           *fCanvas, deepCopyOps)));
     SkSafeSetNull(fCanvas);
 
-    return fPicture.detach();
+    return picture.detach();
 }
 
 void SkPictureRecorder::internalOnly_EnableOpts(bool enableOpts) {
@@ -70,14 +64,13 @@ void SkPictureRecorder::internalOnly_EnableOpts(bool enableOpts) {
 }
 
 void SkPictureRecorder::partialReplay(SkCanvas* canvas) const {
-    if (NULL == fPicture.get() || NULL == canvas) {
+    if (NULL == fCanvas || NULL == canvas) {
         // Not recording or nothing to replay into
         return;
     }
 
-    SkASSERT(NULL != fCanvas);
-
-    SkAutoTDelete<SkPicturePlayback> playback(SkPicture::FakeEndRecording(fPicture.get(),
-                                                                          *fCanvas));
-    playback->draw(*canvas, NULL);
+    const bool deepCopyOps = true;
+    SkAutoTUnref<SkPicture> picture(SkNEW_ARGS(SkPicture, (fWidth, fHeight, 
+                                                           *fCanvas, deepCopyOps)));
+    picture->draw(canvas);
 }