Closed Bug 956993 Opened 6 years ago Closed 6 years ago

Add SharedSurface backend that uses 'external' EGLImages

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jgilbert, Assigned: snorp)

References

(Depends on 1 open bug)

Details

(Whiteboard: webgl-next)

Attachments

(2 files, 6 obsolete files)

15.32 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
1.24 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
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.
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?
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.
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.
Assignee: nobody → snorp
Attachment #8383301 - Flags: feedback?(vladimir)
Attachment #8383301 - Flags: feedback?(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?
Attachment #8383301 - Flags: feedback?(jgilbert) → feedback+
(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.
Attachment #8383301 - Attachment is obsolete: true
Attachment #8383301 - Flags: feedback?(vladimir)
Attachment #8383309 - Flags: review?(jgilbert)
I'm a moron
Attachment #8383309 - Attachment is obsolete: true
Attachment #8383309 - Flags: review?(jgilbert)
Attachment #8383313 - Flags: review?(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.
Attachment #8383313 - Flags: review?(jgilbert) → review-
Attachment #8383313 - Attachment is obsolete: true
Attachment #8383359 - Flags: review?(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/
Attachment #8383359 - Flags: review?(jgilbert) → review+
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)
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.
Attachment #8383833 - Flags: review?(jgilbert)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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 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.
Attachment #8383833 - Flags: review?(jgilbert) → review-
(In reply to Jeff Gilbert [:jgilbert] from comment #15)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
This one makes the factory fail if we lack the required extensions
Attachment #8383359 - Attachment is obsolete: true
Attachment #8383833 - Attachment is obsolete: true
Attachment #8384599 - Flags: review?(jgilbert)
Attachment #8384599 - Flags: review?(jgilbert) → review+
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.
Attachment #8384599 - Attachment is obsolete: true
Attachment #8385810 - Flags: review?(jgilbert)
Attachment #8385810 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/ccf27e254ed8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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 ?
(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...
I noticed that some drivers don't push error and some does... probably you hit driver which does not
(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.
(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.
Attachment #8387018 - Flags: review?(jgilbert) → review+
Depends on: 1020390
You need to log in before you can comment on or make changes to this bug.