Use glTexSubImage2D while binding with external texture for map-image.

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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260821

[sohanjg, 2014-03-28 12:36:20]

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

[reveman, 2014-03-28 13:34:17]

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

[reveman, 2014-03-28 14:28:54]

lgtm thanks!

+piman

[piman, 2014-03-28 17:17:39]

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.

[reveman, 2014-03-28 18:33:35]

+epenner

[reveman, 2014-03-28 18:34:09]

On 2014/03/28 17:17:39, piman wrote:
> 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.

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

[epenner, 2014-03-28 23:19:44]

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.

[reveman, 2014-03-28 23:50:26]

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.

[epenner, 2014-03-29 00:23:48]

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

[reveman, 2014-03-30 15:54:37]

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.

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

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.

[epennerAtGoogle, 2014-03-31 20:23:28]

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.

[reveman, 2014-03-31 22:06:04]

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.

[sohanjg, 2014-04-01 04:38:41]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2014-04-01 04:38:54]

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

[commit-bot: I haz the power, 2014-04-01 04:59:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 04:59:54]

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

[sohanjg, 2014-04-01 06:55:49]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2014-04-01 06:55:50]

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

[commit-bot: I haz the power, 2014-04-01 09:40:33]

Change committed as 260821

[sohanjg, 2015-11-18 09:29:00]

Description was changed from

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

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

to

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

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