cc: Clamp texture coordinates in all tile shaders

cc: Clamp texture coordinates in all tile shaders

Since we can draw quads with a texel:pixel ratio < 1, linear
filtering can cause us to blend texels in that don't have
valid content if we don't clamp texture sampling to the middle
of boundary texels.

Clamping was already done for the anti-aliasing tile shaders.
This patch adds clamping to all tile shaders.

BUG=170722
NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179752

[brianderson, 2013-01-29 03:03:48]

This patch is incomplete, but I would like feedback regarding the parts with
comments.  Is the new version equivalentf? Would it be more efficient?

I would like to apply the AA clamping logic to the non AA fragment shaders.

https://codereview.chromium.org/12086036/diff/1/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/12086036/diff/1/cc/gl_renderer.cc#newcode742
cc/gl_renderer.cc:742: // Map geometry to texture coordinates in the vertex
shader.
I combined the vertex and fragment shader "transformations" into just a vertex
shader "transformation" that doesn't worry about clamping.

https://codereview.chromium.org/12086036/diff/1/cc/gl_renderer.cc#newcode748
cc/gl_renderer.cc:748: // Manually clamp in the fragment shader.
This logic handles the clamping in the fragment shader.

https://codereview.chromium.org/12086036/diff/1/cc/shader.cc
File cc/shader.cc (right):

https://codereview.chromium.org/12086036/diff/1/cc/shader.cc#newcode654
cc/shader.cc:654: vec2 texCoord = clamp(v_texCoord, fragmentTexClamp.xy,
fragmentTexClamp.zw);
Will this be more efficient?

[reveman, 2013-01-29 03:40:23]

https://codereview.chromium.org/12086036/diff/1/cc/shader.cc
File cc/shader.cc (right):

https://codereview.chromium.org/12086036/diff/1/cc/shader.cc#newcode654
cc/shader.cc:654: vec2 texCoord = clamp(v_texCoord, fragmentTexClamp.xy,
fragmentTexClamp.zw);
On 2013/01/29 03:03:48, Brian Anderson wrote:
> Will this be more efficient?

I'm worried it might be significantly less efficient. Afaik, clamp(v_texCoord,
0.0, 1.0) is basically free on all hardware (same as using _SAT modifier in ARB
assembly languages) while other parameters to clamp() might require branching.

https://codereview.chromium.org/12086036/diff/1/cc/tile_draw_quad.h
File cc/tile_draw_quad.h (right):

https://codereview.chromium.org/12086036/diff/1/cc/tile_draw_quad.h#newcode60
cc/tile_draw_quad.h:60: return true || left_edge_aa || top_edge_aa ||
right_edge_aa || bottom_edge_aa;
I assume this is temporary :)

[brianderson, 2013-01-29 18:45:12]

https://codereview.chromium.org/12086036/diff/1/cc/shader.cc
File cc/shader.cc (right):

https://codereview.chromium.org/12086036/diff/1/cc/shader.cc#newcode654
cc/shader.cc:654: vec2 texCoord = clamp(v_texCoord, fragmentTexClamp.xy,
fragmentTexClamp.zw);
On 2013/01/29 03:40:23, David Reveman wrote:
> On 2013/01/29 03:03:48, Brian Anderson wrote:
> > Will this be more efficient?
> 
> I'm worried it might be significantly less efficient. Afaik, clamp(v_texCoord,
> 0.0, 1.0) is basically free on all hardware (same as using _SAT modifier in
ARB
> assembly languages) while other parameters to clamp() might require branching.

The _SAT modifier is pretty cool.

Clamp would be very simple to implement in silicon, so I'd hope hardware
supports it natively, preventing the shader compiler from having to inserting a
branch. If not, a branch would be really bad though...

I guess what is best will be highly dependent on what hardware+drivers we're
running on.

If we stick with the _SAT+MAD, for example, I worry about other shaders that
will have to do the MAD followed by another multiply (such as alpha). Some gpu's
have a pipeline with multiple execution stages where each stage can only execute
a subset of instructions. If a shader needs to use the same stage more than
once, it'll have to cycle through the whole pipeline an additional time.

In any case, figuring out what's best would require a lot of looking at assembly
for different GPU's and then knowing how that assembly runs on the hardware.

My current plan is to revert to _SAT+MAD because it's safer.

We should engage with gpu vendors though - maybe even ask them to evaluate our
shaders for us and submit patches.

https://codereview.chromium.org/12086036/diff/1/cc/tile_draw_quad.h
File cc/tile_draw_quad.h (right):

https://codereview.chromium.org/12086036/diff/1/cc/tile_draw_quad.h#newcode60
cc/tile_draw_quad.h:60: return true || left_edge_aa || top_edge_aa ||
right_edge_aa || bottom_edge_aa;
On 2013/01/29 03:40:23, David Reveman wrote:
> I assume this is temporary :)

Yes, for testing purposes only =)

[brianderson, 2013-01-29 21:54:29]

ptal.

This fixes tile lines when tiles are drawn with ideal_contents_scale < 1.

[enne (OOO), 2013-01-29 22:38:33]

Can you run DumpRenderTree locally and check on:
 compositing/color-matching/image-color-matching.html [ ImageOnlyFailure ]
  compositing/visibility/visibility-image-layers.html [ ImageOnlyFailure ]

Given that images often have some scaling applied, I suspect there are just tiny
clamping differences on the edge of the layer.

If those look fine, this lgtm, although I'd like reveman's feedback as well.

[reveman, 2013-01-29 23:47:26]

lgtm, thanks!

also I'm not against switching away from _SAT + MAD for clamping, especially if
it allows the cleanup you had in the previous patch but I think with right thing
to do is land this first without such a change.

[brianderson, 2013-01-30 00:57:49]

On 2013/01/29 22:38:33, enne wrote:
> Can you run DumpRenderTree locally and check on:
>  compositing/color-matching/image-color-matching.html [ ImageOnlyFailure ]
>   compositing/visibility/visibility-image-layers.html [ ImageOnlyFailure ]
> 
> Given that images often have some scaling applied, I suspect there are just
tiny
> clamping differences on the edge of the layer.

For compositing/color-matching/image-color-matching.html:

Surprisingly, the diffs were not at the edge of any layer; they were at
transitions in color of the image (most of the image is made of regions of
constant color).

Visually, there is not a noticeable difference before and after this patch
though.

I have narrowed this down to rounding differences in how the texture coordinates
are calculated. (Patch Set 1 actually matches the original more closely.)

Although each solution is theoretically equivalent, their results are different
because of rounding. How do we go about committing such a patch without breaking
layout tests?

[enne (OOO), 2013-01-30 01:51:41]

On 2013/01/30 00:57:49, Brian Anderson wrote:

> Although each solution is theoretically equivalent, their results are
different
> because of rounding. How do we go about committing such a patch without
breaking
> layout tests?

This is somewhat tricky as the tests are in WebKit and this change is in
Chromium.  The way you'd do this is to mark the tests as failing in WebKit in
its test expectations, check this change into Chromium, roll the DEPS for
Chromium in WebKit to include this change, then rebaseline the tests in WebKit
once the tests have run there.

[brianderson, 2013-01-30 22:24:31]

Test expectations changed with this patch:
http://trac.webkit.org/changeset/141307

[commit-bot: I haz the power, 2013-01-30 22:26:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/12086036/7001

[brianderson, 2013-01-31 00:33:34]

Going to NOTRY=true. These look like innocuous failures.