Tighten the matrix usage in cc/layer_tree_host_common.cpp

cc/layer_tree_host_common.cc

Index: cc/layer_tree_host_common.cc
diff --git a/cc/layer_tree_host_common.cc b/cc/layer_tree_host_common.cc
index 8aff2029e27fd339917cf9b5afdec7beac69b28e..5979d078cb9e0b3f2de21aeac67b77f2528bfbbc 100644
--- a/cc/layer_tree_host_common.cc
+++ b/cc/layer_tree_host_common.cc
@@ -507,25 +507,18 @@ static void calculateDrawTransformsInternal(LayerType* layer, const gfx::Transfo
     gfx::PointF anchorPoint = layer->anchorPoint();
     gfx::PointF position = layer->position() - layer->scrollDelta();
 
-    gfx::Transform layerLocalTransform;
+    gfx::Transform combinedTransform = layer->implTransform() * parentMatrix;
     // LT = Tr[origin] * Tr[origin2anchor]
-    layerLocalTransform.Translate3d(position.x() + anchorPoint.x() * bounds.width(), position.y() + anchorPoint.y() * bounds.height(), layer->anchorPointZ());
+    combinedTransform.Translate3d(position.x() + anchorPoint.x() * bounds.width(), position.y() + anchorPoint.y() * bounds.height(), layer->anchorPointZ());
     // LT = Tr[origin] * Tr[origin2anchor] * M[layer]
-    layerLocalTransform.PreconcatTransform(layer->transform());
+    combinedTransform.PreconcatTransform(layer->transform());
     // LT = Tr[origin] * Tr[origin2anchor] * M[layer] * Tr[anchor2origin]
-    layerLocalTransform.Translate3d(-anchorPoint.x() * bounds.width(), -anchorPoint.y() * bounds.height(), -layer->anchorPointZ());
-
-    gfx::Transform combinedTransform = parentMatrix;
-    combinedTransform.PreconcatTransform(layerLocalTransform);
+    combinedTransform.Translate3d(-anchorPoint.x() * bounds.width(), -anchorPoint.y() * bounds.height(), -layer->anchorPointZ());
 
     // The layer's contentsSize is determined from the combinedTransform, which then informs the
     // layer's drawTransform.
     updateLayerContentsScale(layer, combinedTransform, deviceScaleFactor, pageScaleFactor, animatingTransformToScreen);
 
-    // If there is a tranformation from the impl thread then it should be at the
-    // start of the combinedTransform, but we don't want it to affect the contentsScale.

[danakj, 2012/12/03 18:24:40]

implTransform should not affect the contentsScale. That's why it's below
updateLayerContentsScale, and what this comment was saying.

[shawnsingh, 2012/12/03 19:37:06]

Yes, I was planning to ask you about this before landing the patch.   All I did
was provable math substitutions.  I'm pretty sure I didn't change what would be
computed here.  In particular, ignoring the comment for a moment, this code
tacks implTransform to the left side of the composite sequence of matrix
multiplications, unconditionally.   So it is equivalent to move it up to the
initialization of combinedTransform (by matrix associativity), as long as the
sequence of multiplications itself doesn't change order.

So, I feel that the comment + original code were not quite adding up clearly...
one of them may need to be fixed.  As far as I could reason, multiplying
layer->implTransform() on the left side does correctly _not_ affect
contentsScale.  So either the left-side multiplication here is not quite in the
correct place (but avoids content scale), or the comment itself should be
updated and I can re-instate the comment in the new code...

[danakj, 2012/12/03 19:56:28]

updateLayerContentsScale wants the combinedTransform without the implTransform
included.

You're right the combinedTransform ends up being the same here. But the
transform passed to updateLayerContentsScale is not the same.

-    combinedTransform = layer->implTransform() * combinedTransform;
-
     if (layer->fixedToContainerLayer()) {
         // Special case: this layer is a composited fixed-position layer; we need to
         // explicitly compensate for all ancestors' nonzero scrollDeltas to keep this layer
@@ -566,22 +559,23 @@ static void calculateDrawTransformsInternal(LayerType* layer, const gfx::Transfo
         RenderSurfaceType* renderSurface = layer->renderSurface();
         renderSurface->clearLayerLists();
 
-        // The owning layer's draw transform has a scale from content to layer space which we need to undo and
-        // replace with a scale from the surface's subtree into layer space.
-        drawTransform.Scale(layer->contentsScaleX(), layer->contentsScaleY());
-        drawTransform.Scale(1 / renderSurfaceSublayerScale.x(), 1 / renderSurfaceSublayerScale.y());
-        renderSurface->setDrawTransform(drawTransform);
+        // The owning layer's draw transform has a scale from content to layer
+        // space which do not want; so here we use the combinedTransform
+        // instead of the drawTransform. However, we do need to add a different
+        // scale factor that accounts for the surface's pixel dimensions.
+        combinedTransform.Scale(1 / renderSurfaceSublayerScale.x(), 1 / renderSurfaceSublayerScale.y());
+        renderSurface->setDrawTransform(combinedTransform);
 
-        // The origin of the new surface is the upper left corner of the layer.
+        // The owning layer's transform was re-parented by the surface, so the layer's new drawTransform
+        // only needs to scale the layer to surface space.
         gfx::Transform layerDrawTransform;
-        layerDrawTransform.Scale(renderSurfaceSublayerScale.x(), renderSurfaceSublayerScale.y());
-        layerDrawTransform.Scale(1.0 / layer->contentsScaleX(), 1.0 / layer->contentsScaleY());
+        layerDrawTransform.Scale(renderSurfaceSublayerScale.x() / layer->contentsScaleX(), renderSurfaceSublayerScale.y() / layer->contentsScaleY());
         layer->setDrawTransform(layerDrawTransform);
 
         // Inside the surface's subtree, we scale everything to the owning layer's scale.
         // The sublayer matrix transforms centered layer rects into target
         // surface content space.
-        sublayerMatrix.MakeIdentity();
+        DCHECK(sublayerMatrix.IsIdentity());
         sublayerMatrix.Scale(renderSurfaceSublayerScale.x(), renderSurfaceSublayerScale.y());
 
         // The opacity value is moved from the layer to its surface, so that the entire subtree properly inherits opacity.
@@ -669,9 +663,11 @@ static void calculateDrawTransformsInternal(LayerType* layer, const gfx::Transfo
         MathUtil::flattenTransformTo2d(sublayerMatrix);
 
     // Apply the sublayer transform at the center of the layer.
-    sublayerMatrix.Translate(0.5 * bounds.width(), 0.5 * bounds.height());
-    sublayerMatrix.PreconcatTransform(layer->sublayerTransform());
-    sublayerMatrix.Translate(-0.5 * bounds.width(), -0.5 * bounds.height());
+    if (!layer->sublayerTransform().IsIdentity()) {
+        sublayerMatrix.Translate(0.5 * bounds.width(), 0.5 * bounds.height());
+        sublayerMatrix.PreconcatTransform(layer->sublayerTransform());
+        sublayerMatrix.Translate(-0.5 * bounds.width(), -0.5 * bounds.height());
+    }
 
     LayerList& descendants = (layer->renderSurface() ? layer->renderSurface()->layerList() : layerList);
 
@@ -761,8 +757,7 @@ static void calculateDrawTransformsInternal(LayerType* layer, const gfx::Transfo
         // The owning layer's screenSpaceTransform has a scale from content to layer space which we need to undo and
         // replace with a scale from the surface's subtree into layer space.
         gfx::Transform screenSpaceTransform = layer->screenSpaceTransform();
-        screenSpaceTransform.Scale(layer->contentsScaleX(), layer->contentsScaleY());
-        screenSpaceTransform.Scale(1 / renderSurfaceSublayerScale.x(), 1 / renderSurfaceSublayerScale.y());
+        screenSpaceTransform.Scale(layer->contentsScaleX() / renderSurfaceSublayerScale.x(), layer->contentsScaleY() / renderSurfaceSublayerScale.y());
         renderSurface->setScreenSpaceTransform(screenSpaceTransform);
 
         if (layer->replicaLayer()) {