Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2067783002:
cc: Calculate visible rect inside non root copy request (Closed)
Descriptioncc: Calculate visible rect inside non root copy request
Modifies how visible rect calculation is done inside non root copy
request. Before the layer with the copy request would be fully visible.
Any other layers that is also inside the copy request would have some
combination of clips applied depending on tree structure. This CL
unifies how layers contributing to non root copy request has clips
applied: clips up to the copy request are applied, clips between copy
request and screen are ignored.
R=ajuma
BUG=594675
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/de7e0c3e9bf26b7c3bad40c6aa8883eb47159989
Cr-Commit-Position: refs/heads/master@{#399898}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 14
Patch Set 3 : address review comments #Patch Set 4 : edit comment to be more explicit #
Messages
Total messages: 21 (9 generated)
Description was changed from ========== cc: Calculate visible rect inside non root copy request Modifies how visible rect calculation is done inside non root copy request. Before the layer with the copy request would be fully visible. Any other layers that is also inside the copy request would have some combination of clips applied depending on tree structure. This CL unifies how layers contributing to non root copy request has clips applied: clips up to the copy request are applied, clips between copy request and screen are ignored. R=ajuma BUG=594675 ========== to ========== cc: Calculate visible rect inside non root copy request Modifies how visible rect calculation is done inside non root copy request. Before the layer with the copy request would be fully visible. Any other layers that is also inside the copy request would have some combination of clips applied depending on tree structure. This CL unifies how layers contributing to non root copy request has clips applied: clips up to the copy request are applied, clips between copy request and screen are ignored. R=ajuma BUG=594675 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Mostly: - add logic to calc non root copy request visible rect - accumulate clip helper functions now returns is_clipped as well as clip_rect.
Looks good overall. https://codereview.chromium.org/2067783002/diff/20001/cc/trees/draw_property_... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/2067783002/diff/20001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:107: return ClipRect{false, gfx::RectF()}; To make the bools here and below more readable (though it's less of an issue here since the declaration of ClipRect is right above), it might be worth defining a local bool is_clipped. https://codereview.chromium.org/2067783002/diff/20001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:110: else Nit: no need for 'else' after return https://codereview.chromium.org/2067783002/diff/20001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:122: } else { Here too, no need to use 'else' here since there's a return above. https://codereview.chromium.org/2067783002/diff/20001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:314: if (clip_in_layer_space.is_clipped) { Since accumulated_clip_rect.is_clipped is true here, would this only be false when there's an uninvertible transform in between the layer and the copy request target? https://codereview.chromium.org/2067783002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/2067783002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5778: host_impl.CreatePendingTree(); Is this needed? It looks like the test is only using the active tree. https://codereview.chromium.org/2067783002/diff/20001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/2067783002/diff/20001/cc/trees/property_tree.... cc/trees/property_tree.cc:1639: return 0; Could node->id be 1 here, and if so, should this return node->id instead of 0?
One more thing: https://codereview.chromium.org/2067783002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/2067783002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5792: copy_layer->SetDrawsContent(true); It be worth including a test where the copy_layer itself masks to bounds, to show that clip is applied when computing visible rects.
playfullbby91@gmail.com changed reviewers: + Playfullbby91@gmail.com
lgtm
Description was changed from ========== cc: Calculate visible rect inside non root copy request Modifies how visible rect calculation is done inside non root copy request. Before the layer with the copy request would be fully visible. Any other layers that is also inside the copy request would have some combination of clips applied depending on tree structure. This CL unifies how layers contributing to non root copy request has clips applied: clips up to the copy request are applied, clips between copy request and screen are ignored. R=ajuma BUG=594675 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Calculate visible rect inside non root copy request Modifies how visible rect calculation is done inside non root copy request. Before the layer with the copy request would be fully visible. Any other layers that is also inside the copy request would have some combination of clips applied depending on tree structure. This CL unifies how layers contributing to non root copy request has clips applied: clips up to the copy request are applied, clips between copy request and screen are ignored. R=ajuma BUG=594675 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
weiliangc@chromium.org changed reviewers: + playfullbby91@gmail.com - Playfullbby91@gmail.com
weiliangc@chromium.org changed reviewers: - playfullbby91@gmail.com
On 2016/06/14 15:25:17, ajuma wrote: > One more thing: > > https://codereview.chromium.org/2067783002/diff/20001/cc/trees/layer_tree_hos... > File cc/trees/layer_tree_host_common_unittest.cc (right): > > https://codereview.chromium.org/2067783002/diff/20001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_common_unittest.cc:5792: > copy_layer->SetDrawsContent(true); > It be worth including a test where the copy_layer itself masks to bounds, to > show that clip is applied when computing visible rects. So I added part where copy_layer was MasksToBounds(true), realized my code to accumulate clip doesn't take into account when the target surface owning layer's clip (as in the clip node created by the surface is ignored). Added the code to take target's clip node's local clip into account, and start getting mismatch between clip_rect check, since this is equivalent of taking viewport into account while accumulating clips, and our current clip tree doesn't. Should our current clip rect calculation take viewport into account?
On 2016/06/15 09:49:48, weiliangc wrote: > On 2016/06/14 15:25:17, ajuma wrote: > > One more thing: > > > > > https://codereview.chromium.org/2067783002/diff/20001/cc/trees/layer_tree_hos... > > File cc/trees/layer_tree_host_common_unittest.cc (right): > > > > > https://codereview.chromium.org/2067783002/diff/20001/cc/trees/layer_tree_hos... > > cc/trees/layer_tree_host_common_unittest.cc:5792: > > copy_layer->SetDrawsContent(true); > > It be worth including a test where the copy_layer itself masks to bounds, to > > show that clip is applied when computing visible rects. > > So I added part where copy_layer was MasksToBounds(true), realized my code to > accumulate clip doesn't take into account when the target surface owning layer's > clip (as in the clip node created by the surface is ignored). > > Added the code to take target's clip node's local clip into account, and start > getting mismatch between clip_rect check, since this is equivalent of taking > viewport into account while accumulating clips, and our current clip tree > doesn't. > > Should our current clip rect calculation take viewport into account? My understanding is that we intentionally leave the viewport out of clip_rect (but not out of the computation of visible rects) since we get this clipping for free when drawing to the root target (since the root target's size accounts for viewport size, we don't need to set a clip rect). But avoiding applying that clip (when that's the only clip) should already be handled by is_clipped being false. So taking the viewport into account when computing clip_rect should be fine.
Err ignore my comments about viewport, I was being dumb and didn't force a property tree rebuild while writing tests. https://codereview.chromium.org/2067783002/diff/20001/cc/trees/draw_property_... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/2067783002/diff/20001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:107: return ClipRect{false, gfx::RectF()}; On 2016/06/14 15:09:53, ajuma wrote: > To make the bools here and below more readable (though it's less of an issue > here since the declaration of ClipRect is right above), it might be worth > defining a local bool is_clipped. Renamed ClipRect to ConditionalClip, and add comments, would this suffice? https://codereview.chromium.org/2067783002/diff/20001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:110: else On 2016/06/14 15:09:53, ajuma wrote: > Nit: no need for 'else' after return Done. https://codereview.chromium.org/2067783002/diff/20001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:122: } else { On 2016/06/14 15:09:53, ajuma wrote: > Here too, no need to use 'else' here since there's a return above. Done. https://codereview.chromium.org/2067783002/diff/20001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:314: if (clip_in_layer_space.is_clipped) { On 2016/06/14 15:09:53, ajuma wrote: > Since accumulated_clip_rect.is_clipped is true here, would this only be false > when there's an uninvertible transform in between the layer and the copy request > target? Yes I think so. https://codereview.chromium.org/2067783002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/2067783002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5778: host_impl.CreatePendingTree(); On 2016/06/14 15:09:53, ajuma wrote: > Is this needed? It looks like the test is only using the active tree. Removed. https://codereview.chromium.org/2067783002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5792: copy_layer->SetDrawsContent(true); On 2016/06/14 15:25:17, ajuma wrote: > It be worth including a test where the copy_layer itself masks to bounds, to > show that clip is applied when computing visible rects. Done. https://codereview.chromium.org/2067783002/diff/20001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/2067783002/diff/20001/cc/trees/property_tree.... cc/trees/property_tree.cc:1639: return 0; On 2016/06/14 15:09:53, ajuma wrote: > Could node->id be 1 here, and if so, should this return node->id instead of 0? Done.
Thanks, lgtm
The CQ bit was checked by weiliangc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from Playfullbby91@gmail.com, ajuma@chromium.org Link to the patchset: https://codereview.chromium.org/2067783002/#ps60001 (title: "edit comment to be more explicit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067783002/60001
Message was sent while issue was closed.
Description was changed from ========== cc: Calculate visible rect inside non root copy request Modifies how visible rect calculation is done inside non root copy request. Before the layer with the copy request would be fully visible. Any other layers that is also inside the copy request would have some combination of clips applied depending on tree structure. This CL unifies how layers contributing to non root copy request has clips applied: clips up to the copy request are applied, clips between copy request and screen are ignored. R=ajuma BUG=594675 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Calculate visible rect inside non root copy request Modifies how visible rect calculation is done inside non root copy request. Before the layer with the copy request would be fully visible. Any other layers that is also inside the copy request would have some combination of clips applied depending on tree structure. This CL unifies how layers contributing to non root copy request has clips applied: clips up to the copy request are applied, clips between copy request and screen are ignored. R=ajuma BUG=594675 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== cc: Calculate visible rect inside non root copy request Modifies how visible rect calculation is done inside non root copy request. Before the layer with the copy request would be fully visible. Any other layers that is also inside the copy request would have some combination of clips applied depending on tree structure. This CL unifies how layers contributing to non root copy request has clips applied: clips up to the copy request are applied, clips between copy request and screen are ignored. R=ajuma BUG=594675 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Calculate visible rect inside non root copy request Modifies how visible rect calculation is done inside non root copy request. Before the layer with the copy request would be fully visible. Any other layers that is also inside the copy request would have some combination of clips applied depending on tree structure. This CL unifies how layers contributing to non root copy request has clips applied: clips up to the copy request are applied, clips between copy request and screen are ignored. R=ajuma BUG=594675 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/de7e0c3e9bf26b7c3bad40c6aa8883eb47159989 Cr-Commit-Position: refs/heads/master@{#399898} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/de7e0c3e9bf26b7c3bad40c6aa8883eb47159989 Cr-Commit-Position: refs/heads/master@{#399898} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
