Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1811423002:
SubtreeShouldBeSkipped uses information from property trees (Closed)
DescriptionSubtreeShouldBeSkipped uses information from property trees, and is
renamed to LayerShouldBeSkippedWithMaskAndReplica, as it is now skipping
individual layers instead of subtrees.
The LayerImpl version FindLayersThatNeedUpdates uses LayerList
iterator and the new version of LayerShouldBeSkippedWithMaskAndReplica.
Fix the unit tests.
BUG=592440
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/71aea3ee6feb1ca9ca8b4b500a1d95f29d7ebbc6
Cr-Commit-Position: refs/heads/master@{#384720}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : For trybot #Patch Set 4 : Fix unit tests #
Total comments: 19
Patch Set 5 : Inherit skip criteria #Patch Set 6 : Rebase #Patch Set 7 : Fix windows compile failure #
Total comments: 19
Patch Set 8 : Make RSLL update use the same logic as SubtreeShouldBeSkipped #Patch Set 9 : Fix windows cc_unittests failure #Patch Set 10 : Remove comments #
Total comments: 7
Patch Set 11 : Resolve comments #
Total comments: 10
Patch Set 12 : Test failure #Patch Set 13 : Resolve comments #
Total comments: 9
Patch Set 14 : Resolve comments #Patch Set 15 : Add TODO for large transforms. #Patch Set 16 : Rebase #
Messages
Total messages: 37 (14 generated)
Description was changed from ========== SubtreeShouldBeSkipped uses information from property trees BUG=592440 ========== to ========== SubtreeShouldBeSkipped uses information from property trees BUG=592440 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== SubtreeShouldBeSkipped uses information from property trees BUG=592440 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== SubtreeShouldBeSkipped uses information from property trees, and is renamed to LayerShouldBeSkippedWithMaskAndReplica. The LayerImpl version FindLayersThatNeedUpdates uses LayerList iterator and the new version of LayerShouldBeSkippedWithMaskAndReplica. Fix the unit tests. BUG=592440 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
sunxd@chromium.org changed reviewers: + ajuma@chromium.org, jaydasika@chromium.org, vollick@chromium.org
ajuma@chromium.org changed reviewers: + weiliangc@chromium.org
https://codereview.chromium.org/1811423002/diff/60001/cc/test/layer_test_comm... File cc/test/layer_test_common.h (right): https://codereview.chromium.org/1811423002/diff/60001/cc/test/layer_test_comm... cc/test/layer_test_common.h:167: LayerImpl* root_layer_impl_; Rather than hanging on to a raw pointer here, can we change root_layer() to get the root from the tree? https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:289: static inline bool LayerShouldBeSkippedWithMaskAndReplica( This name is a bit awkward. How about calling this LayerShouldBeSkipped, and renaming the existing LayerShouldBeSkipped to LayerNeedsUpdate (and inverting the return value of that)? https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:301: if (!(transform_node->data.is_invertible || transform_node->data.is_animated)) This needs to check data.ancestors_are_invertible and data.to_screen_is_animated (since we're now skipping layer-by-layer rather than by subtree). Please add tests for this too. https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:317: effect_node->data.has_animated_opacity) This needs to check whether any ancestor node has animated opacity, that is, we need to add an ancestor_has_opacity_animation (or "to_screen_opacity_is_animated") to each effect node. Please also add a test. https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:322: if (effect_node->data.ancestor_has_background_filters || If we define "ancestor_has_background_filters" to include the node itself, then we only need to check that here. https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:329: return !effect_node->data.opacity; This needs to check screen_space_opacity. And needs a test too :) https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:470: } The loop body has the same logic as the Layer version. Can we put this into a shared templated function that gets called in both places? https://codereview.chromium.org/1811423002/diff/60001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (left): https://codereview.chromium.org/1811423002/diff/60001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:566: DCHECK(!root_layer_ || active_page_scale == 1); Please don't remove this -- see the related discussion in Jayadev's CL: https://codereview.chromium.org/1823833002/.
Description was changed from ========== SubtreeShouldBeSkipped uses information from property trees, and is renamed to LayerShouldBeSkippedWithMaskAndReplica. The LayerImpl version FindLayersThatNeedUpdates uses LayerList iterator and the new version of LayerShouldBeSkippedWithMaskAndReplica. Fix the unit tests. BUG=592440 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== SubtreeShouldBeSkipped uses information from property trees, and is renamed to LayerShouldBeSkippedWithMaskAndReplica, as it is now skipping individual layers instead of subtrees. The LayerImpl version FindLayersThatNeedUpdates uses LayerList iterator and the new version of LayerShouldBeSkippedWithMaskAndReplica. Fix the unit tests. BUG=592440 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Three tests broke because of float matrix multiplication, I'm not very sure if my fixes are correct. https://codereview.chromium.org/1811423002/diff/60001/cc/test/layer_test_comm... File cc/test/layer_test_common.h (right): https://codereview.chromium.org/1811423002/diff/60001/cc/test/layer_test_comm... cc/test/layer_test_common.h:167: LayerImpl* root_layer_impl_; On 2016/03/22 15:04:33, ajuma wrote: > Rather than hanging on to a raw pointer here, can we change root_layer() to get > the root from the tree? Done. https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:289: static inline bool LayerShouldBeSkippedWithMaskAndReplica( On 2016/03/22 15:04:33, ajuma wrote: > This name is a bit awkward. How about calling this LayerShouldBeSkipped, and > renaming the existing LayerShouldBeSkipped to LayerNeedsUpdate (and inverting > the return value of that)? Done. https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:301: if (!(transform_node->data.is_invertible || transform_node->data.is_animated)) On 2016/03/22 15:04:33, ajuma wrote: > This needs to check data.ancestors_are_invertible and data.to_screen_is_animated > (since we're now skipping layer-by-layer rather than by subtree). Please add > tests for this too. Done. https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:317: effect_node->data.has_animated_opacity) On 2016/03/22 15:04:33, ajuma wrote: > This needs to check whether any ancestor node has animated opacity, that is, we > need to add an ancestor_has_opacity_animation (or > "to_screen_opacity_is_animated") to each effect node. Please also add a test. I added the test. But the problem is, if !effect_node->data.has_animated_opacity && effect_node->to_screen_opacity_is_animated, we would only return true if we have a 0 to_screen_opacity. But in that case, effect_node->data.is_drawn is always false as I observed, we would have already decided to skip the layer before this line is executed (line 310). https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:322: if (effect_node->data.ancestor_has_background_filters || On 2016/03/22 15:04:33, ajuma wrote: > If we define "ancestor_has_background_filters" to include the node itself, then > we only need to check that here. Done. https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:329: return !effect_node->data.opacity; On 2016/03/22 15:04:33, ajuma wrote: > This needs to check screen_space_opacity. And needs a test too :) I added the test, but it seems effect_node->data.is_drawn fully covers the situation where a parent node has 0 opacity and the node has non-0 opacity. https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:470: } On 2016/03/22 15:04:33, ajuma wrote: > The loop body has the same logic as the Layer version. Can we put this into a > shared templated function that gets called in both places? I think we can leave this until we have a Layer iterator, because the function names called are different, the layer version skip subtrees. https://codereview.chromium.org/1811423002/diff/60001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (left): https://codereview.chromium.org/1811423002/diff/60001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:566: DCHECK(!root_layer_ || active_page_scale == 1); On 2016/03/22 15:04:33, ajuma wrote: > Please don't remove this -- see the related discussion in Jayadev's CL: > https://codereview.chromium.org/1823833002/. Done.
https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:317: effect_node->data.has_animated_opacity) On 2016/03/23 22:26:27, sunxd wrote: > On 2016/03/22 15:04:33, ajuma wrote: > > This needs to check whether any ancestor node has animated opacity, that is, > we > > need to add an ancestor_has_opacity_animation (or > > "to_screen_opacity_is_animated") to each effect node. Please also add a test. > > I added the test. But the problem is, if !effect_node->data.has_animated_opacity > && effect_node->to_screen_opacity_is_animated, we would only return true if we > have a 0 to_screen_opacity. But in that case, effect_node->data.is_drawn is > always false as I observed, we would have already decided to skip the layer > before this line is executed (line 310). Consider the case A->B, A has animating opacity and opacity 0, in that case, for B, !has_animated_opacity = to_screen_opacity_is_animated = true, screen_space_opacity = 0, but still B's is_drawn = true.
https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... File cc/trees/draw_property_utils.cc (left): https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:318: IsSurfaceBackFaceVisible(layer, tree)) Why is this removed (the surface backface visibility check) ?
https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1811423002/diff/60001/cc/trees/draw_property_... cc/trees/draw_property_utils.cc:470: } On 2016/03/23 22:26:27, sunxd wrote: > On 2016/03/22 15:04:33, ajuma wrote: > > The loop body has the same logic as the Layer version. Can we put this into a > > shared templated function that gets called in both places? > > I think we can leave this until we have a Layer iterator, because the function > names called are different, the layer version skip subtrees. Ok. In that case, please also add in the comment about why we need to include masks but don't need to include replicas in the update list. https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:3594: ->data.ancestors_are_invertible); Hmm. I think what's going wrong here is that we're not computing draw properties for this layer (since we're now correctly skipping it because it's screen space transform is non-invertible), but since the subtree-skipping logic used to compute the render surface layer list hasn't been updated yet, the layer is still in the RSLL, and therefore *thinks* it has up-to-date draw properties (otherwise DrawTransform would be correctly computed on demand). I think this means we need make the layer_tree_host_common and draw_property_utils skipping logic match as a prerequisite for this CL (so that layers without up-to-date draw properties don't wind up in the RSLL). https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:5858: root->layer_tree_impl()->SetRootLayer(std::move(root)); This changes (renaming root_ptr and so on) seem correct, but not really necessary. https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:8940: singular.matrix().set(0, 1, 1); Is this needed? It looks like the matrix is non-invertible either way? https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:8959: child_ptr->SetScrollClipLayer(root_ptr->id()); This test doesn't seem to involve scrolling at all, so is this needed? https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_im... File cc/trees/layer_tree_impl.cc (left): https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:580: DCHECK(!root_layer_ || active_page_scale == 1); This shouldn't be removed. https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_im... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:627: page_scale_layer_id_ == -1); We shouldn't have non-zero page scale when the page scale id is -1. https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_im... File cc/trees/layer_tree_impl_unittest.cc (right): https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl_unittest.cc:2373: EXPECT_EQ(expected_end, output.end); I think this might be another case where we have a layer that's winding up in the RSLL but not getting draw properties computed. https://codereview.chromium.org/1811423002/diff/120001/cc/trees/occlusion_tra... File cc/trees/occlusion_tracker_unittest.cc (right): https://codereview.chromium.org/1811423002/diff/120001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:108: root_ = layer.get(); Are the changes in this file needed? In particular, rather than hanging on to a raw pointer to the root, can we continue to get the root from the LayerTreeImpl when we need it, as before? https://codereview.chromium.org/1811423002/diff/120001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1811423002/diff/120001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:629: data_for_children->effect_tree_parent); parent_id https://codereview.chromium.org/1811423002/diff/120001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:642: node.data.to_screen_opacity_is_animated = has_animated_opacity; Also set node_or_ancestor_has_background_filters here.
I made LayerShouldBeSkipped in draw_property_utils skip back face visible surfaces, but some of the test cases still need to modify the expected results. https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:5858: root->layer_tree_impl()->SetRootLayer(std::move(root)); On 2016/03/24 15:11:59, ajuma wrote: > This changes (renaming root_ptr and so on) seem correct, but not really > necessary. Ah, yes, it is a result of rebasing. Because in other tests we use root_layer, I think it might be better to use the same name here. https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:8940: singular.matrix().set(0, 1, 1); On 2016/03/24 15:11:59, ajuma wrote: > Is this needed? It looks like the matrix is non-invertible either way? The problem here is that a transform matrix 0 0 0 0 0 1 0 0 (0 0 1 0) can make ComputeVisibleRects get a gfx::Rect(0, 0) result for 0 0 0 1 grand_child's visible_layer_rect even though the layer isn't skipped. https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:8959: child_ptr->SetScrollClipLayer(root_ptr->id()); On 2016/03/24 15:11:59, ajuma wrote: > This test doesn't seem to involve scrolling at all, so is this needed? This is for creating a transform_node for child, otherwise we have only one node in the transform_tree. https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_im... File cc/trees/layer_tree_impl.cc (left): https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:580: DCHECK(!root_layer_ || active_page_scale == 1); On 2016/03/24 15:11:59, ajuma wrote: > This shouldn't be removed. Done. https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_im... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:627: page_scale_layer_id_ == -1); On 2016/03/24 15:11:59, ajuma wrote: > We shouldn't have non-zero page scale when the page scale id is -1. Done. https://codereview.chromium.org/1811423002/diff/120001/cc/trees/occlusion_tra... File cc/trees/occlusion_tracker_unittest.cc (right): https://codereview.chromium.org/1811423002/diff/120001/cc/trees/occlusion_tra... cc/trees/occlusion_tracker_unittest.cc:108: root_ = layer.get(); On 2016/03/24 15:12:00, ajuma wrote: > Are the changes in this file needed? In particular, rather than hanging on to a > raw pointer to the root, can we continue to get the root from the LayerTreeImpl > when we need it, as before? Done. https://codereview.chromium.org/1811423002/diff/120001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1811423002/diff/120001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:629: data_for_children->effect_tree_parent); On 2016/03/24 15:12:00, ajuma wrote: > parent_id Done. https://codereview.chromium.org/1811423002/diff/120001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:642: node.data.to_screen_opacity_is_animated = has_animated_opacity; On 2016/03/24 15:12:00, ajuma wrote: > Also set node_or_ancestor_has_background_filters here. Done.
https://codereview.chromium.org/1811423002/diff/180001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1811423002/diff/180001/cc/trees/property_tree... cc/trees/property_tree.cc:1228: if (node->owner_id == -1) { Are there any cases where the owner id will be -1 ?
https://codereview.chromium.org/1811423002/diff/180001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1811423002/diff/180001/cc/trees/property_tree... cc/trees/property_tree.cc:1234: node->data.node_or_ancestor_has_backface_visible_surface = true; I think this early-out is only valid if we're taking double-sidededness into account. That is, if we have a non-double-sided backfacing ancestor surface, then the subtree rooted at that ancestor is invisible. But if that backfacing ancestor is double-sided, then it doesn't cause its descendants to be invisible. https://codereview.chromium.org/1811423002/diff/180001/cc/trees/property_tree... cc/trees/property_tree.cc:1239: TransformNode* transform_node = transform_tree.Node(node->data.transform_id); Is this the right transform id? I think this is the parent node of the node corresponding to this surface.
https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1811423002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:8958: root_ptr->SetTransform(singular); Add comment https://codereview.chromium.org/1811423002/diff/180001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1811423002/diff/180001/cc/trees/property_tree... cc/trees/property_tree.cc:1228: if (node->owner_id == -1) { On 2016/03/29 15:48:43, jaydasika wrote: > Are there any cases where the owner id will be -1 ? The only case is the dummy tree root. Another option to check whether it's the root are its id or parent id. https://codereview.chromium.org/1811423002/diff/180001/cc/trees/property_tree... cc/trees/property_tree.cc:1234: node->data.node_or_ancestor_has_backface_visible_surface = true; On 2016/03/29 17:06:46, ajuma wrote: > I think this early-out is only valid if we're taking double-sidededness into > account. That is, if we have a non-double-sided backfacing ancestor surface, > then the subtree rooted at that ancestor is invisible. But if that backfacing > ancestor is double-sided, then it doesn't cause its descendants to be invisible. Done. https://codereview.chromium.org/1811423002/diff/180001/cc/trees/property_tree... cc/trees/property_tree.cc:1239: TransformNode* transform_node = transform_tree.Node(node->data.transform_id); On 2016/03/29 17:06:46, ajuma wrote: > Is this the right transform id? I think this is the parent node of the node > corresponding to this surface. I think it is the correct one. In AddEffectNodeIfNeeded, the transform_id for a render surface is transform_tree's next available id. (https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/property_...)
https://codereview.chromium.org/1811423002/diff/180001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1811423002/diff/180001/cc/trees/property_tree... cc/trees/property_tree.cc:1228: if (node->owner_id == -1) { On 2016/03/30 15:21:34, sunxd wrote: > On 2016/03/29 15:48:43, jaydasika wrote: > > Are there any cases where the owner id will be -1 ? > > The only case is the dummy tree root. Another option to check whether it's the > root are its id or parent id. How about just checking if parent_node is null? https://codereview.chromium.org/1811423002/diff/200001/cc/trees/draw_property... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1811423002/diff/200001/cc/trees/draw_property... cc/trees/draw_property_utils.cc:316: if (!layer->double_sided() && I think it's the surface's double-sidedness that matters here, not the layer's (and the surface's double-sidedness is accounted for in the value stored on the effect node), since the in the old subtree version of this we only considered the double-sidedness of layers that owned surfaces. Please add a test for this. https://codereview.chromium.org/1811423002/diff/200001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1811423002/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:3530: // CalcDrawProps skips a subtree when a layer's own transform is uninvertible This comment needs to be rephrased to say that a layer gets skipped when its screen space transform is uninvertible. https://codereview.chromium.org/1811423002/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:3599: gfx::Rect layer_bounds = gfx::Rect(); This change is because the layer is now skipped, so we're not computing its occlusion_in_content_space, right? If so please update the comment. https://codereview.chromium.org/1811423002/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:9880: root->layer_tree_impl()->property_trees()->needs_rebuild = true; This line shouldn't be needed (since property trees won't have been build before this point). https://codereview.chromium.org/1811423002/diff/200001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1811423002/diff/200001/cc/trees/property_tree... cc/trees/property_tree.cc:1235: if (parent_node && parent_node->data.double_sided && I think that double-sidedness needs to be taken into account below, not here. (That is, the bool that's being stored on each node is really "the node or some ancestor has a surface that isn't double sided but is backfacking".) Please add some PropertyTreeTests for this.
https://codereview.chromium.org/1811423002/diff/200001/cc/trees/draw_property... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1811423002/diff/200001/cc/trees/draw_property... cc/trees/draw_property_utils.cc:316: if (!layer->double_sided() && On 2016/03/30 17:10:11, ajuma wrote: > I think it's the surface's double-sidedness that matters here, not the layer's > (and the surface's double-sidedness is accounted for in the value stored on the > effect node), since the in the old subtree version of this we only considered > the double-sidedness of layers that owned surfaces. Please add a test for this. I think it would be fine, as property_tree takes the job to verify if an effect node has a render surface. And in fact, I think we can even move the double sided check into property tree updates. https://codereview.chromium.org/1811423002/diff/200001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1811423002/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:3530: // CalcDrawProps skips a subtree when a layer's own transform is uninvertible On 2016/03/30 17:10:11, ajuma wrote: > This comment needs to be rephrased to say that a layer gets skipped when its > screen space transform is uninvertible. Done. https://codereview.chromium.org/1811423002/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:3599: gfx::Rect layer_bounds = gfx::Rect(); On 2016/03/30 17:10:11, ajuma wrote: > This change is because the layer is now skipped, so we're not computing its > occlusion_in_content_space, right? If so please update the comment. Done. https://codereview.chromium.org/1811423002/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:9880: root->layer_tree_impl()->property_trees()->needs_rebuild = true; On 2016/03/30 17:10:11, ajuma wrote: > This line shouldn't be needed (since property trees won't have been build before > this point). Done. https://codereview.chromium.org/1811423002/diff/200001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1811423002/diff/200001/cc/trees/property_tree... cc/trees/property_tree.cc:1235: if (parent_node && parent_node->data.double_sided && On 2016/03/30 17:10:11, ajuma wrote: > I think that double-sidedness needs to be taken into account below, not here. > (That is, the bool that's being stored on each node is really "the node or some > ancestor has a surface that isn't double sided but is backfacking".) > > Please add some PropertyTreeTests for this. Done.
https://codereview.chromium.org/1811423002/diff/240001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1811423002/diff/240001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:9901: EXPECT_EQ(identity_matrix, Why was it not skipped before this CL ? In any case, can you change the transform to a large non-singular transform so that that large transform case remains tested ?
Just a few more things, but this is generally looking pretty good. https://codereview.chromium.org/1811423002/diff/240001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1811423002/diff/240001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:3597: // Since |grand_child| has an univertible draw transform, it is skipped so uninvertible (missing 'n') Also, replace "draw transform" with "screen space transform" https://codereview.chromium.org/1811423002/diff/240001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:9010: child_ptr->ClearRenderSurfaceLayerList(); Is this line needed? We don't normally call this directly from tests. https://codereview.chromium.org/1811423002/diff/240001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1811423002/diff/240001/cc/trees/property_tree... cc/trees/property_tree.cc:1232: if (parent_node && parent_node->data.hidden_by_backface_visibility) { If we reach here, parent_node must be non-null, so no need to check that again.
https://codereview.chromium.org/1811423002/diff/240001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1811423002/diff/240001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:3597: // Since |grand_child| has an univertible draw transform, it is skipped so On 2016/03/31 19:22:32, ajuma wrote: > uninvertible (missing 'n') > Also, replace "draw transform" with "screen space transform" Done. https://codereview.chromium.org/1811423002/diff/240001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:9010: child_ptr->ClearRenderSurfaceLayerList(); On 2016/03/31 19:22:33, ajuma wrote: > Is this line needed? We don't normally call this directly from tests. Hmm, the test can still pass without this line. I thought we should clear everything we add in each cycle so that the next tests can succeed. https://codereview.chromium.org/1811423002/diff/240001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:9901: EXPECT_EQ(identity_matrix, On 2016/03/31 18:32:56, jaydasika wrote: > Why was it not skipped before this CL ? In any case, can you change the > transform to a large non-singular transform so that that large transform case > remains tested ? None of the transforms of the layer and its ancestors are singular, but because of float math, their combination is singular. Thus logically, the layer should be skipped. I will check that to make it test correctly. https://codereview.chromium.org/1811423002/diff/240001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1811423002/diff/240001/cc/trees/property_tree... cc/trees/property_tree.cc:1232: if (parent_node && parent_node->data.hidden_by_backface_visibility) { On 2016/03/31 19:22:33, ajuma wrote: > If we reach here, parent_node must be non-null, so no need to check that again. Done.
Thanks, lgtm % jaydasika
lgtm https://codereview.chromium.org/1811423002/diff/240001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1811423002/diff/240001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:9901: EXPECT_EQ(identity_matrix, On 2016/04/01 17:57:45, sunxd wrote: > On 2016/03/31 18:32:56, jaydasika wrote: > > Why was it not skipped before this CL ? In any case, can you change the > > transform to a large non-singular transform so that that large transform case > > remains tested ? > > None of the transforms of the layer and its ancestors are singular, but because > of float math, their combination is singular. Thus logically, the layer should > be skipped. > > I will check that to make it test correctly. That's weird because only one layer has non-identity transform. But, this test is unrelated to this CL, so feel free to land this and sort it out in a follow up or leave a TODO for me and I will sort it out later.
The CQ bit was checked by sunxd@chromium.org
The CQ bit was unchecked by sunxd@chromium.org
On 2016/04/01 19:10:11, jaydasika wrote: > lgtm > > https://codereview.chromium.org/1811423002/diff/240001/cc/trees/layer_tree_ho... > File cc/trees/layer_tree_host_common_unittest.cc (right): > > https://codereview.chromium.org/1811423002/diff/240001/cc/trees/layer_tree_ho... > cc/trees/layer_tree_host_common_unittest.cc:9901: EXPECT_EQ(identity_matrix, > On 2016/04/01 17:57:45, sunxd wrote: > > On 2016/03/31 18:32:56, jaydasika wrote: > > > Why was it not skipped before this CL ? In any case, can you change the > > > transform to a large non-singular transform so that that large transform > case > > > remains tested ? > > > > None of the transforms of the layer and its ancestors are singular, but > because > > of float math, their combination is singular. Thus logically, the layer should > > be skipped. > > > > I will check that to make it test correctly. > That's weird because only one layer has non-identity transform. But, this test > is unrelated to this CL, so feel free to land this and sort it out in a follow > up or leave a TODO for me and I will sort it out later. After a discussion with Ali, it seems that when contributing sublayer scale, we get NaN when doing the large scale. I removed the render surface from child (which used to be render_surface2), now it can test large transform. We probably need more investigation on that.
The CQ bit was checked by sunxd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jaydasika@chromium.org, ajuma@chromium.org Link to the patchset: https://codereview.chromium.org/1811423002/#ps280001 (title: "Add TODO for large transforms.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811423002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811423002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sunxd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jaydasika@chromium.org, ajuma@chromium.org Link to the patchset: https://codereview.chromium.org/1811423002/#ps300001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811423002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811423002/300001
Message was sent while issue was closed.
Description was changed from ========== SubtreeShouldBeSkipped uses information from property trees, and is renamed to LayerShouldBeSkippedWithMaskAndReplica, as it is now skipping individual layers instead of subtrees. The LayerImpl version FindLayersThatNeedUpdates uses LayerList iterator and the new version of LayerShouldBeSkippedWithMaskAndReplica. Fix the unit tests. BUG=592440 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== SubtreeShouldBeSkipped uses information from property trees, and is renamed to LayerShouldBeSkippedWithMaskAndReplica, as it is now skipping individual layers instead of subtrees. The LayerImpl version FindLayersThatNeedUpdates uses LayerList iterator and the new version of LayerShouldBeSkippedWithMaskAndReplica. Fix the unit tests. BUG=592440 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== SubtreeShouldBeSkipped uses information from property trees, and is renamed to LayerShouldBeSkippedWithMaskAndReplica, as it is now skipping individual layers instead of subtrees. The LayerImpl version FindLayersThatNeedUpdates uses LayerList iterator and the new version of LayerShouldBeSkippedWithMaskAndReplica. Fix the unit tests. BUG=592440 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== SubtreeShouldBeSkipped uses information from property trees, and is renamed to LayerShouldBeSkippedWithMaskAndReplica, as it is now skipping individual layers instead of subtrees. The LayerImpl version FindLayersThatNeedUpdates uses LayerList iterator and the new version of LayerShouldBeSkippedWithMaskAndReplica. Fix the unit tests. BUG=592440 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/71aea3ee6feb1ca9ca8b4b500a1d95f29d7ebbc6 Cr-Commit-Position: refs/heads/master@{#384720} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/71aea3ee6feb1ca9ca8b4b500a1d95f29d7ebbc6 Cr-Commit-Position: refs/heads/master@{#384720} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
