Raster PictureLayerTiling with fractional translation

cc/layers/picture_layer_impl.cc

Index: cc/layers/picture_layer_impl.cc
diff --git a/cc/layers/picture_layer_impl.cc b/cc/layers/picture_layer_impl.cc
index c8b45b730238455f8749fd0d844a377b592d4356..e73848ab322eccff552455124921d1dc61550834 100644
--- a/cc/layers/picture_layer_impl.cc
+++ b/cc/layers/picture_layer_impl.cc
@@ -851,12 +851,15 @@ void PictureLayerImpl::SetNearestNeighbor(bool nearest_neighbor) {
   NoteLayerPropertyChanged();
 }
 
-PictureLayerTiling* PictureLayerImpl::AddTiling(float contents_scale) {
+PictureLayerTiling* PictureLayerImpl::AddTiling(
+    float contents_scale,
+    const gfx::Vector2dF& contents_translation) {
   DCHECK(CanHaveTilings());
   DCHECK_GE(contents_scale, MinimumContentsScale());
   DCHECK_LE(contents_scale, MaximumContentsScale());
   DCHECK(raster_source_->HasRecordings());
-  return tilings_->AddTiling(contents_scale, raster_source_);
+  return tilings_->AddTiling(contents_scale, contents_translation,
+                             raster_source_);
 }
 
 void PictureLayerImpl::RemoveAllTilings() {
@@ -873,8 +876,9 @@ void PictureLayerImpl::AddTilingsForRasterScale() {
   PictureLayerTiling* high_res =
       tilings_->FindTilingWithScale(raster_contents_scale_);

[enne (OOO), 2016/08/22 22:57:39]

What if the layer moves and the contents translation for the high res version
becomes not ideal? Shouldn't this code not consider that high res and make
another?

Your patch also does not adjust PictureLayerImpl::GetPendingOrActiveTwinTiling
and so two tilings that have different content translations will be considered
twins, and so "required for activation" will be incorrect.

I think you need to write a unit test for this activation case to make sure that
if the main thread adjusts the layer offset that activation will not occur until
a tiling with the correct translation is ready.

[trchen, 2016/08/29 10:14:48]

It was what was done in patchset 1, but later I changed to make content
translation recalc lazier. The policy implemented in this patchset is to never
change content translation of existing tile sets.
You are right. It'd be a best if we can allow an ideal tile set and a non-ideal
tile set that has different translation.

In the newly uploaded patchset I made GetPendingOrActiveTwinTiling to return
nullptr if the transform doesn't match. During activation the old tile set will
be purged.

   if (!high_res) {
+    gfx::Vector2dF raster_translation = CalculateRasterTranslation();
     // We always need a high res tiling, so create one if it doesn't exist.
-    high_res = AddTiling(raster_contents_scale_);
+    high_res = AddTiling(raster_contents_scale_, raster_translation);
   } else if (high_res->may_contain_low_resolution_tiles()) {
     // If the tiling we find here was LOW_RESOLUTION previously, it may not be
     // fully rastered, so destroy the old tiles.
@@ -976,11 +980,34 @@ void PictureLayerImpl::AddLowResolutionTilingIfNeeded() {
   bool is_animating = draw_properties().screen_space_transform_is_animating;
   if (!is_pinching && !is_animating) {
     if (!low_res)
-      low_res = AddTiling(low_res_raster_contents_scale_);
+      low_res = AddTiling(low_res_raster_contents_scale_, gfx::Vector2dF());
     low_res->set_resolution(LOW_RESOLUTION);
   }
 }
 
+gfx::Vector2dF PictureLayerImpl::CalculateRasterTranslation() {

[enne (OOO), 2016/08/22 22:57:39]

This function depending on raster_contents_scale_ is a little weird, and means
that it can only be used for high res.  Can you make it take
raster_contents_scale_ as a parameter explicitly?

[trchen, 2016/08/29 10:14:48]

Done.

+  if (draw_properties().screen_space_transform_is_animating)
+    return gfx::Vector2dF();
+
+  gfx::Transform draw_transform = DrawTransform();

[trchen, 2016/08/12 22:11:30]

We can certainly use draw transform to align instead. The above check at line
989 becomes weird though, because we can have static draw transform while having
animating screen transform.

Personally I think it is okay to use either. Using the draw transform makes more
sense in today's world. In the future world I believe draw transform will be
equal to screen transform in most of the cases (i.e. render surfaces align with
device pixels).

[enne (OOO), 2016/08/16 23:02:52]

If you have a static draw transform, then you are already snapped to pixel
coordinates and its your target that needs to adjust.  Adjusting to screen
coordinates would be incorrect.

+  if (!draw_transform.IsScaleOrTranslation())
+    return gfx::Vector2dF();
+
+  // Good match if the maximum alignment error on a layer of size 10000px
+  // does not exceed 0.001px.
+  static constexpr float kErrorThreshold = 0.0000001f;

[enne (OOO), 2016/08/22 22:57:39]

I think this function needs some more comments.  This block of code would be
good to say something like "if the draw transform scale and raster content scale
are not equal, then the tiling will not be aligned to the target and so a
content scale would not help with making the tiling appear crisp".

[trchen, 2016/08/29 10:14:48]

Done.

+  if (std::abs(draw_transform.matrix().getFloat(0, 0) -
+               raster_contents_scale_) > kErrorThreshold ||
+      std::abs(draw_transform.matrix().getFloat(1, 1) -
+               raster_contents_scale_) > kErrorThreshold)
+    return gfx::Vector2dF();
+
+  float origin_x = draw_transform.matrix().getFloat(0, 3);
+  float origin_y = draw_transform.matrix().getFloat(1, 3);
+  return gfx::Vector2dF(origin_x - floorf(origin_x),
+                        origin_y - floorf(origin_y));
+}
+
 void PictureLayerImpl::RecalculateRasterScales() {
   if (is_directly_composited_image_) {
     if (!raster_source_scale_)