Support general raster matrix for RasterSource and DisplayItemList

Support general raster matrix for RasterSource and DisplayItemList

This CL is a prerequisite for making picture layer tiling to support tiling
in general space. Today we only raster in contents space, which is always
a linear scale of the layer space. To improve raster quality, it is best to
raster in target (render surface) space when possible. A follow-up CL will
be written to utilize this capability to raster layers that have fractional
translation transform.

The following API has been changed:
1. RasterSource::PlaybackToCanvas() no longer takes canvas_bitmap_rect (tile
   extent), canvas_playback_rect (damage rect), and contents_scale. Instead,
   the tile size will be extracted from the canvas size directly, and tile
   offset should be applied to canvas matrix by the caller. Likewise, the
   damage rect and contents scale should be applied to canvas clip and canvas
   matrix by the caller respectively.

2. DisplayItemList::Raster() no longer takes canvas_target_playback_rect (cull
   rect), and contents_scale. Again, those should be applied to canvas clip
   and canvas matrix by the caller directly.

3. DisplayItem::Raster() no longer takes canvas_target_playback_rect (cull
   rect). Use SkCanvas::quickReject() instead.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/772e085e184f7036e8714d45db38a97906cd7073
Cr-Commit-Position: refs/heads/master@{#401817}

[trchen, 2016-06-16 20:59:23]

Description was changed from

==========
Support general raster matrix for RasterSource and DisplayItemList

This CL is a prerequisite for making picture layer tiling to support tiling
in general space. Today we only raster in contents space, which is always
a linear scale of the layer space. To improve raster quality, it is best to
raster in target (render surface) space when possible. A follow-up CL will
be written to utilize this capability to raster layers that have fractional
translation transform.

The following API has been changed:
1. RasterSource::PlaybackToCanvas() no longer takes canvas_bitmap_rect (tile
   extent), canvas_playback_rect (damage rect), and contents_scale. Instead,
   the tile size will be extracted from the canvas size directly, and tile
   offset should be applied to canvas matrix by the caller. Likewise, the
   damage rect and contents scale should be applied to canvas clip and canvas
   matrix by the caller respectively.

2. DisplayItemList::Raster() no longer takes canvas_target_playback_rect (cull
   rect), and contents_scale. Again, those should be applied to canvas clip
   and canvas matrix by the caller directly.

3. DisplayItem::Raster() no longer takes canvas_target_playback_rect (cull
   rect). Our culling wasn't 100% correct anyway as it ignores possible paint
   or path effects. Skia performs quick reject for picture playback too and
   should be equivalently efficient. (Need verify with benchmarks!)
==========

to

==========
Support general raster matrix for RasterSource and DisplayItemList

This CL is a prerequisite for making picture layer tiling to support tiling
in general space. Today we only raster in contents space, which is always
a linear scale of the layer space. To improve raster quality, it is best to
raster in target (render surface) space when possible. A follow-up CL will
be written to utilize this capability to raster layers that have fractional
translation transform.

The following API has been changed:
1. RasterSource::PlaybackToCanvas() no longer takes canvas_bitmap_rect (tile
   extent), canvas_playback_rect (damage rect), and contents_scale. Instead,
   the tile size will be extracted from the canvas size directly, and tile
   offset should be applied to canvas matrix by the caller. Likewise, the
   damage rect and contents scale should be applied to canvas clip and canvas
   matrix by the caller respectively.

2. DisplayItemList::Raster() no longer takes canvas_target_playback_rect (cull
   rect), and contents_scale. Again, those should be applied to canvas clip
   and canvas matrix by the caller directly.

3. DisplayItem::Raster() no longer takes canvas_target_playback_rect (cull
   rect). Our culling wasn't 100% correct anyway as it ignores possible paint
   or path effects. Skia performs quick reject for picture playback too and
   should be equivalently efficient. (Need verify with benchmarks!)
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[trchen, 2016-06-16 21:01:27]

trchen@chromium.org changed reviewers:
+ ajuma@chromium.org, danakj@chromium.org

[trchen, 2016-06-16 21:01:27]

This is WIP for early feedback. PTAL thanks!

For context, it is a prerequisite to fix this issue: https://crbug.com/521364

[trchen, 2016-06-16 21:04:23]

BTW, besides rasterize_and_record_micro at ct.skia.org, is there other
benchmarks that you recommend?

[ajuma, 2016-06-20 18:21:35]

Seems like a reasonable approach if perf doesn't regress. Aside from
rasterize_and_record, one thing I'd suggest for the canvas_target_playback_rect
change is looking at how this affects tab spinner raster time (see
https://codereview.chromium.org/1158553004 where canvas_target_playback_rect was
added).

In the description, you mention rastering in target space. Would screen space
work instead? We currently know about target space on the pending tree, but
that's only because render surface determination happens on the main thread
since property trees depend on it. Once property tree node creation is untangled
from render surfaces, we'll hopefully be able to update render surfaces at draw
time without having to wait for the next commit (this would mean, for example,
that once a copy request is serviced, its render surface can immediately be
removed rather than having to force a commit as we do now; similarly, if an
opacity animation changes opacity to 1, we can update surfaces at that point
rather than waiting for the next commit). Once we can do that, there's no need
to compute surfaces at all until draw time, so then the pending tree (and hence
rasterization) won't be able to depend on knowledge about render targets. From
the standpoint of preventing text bluriness, screen space seems no worse target
space. When the transform between target space and screen space is an integer
translation, there's no difference. When the transform is a non-integer
translation, I think we'll get bluriness either way; rastering in target space
would produce non-blurry surface content, but once we translate the surface to
draw it to the screen we'll get bluriness back.

[trchen, 2016-06-20 21:44:51]

On 2016/06/20 18:21:35, ajuma wrote:
> Seems like a reasonable approach if perf doesn't regress. Aside from
> rasterize_and_record, one thing I'd suggest for the
canvas_target_playback_rect
> change is looking at how this affects tab spinner raster time (see
> https://codereview.chromium.org/1158553004 where canvas_target_playback_rect
was
> added).

Skia CT already caught raster regression. :(
It seems that not every picture playback code path in Skia does quick rejection
(thanks Dana for the diagnosis). I uploaded a new CL that added back the cull
rect rejection, except that retrieving the playback rect from canvas clip. Just
launched another Skia CT job. Let's see how it turns out.

> In the description, you mention rastering in target space. Would screen space
> work instead? We currently know about target space on the pending tree, but
> that's only because render surface determination happens on the main thread
> since property trees depend on it. Once property tree node creation is
untangled
> from render surfaces, we'll hopefully be able to update render surfaces at
draw
> time without having to wait for the next commit (this would mean, for example,
> that once a copy request is serviced, its render surface can immediately be
> removed rather than having to force a commit as we do now; similarly, if an
> opacity animation changes opacity to 1, we can update surfaces at that point
> rather than waiting for the next commit). Once we can do that, there's no need
> to compute surfaces at all until draw time, so then the pending tree (and
hence
> rasterization) won't be able to depend on knowledge about render targets. From
> the standpoint of preventing text bluriness, screen space seems no worse
target
> space. When the transform between target space and screen space is an integer
> translation, there's no difference. When the transform is a non-integer
> translation, I think we'll get bluriness either way; rastering in target space
> would produce non-blurry surface content, but once we translate the surface to
> draw it to the screen we'll get bluriness back.

Ah, that's good point. I didn't think about the pending tree issue. 

I think ideally the render surface should almost never have a draw transform.
i.e. They should use device space as local space. One exception I'm aware of is
blur filter, which is not trivial to apply in device space (especially if local
matrix has perspective). Using screen space for rastering would be compatible
with both today's world and the ideal world.

Also thank you for reminded me that just pixel snapping raster contents space to
target space is not enough. We also have to make sure every intermediate render
surface aligns with their target. How do we determine render surface bounds and
draw transforms today? Do we perform any kind of pixel snapping?

[ajuma, 2016-06-20 22:05:48]

On 2016/06/20 21:44:51, trchen wrote:
> Also thank you for reminded me that just pixel snapping raster contents space
to
> target space is not enough. We also have to make sure every intermediate
render
> surface aligns with their target. How do we determine render surface bounds
and
> draw transforms today? Do we perform any kind of pixel snapping?

We pixel-snap at scrollable transform nodes -- see
TransformTree::UpdateSnapping.

[trchen, 2016-06-22 00:27:58]

Patchset #3 (id:40001) has been deleted

[trchen, 2016-06-22 09:18:42]

Description was changed from

==========
Support general raster matrix for RasterSource and DisplayItemList

This CL is a prerequisite for making picture layer tiling to support tiling
in general space. Today we only raster in contents space, which is always
a linear scale of the layer space. To improve raster quality, it is best to
raster in target (render surface) space when possible. A follow-up CL will
be written to utilize this capability to raster layers that have fractional
translation transform.

The following API has been changed:
1. RasterSource::PlaybackToCanvas() no longer takes canvas_bitmap_rect (tile
   extent), canvas_playback_rect (damage rect), and contents_scale. Instead,
   the tile size will be extracted from the canvas size directly, and tile
   offset should be applied to canvas matrix by the caller. Likewise, the
   damage rect and contents scale should be applied to canvas clip and canvas
   matrix by the caller respectively.

2. DisplayItemList::Raster() no longer takes canvas_target_playback_rect (cull
   rect), and contents_scale. Again, those should be applied to canvas clip
   and canvas matrix by the caller directly.

3. DisplayItem::Raster() no longer takes canvas_target_playback_rect (cull
   rect). Our culling wasn't 100% correct anyway as it ignores possible paint
   or path effects. Skia performs quick reject for picture playback too and
   should be equivalently efficient. (Need verify with benchmarks!)
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Support general raster matrix for RasterSource and DisplayItemList

This CL is a prerequisite for making picture layer tiling to support tiling
in general space. Today we only raster in contents space, which is always
a linear scale of the layer space. To improve raster quality, it is best to
raster in target (render surface) space when possible. A follow-up CL will
be written to utilize this capability to raster layers that have fractional
translation transform.

The following API has been changed:
1. RasterSource::PlaybackToCanvas() no longer takes canvas_bitmap_rect (tile
   extent), canvas_playback_rect (damage rect), and contents_scale. Instead,
   the tile size will be extracted from the canvas size directly, and tile
   offset should be applied to canvas matrix by the caller. Likewise, the
   damage rect and contents scale should be applied to canvas clip and canvas
   matrix by the caller respectively.

2. DisplayItemList::Raster() no longer takes canvas_target_playback_rect (cull
   rect), and contents_scale. Again, those should be applied to canvas clip
   and canvas matrix by the caller directly.

3. DisplayItem::Raster() no longer takes canvas_target_playback_rect (cull
   rect). Use SkCanvas::quickReject() instead.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[trchen, 2016-06-22 09:18:59]

Description was changed from

==========
Support general raster matrix for RasterSource and DisplayItemList

This CL is a prerequisite for making picture layer tiling to support tiling
in general space. Today we only raster in contents space, which is always
a linear scale of the layer space. To improve raster quality, it is best to
raster in target (render surface) space when possible. A follow-up CL will
be written to utilize this capability to raster layers that have fractional
translation transform.

The following API has been changed:
1. RasterSource::PlaybackToCanvas() no longer takes canvas_bitmap_rect (tile
   extent), canvas_playback_rect (damage rect), and contents_scale. Instead,
   the tile size will be extracted from the canvas size directly, and tile
   offset should be applied to canvas matrix by the caller. Likewise, the
   damage rect and contents scale should be applied to canvas clip and canvas
   matrix by the caller respectively.

2. DisplayItemList::Raster() no longer takes canvas_target_playback_rect (cull
   rect), and contents_scale. Again, those should be applied to canvas clip
   and canvas matrix by the caller directly.

3. DisplayItem::Raster() no longer takes canvas_target_playback_rect (cull
   rect). Use SkCanvas::quickReject() instead.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Support general raster matrix for RasterSource and DisplayItemList

This CL is a prerequisite for making picture layer tiling to support tiling
in general space. Today we only raster in contents space, which is always
a linear scale of the layer space. To improve raster quality, it is best to
raster in target (render surface) space when possible. A follow-up CL will
be written to utilize this capability to raster layers that have fractional
translation transform.

The following API has been changed:
1. RasterSource::PlaybackToCanvas() no longer takes canvas_bitmap_rect (tile
   extent), canvas_playback_rect (damage rect), and contents_scale. Instead,
   the tile size will be extracted from the canvas size directly, and tile
   offset should be applied to canvas matrix by the caller. Likewise, the
   damage rect and contents scale should be applied to canvas clip and canvas
   matrix by the caller respectively.

2. DisplayItemList::Raster() no longer takes canvas_target_playback_rect (cull
   rect), and contents_scale. Again, those should be applied to canvas clip
   and canvas matrix by the caller directly.

3. DisplayItem::Raster() no longer takes canvas_target_playback_rect (cull
   rect). Use SkCanvas::quickReject() instead.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[trchen, 2016-06-22 23:54:22]

I think this CL is ready for reviewing! PTAL, thanks!

Here's the latest Skia CT run:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/t...

The perf regression we had with earlier patch sets were due to a bug in solid
color check, which I fixed in latest patch set.

Raster time actually got a small bit faster, due to the fact that we do better
culling. Previously we only cull against the canvas_playback_rect, but with this
CL, we use the current canvas clip bounds which takes clipping into
consideration.

The layout test failure was due to a bug in Blink, which I uploaded a separate
fix: https://codereview.chromium.org/2088333002/

[ajuma, 2016-06-23 17:57:37]

lgtm with a question

https://codereview.chromium.org/2075873002/diff/80001/cc/playback/raster_sour...
File cc/playback/raster_source.cc (right):

https://codereview.chromium.org/2075873002/diff/80001/cc/playback/raster_sour...
cc/playback/raster_source.cc:72: SkIRect raster_bounds =
gfx::RectToSkIRect(canvas_bitmap_rect);
Is it worth intersecting this with size_?

[trchen, 2016-06-23 22:18:24]

https://codereview.chromium.org/2075873002/diff/80001/cc/playback/raster_sour...
File cc/playback/raster_source.cc (right):

https://codereview.chromium.org/2075873002/diff/80001/cc/playback/raster_sour...
cc/playback/raster_source.cc:72: SkIRect raster_bounds =
gfx::RectToSkIRect(canvas_bitmap_rect);
On 2016/06/23 17:57:37, ajuma wrote:
> Is it worth intersecting this with size_?

(I assume you meant interest_rect as in PrepareForPlaybackToCanvas)
Interesting. I think it's worth a try. I'll launch a Skia CT in a separate CL.

[trchen, 2016-06-23 22:18:38]

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

[commit-bot: I haz the power, 2016-06-23 22:19:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2075873002/80001

[commit-bot: I haz the power, 2016-06-24 00:40:56]

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

[commit-bot: I haz the power, 2016-06-24 00:40:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[trchen, 2016-06-24 04:20:18]

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

[commit-bot: I haz the power, 2016-06-24 04:20:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2075873002/80001

[commit-bot: I haz the power, 2016-06-24 04:55:14]

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

[commit-bot: I haz the power, 2016-06-24 04:55:15]

Dry run: This issue passed the CQ dry run.

[trchen, 2016-06-24 05:03:16]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2016-06-24 05:03:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2075873002/80001

[commit-bot: I haz the power, 2016-06-24 05:08:34]

Description was changed from

==========
Support general raster matrix for RasterSource and DisplayItemList

This CL is a prerequisite for making picture layer tiling to support tiling
in general space. Today we only raster in contents space, which is always
a linear scale of the layer space. To improve raster quality, it is best to
raster in target (render surface) space when possible. A follow-up CL will
be written to utilize this capability to raster layers that have fractional
translation transform.

The following API has been changed:
1. RasterSource::PlaybackToCanvas() no longer takes canvas_bitmap_rect (tile
   extent), canvas_playback_rect (damage rect), and contents_scale. Instead,
   the tile size will be extracted from the canvas size directly, and tile
   offset should be applied to canvas matrix by the caller. Likewise, the
   damage rect and contents scale should be applied to canvas clip and canvas
   matrix by the caller respectively.

2. DisplayItemList::Raster() no longer takes canvas_target_playback_rect (cull
   rect), and contents_scale. Again, those should be applied to canvas clip
   and canvas matrix by the caller directly.

3. DisplayItem::Raster() no longer takes canvas_target_playback_rect (cull
   rect). Use SkCanvas::quickReject() instead.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Support general raster matrix for RasterSource and DisplayItemList

This CL is a prerequisite for making picture layer tiling to support tiling
in general space. Today we only raster in contents space, which is always
a linear scale of the layer space. To improve raster quality, it is best to
raster in target (render surface) space when possible. A follow-up CL will
be written to utilize this capability to raster layers that have fractional
translation transform.

The following API has been changed:
1. RasterSource::PlaybackToCanvas() no longer takes canvas_bitmap_rect (tile
   extent), canvas_playback_rect (damage rect), and contents_scale. Instead,
   the tile size will be extracted from the canvas size directly, and tile
   offset should be applied to canvas matrix by the caller. Likewise, the
   damage rect and contents scale should be applied to canvas clip and canvas
   matrix by the caller respectively.

2. DisplayItemList::Raster() no longer takes canvas_target_playback_rect (cull
   rect), and contents_scale. Again, those should be applied to canvas clip
   and canvas matrix by the caller directly.

3. DisplayItem::Raster() no longer takes canvas_target_playback_rect (cull
   rect). Use SkCanvas::quickReject() instead.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-24 05:08:35]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-06-24 05:10:03]

Description was changed from

==========
Support general raster matrix for RasterSource and DisplayItemList

This CL is a prerequisite for making picture layer tiling to support tiling
in general space. Today we only raster in contents space, which is always
a linear scale of the layer space. To improve raster quality, it is best to
raster in target (render surface) space when possible. A follow-up CL will
be written to utilize this capability to raster layers that have fractional
translation transform.

The following API has been changed:
1. RasterSource::PlaybackToCanvas() no longer takes canvas_bitmap_rect (tile
   extent), canvas_playback_rect (damage rect), and contents_scale. Instead,
   the tile size will be extracted from the canvas size directly, and tile
   offset should be applied to canvas matrix by the caller. Likewise, the
   damage rect and contents scale should be applied to canvas clip and canvas
   matrix by the caller respectively.

2. DisplayItemList::Raster() no longer takes canvas_target_playback_rect (cull
   rect), and contents_scale. Again, those should be applied to canvas clip
   and canvas matrix by the caller directly.

3. DisplayItem::Raster() no longer takes canvas_target_playback_rect (cull
   rect). Use SkCanvas::quickReject() instead.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Support general raster matrix for RasterSource and DisplayItemList

This CL is a prerequisite for making picture layer tiling to support tiling
in general space. Today we only raster in contents space, which is always
a linear scale of the layer space. To improve raster quality, it is best to
raster in target (render surface) space when possible. A follow-up CL will
be written to utilize this capability to raster layers that have fractional
translation transform.

The following API has been changed:
1. RasterSource::PlaybackToCanvas() no longer takes canvas_bitmap_rect (tile
   extent), canvas_playback_rect (damage rect), and contents_scale. Instead,
   the tile size will be extracted from the canvas size directly, and tile
   offset should be applied to canvas matrix by the caller. Likewise, the
   damage rect and contents scale should be applied to canvas clip and canvas
   matrix by the caller respectively.

2. DisplayItemList::Raster() no longer takes canvas_target_playback_rect (cull
   rect), and contents_scale. Again, those should be applied to canvas clip
   and canvas matrix by the caller directly.

3. DisplayItem::Raster() no longer takes canvas_target_playback_rect (cull
   rect). Use SkCanvas::quickReject() instead.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/772e085e184f7036e8714d45db38a97906cd7073
Cr-Commit-Position: refs/heads/master@{#401817}
==========

[commit-bot: I haz the power, 2016-06-24 05:10:05]

Patchset 4 (id:??) landed as
https://crrev.com/772e085e184f7036e8714d45db38a97906cd7073
Cr-Commit-Position: refs/heads/master@{#401817}

[enne (OOO), 2016-08-29 20:22:52]

enne@chromium.org changed reviewers:
+ enne@chromium.org

[enne (OOO), 2016-08-29 20:22:53]

https://codereview.chromium.org/2075873002/diff/80001/cc/playback/raster_sour...
File cc/playback/raster_source.cc (right):

https://codereview.chromium.org/2075873002/diff/80001/cc/playback/raster_sour...
cc/playback/raster_source.cc:180: // Drawing at most 2 x 2 x (canvas width +
canvas height) texels is 2-3X
This comment seems no longer true.