cc: Main thread skip single layers instead of subtrees

cc/trees/draw_property_utils.cc

Index: cc/trees/draw_property_utils.cc
diff --git a/cc/trees/draw_property_utils.cc b/cc/trees/draw_property_utils.cc
index 605bca37d9eb7a22c1675cf580a95d62e39dbb62..babe43b28f9680e4a727fb7ab75b95beacce4d52 100644
--- a/cc/trees/draw_property_utils.cc
+++ b/cc/trees/draw_property_utils.cc
@@ -314,44 +314,6 @@ static bool HasInvertibleOrAnimatedTransform(LayerType* layer) {
          layer->HasPotentiallyRunningTransformAnimation();
 }
 
-static inline bool SubtreeShouldBeSkipped(Layer* layer,
-                                          bool layer_is_drawn,
-                                          const TransformTree& tree) {
-  // If the layer transform is not invertible, it should not be drawn.
-  if (!layer->transform_is_invertible() &&
-      !layer->HasPotentiallyRunningTransformAnimation())
-    return true;
-
-  // When we need to do a readback/copy of a layer's output, we can not skip
-  // it or any of its ancestors.
-  if (layer->num_copy_requests_in_target_subtree() > 0)
-    return false;
-
-  // If the layer is not drawn, then skip it and its subtree.
-  if (!layer_is_drawn)
-    return true;
-
-  if (layer->has_render_surface() && !layer->double_sided() &&
-      !layer->HasPotentiallyRunningTransformAnimation() &&
-      IsSurfaceBackFaceVisible(layer, tree))
-    return true;
-
-  // If layer has a background filter, don't skip the layer, even it the
-  // opacity is 0.
-  if (!layer->background_filters().IsEmpty())
-    return false;
-
-  // If the opacity is being animated then the opacity on the main thread is
-  // unreliable (since the impl thread may be using a different opacity), so it
-  // should not be trusted.
-  // In particular, it should not cause the subtree to be skipped.
-  // Similarly, for layers that might animate opacity using an impl-only
-  // animation, their subtree should also not be skipped.
-  return !layer->EffectiveOpacity() &&
-         !layer->HasPotentiallyRunningOpacityAnimation() &&
-         !layer->OpacityCanAnimateOnImplThread();
-}
-
 template <typename LayerType>
 static bool LayerNeedsUpdateInternal(LayerType* layer,
                                      bool layer_is_drawn,
@@ -449,6 +411,135 @@ void UpdateRenderSurfacesForLayersRecursive(EffectTree* effect_tree,
 
 }  // namespace
 
+static inline bool LayerShouldBeSkipped(Layer* layer,
+                                        bool layer_is_drawn,
+                                        const TransformTree& transform_tree) {
+  const TransformNode* transform_node =
+      transform_tree.Node(layer->transform_tree_index());
+  const EffectTree& effect_tree = transform_tree.property_trees()->effect_tree;

[Ian Vollick, 2016/04/06 19:42:26]

nit: I'd just pass this in since you have it handy in the outer scope.

[sunxd, 2016/04/07 15:37:13]

Done.

+  const EffectNode* effect_node = effect_tree.Node(layer->effect_tree_index());
+
+  // If the layer transform is not invertible, it should not be drawn.
+  bool has_inherited_invertible_or_animated_transform =
+      (transform_node->data.is_invertible &&
+       transform_node->data.ancestors_are_invertible) ||
+      transform_node->data.to_screen_is_animated;

[Ian Vollick, 2016/04/06 19:42:26]

This isn't worth doing anything about in this CL, but I wonder if is_animated is
misleading. Looking at the property tree builder, I see that it does get set to
true if we have a potentially running animation, but it's not obvious to me that
to_screen_is_animated is true for potential animations, too.

to_screen_is_potentially_animated is quite a mouthful, though. wdyt?

[sunxd, 2016/04/07 15:37:13]

I think so. When updating animation properties, to_screen_is_animated is set to
be node->data.is_animated || parent_node->Data.to_screen_is_animated. So I guess
we can change that to "potentially animated". And probably we can change
is_animated to clarify the meaning in the future.

+  if (!has_inherited_invertible_or_animated_transform)
+    return true;
+
+  // When we need to do a readback/copy of a layer's output, we can not skip
+  // it or any of its ancestors.
+  if (effect_node->data.num_copy_requests_in_subtree > 0)
+    return false;
+
+  // If the layer is not drawn, then skip it and its subtree.
+  if (!effect_node->data.is_drawn)
+    return true;
+
+  if (!transform_node->data.to_screen_is_animated &&
+      effect_node->data.hidden_by_backface_visibility)
+    return true;
+
+  // If layer has a background filter, don't skip the layer, even it the
+  // opacity is 0.
+  if (effect_node->data.node_or_ancestor_has_background_filters)
+    return false;
+
+  // If the opacity is being animated then the opacity on the main thread is
+  // unreliable (since the impl thread may be using a different opacity), so it
+  // should not be trusted.
+  // In particular, it should not cause the subtree to be skipped.
+  // Similarly, for layers that might animate opacity using an impl-only
+  // animation, their subtree should also not be skipped.
+  return !effect_node->data.screen_space_opacity &&
+         !effect_node->data.to_screen_opacity_is_animated &&
+         !effect_node->data.to_screen_opacity_can_animate_on_impl;

[sunxd, 2016/04/06 18:48:48]

I'm also considering merging the logic of main/impl, this is one of the
differences.

[Ian Vollick, 2016/04/06 19:42:26]

It would be fantastic if you could do this. This seems like a difficult
spot-the-differences game at the moment.

[sunxd, 2016/04/07 15:37:12]

Yes, jaydasika@ is going to work on simplifying the skipping logic, I think
after his change it would be easier to merge the two paths together.

[Ian Vollick, 2016/04/07 17:20:25]

kk. makes sense.

+}
+
+bool LayerShouldBeSkipped(LayerImpl* layer,
+                          bool layer_is_drawn,
+                          const TransformTree& transform_tree) {
+  const TransformNode* transform_node =
+      transform_tree.Node(layer->transform_tree_index());
+  const EffectTree& effect_tree = transform_tree.property_trees()->effect_tree;
+  const EffectNode* effect_node = effect_tree.Node(layer->effect_tree_index());
+  // If the layer transform is not invertible, it should not be drawn.
+  // TODO(ajuma): Correctly process subtrees with singular transform for the
+  // case where we may animate to a non-singular transform and wish to
+  // pre-raster.
+  bool has_inherited_invertible_or_animated_transform =
+      (transform_node->data.is_invertible &&
+       transform_node->data.ancestors_are_invertible) ||
+      transform_node->data.to_screen_is_animated;
+  if (!has_inherited_invertible_or_animated_transform)
+    return true;
+
+  // When we need to do a readback/copy of a layer's output, we can not skip
+  // it or any of its ancestors.
+  if (effect_node->data.num_copy_requests_in_subtree > 0)
+    return false;
+
+  // If the layer is not drawn, then skip it and its subtree.
+  if (!effect_node->data.is_drawn)
+    return true;
+
+  if (effect_node->data.hidden_by_backface_visibility)
+    return true;
+
+  // If layer is on the pending tree and opacity is being animated then
+  // this subtree can't be skipped as we need to create, prioritize and
+  // include tiles for this layer when deciding if tree can be activated.
+  if (!transform_tree.property_trees()->is_active &&
+      effect_node->data.to_screen_opacity_is_animated)
+    return false;
+
+  // If layer has a background filter, don't skip the layer, even it the
+  // opacity is 0.
+  if (effect_node->data.node_or_ancestor_has_background_filters)
+    return false;
+
+  // The opacity of a layer always applies to its children (either implicitly
+  // via a render surface or explicitly if the parent preserves 3D), so the
+  // entire subtree can be skipped if this layer is fully transparent.
+  return !effect_node->data.screen_space_opacity;
+}
+
+void FindLayersThatNeedUpdates(Layer* root_layer,
+                               const TransformTree& transform_tree,
+                               const EffectTree& effect_tree,
+                               LayerList* update_layer_list) {
+  DCHECK_GE(root_layer->effect_tree_index(), 0);
+  LayerTreeHostCommon::CallFunctionForEveryLayer(
+      root_layer->layer_tree_host(),

[sunxd, 2016/04/06 18:48:48]

I'm also considering merging the logic of main/impl, this is one of the
differences.

+      [&](Layer* layer) {
+        bool layer_is_drawn =
+            effect_tree.Node(layer->effect_tree_index())->data.is_drawn;
+
+        if (!IsRootLayer(layer) &&
+            LayerShouldBeSkipped(layer, layer_is_drawn, transform_tree))
+          return;
+
+        if (LayerNeedsUpdate(layer, layer_is_drawn, transform_tree)) {
+          update_layer_list->push_back(layer);
+        }
+
+        // Append mask layers to the update layer list.  They don't have valid
+        // visible
+        // rects, so need to get added after the above calculation.  Replica
+        // layers
+        // don't need to be updated.

[Ian Vollick, 2016/04/06 19:42:26]

nit: wrapping issues.

[sunxd, 2016/04/07 15:37:13]

Done.

[ajuma, 2016/04/07 18:46:47]

The wrapping of the comment still needs fixing.

+        if (Layer* mask_layer = layer->mask_layer()) {
+          update_layer_list->push_back(mask_layer);
+        }
+        if (Layer* replica_layer = layer->replica_layer()) {
+          if (Layer* mask_layer = replica_layer->mask_layer()) {
+            update_layer_list->push_back(mask_layer);
+          }
+        }
+      },
+      CallFunctionLayerType::BASIC_LAYER);
+}
+
 static void ResetIfHasNanCoordinate(gfx::RectF* rect) {
   if (std::isnan(rect->x()) || std::isnan(rect->y()) ||
       std::isnan(rect->right()) || std::isnan(rect->bottom()))
@@ -658,33 +749,6 @@ void UpdatePropertyTrees(PropertyTrees* property_trees,
   ComputeEffects(&property_trees->effect_tree);
 }
 
-void FindLayersThatNeedUpdates(Layer* layer,
-                               const TransformTree& transform_tree,
-                               const EffectTree& effect_tree,
-                               LayerList* update_layer_list) {
-  DCHECK_GE(layer->effect_tree_index(), 0);
-  bool layer_is_drawn =
-      effect_tree.Node(layer->effect_tree_index())->data.is_drawn;
-
-  if (!IsRootLayer(layer) &&
-      SubtreeShouldBeSkipped(layer, layer_is_drawn, transform_tree))
-    return;
-
-  if (LayerNeedsUpdate(layer, layer_is_drawn, transform_tree))
-    update_layer_list->push_back(layer);
-  if (Layer* mask_layer = layer->mask_layer())
-    update_layer_list->push_back(mask_layer);
-  if (Layer* replica_layer = layer->replica_layer()) {
-    if (Layer* mask_layer = replica_layer->mask_layer())
-      update_layer_list->push_back(mask_layer);
-  }
-
-  for (size_t i = 0; i < layer->children().size(); ++i) {
-    FindLayersThatNeedUpdates(layer->child_at(i), transform_tree, effect_tree,
-                              update_layer_list);
-  }
-}
-
 void ComputeVisibleRectsForTesting(PropertyTrees* property_trees,
                                    bool can_render_to_separate_surface,
                                    LayerList* update_layer_list) {
@@ -734,54 +798,6 @@ void ComputeVisibleRects(LayerImpl* root_layer,
                               &update_layer_list, visible_layer_list);
 }
 
-bool LayerShouldBeSkipped(LayerImpl* layer,
-                          bool layer_is_drawn,
-                          const TransformTree& transform_tree) {
-  const TransformNode* transform_node =
-      transform_tree.Node(layer->transform_tree_index());
-  const EffectTree& effect_tree = transform_tree.property_trees()->effect_tree;
-  const EffectNode* effect_node = effect_tree.Node(layer->effect_tree_index());
-  // If the layer transform is not invertible, it should not be drawn.
-  // TODO(ajuma): Correctly process subtrees with singular transform for the
-  // case where we may animate to a non-singular transform and wish to
-  // pre-raster.
-  bool has_inherited_invertible_or_animated_transform =
-      (transform_node->data.is_invertible &&
-       transform_node->data.ancestors_are_invertible) ||
-      transform_node->data.to_screen_is_animated;
-  if (!has_inherited_invertible_or_animated_transform)
-    return true;
-
-  // When we need to do a readback/copy of a layer's output, we can not skip
-  // it or any of its ancestors.
-  if (effect_node->data.num_copy_requests_in_subtree > 0)
-    return false;
-
-  // If the layer is not drawn, then skip it and its subtree.
-  if (!effect_node->data.is_drawn)
-    return true;
-
-  if (effect_node->data.hidden_by_backface_visibility)
-    return true;
-
-  // If layer is on the pending tree and opacity is being animated then
-  // this subtree can't be skipped as we need to create, prioritize and
-  // include tiles for this layer when deciding if tree can be activated.
-  if (!transform_tree.property_trees()->is_active &&
-      effect_node->data.to_screen_opacity_is_animated)
-    return false;
-
-  // If layer has a background filter, don't skip the layer, even it the
-  // opacity is 0.
-  if (effect_node->data.node_or_ancestor_has_background_filters)
-    return false;
-
-  // The opacity of a layer always applies to its children (either implicitly
-  // via a render surface or explicitly if the parent preserves 3D), so the
-  // entire subtree can be skipped if this layer is fully transparent.
-  return !effect_node->data.screen_space_opacity;
-}
-
 bool LayerNeedsUpdate(Layer* layer,
                       bool layer_is_drawn,
                       const TransformTree& tree) {