VideoLayerImpl: subtract 1 from visible size when calculating tex stretch.

VideoLayerImpl: subtract 1 from visible size when calculating tex stretch.

The video texture only has "visible rect" valid and we're sampling
outside of that visible rect and into garbage texture space due to
bilinear filtering. The fix is to stretch the video such that it will
always have an extra set of texels to sample from. Subtract one from
the visible size when calculating the tex stretch x and y scale.

BUG=575587
TEST=Test youtube.com/watch?v=CoOFjXk4xLY on nyan_big and peach_pi.
Run cc_unittests.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/64647caadd62fc639f6a3bb194557d8447f6d419
Cr-Commit-Position: refs/heads/master@{#373478}

[wuchengli, 2016-01-30 00:50:14]

Description was changed from

==========
VideoLayerImpl: subtract 1 from visible size when calculating tex stretch.

The video texture only has "visible rect" valid and we're sampling
outside of that visible rect and into garbage texture space due to
bilinear filtering. The fix is to stretch the video such that it will
always have an extra set of texels to sample from. Subtract one from
the visible size when calculating the tex stretch x and y scale.

BUG=575587
TEST=Will test nyan next Monday.
==========

to

==========
VideoLayerImpl: subtract 1 from visible size when calculating tex stretch.

The video texture only has "visible rect" valid and we're sampling
outside of that visible rect and into garbage texture space due to
bilinear filtering. The fix is to stretch the video such that it will
always have an extra set of texels to sample from. Subtract one from
the visible size when calculating the tex stretch x and y scale.

BUG=575587
TEST=Will test nyan next Monday.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[wuchengli, 2016-01-30 00:52:10]

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

[wuchengli, 2016-01-30 00:52:10]

PTAL

[Pawel Osciak, 2016-02-01 00:40:58]

posciak@chromium.org changed reviewers:
+ posciak@chromium.org

[Pawel Osciak, 2016-02-01 00:40:58]

https://codereview.chromium.org/1645353003/diff/1/cc/layers/video_layer_impl.cc
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/1645353003/diff/1/cc/layers/video_layer_impl....
cc/layers/video_layer_impl.cc:189: static_cast<float>(visible_rect.width() - 1)
/ coded_size.width();
I'm wondering if we should be checking for 0,0 (and perhaps 1,1) visible rect
before subtracting?

[wuchengli, 2016-02-01 03:11:24]

Description was changed from

==========
VideoLayerImpl: subtract 1 from visible size when calculating tex stretch.

The video texture only has "visible rect" valid and we're sampling
outside of that visible rect and into garbage texture space due to
bilinear filtering. The fix is to stretch the video such that it will
always have an extra set of texels to sample from. Subtract one from
the visible size when calculating the tex stretch x and y scale.

BUG=575587
TEST=Will test nyan next Monday.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
VideoLayerImpl: subtract 1 from visible size when calculating tex stretch.

The video texture only has "visible rect" valid and we're sampling
outside of that visible rect and into garbage texture space due to
bilinear filtering. The fix is to stretch the video such that it will
always have an extra set of texels to sample from. Subtract one from
the visible size when calculating the tex stretch x and y scale.

BUG=575587
TEST=Test youtube.com/watch?v=CoOFjXk4xLY on nyan-big device.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[wuchengli, 2016-02-01 03:12:23]

On 2016/02/01 00:40:58, Pawel Osciak wrote:
>
https://codereview.chromium.org/1645353003/diff/1/cc/layers/video_layer_impl.cc
> File cc/layers/video_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/1645353003/diff/1/cc/layers/video_layer_impl....
> cc/layers/video_layer_impl.cc:189: static_cast<float>(visible_rect.width() -
1)
> / coded_size.width();
> I'm wondering if we should be checking for 0,0 (and perhaps 1,1) visible rect
> before subtracting?
Good point. Added the checking.

[wuchengli, 2016-02-01 03:12:34]

Tested on nyan-big and it worked. PTAL.

[wuchengli, 2016-02-01 03:14:56]

The CQ bit was checked by wuchengli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-01 03:15:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1645353003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1645353003/20001

[wuchengli, 2016-02-01 03:29:52]

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

https://codereview.chromium.org/1645353003/diff/20001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:192: visible_width -= 1.f;
Do we want this for RGB_RESOURCE only or for all frame resource type?

[commit-bot: I haz the power, 2016-02-01 04:22:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-01 04:22:53]

Dry run: This issue passed the CQ dry run.

[wuchengli, 2016-02-01 08:22:09]

Description was changed from

==========
VideoLayerImpl: subtract 1 from visible size when calculating tex stretch.

The video texture only has "visible rect" valid and we're sampling
outside of that visible rect and into garbage texture space due to
bilinear filtering. The fix is to stretch the video such that it will
always have an extra set of texels to sample from. Subtract one from
the visible size when calculating the tex stretch x and y scale.

BUG=575587
TEST=Test youtube.com/watch?v=CoOFjXk4xLY on nyan-big device.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
VideoLayerImpl: subtract 1 from visible size when calculating tex stretch.

The video texture only has "visible rect" valid and we're sampling
outside of that visible rect and into garbage texture space due to
bilinear filtering. The fix is to stretch the video such that it will
always have an extra set of texels to sample from. Subtract one from
the visible size when calculating the tex stretch x and y scale.

BUG=575587
TEST=Test youtube.com/watch?v=CoOFjXk4xLY on nyan_big and veyron_jerry.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[wuchengli, 2016-02-01 08:22:33]

I tested this CL on veyron_jerry, which used STREAM_TEXTURE_RESOURCE.

[enne (OOO), 2016-02-01 21:00:03]

Can you write a renderer pixel test, with a video frame that is green inside the
visible rect and red outside, and make sure that the final output is all green?

There are some examples of YUV video tests in renderer_pixeltest.cc, and there's
already a green.png output, if that's helpful.

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

https://codereview.chromium.org/1645353003/diff/20001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:188: float visible_width =
static_cast<float>(visible_rect.width());
Please leave these as ints and put the static cast down on tex_width_scale.

https://codereview.chromium.org/1645353003/diff/20001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:192: visible_width -= 1.f;
On 2016/02/01 at 03:29:51, wuchengli wrote:
> Do we want this for RGB_RESOURCE only or for all frame resource type?

If you do this here, it will be for all formats but YUV (just because of how the
code works).  You might want to fix that as well?

If it's the case that textures outside of the visible rect are garbage, then it
probably should be scaled for all formats in this case.

[wuchengli, 2016-02-02 05:37:27]

PTAL. I discussed with enne@ yesterday. I'll try if I can write a test today. If
I cannot figure it out, Adrienne can help write a test. It seems I need to add a
VideoGLRendererPixelTest.

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

https://codereview.chromium.org/1645353003/diff/20001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:188: float visible_width =
static_cast<float>(visible_rect.width());
On 2016/02/01 21:00:03, enne wrote:
> Please leave these as ints and put the static cast down on tex_width_scale.

Done.

https://codereview.chromium.org/1645353003/diff/20001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:192: visible_width -= 1.f;
On 2016/02/01 21:00:03, enne wrote:
> On 2016/02/01 at 03:29:51, wuchengli wrote:
> > Do we want this for RGB_RESOURCE only or for all frame resource type?
> 
> If you do this here, it will be for all formats but YUV (just because of how
the
> code works).  You might want to fix that as well?
> 
> If it's the case that textures outside of the visible rect are garbage, then
it
> probably should be scaled for all formats in this case.

Done.

[wuchengli, 2016-02-02 08:54:50]

By the way, since the issue only happens when scaling up, not scaling down. Do
we want to subtract one only when scaling up? If yes, how do I do it? Check if
|visible_rect| is smaller than |visible_quad_rect|? Or for simplicity, we always
subtract one no matter it's scaling up or down?

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

https://codereview.chromium.org/1645353003/diff/60001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:330: visible_quad_rect, visible_rect.size(),
|visible_rect| is also used here. Is it correct?

[wuchengli, 2016-02-02 14:42:10]

I didn't have enough time to write the test today. I'll try writing it tomorrow.
Please review video_layer_impl.cc first and see my questions.

[wuchengli, 2016-02-03 15:38:27]

PTAL.

I looked at renderer_pixeltest.cc. I think the tests don't call into
VideoLayerImpl::AppendQuads at all. The role of
CreateTestYUVVideoDrawQuad_FromVideoFrame is similar to case YUV_RESOURCE in
VideoLayerImpl::AppendQuads, which is to create a YUVVideoDrawQuad. If I
understand correctly, the test needs a big change to test
VideoLayerImpl::AppendQuads (e.g. create a VideoLayerImpl). Adrienne. Can you
review this CL and help write a test in a separate CL?

[enne (OOO), 2016-02-03 22:17:52]

lgtm

[wuchengli, 2016-02-04 05:32:36]

The CQ bit was checked by wuchengli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-04 05:32:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1645353003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1645353003/80001

[wuchengli, 2016-02-04 07:21:08]

Description was changed from

==========
VideoLayerImpl: subtract 1 from visible size when calculating tex stretch.

The video texture only has "visible rect" valid and we're sampling
outside of that visible rect and into garbage texture space due to
bilinear filtering. The fix is to stretch the video such that it will
always have an extra set of texels to sample from. Subtract one from
the visible size when calculating the tex stretch x and y scale.

BUG=575587
TEST=Test youtube.com/watch?v=CoOFjXk4xLY on nyan_big and veyron_jerry.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
VideoLayerImpl: subtract 1 from visible size when calculating tex stretch.

The video texture only has "visible rect" valid and we're sampling
outside of that visible rect and into garbage texture space due to
bilinear filtering. The fix is to stretch the video such that it will
always have an extra set of texels to sample from. Subtract one from
the visible size when calculating the tex stretch x and y scale.

BUG=575587
TEST=Test youtube.com/watch?v=CoOFjXk4xLY on nyan_big and peach_pi.
Run cc_unittests.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[wuchengli, 2016-02-04 07:45:47]

The CQ bit was unchecked by wuchengli@chromium.org

[wuchengli, 2016-02-04 07:46:00]

The CQ bit was checked by wuchengli@chromium.org

[commit-bot: I haz the power, 2016-02-04 07:46:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1645353003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1645353003/80001

[commit-bot: I haz the power, 2016-02-04 08:12:25]

Description was changed from

==========
VideoLayerImpl: subtract 1 from visible size when calculating tex stretch.

The video texture only has "visible rect" valid and we're sampling
outside of that visible rect and into garbage texture space due to
bilinear filtering. The fix is to stretch the video such that it will
always have an extra set of texels to sample from. Subtract one from
the visible size when calculating the tex stretch x and y scale.

BUG=575587
TEST=Test youtube.com/watch?v=CoOFjXk4xLY on nyan_big and peach_pi.
Run cc_unittests.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
VideoLayerImpl: subtract 1 from visible size when calculating tex stretch.

The video texture only has "visible rect" valid and we're sampling
outside of that visible rect and into garbage texture space due to
bilinear filtering. The fix is to stretch the video such that it will
always have an extra set of texels to sample from. Subtract one from
the visible size when calculating the tex stretch x and y scale.

BUG=575587
TEST=Test youtube.com/watch?v=CoOFjXk4xLY on nyan_big and peach_pi.
Run cc_unittests.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-02-04 08:12:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-04 08:13:43]

Description was changed from

==========
VideoLayerImpl: subtract 1 from visible size when calculating tex stretch.

The video texture only has "visible rect" valid and we're sampling
outside of that visible rect and into garbage texture space due to
bilinear filtering. The fix is to stretch the video such that it will
always have an extra set of texels to sample from. Subtract one from
the visible size when calculating the tex stretch x and y scale.

BUG=575587
TEST=Test youtube.com/watch?v=CoOFjXk4xLY on nyan_big and peach_pi.
Run cc_unittests.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
VideoLayerImpl: subtract 1 from visible size when calculating tex stretch.

The video texture only has "visible rect" valid and we're sampling
outside of that visible rect and into garbage texture space due to
bilinear filtering. The fix is to stretch the video such that it will
always have an extra set of texels to sample from. Subtract one from
the visible size when calculating the tex stretch x and y scale.

BUG=575587
TEST=Test youtube.com/watch?v=CoOFjXk4xLY on nyan_big and peach_pi.
Run cc_unittests.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/64647caadd62fc639f6a3bb194557d8447f6d419
Cr-Commit-Position: refs/heads/master@{#373478}
==========

[commit-bot: I haz the power, 2016-02-04 08:13:44]

Patchset 5 (id:??) landed as
https://crrev.com/64647caadd62fc639f6a3bb194557d8447f6d419
Cr-Commit-Position: refs/heads/master@{#373478}