Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26112002:
cc: Fix hit-testing in zero-opacity layers. (Closed)
|
Created:
7 years, 2 months ago by sadrul Modified:
7 years, 1 month ago CC:
chromium-reviews, cc-bugs_chromium.org, jamesr, awong Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncc: Fix hit-testing in zero-opacity layers.
If a layer that accepts touch or wheel events, but the layer (or one of its
ancestor layers) has zero opacity, then make sure the layer hit-tests correctly.
BUG=295295
R=enne@chromium.org, rbyers@google.com
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232881
Patch Set 1 #Patch Set 2 : child is zero-opacity no-handler, grandchild has handler #
Total comments: 3
Patch Set 3 : . #
Total comments: 5
Patch Set 4 : . #
Total comments: 8
Patch Set 5 : . #Patch Set 6 : . #
Total comments: 10
Patch Set 7 : test #
Total comments: 7
Patch Set 8 : . #Patch Set 9 : . #Patch Set 10 : . #Patch Set 11 : . #Patch Set 12 : . #Patch Set 13 : . #Patch Set 14 : . #
Total comments: 6
Patch Set 15 : skip-drawing #Patch Set 16 : self-nit #
Total comments: 2
Patch Set 17 : . #Patch Set 18 : . #Messages
Total messages: 41 (0 generated)
I have added an obvious test case to make sure the hit-testing happens correctly. It looks like at least the OcclusionTracker would deal with this case correctly. But should this change come with tests to make sure update_count() and append_quad_count() remain zero? https://codereview.chromium.org/26112002/diff/4001/cc/trees/layer_tree_host_c... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/4001/cc/trees/layer_tree_host_c... cc/trees/layer_tree_host_common.cc:73: static bool LayerSubtreeCanAcceptInput(LayerType* layer) { This should be called only for subtrees where the root has zero opacity. So this should be called relatively rarely, and so should not have a significant performance impact. But to be on the safe side, should we add a max-depth for the recursion, and just assume the subtree can accept input events if the recursion exceeds the depth?
https://codereview.chromium.org/26112002/diff/4001/cc/trees/layer_tree_host_c... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/4001/cc/trees/layer_tree_host_c... cc/trees/layer_tree_host_common.cc:73: static bool LayerSubtreeCanAcceptInput(LayerType* layer) { On 2013/10/05 07:57:32, sadrul wrote: > This should be called only for subtrees where the root has zero opacity. So this > should be called relatively rarely, and so should not have a significant > performance impact. But to be on the safe side, should we add a max-depth for > the recursion, and just assume the subtree can accept input events if the > recursion exceeds the depth? What about setting a flag in the prerecursion, like layer_or_descendant_has_copy_request?
https://codereview.chromium.org/26112002/diff/4001/cc/trees/layer_tree_host_c... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/4001/cc/trees/layer_tree_host_c... cc/trees/layer_tree_host_common.cc:73: static bool LayerSubtreeCanAcceptInput(LayerType* layer) { On 2013/10/05 16:40:18, enne wrote: > On 2013/10/05 07:57:32, sadrul wrote: > > This should be called only for subtrees where the root has zero opacity. So > this > > should be called relatively rarely, and so should not have a significant > > performance impact. But to be on the safe side, should we add a max-depth for > > the recursion, and just assume the subtree can accept input events if the > > recursion exceeds the depth? > > What about setting a flag in the prerecursion, like > layer_or_descendant_has_copy_request? Oh, cool. That looks much better. Done.
I don't know this cc code very well, but this LGTM. Dana?
Can you skip painting/drawing layers that have a draw_properties().opacity() of 0 the same way you skipped layers that !DrawsContent()? https://codereview.chromium.org/26112002/diff/10001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/10001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:465: Can you add skipping layers that have draw-opacity of 0 here since they will no longer be skipped entirely by subtree? You'll have to pass in the computed draw-opacity similar to layer_is_visible. Can you add a test with a 0 opacity layer with 2 children, one with an event handler. The layer and the non-event handler should not appear in the RSLL. The event handler should.
https://codereview.chromium.org/26112002/diff/10001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/10001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:465: On 2013/10/07 16:29:13, danakj wrote: > Can you add skipping layers that have draw-opacity of 0 here since they will no > longer be skipped entirely by subtree? You'll have to pass in the computed > draw-opacity similar to layer_is_visible. I have updated the code here to look at layer->draw_opacity() (and layer->draw_opacity_is_animation()). I am not sure what you mean by passing in the computed draw-opacity. Do you mean |data_for_children| should carry this information, e.g.: data_for_children.computed_opacity = data_from_ancestor.computed_opacity * layer->opacity(); ? > > Can you add a test with a 0 opacity layer with 2 children, one with an event > handler. The layer and the non-event handler should not appear in the RSLL. The > event handler should. Done.
https://codereview.chromium.org/26112002/diff/20001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:468: return !layer->draw_opacity() && !layer->draw_opacity_is_animating(); I'm not sure this is adequate. If you have: -A -B -C ...and C accepts input and A has zero opacity, then B will now appear in the RSLL where it didn't before. If B is a PictureLayerImpl then it'll go ahead and create resources wastefully. Does there need to be some "in skipped subtree" boolean that gets passed to children and considered in LayerShouldBeSkipped? https://codereview.chromium.org/26112002/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:515: !layer->draw_properties().layer_or_descendant_has_event_handler; Can you break out this condition from the rest of the opacity ones, since it's not related to opacity? Same thing for the other function above?
https://codereview.chromium.org/26112002/diff/10001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/10001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:465: On 2013/10/07 17:16:49, sadrul wrote: > On 2013/10/07 16:29:13, danakj wrote: > > Can you add skipping layers that have draw-opacity of 0 here since they will > no > > longer be skipped entirely by subtree? You'll have to pass in the computed > > draw-opacity similar to layer_is_visible. > > I have updated the code here to look at layer->draw_opacity() (and > layer->draw_opacity_is_animation()). > > I am not sure what you mean by passing in the computed draw-opacity. Do you mean > |data_for_children| should carry this information, e.g.: > > data_for_children.computed_opacity = data_from_ancestor.computed_opacity * > layer->opacity(); > > ? > > > > > Can you add a test with a 0 opacity layer with 2 children, one with an event > > handler. The layer and the non-event handler should not appear in the RSLL. > The > > event handler should. > > Done. I meant that this function doesn't use draw properties directly right now, which lets it be ignorant of the order things are computed and assigned to draw properties. It'd be nice to just pass the |accumulated_draw_opacity| to this function as an argument instead. https://codereview.chromium.org/26112002/diff/20001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:468: return !layer->draw_opacity() && !layer->draw_opacity_is_animating(); Can you make this a if () return true; and keep return false at the end of the method? https://codereview.chromium.org/26112002/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:468: return !layer->draw_opacity() && !layer->draw_opacity_is_animating(); On 2013/10/07 17:27:23, enne wrote: > I'm not sure this is adequate. > > If you have: > -A > -B > -C > > ...and C accepts input and A has zero opacity, then B will now appear in the > RSLL where it didn't before. If B is a PictureLayerImpl then it'll go ahead and > create resources wastefully. > > Does there need to be some "in skipped subtree" boolean that gets passed to > children and considered in LayerShouldBeSkipped? PictureLayerImpl should probably consider its draw opacity the same way it considers DrawsContent?
https://codereview.chromium.org/26112002/diff/20001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:468: return !layer->draw_opacity() && !layer->draw_opacity_is_animating(); On 2013/10/07 17:37:35, danakj wrote: > On 2013/10/07 17:27:23, enne wrote: > > I'm not sure this is adequate. > > > > If you have: > > -A > > -B > > -C > > > > ...and C accepts input and A has zero opacity, then B will now appear in the > > RSLL where it didn't before. If B is a PictureLayerImpl then it'll go ahead > and > > create resources wastefully. > > > > Does there need to be some "in skipped subtree" boolean that gets passed to > > children and considered in LayerShouldBeSkipped? > > PictureLayerImpl should probably consider its draw opacity the same way it > considers DrawsContent? It's more than that. What if you've set a subtree to not be visible and some descendant has a copy request? I'm suggesting that maybe this could be generalized to a "this subtree could not be skipped because of some descendant" property and then every layer in such a tree is individually skipped except for those specific descendants.
On 2013/10/07 17:45:51, enne wrote: > https://codereview.chromium.org/26112002/diff/20001/cc/trees/layer_tree_host_... > File cc/trees/layer_tree_host_common.cc (right): > > https://codereview.chromium.org/26112002/diff/20001/cc/trees/layer_tree_host_... > cc/trees/layer_tree_host_common.cc:468: return !layer->draw_opacity() && > !layer->draw_opacity_is_animating(); > On 2013/10/07 17:37:35, danakj wrote: > > On 2013/10/07 17:27:23, enne wrote: > > > I'm not sure this is adequate. > > > > > > If you have: > > > -A > > > -B > > > -C > > > > > > ...and C accepts input and A has zero opacity, then B will now appear in the > > > RSLL where it didn't before. If B is a PictureLayerImpl then it'll go ahead > > and > > > create resources wastefully. > > > > > > Does there need to be some "in skipped subtree" boolean that gets passed to > > > children and considered in LayerShouldBeSkipped? > > > > PictureLayerImpl should probably consider its draw opacity the same way it > > considers DrawsContent? > > It's more than that. What if you've set a subtree to not be visible and some > descendant has a copy request? > > I'm suggesting that maybe this could be generalized to a "this subtree could not > be skipped because of some descendant" property and then every layer in such a > tree is individually skipped except for those specific descendants. Hm.. I think they are slightly different. The "visible" is a bool that turns off drawing the tree but does not modify how it would be drawn if it was copied. Opacity 0 should affect how it is drawn, and if you did a copy request in the 0-opacity tree, the result should be a transparent texture, shouldn't it? I think in the 0-opacity-with-a-copy-request, we should have been avoiding making tilings the same way as !DrawsConten() but that was an oversight on my part when I added that.
https://codereview.chromium.org/26112002/diff/10001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/10001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:465: On 2013/10/07 17:37:35, danakj wrote: > On 2013/10/07 17:16:49, sadrul wrote: > > On 2013/10/07 16:29:13, danakj wrote: > > > Can you add skipping layers that have draw-opacity of 0 here since they will > > no > > > longer be skipped entirely by subtree? You'll have to pass in the computed > > > draw-opacity similar to layer_is_visible. > > > > I have updated the code here to look at layer->draw_opacity() (and > > layer->draw_opacity_is_animation()). > > > > I am not sure what you mean by passing in the computed draw-opacity. Do you > mean > > |data_for_children| should carry this information, e.g.: > > > > data_for_children.computed_opacity = data_from_ancestor.computed_opacity * > > layer->opacity(); > > > > ? > > > > > > > > Can you add a test with a 0 opacity layer with 2 children, one with an event > > > handler. The layer and the non-event handler should not appear in the RSLL. > > The > > > event handler should. > > > > Done. > > I meant that this function doesn't use draw properties directly right now, which > lets it be ignorant of the order things are computed and assigned to draw > properties. It'd be nice to just pass the |accumulated_draw_opacity| to this > function as an argument instead. Ah, I see. Done. (needed both |accumulated_draw_opacity| and |animating_opacity_to_target| to be passed in) But quickly reverted this change, since it looks like |accumulated_draw_opacity| is not always the same as layer->draw_opacity() (e.g. when |render_to_separate_surface| is set). Should I just pass in layer->draw_opacity() and layer->draw_opacity_is_animating() as parameters to this function, or keep using these draw_properties from within this function? https://codereview.chromium.org/26112002/diff/20001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:468: return !layer->draw_opacity() && !layer->draw_opacity_is_animating(); On 2013/10/07 17:37:35, danakj wrote: > Can you make this a if () return true; and keep return false at the end of the > method? Done. https://codereview.chromium.org/26112002/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:468: return !layer->draw_opacity() && !layer->draw_opacity_is_animating(); On 2013/10/07 17:45:51, enne wrote: > On 2013/10/07 17:37:35, danakj wrote: > > On 2013/10/07 17:27:23, enne wrote: > > > I'm not sure this is adequate. > > > > > > If you have: > > > -A > > > -B > > > -C > > > > > > ...and C accepts input and A has zero opacity, then B will now appear in the > > > RSLL where it didn't before. If B is a PictureLayerImpl then it'll go ahead > > and > > > create resources wastefully. > > > > > > Does there need to be some "in skipped subtree" boolean that gets passed to > > > children and considered in LayerShouldBeSkipped? > > > > PictureLayerImpl should probably consider its draw opacity the same way it > > considers DrawsContent? > > It's more than that. What if you've set a subtree to not be visible and some > descendant has a copy request? > > I'm suggesting that maybe this could be generalized to a "this subtree could not > be skipped because of some descendant" property and then every layer in such a > tree is individually skipped except for those specific descendants. I am going through the code to completely grok this suggestion. https://codereview.chromium.org/26112002/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:515: !layer->draw_properties().layer_or_descendant_has_event_handler; On 2013/10/07 17:27:23, enne wrote: > Can you break out this condition from the rest of the opacity ones, since it's > not related to opacity? Same thing for the other function above? Done.
https://codereview.chromium.org/26112002/diff/10001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/10001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:465: On 2013/10/07 18:37:24, sadrul wrote: > On 2013/10/07 17:37:35, danakj wrote: > > On 2013/10/07 17:16:49, sadrul wrote: > > > On 2013/10/07 16:29:13, danakj wrote: > > > > Can you add skipping layers that have draw-opacity of 0 here since they > will > > > no > > > > longer be skipped entirely by subtree? You'll have to pass in the computed > > > > draw-opacity similar to layer_is_visible. > > > > > > I have updated the code here to look at layer->draw_opacity() (and > > > layer->draw_opacity_is_animation()). > > > > > > I am not sure what you mean by passing in the computed draw-opacity. Do you > > mean > > > |data_for_children| should carry this information, e.g.: > > > > > > data_for_children.computed_opacity = data_from_ancestor.computed_opacity * > > > layer->opacity(); > > > > > > ? > > > > > > > > > > > Can you add a test with a 0 opacity layer with 2 children, one with an > event > > > > handler. The layer and the non-event handler should not appear in the > RSLL. > > > The > > > > event handler should. > > > > > > Done. > > > > I meant that this function doesn't use draw properties directly right now, > which > > lets it be ignorant of the order things are computed and assigned to draw > > properties. It'd be nice to just pass the |accumulated_draw_opacity| to this > > function as an argument instead. > > Ah, I see. Done. (needed both |accumulated_draw_opacity| and > |animating_opacity_to_target| to be passed in) > > But quickly reverted this change, since it looks like |accumulated_draw_opacity| > is not always the same as layer->draw_opacity() (e.g. when > |render_to_separate_surface| is set). Should I just pass in > layer->draw_opacity() and layer->draw_opacity_is_animating() as parameters to > this function, or keep using these draw_properties from within this function? Oh right, inside a surface the opacity goes back to 1.. since the surface itself will apply the opacity < 1 effect. We don't actually track opacity across surfaces at all, since we assumed you'd early out. I think you'll need to store some extra bit to pass down the tree saying that this is 0 opacity. This is not unlike what enne@ suggested as well.
ping
https://codereview.chromium.org/26112002/diff/37001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/37001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:425: bool visit_for_event_handler_only) { how about bool "layer_has_zero_opacity"? only is a bit of a loaded term here, you might be here for event handlers and also for copy requests. https://codereview.chromium.org/26112002/diff/37001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:429: if (visit_for_event_handler_only && Move this down below the big intro comment, and update the intro comment to accomodate this. https://codereview.chromium.org/26112002/diff/37001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:446: // Note, if the layer should not have been drawn due to being fully This comment is wrong then, and needs updating. https://codereview.chromium.org/26112002/diff/37001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:477: if (!layer->draw_opacity() && !layer->draw_opacity_is_animating()) Animating only matters on the main thread. On the impl thread we should ignore it. This needs to be covered by a test. If the layer owns a surface, it'll have draw opacity of 1 and I believe that visit_for_event_handler_only will be false. But it should be skipped since its surface has opacity 0. This needs to be covered by a test. https://codereview.chromium.org/26112002/diff/37001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common.cc:1895: layer_draw_properties.visible_content_rect = Leave this rect alone. If the layer has 0 opacity but is going to animate to be visible, we'd like to not destroy our chances of knowing what to raster. https://codereview.chromium.org/26112002/diff/37001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/26112002/diff/37001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common_unittest.cc:5598: // Child 1 - Zero opacity, without any event handler. Can you add a case where there is - Opacity 0 layer that DrawsContent() +-- Render surface owning layer (SetForceRenderSurface(true)) that DrawsContent() +-- Touch handler layer. In this case, the 2 ancestors own render surfaces so they should appear in the RSLL. But not in any surface layer_list(). And the touch handler layer should appear in a layer_list() of the latter surface. None of the layers or surfaces should generate any quads in CalculateRenderPasses. I think you need a separate set of tests to verify update (main thread) append quads (impl thread) isn't happening for these layers in each of the cases you're testing here. (And code to make it not happen.) I think you also need a test to verify we don't make tiles for invisible layers since we're going to now call UpdateTilePriorities() on them when we would not have before. If I may suggest, the easiest way to go about all of this would be to decouple UpdateTilePrios and Choosing to Update()/AppendQuads() entirely from this complicated logic around opacity and such. If CalcDrawProps has an output variable (a draw property) that says if the layer should be drawn as part of the frame or not, then these other systems can key off that. And you only need 1 test for each other system to ensure it is using said key, and all your tests for CalcDrawProps can ensure the right key output. I think that I would recommend doing that as a first step, making a "draws_content" (or some better name?) draw property, and having Occlusion/Damage/AppendQuads/Update/Tiling use that value instead of the input-variable "DrawsContent()". Along this train of thought, I had suggested in IRC maybe it would be nice to have compiler enforce this type of thing by only allowing access to draw properties via the RSLL so there's no confusion about what properties make sense and are being used in all these systems, by storing a DrawLayer(Impl) in RSLL and layer_list() instead of Layer(Impl*) directly, and having it wrap access to the underlying draw_properties() and AppendQuads() stuff and prevent access to anything else. I'm not sure enne@ bought into that idea though, so maybe it's not worth going that far. https://codereview.chromium.org/26112002/diff/37001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common_unittest.cc:5600: gfx::Size bounds(30, 30); Can you add a case where the touch handler layer owns a render surface as well? - Opacity 0 layer that DrawsContent() +-- Touch handler layer with SetForceRenderSurface(true) And maybe - Opacity 0 layer that DrawsContent() and SetForceRenderSurface(true) and has touch handler. https://codereview.chromium.org/26112002/diff/37001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common_unittest.cc:5610: child->SetOpacity(0.f); Can you make all of the layers in this test DrawsContent(true) so that we generate render surfaces for them with opacity < 1. https://codereview.chromium.org/26112002/diff/37001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common_unittest.cc:5644: // Child 2 - Zero opacity, with touch event handler. Can you add multiple drawing children of this layer so that it would own a surface. https://codereview.chromium.org/26112002/diff/37001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common_unittest.cc:5662: // Child 3 - Zero opacity, with wheel event handler. Could you split each of these 3 children cases into separate tests so each test is smaller/easier to read.
Some more combinatorially different cases to cover - the layer having a child, the layer owning a surface itself etc. I think the expectations in the 2nd test are correct. One fix for the first test. It occurred to me that I solved a similar problem with the readback code - where we need to draw/readback something even though its subtree was hidden. The RenderSurfaceImpl::contributes_to_drawn_surface flag is used for that, and could be used here as well to prevent the RS from generating quads. Maybe you can piggy-back/extend the |layer_is_visible| flag in LTHCommon to cover both of these cases (readbacks in a hidden tree and touch handlers in an invisible tree)? In both we are traversing thru a tree and not adding anything to the list until we get to some future layer that has a counter-argument saying it wants to be added. https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common_unittest.cc:5506: EXPECT_EQ(kNoHandlerChildId, This shouldn't be present, it doesn't have a handler and it doesn't draw.
https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common_unittest.cc:5596: zero_opacity_raw->render_surface()->layer_list().size()); One of the issues I ran into is that RemoveSurfaceForEarlyExit() removes the surfaces that don't have any layers. So I introduced a |SubtreeGlobals::can_remove_surface_for_early_exit| (and a corresponding entry in CalcDrawPropsInput), which defaults to true, and I set it to false from the test here. This sounds good to you?
https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common_unittest.cc:5596: zero_opacity_raw->render_surface()->layer_list().size()); On 2013/10/10 17:08:46, sadrul wrote: > One of the issues I ran into is that RemoveSurfaceForEarlyExit() removes the > surfaces that don't have any layers. So I introduced a > |SubtreeGlobals::can_remove_surface_for_early_exit| (and a corresponding entry > in CalcDrawPropsInput), which defaults to true, and I set it to false from the > test here. This sounds good to you? Hm... Maybe it's okay and better to not have those surfaces with 0 layers. As long as the LayerIterator can correctly iterate over everything then we're okay. I just want to be careful to not end up with an invalid RSLL that the LayerIterator blows up on. Since those surfaces will not have a layer representing them in another surface's layer_list() either, there's maybe no reason to keep them.
https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common_unittest.cc:5596: zero_opacity_raw->render_surface()->layer_list().size()); On 2013/10/10 17:16:38, danakj wrote: > On 2013/10/10 17:08:46, sadrul wrote: > > One of the issues I ran into is that RemoveSurfaceForEarlyExit() removes the > > surfaces that don't have any layers. So I introduced a > > |SubtreeGlobals::can_remove_surface_for_early_exit| (and a corresponding entry > > in CalcDrawPropsInput), which defaults to true, and I set it to false from the > > test here. This sounds good to you? > > Hm... > > Maybe it's okay and better to not have those surfaces with 0 layers. As long as > the LayerIterator can correctly iterate over everything then we're okay. I just > want to be careful to not end up with an invalid RSLL that the LayerIterator > blows up on. > > Since those surfaces will not have a layer representing them in another > surface's layer_list() either, there's maybe no reason to keep them. |RemoveSurfaceForEarlyExit()| also removes all surfaces after the empty surface. So we also lose the |kForceSurfaceLayer| from the RSLL :( // Technically, we know that the layer we want to remove should be // at the back of the render_surface_layer_list. However, we have had // bugs before that added unnecessary layers here // (https://bugs.webkit.org/show_bug.cgi?id=74147), but that causes // things to crash. So here we proactively remove any additional // layers from the end of the list. while (render_surface_layer_list->back() != layer_to_remove) { render_surface_layer_list->back()->ClearRenderSurface(); render_surface_layer_list->pop_back(); } DCHECK_EQ(render_surface_layer_list->back(), layer_to_remove); Should I change this piece of code to do something different?
https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common_unittest.cc:5587: EXPECT_EQ(kRootId, render_surface_layer_list[0]->id()); The other issue I am hitting is, when we get kRootId from a hit-test on |touch_handler_layer| (looks like because the root is first in the RSLL, and the hit-test on the root is successful, and the touch_handler_layer is not in the root's layer_list()). Should the list be reversed for hit-test?
https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common_unittest.cc:5587: EXPECT_EQ(kRootId, render_surface_layer_list[0]->id()); On 2013/10/10 18:01:31, sadrul wrote: > The other issue I am hitting is, when we get kRootId from a hit-test on > |touch_handler_layer| (looks like because the root is first in the RSLL, and the > hit-test on the root is successful, and the touch_handler_layer is not in the > root's layer_list()). Should the list be reversed for hit-test? Yes. The front-to-back iterator that the LayerTreeHostCommon::FindLayerThatIsHitByPoint function uses goes in reverse order. https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_common_unittest.cc:5596: zero_opacity_raw->render_surface()->layer_list().size()); On 2013/10/10 17:41:39, sadrul wrote: > On 2013/10/10 17:16:38, danakj wrote: > > On 2013/10/10 17:08:46, sadrul wrote: > > > One of the issues I ran into is that RemoveSurfaceForEarlyExit() removes the > > > surfaces that don't have any layers. So I introduced a > > > |SubtreeGlobals::can_remove_surface_for_early_exit| (and a corresponding > entry > > > in CalcDrawPropsInput), which defaults to true, and I set it to false from > the > > > test here. This sounds good to you? > > > > Hm... > > > > Maybe it's okay and better to not have those surfaces with 0 layers. As long > as > > the LayerIterator can correctly iterate over everything then we're okay. I > just > > want to be careful to not end up with an invalid RSLL that the LayerIterator > > blows up on. > > > > Since those surfaces will not have a layer representing them in another > > surface's layer_list() either, there's maybe no reason to keep them. > > |RemoveSurfaceForEarlyExit()| also removes all surfaces after the empty surface. > So we also lose the |kForceSurfaceLayer| from the RSLL :( > > // Technically, we know that the layer we want to remove should be > // at the back of the render_surface_layer_list. However, we have had > // bugs before that added unnecessary layers here > // (https://bugs.webkit.org/show_bug.cgi?id=74147), but that causes > // things to crash. So here we proactively remove any additional > // layers from the end of the list. > while (render_surface_layer_list->back() != layer_to_remove) { > render_surface_layer_list->back()->ClearRenderSurface(); > render_surface_layer_list->pop_back(); > } > DCHECK_EQ(render_surface_layer_list->back(), layer_to_remove); > > Should I change this piece of code to do something different? Ack. That's really unfortunate. As the comment says, this safety has been useful in the past in preventing crashes here. I am also not fully convinced that the front to back iterator will do the right thing if these surfaces are skipped. One alternative that Dana and I talked out a little bit was just to make one flattened list hit-testing list as an additional output from CalculateDrawProperties. This would keep the RSLL (things that get drawn) separate from hit-testing. Would that make things easier?
On 2013/10/15 17:56:49, enne wrote: > https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... > File cc/trees/layer_tree_host_common_unittest.cc (right): > > https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... > cc/trees/layer_tree_host_common_unittest.cc:5587: EXPECT_EQ(kRootId, > render_surface_layer_list[0]->id()); > On 2013/10/10 18:01:31, sadrul wrote: > > The other issue I am hitting is, when we get kRootId from a hit-test on > > |touch_handler_layer| (looks like because the root is first in the RSLL, and > the > > hit-test on the root is successful, and the touch_handler_layer is not in the > > root's layer_list()). Should the list be reversed for hit-test? > > Yes. The front-to-back iterator that the > LayerTreeHostCommon::FindLayerThatIsHitByPoint function uses goes in reverse > order. > > https://codereview.chromium.org/26112002/diff/46001/cc/trees/layer_tree_host_... > cc/trees/layer_tree_host_common_unittest.cc:5596: > zero_opacity_raw->render_surface()->layer_list().size()); > On 2013/10/10 17:41:39, sadrul wrote: > > On 2013/10/10 17:16:38, danakj wrote: > > > On 2013/10/10 17:08:46, sadrul wrote: > > > > One of the issues I ran into is that RemoveSurfaceForEarlyExit() removes > the > > > > surfaces that don't have any layers. So I introduced a > > > > |SubtreeGlobals::can_remove_surface_for_early_exit| (and a corresponding > > entry > > > > in CalcDrawPropsInput), which defaults to true, and I set it to false from > > the > > > > test here. This sounds good to you? > > > > > > Hm... > > > > > > Maybe it's okay and better to not have those surfaces with 0 layers. As long > > as > > > the LayerIterator can correctly iterate over everything then we're okay. I > > just > > > want to be careful to not end up with an invalid RSLL that the LayerIterator > > > blows up on. > > > > > > Since those surfaces will not have a layer representing them in another > > > surface's layer_list() either, there's maybe no reason to keep them. > > > > |RemoveSurfaceForEarlyExit()| also removes all surfaces after the empty > surface. > > So we also lose the |kForceSurfaceLayer| from the RSLL :( > > > > // Technically, we know that the layer we want to remove should be > > // at the back of the render_surface_layer_list. However, we have had > > // bugs before that added unnecessary layers here > > // (https://bugs.webkit.org/show_bug.cgi?id=74147), but that causes > > // things to crash. So here we proactively remove any additional > > // layers from the end of the list. > > while (render_surface_layer_list->back() != layer_to_remove) { > > render_surface_layer_list->back()->ClearRenderSurface(); > > render_surface_layer_list->pop_back(); > > } > > DCHECK_EQ(render_surface_layer_list->back(), layer_to_remove); > > > > Should I change this piece of code to do something different? > > Ack. That's really unfortunate. As the comment says, this safety has been > useful in the past in preventing crashes here. > > I am also not fully convinced that the front to back iterator will do the right > thing if these surfaces are skipped. > > One alternative that Dana and I talked out a little bit was just to make one > flattened list hit-testing list as an additional output from > CalculateDrawProperties. This would keep the RSLL (things that get drawn) > separate from hit-testing. Would that make things easier? This sounds interesting. I thought about it some more, and I am not entirely sure how this would work. To give an example, consider this layer list: Root (R) -> Touch layer (T) -> Non-touch layer (L) T is at 0,0+50x50, and L is 0,25+50x50, and L is above T. In this scenario, if this hit-testing list contains just T, then a hit-test for (25,35) would hit T, and will return it as the hit-testing target, where the correct target would be L, which doesn't actually process any events, and so there would be no candidate target layer for this event (https://codereview.chromium.org/27718006/). So maintaining just the list of layers that can process input events wouldn't be sufficient for hit-testing. Does this make sense?
On 2013/10/17 20:40:10, sadrul wrote: > > One alternative that Dana and I talked out a little bit was just to make one > > flattened list hit-testing list as an additional output from > > CalculateDrawProperties. This would keep the RSLL (things that get drawn) > > separate from hit-testing. Would that make things easier? > > This sounds interesting. I thought about it some more, and I am not entirely > sure how this would work. To give an example, consider this layer list: > > Root (R) > -> Touch layer (T) > -> Non-touch layer (L) > > T is at 0,0+50x50, and L is 0,25+50x50, and L is above T. > > In this scenario, if this hit-testing list contains just T, then a hit-test for > (25,35) would hit T, and will return it as the hit-testing target, where the > correct target would be L, which doesn't actually process any events, and so > there would be no candidate target layer for this event > (https://codereview.chromium.org/27718006/). So maintaining just the list of > layers that can process input events wouldn't be sufficient for hit-testing. > Does this make sense? That's a really good example. To take that one unfortunate step further, consider this tree: Root (R) -> Opacity 0 surface #1 (S1) -> Touch layer (T) -> Opacity 0 surface #2 (S2) -> Non-touch layer (L) I'm assuming that touches that hit L (but would have hit T had L not been there) should not be considered as touching T since L blocks it. If this is the case, then it's a world of unfortunate consequences. If so, it means that any hit-testing solution needs to include all layers: visible, invisible, hit-testing, not hit-testing. That also means that if you want to hit test on the RSLL, then you need to calculate draw properties for all of these invisible S1 and S2 descendants that we normally early out on, even though S2 has no touch descendants. In other words, you can't early out of calc draw properties for any layer or subtree that comes in the tree anywhere after a layer with a touch handler has been processed. Yuck. From your question above, it seems like trying to get these layers into the RSLL is pretty complicated. Maybe we could just walk the layer tree directly? The reason we use the RSLL is because it's guaranteed that every layer in that list has an up-to-date draw properties. (Other layers have bogus or stale draw properties when not in the list). One naive option would be to calculate draw properties for every layer (never early-outing) and then just walk the layer tree directly. This seems unfortunately inefficient. Another option might be to mark which subtrees got skipped and then (during hit-testing) lazily compute them or just re-run CalcDrawProperties with some "no skip" option. However, this seems unfortunately complex. Other options? Am I mistaken in my assumptions here?
On Thu, Oct 17, 2013 at 5:09 PM, <enne@chromium.org> wrote: > On 2013/10/17 20:40:10, sadrul wrote: > > > One alternative that Dana and I talked out a little bit was just to >> make one >> > flattened list hit-testing list as an additional output from >> > CalculateDrawProperties. This would keep the RSLL (things that get >> drawn) >> > separate from hit-testing. Would that make things easier? >> > > This sounds interesting. I thought about it some more, and I am not >> entirely >> sure how this would work. To give an example, consider this layer list: >> > > Root (R) >> -> Touch layer (T) >> -> Non-touch layer (L) >> > > T is at 0,0+50x50, and L is 0,25+50x50, and L is above T. >> > > In this scenario, if this hit-testing list contains just T, then a >> hit-test >> > for > >> (25,35) would hit T, and will return it as the hit-testing target, where >> the >> correct target would be L, which doesn't actually process any events, and >> so >> there would be no candidate target layer for this event >> (https://codereview.chromium.**org/27718006/<https://codereview.chromium.org/2...). >> So maintaining just the list of >> layers that can process input events wouldn't be sufficient for >> hit-testing. >> Does this make sense? >> > > That's a really good example. To take that one unfortunate step further, > consider this tree: > > Root (R) > -> Opacity 0 surface #1 (S1) > -> Touch layer (T) > -> Opacity 0 surface #2 (S2) > -> Non-touch layer (L) > > I'm assuming that touches that hit L (but would have hit T had L not been > there) > should not be considered as touching T since L blocks it. If this is the > case, > then it's a world of unfortunate consequences. > > If so, it means that any hit-testing solution needs to include all layers: > visible, invisible, hit-testing, not hit-testing. That also means that if > you > want to hit test on the RSLL, then you need to calculate draw properties > for all > of these invisible S1 and S2 descendants that we normally early out on, > even > though S2 has no touch descendants. In other words, you can't early out > of calc > draw properties for any layer or subtree that comes in the tree anywhere > after a > layer with a touch handler has been processed. Yuck. > > From your question above, it seems like trying to get these layers into > the RSLL > is pretty complicated. Maybe we could just walk the layer tree directly? > The > reason we use the RSLL is because it's guaranteed that every layer in that > list > has an up-to-date draw properties. (Other layers have bogus or stale draw > properties when not in the list). > Also sorting. > One naive option would be to calculate draw properties for every layer > (never > early-outing) and then just walk the layer tree directly. This seems > unfortunately inefficient. > > Another option might be to mark which subtrees got skipped and then (during > hit-testing) lazily compute them or just re-run CalcDrawProperties with > some "no > skip" option. However, this seems unfortunately complex. > > Other options? Am I mistaken in my assumptions here? > > https://codereview.chromium.**org/26112002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
+jamesr, ajwong for surface implications FYI sadrul/danakj/enne had a talk yesterday about how to handle the above concerns, and from my perspective there's two options: (1) Do lots of refactoring into a cleaner design. CalcDrawProperties operates on Layers/RenderSurfaces/DrawProperties as both inputs and outputs. If this were split out into more of a clear pipeline of inputs and outputs (i.e. RSLL output owning render surfaces, layer lists, and draw properties rather than layers directly or indirectly owning them) then we could first CalcDrawProperties for drawing and then on-demand CalcDrawProperties for hit-testing with some special flag, ignoring early outs and calculating information for all layers (more or less). Only by making CalcDrawProperties not destructive of its previous outputs can this be a sane operation. (2) Disable early-outs for layers and subtrees in CalcDrawProperties. Once the layer with a touch handler, or a layer creating a 3d context with a touch handler descendant is hit, no longer early out layers or subtrees. Prior to that point, don't early out for any subtree with a touch handler descendant. This will process more layers and we'll probably need a draw property flag to inform layers that they will not be drawn so that they can skip creating resources, etc. I was initially leaning towards #1, but now that I realize that if we want to do hit testing in the browser, then we'll need to do option #2, calculate properties for all layers, and then ship invisible quads up to the browser. There's no way to only pay that cost during a hit test. My thought then is that we should decouple refactoring (which might be good to do eventually anyway but is a bunch of work) from correctness (which is a lot easier). The performance implications makes me sad, but I don't know what else to do. Maybe we could UMA stat this when we do and see if our worries are overblown. Maybe somebody will have a brilliant option #3, but I sure can't think of any at the moment.
Option #3 is to be more conservative and map an inflated version of an invisible-layer-with-hit-regions into its nearest visible ancestor. It's not great - I think in many situations we'd have to consider everything potentially touchable on lots of pages. I agree that #2 seems the most sensible for now. We won't need to do any per-tile work for invisible layers that make it through CalcDraw...(), just per-layer work, so maybe it won't be that bad.
On Fri, Oct 18, 2013 at 3:13 PM, <jamesr@chromium.org> wrote: > Option #3 is to be more conservative and map an inflated version of an > invisible-layer-with-hit-**regions into its nearest visible ancestor. > It's not > great - I think in many situations we'd have to consider everything > potentially > touchable on lots of pages. > Also it should be more precise if we're going to use it to route touch events from the browser, if we inflate we send touches to the wrong place. > I agree that #2 seems the most sensible for now. We won't need to do any > per-tile work for invisible layers that make it through CalcDraw...(), just > per-layer work, so maybe it won't be that bad. > Just to devil's advocate a little bit, things like spaceport make 4000 layers. In this case they're visible, but I won't be surprised to find some benchmark that demonstrates badness here. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> > (2) Disable early-outs for layers and subtrees in CalcDrawProperties. > > Once the layer with a touch handler, or a layer creating a 3d context with a > touch handler descendant is hit, no longer early out layers or subtrees. Prior > to that point, don't early out for any subtree with a touch handler descendant. > This will process more layers and we'll probably need a draw property flag to > inform layers that they will not be drawn so that they can skip creating > resources, etc. Does this essentially mean that if any layer in the tree has an event handler (touch or wheel), then none of layer in the tree is skipped?
On 2013/10/26 04:05:37, sadrul wrote: > > > > (2) Disable early-outs for layers and subtrees in CalcDrawProperties. > > > > Once the layer with a touch handler, or a layer creating a 3d context with a > > touch handler descendant is hit, no longer early out layers or subtrees. > Prior > > to that point, don't early out for any subtree with a touch handler > descendant. > > This will process more layers and we'll probably need a draw property flag to > > inform layers that they will not be drawn so that they can skip creating > > resources, etc. > > Does this essentially mean that if any layer in the tree has an event handler > (touch or wheel), then none of layer in the tree is skipped? Yeah. I don't think there's any easy way around that.
On 2013/10/26 05:00:50, enne wrote: > On 2013/10/26 04:05:37, sadrul wrote: > > > > > > (2) Disable early-outs for layers and subtrees in CalcDrawProperties. > > > > > > Once the layer with a touch handler, or a layer creating a 3d context with a > > > touch handler descendant is hit, no longer early out layers or subtrees. > > Prior > > > to that point, don't early out for any subtree with a touch handler > > descendant. > > > This will process more layers and we'll probably need a draw property flag > to > > > inform layers that they will not be drawn so that they can skip creating > > > resources, etc. > > > > Does this essentially mean that if any layer in the tree has an event handler > > (touch or wheel), then none of layer in the tree is skipped? > > Yeah. I don't think there's any easy way around that. I have made this change. I have added a couple of tests to verify that Update() isn't called for zero-opacity layers, and these layers don't occlude layers behind them. PTAL.
https://codereview.chromium.org/26112002/diff/339001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/26112002/diff/339001/cc/trees/layer_tree_host... cc/trees/layer_tree_host.cc:1061: bool zero_opacity = !it->draw_opacity() && Zero opacity is not the only reason to skip a layer or a subtree. Can you add a "visible" draw property that both append quads and update layers can respect? It would be set to true if before this patch that layer would end up in the RSLL (i.e. would be skipped except for input) and false otherwise. https://codereview.chromium.org/26112002/diff/339001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/339001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_common.cc:67: static inline bool LayerCanAcceptInput(LayerType* layer) { Maybe call this LayerHasEventHandler to be consistent? https://codereview.chromium.org/26112002/diff/339001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_common.cc:464: if (layer->draw_properties().layer_or_descendant_has_event_handler) I don't think this is sufficient. I feel like what I said in comment #24 is the right way to go about this. As you are processing layers recursively, there is a point of no return after which all layers must be processed. If you ever: * process a layer with a touch handler * process a layer that creates a 3d context with a touch handler descendant. ...then after that point no layer or subtree should be skipped for opacity reasons. (Are there other properties other than opacity that need to be considered? Can a backfacing layer block input?) Prior to that point of no return, no subtree with a touch handler descendant or layer with a touch handler should be skipped.
https://codereview.chromium.org/26112002/diff/339001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/339001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_common.cc:464: if (layer->draw_properties().layer_or_descendant_has_event_handler) On 2013/10/29 23:54:22, enne wrote: > I don't think this is sufficient. I feel like what I said in comment #24 is the > right way to go about this. > > As you are processing layers recursively, there is a point of no return after > which all layers must be processed. If you ever: > * process a layer with a touch handler > * process a layer that creates a 3d context with a touch handler descendant. > ...then after that point no layer or subtree should be skipped for opacity > reasons. > > (Are there other properties other than opacity that need to be considered? Can a > backfacing layer block input?) > > Prior to that point of no return, no subtree with a touch handler descendant or > layer with a touch handler should be skipped. I am using DataForRecursion::subtree_has_event_handler to keep track of whether a layer is part of a subtree that has a layer with an event handler. When |subtree_has_event_handler| is set, the call to LayerShouldBeSkipped() or SubtreeShouldBeSkipped() aren't made at all. So, essentially, no layer/subtree in the entire tree is skipped when there is any layer that handles touch/wheel events. To explain with an example: Root +---> L1 | +----> L2 +---> L3 L1 has opacity zero, L3 has an event handler. In this case, neither L1 nor L2 are skipped. I may have misunderstood, but I believe this is what we want?
https://codereview.chromium.org/26112002/diff/339001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/26112002/diff/339001/cc/trees/layer_tree_host... cc/trees/layer_tree_host.cc:1061: bool zero_opacity = !it->draw_opacity() && On 2013/10/29 23:54:22, enne wrote: > Zero opacity is not the only reason to skip a layer or a subtree. Can you add > a "visible" draw property that both append quads and update layers can respect? > It would be set to true if before this patch that layer would end up in the RSLL > (i.e. would be skipped except for input) and false otherwise. Done (I am calling it 'skip_drawing' to be more like *ShouldBeSkipped()). https://codereview.chromium.org/26112002/diff/339001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/26112002/diff/339001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_common.cc:67: static inline bool LayerCanAcceptInput(LayerType* layer) { On 2013/10/29 23:54:22, enne wrote: > Maybe call this LayerHasEventHandler to be consistent? Done.
On 2013/10/30 05:20:20, sadrul wrote: > To explain with an example: > > Root > +---> L1 > | +----> L2 > +---> L3 > > L1 has opacity zero, L3 has an event handler. In this case, neither L1 nor L2 > are skipped. > > I may have misunderstood, but I believe this is what we want? Ok, I think I misunderstood your patch. If I'm reading it correctly, when a touch handler exists anywhere in the tree, no layers or subtrees are skipped anywhere in the tree. So, "subtree_has_event_handler" is really not mutable data for recursion but is instead a global that's true for the entire CalcDrawProperties pass, right?
On 2013/10/30 22:20:49, enne wrote: > On 2013/10/30 05:20:20, sadrul wrote: > > > To explain with an example: > > > > Root > > +---> L1 > > | +----> L2 > > +---> L3 > > > > L1 has opacity zero, L3 has an event handler. In this case, neither L1 nor L2 > > are skipped. > > > > I may have misunderstood, but I believe this is what we want? > > Ok, I think I misunderstood your patch. If I'm reading it correctly, when a > touch handler exists anywhere in the tree, no layers or subtrees are skipped > anywhere in the tree. So, "subtree_has_event_handler" is really not mutable > data for recursion but is instead a global that's true for the entire > CalcDrawProperties pass, right? Right. Perhaps DataForRecursion is not the right place for this, and this should be in SubtreeGlobals? (I did this in a more weird way in my earlier patch)
> Right. Perhaps DataForRecursion is not the right place for this, and this should > be in SubtreeGlobals? (I did this in a more weird way in my earlier patch) Yeah, I think that would be a lot more clear. Can you change the wording of subtree to maybe just tree to also clarify it more? I think there needs to be some more work done around quads, too. Right now invisible layers might generate non-invisible quads. Should there be an additional quad type (that doesn't draw) or maybe a flag on the SharedQuadState? https://codereview.chromium.org/26112002/diff/489001/cc/layers/draw_properties.h File cc/layers/draw_properties.h (right): https://codereview.chromium.org/26112002/diff/489001/cc/layers/draw_propertie... cc/layers/draw_properties.h:126: bool skip_drawing; Can you add a comment here to describe this?
> > Right. Perhaps DataForRecursion is not the right place for this, and this > > should > > be in SubtreeGlobals? (I did this in a more weird way in my earlier patch) > > Yeah, I think that would be a lot more clear. Can you change the wording of > subtree to maybe just tree to also clarify it more? Done. > I think there needs to be some more work done around quads, too. Right now > invisible layers might generate non-invisible quads. Should there be an > additional quad type (that doesn't draw) or maybe a flag on the SharedQuadState? There is a test in this CL to ensure that zero opacity layers don't get any quads (i.e. AppendQuads() doesn't get called for them) . Do you think there needs to be additional changes after that? https://codereview.chromium.org/26112002/diff/489001/cc/layers/draw_properties.h File cc/layers/draw_properties.h (right): https://codereview.chromium.org/26112002/diff/489001/cc/layers/draw_propertie... cc/layers/draw_properties.h:126: bool skip_drawing; On 2013/10/31 18:15:12, enne wrote: > Can you add a comment here to describe this? Done.
On 2013/10/31 22:49:54, sadrul wrote: > > I think there needs to be some more work done around quads, too. Right now > > invisible layers might generate non-invisible quads. Should there be an > > additional quad type (that doesn't draw) or maybe a flag on the > SharedQuadState? > > There is a test in this CL to ensure that zero opacity layers don't get any > quads (i.e. AppendQuads() doesn't get called for them) . Do you think there > needs to be additional changes after that? Yeah, I think you need to change https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... to check skip_drawing. There are other reasons layers can get skipped (e.g. backfacing) that aren't zero opacity. What makes layers with 0 draw opacity but that are in the RSLL not get AppendQuads called on them? I'm not sure I see that code.
Also, I think that the iteration over layers to UpdateTilePriorities inside of LayerTreeImpl::UpdateDrawProperties should respect this skip_drawing flag too.
On 2013/11/01 18:38:21, enne wrote: > On 2013/10/31 22:49:54, sadrul wrote: > > > > I think there needs to be some more work done around quads, too. Right now > > > invisible layers might generate non-invisible quads. Should there be an > > > additional quad type (that doesn't draw) or maybe a flag on the > > SharedQuadState? > > > > There is a test in this CL to ensure that zero opacity layers don't get any > > quads (i.e. AppendQuads() doesn't get called for them) . Do you think there > > needs to be additional changes after that? > > Yeah, I think you need to change > https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... > to check skip_drawing. There are other reasons layers can get skipped (e.g. > backfacing) that aren't zero opacity. > > What makes layers with 0 draw opacity but that are in the RSLL not get > AppendQuads called on them? I'm not sure I see that code. You are right. There was no such code. My test was buggy. I have made this change and fixed the test. On 2013/11/04 19:49:20, enne wrote: > Also, I think that the iteration over layers to UpdateTilePriorities inside of > LayerTreeImpl::UpdateDrawProperties should respect this skip_drawing flag too. Done. Added a corresponding test for this.
lgtm, thanks! jamesr/awong FYI: when the site isolation folks want to start hit-testing in the browser, then this will have to be extended to generate quads for these skipped layers, but there's no need to implement that now until that point.
Message was sent while issue was closed.
Committed patchset #18 manually as r232881 (presubmit successful). |
