956993 - Add SharedSurface backend that uses 'external' EGLImages

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Currently, we use regular EGLImageTargets, which make a GLTexture into a sibling of an EGLImage. Notably, the act of making this association causes the pixel data of the EGLImage to become undefined.

Also, some desktop implementations of EGL don't support GL_OES_EGL_image, but do support GL_OES_EGL_image_external.

Comment 1

[Chiajung Hung [:chiajung]]

Some specs:
[1] http://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image_external.txt
[2] http://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_image_base.txt
[3] https://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_image.txt

[1] depend on [2] or [3], where [2] should provide EGL_IMAGE_PRESERVED_KHR flag when create EGLImage, but [3] not. And GL_OES_EGL_image_external is basically used for special color format, if what you want is to avoid data upload via glTexImage2D, you may consider the PBO and mmap functions provided by GLES3/GL3. 

Or we may try to create EGLImage against a client buffer first, then fill the client buffer, and then bind the EGLImage as texture. This method may deadlock since we do not know when GL driver really lock the client buffer.

I have some more basic problems about this implementation: 
(1)On Android, EGLImage is created against a IPC compatible GraphicBuffer, so we can prepare the content in any process then use it as texture at any other process, how can we simulate the IPC part on desktop? If normal IPC involved, we have to memcpy the data into an EGLImage compatible buffer, then upload. In this case, the only usage is for video rendering, and video texture, right? (avoid color space conversion)

(2)Following(1). The spec [1] does not export any API to determine what color space the GPU really supports. How to prepare the content of EGLImage from other library such as image/video decoder?

Comment 2

[Kelsey Gilbert [:jgilbert]]

Ah, ok, it was buried in EGL_KHR_image_base, and I didn't see it the first time through:
Applications may create client API resources from an EGLImageKHR using
    client API extensions outside the scope of this document (such as
    GL_OES_EGL_image, which creates OpenGL ES texture and renderbuffer
    objects). If the EGLImageKHR used to create the client resource was
    created with the EGL_IMAGE_PRESERVED_KHR attribute set to EGL_TRUE, then
    the pixel data values associated with the image will be preserved after
    creating the client resource; otherwise, the pixel data values will be
    undefined. If the EGLImageKHR was created with the
    EGL_IMAGE_PRESERVED_KHR attribute set to EGL_TRUE, and EGL is unable to
    create the client resource without modifying the pixel values, then
    creation will fail and the pixel data values will be preserved.

Basically, EGL_KHR_image_base's EGL_IMAGE_PRESERVED can change the behavior of OES_EGL_image such that glEGLImageTargetTexture2D doesn't cause the EGLImage's pixel data to become undefined.

We still might want this for drivers that supply EGLImage-external but not EGLImage. It also better expresses to the driver that we're a pure-consumer, and won't want to modify the data.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Rely on OES_EGL_image_external for SharedSurface_EGLImage

This seems to work at least on the one device I tested on (Nexus 4).

I removed all the pipe setup code, since that's not needed anymore.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8383301 [details] [diff] [review]
Rely on OES_EGL_image_external for SharedSurface_EGLImage

Review of attachment 8383301 [details] [diff] [review]:
-----------------------------------------------------------------

Would be r-, but it's small things, overall f+.

::: gfx/gl/SharedSurfaceEGL.cpp
@@ +40,5 @@
>  
> +    EGLint attribs[] = {
> +        LOCAL_EGL_IMAGE_PRESERVED_KHR, LOCAL_EGL_TRUE,
> +        LOCAL_EGL_NONE, LOCAL_EGL_NONE
> +    };

Don't try this. It can refuse to create an EGLImage if it doesn't want to do this or doesn't support the right extension. (which I don't think you added anyways)

Luckily, we don't need this, since it was only a problem for when we attached the consumer side of this after drawing in the producer. Just remove these lines.

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +515,5 @@
>        SharedSurface_EGLImage* eglImageSurf =
>            SharedSurface_EGLImage::Cast(sharedSurf);
>  
> +      eglImageSurf->AcquireConsumerTexture(gl(), &mTextureHandle, &mTextureTarget);
> +      MOZ_ASSERT(mTextureHandle);

Don't you need to set mTextureTarget to EXTERNAL here?

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> 
> ::: gfx/layers/opengl/TextureHostOGL.cpp
> @@ +515,5 @@
> >        SharedSurface_EGLImage* eglImageSurf =
> >            SharedSurface_EGLImage::Cast(sharedSurf);
> >  
> > +      eglImageSurf->AcquireConsumerTexture(gl(), &mTextureHandle, &mTextureTarget);
> > +      MOZ_ASSERT(mTextureHandle);
> 
> Don't you need to set mTextureTarget to EXTERNAL here?

We do, but it's a little wonky. AcquireConsumerTexture() sets it via &mTextureTarget being passed in.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Rely on OES_EGL_image_external for SharedSurface_EGLImage

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Rely on OES_EGL_image_external for SharedSurface_EGLImage

I'm a moron

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8383313 [details] [diff] [review]
Rely on OES_EGL_image_external for SharedSurface_EGLImage

Review of attachment 8383313 [details] [diff] [review]:
-----------------------------------------------------------------

R- so we don't forget about removing this unneeded extension from the requirements here.

::: gfx/gl/SharedSurfaceEGL.cpp
@@ +63,1 @@
>             gl->IsExtensionSupported(GLContext::OES_EGL_image);

I think we actually don't need this extension anymore. We only need OES_EGL_image* extensions if we're using their respective versions of glEGLImageTextureTarget2D, and we're only using the EXTERNAL version now.

@@ +177,5 @@
>      return mEGL->Display();
>  }
>  
> +void
> +SharedSurface_EGLImage::AcquireConsumerTexture(GLContext* consGL, GLuint* texture, GLuint* target)

I would really prefer outvars would be explicit. Something like out_texture and out_target. This makes it much more obvious from the function declaration, and here in the definition. I don't think moz-style says anything about this, though.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Rely on OES_EGL_image_external for SharedSurface_EGLImage

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8383359 [details] [diff] [review]
Rely on OES_EGL_image_external for SharedSurface_EGLImage

Review of attachment 8383359 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0b4c5c28fb0

Comment 12

[Phil Ringnalda (:philor)]

So far, mochitest-7 (https://tbpl.mozilla.org/php/getParsedLog.php?id=35397313&tree=Mozilla-Inbound) and mochitest-8 (https://tbpl.mozilla.org/php/getParsedLog.php?id=35397603&tree=Mozilla-Inbound) have finished running.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/416792ed8ef5

Comment 13

[Oleg Romashin (:romaxa)]

OpenGL version detected: 200
OpenGL vendor: Qualcomm
OpenGL renderer: Adreno (TM) 305
OpenGL vendor ('Qualcomm') recognized as: Qualcomm
Extensions: GL_EXT_debug_marker GL_AMD_compressed_ATC_texture GL_AMD_performance_monitor GL_AMD_program_binary_Z400 GL_EXT_robustness GL_EXT_texture_format_BGRA8888 GL_EXT_texture_type_2_10_10_10_REV GL_NV_fence GL_OES_compressed_ETC1_RGB8_texture GL_OES_depth_texture GL_OES_depth24 GL_OES_EGL_image GL_OES_EGL_image_external GL_OES_element_index_uint GL_OES_fbo_render_mipmap GL_OES_fragment_precision_high GL_OES_get_program_binary GL_OES_packed_depth_stencil GL_OES_rgb8_rgba8 GL_OES_standard_derivatives GL_OES_texture_3D GL_OES_texture_float GL_OES_texture_half_float GL_OES_texture_half_float_linear GL_OES_texture_npot GL_OES_vertex_half_float GL_OES_vertex_type_10_10_10_2 GL_OES_vertex_array_object GL_QCOM_alpha_test GL_QCOM_binning_control GL_QCOM_driver_control GL_QCOM_perfmon_global_mode GL_QCOM_extended_get GL_QCOM_extended_get2 GL_QCOM_tiled_rendering GL_QCOM_writeonly_rendering GL_EXT_sRGB 
Found extension GL_AMD_compressed_ATC_texture
Found extension GL_EXT_robustness
Found extension GL_EXT_texture_format_BGRA8888
Found extension GL_OES_depth_texture
Found extension GL_OES_depth24
Found extension GL_OES_EGL_image
Found extension GL_OES_EGL_image_external
Found extension GL_OES_element_index_uint
Found extension GL_OES_packed_depth_stencil
Found extension GL_OES_rgb8_rgba8
Found extension GL_OES_standard_derivatives
Found extension GL_OES_texture_float
Found extension GL_OES_texture_half_float
Found extension GL_OES_texture_half_float_linear
Found extension GL_OES_texture_npot
Found extension GL_OES_vertex_array_object
Found extension GL_EXT_sRGB
[23937] WARNING: Transparent content with displayports can be expensive.: file layout/base/nsDisplayList.cpp, line 1256
Created LOG for EmbedLiteCompositorParent
[23937] WARNING: Transparent content with displayports can be expensive.: file layout/base/nsDisplayList.cpp, line 1256
OpenGL version detected: 200
OpenGL vendor: Qualcomm
OpenGL renderer: Adreno (TM) 305
GL ERROR: void mozilla::gl::GLContext::raw_fGetIntegerv(GLenum, GLint*) generated GL error GL_INVALID_ENUM(0x0500)
GL ERROR: void mozilla::gl::GLContext::raw_fGenTextures(GLsizei, GLuint*) generated GL error GL_INVALID_OPERATION(0x0502)
GL ERROR: void mozilla::gl::GLContext::raw_fGetIntegerv(GLenum, GLint*) generated GL error GL_INVALID_ENUM(0x0500)

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Don't move surfaces around if we can't allocate a new one first

We need this additional patch, because now SharedSurfaceEGL::Create() will return null on devices without the extension. SurfaceStream was not accounting for this, and moved mProducer and mStaging around. When we fall back to the basic factory, we end up moving those surfaces *again*, throwing good stuff away in the process.

We might want to make SurfaceFactory_EGLImage::Create() fallible, to avoid this type of common failure case.

Comment 15

[Kelsey Gilbert [:jgilbert]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #14)
> Created attachment 8383833 [details] [diff] [review]
> Don't move surfaces around if we can't allocate a new one first
> 
> We need this additional patch, because now SharedSurfaceEGL::Create() will
> return null on devices without the extension. SurfaceStream was not
> accounting for this, and moved mProducer and mStaging around. When we fall
> back to the basic factory, we end up moving those surfaces *again*, throwing
> good stuff away in the process.
It doesn't look like the code does this, though, since the majority of it (specifically prod->staging promotion) doesn't run if mProd is null, as it would be if we failed to create a surface.

> 
> We might want to make SurfaceFactory_EGLImage::Create() fallible, to avoid
> this type of common failure case.

Yep, making the factory fallible is fine. We already do this for some other factories.

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8383833 [details] [diff] [review]
Don't move surfaces around if we can't allocate a new one first

Review of attachment 8383833 [details] [diff] [review]:
-----------------------------------------------------------------

I want to talk more about the premise for this, which I think is flawed.

::: gfx/gl/SurfaceStream.cpp
@@ +318,5 @@
>      if (mProducer) {
> +        SharedSurface* nextProd = nullptr;
> +        New(factory, size, nextProd);
> +        if (!nextProd) {
> +            // No point in continuing if we can't allocate a new surface

We still want to promote the previous (successfully-allocated) mProd to mStaging.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #15)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #14)
> > Created attachment 8383833 [details] [diff] [review]
> > Don't move surfaces around if we can't allocate a new one first
> > 
> > We need this additional patch, because now SharedSurfaceEGL::Create() will
> > return null on devices without the extension. SurfaceStream was not
> > accounting for this, and moved mProducer and mStaging around. When we fall
> > back to the basic factory, we end up moving those surfaces *again*, throwing
> > good stuff away in the process.
> It doesn't look like the code does this, though, since the majority of it
> (specifically prod->staging promotion) doesn't run if mProd is null, as it
> would be if we failed to create a surface.
> 

We do have a mProd, though, because we started off the first frame (with WebGL) with a basic factory. So after that is morphed to a EGLImage one, we hit this case.

> > 
> > We might want to make SurfaceFactory_EGLImage::Create() fallible, to avoid
> > this type of common failure case.
> 
> Yep, making the factory fallible is fine. We already do this for some other
> factories.

Ah, ok. That's a better way to fix this, then.

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Rely on OES_EGL_image_external for SharedSurface_EGLImage

This one makes the factory fail if we lack the required extensions

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Rely on OES_EGL_image_external for SharedSurface_EGLImage

One more tweak here. Always disable SkiaGL on Android for API version < 11. Pretty sure we won't have the necessary extensions there, which means we'd use the basic readback surfaces. Software will be faster than that.

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccf27e254ed8

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ccf27e254ed8

Comment 22

[Oleg Romashin (:romaxa)]

Comment on attachment 8385810 [details] [diff] [review]
Rely on OES_EGL_image_external for SharedSurface_EGLImage


>+                         : (aTarget == LOCAL_GL_TEXTURE_EXTERNAL) ? LOCAL_GL_TEXTURE_EXTERNAL
>                          : LOCAL_GL_NONE;
>     MOZ_ASSERT(bindingTarget != LOCAL_GL_NONE);
>     mGL->GetUIntegerv(bindingTarget, &mOldTex);

This looks wrong, should not it be LOCAL_GL_TEXTURE_BINDING_EXTERNAL ?

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Oleg Romashin (:romaxa) from comment #22)
> Comment on attachment 8385810 [details] [diff] [review]
> Rely on OES_EGL_image_external for SharedSurface_EGLImage
> 
> 
> >+                         : (aTarget == LOCAL_GL_TEXTURE_EXTERNAL) ? LOCAL_GL_TEXTURE_EXTERNAL
> >                          : LOCAL_GL_NONE;
> >     MOZ_ASSERT(bindingTarget != LOCAL_GL_NONE);
> >     mGL->GetUIntegerv(bindingTarget, &mOldTex);
> 
> This looks wrong, should not it be LOCAL_GL_TEXTURE_BINDING_EXTERNAL ?

Indeed, I think you're right. I wonder how it's working without that...

Comment 24

[Oleg Romashin (:romaxa)]

I noticed that some drivers don't push error and some does... probably you hit driver which does not

Comment 25

[Kelsey Gilbert [:jgilbert]]

(In reply to Oleg Romashin (:romaxa) from comment #24)
> I noticed that some drivers don't push error and some does... probably you
> hit driver which does not

Yep, some drivers try to do what you mean even if it's wrong.

Comment 26

[Kelsey Gilbert [:jgilbert]]

(In reply to Oleg Romashin (:romaxa) from comment #22)
> Comment on attachment 8385810 [details] [diff] [review]
> Rely on OES_EGL_image_external for SharedSurface_EGLImage
> 
> 
> >+                         : (aTarget == LOCAL_GL_TEXTURE_EXTERNAL) ? LOCAL_GL_TEXTURE_EXTERNAL
> >                          : LOCAL_GL_NONE;
> >     MOZ_ASSERT(bindingTarget != LOCAL_GL_NONE);
> >     mGL->GetUIntegerv(bindingTarget, &mOldTex);
> 
> This looks wrong, should not it be LOCAL_GL_TEXTURE_BINDING_EXTERNAL ?

Yep, this is correct. My bad for not catching it.

Comment 27

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix typo with GL_TEXTURE_EXTERNAL textures

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbaed89f36e9

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dbaed89f36e9