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..1796511a48444ef2237c6246d070c4bb4212b56e 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 near-integer scales like 1.00001.
+// Round them to simplify the transform and avoid blurriness.
+static void roundNearIntegerScales(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)

[Sami, 2013/03/05 11:28:34]

I'm wondering if it'd be better to only do this for both x and y scales or
neither. Otherwise we might adjust one axis but the result still won't be
integer scaled on the orthogonal axis.

[Xianzhu, 2013/03/05 17:26:08]

I'm not sure. Considering the case of (scalex, scaley) = (1.00001, 3.5), the
current method they will be rounded to (1, 3.5). This seems still better if
there is only '-webkit-transform: scaleY(3.5)' because the scaleX will be
rounded back to the expected value 1.

BTW, I guess this small inaccuracy of scale is the reason that you see the
pattern in your checkerboard test case.

+            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>
@@ -707,12 +731,27 @@ 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());
+    roundNearIntegerScales(&layerDrawProperties.target_space_transform);

[shawnsingh, 2013/03/05 05:29:45]

(1) It might be a problem that this value is used to compute the subsequent
screen-space transform, isn't it? Does that mean we round twice for the screen
space transform?

(2) What about the situation where a renderSurface re-parents the transform?  in
that case, the layer gets a different target_space transform which may also need
to be correctly rounded?

I'm still uneasy about adding rounding logic of this sort directly into
calcDrawProperties.  Knowing that the code will evolve, it might be less error
prone to make this rounding an explicit step that happens immediately after all
calcDrawProperties recursion. Would that work?

[Sami, 2013/03/05 11:28:34]

One problem I can think of is that rounding can move layers around, so some of
the things we calculate here such as the visible layer rectangle may become
invalid.

[Xianzhu, 2013/03/05 17:26:08]

roundNearIntegerScales() sets back the expected values of scales that contains
small calculation errors. Round it here prevents the errors from propagating.
The change of scale is too small to have visual effects but will make later
Transform::IsIdentityOrXXX() calls work.

roundTranslates() moves the layer, but that only happens if the translates are
merely caused by scrolling and affects only the actual scroll offset.
Would calculateDrawProperties() be called in the case?
Will change to round position() to screen space pixel to avoid the complex
calculation on transforms.

[Xianzhu, 2013/03/05 17:26:08]

Will try to round position to screen space pixel, so that combinedTransform will
based on the adjusted value and make surface's drawTransform (used to calculate
visible rects) and the layers target_space_transform and screen_space_transform
are consistent.
 

[Xianzhu, 2013/03/05 23:03:14]

Sorry, I didn't get your point when posting the reply. In the newest CL the
pixel alignment also affects renderSurface's drawTransform.

 
     // 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);
+    roundNearIntegerScales(&layerDrawProperties.screen_space_transform);
+
+    // Round the translation in simple transforms if translation is merely caused by the scroll offset,
+    // so that the pixels of the layer and the target space can be aligned.
+    LayerType* scrollLayer = LayerTreeHostCommon::findScrollLayerForContentLayer(layer);
+    if (scrollLayer) {

[Sami, 2013/03/05 11:28:34]

Would it be simpler to do this for all scrollable layers instead of checking for
a scrollable parent? We're already walking through the entire hierarchy so we've
already processed the parent at this point; had we aligned the child layer
transform for that parent there's no need to do anything to any of its children,
right?

Maybe another way would be to round layer->scrollDelta() on line 697 to the
nearest screen space pixel? That way there wouldn't be a need to adjust the
transforms after they have been calculated.

[Xianzhu, 2013/03/05 17:26:08]

If the scrollable layer is the content layer itself, its scroll position is
handled here. If the scrollable layer is the parent, the scrollable layer's
position should be zero (correct me if wrong) and the scrolling position can
only be adjusted for the content layer here.
It won't work, because actually the other main cause of the subpixel position is
the integeral scrollOffset() in CSS coordinates (equals to position() of the
content layer).

However, I think this is a good direction to go. I'll try to round position to
screen space pixel to avoid adjusting the transforms.

[Xianzhu, 2013/03/05 23:03:14]

I was wrong about this. Actually position() is non-zero for the scrollable layer
and is zero for the content layer. Now deal with the scrollable layer only.

[Xianzhu, 2013/03/05 23:03:14]

Done. Slightly different: now rounds the translations of combinedTransform.

+        gfx::Vector2dF scrollPosition = scrollLayer->scrollOffset() + scrollLayer->scrollDelta();
+        if (position.x() == -scrollPosition.x() && position.y() == -scrollPosition.y()) {
+            if (layerDrawProperties.target_space_transform.IsIdentityOrTranslation())
+                roundTranslates(&layerDrawProperties.target_space_transform);
+            if (layerDrawProperties.screen_space_transform.IsIdentityOrTranslation())
+                roundTranslates(&layerDrawProperties.screen_space_transform);
+        }
+    }
 
     // Adjusting text AA method during animation may cause repaints, which in-turn causes jank.
     bool adjustTextAA = !animatingOpacityToScreen && !animatingTransformToScreen;