Align physical pixels of scrolled layers

cc/layer_tree_host_common.cc

 // Copyright 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/layer_tree_host_common.h"
 
 #include <algorithm>
 
 #include "base/debug/trace_event.h"
 #include "cc/heads_up_display_layer_impl.h"
@@ skipping 522 matching lines @@
             !childLayer->drawProperties().descendants_can_clip_selves ||
             sublayerTransformPreventsClip ||
             !childLayer->transform().IsPositiveScaleOrTranslation())
             descendantsCanClipSelves = false;
     }
 
     layer->drawProperties().num_descendants_that_draw_content = numDescendantsThatDrawContent;
     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>
 static void calculateDrawPropertiesInternal(LayerType* layer, const gfx::Transform& parentMatrix,
     const gfx::Transform& fullHierarchyMatrix, const gfx::Transform& currentScrollCompensationMatrix,
     const gfx::Rect& clipRectFromAncestor, const gfx::Rect& clipRectFromAncestorInDescendantSpace, bool ancestorClipsSubtree,
     RenderSurfaceType* nearestAncestorThatMovesPixels, LayerList& renderSurfaceLayerList, LayerList& layerList,
     LayerSorter* layerSorter, int maxTextureSize, float deviceScaleFactor, float pageScaleFactor, bool subtreeCanUseLCDText,
     gfx::Rect& drawableContentRectOfSubtree, bool updateTilePriorities)
 {
@@ skipping 147 matching lines @@
         // fixed correctly.
         // Note carefully: this is Concat, not Preconcat (currentScrollCompensation * combinedTransform).
         combinedTransform.ConcatTransform(currentScrollCompensationMatrix);
     }
 
     // The drawTransform that gets computed below is effectively the layer's drawTransform, unless
     // the layer itself creates a renderSurface. In that case, the renderSurface re-parents the transforms.
     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;
     // To avoid color fringing, LCD text should only be used on opaque layers with just integral translation.
     bool layerCanUseLCDText = subtreeCanUseLCDText &&
                               (accumulatedDrawOpacity == 1.0) &&
                               layerDrawProperties.target_space_transform.IsIdentityOrIntegerTranslation();
 
     gfx::RectF contentRect(gfx::PointF(), layer->contentBounds());
 
@@ skipping 476 matching lines @@
 
   // At this point, we think the point does hit the touch event handler region on the layer, but we need to walk up
   // the parents to ensure that the layer was not clipped in such a way that the
   // hit point actually should not hit the layer.
   if (pointIsClippedBySurfaceOrClipRect(screenSpacePoint, layerImpl))
      return false;
 
   return true;
 }
 }  // namespace cc