Use LCD text if the transform IsAlmostIdentityAndIntegerTranslation

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 c28db4087d3f31f8d5c671f1042c957d8e43b441..f7eee5b7ed30fa196558f00a12e8b5d81964dec1 100644
--- a/cc/layer_tree_host_common.cc
+++ b/cc/layer_tree_host_common.cc
@@ -546,6 +546,30 @@ static void roundTranslationComponents(gfx::Transform* transform)
     transform->matrix().setDouble(1, 3, MathUtil::Round(transform->matrix().getDouble(1, 3)));
 }
 
+static void roundAlmostIntegerComponent(SkMatrix44* matrix, int i, int j)
+{
+    const double kErrorThreshold = 0.0001;
+    double d = matrix->getDouble(i, j);
+    double rounded = MathUtil::Round(d);
+    double error = std::abs(rounded - d);
+    if (error > 0 && error < kErrorThreshold)
+        matrix->setDouble(i, j, rounded);
+}
+
+// Tiny errors caused by multiple multiplications and divisions on mixed double
+// and float operands may result almost-integer scales and translations like
+// 1.00001. Round them so that some transform can be identity as expected.
+static void roundAlmostIntegerScalesAndTranslationComponents(gfx::Transform* transform)
+{
+    if (transform->IsScaleOrTranslation() && !transform->IsIdentityOrTranslation()) {

[Ian Vollick, 2013/03/07 21:18:36]

Unless I'm reading this wrong, it looks like we'll opt in if (scale ||
translation) && (!identity && !translation) -- i.e., a straight translation
can't be rounded. Isn't that the the most important case?

+        SkMatrix44* matrix = &transform->matrix();
+        roundAlmostIntegerComponent(matrix, 0, 0);
+        roundAlmostIntegerComponent(matrix, 1, 1);
+        roundAlmostIntegerComponent(matrix, 0, 3);
+        roundAlmostIntegerComponent(matrix, 1, 3);
+    }
+}
+
 // Recursively walks the layer tree starting at the given node and computes all the
 // necessary transformations, clipRects, render surfaces, etc.
 template<typename LayerType, typename LayerList, typename RenderSurfaceType>
@@ -719,6 +743,9 @@ static void calculateDrawPropertiesInternal(LayerType* layer, const gfx::Transfo
     layerDrawProperties.target_space_transform = combinedTransform;
     // M[draw] = M[parent] * LT * S[layer2content]
     layerDrawProperties.target_space_transform.Scale(1.0 / layer->contentsScaleX(), 1.0 / layer->contentsScaleY());
+    // Eliminate the tiny errors in scales and translations to prevent the errors from propagating,
+    // to prevent bluriness and ensures LCDText working as expected.
+    roundAlmostIntegerScalesAndTranslationComponents(&layerDrawProperties.target_space_transform);

[enne (OOO), 2013/03/07 19:05:34]

Is it ok to round the draw transform but not the screen space transform
equivalently? Is that dangerous for damage tracking?

[Xianzhu, 2013/03/07 19:38:17]

I think the rounding will reflect in screen_space_transform because it includes
target_space_transform.

[shawnsingh, 2013/03/07 20:13:36]

I agree with Xianzhu, it should be OK with respect to damage tracking, because
the screen space transform is computed from the rounded target space transform. 
I think the damage tracker will work fine as long as it uses the exact same
transforms that are used for drawing.

[Ian Vollick, 2013/03/07 21:18:36]

I'd considered tackling this sort of rounding, too. At that time, I'd thought
that the safest spot was at the last possible moment -- in gl_renderer.cc right
before we hand the matrix off to gl. This would protect us from a mistake where
we modify the screen space transform after rounding it and busting our LCD text.
Is that an option?

 
     // layerScreenSpaceTransform represents the transform between root layer's "screen space" and local content space.
     layerDrawProperties.screen_space_transform = fullHierarchyMatrix;
@@ -764,6 +791,9 @@ static void calculateDrawPropertiesInternal(LayerType* layer, const gfx::Transfo
         // only needs to scale the layer to surface space.
         layerDrawProperties.target_space_transform.MakeIdentity();
         layerDrawProperties.target_space_transform.Scale(renderSurfaceSublayerScale.x() / layer->contentsScaleX(), renderSurfaceSublayerScale.y() / layer->contentsScaleY());
+        // Eliminate the tiny errors in scales and translations to prevent the errors from propagating,
+        // to prevent bluriness and to ensure LCDText working as expected.
+        roundAlmostIntegerScalesAndTranslationComponents(&layerDrawProperties.target_space_transform);

[enne (OOO), 2013/03/07 19:05:34]

I don't understand what this change does.  The LCD text setting has already been
set above for this layer and the target space transform is not the value that is
propagated down the subtree.

[Xianzhu, 2013/03/07 19:38:17]

Based on the exisiting code, we enable LCD text only when the transform
IsIdentityOrIntegerTranslation, otherwise there might be "color fringing". I
think if this target_spae_transform is not IdentityOrIntegerTranslation, LCD
text may be incorrect (in theory). I don't know much about the underlying
theories :)

[enne (OOO), 2013/03/07 20:19:36]

What you say is correct.  However, we enable LCD text on line 759 and this is
line 796.  Also, the transform that is passed to children is the
combinedTransform (well, later sublayerMatrix), which does not come from
target_space_transform.  So, setting this value will not change LCD text for
this surface's subtree or for this layer.  That's what I'm trying to say.

If you want to affect surfaces, you need to round the combinedTransform up
above, since that's what's used on line 788.

 
         // Inside the surface's subtree, we scale everything to the owning layer's scale.
         // The sublayer matrix transforms layer rects into target