|
|
Descriptioncc:: Use clip tree instead of layer tree during hit testing
During hit testing, we walk up the layer tree to check if the test point
is clipped by some ancestor. This CL makes it use the clip tree instead.
(when property trees are enabled)
BUG=568758
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/4bab6cab4df2a082f47729972d642afa022afc47
Cr-Commit-Position: refs/heads/master@{#367963}
Patch Set 1 #
Total comments: 5
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 4
Patch Set 4 : Rebase #Patch Set 5 : #Patch Set 6 : #
Total comments: 4
Patch Set 7 : #Patch Set 8 : #Messages
Total messages: 18 (4 generated)
Description was changed from ========== cc:: Use clip tree instead of layer tree during hit testing During hit testing, we walk up the layer tree to check if the test point is clipped by some ancestor. This CL makes it use the clip tree instead. (when property trees are enabled) BUG=568758 ========== to ========== cc:: Use clip tree instead of layer tree during hit testing During hit testing, we walk up the layer tree to check if the test point is clipped by some ancestor. This CL makes it use the clip tree instead. (when property trees are enabled) BUG=568758 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
jaydasika@chromium.org changed reviewers: + ajuma@chromium.org
We already have many unit tests that test hit testing and all of them pass with this patch. I am not sure though if that's enough to validate the equivalence of old and new approaches. https://codereview.chromium.org/1551333003/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1551333003/diff/1/cc/trees/layer_tree_impl.cc... cc/trees/layer_tree_impl.cc:1491: layer->render_surface()->content_rect(), NULL)) { I haven't added checking with render surface's content rect to the new approach and still all tests pass. I am still thinking if its because surface content rect is union of all drawable content rects and so whenever some point is clipped by it, it is also clipped by some clip rect or because we dont have good unit test coverage?
https://codereview.chromium.org/1551333003/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1551333003/diff/1/cc/trees/layer_tree_impl.cc... cc/trees/layer_tree_impl.cc:1457: for (int i = 2; i <= layer->clip_tree_index(); ++i) { This seems to visit all previous clip nodes rather than just those that are ancestors? If so, and if this still managed to pass all tests, we're probably lacking test coverage :)
https://codereview.chromium.org/1551333003/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1551333003/diff/1/cc/trees/layer_tree_impl.cc... cc/trees/layer_tree_impl.cc:1457: for (int i = 2; i <= layer->clip_tree_index(); ++i) { On 2016/01/05 00:44:43, ajuma wrote: > This seems to visit all previous clip nodes rather than just those that are > ancestors? If so, and if this still managed to pass all tests, we're probably > lacking test coverage :) Corrected this and added a test.
https://codereview.chromium.org/1551333003/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1551333003/diff/1/cc/trees/layer_tree_impl.cc... cc/trees/layer_tree_impl.cc:1491: layer->render_surface()->content_rect(), NULL)) { On 2016/01/04 23:58:36, jaydasika wrote: > I haven't added checking with render surface's content rect to the new approach > and still all tests pass. I am still thinking if its because surface content > rect is union of all drawable content rects and so whenever some point is > clipped by it, it is also clipped by some clip rect or because we dont have good > unit test coverage? As we discussed offline, we probably still need to do something like this (and even if not, it'd be good to add a test). https://codereview.chromium.org/1551333003/diff/20001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1551333003/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:1458: clip_node->id > 1; clip_node = clip_tree.parent(clip_node)) { Should this be >= 1? (We're probably missing test coverage for this too.)
https://codereview.chromium.org/1551333003/diff/20001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1551333003/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:1458: clip_node->id > 1; clip_node = clip_tree.parent(clip_node)) { On 2016/01/05 18:55:11, ajuma wrote: > Should this be >= 1? (We're probably missing test coverage for this too.) Clip node 1(viewport) is already there at line 1450. It is outside the loop as it may not have local clip and we still need to check.
https://codereview.chromium.org/1551333003/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1551333003/diff/1/cc/trees/layer_tree_impl.cc... cc/trees/layer_tree_impl.cc:1491: layer->render_surface()->content_rect(), NULL)) { On 2016/01/05 18:55:11, ajuma wrote: > On 2016/01/04 23:58:36, jaydasika wrote: > > I haven't added checking with render surface's content rect to the new > approach > > and still all tests pass. I am still thinking if its because surface content > > rect is union of all drawable content rects and so whenever some point is > > clipped by it, it is also clipped by some clip rect or because we dont have > good > > unit test coverage? > > As we discussed offline, we probably still need to do something like this (and > even if not, it'd be good to add a test). We do need to check with content rect because content rects are clipped by the max texture size. I have added a unit test that fails with this patch.
https://codereview.chromium.org/1551333003/diff/40001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl_unittest.cc (right): https://codereview.chromium.org/1551333003/diff/40001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl_unittest.cc:443: ASSERT_TRUE(result_layer); EXPECT_TRUE (it would make sense to use ASSERT if you were next going to dereference result_layer and prefered to crash at the assert rather later when result_layer is null, but in this case we're not using result_layer) https://codereview.chromium.org/1551333003/diff/40001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl_unittest.cc:448: ASSERT_FALSE(result_layer); EXPECT_FALSE
https://codereview.chromium.org/1551333003/diff/40001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl_unittest.cc (right): https://codereview.chromium.org/1551333003/diff/40001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl_unittest.cc:443: ASSERT_TRUE(result_layer); On 2016/01/06 14:18:55, ajuma wrote: > EXPECT_TRUE > (it would make sense to use ASSERT if you were next going to dereference > result_layer and prefered to crash at the assert rather later when result_layer > is null, but in this case we're not using result_layer) Done. https://codereview.chromium.org/1551333003/diff/40001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl_unittest.cc:448: ASSERT_FALSE(result_layer); On 2016/01/06 14:18:55, ajuma wrote: > EXPECT_FALSE Done.
https://codereview.chromium.org/1551333003/diff/100001/cc/trees/layer_tree_im... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1551333003/diff/100001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:1443: // clip's bounds. Also, the point can be clipped by the contect rect of an s/contect/content https://codereview.chromium.org/1551333003/diff/100001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:1458: transform_tree.Node(clip_node->data.transform_id); Should this be target_id (given that the clip we're using below is in target space)? Do we have unit tests for the case that the clip node's own transform id is different from its target id?
https://codereview.chromium.org/1551333003/diff/100001/cc/trees/layer_tree_im... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1551333003/diff/100001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:1443: // clip's bounds. Also, the point can be clipped by the contect rect of an On 2016/01/06 19:53:44, ajuma wrote: > s/contect/content Done. https://codereview.chromium.org/1551333003/diff/100001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:1458: transform_tree.Node(clip_node->data.transform_id); On 2016/01/06 19:53:44, ajuma wrote: > Should this be target_id (given that the clip we're using below is in target > space)? Do we have unit tests for the case that the clip node's own transform id > is different from its target id? It should be target id. Did not find any unit test with that case. So added one.
On 2016/01/06 22:44:46, jaydasika wrote: > https://codereview.chromium.org/1551333003/diff/100001/cc/trees/layer_tree_im... > File cc/trees/layer_tree_impl.cc (right): > > https://codereview.chromium.org/1551333003/diff/100001/cc/trees/layer_tree_im... > cc/trees/layer_tree_impl.cc:1443: // clip's bounds. Also, the point can be > clipped by the contect rect of an > On 2016/01/06 19:53:44, ajuma wrote: > > s/contect/content > > Done. > > https://codereview.chromium.org/1551333003/diff/100001/cc/trees/layer_tree_im... > cc/trees/layer_tree_impl.cc:1458: > transform_tree.Node(clip_node->data.transform_id); > On 2016/01/06 19:53:44, ajuma wrote: > > Should this be target_id (given that the clip we're using below is in target > > space)? Do we have unit tests for the case that the clip node's own transform > id > > is different from its target id? > > It should be target id. Did not find any unit test with that case. So added one. 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/1551333003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1551333003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== cc:: Use clip tree instead of layer tree during hit testing During hit testing, we walk up the layer tree to check if the test point is clipped by some ancestor. This CL makes it use the clip tree instead. (when property trees are enabled) BUG=568758 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc:: Use clip tree instead of layer tree during hit testing During hit testing, we walk up the layer tree to check if the test point is clipped by some ancestor. This CL makes it use the clip tree instead. (when property trees are enabled) BUG=568758 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/4bab6cab4df2a082f47729972d642afa022afc47 Cr-Commit-Position: refs/heads/master@{#367963} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4bab6cab4df2a082f47729972d642afa022afc47 Cr-Commit-Position: refs/heads/master@{#367963} |