Increase YUV video clamping

Increase YUV video clamping

The current clamping is not enough to avoid bleeding issues. This is
exposed when changing the background color in CreateEdgeBleedPass from
black to gray. The reason it works now is because interpolating U/V
textures from black (YUV = 0, 0, 0) to green (YUV = 149, 43, 21) will
still result in saturated green pixels (RGB = 0, 255, 0). All YUV values
in the range (149, 0-43, 0-21) will end up as RGB = (0, 255, 0).

The current clamping is not enough, because the calculation
'0.5f / quad->tex_size.width()' is using the Y texture size, the highest
resolution plane, but the U/V texture size is only half of that. This CL
adds individual sizes for the Y/A and U/V textures.

BUG=429640, 467283
Committed: https://crrev.com/4be817dd606efdba1614d32608ddd6e16bfa687d
Cr-Commit-Position: refs/heads/master@{#320567}

Committed: https://crrev.com/7364ff94a2bd814dd198f609f8d5bacabe1d563b
Cr-Commit-Position: refs/heads/master@{#322541}

[magjed_chromium, 2015-03-12 20:25:35]

magjed@chromium.org changed reviewers:
+ enne@chromium.org

[magjed_chromium, 2015-03-12 20:25:55]

Please take a look.

[enne (OOO), 2015-03-12 21:33:45]

https://codereview.chromium.org/1008493002/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1008493002/diff/40001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2015: const float inset_x = 1.0f /
quad->tex_size.width();
Can you make this calculation more clear that it's still half a pixel, maybe
with a comment?

https://codereview.chromium.org/1008493002/diff/40001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2017: clamp_rect.Inset(clamp_rect.x() == 0.0f ? 0.0f :
inset_x,
You're doing this in case the video is scaled up and then the first/last
row/column would be sampled slightly incorrectly? Is this why the
yuv_stripes_offset test changed?

[magjed_chromium, 2015-03-12 21:49:47]

https://codereview.chromium.org/1008493002/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1008493002/diff/40001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2015: const float inset_x = 1.0f /
quad->tex_size.width();
On 2015/03/12 21:33:45, enne wrote:
> Can you make this calculation more clear that it's still half a pixel, maybe
> with a comment?

Is there any way to get the size of each texture? Then I can select the smallest
one here. Also, some YUV formats do not have U/V subsampled 2x2, e.g. YV16. For
these formats, we don't need to inset so much.

https://codereview.chromium.org/1008493002/diff/40001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2017: clamp_rect.Inset(clamp_rect.x() == 0.0f ? 0.0f :
inset_x,
On 2015/03/12 21:33:45, enne wrote:
> You're doing this in case the video is scaled up and then the first/last
> row/column would be sampled slightly incorrectly? Is this why the
> yuv_stripes_offset test changed?

I just felt it was unnecessary to crop the image if we are already at the border
of the texture, which should be a common case. At the border we don't have any
sampling issues. The yuv_stripes_offset test will change regardless, but if I
don't have the conditionals in the Inset, I need to update the SimpleYUVRect
test case as well.

[enne (OOO), 2015-03-12 21:53:57]

https://codereview.chromium.org/1008493002/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1008493002/diff/40001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2015: const float inset_x = 1.0f /
quad->tex_size.width();
On 2015/03/12 21:49:47, magjed wrote:
> On 2015/03/12 21:33:45, enne wrote:
> > Can you make this calculation more clear that it's still half a pixel, maybe
> > with a comment?
> 
> Is there any way to get the size of each texture? Then I can select the
smallest
> one here. Also, some YUV formats do not have U/V subsampled 2x2, e.g. YV16.
For
> these formats, we don't need to inset so much.

To get the size of the texture, you'd have to pass it with the quad.  I added
tex_size to the yuv quad for this clamping.  If you wanted, you could make the
yuv quad contain precomputed x/y inset values instead?

[magjed_chromium, 2015-03-13 19:49:05]

https://codereview.chromium.org/1008493002/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1008493002/diff/40001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2015: const float inset_x = 1.0f /
quad->tex_size.width();
On 2015/03/12 21:53:57, enne wrote:
> On 2015/03/12 21:49:47, magjed wrote:
> > On 2015/03/12 21:33:45, enne wrote:
> > > Can you make this calculation more clear that it's still half a pixel,
maybe
> > > with a comment?
> > 
> > Is there any way to get the size of each texture? Then I can select the
> smallest
> > one here. Also, some YUV formats do not have U/V subsampled 2x2, e.g. YV16.
> For
> > these formats, we don't need to inset so much.
> 
> To get the size of the texture, you'd have to pass it with the quad.  I added
> tex_size to the yuv quad for this clamping.  If you wanted, you could make the
> yuv quad contain precomputed x/y inset values instead?

Yeah, that would solve the problem. Maybe an even simpler solution is to let
|tex_size| represent the lowest texture resolution instead? That way we can keep
0.5f in the calculation for example. I updated the CL with this change. What do
you think?

[enne (OOO), 2015-03-13 19:53:16]

lgtm, thanks!

Can you also wrap your change description to 72 columns so it plays well with
git?

[magjed_chromium, 2015-03-13 20:03:24]

The CQ bit was checked by magjed@chromium.org

[commit-bot: I haz the power, 2015-03-13 20:04:39]

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

[magjed_chromium, 2015-03-13 20:21:45]

The CQ bit was unchecked by magjed@chromium.org

[magjed_chromium, 2015-03-13 20:24:54]

magjed@chromium.org changed reviewers:
+ danakj@chromium.org

[magjed_chromium, 2015-03-13 20:25:15]

danakj - Can you take a look?

[danakj, 2015-03-13 20:26:09]

content/ changes LGTM

[magjed_chromium, 2015-03-13 20:27:40]

magjed@chromium.org changed reviewers:
+ wfh@chromium.org

[magjed_chromium, 2015-03-13 20:28:39]

Will Harris - Can you review content/common/cc_messages.h?

[Will Harris, 2015-03-13 20:35:06]

On 2015/03/13 20:28:39, magjed wrote:
> Will Harris - Can you review content/common/cc_messages.h?

content/common/cc_messages.h lgtm

[magjed_chromium, 2015-03-13 20:44:40]

The CQ bit was checked by magjed@chromium.org

[commit-bot: I haz the power, 2015-03-13 20:45:15]

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

[commit-bot: I haz the power, 2015-03-13 20:48:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-13 20:49:37]

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

[pdr., 2015-03-14 04:24:38]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1007953003/ by pdr@chromium.org.

The reason for reverting is: I think this patch caused blink layouttest failures
(e.g., media/video-colorspace-yuv420.html). See http://crbug.com/467283.

[magjed_chromium, 2015-03-14 18:13:22]

enne - Please take another look. I had to add another texture size to
YUVVideoDrawQuad to pass the layout tests. Now uv and ya have separate clamping
so that we don’t ignore any pixels. It's getting a bit bloated, but I couldn’t
think of any other way to solve this. What do you think?

[magjed_chromium, 2015-03-18 16:08:34]

Ping. enne - Can you take a look again?

[enne (OOO), 2015-03-18 19:25:58]

lgtm

https://codereview.chromium.org/1008493002/diff/80001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/1008493002/diff/80001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:239: DCHECK(ya_tex_size ==
DCHECK_EQ

https://codereview.chromium.org/1008493002/diff/80001/cc/output/renderer_pixe...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/1008493002/diff/80001/cc/output/renderer_pixe...
cc/output/renderer_pixeltest.cc:258: DCHECK(ya_tex_size ==
media::VideoFrame::PlaneSize(
DCHECK_EQ

[magjed_chromium, 2015-03-18 20:21:54]

Patchset #6 (id:100001) has been deleted

[magjed_chromium, 2015-03-18 21:38:43]

danakj, Will Harris - Can you take another look?

https://codereview.chromium.org/1008493002/diff/80001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/1008493002/diff/80001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:239: DCHECK(ya_tex_size ==
On 2015/03/18 19:25:57, enne wrote:
> DCHECK_EQ

Unfortunately it's not possible to print gfx::Size.

[danakj, 2015-03-18 21:40:23]

content/ still lgtm

[Will Harris, 2015-03-18 21:40:29]

content/common/cc_messages.h lgtm

looks like you still need a reviewer for the mojo code.

[magjed_chromium, 2015-03-18 21:46:02]

magjed@chromium.org changed reviewers:
+ jamesr@chromium.org

[magjed_chromium, 2015-03-18 21:48:06]

jamesr - Can you take a look at
mojo/converters/surfaces/surfaces_type_converters.cc?

[magjed_chromium, 2015-03-26 17:07:40]

Ping. jamesr - Can you take a look?

[jamesr, 2015-03-26 19:18:42]

On 2015/03/26 17:07:40, magjed wrote:
> Ping. jamesr - Can you take a look?

lgtm

[magjed_chromium, 2015-03-27 08:02:51]

The CQ bit was checked by magjed@chromium.org

[magjed_chromium, 2015-03-27 08:02:52]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1008493002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-03-27 08:03:03]

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

[commit-bot: I haz the power, 2015-03-27 09:11:21]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-03-27 09:11:58]

Patchset 6 (id:??) landed as
https://crrev.com/7364ff94a2bd814dd198f609f8d5bacabe1d563b
Cr-Commit-Position: refs/heads/master@{#322541}