Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(16)

Issue 2075873002: Support general raster matrix for RasterSource and DisplayItemList (Closed)

Created:
4 years ago by trchen
Modified:
3 years, 10 months ago
Reviewers:
danakj, ajuma, enne (OOO)
CC:
cc-bugs_chromium.org, chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : add back cullrect quick reject #

Patch Set 3 : fix empty playback rect #

Patch Set 4 : fix a bug in PrepareForPlaybackToCanvas and fix cc_unittests #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+145 lines, -186 lines) Patch
M cc/layers/picture_layer_impl_unittest.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M cc/playback/clip_display_item.h View 2 chunks +0 lines, -2 lines 0 comments Download
M cc/playback/clip_display_item.cc View 2 chunks +0 lines, -2 lines 0 comments Download
M cc/playback/clip_path_display_item.h View 2 chunks +0 lines, -2 lines 0 comments Download
M cc/playback/clip_path_display_item.cc View 2 chunks +0 lines, -2 lines 0 comments Download
M cc/playback/compositing_display_item.h View 2 chunks +0 lines, -2 lines 0 comments Download
M cc/playback/compositing_display_item.cc View 2 chunks +0 lines, -2 lines 0 comments Download
M cc/playback/display_item.h View 1 chunk +0 lines, -1 line 0 comments Download
M cc/playback/display_item_list.h View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M cc/playback/display_item_list.cc View 1 2 3 chunks +21 lines, -7 lines 0 comments Download
M cc/playback/drawing_display_item.h View 1 chunk +0 lines, -1 line 0 comments Download
M cc/playback/drawing_display_item.cc View 1 1 chunk +2 lines, -10 lines 0 comments Download
M cc/playback/filter_display_item.h View 2 chunks +0 lines, -2 lines 0 comments Download
M cc/playback/filter_display_item.cc View 2 chunks +0 lines, -2 lines 0 comments Download
M cc/playback/float_clip_display_item.h View 2 chunks +0 lines, -2 lines 0 comments Download
M cc/playback/float_clip_display_item.cc View 2 chunks +0 lines, -2 lines 0 comments Download
M cc/playback/raster_source.h View 1 2 2 chunks +14 lines, -19 lines 0 comments Download
M cc/playback/raster_source.cc View 1 2 3 3 chunks +102 lines, -113 lines 3 comments Download
M cc/playback/transform_display_item.h View 2 chunks +0 lines, -2 lines 0 comments Download
M cc/playback/transform_display_item.cc View 2 chunks +0 lines, -2 lines 0 comments Download
M cc/test/fake_raster_source.h View 1 chunk +0 lines, -3 lines 0 comments Download
M cc/test/fake_raster_source.cc View 1 chunk +1 line, -6 lines 0 comments Download
M cc/trees/layer_tree_host_pixeltest_masks.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 29 (13 generated)
trchen
This is WIP for early feedback. PTAL thanks! For context, it is a prerequisite to ...
4 years ago (2016-06-16 21:01:27 UTC) #3
trchen
BTW, besides rasterize_and_record_micro at ct.skia.org, is there other benchmarks that you recommend?
4 years ago (2016-06-16 21:04:23 UTC) #4
ajuma
Seems like a reasonable approach if perf doesn't regress. Aside from rasterize_and_record, one thing I'd ...
4 years ago (2016-06-20 18:21:35 UTC) #5
trchen
On 2016/06/20 18:21:35, ajuma wrote: > Seems like a reasonable approach if perf doesn't regress. ...
4 years ago (2016-06-20 21:44:51 UTC) #6
ajuma
On 2016/06/20 21:44:51, trchen wrote: > Also thank you for reminded me that just pixel ...
4 years ago (2016-06-20 22:05:48 UTC) #7
trchen
I think this CL is ready for reviewing! PTAL, thanks! Here's the latest Skia CT ...
4 years ago (2016-06-22 23:54:22 UTC) #11
ajuma
lgtm with a question https://codereview.chromium.org/2075873002/diff/80001/cc/playback/raster_source.cc File cc/playback/raster_source.cc (right): https://codereview.chromium.org/2075873002/diff/80001/cc/playback/raster_source.cc#newcode72 cc/playback/raster_source.cc:72: SkIRect raster_bounds = gfx::RectToSkIRect(canvas_bitmap_rect); Is ...
4 years ago (2016-06-23 17:57:37 UTC) #12
trchen
https://codereview.chromium.org/2075873002/diff/80001/cc/playback/raster_source.cc File cc/playback/raster_source.cc (right): https://codereview.chromium.org/2075873002/diff/80001/cc/playback/raster_source.cc#newcode72 cc/playback/raster_source.cc:72: SkIRect raster_bounds = gfx::RectToSkIRect(canvas_bitmap_rect); On 2016/06/23 17:57:37, ajuma wrote: ...
4 years ago (2016-06-23 22:18:24 UTC) #13
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2075873002/80001
4 years ago (2016-06-23 22:19:15 UTC) #15
commit-bot: I haz the power
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_rel_ng/builds/251608)
4 years ago (2016-06-24 00:40:57 UTC) #17
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2075873002/80001
4 years ago (2016-06-24 04:20:35 UTC) #19
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years ago (2016-06-24 04:55:15 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2075873002/80001
4 years ago (2016-06-24 05:03:33 UTC) #23
commit-bot: I haz the power
Committed patchset #4 (id:80001)
4 years ago (2016-06-24 05:08:35 UTC) #25
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/772e085e184f7036e8714d45db38a97906cd7073 Cr-Commit-Position: refs/heads/master@{#401817}
4 years ago (2016-06-24 05:10:05 UTC) #27
enne (OOO)
3 years, 10 months ago (2016-08-29 20:22:53 UTC) #29
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698