GLImage support for Android zero-copy pixel buffers

GLImage support for Android zero-copy pixel buffers


BUG=175012
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192971

[kaanb, 2013-04-04 22:53:43]

I'm splitting up the big patch in https://codereview.chromium.org/12386027/ for
landing.

[danakj, 2013-04-04 22:57:36]

+reveman

[apatrick_chromium, 2013-04-04 23:33:40]

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_android.cc
File ui/gl/gl_image_android.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_android.cc#ne...
ui/gl/gl_image_android.cc:43: return NULL;
nit: redundant

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode46
ui/gl/gl_image_egl.cc:46: glEGLImageTargetTexture2DOES(GL_TEXTURE_2D,
egl_image_);
This could generate a GL error that should not be visible to command buffer
clients. Maybe you should check for the error at a higher level so that any
existing errors can be gathered beforehand and any new ones hidden from the
client?

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode49
ui/gl/gl_image_egl.cc:49: while ((error = glGetError()) != GL_NO_ERROR) {
This will never iterate more than once. Again, maybe do it at a higher level
after ensuring that any existing errors are cached off and cleared so we know
that glEGLImageTargetTexture2DOES was responsible for the error?

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode53
ui/gl/gl_image_egl.cc:53: EGLBoolean success = eglDestroyImageKHR(
Why destroy it here rather than in GLImageEGL::Destroy?

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode73
ui/gl/gl_image_egl.cc:73: return;
nit: redundant

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.h
File ui/gl/gl_image_egl.h (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.h#newcode15
ui/gl/gl_image_egl.h:15: GLImageEGL(const gfx::Size& size);
nit: explicit

[reveman, 2013-04-05 00:13:24]

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image.h#newcode40
ui/gl/gl_image.h:40: gfx::PixelBufferHandle window, const gfx::Size& size);
"window" is not a very good name for that parameter. "buffer" as used in
gl_image_android.cc is better.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_android.cc
File ui/gl/gl_image_android.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_android.cc#ne...
ui/gl/gl_image_android.cc:16: switch (GetGLImplementation()) {
nit: please return NULL here for kGLImplementationEGLGLES2 to be consistent with
GLImage::CreateGLImageForPixelBuffer.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_android.cc#ne...
ui/gl/gl_image_android.cc:23: return NULL;
nit: "return NULL" is redundant

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode9
ui/gl/gl_image_egl.cc:9: #define EGL_NATIVE_BUFFER_EGL 0x3140
Is this supposed to be EGL_NATIVE_BUFFER_ANDROID 0x3140? Does this belong in
third_party/khronos/EGL/eglext.h instead?

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode14
ui/gl/gl_image_egl.cc:14: : egl_image_(NULL),
EGL_NO_IMAGE_KHR instead of NULL?

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode18
ui/gl/gl_image_egl.cc:18: GLImageEGL::~GLImageEGL() {
call Destroy() here for consistency.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode27
ui/gl/gl_image_egl.cc:27: egl_image_ =
eglCreateImageKHR(eglGetDisplay(EGL_DEFAULT_DISPLAY),
nit: one argument per line if you can't fit them all in one line.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode32
ui/gl/gl_image_egl.cc:32: LOG(ERROR) << "GLImageEGL - error creating EGLImage: "
nit: "GLImageEGL::Initialize: Error cre..."

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode42
ui/gl/gl_image_egl.cc:42: LOG(ERROR) << "GLImageEGL - null EGLImage in
BindTexImage";
I don't think you need the GLImageEGL prefix here. Assuming this will
automatically include the function name.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode58
ui/gl/gl_image_egl.cc:58: LOG(ERROR) << "GLImageEGL - error destroying EGLImage:
"
I don't think you need the GLImageEGL prefix here.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode73
ui/gl/gl_image_egl.cc:73: return;
Can we add a DCHECK_EQ(EGL_NO_IMAGE_KHR, egl_image_) here?

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode78
ui/gl/gl_image_egl.cc:78: return;
nit: "return" is redundant

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.h
File ui/gl/gl_image_egl.h (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.h#newcode9
ui/gl/gl_image_egl.h:9: #include "ui/gl/gl_bindings.h" // for EGLImageKHR
nit: At least two spaces is best between code and comments
Include "ui/gl/gl_bindings.h" not in alphabetical order

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_linux.cc
File ui/gl/gl_image_linux.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_linux.cc#newc...
ui/gl/gl_image_linux.cc:41: switch (GetGLImplementation()) {
Please return NULL here for kGLImplementationOSMesaGL,
kGLImplementationDesktopGL, kGLImplementationEGLGLES2 to be consistent with
GLImage::CreateGLImage above.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_linux.cc#newc...
ui/gl/gl_image_linux.cc:48: return NULL;
nit: "return NULL" is redundant.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_mac.cc
File ui/gl/gl_image_mac.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_mac.cc#newcode32
ui/gl/gl_image_mac.cc:32: switch (GetGLImplementation()) {
Please return NULL here for kGLImplementationOSMesaGL,
kGLImplementationDesktopGL, kGLImplementationAppleGL to be consistent with
GLImage::CreateGLImage above.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_mac.cc#newcode39
ui/gl/gl_image_mac.cc:39: return NULL;
nit: "return NULL" is redundant

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_win.cc
File ui/gl/gl_image_win.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_win.cc#newcode32
ui/gl/gl_image_win.cc:32: switch (GetGLImplementation()) {
Please return NULL here for kGLImplementationOSMesaGL,
kGLImplementationDesktopGL, kGLImplementationEGLGLES2 to be consistent with
GLImage::CreateGLImage above.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_win.cc#newcode39
ui/gl/gl_image_win.cc:39: return NULL;
nit: "return NULL" is redundant

[no sievers, 2013-04-05 00:23:02]

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode53
ui/gl/gl_image_egl.cc:53: EGLBoolean success = eglDestroyImageKHR(
On 2013/04/04 23:33:40, apatrick_chromium wrote:
> Why destroy it here rather than in GLImageEGL::Destroy?

Yes please destroy it down there. We might want to use EGLimages to share
resources and be able to bind/unbind it to different textures.

That being said, what would need to happen in ReleaseTexImage() to unbind the
texture from the image?

[apatrick_chromium, 2013-04-05 00:38:04]

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode53
ui/gl/gl_image_egl.cc:53: EGLBoolean success = eglDestroyImageKHR(
> That being said, what would need to happen in ReleaseTexImage() to unbind the
> texture from the image?

GLES2DecoderImpl::DoReleaseTexImage2DCHROMIUM is going to this afterwards:

  texture_manager()->SetLevelInfo(
      texture, target, 0, GL_RGBA, 0, 0, 1, 0,
      GL_RGBA, GL_UNSIGNED_BYTE, false);

So maybe the texture should be redefined with glTexImage2D with the
corresponding arguments. Will that effectively break its tie to the EGLImage?

[kaanb, 2013-04-05 01:02:52]

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_android.cc
File ui/gl/gl_image_android.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_android.cc#ne...
ui/gl/gl_image_android.cc:16: switch (GetGLImplementation()) {
On 2013/04/05 00:13:24, David Reveman wrote:
> nit: please return NULL here for kGLImplementationEGLGLES2 to be consistent
with
> GLImage::CreateGLImageForPixelBuffer.

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_android.cc#ne...
ui/gl/gl_image_android.cc:23: return NULL;
On 2013/04/05 00:13:24, David Reveman wrote:
> nit: "return NULL" is redundant

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_android.cc#ne...
ui/gl/gl_image_android.cc:43: return NULL;
On 2013/04/04 23:33:40, apatrick_chromium wrote:
> nit: redundant

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode9
ui/gl/gl_image_egl.cc:9: #define EGL_NATIVE_BUFFER_EGL 0x3140
On 2013/04/05 00:13:24, David Reveman wrote:
> Is this supposed to be EGL_NATIVE_BUFFER_ANDROID 0x3140? Does this belong in
> third_party/khronos/EGL/eglext.h instead?

Yes, do I need to make the change in a different repository to add this?

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode14
ui/gl/gl_image_egl.cc:14: : egl_image_(NULL),
On 2013/04/05 00:13:24, David Reveman wrote:
> EGL_NO_IMAGE_KHR instead of NULL?

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode18
ui/gl/gl_image_egl.cc:18: GLImageEGL::~GLImageEGL() {
On 2013/04/05 00:13:24, David Reveman wrote:
> call Destroy() here for consistency.

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode27
ui/gl/gl_image_egl.cc:27: egl_image_ =
eglCreateImageKHR(eglGetDisplay(EGL_DEFAULT_DISPLAY),
On 2013/04/05 00:13:24, David Reveman wrote:
> nit: one argument per line if you can't fit them all in one line.

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode32
ui/gl/gl_image_egl.cc:32: LOG(ERROR) << "GLImageEGL - error creating EGLImage: "
On 2013/04/05 00:13:24, David Reveman wrote:
> nit: "GLImageEGL::Initialize: Error cre..."

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode42
ui/gl/gl_image_egl.cc:42: LOG(ERROR) << "GLImageEGL - null EGLImage in
BindTexImage";
On 2013/04/05 00:13:24, David Reveman wrote:
> I don't think you need the GLImageEGL prefix here. Assuming this will
> automatically include the function name.

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode46
ui/gl/gl_image_egl.cc:46: glEGLImageTargetTexture2DOES(GL_TEXTURE_2D,
egl_image_);
On 2013/04/04 23:33:40, apatrick_chromium wrote:
> This could generate a GL error that should not be visible to command buffer
> clients. Maybe you should check for the error at a higher level so that any
> existing errors can be gathered beforehand and any new ones hidden from the
> client?

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode49
ui/gl/gl_image_egl.cc:49: while ((error = glGetError()) != GL_NO_ERROR) {
On 2013/04/04 23:33:40, apatrick_chromium wrote:
> This will never iterate more than once. Again, maybe do it at a higher level
> after ensuring that any existing errors are cached off and cleared so we know
> that glEGLImageTargetTexture2DOES was responsible for the error?

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode53
ui/gl/gl_image_egl.cc:53: EGLBoolean success = eglDestroyImageKHR(
On 2013/04/05 00:23:03, Daniel Sievers wrote:
> On 2013/04/04 23:33:40, apatrick_chromium wrote:
> > Why destroy it here rather than in GLImageEGL::Destroy?
> 
> Yes please destroy it down there. We might want to use EGLimages to share
> resources and be able to bind/unbind it to different textures.
> 
> That being said, what would need to happen in ReleaseTexImage() to unbind the
> texture from the image?

Good question, I don't know the answer.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode53
ui/gl/gl_image_egl.cc:53: EGLBoolean success = eglDestroyImageKHR(
On 2013/04/05 00:38:04, apatrick_chromium wrote:
> > That being said, what would need to happen in ReleaseTexImage() to unbind
the
> > texture from the image?
> 
> GLES2DecoderImpl::DoReleaseTexImage2DCHROMIUM is going to this afterwards:
> 
>   texture_manager()->SetLevelInfo(
>       texture, target, 0, GL_RGBA, 0, 0, 1, 0,
>       GL_RGBA, GL_UNSIGNED_BYTE, false);
> 
> So maybe the texture should be redefined with glTexImage2D with the
> corresponding arguments. Will that effectively break its tie to the EGLImage?

Actually I realized epenner@ had suggested something similar in the parent cl.
Should I call glTexImage2D with a NULL pixels pointer? Also, how do we test
this?

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode58
ui/gl/gl_image_egl.cc:58: LOG(ERROR) << "GLImageEGL - error destroying EGLImage:
"
On 2013/04/05 00:13:24, David Reveman wrote:
> I don't think you need the GLImageEGL prefix here.

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode73
ui/gl/gl_image_egl.cc:73: return;
On 2013/04/04 23:33:40, apatrick_chromium wrote:
> nit: redundant

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode73
ui/gl/gl_image_egl.cc:73: return;
On 2013/04/05 00:13:24, David Reveman wrote:
> Can we add a DCHECK_EQ(EGL_NO_IMAGE_KHR, egl_image_) here?

DCHECK may fail when if we never called Destroy() before and it's getting called
within the destructor. Are we OK with that?

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode78
ui/gl/gl_image_egl.cc:78: return;
On 2013/04/05 00:13:24, David Reveman wrote:
> nit: "return" is redundant

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.h
File ui/gl/gl_image_egl.h (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.h#newcode9
ui/gl/gl_image_egl.h:9: #include "ui/gl/gl_bindings.h" // for EGLImageKHR
On 2013/04/05 00:13:24, David Reveman wrote:
> nit: At least two spaces is best between code and comments
> Include "ui/gl/gl_bindings.h" not in alphabetical order

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.h#newcode15
ui/gl/gl_image_egl.h:15: GLImageEGL(const gfx::Size& size);
On 2013/04/04 23:33:40, apatrick_chromium wrote:
> nit: explicit

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_linux.cc
File ui/gl/gl_image_linux.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_linux.cc#newc...
ui/gl/gl_image_linux.cc:41: switch (GetGLImplementation()) {
On 2013/04/05 00:13:24, David Reveman wrote:
> Please return NULL here for kGLImplementationOSMesaGL,
> kGLImplementationDesktopGL, kGLImplementationEGLGLES2 to be consistent with
> GLImage::CreateGLImage above.

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_linux.cc#newc...
ui/gl/gl_image_linux.cc:48: return NULL;
On 2013/04/05 00:13:24, David Reveman wrote:
> nit: "return NULL" is redundant.

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_mac.cc
File ui/gl/gl_image_mac.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_mac.cc#newcode32
ui/gl/gl_image_mac.cc:32: switch (GetGLImplementation()) {
On 2013/04/05 00:13:24, David Reveman wrote:
> Please return NULL here for kGLImplementationOSMesaGL,
> kGLImplementationDesktopGL, kGLImplementationAppleGL to be consistent with
> GLImage::CreateGLImage above.

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_mac.cc#newcode39
ui/gl/gl_image_mac.cc:39: return NULL;
On 2013/04/05 00:13:24, David Reveman wrote:
> nit: "return NULL" is redundant

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_win.cc
File ui/gl/gl_image_win.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_win.cc#newcode32
ui/gl/gl_image_win.cc:32: switch (GetGLImplementation()) {
On 2013/04/05 00:13:24, David Reveman wrote:
> Please return NULL here for kGLImplementationOSMesaGL,
> kGLImplementationDesktopGL, kGLImplementationEGLGLES2 to be consistent with
> GLImage::CreateGLImage above.

Done.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_win.cc#newcode39
ui/gl/gl_image_win.cc:39: return NULL;
On 2013/04/05 00:13:24, David Reveman wrote:
> nit: "return NULL" is redundant

Done.

[reveman, 2013-04-05 04:35:32]

GLImage changes looks good, just move EGL_NATIVE_BUFFER_ANDROID to the right
place

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode9
ui/gl/gl_image_egl.cc:9: #define EGL_NATIVE_BUFFER_EGL 0x3140
no, third_party/khronos/EGL/eglext.h is part of the top level repo. you also
need update third_party/khronos/README.chromium after making this change.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode73
ui/gl/gl_image_egl.cc:73: return;
My point was that you should either destroy the image here or DCHECK to make
sure it's not leaked. Your latest patch that destroys the egl image here looks
good. No need for a DCHECK when you do that.

https://codereview.chromium.org/13543007/diff/18001/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/13543007/diff/18001/ui/gl/gl_image_egl.cc#new...
ui/gl/gl_image_egl.cc:14: : egl_image_(EGL_NO_IMAGE_KHR),
nit: indent 4 spaces here

[kaanb, 2013-04-05 07:17:05]

Should ReleaseTexImage() remain as NOTREACHED()?

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode9
ui/gl/gl_image_egl.cc:9: #define EGL_NATIVE_BUFFER_EGL 0x3140
On 2013/04/05 04:35:32, David Reveman wrote:
> no, third_party/khronos/EGL/eglext.h is part of the top level repo. you also
> need update third_party/khronos/README.chromium after making this change.
Actually this constant is already defined in third_party/khronos/EGL/eglext.h
but
gl.gyp specifies /third_party/angle/include to be used as the include directory
for Android. git points to an external repository for angle. What do you
suggest?

[no sievers, 2013-04-05 16:55:57]

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode53
ui/gl/gl_image_egl.cc:53: EGLBoolean success = eglDestroyImageKHR(
On 2013/04/05 00:38:04, apatrick_chromium wrote:
> > That being said, what would need to happen in ReleaseTexImage() to unbind
the
> > texture from the image?
> 
> GLES2DecoderImpl::DoReleaseTexImage2DCHROMIUM is going to this afterwards:
> 
>   texture_manager()->SetLevelInfo(
>       texture, target, 0, GL_RGBA, 0, 0, 1, 0,
>       GL_RGBA, GL_UNSIGNED_BYTE, false);
> 
> So maybe the texture should be redefined with glTexImage2D with the
> corresponding arguments. Will that effectively break its tie to the EGLImage?

If I read the spec correctly
(http://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image.txt), I think
that't the case and it says that the implementation/driver should reallocate
storage, copy existing levels and that the redefined texture is not en EGLimage
target (texture connected to an EGLimage) anymore. So redefining it to 0,0 with
glTexImage2D() seems like the right thing to do in ReleaseTexImage().

[apatrick_chromium, 2013-04-05 17:35:58]

LGTM with nits for my part. I defer sign-off to Daniel for correct
ReleaseTexImage implementation.

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_linux.cc
File ui/gl/gl_image_linux.cc (right):

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_linux.cc#n...
ui/gl/gl_image_linux.cc:44: case kGLImplementationEGLGLES2: {
nit: braces are redundant

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_mac.cc
File ui/gl/gl_image_mac.cc (right):

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_mac.cc#new...
ui/gl/gl_image_mac.cc:35: case kGLImplementationAppleGL: {
nit: braces are redundant

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_win.cc
File ui/gl/gl_image_win.cc (right):

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_win.cc#new...
ui/gl/gl_image_win.cc:35: case kGLImplementationEGLGLES2: {
nit: braces are redundant

[reveman, 2013-04-05 17:58:06]

It makes sense to add a proper implementation of ReleaseTexImage even if we
don't necessarily need it for 0-copy.

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/13543007/diff/12/ui/gl/gl_image_egl.cc#newcode9
ui/gl/gl_image_egl.cc:9: #define EGL_NATIVE_BUFFER_EGL 0x3140
On 2013/04/05 07:17:06, kaanb wrote:
> On 2013/04/05 04:35:32, David Reveman wrote:
> > no, third_party/khronos/EGL/eglext.h is part of the top level repo. you also
> > need update third_party/khronos/README.chromium after making this change.
> Actually this constant is already defined in third_party/khronos/EGL/eglext.h
> but
> gl.gyp specifies /third_party/angle/include to be used as the include
directory
> for Android. git points to an external repository for angle. What do you
> suggest?

hm, I'm not sure, maybe we should be using third_party/khronos if 'OS != "win"'.
apatrick, wdyt?

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_linux.cc
File ui/gl/gl_image_linux.cc (right):

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_linux.cc#n...
ui/gl/gl_image_linux.cc:17: case kGLImplementationOSMesaGL: {
if you remove redundant braces below, please remove them from here as well so
it's consistent.

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_linux.cc#n...
ui/gl/gl_image_linux.cc:27: case kGLImplementationEGLGLES2: {
and here

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_mac.cc
File ui/gl/gl_image_mac.cc (right):

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_mac.cc#new...
ui/gl/gl_image_mac.cc:18: case kGLImplementationAppleGL: {
and here

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_win.cc
File ui/gl/gl_image_win.cc (right):

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_win.cc#new...
ui/gl/gl_image_win.cc:18: case kGLImplementationEGLGLES2: {
and here

[reveman, 2013-04-05 18:25:57]

https://codereview.chromium.org/13543007/diff/21001/ui/gfx/native_widget_types.h
File ui/gfx/native_widget_types.h (right):

https://codereview.chromium.org/13543007/diff/21001/ui/gfx/native_widget_type...
ui/gfx/native_widget_types.h:324: typedef void* PixelBufferHandle;
I'm not sure we should be calling this "PixelBuffer". I think that might cause
name conflicts and be confused with PBOs.

I think GraphicBuffer is better.

[no sievers, 2013-04-05 18:34:41]

On 2013/04/05 18:25:57, David Reveman wrote:
>
https://codereview.chromium.org/13543007/diff/21001/ui/gfx/native_widget_types.h
> File ui/gfx/native_widget_types.h (right):
> 
>
https://codereview.chromium.org/13543007/diff/21001/ui/gfx/native_widget_type...
> ui/gfx/native_widget_types.h:324: typedef void* PixelBufferHandle;
> I'm not sure we should be calling this "PixelBuffer". I think that might cause
> name conflicts and be confused with PBOs.
> 
> I think GraphicBuffer is better.

or GpuMemoryBuffer?

The reason for the back and forth is that we were just looking at how to hook
this into Map/Unmap/BufferData in the gl implementation.

And it seems like we already implement the PBO api as shared memory (shared
between client and decoder).  What we are adding now is a class for
gpu-mapped/driver allocated graphics memory. So after all, maybe it's better to
somehow express that clearly.

[reveman, 2013-04-05 18:36:38]

On 2013/04/05 18:25:57, David Reveman wrote:
>
https://codereview.chromium.org/13543007/diff/21001/ui/gfx/native_widget_types.h
> File ui/gfx/native_widget_types.h (right):
> 
>
https://codereview.chromium.org/13543007/diff/21001/ui/gfx/native_widget_type...
> ui/gfx/native_widget_types.h:324: typedef void* PixelBufferHandle;
> I'm not sure we should be calling this "PixelBuffer". I think that might cause
> name conflicts and be confused with PBOs.
> 
> I think GraphicBuffer is better.

Or maybe PixelZeroCopyBuffer as we currently have something we call
PixelTransferBuffer for async uploads.

[kaanb, 2013-04-05 18:49:24]

https://codereview.chromium.org/13543007/diff/21001/ui/gfx/native_widget_types.h
File ui/gfx/native_widget_types.h (right):

https://codereview.chromium.org/13543007/diff/21001/ui/gfx/native_widget_type...
ui/gfx/native_widget_types.h:324: typedef void* PixelBufferHandle;
On 2013/04/05 18:25:58, David Reveman wrote:
> I'm not sure we should be calling this "PixelBuffer". I think that might cause
> name conflicts and be confused with PBOs.
> 
> I think GraphicBuffer is better.

I guess people didn't like GraphicBuffer because it was too tied to the specific
Android implementation. I'll stick with PixelBuffer for now.

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_linux.cc
File ui/gl/gl_image_linux.cc (right):

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_linux.cc#n...
ui/gl/gl_image_linux.cc:17: case kGLImplementationOSMesaGL: {
On 2013/04/05 17:58:07, David Reveman wrote:
> if you remove redundant braces below, please remove them from here as well so
> it's consistent.

Done.

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_linux.cc#n...
ui/gl/gl_image_linux.cc:27: case kGLImplementationEGLGLES2: {
On 2013/04/05 17:58:07, David Reveman wrote:
> and here

Done.

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_linux.cc#n...
ui/gl/gl_image_linux.cc:44: case kGLImplementationEGLGLES2: {
On 2013/04/05 17:35:58, apatrick_chromium wrote:
> nit: braces are redundant

Done.

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_mac.cc
File ui/gl/gl_image_mac.cc (right):

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_mac.cc#new...
ui/gl/gl_image_mac.cc:18: case kGLImplementationAppleGL: {
On 2013/04/05 17:58:07, David Reveman wrote:
> and here

Done.

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_mac.cc#new...
ui/gl/gl_image_mac.cc:35: case kGLImplementationAppleGL: {
On 2013/04/05 17:35:58, apatrick_chromium wrote:
> nit: braces are redundant

Done.

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_win.cc
File ui/gl/gl_image_win.cc (right):

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_win.cc#new...
ui/gl/gl_image_win.cc:18: case kGLImplementationEGLGLES2: {
On 2013/04/05 17:58:07, David Reveman wrote:
> and here

Done.

https://codereview.chromium.org/13543007/diff/21001/ui/gl/gl_image_win.cc#new...
ui/gl/gl_image_win.cc:35: case kGLImplementationEGLGLES2: {
On 2013/04/05 17:35:58, apatrick_chromium wrote:
> nit: braces are redundant

Done.

[epennerAtGoogle, 2013-04-05 19:01:30]

Looking good!

Just a couple random notes:
- The API can look like GL (map/unmap), but please don't make zero-copy a GL
API. We should preserve the concept of having a native type in the client (which
can be locked on arbitrary threads etc).

- The reading of the spec for ReleaseTexImage is correct, but in practice all
devices have problems with complicated binding/rebinding/unbinding, so we might
want to keep the NOTREACHED to prevent it's use. We'll definitely need that API
for surface texture, though.

[kaanb, 2013-04-05 19:55:27]

Ok, not sure why the latest mail came out empty. Is everybody okay with
GpuMemoryBuffer? I'll need to change quite a bit of files so it would be good to
have everybody in agreement.

[epennerAtGoogle, 2013-04-05 20:51:26]

On 2013/04/05 19:55:27, kaanb wrote:
> Ok, not sure why the latest mail came out empty. Is everybody okay with
> GpuMemoryBuffer? I'll need to change quite a bit of files so it would be good
to
> have everybody in agreement.

+1 From me. Best one I've heard so far.

[kaanb, 2013-04-05 21:18:35]

On 2013/04/05 20:51:26, epennerAtGoogle wrote:
> On 2013/04/05 19:55:27, kaanb wrote:
> > Ok, not sure why the latest mail came out empty. Is everybody okay with
> > GpuMemoryBuffer? I'll need to change quite a bit of files so it would be
good
> to
> > have everybody in agreement.
> 
> +1 From me. Best one I've heard so far.

For the sake of brevity should we go with 'GpuBuffer'? Buffer already implies
it's related to memory. What do you think?

[epennerAtGoogle, 2013-04-05 21:28:12]

I like GpuMemoryBuffer. No particular reason ;)

Hmm, I guess 'buffer' could be anything, like command buffer, vertex buffer etc.
and this is specifically a gpu texture memory buffer.

Anyway, I like all of the above.

[reveman, 2013-04-05 21:29:42]

GpuMemoryBuffer sgtm.

[kaanb1, 2013-04-05 22:12:46]

GpuMemoryBuffer for the win. I've done s/PixelBuffer/GpuMemoryBuffer/
everywhere.

I'd prefer to tackle fixing the gyp file to pull the Khronos headers later
on as that change could cause some issues on various platforms.



On Fri, Apr 5, 2013 at 2:29 PM, <reveman@chromium.org> wrote:

> GpuMemoryBuffer sgtm.
>
>
https://codereview.chromium.**org/13543007/<https://codereview.chromium.org/1...
>



-- 
Thanks,
Kaan

[no sievers, 2013-04-05 22:16:24]

lgtm, just a few nits.

[no sievers, 2013-04-05 22:16:34]

https://codereview.chromium.org/13543007/diff/30005/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/13543007/diff/30005/ui/gl/gl_image.h#newcode40
ui/gl/gl_image.h:40: gfx::GpuMemoryBufferHandle buffer, const gfx::Size& size);
nit: gfx::Size by value

https://codereview.chromium.org/13543007/diff/30005/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/13543007/diff/30005/ui/gl/gl_image_egl.cc#new...
ui/gl/gl_image_egl.cc:8: 
nit: TODO: <...third_party headers etc...>

https://codereview.chromium.org/13543007/diff/30005/ui/gl/gl_image_egl.h
File ui/gl/gl_image_egl.h (right):

https://codereview.chromium.org/13543007/diff/30005/ui/gl/gl_image_egl.h#newc...
ui/gl/gl_image_egl.h:16: explicit GLImageEGL(const gfx::Size& size);
nit: gfx::Size by value

https://codereview.chromium.org/13543007/diff/39001/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/13543007/diff/39001/ui/gl/gl_image_egl.cc#new...
ui/gl/gl_image_egl.cc:50: glEGLImageTargetTexture2DOES(GL_TEXTURE_2D,
egl_image_);
Call glGetError() and return false here if not GL_NO_ERROR.

Looks like the error suppressor pops all errors before coming in here, so the
error is guaranteed to come from this gl call.

[kaanb, 2013-04-06 00:33:20]

adding sky@ as reviewer for ui/gfx/native_widget_types.h per danakj@'s
suggestion

[sky, 2013-04-08 14:45:09]

LGTM

[kaanb, 2013-04-08 15:48:11]

Thanks for the reviews, I'll try to commit.

https://codereview.chromium.org/13543007/diff/30005/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/13543007/diff/30005/ui/gl/gl_image.h#newcode40
ui/gl/gl_image.h:40: gfx::GpuMemoryBufferHandle buffer, const gfx::Size& size);
On 2013/04/05 22:16:34, Daniel Sievers wrote:
> nit: gfx::Size by value

Done.

https://codereview.chromium.org/13543007/diff/30005/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/13543007/diff/30005/ui/gl/gl_image_egl.cc#new...
ui/gl/gl_image_egl.cc:8: 
On 2013/04/05 22:16:34, Daniel Sievers wrote:
> nit: TODO: <...third_party headers etc...>

Done.

https://codereview.chromium.org/13543007/diff/30005/ui/gl/gl_image_egl.h
File ui/gl/gl_image_egl.h (right):

https://codereview.chromium.org/13543007/diff/30005/ui/gl/gl_image_egl.h#newc...
ui/gl/gl_image_egl.h:16: explicit GLImageEGL(const gfx::Size& size);
On 2013/04/05 22:16:34, Daniel Sievers wrote:
> nit: gfx::Size by value

Done.

https://codereview.chromium.org/13543007/diff/39001/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/13543007/diff/39001/ui/gl/gl_image_egl.cc#new...
ui/gl/gl_image_egl.cc:50: glEGLImageTargetTexture2DOES(GL_TEXTURE_2D,
egl_image_);
On 2013/04/05 22:16:34, Daniel Sievers wrote:
> Call glGetError() and return false here if not GL_NO_ERROR.
> 
> Looks like the error suppressor pops all errors before coming in here, so the
> error is guaranteed to come from this gl call.

Done.

[commit-bot: I haz the power, 2013-04-08 15:48:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/13543007/44001

[commit-bot: I haz the power, 2013-04-08 15:56:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-08 17:29:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/13543007/55002

[commit-bot: I haz the power, 2013-04-08 18:09:50]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-08 19:25:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/13543007/57003

[no sievers, 2013-04-08 20:02:52]

https://codereview.chromium.org/13543007/diff/57003/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):

https://codereview.chromium.org/13543007/diff/57003/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:7919:
.WillOnce(Return(GL_NO_ERROR))
EXPECT_CALL(*gl_, GetError())
.WillOnce(Return(GL_NO_ERROR))
.WillOnce(Return(GL_NO_ERROR))
.RetiresOnSaturation(); here and below

[commit-bot: I haz the power, 2013-04-09 00:41:09]

Change committed as 192971