GLImage support for Android zero-copy pixel buffers

ui/gl/gl_image_egl.cc

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/gl/gl_image_egl.h"
+
+#include "ui/gl/gl_bindings.h"
+
+#define EGL_NATIVE_BUFFER_EGL 0x3140

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

Is this supposed to be EGL_NATIVE_BUFFER_ANDROID 0x3140? Does this belong in
third_party/khronos/EGL/eglext.h instead?

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

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

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

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.

[kaanb, 2013/04/05 07:17:06]

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?

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

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

+
+namespace gfx {
+
+GLImageEGL::GLImageEGL(const gfx::Size& size)
+  : egl_image_(NULL),

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

EGL_NO_IMAGE_KHR instead of NULL?

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

Done.

+    size_(size) {
+}
+
+GLImageEGL::~GLImageEGL() {

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

call Destroy() here for consistency.

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

Done.

+}
+
+bool GLImageEGL::Initialize(gfx::PixelBufferHandle buffer) {
+  EGLClientBuffer cbuf = static_cast<EGLClientBuffer>(buffer);
+  EGLint attrs[] = {
+    EGL_IMAGE_PRESERVED_KHR, EGL_TRUE,
+    EGL_NONE,
+  };
+  egl_image_ = eglCreateImageKHR(eglGetDisplay(EGL_DEFAULT_DISPLAY),

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

nit: one argument per line if you can't fit them all in one line.

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

Done.

+      EGL_NO_CONTEXT, EGL_NATIVE_BUFFER_EGL, cbuf, attrs);
+
+  if (egl_image_ == EGL_NO_IMAGE_KHR) {
+    EGLint error = eglGetError();
+    LOG(ERROR) << "GLImageEGL - error creating EGLImage: "

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

nit: "GLImageEGL::Initialize: Error cre..."

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

Done.

+               << error;
+    return false;
+  }
+
+  return true;
+}
+
+bool GLImageEGL::BindTexImage() {
+  if (egl_image_ == EGL_NO_IMAGE_KHR) {
+    LOG(ERROR) << "GLImageEGL - null EGLImage in BindTexImage";

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

I don't think you need the GLImageEGL prefix here. Assuming this will
automatically include the function name.

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

Done.

+    return false;
+  }
+
+  glEGLImageTargetTexture2DOES(GL_TEXTURE_2D, egl_image_);

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

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?

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

Done.

+
+  GLint error;
+  while ((error = glGetError()) != GL_NO_ERROR) {

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

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?

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

Done.

+    return false;
+  }
+
+  EGLBoolean success = eglDestroyImageKHR(

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

Why destroy it here rather than in GLImageEGL::Destroy?

[no sievers, 2013/04/05 00:23:03]

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]

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]

Good question, I don't know the answer.

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

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?

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

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

+      eglGetDisplay(EGL_DEFAULT_DISPLAY), egl_image_);
+
+  if (success == EGL_FALSE) {
+    EGLint error = eglGetError();
+    LOG(ERROR) << "GLImageEGL - error destroying EGLImage: "

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

I don't think you need the GLImageEGL prefix here.

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

Done.

+               << error;
+    return false;
+  }
+
+  egl_image_ = EGL_NO_IMAGE_KHR;
+
+  return true;
+}
+
+gfx::Size GLImageEGL::GetSize() {
+  return size_;
+}
+
+void GLImageEGL::Destroy() {
+  return;

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

nit: redundant

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

Can we add a DCHECK_EQ(EGL_NO_IMAGE_KHR, egl_image_) here?

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

Done.

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

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

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

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.

+}
+
+void GLImageEGL::ReleaseTexImage() {
+  NOTREACHED();
+  return;

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

nit: "return" is redundant

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

Done.

+}
+
+}  // namespace gfx