gpu: Add YCbCr 420v extension.

gpu: Add YCbCr 420v extension.

On Mac we currently support textures backed by GpuMemoryBuffer
with UYVY_422 format when GL_APPLE_ycbcr_422 is available.
This allows both to use the backing IOSurface as CALayers and to
bind the IOSurface to a GL texture for sampling.

420v IOSurfaces, while being more efficient when used as a CALayer,
don't support being sampled from GL.

This CL adds a CHROMIUM extension, that when available, allows users
of CHROMIUM command buffer to use a 420v IOSurface from GL as an RGB
texture.

This is achieved converting the IOSurface from YUV to RGB when it's
bound to a texture on the service side.

This will allow to use 420v for video playback using VideoLayerImpl
(promoted to CALayer) and from SkCanvasVideoRenderer when needed (WebGL,
Canvas & co.)

BUG=524582
Committed: https://crrev.com/f0b514cca449dee0c89b3c3477ee567e9bb110f1
Cr-Commit-Position: refs/heads/master@{#363148}

[Daniele Castagna, 2015-10-27 01:11:27]

Description was changed from

==========
WIP: Add YCbCr 420v extension.

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

to

==========
gpu: Add YCbCr 420v extension.

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

[Daniele Castagna, 2015-10-27 01:23:48]

Description was changed from

==========
gpu: Add YCbCr 420v extension.

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

to

==========
gpu: Add YCbCr 420v extension.

On Mac we currently support textures backed by GpuMemoryBuffer
with UYVY_422 format when GL_APPLE_ycbcr_422 is available.
This allows both to use the backing IOSurface as CALayers and to
bind the IOSurface to a GL texture for sampling.

420v IOSurfaces, while being more efficient when used as a CALayer,
don't support being sampled from GL.

This CL adds a CHROMIUM extension, that when available, allows users
of CHROMIUM command buffer to use a 420v IOSurface from GL as an RGBA
texture.

This is achieved converting the IOSurface from YUV to RGBA when it's
bound to a texture on the service side.

This will allow to use 420v for video playback using VideoLayerImpl
(promoted to CALayer) and from SkCanvasVideoRenderer when needed (WebGL,
Canvas & co.) 

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

[Daniele Castagna, 2015-10-27 01:26:08]

dcastagna@chromium.org changed reviewers:
+ reveman@chromium.org

[reveman, 2015-10-27 19:31:55]

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_helper.cc
File ui/gl/gl_helper.cc (right):

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_helper.cc#newc...
ui/gl/gl_helper.cc:7: #include <fstream>
fstream? what is this for?

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_helper.cc#newc...
ui/gl/gl_helper.cc:10: #include "base/memory/scoped_ptr.h"
why this include? not seeing any use of scoped_ptr here

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_helper.cc#newc...
ui/gl/gl_helper.cc:11: #include "ui/gfx/geometry/size.h"
this doesn't seem to be used either but it looks like a base/logging.h include
is missing

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_helper.h
File ui/gl/gl_helper.h (right):

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_helper.h#newcode6
ui/gl/gl_helper.h:6: #define UI_GL_TEST_GL_HELPER_H_
UI_GL_GL_HELPER_H_

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_helper.h#newco...
ui/gl/gl_helper.h:34: #endif  // UI_GL_TEST_GL_HELPER_H_
UI_GL_GL_HELPER_H_

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_image_io_surfa...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.mm:338: i420v_to_RGBA_->Destroy();
without looking at what this does, it's confusing that we would only destroy an
object if "have_context" is true. I think this would be better if there was no
i420v_to_RGBA_ instance. See my comment below for more on this.

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.mm:384: if (!i420v_to_RGBA_) {
I would prefer to see all this state just part of the GLImageIOSurface instead
and all the code right here in this function with helper functions in anonymous
namespace. This way we don't have to deal with a "initialize GL structures..."
failing and there's no need for scoped bind textures here and then again in
DrawToTexture..

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.mm:393: DCHECK(target == GL_TEXTURE_2D);
What prevents a client from using a different target and that because of this
causing the GPU process to crash?

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_image_io_surfa...
File ui/gl/gl_image_io_surface_unittest.cc (right):

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface_unittest.cc:19: IOSurfaceRef CreateIOSurface(const
gfx::Size& size) {
hm, can we move the IOSurfaceManager interface and the single process impl of
that into ui/gl so we don't have to duplicate this code here?

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface_unittest.cc:54: void SetBuffersDataToColor(const
uint8_t color[4],
Can this be part of GLImageTestSupport::SetBufferDataToColor so it can be reused
by other implementations?

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/scoped_binders.cc
File ui/gl/scoped_binders.cc (right):

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/scoped_binders.cc...
ui/gl/scoped_binders.cc:54: NOTIMPLEMENTED() << " Target not part of OpenGL ES
2.0 spec.";
why the space here? also I'm not sure this is accurate anymore. RECTANGLE is not
part of the ES 2.0 spec. we should update the comment so it's not confusing.

[Daniele Castagna, 2015-10-29 20:09:05]

PTAL.

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_helper.cc
File ui/gl/gl_helper.cc (right):

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_helper.cc#newc...
ui/gl/gl_helper.cc:7: #include <fstream>
On 2015/10/27 at 19:31:54, reveman wrote:
> fstream? what is this for?

Leftover since I had a function to save the output to png. Removed.

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_helper.cc#newc...
ui/gl/gl_helper.cc:10: #include "base/memory/scoped_ptr.h"
On 2015/10/27 at 19:31:54, reveman wrote:
> why this include? not seeing any use of scoped_ptr here

Leftover.

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_helper.cc#newc...
ui/gl/gl_helper.cc:11: #include "ui/gfx/geometry/size.h"
On 2015/10/27 at 19:31:54, reveman wrote:
> this doesn't seem to be used either but it looks like a base/logging.h include
is missing

Removed size.h and added base/logging.h

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_helper.h
File ui/gl/gl_helper.h (right):

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_helper.h#newcode6
ui/gl/gl_helper.h:6: #define UI_GL_TEST_GL_HELPER_H_
On 2015/10/27 at 19:31:54, reveman wrote:
> UI_GL_GL_HELPER_H_

Done.

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_helper.h#newco...
ui/gl/gl_helper.h:34: #endif  // UI_GL_TEST_GL_HELPER_H_
On 2015/10/27 at 19:31:54, reveman wrote:
> UI_GL_GL_HELPER_H_

Done.

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_image_io_surfa...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.mm:338: i420v_to_RGBA_->Destroy();
On 2015/10/27 at 19:31:54, reveman wrote:
> without looking at what this does, it's confusing that we would only destroy
an object if "have_context" is true. I think this would be better if there was
no i420v_to_RGBA_ instance. See my comment below for more on this.

Removed the class as suggested in the comment below.

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.mm:384: if (!i420v_to_RGBA_) {
On 2015/10/27 at 19:31:54, reveman wrote:
> I would prefer to see all this state just part of the GLImageIOSurface instead
and all the code right here in this function with helper functions in anonymous
namespace. This way we don't have to deal with a "initialize GL structures..."
failing and there's no need for scoped bind textures here and then again in
DrawToTexture..

Done.

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.mm:393: DCHECK(target == GL_TEXTURE_2D);
On 2015/10/27 at 19:31:54, reveman wrote:
> What prevents a client from using a different target and that because of this
causing the GPU process to crash?

Changed that to LOG(ERROR) ... + return false.

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_image_io_surfa...
File ui/gl/gl_image_io_surface_unittest.cc (right):

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface_unittest.cc:19: IOSurfaceRef CreateIOSurface(const
gfx::Size& size) {
On 2015/10/27 at 19:31:54, reveman wrote:
> hm, can we move the IOSurfaceManager interface and the single process impl of
that into ui/gl so we don't have to duplicate this code here?

This function is similar to GpuMemoryBufferFactoryIOSurface::CreateIOSurface.
Did you mean to move CreateIOSurface to IOSurfaceManager?

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface_unittest.cc:54: void SetBuffersDataToColor(const
uint8_t color[4],
On 2015/10/27 at 19:31:54, reveman wrote:
> Can this be part of GLImageTestSupport::SetBufferDataToColor so it can be
reused by other implementations?

Done.

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/scoped_binders.cc
File ui/gl/scoped_binders.cc (right):

https://codereview.chromium.org/1419733005/diff/60001/ui/gl/scoped_binders.cc...
ui/gl/scoped_binders.cc:54: NOTIMPLEMENTED() << " Target not part of OpenGL ES
2.0 spec.";
On 2015/10/27 at 19:31:54, reveman wrote:
> why the space here? also I'm not sure this is accurate anymore. RECTANGLE is
not part of the ES 2.0 spec. we should update the comment so it's not confusing.

Fixed the string.
The added space is because NOTIMPLEMENTED prints out "NOT IMPLEMENTED" and
concatenating a string without spaces ended up printing
"[...]IMPLEMENTEDTarget[...]"

[reveman, 2015-10-29 22:08:04]

Mostly nits. Patch looks good in general. I think we just need to fix the
duplication of IOSurface allocation code. The same code already exists in
content/.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.h (right):

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.h:67: // GL state to support 420v IOSurface binding as
RGBA.
nit: s/binding as RGBA/conversion to RGB/

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.h:75: static const char kFragmentShader[];
does these last two need to be here? anonymous namespace or just constants in
the function where they are used instead?

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:155: gfx::ScopedBufferBinder sbb(GL_ARRAY_BUFFER,
vertex_buffer);
nit: s/sbb/scoped_buffer_binder/ or buffer_binder, or whatever we typically use
for the texture binder equivalent in glimage code.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:231: glDeleteProgram(program_);
what if you haven't created a program?

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:249: if (format_ == BufferFormat::YUV_420_BIPLANAR)
Can you add a comment here to explain that this format needs to be converted to
RGB and as a result, BindTexImage is not supported but CopyTexImage is?

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:313: GLint service_texture = 0;
nit: service_texture -> target_texture as there's technically no such thing as a
"service" from the pov of this code

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:330: glDeleteTextures(2, yuv_textures);
Do we need to restore the active texture here? Maybe add a ScopedActiveTexture?

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:346: gfx::ScopedFrameBufferBinder fb(framebuffer_);
nit: s/fb/scoped_framebuffer/

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:347: glViewport(0, 0, size_.width(),
size_.height());
do we need to restore the viewport before we exit?

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:353: gfx::ScopedUseProgram pr(program_);
nit: please remove "pr" abbreviation here and similar below

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:360: // Detach the service texture from the fbo.
target texture

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:365: glActiveTexture(GL_TEXTURE0);
previous_active_texture?

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/scoped_binders.cc
File ui/gl/scoped_binders.cc (right):

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/scoped_binders.c...
ui/gl/scoped_binders.cc:81: : enabled_(0), index_(index) {
s/0/GL_FALSE/

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/scoped_binders.c...
ui/gl/scoped_binders.cc:87: if (!enabled_) {
if (enabled_ == GL_FALSE)

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_support.h (right):

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_support.h:27: uint8_t* data[]);
Let's pass a plane argument to this function instead as discussed. I think that
will also eliminate the need for the helper function below.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_template.h (right):

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_template.h:93: const uint8_t image_color[] = {0x10,
0x20, 0, 0xff};
nit: can you add a comment here to explain why these values have been chosen.
less confusing that way if someone tries to change them and tests suddenly start
failing..

[Daniele Castagna, 2015-10-30 23:51:57]

On 2015/10/29 at 22:08:04, reveman wrote:
> Mostly nits. Patch looks good in general. I think we just need to fix the
duplication of IOSurface allocation code. The same code already exists in
content/.
> 

Removed code duplication. This patch now depends on crrev.com/1408753003

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.h (right):

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.h:67: // GL state to support 420v IOSurface binding as
RGBA.
On 2015/10/29 at 22:08:04, reveman wrote:
> nit: s/binding as RGBA/conversion to RGB/

Done.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.h:75: static const char kFragmentShader[];
On 2015/10/29 at 22:08:03, reveman wrote:
> does these last two need to be here? anonymous namespace or just constants in
the function where they are used instead?

No, moved in the anonymous namespace in the .mm file.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:155: gfx::ScopedBufferBinder sbb(GL_ARRAY_BUFFER,
vertex_buffer);
On 2015/10/29 at 22:08:04, reveman wrote:
> nit: s/sbb/scoped_buffer_binder/ or buffer_binder, or whatever we typically
use for the texture binder equivalent in glimage code.

Renamed just buffer_binder, also changed the name of the other Scoped* things.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:231: glDeleteProgram(program_);
On 2015/10/29 at 22:08:04, reveman wrote:
> what if you haven't created a program?

In that case program_ would be 0.
The documentation says: "A value of 0 for program will be silently ignored."
Same for all the other calls.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:249: if (format_ == BufferFormat::YUV_420_BIPLANAR)
On 2015/10/29 at 22:08:04, reveman wrote:
> Can you add a comment here to explain that this format needs to be converted
to RGB and as a result, BindTexImage is not supported but CopyTexImage is?

Done.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:313: GLint service_texture = 0;
On 2015/10/29 at 22:08:04, reveman wrote:
> nit: service_texture -> target_texture as there's technically no such thing as
a "service" from the pov of this code

Done.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:330: glDeleteTextures(2, yuv_textures);
On 2015/10/29 at 22:08:04, reveman wrote:
> Do we need to restore the active texture here? Maybe add a
ScopedActiveTexture?

Done.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:346: gfx::ScopedFrameBufferBinder fb(framebuffer_);
On 2015/10/29 at 22:08:04, reveman wrote:
> nit: s/fb/scoped_framebuffer/

Changed all of them!

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:347: glViewport(0, 0, size_.width(),
size_.height());
On 2015/10/29 at 22:08:04, reveman wrote:
> do we need to restore the viewport before we exit?

Added another Scoped* class...

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:353: gfx::ScopedUseProgram pr(program_);
On 2015/10/29 at 22:08:04, reveman wrote:
> nit: please remove "pr" abbreviation here and similar below

Done.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:360: // Detach the service texture from the fbo.
On 2015/10/29 at 22:08:04, reveman wrote:
> target texture

Done.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:365: glActiveTexture(GL_TEXTURE0);
On 2015/10/29 at 22:08:04, reveman wrote:
> previous_active_texture?

ack.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/scoped_binders.cc
File ui/gl/scoped_binders.cc (right):

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/scoped_binders.c...
ui/gl/scoped_binders.cc:81: : enabled_(0), index_(index) {
On 2015/10/29 at 22:08:04, reveman wrote:
> s/0/GL_FALSE/

Done.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/scoped_binders.c...
ui/gl/scoped_binders.cc:87: if (!enabled_) {
On 2015/10/29 at 22:08:04, reveman wrote:
> if (enabled_ == GL_FALSE)

Done.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_support.h (right):

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_support.h:27: uint8_t* data[]);
On 2015/10/29 at 22:08:04, reveman wrote:
> Let's pass a plane argument to this function instead as discussed. I think
that will also eliminate the need for the helper function below.

Done.

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_template.h (right):

https://codereview.chromium.org/1419733005/diff/100001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_template.h:93: const uint8_t image_color[] = {0x10,
0x20, 0, 0xff};
On 2015/10/29 at 22:08:04, reveman wrote:
> nit: can you add a comment here to explain why these values have been chosen.
less confusing that way if someone tries to change them and tests suddenly start
failing..

Done.

[reveman, 2015-11-01 14:19:05]

this looks great, mostly nits

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/BUILD.gn
File ui/gl/BUILD.gn (right):

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/BUILD.gn#newcode351
ui/gl/BUILD.gn:351: }
nit: blankline before include_dirs

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_bindings.h
File ui/gl/gl_bindings.h (right):

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_bindings.h#ne...
ui/gl/gl_bindings.h:139: #define GL_RGB_YCBCR_420V_CHROMIUM 0x78FC
nit: horizontally align 0x78FC with 0x78FB. yes, not what cl format wants..

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_helper.h
File ui/gl/gl_helper.h (right):

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_helper.h#newc...
ui/gl/gl_helper.h:13: class GL_EXPORT GLHelper {
include gl_export.h for this?

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:9: #include "base/callback_helpers.h"
is this used?

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:41: v_texCoord = (a_position + vec2(1,1)) * 0.5 *
a_size;
s/vec2(1,1)/vec2(1, 1)/ maybe vec2(1.0, 1.0)?

also maybe rename a_size to a_texScale to make it more clear what that is used
for and that also makes this shader not explicitly aware of the use of
texture_rectangle.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:56: texture2DRect(a_uv_texture, v_texCoord *
0.5).rg);
nit: Horizontally align texture2DRect to have consistent formatting within this
shader (match alignment of arguments passed to mat3 above)

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:57: gl_FragColor = vec4( yuv_matrix * (yuv +
yuv_adj), 1.0);
nit: s/vec4( yuv_mat.../vec4(yuv_mat.../

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:231: if (have_context) {
Can you add framebuffer_ to this 'if' clause? I know the GL will ignore these
calls when passed 0 but "if (have_context && framebuffer_)" match the condition
for when we create this state and I think that makes it a bit easier to
understand.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:249: // YUV_420_BIPLANAR is not supported by
BindTexImage.
nit: can you add a blinkline after the DCHECK as otherwise is looks like it's
grouped and related to this format check

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:250: // Returning false here will result in
conversion to RGB in CopyTexImage.
nit: maybe something like "CopyTexImage is supported by this format as that
performs conversion to RGB as part of the copy operation." as what returning
false here results in is really up to the user of this API. They can call
CopyTexImage if they like but they don't have to.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:280: if (format_ != BufferFormat::YUV_420_BIPLANAR)
nit: blankline after DCHECKs and maybe move the io_surface_ DCHECK down to where
it's first used..

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:311: GLuint yuv_textures[2];
nit: ..textures[2] = {};

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:313: DCHECK(yuv_textures[0] && yuv_textures[1]);
nit: one DCHECK for each texture is preferred

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:320: glGetIntegerv(GL_ACTIVE_TEXTURE,
&previous_active_texture);
is this still used?

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:354: GL_TEXTURE_2D, target_texture, 0);
Do we need a glTexImage2D call for the target texture before this? there's no
guarantee that the target texture has been defined when CopyTexImage is called.
This function should define it. It's only in CopyTexSubImage that we can assume
that the texture has already been defined.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/scoped_binders.h
File ui/gl/scoped_binders.h (right):

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/scoped_binders.h...
ui/gl/scoped_binders.h:37: int old_texture_;
nit: blankline before DISALLOW_COP..

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/scoped_binders.h...
ui/gl/scoped_binders.h:103: DISALLOW_COPY_AND_ASSIGN(ScopedViewport);
nit: blankline before DISALLOW_COP..

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_support.cc (right):

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_support.cc:93: DCHECK(height % 2 == 0);
nit: DCHECK_EQ

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_support.cc:94: DCHECK(width % 2 == 0);
nit: DCHECK_EQ

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_support.cc:117: return;
nit: move 'return' into the above scope

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_template.h (right):

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_template.h:93: // This values are picked so that RGB ->
YUV on the CPU converted
nit: "These values.."

[Daniele Castagna, 2015-11-01 21:55:31]

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/BUILD.gn
File ui/gl/BUILD.gn (right):

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/BUILD.gn#newcode351
ui/gl/BUILD.gn:351: }
On 2015/11/01 at 14:19:04, reveman wrote:
> nit: blankline before include_dirs

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_bindings.h
File ui/gl/gl_bindings.h (right):

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_bindings.h#ne...
ui/gl/gl_bindings.h:139: #define GL_RGB_YCBCR_420V_CHROMIUM 0x78FC
On 2015/11/01 at 14:19:04, reveman wrote:
> nit: horizontally align 0x78FC with 0x78FB. yes, not what cl format wants..

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_helper.h
File ui/gl/gl_helper.h (right):

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_helper.h#newc...
ui/gl/gl_helper.h:13: class GL_EXPORT GLHelper {
On 2015/11/01 at 14:19:04, reveman wrote:
> include gl_export.h for this?

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:9: #include "base/callback_helpers.h"
On 2015/11/01 at 14:19:04, reveman wrote:
> is this used?

Not anymore. Removed.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:41: v_texCoord = (a_position + vec2(1,1)) * 0.5 *
a_size;
On 2015/11/01 at 14:19:05, reveman wrote:
> s/vec2(1,1)/vec2(1, 1)/ maybe vec2(1.0, 1.0)?
> 
> also maybe rename a_size to a_texScale to make it more clear what that is used
for and that also makes this shader not explicitly aware of the use of
texture_rectangle.

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:56: texture2DRect(a_uv_texture, v_texCoord *
0.5).rg);
On 2015/11/01 at 14:19:04, reveman wrote:
> nit: Horizontally align texture2DRect to have consistent formatting within
this shader (match alignment of arguments passed to mat3 above)

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:57: gl_FragColor = vec4( yuv_matrix * (yuv +
yuv_adj), 1.0);
On 2015/11/01 at 14:19:04, reveman wrote:
> nit: s/vec4( yuv_mat.../vec4(yuv_mat.../

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:231: if (have_context) {
On 2015/11/01 at 14:19:04, reveman wrote:
> Can you add framebuffer_ to this 'if' clause? I know the GL will ignore these
calls when passed 0 but "if (have_context && framebuffer_)" match the condition
for when we create this state and I think that makes it a bit easier to
understand.

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:249: // YUV_420_BIPLANAR is not supported by
BindTexImage.
On 2015/11/01 at 14:19:04, reveman wrote:
> nit: can you add a blinkline after the DCHECK as otherwise is looks like it's
grouped and related to this format check

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:250: // Returning false here will result in
conversion to RGB in CopyTexImage.
On 2015/11/01 at 14:19:04, reveman wrote:
> nit: maybe something like "CopyTexImage is supported by this format as that
performs conversion to RGB as part of the copy operation." as what returning
false here results in is really up to the user of this API. They can call
CopyTexImage if they like but they don't have to.

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:280: if (format_ != BufferFormat::YUV_420_BIPLANAR)
On 2015/11/01 at 14:19:05, reveman wrote:
> nit: blankline after DCHECKs and maybe move the io_surface_ DCHECK down to
where it's first used..

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:311: GLuint yuv_textures[2];
On 2015/11/01 at 14:19:04, reveman wrote:
> nit: ..textures[2] = {};

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:313: DCHECK(yuv_textures[0] && yuv_textures[1]);
On 2015/11/01 at 14:19:04, reveman wrote:
> nit: one DCHECK for each texture is preferred

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:320: glGetIntegerv(GL_ACTIVE_TEXTURE,
&previous_active_texture);
On 2015/11/01 at 14:19:04, reveman wrote:
> is this still used?

Nop. Gone.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:354: GL_TEXTURE_2D, target_texture, 0);
On 2015/11/01 at 14:19:04, reveman wrote:
> Do we need a glTexImage2D call for the target texture before this? there's no
guarantee that the target texture has been defined when CopyTexImage is called.
This function should define it. It's only in CopyTexSubImage that we can assume
that the texture has already been defined.

We do need it. Added it.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/scoped_binders.h
File ui/gl/scoped_binders.h (right):

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/scoped_binders.h...
ui/gl/scoped_binders.h:37: int old_texture_;
On 2015/11/01 at 14:19:05, reveman wrote:
> nit: blankline before DISALLOW_COP..

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/scoped_binders.h...
ui/gl/scoped_binders.h:103: DISALLOW_COPY_AND_ASSIGN(ScopedViewport);
On 2015/11/01 at 14:19:05, reveman wrote:
> nit: blankline before DISALLOW_COP..

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_support.cc (right):

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_support.cc:93: DCHECK(height % 2 == 0);
On 2015/11/01 at 14:19:05, reveman wrote:
> nit: DCHECK_EQ

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_support.cc:94: DCHECK(width % 2 == 0);
On 2015/11/01 at 14:19:05, reveman wrote:
> nit: DCHECK_EQ

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_support.cc:117: return;
On 2015/11/01 at 14:19:05, reveman wrote:
> nit: move 'return' into the above scope

Done.

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/test/gl_image_te...
File ui/gl/test/gl_image_test_template.h (right):

https://codereview.chromium.org/1419733005/diff/120001/ui/gl/test/gl_image_te...
ui/gl/test/gl_image_test_template.h:93: // This values are picked so that RGB ->
YUV on the CPU converted
On 2015/11/01 at 14:19:05, reveman wrote:
> nit: "These values.."

Done.

[reveman, 2015-11-02 01:09:44]

https://codereview.chromium.org/1419733005/diff/140001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1419733005/diff/140001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:316: glGenTextures(2, yuv_textures);
we seem to be leaking these when the copy operation completes successfully.
should we just make these part of the state we initialize above if
!framebuffer_? the CGLTexImageIOSurface2D implementation might be more efficient
when we're re-binding the same IOSurface plane to the same texture..

https://codereview.chromium.org/1419733005/diff/140001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface_unittest.cc (right):

https://codereview.chromium.org/1419733005/diff/140001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface_unittest.cc:47: GLImageIOSurface420vTestDelegate);
nit: we should run this test for every format we support. if you're adding this
test as part of this patch then run it for every format and not just 420v. fine
to not include the test in this patch at all if you prefer.

[Daniele Castagna, 2015-11-02 19:35:52]

https://codereview.chromium.org/1419733005/diff/140001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1419733005/diff/140001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:316: glGenTextures(2, yuv_textures);
On 2015/11/02 at 01:09:44, reveman wrote:
> we seem to be leaking these when the copy operation completes successfully.
should we just make these part of the state we initialize above if
!framebuffer_? the CGLTexImageIOSurface2D implementation might be more efficient
when we're re-binding the same IOSurface plane to the same texture..

Fixed it using ScopedClosureRunner.
I'd prefer to delete the temporary textures. Considering we'll be using this for
videos, and considering that SkCanvasVideoRender sometimes is used by slimming
paint for a few frames, keeping around the two textures would mean to keep
around a significant amount of memory when it's not needed.

Happy to do otherwise if you prefer, or we find out that performance is a
problem.

https://codereview.chromium.org/1419733005/diff/140001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface_unittest.cc (right):

https://codereview.chromium.org/1419733005/diff/140001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface_unittest.cc:47: GLImageIOSurface420vTestDelegate);
On 2015/11/02 at 01:09:44, reveman wrote:
> nit: we should run this test for every format we support. if you're adding
this test as part of this patch then run it for every format and not just 420v.
fine to not include the test in this patch at all if you prefer.

Done.

[reveman, 2015-11-02 21:13:33]

lgtm

https://codereview.chromium.org/1419733005/diff/140001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1419733005/diff/140001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:316: glGenTextures(2, yuv_textures);
On 2015/11/02 at 19:35:51, Daniele Castagna wrote:
> On 2015/11/02 at 01:09:44, reveman wrote:
> > we seem to be leaking these when the copy operation completes successfully.
should we just make these part of the state we initialize above if
!framebuffer_? the CGLTexImageIOSurface2D implementation might be more efficient
when we're re-binding the same IOSurface plane to the same texture..
> 
> Fixed it using ScopedClosureRunner.
> I'd prefer to delete the temporary textures. Considering we'll be using this
for videos, and considering that SkCanvasVideoRender sometimes is used by
slimming paint for a few frames, keeping around the two textures would mean to
keep around a significant amount of memory when it's not needed.

This is not actually adding any more memory usage as we're binding the storage
of the IOSurface to theses textures. 

> 
> Happy to do otherwise if you prefer, or we find out that performance is a
problem.

Either way is fine with me.

[Daniele Castagna, 2015-11-02 22:05:04]

On 2015/11/02 at 21:13:33, reveman wrote:
> lgtm
> 
>
https://codereview.chromium.org/1419733005/diff/140001/ui/gl/gl_image_io_surf...
> File ui/gl/gl_image_io_surface.mm (right):
> 
>
https://codereview.chromium.org/1419733005/diff/140001/ui/gl/gl_image_io_surf...
> ui/gl/gl_image_io_surface.mm:316: glGenTextures(2, yuv_textures);
> On 2015/11/02 at 19:35:51, Daniele Castagna wrote:
> > On 2015/11/02 at 01:09:44, reveman wrote:
> > > we seem to be leaking these when the copy operation completes
successfully. should we just make these part of the state we initialize above if
!framebuffer_? the CGLTexImageIOSurface2D implementation might be more efficient
when we're re-binding the same IOSurface plane to the same texture..
> > 
> > Fixed it using ScopedClosureRunner.
> > I'd prefer to delete the temporary textures. Considering we'll be using this
for videos, and considering that SkCanvasVideoRender sometimes is used by
slimming paint for a few frames, keeping around the two textures would mean to
keep around a significant amount of memory when it's not needed.
> 
> This is not actually adding any more memory usage as we're binding the storage
of the IOSurface to theses textures. 
> 

Right. Changed it as you suggested.

> > 
> > Happy to do otherwise if you prefer, or we find out that performance is a
problem.
> 
> Either way is fine with me.

[Daniele Castagna, 2015-11-02 22:06:48]

Description was changed from

==========
gpu: Add YCbCr 420v extension.

On Mac we currently support textures backed by GpuMemoryBuffer
with UYVY_422 format when GL_APPLE_ycbcr_422 is available.
This allows both to use the backing IOSurface as CALayers and to
bind the IOSurface to a GL texture for sampling.

420v IOSurfaces, while being more efficient when used as a CALayer,
don't support being sampled from GL.

This CL adds a CHROMIUM extension, that when available, allows users
of CHROMIUM command buffer to use a 420v IOSurface from GL as an RGBA
texture.

This is achieved converting the IOSurface from YUV to RGBA when it's
bound to a texture on the service side.

This will allow to use 420v for video playback using VideoLayerImpl
(promoted to CALayer) and from SkCanvasVideoRenderer when needed (WebGL,
Canvas & co.) 

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

to

==========
gpu: Add YCbCr 420v extension.

On Mac we currently support textures backed by GpuMemoryBuffer
with UYVY_422 format when GL_APPLE_ycbcr_422 is available.
This allows both to use the backing IOSurface as CALayers and to
bind the IOSurface to a GL texture for sampling.

420v IOSurfaces, while being more efficient when used as a CALayer,
don't support being sampled from GL.

This CL adds a CHROMIUM extension, that when available, allows users
of CHROMIUM command buffer to use a 420v IOSurface from GL as an RGB
texture.

This is achieved converting the IOSurface from YUV to RGB when it's
bound to a texture on the service side.

This will allow to use 420v for video playback using VideoLayerImpl
(promoted to CALayer) and from SkCanvasVideoRenderer when needed (WebGL,
Canvas & co.) 

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

[Daniele Castagna, 2015-11-02 22:07:29]

dcastagna@chromium.org changed reviewers:
+ piman@chromium.org

[Daniele Castagna, 2015-11-02 22:07:29]

Adding owners:
+piman for everything :)

[reveman, 2015-11-03 00:32:35]

noticed these when applying this locally

https://codereview.chromium.org/1419733005/diff/180001/gpu/GLES2/extensions/C...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_ycbcr_420v_image.txt (right):

https://codereview.chromium.org/1419733005/diff/180001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_ycbcr_420v_image.txt:21: This extension
provides a new internal image format to use when creating an
nit: unnecessary whitespace at end of line

https://codereview.chromium.org/1419733005/diff/180001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_ycbcr_420v_image.txt:22: image from
underlying '420v' buffers.
nit: unnecessary whitespace at end of line

https://codereview.chromium.org/1419733005/diff/180001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_ycbcr_420v_image.txt:25:
CreateGpuMemoryBufferImageCHROMIUM to define the format of GpuMemoryBuffer
nit: unnecessary whitespace at end of line

https://codereview.chromium.org/1419733005/diff/180001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_ycbcr_420v_image.txt:37: 
nit: unnecessary whitespace at end of line

https://codereview.chromium.org/1419733005/diff/180001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_ycbcr_420v_image.txt:38: Accepted by the
<internalformat> parameter of CreateImageCHROMIUM, and
nit: unnecessary whitespace at end of line

[Daniele Castagna, 2015-11-03 00:48:06]

https://codereview.chromium.org/1419733005/diff/180001/gpu/GLES2/extensions/C...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_ycbcr_420v_image.txt (right):

https://codereview.chromium.org/1419733005/diff/180001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_ycbcr_420v_image.txt:21: This extension
provides a new internal image format to use when creating an
On 2015/11/03 at 00:32:34, reveman wrote:
> nit: unnecessary whitespace at end of line

Done.

https://codereview.chromium.org/1419733005/diff/180001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_ycbcr_420v_image.txt:22: image from
underlying '420v' buffers.
On 2015/11/03 at 00:32:34, reveman wrote:
> nit: unnecessary whitespace at end of line

Done.

https://codereview.chromium.org/1419733005/diff/180001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_ycbcr_420v_image.txt:25:
CreateGpuMemoryBufferImageCHROMIUM to define the format of GpuMemoryBuffer
On 2015/11/03 at 00:32:34, reveman wrote:
> nit: unnecessary whitespace at end of line

Done.

https://codereview.chromium.org/1419733005/diff/180001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_ycbcr_420v_image.txt:37: 
On 2015/11/03 at 00:32:34, reveman wrote:
> nit: unnecessary whitespace at end of line

Done.

https://codereview.chromium.org/1419733005/diff/180001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_ycbcr_420v_image.txt:38: Accepted by the
<internalformat> parameter of CreateImageCHROMIUM, and
On 2015/11/03 at 00:32:34, reveman wrote:
> nit: unnecessary whitespace at end of line

Done.

[reveman, 2015-11-03 02:44:34]

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface_unittest.cc (right):

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface_unittest.cc:15: class GLImageIOSurface420vTestDelegate
{
nit: s/GLImageIOSurface420vTestDelegate/GLImageIOSurfaceTestDelegate/

[reveman, 2015-11-03 02:47:19]

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface_unittest.cc (right):

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface_unittest.cc:21: new gl::GLImageIOSurface(size,
GL_RGB_YCBCR_420V_CHROMIUM));
nit: GL_RGB_YCBCR_420V_CHROMIUM is not correct if format is *GBA_8888.

[piman, 2015-11-03 04:12:59]

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:189: glVertexAttribPointer(0, 2, GL_FLOAT,
GL_FALSE, sizeof(GLfloat) * 2, 0);
What resets this state?

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:299: program_ =
gfx::GLHelper::SetupProgram(vertex_shader_, fragment_shader_);
It seems wasteful to compile & link a program for every single GLImage that goes
through this path.

Would there be a way to share this state somehow? E.g. hook it up to the context
maybe?


Part of me is wondering - does this really belong to GLImage? We have some
amount of infrastructure for similar things, that we could share in
CopyTextureCHROMIUMResourceManager...  I wonder if it could make more sense for
the decoder to do the color conversion.

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:353: gfx::ScopedViewport viewport(0, 0,
size_.width(), size_.height());
you probably also need to disable face cull, scissor, blending, color mask, ....

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:355: glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB,
size_.width(), size_.height(), 0,
You're modifying the texture bound to the GL_TEXTURE1 unit, which is not
necessarily target_texture (which was bound to the current unit when we entered
the function, not necessarily, and likely not, GL_TEXTURE1).

[reveman, 2015-11-03 14:59:30]

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:299: program_ =
gfx::GLHelper::SetupProgram(vertex_shader_, fragment_shader_);
On 2015/11/03 at 04:12:59, piman (slow to review) wrote:
> It seems wasteful to compile & link a program for every single GLImage that
goes through this path.
> 
> Would there be a way to share this state somehow? E.g. hook it up to the
context maybe?

We discussed this and agreed that the context would be an appropriate way to
share this state but thought it would be better to do that as a follow up.

> 
> 
> Part of me is wondering - does this really belong to GLImage? We have some
amount of infrastructure for similar things, that we could share in
CopyTextureCHROMIUMResourceManager...  I wonder if it could make more sense for
the decoder to do the color conversion.

Sharing the general color space conversion code would be nice but there's a lot
of IOSurface specific code here that belongs in GLImage and shouldn't be exposed
to the decoder. Moving large parts of the logic in GLImageIOSurface to some
GLBlitter/GLColorSpaceConverter classes that could be part of the GLContext
state sgtm. Maybe CopyTextureCHROMIUMResourceManager could take advantage of
that too.

[Daniele Castagna, 2015-11-04 20:35:20]

Patchset #12 (id:210001) has been deleted

[Daniele Castagna, 2015-11-04 20:41:33]

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:189: glVertexAttribPointer(0, 2, GL_FLOAT,
GL_FALSE, sizeof(GLfloat) * 2, 0);
On 2015/11/03 at 04:12:59, piman (slow to review) wrote:
> What resets this state?

Not only resets, but also what sets it again before the draw. :/

Renamed ScopedEnableVertexAttribArray to ScopedVertexAttribArray and let it
set/restore these parameters before/after the drawarrays.

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:353: gfx::ScopedViewport viewport(0, 0,
size_.width(), size_.height());
On 2015/11/03 at 04:12:59, piman (slow to review) wrote:
> you probably also need to disable face cull, scissor, blending, color mask,
....

Added a ScopedCapabilitiesRestorer and disabled blending, dithering, face
culling, scissor/depth testing.

Also added ScopedColorMask to disable/restore the color mask.

Did I miss something?

Happy to listen to any suggestion to make this code better.

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:355: glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB,
size_.width(), size_.height(), 0,
On 2015/11/03 at 04:12:59, piman (slow to review) wrote:
> You're modifying the texture bound to the GL_TEXTURE1 unit, which is not
necessarily target_texture (which was bound to the current unit when we entered
the function, not necessarily, and likely not, GL_TEXTURE1).

That was intended to allocate the storage for target_texture. Moved just after
getting the bound texture name.

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface_unittest.cc (right):

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface_unittest.cc:15: class GLImageIOSurface420vTestDelegate
{
On 2015/11/03 at 02:44:34, reveman wrote:
> nit: s/GLImageIOSurface420vTestDelegate/GLImageIOSurfaceTestDelegate/

Done.

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface_unittest.cc:21: new gl::GLImageIOSurface(size,
GL_RGB_YCBCR_420V_CHROMIUM));
On 2015/11/03 at 02:47:18, reveman wrote:
> nit: GL_RGB_YCBCR_420V_CHROMIUM is not correct if format is *GBA_8888.

Done.

[piman, 2015-11-04 22:49:46]

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:299: program_ =
gfx::GLHelper::SetupProgram(vertex_shader_, fragment_shader_);
On 2015/11/03 14:59:30, reveman wrote:
> On 2015/11/03 at 04:12:59, piman (slow to review) wrote:
> > It seems wasteful to compile & link a program for every single GLImage that
> goes through this path.
> > 
> > Would there be a way to share this state somehow? E.g. hook it up to the
> context maybe?
> 
> We discussed this and agreed that the context would be an appropriate way to
> share this state but thought it would be better to do that as a follow up.
> 
> > 
> > 
> > Part of me is wondering - does this really belong to GLImage? We have some
> amount of infrastructure for similar things, that we could share in
> CopyTextureCHROMIUMResourceManager...  I wonder if it could make more sense
for
> the decoder to do the color conversion.
> 
> Sharing the general color space conversion code would be nice but there's a
lot
> of IOSurface specific code here that belongs in GLImage and shouldn't be
exposed
> to the decoder. Moving large parts of the logic in GLImageIOSurface to some
> GLBlitter/GLColorSpaceConverter classes that could be part of the GLContext
> state sgtm. Maybe CopyTextureCHROMIUMResourceManager could take advantage of
> that too.

I don't want to block this, but this is at least the 4th implementation of a
blit-a-quad-with-a-shader-and-maybe-do-color-conversion infrastructure (cc,
content::GLHelper, CopyTextureCHROMIUMResourceManager, now here), and none of
them share anything, so I think it's worth thinking about whether or not could
do better.

https://codereview.chromium.org/1419733005/diff/230001/ui/gl/scoped_binders.cc
File ui/gl/scoped_binders.cc (right):

https://codereview.chromium.org/1419733005/diff/230001/ui/gl/scoped_binders.c...
ui/gl/scoped_binders.cc:116: glVertexAttribPointer(index_, size_, type_,
normalized_, stride_, pointer_);
So this one is a little tricky, because there's an implicit state that this call
latches on, which is the current array buffer.

So you need to:
1- in the constructor, query GL_VERTEX_ATTRIB_ARRAY_BUFFER_BINDING as the saved
array buffer
2- in the destructor, before this call, save the current array buffer, and bind
the saved array buffer as the current array buffer
3- do this call
4- restore the current array buffer

https://codereview.chromium.org/1419733005/diff/230001/ui/gl/scoped_binders.c...
ui/gl/scoped_binders.cc:161: GL_DITHER,       GL_POLYGON_OFFSET_FILL,
GL_SAMPLE_ALPHA_TO_COVERAGE,
- polygon offset shouldn't matter since you disable depth
- GL_SAMPLE_ALPHA_TO_COVERAGE doesn't matter since you're not rendering to a
multisample target

[Daniele Castagna, 2015-12-03 23:12:08]

On 2015/11/04 at 22:49:46, piman wrote:
>
https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
> File ui/gl/gl_image_io_surface.mm (right):
> 
>
https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
> ui/gl/gl_image_io_surface.mm:299: program_ =
gfx::GLHelper::SetupProgram(vertex_shader_, fragment_shader_);
> On 2015/11/03 14:59:30, reveman wrote:
> > On 2015/11/03 at 04:12:59, piman (slow to review) wrote:
> > > It seems wasteful to compile & link a program for every single GLImage
that
> > goes through this path.
> > > 
> > > Would there be a way to share this state somehow? E.g. hook it up to the
> > context maybe?
> > 
> > We discussed this and agreed that the context would be an appropriate way to
> > share this state but thought it would be better to do that as a follow up.
> > 
> > > 
> > > 
> > > Part of me is wondering - does this really belong to GLImage? We have some
> > amount of infrastructure for similar things, that we could share in
> > CopyTextureCHROMIUMResourceManager...  I wonder if it could make more sense
for
> > the decoder to do the color conversion.
> > 
> > Sharing the general color space conversion code would be nice but there's a
lot
> > of IOSurface specific code here that belongs in GLImage and shouldn't be
exposed
> > to the decoder. Moving large parts of the logic in GLImageIOSurface to some
> > GLBlitter/GLColorSpaceConverter classes that could be part of the GLContext
> > state sgtm. Maybe CopyTextureCHROMIUMResourceManager could take advantage of
> > that too.
> 
> I don't want to block this, but this is at least the 4th implementation of a
blit-a-quad-with-a-shader-and-maybe-do-color-conversion infrastructure (cc,
content::GLHelper, CopyTextureCHROMIUMResourceManager, now here), and none of
them share anything, so I think it's worth thinking about whether or not could
do better.
> 

Unfortunately there are other implementations (GLImageCopyTest.CopyTexImage and
in TextureUploadPerfTest), and another one is lined up too
(crrev.com/1419623008).

It might be a little tricky to merge implementations in cc and content::GLHelper
since there are many details that we don't care about in this patch and would
add a lot of complexity to gl/.

WDYT if we start moving some duplicated code for the simplest case to
gl/gl_helper.cc so that at least we avoid adding more draw-quad implementations?
Something like crrev.com/1496153002?

https://codereview.chromium.org/1419733005/diff/230001/ui/gl/scoped_binders.cc
File ui/gl/scoped_binders.cc (right):

https://codereview.chromium.org/1419733005/diff/230001/ui/gl/scoped_binders.c...
ui/gl/scoped_binders.cc:116: glVertexAttribPointer(index_, size_, type_,
normalized_, stride_, pointer_);
On 2015/11/04 at 22:49:45, piman (slow to review) wrote:
> So this one is a little tricky, because there's an implicit state that this
call latches on, which is the current array buffer.
> 
> So you need to:
> 1- in the constructor, query GL_VERTEX_ATTRIB_ARRAY_BUFFER_BINDING as the
saved array buffer
> 2- in the destructor, before this call, save the current array buffer, and
bind the saved array buffer as the current array buffer
> 3- do this call
> 4- restore the current array buffer

Done.

https://codereview.chromium.org/1419733005/diff/230001/ui/gl/scoped_binders.c...
ui/gl/scoped_binders.cc:161: GL_DITHER,       GL_POLYGON_OFFSET_FILL,
GL_SAMPLE_ALPHA_TO_COVERAGE,
On 2015/11/04 at 22:49:45, piman (slow to review) wrote:
> - polygon offset shouldn't matter since you disable depth
> - GL_SAMPLE_ALPHA_TO_COVERAGE doesn't matter since you're not rendering to a
multisample target

Ack.
This ScopedCapabilitiesRestorer could be used in other places though where those
capabilities could matter.

[piman, 2015-12-04 00:02:16]

https://codereview.chromium.org/1419733005/diff/230001/ui/gl/scoped_binders.cc
File ui/gl/scoped_binders.cc (right):

https://codereview.chromium.org/1419733005/diff/230001/ui/gl/scoped_binders.c...
ui/gl/scoped_binders.cc:161: GL_DITHER,       GL_POLYGON_OFFSET_FILL,
GL_SAMPLE_ALPHA_TO_COVERAGE,
On 2015/12/03 23:12:08, Daniele Castagna wrote:
> On 2015/11/04 at 22:49:45, piman (slow to review) wrote:
> > - polygon offset shouldn't matter since you disable depth
> > - GL_SAMPLE_ALPHA_TO_COVERAGE doesn't matter since you're not rendering to a
> multisample target
> 
> Ack.
> This ScopedCapabilitiesRestorer could be used in other places though where
those
> capabilities could matter.

Sure, but why these ones and not others too? E.g. GL_SAMPLE_COVERAGE is in core
ES 2.0, and if 3.0 is supported and/or extensions, we add other ones.

Maybe I'd argue for a separate ScopedCapabilityRestorer for each individual
capability.

[piman, 2015-12-04 00:03:32]

On Thu, Dec 3, 2015 at 3:12 PM, <dcastagna@chromium.org> wrote:

> On 2015/11/04 at 22:49:46, piman wrote:
>
>
>
https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
>
>> File ui/gl/gl_image_io_surface.mm (right):
>>
>
>
>
>
https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
>
>> ui/gl/gl_image_io_surface.mm:299: program_ =
>>
> gfx::GLHelper::SetupProgram(vertex_shader_, fragment_shader_);
>
>> On 2015/11/03 14:59:30, reveman wrote:
>> > On 2015/11/03 at 04:12:59, piman (slow to review) wrote:
>> > > It seems wasteful to compile & link a program for every single GLImage
>>
> that
>
>> > goes through this path.
>> > >
>> > > Would there be a way to share this state somehow? E.g. hook it up to
>> the
>> > context maybe?
>> >
>> > We discussed this and agreed that the context would be an appropriate
>> way to
>> > share this state but thought it would be better to do that as a follow
>> up.
>> >
>> > >
>> > >
>> > > Part of me is wondering - does this really belong to GLImage? We have
>> some
>> > amount of infrastructure for similar things, that we could share in
>> > CopyTextureCHROMIUMResourceManager...  I wonder if it could make more
>> sense
>>
> for
>
>> > the decoder to do the color conversion.
>> >
>> > Sharing the general color space conversion code would be nice but
>> there's a
>>
> lot
>
>> > of IOSurface specific code here that belongs in GLImage and shouldn't be
>>
> exposed
>
>> > to the decoder. Moving large parts of the logic in GLImageIOSurface to
>> some
>> > GLBlitter/GLColorSpaceConverter classes that could be part of the
>> GLContext
>> > state sgtm. Maybe CopyTextureCHROMIUMResourceManager could take
>> advantage of
>> > that too.
>>
>
> I don't want to block this, but this is at least the 4th implementation of
>> a
>>
> blit-a-quad-with-a-shader-and-maybe-do-color-conversion infrastructure (cc,
> content::GLHelper, CopyTextureCHROMIUMResourceManager, now here), and none
> of
> them share anything, so I think it's worth thinking about whether or not
> could
> do better.
>
>
> Unfortunately there are other implementations
> (GLImageCopyTest.CopyTexImage and
> in TextureUploadPerfTest), and another one is lined up too
> (crrev.com/1419623008).
>
> It might be a little tricky to merge implementations in cc and
> content::GLHelper
> since there are many details that we don't care about in this patch and
> would
> add a lot of complexity to gl/.
>
> WDYT if we start moving some duplicated code for the simplest case to
> gl/gl_helper.cc so that at least we avoid adding more draw-quad
> implementations?
> Something like crrev.com/1496153002?
>
>
Sharing code as a follow-up SGTM. Can you add a TODO in this CL?


>
> https://codereview.chromium.org/1419733005/
>

-- 
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.

[Daniele Castagna, 2015-12-04 00:40:18]

On 2015/12/04 at 00:03:32, piman wrote:
> On Thu, Dec 3, 2015 at 3:12 PM, <dcastagna@chromium.org> wrote:
> 
> > On 2015/11/04 at 22:49:46, piman wrote:
> >
> >
> >
https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
> >
> >> File ui/gl/gl_image_io_surface.mm (right):
> >>
> >
> >
> >
> >
https://codereview.chromium.org/1419733005/diff/100002/ui/gl/gl_image_io_surf...
> >
> >> ui/gl/gl_image_io_surface.mm:299: program_ =
> >>
> > gfx::GLHelper::SetupProgram(vertex_shader_, fragment_shader_);
> >
> >> On 2015/11/03 14:59:30, reveman wrote:
> >> > On 2015/11/03 at 04:12:59, piman (slow to review) wrote:
> >> > > It seems wasteful to compile & link a program for every single GLImage
> >>
> > that
> >
> >> > goes through this path.
> >> > >
> >> > > Would there be a way to share this state somehow? E.g. hook it up to
> >> the
> >> > context maybe?
> >> >
> >> > We discussed this and agreed that the context would be an appropriate
> >> way to
> >> > share this state but thought it would be better to do that as a follow
> >> up.
> >> >
> >> > >
> >> > >
> >> > > Part of me is wondering - does this really belong to GLImage? We have
> >> some
> >> > amount of infrastructure for similar things, that we could share in
> >> > CopyTextureCHROMIUMResourceManager...  I wonder if it could make more
> >> sense
> >>
> > for
> >
> >> > the decoder to do the color conversion.
> >> >
> >> > Sharing the general color space conversion code would be nice but
> >> there's a
> >>
> > lot
> >
> >> > of IOSurface specific code here that belongs in GLImage and shouldn't be
> >>
> > exposed
> >
> >> > to the decoder. Moving large parts of the logic in GLImageIOSurface to
> >> some
> >> > GLBlitter/GLColorSpaceConverter classes that could be part of the
> >> GLContext
> >> > state sgtm. Maybe CopyTextureCHROMIUMResourceManager could take
> >> advantage of
> >> > that too.
> >>
> >
> > I don't want to block this, but this is at least the 4th implementation of
> >> a
> >>
> > blit-a-quad-with-a-shader-and-maybe-do-color-conversion infrastructure (cc,
> > content::GLHelper, CopyTextureCHROMIUMResourceManager, now here), and none
> > of
> > them share anything, so I think it's worth thinking about whether or not
> > could
> > do better.
> >
> >
> > Unfortunately there are other implementations
> > (GLImageCopyTest.CopyTexImage and
> > in TextureUploadPerfTest), and another one is lined up too
> > (crrev.com/1419623008).
> >
> > It might be a little tricky to merge implementations in cc and
> > content::GLHelper
> > since there are many details that we don't care about in this patch and
> > would
> > add a lot of complexity to gl/.
> >
> > WDYT if we start moving some duplicated code for the simplest case to
> > gl/gl_helper.cc so that at least we avoid adding more draw-quad
> > implementations?
> > Something like crrev.com/1496153002?
> >
> >
> Sharing code as a follow-up SGTM. Can you add a TODO in this CL?
> 

TODO added.

https://codereview.chromium.org/1419733005/diff/230001/ui/gl/scoped_binders.cc
File ui/gl/scoped_binders.cc (right):

https://codereview.chromium.org/1419733005/diff/230001/ui/gl/scoped_binders.c...
ui/gl/scoped_binders.cc:161: GL_DITHER,       GL_POLYGON_OFFSET_FILL,
GL_SAMPLE_ALPHA_TO_COVERAGE,
On 2015/12/04 at 00:02:16, piman (slow to review) wrote:
> On 2015/12/03 23:12:08, Daniele Castagna wrote:
> > On 2015/11/04 at 22:49:45, piman (slow to review) wrote:
> > > - polygon offset shouldn't matter since you disable depth
> > > - GL_SAMPLE_ALPHA_TO_COVERAGE doesn't matter since you're not rendering to
a
> > multisample target
> > 
> > Ack.
> > This ScopedCapabilitiesRestorer could be used in other places though where
those
> > capabilities could matter.
> 
> Sure, but why these ones and not others too? E.g. GL_SAMPLE_COVERAGE is in
core ES 2.0, and if 3.0 is supported and/or extensions, we add other ones.
> 

Not sure why I left GL_SAMPLE_COVERAGE out. My intention was to add all the
capabilities in core ES 2.0. :/

> Maybe I'd argue for a separate ScopedCapabilityRestorer for each individual
capability.

Removed ScopedCapabilities and added a ScopedCapability that takes the
capability enum as parameter:

gfx::ScopedCapability disable_blending(GL_BLEND, GL_FALSE);

[piman, 2015-12-04 03:23:31]

https://codereview.chromium.org/1419733005/diff/290001/ui/gl/scoped_binders.cc
File ui/gl/scoped_binders.cc (right):

https://codereview.chromium.org/1419733005/diff/290001/ui/gl/scoped_binders.c...
ui/gl/scoped_binders.cc:163: enabled_ = glIsEnabled(capability_);
I assume you also want to:
if (enabled)
  glEnable(capability);
else
  glDisable(capability);

[Daniele Castagna, 2015-12-04 04:41:30]

https://codereview.chromium.org/1419733005/diff/290001/ui/gl/scoped_binders.cc
File ui/gl/scoped_binders.cc (right):

https://codereview.chromium.org/1419733005/diff/290001/ui/gl/scoped_binders.c...
ui/gl/scoped_binders.cc:163: enabled_ = glIsEnabled(capability_);
On 2015/12/04 at 03:23:31, piman (slow to review) wrote:
> I assume you also want to:
> if (enabled)
>   glEnable(capability);
> else
>   glDisable(capability);

Right. Done.

[piman, 2015-12-04 04:44:23]

lgtm

[Daniele Castagna, 2015-12-04 04:53:25]

Description was changed from

==========
gpu: Add YCbCr 420v extension.

On Mac we currently support textures backed by GpuMemoryBuffer
with UYVY_422 format when GL_APPLE_ycbcr_422 is available.
This allows both to use the backing IOSurface as CALayers and to
bind the IOSurface to a GL texture for sampling.

420v IOSurfaces, while being more efficient when used as a CALayer,
don't support being sampled from GL.

This CL adds a CHROMIUM extension, that when available, allows users
of CHROMIUM command buffer to use a 420v IOSurface from GL as an RGB
texture.

This is achieved converting the IOSurface from YUV to RGB when it's
bound to a texture on the service side.

This will allow to use 420v for video playback using VideoLayerImpl
(promoted to CALayer) and from SkCanvasVideoRenderer when needed (WebGL,
Canvas & co.) 

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

to

==========
gpu: Add YCbCr 420v extension.

On Mac we currently support textures backed by GpuMemoryBuffer
with UYVY_422 format when GL_APPLE_ycbcr_422 is available.
This allows both to use the backing IOSurface as CALayers and to
bind the IOSurface to a GL texture for sampling.

420v IOSurfaces, while being more efficient when used as a CALayer,
don't support being sampled from GL.

This CL adds a CHROMIUM extension, that when available, allows users
of CHROMIUM command buffer to use a 420v IOSurface from GL as an RGB
texture.

This is achieved converting the IOSurface from YUV to RGB when it's
bound to a texture on the service side.

This will allow to use 420v for video playback using VideoLayerImpl
(promoted to CALayer) and from SkCanvasVideoRenderer when needed (WebGL,
Canvas & co.) 

BUG=533690
==========

[Daniele Castagna, 2015-12-04 04:53:32]

The CQ bit was checked by dcastagna@chromium.org

[Daniele Castagna, 2015-12-04 04:53:33]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/1419733005/#ps310001
(title: "Actually enable/disable capabilities. Typo.")

[Daniele Castagna, 2015-12-04 04:54:01]

The CQ bit was unchecked by dcastagna@chromium.org

[Daniele Castagna, 2015-12-04 04:56:42]

Description was changed from

==========
gpu: Add YCbCr 420v extension.

On Mac we currently support textures backed by GpuMemoryBuffer
with UYVY_422 format when GL_APPLE_ycbcr_422 is available.
This allows both to use the backing IOSurface as CALayers and to
bind the IOSurface to a GL texture for sampling.

420v IOSurfaces, while being more efficient when used as a CALayer,
don't support being sampled from GL.

This CL adds a CHROMIUM extension, that when available, allows users
of CHROMIUM command buffer to use a 420v IOSurface from GL as an RGB
texture.

This is achieved converting the IOSurface from YUV to RGB when it's
bound to a texture on the service side.

This will allow to use 420v for video playback using VideoLayerImpl
(promoted to CALayer) and from SkCanvasVideoRenderer when needed (WebGL,
Canvas & co.) 

BUG=533690
==========

to

==========
gpu: Add YCbCr 420v extension.

On Mac we currently support textures backed by GpuMemoryBuffer
with UYVY_422 format when GL_APPLE_ycbcr_422 is available.
This allows both to use the backing IOSurface as CALayers and to
bind the IOSurface to a GL texture for sampling.

420v IOSurfaces, while being more efficient when used as a CALayer,
don't support being sampled from GL.

This CL adds a CHROMIUM extension, that when available, allows users
of CHROMIUM command buffer to use a 420v IOSurface from GL as an RGB
texture.

This is achieved converting the IOSurface from YUV to RGB when it's
bound to a texture on the service side.

This will allow to use 420v for video playback using VideoLayerImpl
(promoted to CALayer) and from SkCanvasVideoRenderer when needed (WebGL,
Canvas & co.) 

BUG=524582
==========

[Daniele Castagna, 2015-12-04 04:56:48]

The CQ bit was checked by dcastagna@chromium.org

[commit-bot: I haz the power, 2015-12-04 04:57:11]

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

[commit-bot: I haz the power, 2015-12-04 06:06:22]

Committed patchset #16 (id:310001)

[commit-bot: I haz the power, 2015-12-04 06:07:01]

Description was changed from

==========
gpu: Add YCbCr 420v extension.

On Mac we currently support textures backed by GpuMemoryBuffer
with UYVY_422 format when GL_APPLE_ycbcr_422 is available.
This allows both to use the backing IOSurface as CALayers and to
bind the IOSurface to a GL texture for sampling.

420v IOSurfaces, while being more efficient when used as a CALayer,
don't support being sampled from GL.

This CL adds a CHROMIUM extension, that when available, allows users
of CHROMIUM command buffer to use a 420v IOSurface from GL as an RGB
texture.

This is achieved converting the IOSurface from YUV to RGB when it's
bound to a texture on the service side.

This will allow to use 420v for video playback using VideoLayerImpl
(promoted to CALayer) and from SkCanvasVideoRenderer when needed (WebGL,
Canvas & co.) 

BUG=524582
==========

to

==========
gpu: Add YCbCr 420v extension.

On Mac we currently support textures backed by GpuMemoryBuffer
with UYVY_422 format when GL_APPLE_ycbcr_422 is available.
This allows both to use the backing IOSurface as CALayers and to
bind the IOSurface to a GL texture for sampling.

420v IOSurfaces, while being more efficient when used as a CALayer,
don't support being sampled from GL.

This CL adds a CHROMIUM extension, that when available, allows users
of CHROMIUM command buffer to use a 420v IOSurface from GL as an RGB
texture.

This is achieved converting the IOSurface from YUV to RGB when it's
bound to a texture on the service side.

This will allow to use 420v for video playback using VideoLayerImpl
(promoted to CALayer) and from SkCanvasVideoRenderer when needed (WebGL,
Canvas & co.)

BUG=524582

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

[commit-bot: I haz the power, 2015-12-04 06:07:02]

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