Use glTexSubImage2D while binding with external texture for map-image. (issue 216873003)

[sohan...@samsung.com]

Reviewers: reveman,

Message:
PTAL. Thanks


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

https://codereview.chromium.org/216873003/diff/1/ui/gl/gl_image_shm.cc#newcode142
ui/gl/gl_image_shm.cc:142: if (egl_image_ == EGL_NO_IMAGE_KHR) {
Should we add an extra check for egl_texture_id_ being 0.

Description:
Use glTexSubImage2D while binding with GL_TEXTURE_EXTERNAL_OES for
map-image.

In GLImageShm, use glTexSub instead of glTex during binding,
as it is more efficient and avoids gpu-workarounds.


BUG=357493

Please review this at https://codereview.chromium.org/216873003/

SVN Base: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+43, -34 lines):
M ui/gl/gl_image_shm.cc


Index: ui/gl/gl_image_shm.cc
diff --git a/ui/gl/gl_image_shm.cc b/ui/gl/gl_image_shm.cc
index
081623ed93d175a7fe593308c641ef4125cb1af6..895c389a4c410eac4796ceb90b4600d3e275e2d5
100644
--- a/ui/gl/gl_image_shm.cc
+++ b/ui/gl/gl_image_shm.cc
@@ -139,45 +139,54 @@ bool GLImageShm::BindTexImage(unsigned target) {
#if defined(OS_WIN) || defined(USE_X11) || defined(OS_ANDROID) || \
defined(USE_OZONE)
if (target == GL_TEXTURE_EXTERNAL_OES) {
- if (egl_image_ != EGL_NO_IMAGE_KHR)
- eglDestroyImageKHR(GLSurfaceEGL::GetHardwareDisplay(), egl_image_);
-
- if (!egl_texture_id_)
+ if (egl_image_ == EGL_NO_IMAGE_KHR) {
glGenTextures(1, &egl_texture_id_);

- {
+ {
+ ScopedTextureBinder texture_binder(GL_TEXTURE_2D, egl_texture_id_);
+ glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
+ glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S,
GL_CLAMP_TO_EDGE);
+ glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T,
GL_CLAMP_TO_EDGE);
+
+ glTexImage2D(GL_TEXTURE_2D,
+ 0, // mip level
+ TextureFormat(internalformat_),
+ size_.width(),
+ size_.height(),
+ 0, // border
+ DataFormat(internalformat_),
+ DataType(internalformat_),
+ shared_memory_->memory());
+ }
+
+ EGLint attrs[] = {EGL_GL_TEXTURE_LEVEL_KHR, 0,
EGL_IMAGE_PRESERVED_KHR,
+ EGL_TRUE, EGL_NONE};
+ // Need to pass current EGL rendering context to eglCreateImageKHR
for
+ // target type EGL_GL_TEXTURE_2D_KHR.
+ egl_image_ =
+ eglCreateImageKHR(GLSurfaceEGL::GetHardwareDisplay(),
+ eglGetCurrentContext(),
+ EGL_GL_TEXTURE_2D_KHR,
+
reinterpret_cast<EGLClientBuffer>(egl_texture_id_),
+ attrs);
+ DCHECK_NE(EGL_NO_IMAGE_KHR, egl_image_)
+ << "Error creating EGLImage: " << eglGetError();
+
+ glEGLImageTargetTexture2DOES(target, egl_image_);
+ DCHECK_EQ(static_cast<GLenum>(GL_NO_ERROR), glGetError());
+ } else {
ScopedTextureBinder texture_binder(GL_TEXTURE_2D, egl_texture_id_);
- glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
- glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
- glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
-
- glTexImage2D(GL_TEXTURE_2D,
- 0, // mip level
- TextureFormat(internalformat_),
- size_.width(),
- size_.height(),
- 0, // border
- DataFormat(internalformat_),
- DataType(internalformat_),
- shared_memory_->memory());
+ glTexSubImage2D(GL_TEXTURE_2D,
+ 0, // mip level
+ 0, // x-offset
+ 0, // y-offset
+ size_.width(),
+ size_.height(),
+ DataFormat(internalformat_),
+ DataType(internalformat_),
+ shared_memory_->memory());
}

- EGLint attrs[] = {EGL_GL_TEXTURE_LEVEL_KHR, 0, EGL_IMAGE_PRESERVED_KHR,
- EGL_TRUE, EGL_NONE};
- // Need to pass current EGL rendering context to eglCreateImageKHR for
- // target type EGL_GL_TEXTURE_2D_KHR.
- egl_image_ =
- eglCreateImageKHR(GLSurfaceEGL::GetHardwareDisplay(),
- eglGetCurrentContext(),
- EGL_GL_TEXTURE_2D_KHR,
-
reinterpret_cast<EGLClientBuffer>(egl_texture_id_),
- attrs);
- DCHECK_NE(EGL_NO_IMAGE_KHR, egl_image_)
- << "Error creating EGLImage: " << eglGetError();
-
- glEGLImageTargetTexture2DOES(target, egl_image_);
- DCHECK_EQ(static_cast<GLenum>(GL_NO_ERROR), glGetError());
-
shared_memory_->Unmap();
return true;
}

[rev...@chromium.org]

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

https://codereview.chromium.org/216873003/diff/1/ui/gl/gl_image_shm.cc#newcode142
ui/gl/gl_image_shm.cc:142: if (egl_image_ == EGL_NO_IMAGE_KHR) {

On 2014/03/28 12:36:20, sohanjg wrote:
> Should we add an extra check for egl_texture_id_ being 0.

Add a DCHECK_EQ(0, ..) check.

https://codereview.chromium.org/216873003/diff/1/ui/gl/gl_image_shm.cc#newcode175
ui/gl/gl_image_shm.cc:175: glEGLImageTargetTexture2DOES(target,
egl_image_);
You need to do this in the case below too. You're assuming that the
client will always attach the image to the same target and id otherwise.

https://codereview.chromium.org/216873003/

[rev...@chromium.org]

lgtm thanks!

+piman

https://codereview.chromium.org/216873003/

[pi...@chromium.org]

LGTM if it works around a bug, but I'm not positive that glTexSubImage2D is
better than glTexImage2D, quite often it is the opposite: glTexImage2D can
do
texture renaming because it replaces the entire image, so it doesn't have to
wait until the GPU is done with the texture, it can allocate a new one.
OTOH,
glTexSubImage2D requires either blocking on the GPU or pipelining the update
which involves another copy, unless the driver has a special case
to "promote" a
glTexSubImage2D into a glTexImage2D when you are specifying the entire
image.
Either way, it's likely to be driver specific. We actually have
a "workaround"
entry in the blacklist stuff ("texsubimage2d_faster_than_teximage2d"), which
ideally we'd set on all drivers we know it's true.

https://codereview.chromium.org/216873003/

[rev...@chromium.org]

+epenner

https://codereview.chromium.org/216873003/

[rev...@chromium.org]

I think this is a somehow different situation and specific to the use of EGL
images and EGL_KHR_gl_image [1].
The EGLImage is orphaned after a call to glTexImage2D, which means that we
have
to create a new EGLImage every time. We have similar logic in our EGL based
async upload manager and if I remember correctly it was more efficient to
use
TexSubImage2D instead of calling TexImage2D and re-creating the EGL image
for
each upload. Eric is the expert here and can probably provide some more
details.

In regards to the workaround, it seems like some drivers are unhappy with us
reusing the texture id after deleting an image. That's the problem we're
avoiding with this patch but we could also just delete the texture id and
create
a new one for each upload instead. Fixing this is not critical as this code
is
mostly for completeness and testing purposes right now. I'm interested in
using
it for my "staging pool" work though. And in that case, avoiding driver
bugs and
getting good performance is important. This patch would help in both
aspects,
which is why I'm interested in landing this rather than a DeleteTextures
workaround that can only be explained with "because drivers are not happy
otherwise"..

[1] http://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_gl_image.txt

https://codereview.chromium.org/216873003/

[epe...@chromium.org]

Reveman's description sums up my take on it. EGLImage object creation can be
expensive, and in some cases it wasn't even possible to recreate them
repeatedly
on the same texture. So basically after you create one, your only (good)
option
is to use glTexSubImage2D on them. The performance w.r.t. synchronization
is not
as relevant if you have another thread involved.

For full updates in other cases indeed glTexImage2D is much better in a lot
of
cases, and we have a flag for flipping most uploads to glTexImage2D.

However I have two questions about this code:
1.) Why is the target GL_TEXTURE_EXTERNAL_OES? I don't believe we can create
EXTERNAL_OES textures outside of Gralloc/SurfaceTexture, and I thought it
was
impossible to copy/modify them via any GL function. EGLImage can use
TEXTURE_2D
binding just fine.

2.) If the 'image' is shared-memory in this case, why do we need an
EGL 'image'?
I'm missing why we need an EGLImage at all in this case.

https://codereview.chromium.org/216873003/

[rev...@chromium.org]

On 2014/03/28 23:19:44, epenner wrote:
> Reveman's description sums up my take on it. EGLImage object creation can
> be
> expensive, and in some cases it wasn't even possible to recreate them
repeatedly
> on the same texture. So basically after you create one, your only (good)
option
> is to use glTexSubImage2D on them. The performance w.r.t. synchronization
> is
not
> as relevant if you have another thread involved.

> For full updates in other cases indeed glTexImage2D is much better in a
> lot of
> cases, and we have a flag for flipping most uploads to glTexImage2D.

> However I have two questions about this code:
> 1.) Why is the target GL_TEXTURE_EXTERNAL_OES? I don't believe we can
> create
> EXTERNAL_OES textures outside of Gralloc/SurfaceTexture, and I thought it
> was
> impossible to copy/modify them via any GL function. EGLImage can use
TEXTURE_2D
> binding just fine.

This is to ensure that GLImageShm works with all possible configurations.
Right
now, it's mostly for testing purposes but it allows the compositor to use
TEXTURE_EXTERNAL_OES target independent of what zero-copy implementations
available. ie. if gralloc or surface texture's are not available it can
still
produce correct output using GLImageShm.

EGL_GL_TEXTURE_2D_KHR backed EGLImage's seem to work fine with
TEXTURE_EXTERNAL_OES target on the devices Sohan and I have tested this on
so
far. I don't see why it wouldn't work. Are we missing something?

> 2.) If the 'image' is shared-memory in this case, why do we need an EGL
'image'?
> I'm missing why we need an EGLImage at all in this case.

We just need the EGLImage to support the TEXTURE_EXTERNAL_OES target.
Mostly for
completeness and testing purposes right now but I think it's also possible
that
we can use this as a fall-back instead of the current async upload system
when
surface texture's are not available.

https://codereview.chromium.org/216873003/

[epe...@chromium.org]

> I don't see why it wouldn't work. Are we missing something?

I think I get what you are going for now, but here's the problem:
https://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image_external.txt

"This texture target can only be specified using an EGLImage. There is no
support for most of the functions that manipulate other texture targets
(e.g.
you cannot use gl*Tex*Image*() functions with TEXTURE_EXTERNAL_OES). ...
an
INVALID_ENUM error will be generated if this is attempted"

So you aren't supposed to be able to modify the texture at all while using
this
target. The idea with EXTERNAL_OES is that the texture is created in an
external
producer in some unknown format that can't be modified. All you can do is
bind
an EGLImage that was created elsewhere to this target, and that's it.

One crazy way to achieve what you want would be to bind the texture to a 2D
target, create the EGLImage, then bind the EGLImage to an EXTERNAL_OES
target as
well. For modification you can now use the 2D target, while reading you
could
use EXTERNAL_OES... However, this is in theory as in practice that is a
pretty
crazy use of the API, and I doubt they would expect or support this.

Perhaps another way would be to expose a 'RealTarget()' from the texture or
GLImage, and just map the target back to 2D in this case?

> We just need the EGLImage to support the TEXTURE_EXTERNAL_OES target.
> Mostly
for
> completeness and testing purposes right now but I think it's also possible
that
> we can use this as a fall-back instead of the current async upload system
> when
> surface texture's are not available.

I get the testing purposes part, so as long as it works on some devices I
suppose this is fine. However, for production we would be way better off
just
using glMapTexImage for this fall-back, and I don't think it's worth all of
this
overhead (shared memory handles, mapping, EGLImage creation) just to have
the
same API in the client.

https://codereview.chromium.org/216873003/

[rev...@chromium.org]

On 2014/03/29 00:23:48, epenner wrote:
> > I don't see why it wouldn't work. Are we missing something?

> I think I get what you are going for now, but here's the problem:

https://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image_external.txt

> "This texture target can only be specified using an EGLImage. There is no
> support for most of the functions that manipulate other texture targets
> (e.g.
> you cannot use gl*Tex*Image*() functions with
> TEXTURE_EXTERNAL_OES). ... an
> INVALID_ENUM error will be generated if this is attempted"

I think we're in compliance with this as we never issue any gl*Tex*Image*()
calls for the texture bound to TEXTURE_EXTERNAL_OES.

We're using
http://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_gl_image.txt as
the
external producer mechanism. That extension explicitly specifies the
behavior of
gl*Tex*Image*() commands on the texture while used as an EGLImage. I can
only
make sense of this extension if that's transparent to
OES_EGL_image_external.

To make it clear. Here's our usage pattern:

glGenTextures(1, &texture_id);
glGenTextures(1, &egl_texture_id);
{
ScopedTextureBinder binder(GL_TEXTURE_2D, egl_texture_id);
glBindTexture(GL_TEXTURE_2D, egl_texture_id);
glTexParameteri(GL_TEXTURE_2D, ..);
glTexImage2D(GL_TEXTURE_2D, ..);
}
egl_image = eglCreateImageKHR(dpy, ctx, EGL_GL_TEXTURE_2D_KHR,
egl_texture_id,
..);

while (draw_more_frames) {
{
ScopedTextureBinder binder(GL_TEXTURE_2D, egl_texture_id);
glTexSubImage2D(GL_TEXTURE_2D, ..);
}

glBindTexture(GL_TEXTURE_EXTERNAL_OES, texture_id);
glEGLImageTargetTexture2DOES(GL_TEXTURE_EXTERNAL_OES, egl_image);

// Issue draw commands that sample from texture bound to
TEXTURE_EXTERNAL_OES
target.

}


> So you aren't supposed to be able to modify the texture at all while using
this
> target. The idea with EXTERNAL_OES is that the texture is created in an
external
> producer in some unknown format that can't be modified. All you can do is
> bind
> an EGLImage that was created elsewhere to this target, and that's it.

The external producer can modify the EGLImage though, right? In this case
EGL_KHR_gl_image is the producer.

I'm not sure I understand what you mean by using glMapTexImage.

The idea would be to use this with a fixed size staging pool controlled by
the
compositor. It would use GLImageSurfaceTexture when available but fallback
to
GLImageShm when not. GLImageShm could be using async uploads under the hood
as
it just needs to make sure the upload has finished at the time the texture
is
used for drawing (when GLImage::WillUseTexImage is called). Not sure how
well
this will work in practice but it will be easy to find out once we've
implemented the staging pool that is needed for surface textures.

https://codereview.chromium.org/216873003/

[epe...@google.com]

Okay, if you are binding the same texture to two different IDs and
uploading to
only the 2D one, that sounds okay. That's what I mentioned above, but from
your
usage pattern it looks like what you are already doing. I would be
concerned if
these two textures are both bound in the same context, but according to the
spec
it looks fine. Thanks for the clarification.

Regarding glMapTexImage, that uses shared memory and then does a synchronous
upload in the client, which is what it looks like this fallback does.
However
the threading of the upload at the very end might be slightly better in some
cases, indeed. The problem is we don't have completion notification, so the
client would use the texture immediately and cause the same stall as being
synchronous.

I think better would be to do all the operations via IPCs or something if we
want to do much better than synchronous uploads with glMapTexImage, but
since
this is also just a fallback for testing purposes, lgtm for that.

https://codereview.chromium.org/216873003/

[rev...@chromium.org]

On 2014/03/31 20:23:28, epennerAtGoogle wrote:
> Okay, if you are binding the same texture to two different IDs and
> uploading
to
> only the 2D one, that sounds okay. That's what I mentioned above, but from
your
> usage pattern it looks like what you are already doing. I would be
> concerned
if
> these two textures are both bound in the same context, but according to
> the
spec
> it looks fine. Thanks for the clarification.

> Regarding glMapTexImage, that uses shared memory and then does a
> synchronous
> upload in the client, which is what it looks like this fallback does.
> However
> the threading of the upload at the very end might be slightly better in
> some
> cases, indeed. The problem is we don't have completion notification, so
> the
> client would use the texture immediately and cause the same stall as being
> synchronous.

The difference is that we would have up until the time when the texture is
actually used on the gpu service side by the parent compositor to finish the
upload and we would never have to stall on prepaint uploads. The question is
whether this is good enough. One major advantage is of course that it would
reduce the complexity on the compositor side and unify our texture update
logic.

> I think better would be to do all the operations via IPCs or something if
> we
> want to do much better than synchronous uploads with glMapTexImage, but
> since
> this is also just a fallback for testing purposes, lgtm for that.

It will be easy to see how well it works once we add 0-copy staging
implementation of the RasterWorkerPool interface.

https://codereview.chromium.org/216873003/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sohan...@samsung.com/216873003/40001


https://codereview.chromium.org/216873003/

[commi...@chromium.org]

Try jobs failed on following builders:
tryserver.chromium on mac_chromium_rel

https://codereview.chromium.org/216873003/

[commi...@chromium.org]

[commi...@chromium.org]

Change committed as 260821

https://codereview.chromium.org/216873003/