Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely

cc/tiles/picture_layer_tiling.cc

Index: cc/tiles/picture_layer_tiling.cc
diff --git a/cc/tiles/picture_layer_tiling.cc b/cc/tiles/picture_layer_tiling.cc
index 94a36cdde51411ad70db1f54187c9c44e042301d..b06610ff3c5805450478717f6d2d26f30d481ff9 100644
--- a/cc/tiles/picture_layer_tiling.cc
+++ b/cc/tiles/picture_layer_tiling.cc
@@ -405,22 +405,28 @@ PictureLayerTiling::CoverageIterator::CoverageIterator(
 
   dest_to_content_scale_ = tiling_->contents_scale_ / dest_scale;
 
-  gfx::Rect content_rect =
-      gfx::ScaleToEnclosingRect(dest_rect_,
-                                dest_to_content_scale_,
-                                dest_to_content_scale_);
+  SkRect content_rect;

[enne (OOO), 2016/09/02 19:04:54]

What's with SkRect here?

[trchen, 2016/09/02 23:06:54]

LTRB rect is also preferred here. I'll covert it to explicit computation as
well.

+  SkMatrix::MakeScale(dest_to_content_scale_)
+      .mapRectScaleTranslate(&content_rect, gfx::RectToSkRect(dest_rect_));
+  // Shrink rect by 1/512 to avoid generating empty tiles due to FP error.

[enne (OOO), 2016/09/02 19:04:54]

Sorry, but I'm not sure I follow this.

[trchen, 2016/09/02 23:06:54]

It is for avoid hitting DCHECK on line #487.

Although we didn't take enclosing rect on line #410, FP error can still expand
content_rect by 0.5 ulp. Also on line #470, the texel_extent can be shrunk by
0.5 ulp. This can cause a situation that at line #420~#429 we thought a tile
barely intersected with dest rect then later at line #487 found otherwise.

The constant 1/512px is picked by the fact that even if we missed a pixel for
shrinking, the pixel value is guaranteed to be transparent after bilinear
filtering.

+  float epsilon = 1.f / 512.f;
+  content_rect.inset(epsilon, epsilon);
   // IndexFromSrcCoord clamps to valid tile ranges, so it's necessary to
   // check for non-intersection first.
-  content_rect.Intersect(gfx::Rect(tiling_->tiling_size()));
-  if (content_rect.IsEmpty())
+  if (!content_rect.intersect(
+          gfx::RectToSkRect(gfx::Rect(tiling_->tiling_size()))))
     return;
 
-  left_ = tiling_->tiling_data_.TileXIndexFromSrcCoord(content_rect.x());
-  top_ = tiling_->tiling_data_.TileYIndexFromSrcCoord(content_rect.y());
-  right_ = tiling_->tiling_data_.TileXIndexFromSrcCoord(
-      content_rect.right() - 1);
-  bottom_ = tiling_->tiling_data_.TileYIndexFromSrcCoord(
-      content_rect.bottom() - 1);
+  left_ = tiling_->tiling_data_.LastBorderTileXIndexFromSrcCoord(

[enne (OOO), 2016/09/02 19:04:54]

Can you leave some comments about where the 0.5 is coming from here?

[trchen, 2016/09/02 23:06:54]

Acknowledged.

It is difference between sample point location and its integer index.

+      floorf(content_rect.left() - 0.5f));
+  top_ = tiling_->tiling_data_.LastBorderTileYIndexFromSrcCoord(
+      floorf(content_rect.top() - 0.5f));
+  right_ = std::max(tiling_->tiling_data_.FirstBorderTileXIndexFromSrcCoord(
+                        ceilf(content_rect.right() - 0.5f)),
+                    left_);
+  bottom_ = std::max(tiling_->tiling_data_.FirstBorderTileYIndexFromSrcCoord(
+                         ceilf(content_rect.bottom() - 0.5f)),
+                     top_);
 
   tile_i_ = left_ - 1;
   tile_j_ = top_;
@@ -455,10 +461,27 @@ PictureLayerTiling::CoverageIterator::operator++() {
   // edges.
   gfx::Rect last_geometry_rect = current_geometry_rect_;
 
-  gfx::Rect content_rect = tiling_->tiling_data_.TileBounds(tile_i_, tile_j_);
-
-  current_geometry_rect_ =
-      gfx::ScaleToEnclosingRect(content_rect, 1 / dest_to_content_scale_);
+  // Important: To ensure no seams between tiles due to FP error, intermediate

[enne (OOO), 2016/09/02 19:04:54]

I would rather do the computations using gfx::Rect where you can (and explicit
computation where you can't) rather than depending on SkRect implementation
details.

[trchen, 2016/09/02 23:06:54]

Acknowledged.

+  // computation needs to be done in SkRect instead of gfx::RectF. It is
+  // because SkRect stores LTRB internally instead of XYWH.
+  SkRect texel_extent =
+      gfx::RectFToSkRect(tiling_->tiling_data_.TexelExtent(tile_i_, tile_j_));
+  SkMatrix::MakeScale(1 / dest_to_content_scale_)
+      .mapRectScaleTranslate(&texel_extent, texel_extent);
+  SkIRect geometry_rect;
+  texel_extent.roundIn(&geometry_rect);
+  // Extend external edges. Sample coordinates beyond the extent of texels
+  // will be clamped, but will be assigned transparency by AA fragment shaders.
+  {
+    const TilingData& data = tiling_->tiling_data_;
+    int l = tile_i_ ? geometry_rect.left() : 0;
+    int t = tile_j_ ? geometry_rect.top() : 0;
+    int r = (tile_i_ == data.num_tiles_x() - 1) ? data.tiling_size().width()

[enne (OOO), 2016/09/02 19:04:54]

Why do you need this conditional here?

[trchen, 2016/09/02 23:06:54]

Extend the right edge to layer bound like the left/top edge did.

It is due to the fact that the texel extent only covers up to
tiling_size().width() - 0.5, but we still want the quad to cover the whole layer
rect, as that's what the PictureLayerTilingSet and quad system expects.

BTW I'm thinking an alternative layer AA scheme using GL_CLAMP_TO_BORDER with
transparent border. We can discuss about that next time we meet.

+                                                : geometry_rect.right();
+    int b = (tile_j_ == data.num_tiles_y() - 1) ? data.tiling_size().height()
+                                                : geometry_rect.bottom();
+    current_geometry_rect_ = gfx::Rect(l, t, r - l, b - t);
+  }
 
   current_geometry_rect_.Intersect(dest_rect_);
   DCHECK(!current_geometry_rect_.IsEmpty());