Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2565643002:
[5/5] Add translated rasterization for PictureLayerImpl (Closed)
|
Created:
4 years ago by trchen Modified:
3 years, 7 months ago Reviewers:
enne (OOO) CC:
cc-bugs_chromium.org, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[5/5] Add translated rasterization for PictureLayerImpl
This CL implements raster translation update policy in PictureLayerImpl,
so that raster translation is only recomputed when there are changes that
would typically recompute the raster scale. Also when a pending layer gets
a new raster translation that results in a slot conflict with its twins,
this CL make sure all tiles are generated for the new tiling and old ones
are evicted.
This CL completes the fractional translated layer series. The end-to-end
result is that indirectly composited Blink layers would be rasterized in
its target space, similar to how non-composited layers are rendered.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2565643002
Cr-Commit-Position: refs/heads/master@{#470722}
Committed: https://chromium.googlesource.com/chromium/src/+/7b14a47b548f3d100a6bd6b163832b71e0236afd
Patch Set 1 #
Total comments: 1
Patch Set 2 : rebase && add unittests for slot conflict resolution and raster translation update policy #
Total comments: 11
Patch Set 3 : all done #Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 : rebase #Patch Set 7 : forgot to update cc_perftests too #
Messages
Total messages: 36 (20 generated)
Description was changed from ========== [5/6] Add translated rasterization for PictureLayerImpl ========== to ========== [5/6] Add translated rasterization for PictureLayerImpl CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
enne@chromium.org changed reviewers: + enne@chromium.org
https://codereview.chromium.org/2565643002/diff/1/cc/layers/picture_layer_imp... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/2565643002/diff/1/cc/layers/picture_layer_imp... cc/layers/picture_layer_impl.cc:912: if (high_res && high_res->raster_transform().translation() != raster_translation) { This logic about changing the raster translation but not changing the scale needs a unit test for sure.
Description was changed from ========== [5/6] Add translated rasterization for PictureLayerImpl CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== [5/5] Add translated rasterization for PictureLayerImpl CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== [5/5] Add translated rasterization for PictureLayerImpl CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== [5/5] Add translated rasterization for PictureLayerImpl This CL implements raster translation update policy in PictureLayerImpl, so that raster translation is only recomputed when there are changes that would typically recompute the raster scale. Also when a pending layer gets a new raster translation that results in a slot conflict with its twins, this CL make sure all tiles are generated for the new tiling and old ones are evicted. This CL completes the fractional translated layer series. The end-to-end result is that indirectly composited Blink layers would be rasterized in its target space, similar to how non-composited layers are rendered. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:5010: // Mark some arbitrary flags on the tiles for later checking. Are you trying to make sure that these tiles are the same tiles? Maybe create a set of tiles and compare below? This seems a little fragile, especially since you're not testing that solid color analysis performed is false to start. https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:5037: // Verifies the old tiles get evicted due to slot conflict. What does this mean? https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:5101: // Verifies the active tiles get evicted due to slot conflict. I'm a little unclear what the behavior is here. If the translation changes on one tree, the tiles are kept. But there's no coordination between pending and active about raster scale? So...composited animations don't cause reraster but non-composited animations do? Or, high res recreates if the translation changes, but not non-ideal scales? Can you provide a summary of what the heuristics are for rerastering? I feel like I've forgotten the discussions we've had in terms of what the goals are here.
https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:5010: // Mark some arbitrary flags on the tiles for later checking. On 2017/03/29 13:28:59, enne wrote: > Are you trying to make sure that these tiles are the same tiles? Maybe create a > set of tiles and compare below? This seems a little fragile, especially since > you're not testing that solid color analysis performed is false to start. Yes. Okay I'll try if I can inject some fingerprinted tiles... https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:5037: // Verifies the old tiles get evicted due to slot conflict. On 2017/03/29 13:28:59, enne wrote: > What does this mean? The layer property change is significant enough to trigger raster scale recomputation. The policy then also recomputes raster translation. In this case the new raster scale remains the same as the old one, but the raster translation changed. Ideally if we can create a set of tiles with the new translation as ideal tile, while keeping the old ones as a backup, but there is a slot conflict due to only scale is used as the key. I just realized the tiles would be evicted no matter what, since pending layer only keeps ideal tiles. I should change the comments to just say the old tiles are gone for being non-ideal. https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:5101: // Verifies the active tiles get evicted due to slot conflict. On 2017/03/29 13:28:59, enne wrote: > I'm a little unclear what the behavior is here. If the translation changes on > one tree, the tiles are kept. But there's no coordination between pending and > active about raster scale? So...composited animations don't cause reraster but > non-composited animations do? Or, high res recreates if the translation > changes, but not non-ideal scales? > > Can you provide a summary of what the heuristics are for rerastering? I feel > like I've forgotten the discussions we've had in terms of what the goals are > here. The general goal is to make indirectly composited layers have similar quality, while having no-worse performance than non-composited layers. The heuristics is to not updating raster translation if not updating raster scale. However if raster scale gets recomputed but the result is the same, still update raster translation and evict the old tiles if translation changed. Yea I agree this policy is weird, but should be pretty safe in terms of performance.
https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:5101: // Verifies the active tiles get evicted due to slot conflict. On 2017/03/30 at 21:48:47, trchen wrote: > On 2017/03/29 13:28:59, enne wrote: > > I'm a little unclear what the behavior is here. If the translation changes on > > one tree, the tiles are kept. But there's no coordination between pending and > > active about raster scale? So...composited animations don't cause reraster but > > non-composited animations do? Or, high res recreates if the translation > > changes, but not non-ideal scales? > > > > Can you provide a summary of what the heuristics are for rerastering? I feel > > like I've forgotten the discussions we've had in terms of what the goals are > > here. > > The general goal is to make indirectly composited layers have similar quality, while having no-worse performance than non-composited layers. > > The heuristics is to not updating raster translation if not updating raster scale. However if raster scale gets recomputed but the result is the same, still update raster translation and evict the old tiles if translation changed. > > Yea I agree this policy is weird, but should be pretty safe in terms of performance. Thanks for the reminder that this is indirectly composited content. The author doesn't expect translation to be fast here, so it's ok to reraster between different source frames. Sounds good. Maybe it'd be good to leave a comment about this nearer to the heuristics so other people can understand the motivations here? https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:5101: // Verifies the active tiles get evicted due to slot conflict. On 2017/03/30 at 21:48:47, trchen wrote: > On 2017/03/29 13:28:59, enne wrote: > > I'm a little unclear what the behavior is here. If the translation changes on > > one tree, the tiles are kept. But there's no coordination between pending and > > active about raster scale? So...composited animations don't cause reraster but > > non-composited animations do? Or, high res recreates if the translation > > changes, but not non-ideal scales? > > > > Can you provide a summary of what the heuristics are for rerastering? I feel > > like I've forgotten the discussions we've had in terms of what the goals are > > here. > > The general goal is to make indirectly composited layers have similar quality, while having no-worse performance than non-composited layers. > > The heuristics is to not updating raster translation if not updating raster scale. However if raster scale gets recomputed but the result is the same, still update raster translation and evict the old tiles if translation changed. > > Yea I agree this policy is weird, but should be pretty safe in terms of performance. Thanks for the reminder that this is indirectly composited content. The author doesn't expect translation to be fast here, so it's ok to reraster between different source frames. Sounds good. Maybe it'd be good to leave a comment about this nearer to the heuristics so other people can understand the motivations here?
All done. Thanks for reviewing the long series! https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:5010: // Mark some arbitrary flags on the tiles for later checking. On 2017/03/30 21:48:47, trchen wrote: > On 2017/03/29 13:28:59, enne wrote: > > Are you trying to make sure that these tiles are the same tiles? Maybe create > a > > set of tiles and compare below? This seems a little fragile, especially since > > you're not testing that solid color analysis performed is false to start. > > Yes. Okay I'll try if I can inject some fingerprinted tiles... Actually the tiles remember its raster transform at creation. I'll just check that instead. https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:5037: // Verifies the old tiles get evicted due to slot conflict. On 2017/03/30 21:48:47, trchen wrote: > On 2017/03/29 13:28:59, enne wrote: > > What does this mean? > > The layer property change is significant enough to trigger raster scale > recomputation. The policy then also recomputes raster translation. > > In this case the new raster scale remains the same as the old one, but the > raster translation changed. Ideally if we can create a set of tiles with the new > translation as ideal tile, while keeping the old ones as a backup, but there is > a slot conflict due to only scale is used as the key. > > I just realized the tiles would be evicted no matter what, since pending layer > only keeps ideal tiles. I should change the comments to just say the old tiles > are gone for being non-ideal. Done. https://codereview.chromium.org/2565643002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer_impl_unittest.cc:5101: // Verifies the active tiles get evicted due to slot conflict. On 2017/03/30 21:55:23, enne wrote: > On 2017/03/30 at 21:48:47, trchen wrote: > > On 2017/03/29 13:28:59, enne wrote: > > > I'm a little unclear what the behavior is here. If the translation changes > on > > > one tree, the tiles are kept. But there's no coordination between pending > and > > > active about raster scale? So...composited animations don't cause reraster > but > > > non-composited animations do? Or, high res recreates if the translation > > > changes, but not non-ideal scales? > > > > > > Can you provide a summary of what the heuristics are for rerastering? I feel > > > like I've forgotten the discussions we've had in terms of what the goals are > > > here. > > > > The general goal is to make indirectly composited layers have similar quality, > while having no-worse performance than non-composited layers. > > > > The heuristics is to not updating raster translation if not updating raster > scale. However if raster scale gets recomputed but the result is the same, still > update raster translation and evict the old tiles if translation changed. > > > > Yea I agree this policy is weird, but should be pretty safe in terms of > performance. > > Thanks for the reminder that this is indirectly composited content. The author > doesn't expect translation to be fast here, so it's ok to reraster between > different source frames. Sounds good. Maybe it'd be good to leave a comment > about this nearer to the heuristics so other people can understand the > motivations here? Done.
lgtm
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by trchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org Link to the patchset: https://codereview.chromium.org/2565643002/#ps120001 (title: "forgot to update cc_perftests too")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by trchen@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by trchen@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by trchen@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by trchen@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494447053083120,
"parent_rev": "9913fd894e029450864bbc1962290b0b76f2300e", "commit_rev":
"7b14a47b548f3d100a6bd6b163832b71e0236afd"}
Message was sent while issue was closed.
Description was changed from ========== [5/5] Add translated rasterization for PictureLayerImpl This CL implements raster translation update policy in PictureLayerImpl, so that raster translation is only recomputed when there are changes that would typically recompute the raster scale. Also when a pending layer gets a new raster translation that results in a slot conflict with its twins, this CL make sure all tiles are generated for the new tiling and old ones are evicted. This CL completes the fractional translated layer series. The end-to-end result is that indirectly composited Blink layers would be rasterized in its target space, similar to how non-composited layers are rendered. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== [5/5] Add translated rasterization for PictureLayerImpl This CL implements raster translation update policy in PictureLayerImpl, so that raster translation is only recomputed when there are changes that would typically recompute the raster scale. Also when a pending layer gets a new raster translation that results in a slot conflict with its twins, this CL make sure all tiles are generated for the new tiling and old ones are evicted. This CL completes the fractional translated layer series. The end-to-end result is that indirectly composited Blink layers would be rasterized in its target space, similar to how non-composited layers are rendered. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2565643002 Cr-Commit-Position: refs/heads/master@{#470722} Committed: https://chromium.googlesource.com/chromium/src/+/7b14a47b548f3d100a6bd6b16383... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/7b14a47b548f3d100a6bd6b16383... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
