MultiPictureDraw is taskgroup aware.

src/core/SkMultiPictureDraw.cpp

Index: src/core/SkMultiPictureDraw.cpp
diff --git a/src/core/SkMultiPictureDraw.cpp b/src/core/SkMultiPictureDraw.cpp
index 5fe3c0ea5210aa0a5969e7afb97143991bd7a37d..493196d0adba7bc7b35cfbbf8a974376e9898f46 100644
--- a/src/core/SkMultiPictureDraw.cpp
+++ b/src/core/SkMultiPictureDraw.cpp
@@ -14,20 +14,41 @@
 #include "SkMultiPictureDraw.h"
 #include "SkPicture.h"
 
+void SkMultiPictureDraw::DrawData::init(SkCanvas* canvas, const SkPicture* picture,
+                                        const SkMatrix* matrix, const SkPaint* paint) {
+    fPicture = SkRef(picture);
+    fCanvas = SkRef(canvas);
+    if (matrix) {
+        fMatrix = *matrix;
+    } else {
+        fMatrix.setIdentity();
+    }
+    if (paint) {
+        fPaint = SkNEW_ARGS(SkPaint, (*paint));
+    } else {
+        fPaint = NULL;
+    }
+}
+
+void SkMultiPictureDraw::DrawData::Reset(SkTDArray<DrawData>& data) {
+    for (int i = 0; i < data.count(); ++i) {
+        data[i].fPicture->unref();
+        data[i].fCanvas->unref();
+        SkDELETE(data[i].fPaint);
+    }
+    data.rewind();
+}
+
 SkMultiPictureDraw::SkMultiPictureDraw(int reserve) {
     if (reserve > 0) {
-        fDrawData.setReserve(reserve);
+        fGPUDrawData.setReserve(reserve);
+        fThreadSafeDrawData.setReserve(reserve);
     }
 }
 
 void SkMultiPictureDraw::reset() {
-    for (int i = 0; i < fDrawData.count(); ++i) {
-        fDrawData[i].picture->unref();
-        fDrawData[i].canvas->unref();
-        SkDELETE(fDrawData[i].paint);
-    }
-
-    fDrawData.rewind();
+    fGPUDrawData.reset();
+    fThreadSafeDrawData.reset();
 }
 
 void SkMultiPictureDraw::add(SkCanvas* canvas, 
@@ -39,47 +60,88 @@ void SkMultiPictureDraw::add(SkCanvas* canvas,
         return;
     }
 
-    DrawData* data = fDrawData.append();
-
-    data->picture = SkRef(picture);
-    data->canvas = SkRef(canvas);
-    if (matrix) {
-        data->matrix = *matrix;
-    } else {
-        data->matrix.setIdentity();
-    }
-    if (paint) {
-        data->paint = SkNEW_ARGS(SkPaint, (*paint));
-    } else {
-        data->paint = NULL;
-    }
+    SkTDArray<DrawData>& array = canvas->getGrContext() ? fGPUDrawData : fThreadSafeDrawData;
+    array.append()->init(canvas, picture, matrix, paint);
 }
 
 #undef SK_IGNORE_GPU_LAYER_HOISTING
 #define SK_IGNORE_GPU_LAYER_HOISTING 1
 
+#include "SkTaskGroup.h"
+
+struct PictureDrawRunnable : public SkRunnable {
+    SkCanvas*        fCanvas;
+    const SkPicture* fPicture;
+    const SkMatrix*  fMatrix;
+    const SkPaint*   fPaint;
+    
+    PictureDrawRunnable* init(SkCanvas* canvas, const SkPicture* picture, const SkMatrix* matrix,
+                              const SkPaint* paint) {
+        // no need to ref/copy these, as the caller ensures they all survive our task.
+        fCanvas = canvas;
+        fPicture = picture;
+        fMatrix = matrix;
+        fPaint = paint;
+        return this;
+    }
+
+    virtual void run() SK_OVERRIDE {
+        fCanvas->drawPicture(fPicture, fMatrix, fPaint);
+    }
+};
+
+class AutoMPDReset : SkNoncopyable {
+    SkMultiPictureDraw* fMPD;
+public:
+    AutoMPDReset(SkMultiPictureDraw* mpd) : fMPD(mpd) {}
+    ~AutoMPDReset() { fMPD->reset(); }
+};
+
 void SkMultiPictureDraw::draw() {
+    AutoMPDReset mpdreset(this);
+    // we place the taskgroup after the MPDReset, to ensure that we don't delete the DrawData
+    // objects until after we're finished the tasks (which have pointers to the data).

[mtklein, 2014/10/29 14:29:03]

If we're worried about scope timing, can't hurt to scope the SkTaskGroup in a
block, or make an explicit call to wait().

[reed1, 2014/10/29 17:05:11]

deliberately want to not force the wait until the gpu work was done.

+    SkTaskGroup group;
+
+    // Queue up the non-gpu tasks first, so they can execute in parallel while we then
+    // handle the gpu tasks.
+    if (fThreadSafeDrawData.count() > 0) {

[mtklein, 2014/10/29 14:29:03]

My instinct is to drop these extra checks (this and 0 == count below) until a
profile wants us to put them back.  The code should all work fine without them.

[reed1, 2014/10/29 17:05:11]

Done.

+        const int count = fThreadSafeDrawData.count();
+        SkAutoSTArray<32, PictureDrawRunnable> pdr(count);
+        for (int i = 0; i < count; ++i) {
+            const DrawData& data = fThreadSafeDrawData[i];
+            group.add(pdr[i].init(data.fCanvas, data.fPicture, &data.fMatrix, data.fPaint));
+        }
+    }
+
+    // Enter the GPU-only section
+
+    const int count = fGPUDrawData.count();
+    if (0 == count) {
+        return;
+    }
 
 #ifndef SK_IGNORE_GPU_LAYER_HOISTING
-    GrContext* context = NULL;
+    GrContext* context = fGPUDrawData[0].fCanvas->getGrContext();
+    SkASSERT(context);
 
-    // Start by collecting all the layers that are going to be atlased and render 
+    // Start by collecting all the layers that are going to be atlased and render
     // them (if necessary). Hoisting the free floating layers is deferred until
     // drawing the canvas that requires them.
     SkTDArray<GrHoistedLayer> atlasedNeedRendering, atlasedRecycled;
 
-    for (int i = 0; i < fDrawData.count(); ++i) {
-        if (fDrawData[i].canvas->getGrContext() &&
-            !fDrawData[i].paint && fDrawData[i].matrix.isIdentity()) {
-            SkASSERT(NULL == context || context == fDrawData[i].canvas->getGrContext());
-            context = fDrawData[i].canvas->getGrContext();
+    for (int i = 0; i < count; ++i) {
+        const DrawData& data = fGPUDrawData[i];
+        // we only expect 1 context for all the canvases
+        SkASSERT(data.canvas->getGrContext() == context);
 
+        if (!data.fPaint && data.fMatrix.isIdentity()) {
             // TODO: this path always tries to optimize pictures. Should we
             // switch to this API approach (vs. SkCanvas::EXPERIMENTAL_optimize)?
-            fDrawData[i].canvas->EXPERIMENTAL_optimize(fDrawData[i].picture);
+            data.fCanvas->EXPERIMENTAL_optimize(data.fPicture);
 
             SkRect clipBounds;
-            if (!fDrawData[i].canvas->getClipBounds(&clipBounds)) {
+            if (!data.fCanvas->getClipBounds(&clipBounds)) {
                 continue;
             }
 
@@ -87,32 +149,33 @@ void SkMultiPictureDraw::draw() {
             // would improve the packing and reduce the number of swaps
             // TODO: another optimization would be to make a first pass to
             // lock any required layer that is already in the atlas
-            GrLayerHoister::FindLayersToAtlas(context, fDrawData[i].picture,
+            GrLayerHoister::FindLayersToAtlas(context, data.fPicture,
                                               clipBounds, 
                                               &atlasedNeedRendering, &atlasedRecycled);
         }
     }
 
-    if (NULL != context) {
-        GrLayerHoister::DrawLayersToAtlas(context, atlasedNeedRendering);
-    }
+    GrLayerHoister::DrawLayersToAtlas(context, atlasedNeedRendering);
 
     SkTDArray<GrHoistedLayer> needRendering, recycled;
 #endif
 
-    for (int i = 0; i < fDrawData.count(); ++i) {
+    for (int i = 0; i < count; ++i) {
+        const DrawData& data = fGPUDrawData[i];
+        SkCanvas* canvas = data.fCanvas;
+        const SkPicture* picture = data.fPicture;
+
 #ifndef SK_IGNORE_GPU_LAYER_HOISTING
-        if (fDrawData[i].canvas->getGrContext() && 
-            !fDrawData[i].paint && fDrawData[i].matrix.isIdentity()) {
+        if (!data.fPaint && data.fMatrix.isIdentity()) {
 
             SkRect clipBounds;
-            if (!fDrawData[i].canvas->getClipBounds(&clipBounds)) {
+            if (!canvas->getClipBounds(&clipBounds)) {
                 continue;
             }
 
             // Find the layers required by this canvas. It will return atlased
             // layers in the 'recycled' list since they have already been drawn.
-            GrLayerHoister::FindLayersToHoist(context, fDrawData[i].picture,
+            GrLayerHoister::FindLayersToHoist(context, picture,
                                               clipBounds, &needRendering, &recycled);
 
             GrLayerHoister::DrawLayers(context, needRendering);
@@ -122,11 +185,10 @@ void SkMultiPictureDraw::draw() {
             GrLayerHoister::ConvertLayersToReplacements(needRendering, &replacements);
             GrLayerHoister::ConvertLayersToReplacements(recycled, &replacements);
 
-            const SkMatrix initialMatrix = fDrawData[i].canvas->getTotalMatrix();
+            const SkMatrix initialMatrix = canvas->getTotalMatrix();
 
             // Render the entire picture using new layers
-            GrRecordReplaceDraw(fDrawData[i].picture, fDrawData[i].canvas, 
-                                &replacements, initialMatrix, NULL);
+            GrRecordReplaceDraw(picture, canvas, &replacements, initialMatrix, NULL);
 
             GrLayerHoister::UnlockLayers(context, needRendering);
             GrLayerHoister::UnlockLayers(context, recycled);
@@ -136,19 +198,13 @@ void SkMultiPictureDraw::draw() {
         } else
 #endif
         {
-            fDrawData[i].canvas->drawPicture(fDrawData[i].picture,
-                                             &fDrawData[i].matrix,
-                                             fDrawData[i].paint);
+            canvas->drawPicture(picture, &data.fMatrix, data.fPaint);
         }
     }
 
 #ifndef SK_IGNORE_GPU_LAYER_HOISTING
-    if (NULL != context) {
-        GrLayerHoister::UnlockLayers(context, atlasedNeedRendering);
-        GrLayerHoister::UnlockLayers(context, atlasedRecycled);
-    }
+    GrLayerHoister::UnlockLayers(context, atlasedNeedRendering);
+    GrLayerHoister::UnlockLayers(context, atlasedRecycled);
 #endif
-
-    this->reset();
 }