Attach YUV420 to RGB converter to GLContext

Attach YUV420 to RGB converter to GLContext

This is less hacky than the global lookup table.

Leave the IOSurface-specific parts of the conversion inside
GLImageIOSurface. If this converter is needed on other platforms, this
will make it easier to reuse.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

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

[ccameron, 2016-04-14 20:28:46]

Description was changed from

==========
Attach YUV420 to RGB converter to GLContext

This is less hacky than the global lookup table.

Leave the IOSurface-specific parts of the conversion inside
GLImageIOSurface. If this converter is needed on other platforms, this
will make it easier to reuse.

BUG=
==========

to

==========
Attach YUV420 to RGB converter to GLContext

This is less hacky than the global lookup table.

Leave the IOSurface-specific parts of the conversion inside
GLImageIOSurface. If this converter is needed on other platforms, this
will make it easier to reuse.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ccameron, 2016-04-14 20:32:36]

ccameron@chromium.org changed reviewers:
+ bajones@chromium.org, dcastagna@chromium.org

[ccameron, 2016-04-14 20:32:36]

This is a refactor that I promised. Feels much less hacky.

dcastagna: ptal at GLImage parts
bajones: stamp gpu/ bits

[bajones, 2016-04-15 16:56:26]

gpu/ duly stamped. LGTM

[Daniele Castagna, 2016-04-16 02:45:59]

Thank you for doing this.
This is going to be useful for implementing other converters (i.e: CrOS UYVY to
RGB).

https://codereview.chromium.org/1882953006/diff/1/ui/gl/gl_image_io_surface.mm
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1882953006/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:263: if (!gl_context)
Is this possible? Shouldn't we just DCHECK here?

https://codereview.chromium.org/1882953006/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:268: if (!yuv420_rgb_converter)
Same here, if we're in GLImageIOSurface the GLContext should return a converter.
I think it's better to DCHECK(yuv420...) instead of failing to do copyteximage.

https://codereview.chromium.org/1882953006/diff/1/ui/gl/yuv420_rgb_converter.cc
File ui/gl/yuv420_rgb_converter.cc (right):

https://codereview.chromium.org/1882953006/diff/1/ui/gl/yuv420_rgb_converter....
ui/gl/yuv420_rgb_converter.cc:130: GLint target_texture = 0;
Should this be passed as a parameter instead?

https://codereview.chromium.org/1882953006/diff/1/ui/gl/yuv420_rgb_converter.h
File ui/gl/yuv420_rgb_converter.h (right):

https://codereview.chromium.org/1882953006/diff/1/ui/gl/yuv420_rgb_converter....
ui/gl/yuv420_rgb_converter.h:10: namespace gfx {
Shouldn't this be gl?

https://codereview.chromium.org/1882953006/diff/1/ui/gl/yuv420_rgb_converter....
ui/gl/yuv420_rgb_converter.h:12: class YUV420RGBConverter {
I'm not sure about the name.
YUV420 doesn't identify a specific YUV format (I guess you could say both NV12
and I420 are YUV420). Maybe we don't really want to specify one though.
For CrOS we'll have to implement something similar to this converter for UYVY.

We could just call this class YUVToRBGConverter and have different CopyTexImage
methods, one for each source format.

Conversely, in case we want to have one class for each format, we should
probably be more specific about the YUV format in the name of the class.

[ccameron, 2016-04-16 16:19:19]

Updated.

I've left this as being a virtual method for retrieving the YUVToRGBConverter so
that virtual contexts can share the underlying object. I've also only included
it in the CGL context, because the specifics of how it should be shut down may
vary from platform to platform (on Mac, we can make the context current --
different platforms may behave differently).

https://codereview.chromium.org/1882953006/diff/1/ui/gl/gl_image_io_surface.mm
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1882953006/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:263: if (!gl_context)
On 2016/04/16 02:45:59, Daniele Castagna wrote:
> Is this possible? Shouldn't we just DCHECK here?

Changed to DCHECK.

https://codereview.chromium.org/1882953006/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:268: if (!yuv420_rgb_converter)
On 2016/04/16 02:45:59, Daniele Castagna wrote:
> Same here, if we're in GLImageIOSurface the GLContext should return a
converter.
> I think it's better to DCHECK(yuv420...) instead of failing to do
copyteximage.

Similarly changed to DCHECK.

https://codereview.chromium.org/1882953006/diff/1/ui/gl/yuv420_rgb_converter.cc
File ui/gl/yuv420_rgb_converter.cc (right):

https://codereview.chromium.org/1882953006/diff/1/ui/gl/yuv420_rgb_converter....
ui/gl/yuv420_rgb_converter.cc:130: GLint target_texture = 0;
On 2016/04/16 02:45:59, Daniele Castagna wrote:
> Should this be passed as a parameter instead?

Good idea -- updated.

https://codereview.chromium.org/1882953006/diff/1/ui/gl/yuv420_rgb_converter.h
File ui/gl/yuv420_rgb_converter.h (right):

https://codereview.chromium.org/1882953006/diff/1/ui/gl/yuv420_rgb_converter....
ui/gl/yuv420_rgb_converter.h:10: namespace gfx {
On 2016/04/16 02:45:59, Daniele Castagna wrote:
> Shouldn't this be gl?

Done.

https://codereview.chromium.org/1882953006/diff/1/ui/gl/yuv420_rgb_converter....
ui/gl/yuv420_rgb_converter.h:12: class YUV420RGBConverter {
On 2016/04/16 02:45:59, Daniele Castagna wrote:
> I'm not sure about the name.
> YUV420 doesn't identify a specific YUV format (I guess you could say both NV12
> and I420 are YUV420). Maybe we don't really want to specify one though.
> For CrOS we'll have to implement something similar to this converter for UYVY.
> 
> We could just call this class YUVToRBGConverter and have different
CopyTexImage
> methods, one for each source format.
> 
> Conversely, in case we want to have one class for each format, we should
> probably be more specific about the YUV format in the name of the class.

I changed the structure from YUV420RGBConverter to YUVToRGBConverter, and then
added a specific method for 420 biplanar to RGB.

[Daniele Castagna, 2016-04-17 07:56:04]

LGTM

https://codereview.chromium.org/1882953006/diff/40001/ui/gl/yuv_to_rgb_conver...
File ui/gl/yuv_to_rgb_converter.h (right):

https://codereview.chromium.org/1882953006/diff/40001/ui/gl/yuv_to_rgb_conver...
ui/gl/yuv_to_rgb_converter.h:16: void CopyYUV420ToRGB(unsigned target,
nit: the style guide suggests to have input paramenters followed by output
parameters. While |rgb_texture| is not really an output parameter, semantically
it is, maybe it should be the last parameter? I don't have any problem if you
want to leave it as it is.

[ccameron, 2016-04-18 18:17:40]

Thanks!

https://codereview.chromium.org/1882953006/diff/40001/ui/gl/yuv_to_rgb_conver...
File ui/gl/yuv_to_rgb_converter.h (right):

https://codereview.chromium.org/1882953006/diff/40001/ui/gl/yuv_to_rgb_conver...
ui/gl/yuv_to_rgb_converter.h:16: void CopyYUV420ToRGB(unsigned target,
On 2016/04/17 07:56:03, Daniele Castagna wrote:
> nit: the style guide suggests to have input paramenters followed by output
> parameters. While |rgb_texture| is not really an output parameter,
semantically
> it is, maybe it should be the last parameter? I don't have any problem if you
> want to leave it as it is.

Good point --done.

[ccameron, 2016-04-18 18:17:45]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-04-18 18:17:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, dcastagna@chromium.org
Link to the patchset: https://codereview.chromium.org/1882953006/#ps60001
(title: "Move output arg")

[commit-bot: I haz the power, 2016-04-18 18:18:07]

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

[commit-bot: I haz the power, 2016-04-18 19:36:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-18 19:38:08]

Description was changed from

==========
Attach YUV420 to RGB converter to GLContext

This is less hacky than the global lookup table.

Leave the IOSurface-specific parts of the conversion inside
GLImageIOSurface. If this converter is needed on other platforms, this
will make it easier to reuse.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Attach YUV420 to RGB converter to GLContext

This is less hacky than the global lookup table.

Leave the IOSurface-specific parts of the conversion inside
GLImageIOSurface. If this converter is needed on other platforms, this
will make it easier to reuse.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

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

[commit-bot: I haz the power, 2016-04-18 19:38:09]

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