Switching Chromium to use new Skia SkPictureRecorder API

Switching Chromium to use new Skia SkPictureRecorder API

The Skia SkPicture recording API is changing to make it
clearer when the SkPicture is valid. In the new API
SkPictureRecorder::endRecording returns the new SkPicture.

This isn't landable until Skia DEPS rolls past r14171.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264243

[reed1, 2014-04-14 15:19:02]

https://codereview.chromium.org/236633008/diff/1/cc/resources/skpicture_conte...
File cc/resources/skpicture_content_layer_updater.cc (right):

https://codereview.chromium.org/236633008/diff/1/cc/resources/skpicture_conte...
cc/resources/skpicture_content_layer_updater.cc:30: SkCanvas* canvas =
picture_recorder_.beginRecording(content_rect.width(),
Why does picture_recorder_ have to be a persistent object, instead of just on
the stack?

[robertphillips, 2014-04-14 15:36:28]

https://codereview.chromium.org/236633008/diff/1/cc/resources/skpicture_conte...
File cc/resources/skpicture_content_layer_updater.cc (right):

https://codereview.chromium.org/236633008/diff/1/cc/resources/skpicture_conte...
cc/resources/skpicture_content_layer_updater.cc:30: SkCanvas* canvas =
picture_recorder_.beginRecording(content_rect.width(),
On 2014/04/14 15:19:02, reed1 wrote:
> Why does picture_recorder_ have to be a persistent object, instead of just on
> the stack?

Done.

[tomhudson, 2014-04-14 15:37:52]

On 2014/04/14 15:19:02, reed1 wrote:
>
https://codereview.chromium.org/236633008/diff/1/cc/resources/skpicture_conte...
> File cc/resources/skpicture_content_layer_updater.cc (right):
> 
>
https://codereview.chromium.org/236633008/diff/1/cc/resources/skpicture_conte...
> cc/resources/skpicture_content_layer_updater.cc:30: SkCanvas* canvas =
> picture_recorder_.beginRecording(content_rect.width(),
> Why does picture_recorder_ have to be a persistent object, instead of just on
> the stack?

Guesswork: picture_recorder_ is initialized to hold a factory?
Then again, perhaps we could keep a factory around, instead of the picture
recorder?
(Or pass in the factory, but it might be awkward to plumb through...)

[robertphillips, 2014-04-14 15:44:35]

It was just an oversight on my part. The class was holding a vanilla SkPicture
so it should be okay that a default SkPictureRecorder is created on the stack.

[enne (OOO), 2014-04-15 01:10:39]

Could you format your change description to 72 characters max?

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:258: canvas = skia::SharePtr(recorder.beginRecording(
Should all the beginRecording calls be wrapped in RefPtr+SharePtr (or none of
them)?

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:266: // Already setup for normal recording
Why did the code move out of this switch?

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:297: picture_ = skia::AdoptRef(recorder.endRecording());
Previously, picture_ was always non-null if the cc::Picture had been recorded. 
Are there any other NULL checks that need to be made elsewhere if this is now
optional?

https://codereview.chromium.org/236633008/diff/20001/cc/resources/skpicture_c...
File cc/resources/skpicture_content_layer_updater.h (right):

https://codereview.chromium.org/236633008/diff/20001/cc/resources/skpicture_c...
cc/resources/skpicture_content_layer_updater.h:36: // Recorded picture
You could probably just remove this comment.

[robertphillips, 2014-04-15 15:37:04]

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:258: canvas = skia::SharePtr(recorder.beginRecording(
As is, I think the behavior is correct here but mainly b.c. of the need to free
the NullCanvas. If not for that requirement, I think the canvas returned from
beginRecording shouldn't be reffed here at all. Note that there is some
discussion about make SkCanvas non-ref-counted which would resolve this issue.

Perhaps I misunderstood your question though?

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:266: // Already setup for normal recording
The old code always generated an SkPicture (albeit empty in 2 of the cases). To
always get an SkPicture with the new API one needs to always call beginRecording
and endRecording.

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:297: picture_ = skia::AdoptRef(recorder.endRecording());
picture_ should still always be non-null after a call to Picture::Record.

https://codereview.chromium.org/236633008/diff/20001/cc/resources/skpicture_c...
File cc/resources/skpicture_content_layer_updater.h (right):

https://codereview.chromium.org/236633008/diff/20001/cc/resources/skpicture_c...
cc/resources/skpicture_content_layer_updater.h:36: // Recorded picture
On 2014/04/15 01:10:39, enne wrote:
> You could probably just remove this comment.

Done.

[enne (OOO), 2014-04-15 16:45:16]

Thanks for all the explanation.  :)

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:258: canvas = skia::SharePtr(recorder.beginRecording(
On 2014/04/15 15:37:04, robertphillips wrote:
> As is, I think the behavior is correct here but mainly b.c. of the need to
free
> the NullCanvas. If not for that requirement, I think the canvas returned from
> beginRecording shouldn't be reffed here at all. Note that there is some
> discussion about make SkCanvas non-ref-counted which would resolve this issue.
> 
> Perhaps I misunderstood your question though?

Gotcha.  The ref here is just RAII.  Makes sense.

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:266: // Already setup for normal recording
On 2014/04/15 15:37:04, robertphillips wrote:
> The old code always generated an SkPicture (albeit empty in 2 of the cases).
To
> always get an SkPicture with the new API one needs to always call
beginRecording
> and endRecording.

I guess I'm asking because in all other cases, the canvas gets clobbered.  So,
RECORD_WITH_PAINTING_DISABLED doesn't need that code above (because it deletes
that canvas) and RECORD_WITH_SK_NULL_CANVAS doesn't either because it creates a
new canvas (and also gets an endRecording call but no beginRecording? Is that
legit?).

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:297: picture_ = skia::AdoptRef(recorder.endRecording());
On 2014/04/15 15:37:04, robertphillips wrote:
> picture_ should still always be non-null after a call to Picture::Record.

Oh, ok! I misunderstood the change description here and thought that maybe an
invalid picture meant this could return NULL.  (Are there any cases it can
return NULL? Should this DCHECK?)

[robertphillips, 2014-04-15 19:17:32]

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:266: // Already setup for normal recording
The RECORD_WITH_PAINTING_DISABLED and RECORD_WITH_SK_NULL_CANVAS cases are just
playing a game with where the drawing calls get directed. In all three cases we
want an SkPicture at the end so we need to call (in the new system)
beginRecording and endRecording. The switching of the canvas just controls
whether the drawing calls actually end up in the picture, pass through a Null
Canvas or are completely ignored.

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:297: picture_ = skia::AdoptRef(recorder.endRecording());
I have added a DCHECK.

[enne (OOO), 2014-04-15 19:43:27]

lgtm

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/236633008/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:266: // Already setup for normal recording
On 2014/04/15 19:17:32, robertphillips wrote:
> The RECORD_WITH_PAINTING_DISABLED and RECORD_WITH_SK_NULL_CANVAS cases are
just
> playing a game with where the drawing calls get directed. In all three cases
we
> want an SkPicture at the end so we need to call (in the new system)
> beginRecording and endRecording. The switching of the canvas just controls
> whether the drawing calls actually end up in the picture, pass through a Null
> Canvas or are completely ignored.

I see.  So even though you delete the canvas that comes back from
beginRecording, you still have to make that call in all cases.  Thanks for all
the explanation.  :)

[robertphillips, 2014-04-16 11:44:20]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-04-16 11:44:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertphillips@google.com/236633008/60001

[commit-bot: I haz the power, 2014-04-16 11:44:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-16 11:44:52]

Failed to apply patch for cc/resources/picture_pile_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/resources/picture_pile_impl.cc
  Hunk #1 FAILED at 333.
  1 out of 1 hunk FAILED -- saving rejects to file
cc/resources/picture_pile_impl.cc.rej

Patch:       cc/resources/picture_pile_impl.cc
Index: cc/resources/picture_pile_impl.cc
diff --git a/cc/resources/picture_pile_impl.cc
b/cc/resources/picture_pile_impl.cc
index
3f8e167b3c5d067e2531401ba21da2698691604c..0ecdba1c6b9fbb562b4a3b5a11c125b9628698c8
100644
--- a/cc/resources/picture_pile_impl.cc
+++ b/cc/resources/picture_pile_impl.cc
@@ -333,18 +333,14 @@ skia::RefPtr<SkPicture>
PicturePileImpl::GetFlattenedPicture() {
   TRACE_EVENT0("cc", "PicturePileImpl::GetFlattenedPicture");
 
   gfx::Rect layer_rect(tiling_.total_size());
-  skia::RefPtr<SkPicture> picture = skia::AdoptRef(new SkPicture);
-  if (layer_rect.IsEmpty())
-    return picture;
-
-  SkCanvas* canvas = picture->beginRecording(
-      layer_rect.width(),
-      layer_rect.height(),
-      SkPicture::kUsePathBoundsForClip_RecordingFlag);
-
-  RasterToBitmap(canvas, layer_rect, 1.0, NULL);
-  picture->endRecording();
-
+  SkPictureRecorder recorder;
+  SkCanvas* canvas =
+      recorder.beginRecording(layer_rect.width(),
+                              layer_rect.height(),
+                              SkPicture::kUsePathBoundsForClip_RecordingFlag);
+  if (!layer_rect.IsEmpty())
+    RasterToBitmap(canvas, layer_rect, 1.0, NULL);
+  skia::RefPtr<SkPicture> picture = skia::AdoptRef(recorder.endRecording());
   return picture;
 }

[robertphillips, 2014-04-16 12:33:12]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-04-16 12:33:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertphillips@google.com/236633008/80001

[commit-bot: I haz the power, 2014-04-16 17:34:26]

Change committed as 264243