Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1865693003:
cc : Remove subtree skipping in LayerTreeHostCommon (Closed)
Descriptioncc : Remove subtree skipping in LayerTreeHostCommon
This CL makes sure we visit all layers while calculating
render target and RSLL. This will help us in deleting code
in layer skipping that is there to handle subtree skipping.
BUG=601088
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/5e83ea4d3b872053c0304bc81b71262041be8c3b
Cr-Commit-Position: refs/heads/master@{#385836}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebase #Patch Set 3 : #Patch Set 4 : #
Total comments: 5
Patch Set 5 : #Messages
Total messages: 17 (6 generated)
Description was changed from ========== cc : Remove subtree skippping in LayerTreeHostCommon BUG=601088 ========== to ========== cc : Remove subtree skippping in LayerTreeHostCommon BUG=601088 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
jaydasika@chromium.org changed reviewers: + ajuma@chromium.org, vollick@chromium.org, weiliangc@chromium.org
This CL makes CalculateRenderTarget and CalculateRSLL visit all layers.
Please add a description about where this is headed. (On its own, this CL still has the effect of skipping subtrees, so say more about why this is a step towards doing layer-by-layer skipping.) https://codereview.chromium.org/1865693003/diff/1/cc/trees/layer_tree_host_co... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/1865693003/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common.cc:546: can_render_to_separate_surface_for_children = false; If I'm understanding, in order to completely clean up the skipping logic, it needs to be possible to skip an ancestor of a layer with a copy request but still property handle the copy request. In that, this approach (of using can_render_to_separate_surface to represent that some ancestor has been skipped) won't work. So I think this needs to be split into a separate argument to be passed to descendants.
https://codereview.chromium.org/1865693003/diff/1/cc/trees/layer_tree_host_co... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/1865693003/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common.cc:546: can_render_to_separate_surface_for_children = false; On 2016/04/06 22:02:41, ajuma wrote: > If I'm understanding, in order to completely clean up the skipping logic, it > needs to be possible to skip an ancestor of a layer with a copy request but > still property handle the copy request. In that, this approach (of using > can_render_to_separate_surface to represent that some ancestor has been skipped) > won't work. So I think this needs to be split into a separate argument to be > passed to descendants. Just removing it should handle it, right? https://codereview.chromium.org/1865693003/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common.cc:785: return; This is a clean up unrelated to the CL. This is now at the end of the function, so we don't have to return. https://codereview.chromium.org/1865693003/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/1865693003/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:745: if (parent_target && !IsRootLayer(parent_target)) { Adding this because layers with copy requests may not always have parent_target.
Description was changed from ========== cc : Remove subtree skippping in LayerTreeHostCommon BUG=601088 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc : Remove subtree skipping in LayerTreeHostCommon This CL makes sure we visit all layers while calculating render target and RSLL. This will help us in deleting code in layer skipping that is there to handle subtree skipping. BUG=601088 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
On 2016/04/06 22:02:41, ajuma wrote: > Please add a description about where this is headed. (On its own, this CL still > has the effect of skipping subtrees, so say more about why this is a step > towards doing layer-by-layer skipping.) Updated description. After this CL, we won't be skipping subtrees, am I missing something ? > > https://codereview.chromium.org/1865693003/diff/1/cc/trees/layer_tree_host_co... > File cc/trees/layer_tree_host_common.cc (right): > > https://codereview.chromium.org/1865693003/diff/1/cc/trees/layer_tree_host_co... > cc/trees/layer_tree_host_common.cc:546: > can_render_to_separate_surface_for_children = false; > If I'm understanding, in order to completely clean up the skipping logic, it > needs to be possible to skip an ancestor of a layer with a copy request but > still property handle the copy request. In that, this approach (of using > can_render_to_separate_surface to represent that some ancestor has been skipped) > won't work. So I think this needs to be split into a separate argument to be > passed to descendants.
On 2016/04/07 00:38:51, jaydasika wrote: > On 2016/04/06 22:02:41, ajuma wrote: > > Please add a description about where this is headed. (On its own, this CL > still > > has the effect of skipping subtrees, so say more about why this is a step > > towards doing layer-by-layer skipping.) > > Updated description. After this CL, we won't be skipping subtrees, am I missing > something ? Thanks. I was confused by the can_render_to_separate_surface_for_children logic in the original patch set, which seemed to have the effect of making us logically skip subtrees (since as soon as one layer got a null render target, all its descendants got null render targets too). https://codereview.chromium.org/1865693003/diff/1/cc/trees/layer_tree_host_co... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/1865693003/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common.cc:546: can_render_to_separate_surface_for_children = false; On 2016/04/07 00:32:21, jaydasika wrote: > On 2016/04/06 22:02:41, ajuma wrote: > > If I'm understanding, in order to completely clean up the skipping logic, it > > needs to be possible to skip an ancestor of a layer with a copy request but > > still property handle the copy request. In that, this approach (of using > > can_render_to_separate_surface to represent that some ancestor has been > skipped) > > won't work. So I think this needs to be split into a separate argument to be > > passed to descendants. > > Just removing it should handle it, right? Yes, that works. https://codereview.chromium.org/1865693003/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/1865693003/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:666: if (!layer_should_be_skipped) { Please add a DCHECK here that we're either rendering to a separate surface or have a non-null render target. https://codereview.chromium.org/1865693003/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:745: if (parent_target && !IsRootLayer(parent_target)) { On 2016/04/07 00:32:21, jaydasika wrote: > Adding this because layers with copy requests may not always have parent_target. Can this happen now, or only after the layer skipping logic gets changed? (If it can happen now, please add a test for this.)
https://codereview.chromium.org/1865693003/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/1865693003/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:666: if (!layer_should_be_skipped) { On 2016/04/07 15:19:56, ajuma wrote: > Please add a DCHECK here that we're either rendering to a separate surface or > have a non-null render target. Done. https://codereview.chromium.org/1865693003/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:745: if (parent_target && !IsRootLayer(parent_target)) { On 2016/04/07 15:19:56, ajuma wrote: > On 2016/04/07 00:32:21, jaydasika wrote: > > Adding this because layers with copy requests may not always have > parent_target. > > Can this happen now, or only after the layer skipping logic gets changed? (If it > can happen now, please add a test for this.) Cannot happen now.
Thanks, lgtm.
The CQ bit was checked by jaydasika@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1865693003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1865693003/80001
Message was sent while issue was closed.
Description was changed from ========== cc : Remove subtree skipping in LayerTreeHostCommon This CL makes sure we visit all layers while calculating render target and RSLL. This will help us in deleting code in layer skipping that is there to handle subtree skipping. BUG=601088 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc : Remove subtree skipping in LayerTreeHostCommon This CL makes sure we visit all layers while calculating render target and RSLL. This will help us in deleting code in layer skipping that is there to handle subtree skipping. BUG=601088 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== cc : Remove subtree skipping in LayerTreeHostCommon This CL makes sure we visit all layers while calculating render target and RSLL. This will help us in deleting code in layer skipping that is there to handle subtree skipping. BUG=601088 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc : Remove subtree skipping in LayerTreeHostCommon This CL makes sure we visit all layers while calculating render target and RSLL. This will help us in deleting code in layer skipping that is there to handle subtree skipping. BUG=601088 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/5e83ea4d3b872053c0304bc81b71262041be8c3b Cr-Commit-Position: refs/heads/master@{#385836} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5e83ea4d3b872053c0304bc81b71262041be8c3b Cr-Commit-Position: refs/heads/master@{#385836}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1872003003/ by jaydasika@chromium.org. The reason for reverting is: crbug.com/601812. |
