New issue
Advanced search Search tips

Issue 643464 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

PictureLayerTiling generates quads that exceed valid sample extent

Project Member Reported by trchen@chromium.org, Sep 2 2016

Issue description

Version: ToT
OS: All

What steps will reproduce the problem?
(1) Go to http://jsbin.com/dezalef/

What is the expected output?
The top and bottom pattern should look identical.
Ideally there should be no seams at tile boundary, but a separate issue with AA fragment shader will cause seams for quads that reveal layer edges.

What do you see instead?
There are visible seams even between internal tiles.

Detailed analysis from original thread https://codereview.chromium.org/2175553002/#msg41 pasted below:

On 2016/08/29 20:21:19, enne wrote:
https://codereview.chromium.org/2175553002/diff/60001/cc/tiles/picture_layer_...
> > cc/tiles/picture_layer_tiling.cc:468:
> tile_bounds_in_dest_space.Offset(-tiling_->contents_translation());
> > On 2016/08/22 22:57:39, enne wrote:
> > > I'm not 100% convinced that this satisfies the assumptions being made by
> > > coverage iterator, in particular that it will always fill the destination
> space
> > > perfectly.  Can you add some PictureLayerTilingSetTestWithResources
> unittests
> > > that include translation offsets as well at different scales?
> > 
> > It will because the pre-rounding tile_bounds_in_dest_space already fill the
> dest space. With ToEnclosingRect, some pixels in the dest space will be
covered
> by more than one tiles.
> > 
> > In fact, some rounding math here is already questionable prior to my CL. I
> don't think we can simply take the enclosing rect for geometry rect. I will
> follow up with more numerical analysis.
> 
> What is questionable about it? I'll be interested to hear your analysis.

Assuming no MSAA, the sample point of each pixel will be the center of that
pixel. For a tile of 256x256, the top-left sample point has coordinate of (0.5,
0.5). Likewise, (255.5, 255.5) for the bottom-right.

In other words, the texture sampling extent for a tile is only
tile_size.Inset(0.5). Sample points outside of that range will be wrapped
depending on the GL wrap mode. This is undesired for tiling except for the layer
edges.

Now look at our 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_);

Is it possible for the geometry rect to cover a point that will result in an
invalid sample point? The answer is yes, consider the configuration:

layer_size = 1000x1000
layer_transform = = draw_transform = translate2d(0.6px, 0.6px)
ideal_scale = 1
dest_scale = 1 + ε

Consider an ideal tiling of
contents_scale = 1
The content_rect of the top-left tile would be
(l=0, t=0, w=255, h=255)
The rect scaled to the dest space became (l=0, t=0, w=255+ε, h=255+ε)
Taking the enclosing rect,
current_geometry_rect = (l=0, t=0, w=256, h=256)

Now as we draw the quad, the quad will cover (l=0.6, t=0.6, w=256-ε, h=256-ε),
which include the screen pixel at (256.5, 256.5). Mapping it back to the texture
space the sampling point would be (255.9, 255.9), that exceeded the valid
texture range.

We don't see this drawing artifacts often because the condition is very rare as
most of the time the ideal tiling is the tiling with maximum scale. I think it
can only happen during a pinch zoom out, and only on tiles that are
unfortunately rounded outward by more than 0.5px.

Anyway, bad math is bad math and we can certainly do it more correctly.

The right way to calculate geometry rect is to take the inset of the tile size:

gfx::RectF tile_sample_extent = tiling_data.TileBoundsWithBorder().Inset(0.5);
gfx::RectF unrounded_geometry_rect = gfx::ScaleRect(tile_sample_extent,
content_to_dest_scale);

These are exact rect that contains neither false positive or false negative.

And I would recommend to do coverage in float region instead of integer region.
I don't think float region would be much slower than integer region as there is
no arithmetic involved, only comparison. FP comparison should be as fast as
integers as long as no special numbers are involved. Doing FP region would allow
us to use a tighter overlap between tiles (1 instead of 2px), that saves about
0.7% of texture memory.

If we have to stay with integer region, then we'll need to round inwards instead
of outward:

current_geometry_rect_ = gfx::ToEnclosedRect(unrounded_geometry_rect);

Now the question becomes whether the rounded geometry rect will cover the dest
space. The answer is yes with the following proof:

1. content_to_dest_scale >= 1                        // because dest scale is
max scale
2. right_edge_of_tile = left_edge_of_next_tile + 2   // 2px overlap between
tiles
3. right_edge_of_sample_extent = right_edge_of_tile - 0.5
4. left_edge_of_next_sample_extent = left_edge_of_next_tile + 0.5
5. right_edge_of_geometry_rect = floor(right_edge_of_sample_extent *
content_to_dest_scale)
6. left_edge_of_next_geometry_rect = ceil(left_edge_of_next_sample_extent *
content_to_dest_scale)
7. right_edge_of_geometry_rect
= floor(right_edge_of_sample_extent * content_to_dest_scale)
= floor((left_edge_of_next_sample_extent + 1) * content_to_dest_scale)  // plug
2,3,4
= floor(left_edge_of_next_sample_extent * content_to_dest_scale +
content_to_dest_scale)
>= floor(left_edge_of_next_sample_extent * content_to_dest_scale + 1)   //
because 1
>= ceil(left_edge_of_next_sample_extent * content_to_dest_scale)
= left_edge_of_next_geometry_rect
Q.E.D.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d5e2d15230ddfe690803663e408389eeaf17afe6

commit d5e2d15230ddfe690803663e408389eeaf17afe6
Author: trchen <trchen@chromium.org>
Date: Wed Sep 21 21:49:39 2016

Improve PictureLayerTiling::CoverageIterator to handle rounding more precisely

There are some rare corner cases that PictureLayerTiling::CoverageIterator
could generate quads that slightly exceed valid sample extent due to rounding
error.

Due to bilinear filtering, the valid sample extent of a texture without
introducing clamping error is the rect enclosed by texel sample points.
For example, a texture of 256 x 256 without MSAA has top-left sample point at
(0.5, 0.5) and bottom-right sample point at (255.5, 255.5). Therefore the
sample extent of such texture is the rect (x=0.5, y=0.5, w=255, h=255).

Previously the geometry rect of a tile is computed by:
geometry_rect = ScaleToEnclosingRect(tile_bounds, content_to_dest)

The tile_bounds is enclosed by the pixel border of the tile minus the extra
border texels, which a conservative estimate of the sample extent (reserves
a margin of 0.5 texels). However rounding out the rect in dest space can
introduce rounding error up to 1.0 texels, which blew up the error budget.

This CL changed the formula to:
geometry_rect = ScaleToEnclosedRect(sample_extent, content_to_dest)
Where the sample extent is an exact rect with no error margin. Then we round
inward instead of outward for a conservative rect after scaling to dest space.

Numerical analysis for coverage proof can be found at https://crbug.com/643464

BUG=643464
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2295343005
Cr-Commit-Position: refs/heads/master@{#420170}

[modify] https://crrev.com/d5e2d15230ddfe690803663e408389eeaf17afe6/cc/base/tiling_data.cc
[modify] https://crrev.com/d5e2d15230ddfe690803663e408389eeaf17afe6/cc/base/tiling_data.h
[modify] https://crrev.com/d5e2d15230ddfe690803663e408389eeaf17afe6/cc/layers/picture_layer_impl.cc
[modify] https://crrev.com/d5e2d15230ddfe690803663e408389eeaf17afe6/cc/layers/picture_layer_impl_unittest.cc
[modify] https://crrev.com/d5e2d15230ddfe690803663e408389eeaf17afe6/cc/tiles/picture_layer_tiling.cc
[modify] https://crrev.com/d5e2d15230ddfe690803663e408389eeaf17afe6/cc/tiles/picture_layer_tiling.h
[modify] https://crrev.com/d5e2d15230ddfe690803663e408389eeaf17afe6/cc/tiles/picture_layer_tiling_unittest.cc

Owner: danakj@chromium.org
I'm leaving the team thus re-assigning bugs.

As of today the tiling seam still can be seen in the above example. Is due to AA shader not distinguishing external and internal edge. Only an external edges need linear fade-off.
Owner: ----
Status: Available (was: Started)

Sign in to add a comment