Clamp YUV videos to their visible size in the shader

Clamp YUV videos to their visible size in the shader

Video textures can be larger than the visible size of the video.
Because they have bilinear filtering turned on for them, if the video is
scaled, then the this means that the part of the texture outside of the
video itself can be sampled, causing odd visible artifacts like
hairlines on the side of the image.

To fix this, pass in a clamp value of half a pixel (because of bilinear
filtering) inside of the visible rect and clamp all texture coordinates
to this maximum value in the fragment shader.

BUG=429640
Committed: https://crrev.com/a2a2970d72e7856444896600874fca9594f55437
Cr-Commit-Position: refs/heads/master@{#314200}

[enne (OOO), 2015-01-28 00:38:42]

enne@chromium.org changed reviewers:
+ danakj@chromium.org, scherkus@chromium.org

[danakj, 2015-01-28 01:02:00]

Can we pixeltest it?

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

https://codereview.chromium.org/881963002/diff/1/cc/output/shader.cc#newcode229
cc/output/shader.cc:229: uniform TexCoordPrecision vec2 clampSize; void main() {
formatting..

https://codereview.chromium.org/881963002/diff/1/cc/output/shader.cc#newcode231
cc/output/shader.cc:231: v_texCoord = min(clampSize, a_texCoord * texScale +
texOffset);
do we not have to clamp on the left/top too?

[enne (OOO), 2015-01-31 01:32:14]

The new yuv_striped_offset.png is because that test also no longer has bleed
through on the edges.  (I think it looks better.)

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

https://codereview.chromium.org/881963002/diff/1/cc/output/shader.cc#newcode229
cc/output/shader.cc:229: uniform TexCoordPrecision vec2 clampSize; void main() {
On 2015/01/28 01:02:00, danakj wrote:
> formatting..

git cl format T_T

https://codereview.chromium.org/881963002/diff/1/cc/output/shader.cc#newcode231
cc/output/shader.cc:231: v_texCoord = min(clampSize, a_texCoord * texScale +
texOffset);
On 2015/01/28 01:02:00, danakj wrote:
> do we not have to clamp on the left/top too?

Turns out we do.  Thanks.  I changed clampSize to clampRect in the shader, but
left it as "texture size" in the quad, since that's easier to specify.

https://codereview.chromium.org/881963002/diff/20001/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/881963002/diff/20001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:530: const gfx::Rect
rect(shared_state->content_bounds);
Without this change, the video quads were ending up being the entire viewport
size (and then scaled).  Oops.

https://codereview.chromium.org/881963002/diff/20001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/881963002/diff/20001/cc/output/shader.cc#newc...
cc/output/shader.cc:1792: vec2 clamped = max(clamp_rect.xy, min(clamp_rect.zw,
v_texCoord));
I moved the clamping from the vertex to the fragment shader, because otherwise
it would cause stretching of the texture, rather than just more localized
clamping.

[danakj, 2015-02-02 18:43:03]

LGTM

https://codereview.chromium.org/881963002/diff/20001/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/881963002/diff/20001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:463: void
CreateTestYUVVideoDrawQuad_TwoColor(const SharedQuadState* shared_state,
can you leave a comment in here somewhere that explains what this function
creates in a bit more detail?

https://codereview.chromium.org/881963002/diff/20001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:694: // Test that a YUV video doesn't bleed
outside of its tex coords when the
Should we test YUVA shader too?

https://codereview.chromium.org/881963002/diff/20001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/881963002/diff/20001/cc/output/shader.cc#newc...
cc/output/shader.cc:222: precision mediump float; attribute vec4 a_position;
?

[enne (OOO), 2015-02-02 19:37:08]

enne@chromium.org changed reviewers:
+ jamesr@chromium.org, wfh@chromium.org

[enne (OOO), 2015-02-02 19:37:39]

danakj: take another look if you want (or not)
jamesr: mojo/ OWNERS
wfh: content/common/cc_messages.h OWNERS

[Will Harris, 2015-02-02 19:41:57]

On 2015/02/02 19:37:39, enne wrote:
> danakj: take another look if you want (or not)
> jamesr: mojo/ OWNERS
> wfh: content/common/cc_messages.h OWNERS

LGTM on content/common/cc_messages.h

[danakj, 2015-02-02 19:42:09]

lgtm

[jamesr, 2015-02-02 20:23:11]

lgtm

[enne (OOO), 2015-02-02 20:49:52]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2015-02-02 20:51:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/881963002/60001

[commit-bot: I haz the power, 2015-02-02 22:21:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-02 22:23:09]

Patchset 4 (id:??) landed as
https://crrev.com/a2a2970d72e7856444896600874fca9594f55437
Cr-Commit-Position: refs/heads/master@{#314200}