cc: Compute target space transform dynamically

cc/trees/property_tree.cc

Index: cc/trees/property_tree.cc
diff --git a/cc/trees/property_tree.cc b/cc/trees/property_tree.cc
index e985d75f44b180278e31c532841748655da556fc..d1dd7b7c4f899c2f8eb7ba6f9e800bd9b40a7a16 100644
--- a/cc/trees/property_tree.cc
+++ b/cc/trees/property_tree.cc
@@ -265,9 +265,9 @@ bool TransformTree::CombineTransformsBetween(int source_id,
 
   gfx::Transform combined_transform;
   if (current->id > dest_id) {
-    combined_transform = ToTarget(current->id);
-    // The stored target space transform has surface contents scale baked in,
-    // but we need the unscaled transform.
+    combined_transform = ToTarget(current->id, kInvalidNodeId);
+    // The stored target space transform has sublayer scale baked in, but we

[jaydasika, 2016/07/20 23:34:57]

sublayer scale got renamed to surface contents scale. So, leave it as 'surface
contents scale' in the comment.

[sunxd, 2016/07/21 21:50:08]

Done.

+    // need the unscaled transform.
     combined_transform.matrix().postScale(
         1.0f / dest->surface_contents_scale.x(),
         1.0f / dest->surface_contents_scale.y(), 1.0f);
@@ -578,8 +578,15 @@ bool TransformTree::HasNodesAffectedByOuterViewportBoundsDelta() const {
   return !nodes_affected_by_outer_viewport_bounds_delta_.empty();
 }
 
-const gfx::Transform& TransformTree::FromTarget(int node_id) const {
+const gfx::Transform& TransformTree::FromTarget(int node_id,
+                                                int effect_id) const {
   DCHECK(static_cast<int>(cached_data_.size()) > node_id);
+  if (effect_id != kInvalidNodeId &&
+      property_trees()->verify_transform_tree_calculations) {
+    const gfx::Transform& transform =
+        property_trees()->GetDrawTransforms(node_id, effect_id).from_target;
+    CHECK(transform.ApproximatelyEqual(cached_data_[node_id].from_target));
+  }
   return cached_data_[node_id].from_target;
 }
 
@@ -589,8 +596,15 @@ void TransformTree::SetFromTarget(int node_id,
   cached_data_[node_id].from_target = transform;
 }
 
-const gfx::Transform& TransformTree::ToTarget(int node_id) const {
+const gfx::Transform& TransformTree::ToTarget(int node_id,
+                                              int effect_id) const {
   DCHECK(static_cast<int>(cached_data_.size()) > node_id);
+  if (effect_id != kInvalidNodeId &&
+      property_trees()->verify_transform_tree_calculations) {
+    const gfx::Transform& transform =
+        property_trees()->GetDrawTransforms(node_id, effect_id).to_target;
+    CHECK(transform.ApproximatelyEqual(cached_data_[node_id].to_target));
+  }
   return cached_data_[node_id].to_target;
 }
 
@@ -1480,7 +1494,8 @@ PropertyTrees::PropertyTrees()
       full_tree_damaged(false),
       sequence_number(0),
       is_main_thread(true),
-      is_active(false) {
+      is_active(false),
+      verify_transform_tree_calculations(false) {
   transform_tree.SetPropertyTrees(this);
   effect_tree.SetPropertyTrees(this);
   clip_tree.SetPropertyTrees(this);
@@ -1525,6 +1540,7 @@ PropertyTrees& PropertyTrees::operator=(const PropertyTrees& from) {
   sequence_number = from.sequence_number;
   is_main_thread = from.is_main_thread;
   is_active = from.is_active;
+  verify_transform_tree_calculations = from.verify_transform_tree_calculations;
   inner_viewport_container_bounds_delta_ =
       from.inner_viewport_container_bounds_delta();
   outer_viewport_container_bounds_delta_ =
@@ -1552,6 +1568,8 @@ void PropertyTrees::ToProtobuf(proto::PropertyTrees* proto) const {
   proto->set_non_root_surfaces_enabled(non_root_surfaces_enabled);
   proto->set_is_main_thread(is_main_thread);
   proto->set_is_active(is_active);
+  proto->set_verify_transform_tree_calculations(
+      verify_transform_tree_calculations);
 
   // TODO(khushalsagar): Consider using the sequence number to decide if
   // property trees need to be serialized again for a commit. See crbug/555370.
@@ -1576,6 +1594,8 @@ void PropertyTrees::FromProtobuf(const proto::PropertyTrees& proto) {
   sequence_number = proto.sequence_number();
   is_main_thread = proto.is_main_thread();
   is_active = proto.is_active();
+  verify_transform_tree_calculations =
+      proto.verify_transform_tree_calculations();
 
   transform_tree.SetPropertyTrees(this);
   effect_tree.SetPropertyTrees(this);
@@ -1767,7 +1787,8 @@ CombinedAnimationScale PropertyTrees::GetAnimationScales(
     // Computing maximum animated scale in the presence of non-scale/translation
     // transforms isn't supported.
     bool failed_for_non_scale_or_translation =
-        !transform_tree.ToTarget(transform_node_id).IsScaleOrTranslation();
+        !transform_tree.ToTarget(transform_node_id, effect_tree.kInvalidNodeId)
+             .IsScaleOrTranslation();
 
     // We don't attempt to accumulate animation scale from multiple nodes with
     // scale animations, because of the risk of significant overestimation. For
@@ -1834,7 +1855,9 @@ CombinedAnimationScale PropertyTrees::GetAnimationScales(
       } else {
         gfx::Vector2dF ancestor_scales =
             parent_node ? MathUtil::ComputeTransform2dScaleComponents(
-                              transform_tree.ToTarget(parent_node->id), 0.f)
+                              transform_tree.ToTarget(
+                                  parent_node->id, effect_tree.kInvalidNodeId),
+                              0.f)
                         : gfx::Vector2dF(1.f, 1.f);
         float max_ancestor_scale =
             std::max(ancestor_scales.x(), ancestor_scales.y());
@@ -1871,10 +1894,69 @@ void PropertyTrees::SetAnimationScalesForTesting(
       cached_data_.property_tree_update_number;
 }
 
+const DrawTransforms& PropertyTrees::GetDrawTransforms(int transform_id,
+                                                       int effect_id) const {
+  if (cached_data_.draw_transforms[effect_id][transform_id].update_number !=
+      cached_data_.property_tree_update_number) {
+    // update
+    gfx::Transform target_space_transform;
+    gfx::Transform from_target;
+    const TransformNode* transform_node = transform_tree.Node(transform_id);
+    const EffectNode* effect_node = effect_tree.Node(effect_id);

[jaydasika, 2016/07/20 23:34:57]

Can you add a DCHECK(effect_node->has_render_surface) here ?

[sunxd, 2016/07/21 18:43:07]

Done.

+    if (transform_node->needs_surface_contents_scale) {
+      target_space_transform.MakeIdentity();
+      target_space_transform.Scale(transform_node->surface_contents_scale.x(),
+                                   transform_node->surface_contents_scale.y());

[jaydasika, 2016/07/20 23:34:57]

Can you use the surface_contents_scale from effect node? This condition is true
when the transform node is created for the surface. So you could check
effect_node->transform_id == transform_id instead of
needs_surface_contents_scale and then use the scale in effect node. (You may
need to handle root separately)

[sunxd, 2016/07/21 18:43:08]

I think in some situations we want to take surface contents scale of the source
node into consideration, even if src != dst. So I think we still need to judge
it using the needs_surface_contents_scale here and keep an if-branch for the
case where src == dst. This is the case in some unit tests like
LayerTreeHostCommonTest.ClipRectWhenCannotRenderToSeparateSurface, but I'm not
sure if needing contents scale implies src == dst in the real case.

Also I'm uncertain if we can use the dest node / effect node's surface contents
scale instead of the one of the src node. Unit test
PropertyTreeTestTransformsWithFlattening would fail if I change it.

[jaydasika, 2016/07/21 19:32:12]

Its confusing that we want to use src surface content scale for target space
transform when src != dst and dst never gets used.(We should probably call
target space transform using content target id instead of target id)  
Having said that, I think this CL is trying to do what we already do using an
effect node instead of transform node. So, as far as this CL goes, I am fine,
but that confusion needs to be sorted out.
 

[sunxd, 2016/07/21 21:50:08]

I sorted out that the problem is when we disable rendering to separate surfaces,
and now it's not influencing the check here.

+    } else {
+      // Compute transform from transform_id to effect_node->transform
+      DCHECK(transform_id >= effect_node->transform_id);

[jaydasika, 2016/07/20 23:34:57]

nit : Use DCHECK_GE. Or actually, this could be DCHECK_GT

[sunxd, 2016/07/21 18:43:08]

Done.

[sunxd, 2016/07/21 18:43:08]

Done.

+      const TransformNode* dest_node =
+          transform_tree.Node(effect_node->transform_id);
+      if (!dest_node || (dest_node->ancestors_are_invertible &&
+                         dest_node->node_and_ancestors_are_flat)) {
+        target_space_transform.ConcatTransform(
+            transform_tree.ToScreen(transform_id));
+        if (dest_node)
+          target_space_transform.ConcatTransform(
+              transform_tree.FromScreen(dest_node->id));
+        if (dest_node->needs_surface_contents_scale)
+          target_space_transform.matrix().postScale(
+              dest_node->surface_contents_scale.x(),
+              dest_node->surface_contents_scale.y(), 1.f);
+      } else if (transform_id == effect_node->transform_id) {
+        // sublayer scale

[jaydasika, 2016/07/20 23:34:57]

I don't see how this can be true (or maybe true only for root node). If
transform_id == effect_node->transform_id, then needs_surface_contents_scale
will be true and we won't enter the else (in line 1910)

[sunxd, 2016/07/21 18:43:08]

See the above comment.

[jaydasika, 2016/07/21 19:32:12]

nit : surface contents scale

[sunxd, 2016/07/21 21:50:08]

Done.

+        if (dest_node->needs_surface_contents_scale)
+          target_space_transform.matrix().postScale(
+              dest_node->surface_contents_scale.x(),
+              dest_node->surface_contents_scale.y(), 1.f);
+      } else {
+        target_space_transform =
+            GetDrawTransforms(transform_node->parent_id, effect_id).to_target;
+        if (transform_node->flattens_inherited_transform)
+          target_space_transform.FlattenTo2d();
+        target_space_transform.PreconcatTransform(transform_node->to_parent);
+      }
+    }
+    if (!target_space_transform.GetInverse(&from_target))
+      transform_node->ancestors_are_invertible = false;
+    cached_data_.draw_transforms[effect_id][transform_id].update_number =
+        cached_data_.property_tree_update_number;
+    cached_data_.draw_transforms[effect_id][transform_id]
+        .transforms.from_target = from_target;
+    cached_data_.draw_transforms[effect_id][transform_id].transforms.to_target =
+        target_space_transform;
+  }
+  return cached_data_.draw_transforms[effect_id][transform_id].transforms;
+}
+
 void PropertyTrees::ResetCachedData() {
   cached_data_.property_tree_update_number = 0;
   cached_data_.animation_scales = std::vector<AnimationScaleData>(
       transform_tree.nodes().size(), AnimationScaleData());
+  cached_data_.draw_transforms =
+      std::vector<std::unordered_map<int, DrawTransformData>>(
+          effect_tree.nodes().size(),
+          std::unordered_map<int, DrawTransformData>());
 }
 
 void PropertyTrees::UpdateCachedNumber() {