Align physical pixels of scrolled layers

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 3d06aa4a12b98233ed420ff14677606e16c3c93e..27d1e10027d1cbee1740b6dd351e42cc6672b7d8 100644
--- a/cc/layer_tree_host_common.cc
+++ b/cc/layer_tree_host_common.cc
@@ -540,6 +540,30 @@ static void preCalculateMetaInformation(LayerType* layer)
     layer->drawProperties().descendants_can_clip_selves = descendantsCanClipSelves;
 }
 
+// Small calculation errors may result almost-integer scales like 1.00001.

[enne (OOO), 2013/03/05 23:46:28]

Where is this calculation error introduced?

[Xianzhu, 2013/03/06 00:26:13]

I observed the near 1 scales in content transformations, but didn't trace the
exact places where the error is introduced. I think it comes from accumulated
errors of multiple multiplication and division operations over mixed float
(individual scales) and double (in matrix) operands.

+// Round them so that some transform can be identity as expected.
+static void roundAlmostIntegerScales(gfx::Transform* transform)
+{
+    const double kErrorThreshold = 0.0001;
+    if (transform->IsScaleOrTranslation() && !transform->IsIdentityOrTranslation()) {
+        double scaleX = transform->matrix().getDouble(0, 0);
+        double roundedScaleX = round(scaleX);
+        if (std::abs(scaleX - roundedScaleX) < kErrorThreshold)
+            transform->matrix().setDouble(0, 0, roundedScaleX);
+
+        double scaleY = transform->matrix().getDouble(1, 1);
+        double roundedScaleY = round(scaleY);
+        if (std::abs(scaleY - roundedScaleY) < kErrorThreshold)
+            transform->matrix().setDouble(1, 1, roundedScaleY);
+    }
+}
+
+static void roundTranslates(gfx::Transform* transform)
+{
+    transform->matrix().setDouble(0, 3, round(transform->matrix().getDouble(0, 3)));
+    transform->matrix().setDouble(1, 3, round(transform->matrix().getDouble(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>
@@ -694,6 +718,12 @@ static void calculateDrawPropertiesInternal(LayerType* layer, const gfx::Transfo
     // Note carefully: this is Concat, not Preconcat (implTransform * combinedTransform).
     combinedTransform.ConcatTransform(layer->implTransform());
 
+    if (layer->scrollable() && combinedTransform.IsScaleOrTranslation() && layer->transform().IsIdentity()) {

[enne (OOO), 2013/03/05 23:46:28]

Why is it so important that layer->transform().IsIdentity() here?  I might be
missing something, but it seems like the
combinedTransform.IsScaleOrTranslation() check should be sufficient.

[Xianzhu, 2013/03/06 00:26:13]

I think for example if the layer itself has a non-integral translation, it'd be
better not to round to avoid damage to the original transform.

In addition I couldn't make sure how the rounding and the layer transform
interferes and couldn't create tests to cover this in time. Would a TODO look
good?

Don't know if we have real examples of scrollable layer having a non-identity
transform. Just for analogy a non-scrollable layer example is maps for which we
must keep the exact original transforms to avoid gaps between the map tiles.

[enne (OOO), 2013/03/06 00:40:17]

I still don't follow what the case you're trying to prevent here with the
IsIdentity check, other than just trying to be conservative against the unknown.
 If you want to test this, just add a transform with a non-integer translation
to the scrollable layer in your unit test, and I suspect it will continue to
work as expected.

+        // Align the scrollable layer's position to screen space pixels to avoid blurriness.
+        // To avoid side-effects, do this only if the transform is simple.
+        roundTranslates(&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
@@ -707,12 +737,14 @@ 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());
+    roundAlmostIntegerScales(&layerDrawProperties.target_space_transform);

[enne (OOO), 2013/03/05 23:46:28]

As you've already rounded combinedTransform above, why is this necessary?

[Xianzhu, 2013/03/06 00:26:13]

This is to meet the condition of layerCanUseLCDText below. It may also affect
sharpness of text. If the scale is almost 1 but not 1, LCD text won't be used.

Will add some comments.

 
     // layerScreenSpaceTransform represents the transform between root layer's "screen space" and local content space.
     layerDrawProperties.screen_space_transform = fullHierarchyMatrix;
     if (!layer->preserves3D())
         layerDrawProperties.screen_space_transform.FlattenTo2d();
     layerDrawProperties.screen_space_transform.PreconcatTransform(layerDrawProperties.target_space_transform);
+    roundAlmostIntegerScales(&layerDrawProperties.screen_space_transform);

[enne (OOO), 2013/03/05 23:46:28]

Is this rounding necessary? As Shawn points out, if there's an intermediate
render surface (i.e. target_space_transform != screen_space_transform), then
rounding may not make any sense.

[Xianzhu, 2013/03/06 00:26:13]

Will remove.

 
     // Adjusting text AA method during animation may cause repaints, which in-turn causes jank.
     bool adjustTextAA = !animatingOpacityToScreen && !animatingTransformToScreen;