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 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>
 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 134 matching lines @@
     // 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 transformation from the impl thread then it should be at
     // the start of the combinedTransform, but we don't want it to affect the
     // computation of contentsScale above.
     // 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
         // 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());
+    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;
     // 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