Enable sampling from DXGI NV12 formats in GLRenderer.

Enable sampling from DXGI NV12 formats in GLRenderer.

NV12 texture formats are treated as two texture objects. The first
texture object has the Y data, and the second has the U data in the R
channel and the V data in the G channel. So we can treat them as similar
to I420 formats, except sampled slightly differently.

BUG=574292
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/cc97f19bb33acd5768246c049c61002d0b2168d5
Cr-Commit-Position: refs/heads/master@{#387170}

[jbauman, 2016-04-01 23:35:18]

Description was changed from

==========
Enable sampling from DXGI NV12 formats in GLRenderer.

NV12 texture formats are treated as two texture objects. The first
texture object has the Y data, and the second has the U data in the R
channel and the V data in the G channel. So we can treat them as similar
to I420 formats, except sampled slightly differently.


BUG=574292
==========

to

==========
Enable sampling from DXGI NV12 formats in GLRenderer.

NV12 texture formats are treated as two texture objects. The first
texture object has the Y data, and the second has the U data in the R
channel and the V data in the G channel. So we can treat them as similar
to I420 formats, except sampled slightly differently.


BUG=574292
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jbauman, 2016-04-06 23:09:27]

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

[danakj, 2016-04-11 23:28:28]

https://codereview.chromium.org/1852923002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1852923002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1971: return;
do you mean dcheck?

https://codereview.chromium.org/1852923002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1978: scoped_ptr<ResourceProvider::ScopedSamplerGL>
v_plane_lock;
TODO to use base::Optional, no need for mallocs here.

https://codereview.chromium.org/1852923002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:3438: const GLRenderer::VideoNV12Program*
GLRenderer::GetVideoNV12Program(
This program should be tested in our unit tests that cover every shader right?

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

https://codereview.chromium.org/1852923002/diff/20001/cc/output/shader.cc#new...
cc/output/shader.cc:2137: uniform float alpha;
wait why is there alpha? and if we did use it we set it as location 4 in the
gl_renderer.

[jbauman, 2016-04-13 00:15:35]

PTAL.

On 2016/04/11 23:28:28, danakj wrote:
> https://codereview.chromium.org/1852923002/diff/20001/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/1852923002/diff/20001/cc/output/gl_renderer.c...
> cc/output/gl_renderer.cc:1971: return;
> do you mean dcheck?

Yeah, was a bit worried about this being maliciously sent by a renderer, but
looking at it more it seems fine.
> 
>
https://codereview.chromium.org/1852923002/diff/20001/cc/output/gl_renderer.c...
> cc/output/gl_renderer.cc:1978: scoped_ptr<ResourceProvider::ScopedSamplerGL>
> v_plane_lock;
> TODO to use base::Optional, no need for mallocs here.
> 
Done.

>
https://codereview.chromium.org/1852923002/diff/20001/cc/output/gl_renderer.c...
> cc/output/gl_renderer.cc:3438: const GLRenderer::VideoNV12Program*
> GLRenderer::GetVideoNV12Program(
> This program should be tested in our unit tests that cover every shader right?
> 
Done. Also had to add in code to VideoResourceUpdater to handle uploading NV12
for testing.

> https://codereview.chromium.org/1852923002/diff/20001/cc/output/shader.cc
> File cc/output/shader.cc (right):
> 
>
https://codereview.chromium.org/1852923002/diff/20001/cc/output/shader.cc#new...
> cc/output/shader.cc:2137: uniform float alpha;
> wait why is there alpha? and if we did use it we set it as location 4 in the
> gl_renderer.

"alpha" is unrelated to the alpha texture plane, but instead is common to every
shader and is used to implement per-quad opacity.

[danakj, 2016-04-13 00:24:19]

https://codereview.chromium.org/1852923002/diff/100001/cc/resources/video_res...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/1852923002/diff/100001/cc/resources/video_res...
cc/resources/video_resource_updater.cc:350: if ((input_frame_format ==
media::PIXEL_FORMAT_NV12) && (i == 1)) {
If all of this is for tests I'd prefer to just not support NV12 here at all. Is
that the case?

I was thinking of these tests:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...

Are the pixel tests testing more production code than that would?

[jbauman, 2016-04-13 00:25:45]

On 2016/04/13 00:24:19, danakj wrote:
>
https://codereview.chromium.org/1852923002/diff/100001/cc/resources/video_res...
> File cc/resources/video_resource_updater.cc (right):
> 
>
https://codereview.chromium.org/1852923002/diff/100001/cc/resources/video_res...
> cc/resources/video_resource_updater.cc:350: if ((input_frame_format ==
> media::PIXEL_FORMAT_NV12) && (i == 1)) {
> If all of this is for tests I'd prefer to just not support NV12 here at all.
Is
> that the case?

Yeah, that's just for tests.
> 
> I was thinking of these tests:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...
> 
> Are the pixel tests testing more production code than that would?

Yeah, the code in gl_renderer_unittest tests that the shader compiles, but not
that it works.

[danakj, 2016-04-13 00:32:04]

On Tue, Apr 12, 2016 at 5:25 PM, <jbauman@chromium.org> wrote:

> On 2016/04/13 00:24:19, danakj wrote:
> >
>
>
https://codereview.chromium.org/1852923002/diff/100001/cc/resources/video_res...
> > File cc/resources/video_resource_updater.cc (right):
> >
> >
>
>
https://codereview.chromium.org/1852923002/diff/100001/cc/resources/video_res...
> > cc/resources/video_resource_updater.cc:350: if ((input_frame_format ==
> > media::PIXEL_FORMAT_NV12) && (i == 1)) {
> > If all of this is for tests I'd prefer to just not support NV12 here at
> all.
> Is
> > that the case?
>
> Yeah, that's just for tests.
>

Is it possible to test with a hardware frame instead of a software one?


>
> >
> > I was thinking of these tests:
> >
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...
> >
> > Are the pixel tests testing more production code than that would?
>
> Yeah, the code in gl_renderer_unittest tests that the shader compiles, but
> not
> that it works.
>
> https://codereview.chromium.org/1852923002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jbauman, 2016-04-13 22:25:05]

On 2016/04/13 00:32:04, danakj wrote:
> On Tue, Apr 12, 2016 at 5:25 PM, <mailto:jbauman@chromium.org> wrote:
> 
> > On 2016/04/13 00:24:19, danakj wrote:
> > >
> >
> >
>
https://codereview.chromium.org/1852923002/diff/100001/cc/resources/video_res...
> > > File cc/resources/video_resource_updater.cc (right):
> > >
> > >
> >
> >
>
https://codereview.chromium.org/1852923002/diff/100001/cc/resources/video_res...
> > > cc/resources/video_resource_updater.cc:350: if ((input_frame_format ==
> > > media::PIXEL_FORMAT_NV12) && (i == 1)) {
> > > If all of this is for tests I'd prefer to just not support NV12 here at
> > all.
> > Is
> > > that the case?
> >
> > Yeah, that's just for tests.
> >
> 
> Is it possible to test with a hardware frame instead of a software one?

Sort of. I've modified the test to create the textures itself, which at least
localizes the testing code.
> 
> 
> >
> > >
> > > I was thinking of these tests:
> > >
> >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...
> > >
> > > Are the pixel tests testing more production code than that would?
> >
> > Yeah, the code in gl_renderer_unittest tests that the shader compiles, but
> > not
> > that it works.
> >
> > https://codereview.chromium.org/1852923002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-04-13 22:34:10]

Thanks! LGTM

[jbauman, 2016-04-13 23:48:36]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2016-04-13 23:49:27]

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

[commit-bot: I haz the power, 2016-04-14 00:33:06]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-14 00:34:44]

Description was changed from

==========
Enable sampling from DXGI NV12 formats in GLRenderer.

NV12 texture formats are treated as two texture objects. The first
texture object has the Y data, and the second has the U data in the R
channel and the V data in the G channel. So we can treat them as similar
to I420 formats, except sampled slightly differently.


BUG=574292
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Enable sampling from DXGI NV12 formats in GLRenderer.

NV12 texture formats are treated as two texture objects. The first
texture object has the Y data, and the second has the U data in the R
channel and the V data in the G channel. So we can treat them as similar
to I420 formats, except sampled slightly differently.

BUG=574292
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/cc97f19bb33acd5768246c049c61002d0b2168d5
Cr-Commit-Position: refs/heads/master@{#387170}
==========

[commit-bot: I haz the power, 2016-04-14 00:34:46]

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