cc: Update discardable image metadata generation to get PaintImages.

cc: Update discardable image metadata generation to get PaintImages.

The process of generating the metadata for DrawImages involves
rastering the paint ops on an analysis canvas to capture SkImages. Since
the ops now store PaintImages which are required in the raster pipeline,
this change updates this step to directly iterate through the ops stored
in the buffer and add images onto the map.

We still maintain an SkNoDrawCanvas to track the transform, clip and
saved paints state from the non-drawing ops.

BUG=715886
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2842333003
Cr-Commit-Position: refs/heads/master@{#469873}
Committed: https://chromium.googlesource.com/chromium/src/+/83f8bbb8ac2dbe76c00360bfd7c99ac09a458d0b

[Khushal, 2017-04-27 03:56:17]

Description was changed from

==========
cc: Update discardable image metadata generation to get PaintImages.

The process of generating the metadata for DrawImages involves
rastering the paint ops on an analysis canvas to capture SkImages. Since
the ops now store PaintImages which are required in the raster pipeline,
this change updates this step to directly iterate through the ops stored
in the buffer and add images onto the map.

We still maintain an SkNoDrawCanvas to track the transform, clip and
saved paints state from the non-drawing ops.

BUG=
==========

to

==========
cc: Update discardable image metadata generation to get PaintImages.

The process of generating the metadata for DrawImages involves
rastering the paint ops on an analysis canvas to capture SkImages. Since
the ops now store PaintImages which are required in the raster pipeline,
this change updates this step to directly iterate through the ops stored
in the buffer and add images onto the map.

We still maintain an SkNoDrawCanvas to track the transform, clip and
saved paints state from the non-drawing ops.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Khushal, 2017-04-27 03:59:02]

Description was changed from

==========
cc: Update discardable image metadata generation to get PaintImages.

The process of generating the metadata for DrawImages involves
rastering the paint ops on an analysis canvas to capture SkImages. Since
the ops now store PaintImages which are required in the raster pipeline,
this change updates this step to directly iterate through the ops stored
in the buffer and add images onto the map.

We still maintain an SkNoDrawCanvas to track the transform, clip and
saved paints state from the non-drawing ops.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Update discardable image metadata generation to get PaintImages.

The process of generating the metadata for DrawImages involves
rastering the paint ops on an analysis canvas to capture SkImages. Since
the ops now store PaintImages which are required in the raster pipeline,
this change updates this step to directly iterate through the ops stored
in the buffer and add images onto the map.

We still maintain an SkNoDrawCanvas to track the transform, clip and
saved paints state from the non-drawing ops.

BUG=715886
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Khushal, 2017-04-27 04:02:13]

khushalsagar@chromium.org changed reviewers:
+ enne@chromium.org, vmpstr@chromium.org

[vmpstr, 2017-04-27 16:28:54]

https://codereview.chromium.org/2842333003/diff/1/cc/paint/discardable_image_...
File cc/paint/discardable_image_map.cc (right):

https://codereview.chromium.org/2842333003/diff/1/cc/paint/discardable_image_...
cc/paint/discardable_image_map.cc:33: public SkNWayCanvas {
Why is this still deriving from SkNWayCanvas? Can we make SkNoDrawCanvas a
member instead?

https://codereview.chromium.org/2842333003/diff/1/cc/paint/discardable_image_...
cc/paint/discardable_image_map.cc:46: void AddImage(const PaintImage&
paint_image,
This function is unchanged, right?

https://codereview.chromium.org/2842333003/diff/1/cc/paint/discardable_image_...
File cc/paint/discardable_image_store.h (right):

https://codereview.chromium.org/2842333003/diff/1/cc/paint/discardable_image_...
cc/paint/discardable_image_store.h:16: class DiscardableImageStore {
Why don't we just make this a concrete class?

https://codereview.chromium.org/2842333003/diff/1/cc/paint/paint_op_buffer.cc
File cc/paint/paint_op_buffer.cc (right):

https://codereview.chromium.org/2842333003/diff/1/cc/paint/paint_op_buffer.cc...
cc/paint/paint_op_buffer.cc:17: void AddImageFromFlags(DiscardableImageStore*
image_store,
It feels like this should be a member on DiscardableImageStore

https://codereview.chromium.org/2842333003/diff/1/cc/paint/paint_op_buffer.cc...
cc/paint/paint_op_buffer.cc:603: void PaintOpBuffer::GatherDiscardableImages(
hmmmmm. Have you considered flipping |this| and |image_store|? Ie,
image_store->GatherDiscardableImages(paint_op_buffer) and implementing this in
there? Is there something that precludes this?

https://codereview.chromium.org/2842333003/diff/1/cc/paint/paint_op_buffer.cc...
cc/paint/paint_op_buffer.cc:611: for (auto* op : Iterator(this)) {
I was hoping to implement a smarter iterator for this that can skip save/restore
blocks if there are no images. Do you think that's a follow up work?

[Khushal, 2017-04-27 22:06:03]

https://codereview.chromium.org/2842333003/diff/1/cc/paint/discardable_image_...
File cc/paint/discardable_image_map.cc (right):

https://codereview.chromium.org/2842333003/diff/1/cc/paint/discardable_image_...
cc/paint/discardable_image_map.cc:33: public SkNWayCanvas {
On 2017/04/27 16:28:53, vmpstr wrote:
> Why is this still deriving from SkNWayCanvas? Can we make SkNoDrawCanvas a
> member instead?

Done.

https://codereview.chromium.org/2842333003/diff/1/cc/paint/discardable_image_...
cc/paint/discardable_image_map.cc:46: void AddImage(const PaintImage&
paint_image,
On 2017/04/27 16:28:53, vmpstr wrote:
> This function is unchanged, right?

Nope. It includes a fix for using the clip rect to clamp the paint rect instead
of the canvas_bounds_ which we were using right now.

I think the rect mapping using the paints might also be incorrect, because we
should be mapping the rect first using the ctm in the save operations (that's
what SkRecordDraw does). I was planning on doing this in a subsequent patch.

https://codereview.chromium.org/2842333003/diff/1/cc/paint/discardable_image_...
File cc/paint/discardable_image_store.h (right):

https://codereview.chromium.org/2842333003/diff/1/cc/paint/discardable_image_...
cc/paint/discardable_image_store.h:16: class DiscardableImageStore {
On 2017/04/27 16:28:54, vmpstr wrote:
> Why don't we just make this a concrete class?

Done.

https://codereview.chromium.org/2842333003/diff/1/cc/paint/paint_op_buffer.cc
File cc/paint/paint_op_buffer.cc (right):

https://codereview.chromium.org/2842333003/diff/1/cc/paint/paint_op_buffer.cc...
cc/paint/paint_op_buffer.cc:17: void AddImageFromFlags(DiscardableImageStore*
image_store,
On 2017/04/27 16:28:54, vmpstr wrote:
> It feels like this should be a member on DiscardableImageStore

Done.

https://codereview.chromium.org/2842333003/diff/1/cc/paint/paint_op_buffer.cc...
cc/paint/paint_op_buffer.cc:603: void PaintOpBuffer::GatherDiscardableImages(
On 2017/04/27 16:28:54, vmpstr wrote:
> hmmmmm. Have you considered flipping |this| and |image_store|? Ie,
> image_store->GatherDiscardableImages(paint_op_buffer) and implementing this in
> there? Is there something that precludes this?

Done.

https://codereview.chromium.org/2842333003/diff/1/cc/paint/paint_op_buffer.cc...
cc/paint/paint_op_buffer.cc:611: for (auto* op : Iterator(this)) {
On 2017/04/27 16:28:54, vmpstr wrote:
> I was hoping to implement a smarter iterator for this that can skip
save/restore
> blocks if there are no images. Do you think that's a follow up work?

Yeah. Its worth looking at in a follow up patch.

[vmpstr, 2017-04-28 16:40:11]

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
File cc/paint/discardable_image_store.cc (right):

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
cc/paint/discardable_image_store.cc:93: // TODO(khushalsagar): Optimize out
save/restore blocks if the next op is a
... if draws between them don't have images.

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
cc/paint/discardable_image_store.cc:217: if (!paint_rect.intersect(clip_rect))
I think you might want to add a similar check to image hijack canvas, since we
don't want to send skia images that are clipped by this as a precaution so that
skia doesn't cache the decode.

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
File cc/paint/discardable_image_store.h (right):

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
cc/paint/discardable_image_store.h:28: SkCanvas* NoDrawCanvas();
nit: GetNoDrawCanvas() or just GetCanvas()? or canvas()? Is it important that
it's a no draw here? If so, make the return value SkNoDrawCanvas*

[Khushal, 2017-05-03 21:07:07]

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
File cc/paint/discardable_image_store.cc (right):

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
cc/paint/discardable_image_store.cc:93: // TODO(khushalsagar): Optimize out
save/restore blocks if the next op is a
On 2017/04/28 16:40:11, vmpstr wrote:
> ... if draws between them don't have images.

Done.

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
cc/paint/discardable_image_store.cc:217: if (!paint_rect.intersect(clip_rect))
On 2017/04/28 16:40:11, vmpstr wrote:
> I think you might want to add a similar check to image hijack canvas, since we
> don't want to send skia images that are clipped by this as a precaution so
that
> skia doesn't cache the decode.

I added a check to the hijack canvas to make sure we skip ops outside the
current clip, similar to what skia will do. The problem is not that we might
skip images here that skia will decode, since SkCanvas skips these ops
internally. But we might be capturing and decoding images that are never
actually used.

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
File cc/paint/discardable_image_store.h (right):

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
cc/paint/discardable_image_store.h:28: SkCanvas* NoDrawCanvas();
On 2017/04/28 16:40:11, vmpstr wrote:
> nit: GetNoDrawCanvas() or just GetCanvas()? or canvas()? Is it important that
> it's a no draw here? If so, make the return value SkNoDrawCanvas*

The no draw part was just to make it clear that its shouldn't be used to
playback any draw ops. I was also considering just adding an iterator for the
DisplayItemList, similar to PaintOpBuffer, so we can do the iteration inside the
store itself and there is no need to expose the canvas. Would that be better?

[vmpstr, 2017-05-05 18:56:20]

lgtm with comment

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
File cc/paint/discardable_image_store.cc (right):

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
cc/paint/discardable_image_store.cc:217: if (!paint_rect.intersect(clip_rect))
On 2017/05/03 21:07:07, Khushal wrote:
> On 2017/04/28 16:40:11, vmpstr wrote:
> > I think you might want to add a similar check to image hijack canvas, since
we
> > don't want to send skia images that are clipped by this as a precaution so
> that
> > skia doesn't cache the decode.
> 
> I added a check to the hijack canvas to make sure we skip ops outside the
> current clip, similar to what skia will do. The problem is not that we might
> skip images here that skia will decode, since SkCanvas skips these ops
> internally. But we might be capturing and decoding images that are never
> actually used.

Acknowledged.

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
File cc/paint/discardable_image_store.h (right):

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
cc/paint/discardable_image_store.h:28: SkCanvas* NoDrawCanvas();
On 2017/05/03 21:07:07, Khushal wrote:
> On 2017/04/28 16:40:11, vmpstr wrote:
> > nit: GetNoDrawCanvas() or just GetCanvas()? or canvas()? Is it important
that
> > it's a no draw here? If so, make the return value SkNoDrawCanvas*
> 
> The no draw part was just to make it clear that its shouldn't be used to
> playback any draw ops. I was also considering just adding an iterator for the
> DisplayItemList, similar to PaintOpBuffer, so we can do the iteration inside
the
> store itself and there is no need to expose the canvas. Would that be better?

I think we can do that second step as a follow up. For now, I would prefer if
the return type of the canvas was SkNoDrawCanvas to make it reaally clear and
name the function GetNoDrawCanvas.

[vmpstr, 2017-05-05 18:57:02]

oh before landing, can you run this through ct to see if there's impact after
the change we made to make sure the current r&r benchmark captures image
gathering?

[Khushal, 2017-05-05 21:54:12]

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
File cc/paint/discardable_image_store.h (right):

https://codereview.chromium.org/2842333003/diff/20001/cc/paint/discardable_im...
cc/paint/discardable_image_store.h:28: SkCanvas* NoDrawCanvas();
On 2017/05/05 18:56:20, vmpstr wrote:
> On 2017/05/03 21:07:07, Khushal wrote:
> > On 2017/04/28 16:40:11, vmpstr wrote:
> > > nit: GetNoDrawCanvas() or just GetCanvas()? or canvas()? Is it important
> that
> > > it's a no draw here? If so, make the return value SkNoDrawCanvas*
> > 
> > The no draw part was just to make it clear that its shouldn't be used to
> > playback any draw ops. I was also considering just adding an iterator for
the
> > DisplayItemList, similar to PaintOpBuffer, so we can do the iteration inside
> the
> > store itself and there is no need to expose the canvas. Would that be
better?
> 
> I think we can do that second step as a follow up. For now, I would prefer if
> the return type of the canvas was SkNoDrawCanvas to make it reaally clear and
> name the function GetNoDrawCanvas.

Done.

[Khushal, 2017-05-05 21:54:55]

The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 21:55:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 22:06:48]

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

[commit-bot: I haz the power, 2017-05-05 22:06:49]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Khushal, 2017-05-05 22:13:41]

The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 22:15:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 22:48:54]

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

[commit-bot: I haz the power, 2017-05-05 22:48:55]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Khushal, 2017-05-05 23:40:18]

The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 23:41:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-06 00:30:20]

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

[commit-bot: I haz the power, 2017-05-06 00:30:21]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[Khushal, 2017-05-06 01:31:57]

The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-06 01:33:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-06 02:56:42]

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

[commit-bot: I haz the power, 2017-05-06 02:56:43]

Dry run: This issue passed the CQ dry run.

[Khushal, 2017-05-06 17:41:11]

On 2017/05/05 18:57:02, vmpstr wrote:
> oh before landing, can you run this through ct to see if there's impact after
> the change we made to make sure the current r&r benchmark captures image
> gathering?

CT is very happy. This show a ~15-16% improvement in record time. The previous
change that was avoiding the generation process for lists with no images was
only ~4%. I think this made it more better because now we are skipping
PaintRecords with no images as well.

[Khushal, 2017-05-06 17:41:18]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-05-06 17:41:19]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2842333003/#ps160001
(title: "win")

[commit-bot: I haz the power, 2017-05-06 17:41:50]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-06 19:13:21]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494092478773790,
"parent_rev": "a3b9a27415726553953ea3f0d1c926a51d26a412", "commit_rev":
"83f8bbb8ac2dbe76c00360bfd7c99ac09a458d0b"}

[commit-bot: I haz the power, 2017-05-06 19:13:31]

Description was changed from

==========
cc: Update discardable image metadata generation to get PaintImages.

The process of generating the metadata for DrawImages involves
rastering the paint ops on an analysis canvas to capture SkImages. Since
the ops now store PaintImages which are required in the raster pipeline,
this change updates this step to directly iterate through the ops stored
in the buffer and add images onto the map.

We still maintain an SkNoDrawCanvas to track the transform, clip and
saved paints state from the non-drawing ops.

BUG=715886
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Update discardable image metadata generation to get PaintImages.

The process of generating the metadata for DrawImages involves
rastering the paint ops on an analysis canvas to capture SkImages. Since
the ops now store PaintImages which are required in the raster pipeline,
this change updates this step to directly iterate through the ops stored
in the buffer and add images onto the map.

We still maintain an SkNoDrawCanvas to track the transform, clip and
saved paints state from the non-drawing ops.

BUG=715886
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2842333003
Cr-Commit-Position: refs/heads/master@{#469873}
Committed:
https://chromium.googlesource.com/chromium/src/+/83f8bbb8ac2dbe76c00360bfd7c9...
==========

[commit-bot: I haz the power, 2017-05-06 19:13:32]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/83f8bbb8ac2dbe76c00360bfd7c9...