add heuristic to pour small pictures into recordings, rather than ref'ing

BUG=skia:

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

diff --git a/include/core/SkCanvas.h b/include/core/SkCanvas.h
index dcbff3f60b6030f619358539d0f2b798dc0d6416..9282267721eff192e66efd7d2cf2ace63cb9a9e0 100644 (file)
--- a/include/core/SkCanvas.h
+++ b/include/core/SkCanvas.h
@@ -951,7 +951,9 @@ public:
         @param picture The recorded drawing commands to playback into this
                        canvas.
     */
-    void drawPicture(const SkPicture* picture);
+    void drawPicture(const SkPicture* picture) {
+        this->drawPicture(picture, NULL, NULL);
+    }
 
     /**
      *  Draw the picture into this canvas.

diff --git a/src/core/SkCanvas.cpp b/src/core/SkCanvas.cpp
index 95f9560f1fe2004db03ee12aa65017f94f830310..c87c6c5f7c1bfa1a6cfee06c5b93d82a6ab63cae 100644 (file)
--- a/src/core/SkCanvas.cpp
+++ b/src/core/SkCanvas.cpp
@@ -2508,20 +2508,28 @@ void SkCanvas::drawTextOnPathHV(const void* text, size_t byteLength,
 }
 
 ///////////////////////////////////////////////////////////////////////////////
-void SkCanvas::drawPicture(const SkPicture* picture) {
-    TRACE_EVENT0("disabled-by-default-skia", "SkCanvas::drawPicture()");
-    if (picture) {
-        this->onDrawPicture(picture, NULL, NULL);
-    }
-}
+
+/**
+ *  This constant is trying to balance the speed of ref'ing a subpicture into a parent picture,
+ *  against the playback cost of recursing into the subpicture to get at its actual ops.
+ *
+ *  For now we pick a conservatively small value, though measurement (and other heuristics like
+ *  the type of ops contained) may justify changing this value.
+ */
+#define kMaxPictureOpsToUnrollInsteadOfRef  1
 
 void SkCanvas::drawPicture(const SkPicture* picture, const SkMatrix* matrix, const SkPaint* paint) {
-    TRACE_EVENT0("disabled-by-default-skia", "SkCanvas::drawPicture(SkMatrix, SkPaint)");
+    TRACE_EVENT0("disabled-by-default-skia", "SkCanvas::drawPicture()");
     if (picture) {
         if (matrix && matrix->isIdentity()) {
             matrix = NULL;
         }
-        this->onDrawPicture(picture, matrix, paint);
+        if (picture->approximateOpCount() <= kMaxPictureOpsToUnrollInsteadOfRef) {
+            SkAutoCanvasMatrixPaint acmp(this, matrix, paint, picture->cullRect());
+            picture->playback(this);
+        } else {
+            this->onDrawPicture(picture, matrix, paint);
+        }
     }
 }
 
@@ -2537,7 +2545,6 @@ void SkCanvas::onDrawPicture(const SkPicture* picture, const SkMatrix* matrix,
     }
 
     SkAutoCanvasMatrixPaint acmp(this, matrix, paint, picture->cullRect());
-
     picture->playback(this);
 }
 

diff --git a/tests/PictureTest.cpp b/tests/PictureTest.cpp
index 4fe08379dd0545e8648f660fb514d28d8098f525..16d98b324517d670339181410e8892deb53ea898 100644 (file)
--- a/tests/PictureTest.cpp
+++ b/tests/PictureTest.cpp
@@ -1127,7 +1127,7 @@ static void test_bytes_used(skiatest::Reporter* reporter) {
     r2.getRecordingCanvas()->drawPicture(empty.get());
     SkAutoTUnref<SkPicture> nested(r2.endRecording());
 
-    REPORTER_ASSERT(reporter, SkPictureUtils::ApproximateBytesUsed(nested.get()) >
+    REPORTER_ASSERT(reporter, SkPictureUtils::ApproximateBytesUsed(nested.get()) >=
                               SkPictureUtils::ApproximateBytesUsed(empty.get()));
 }
 

diff --git a/tests/RecorderTest.cpp b/tests/RecorderTest.cpp
index 4fac1af58c274ed994619503c4a97f766972897d..e67de58acbd73a15964ae03cad27c4d6ddc5eb2f 100644 (file)
--- a/tests/RecorderTest.cpp
+++ b/tests/RecorderTest.cpp
@@ -89,28 +89,6 @@ DEF_TEST(Recorder_RefLeaking, r) {
     REPORTER_ASSERT(r, paint.getShader()->unique());
 }
 
-DEF_TEST(Recorder_RefPictures, r) {
-    SkAutoTUnref<SkPicture> pic;
-
-    {
-        SkPictureRecorder pr;
-        SkCanvas* canvas = pr.beginRecording(100, 100);
-        canvas->drawColor(SK_ColorRED);
-        pic.reset(pr.endRecording());
-    }
-    REPORTER_ASSERT(r, pic->unique());
-
-    {
-        SkRecord record;
-        SkRecorder recorder(&record, 100, 100);
-        recorder.drawPicture(pic);
-        // the recorder should now also be an owner
-        REPORTER_ASSERT(r, !pic->unique());
-    }
-    // the recorder destructor should have released us (back to unique)
-    REPORTER_ASSERT(r, pic->unique());
-}
-
 DEF_TEST(Recorder_drawImage_takeReference, reporter) {
 
     SkAutoTUnref<SkImage> image;