From 31fee3a24442e42c09f90893702e1e210495166d Mon Sep 17 00:00:00 2001
From: Nathaniel Nifong <nifong@google.com>
Date: Thu, 11 Jul 2019 16:27:14 -0400
Subject: [PATCH] Resolve drawable when playing back DisplayListData in the
 RenderThread.

Drawables are mutable, and not expected to produce the same content if retained for longer than the
duration of the frame. This becomes relevant when recording multi-frame SKPs because the
serialization step forces drawables to be played back long after the frame completed. By resolving
the drawable when we are drawing the frame we avoid ref'ing the SkDrawable and do not extend its
lifetime beyond the frame.

Bug: skia:9234
Test: Captured SKPs on a Pixel 3
Change-Id: I5d317dd14e20be9d5e18675df8667327bd05aff0
---
 libs/hwui/RecordingCanvas.cpp                    |  7 ++++++-
 libs/hwui/pipeline/skia/SkiaPipeline.cpp         |  3 +--
 libs/hwui/tests/unit/RenderNodeDrawableTests.cpp | 10 ++++------
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/libs/hwui/RecordingCanvas.cpp b/libs/hwui/RecordingCanvas.cpp
index e58fbbe8e6673..c8eb1ca559107 100644
--- a/libs/hwui/RecordingCanvas.cpp
+++ b/libs/hwui/RecordingCanvas.cpp
@@ -274,7 +274,12 @@ struct DrawDrawable final : Op {
     }
     sk_sp<SkDrawable> drawable;
     SkMatrix matrix = SkMatrix::I();
-    void draw(SkCanvas* c, const SkMatrix&) const { c->drawDrawable(drawable.get(), &matrix); }
+    // It is important that we call drawable->draw(c) here instead of c->drawDrawable(drawable).
+    // Drawables are mutable and in cases, like RenderNodeDrawable, are not expected to produce the
+    // same content if retained outside the duration of the frame. Therefore we resolve
+    // them now and do not allow the canvas to take a reference to the drawable and potentially
+    // keep it alive for longer than the frames duration (e.g. SKP serialization).
+    void draw(SkCanvas* c, const SkMatrix&) const { drawable->draw(c, &matrix); }
 };
 struct DrawPicture final : Op {
     static const auto kType = Type::DrawPicture;
diff --git a/libs/hwui/pipeline/skia/SkiaPipeline.cpp b/libs/hwui/pipeline/skia/SkiaPipeline.cpp
index 84c0d1369e835..530926bf8dd01 100644
--- a/libs/hwui/pipeline/skia/SkiaPipeline.cpp
+++ b/libs/hwui/pipeline/skia/SkiaPipeline.cpp
@@ -313,8 +313,7 @@ SkCanvas* SkiaPipeline::tryCapture(SkSurface* surface) {
         case CaptureMode::CallbackAPI:
         case CaptureMode::SingleFrameSKP:
             mRecorder.reset(new SkPictureRecorder());
-            pictureCanvas = mRecorder->beginRecording(surface->width(), surface->height(),
-                nullptr, SkPictureRecorder::kPlaybackDrawPicture_RecordFlag);
+            pictureCanvas = mRecorder->beginRecording(surface->width(), surface->height());
             break;
         case CaptureMode::MultiFrameSKP:
             // If a multi frame recording is active, initialize recording for a single frame of a
diff --git a/libs/hwui/tests/unit/RenderNodeDrawableTests.cpp b/libs/hwui/tests/unit/RenderNodeDrawableTests.cpp
index c813cd945905b..e70378bd15a59 100644
--- a/libs/hwui/tests/unit/RenderNodeDrawableTests.cpp
+++ b/libs/hwui/tests/unit/RenderNodeDrawableTests.cpp
@@ -1096,10 +1096,8 @@ TEST(ReorderBarrierDrawable, testShadowMatrix) {
         int getDrawCounter() { return mDrawCounter; }
 
         virtual void onDrawDrawable(SkDrawable* drawable, const SkMatrix* matrix) override {
-            // expect to draw 2 RenderNodeDrawable, 1 StartReorderBarrierDrawable,
-            // 1 EndReorderBarrierDrawable
-            mDrawCounter++;
-            SkCanvas::onDrawDrawable(drawable, matrix);
+            // Do not expect this to be called. See RecordingCanvas.cpp DrawDrawable for context.
+            EXPECT_TRUE(false);
         }
 
         virtual void didTranslate(SkScalar dx, SkScalar dy) override {
@@ -1159,8 +1157,8 @@ TEST(ReorderBarrierDrawable, testShadowMatrix) {
     // create a canvas not backed by any device/pixels, but with dimensions to avoid quick rejection
     ShadowTestCanvas canvas(CANVAS_WIDTH, CANVAS_HEIGHT);
     RenderNodeDrawable drawable(parent.get(), &canvas, false);
-    canvas.drawDrawable(&drawable);
-    EXPECT_EQ(9, canvas.getDrawCounter());
+    drawable.draw(&canvas);
+    EXPECT_EQ(5, canvas.getDrawCounter());
 }
 
 // Draw a vector drawable twice but with different bounds and verify correct bounds are used.