1208513 - Resurrect SharedTexture_GLTexture for iOS

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

Shared contexts are apparently the best thing to use on iOS for sharing textures, so we need to bring back SharedSurface_GLTexture.

Comment 1

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

Attachment: Resurrect SharedSurface_GLTexture for use on iOS

I am not totally sure if this texture lifecycle stuff is correct. Can we
call ToSurfaceDescriptor() more than once on a SharedSurface?

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8666043 [details] [diff] [review]
Resurrect SharedSurface_GLTexture for use on iOS

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

s/GLTextureShare/SharedGLTexture/g

Also delete the crufty consGL stuff. I'll write a patch elsewhere to get rid of the deprecated Fence/WaitSync/PollSync functions.

::: gfx/gl/SharedSurfaceGL.cpp
@@ +158,5 @@
> +{
> +    MutexAutoLock lock(mMutex);
> +    mGL->MakeCurrent();
> +
> +    if (mConsGL && mGL->IsExtensionSupported(GLContext::ARB_sync)) {

You're going to need to add a GLFeature for FenceSync and add the APPLE extension to it.

@@ +242,5 @@
> +bool
> +SharedSurface_GLTexture::ToSurfaceDescriptor(layers::SurfaceDescriptor* const out_descriptor)
> +{
> +    *out_descriptor = layers::GLTextureShareDescriptor((uintptr_t)mTex,
> +                                                       (uintptr_t)mSync,

You shouldn't need to cast mTex here, since it should coerce fine into the uint32_t that it should be.

@@ +248,5 @@
> +                                                       mHasAlpha);
> +
> +    // Ownership of the texture is transferred to the host at this point
> +    mTex = 0;
> +    mSync = 0;

This isn't acceptable. We recycle ShSurfs, so on recycle, this would break horribly.

We can transfer ownership, but we can't zero them. We just need to not delete them.

::: gfx/gl/SharedSurfaceGL.h
@@ +99,5 @@
> +    : public SharedSurface
> +{
> +public:
> +    static UniquePtr<SharedSurface_GLTexture> Create(GLContext* prodGL,
> +                                                     GLContext* consGL,

We don't use consGL anymore, so get rid of it here and elsewhere in the patch.

@@ +141,5 @@
> +    virtual void UnlockProdImpl() override {}
> +
> +    virtual void Fence() override;
> +    virtual bool WaitSync() override;
> +    virtual bool PollSync() override;

Override these three with MOZ_CRASH, and instead override the acquire/release functions.

::: gfx/gl/SurfaceTypes.h
@@ +76,5 @@
>      DXGLInterop2,
>      Gralloc,
>      IOSurface,
>      GLXDrawable,
> +    GLTextureShare,

SharedGLTexture

::: gfx/layers/client/ClientCanvasLayer.cpp
@@ +94,5 @@
>  #elif defined(GL_PROVIDER_GLX)
>          if (sGLXLibrary.UseTextureFromPixmap())
>            factory = SurfaceFactory_GLXDrawable::Create(mGLContext, caps, forwarder, mFlags);
> +#elif defined(MOZ_WIDGET_UIKIT)
> +        GLContext* nullConsGL = nullptr; // Bug 1050044.

This should be fine to skip now.

::: gfx/layers/ipc/LayersSurfaces.ipdlh
@@ +82,5 @@
>    bool hasAlpha;
>  };
>  
> +struct GLTextureShareDescriptor {
> +  int32_t texture;

uint32_t

::: gfx/layers/opengl/TextureHostOGL.h
@@ +320,5 @@
> +  virtual const char* Name() override { return "GLTextureHost"; }
> +
> +protected:
> +  const GLuint mTexture;
> +  const GLenum mTarget;

Why is mTarget here if mTarget isn't communicated by the ipdl?

Comment 3

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

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 8666043 [details] [diff] [review]
> Resurrect SharedSurface_GLTexture for use on iOS
> 
> Review of attachment 8666043 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> s/GLTextureShare/SharedGLTexture/g
> 
> Also delete the crufty consGL stuff. I'll write a patch elsewhere to get rid
> of the deprecated Fence/WaitSync/PollSync functions.
> 
> ::: gfx/gl/SharedSurfaceGL.cpp
> @@ +158,5 @@
> > +{
> > +    MutexAutoLock lock(mMutex);
> > +    mGL->MakeCurrent();
> > +
> > +    if (mConsGL && mGL->IsExtensionSupported(GLContext::ARB_sync)) {
> 
> You're going to need to add a GLFeature for FenceSync and add the APPLE
> extension to it.

OK. I have that in another patch that I will add to this bug.

> 
> @@ +248,5 @@
> > +                                                       mHasAlpha);
> > +
> > +    // Ownership of the texture is transferred to the host at this point
> > +    mTex = 0;
> > +    mSync = 0;
> 
> This isn't acceptable. We recycle ShSurfs, so on recycle, this would break
> horribly.
> 
> We can transfer ownership, but we can't zero them. We just need to not
> delete them.

I disabled recycling by passing 'false' to the SharedSurface constructor. I thought I needed to add special messaging or something to make this work, but it looks like all of that is already handled via SurfaceFactory::RecycleCallback(). In this case I don't think we want to transfer ownership to the host, so I'll change that.

Comment 4

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

Attachment: Resurrect SharedSurface_GLTexture for use on iOS

Comment 5

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

Attachment: Add support for GL_APPLE_sync

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8666720 [details] [diff] [review]
Resurrect SharedSurface_GLTexture for use on iOS

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

::: gfx/gl/SharedSurfaceGL.cpp
@@ +125,5 @@
> +
> +    GLuint tex = CreateTextureForOffscreen(prodGL, formats, size);
> +
> +    GLenum err = localError.GetError();
> +    MOZ_ASSERT_IF(err != LOCAL_GL_NO_ERROR, err == LOCAL_GL_OUT_OF_MEMORY);

GL_NO_ERROR is 0, so `!err` would be fine.

::: gfx/gl/SharedSurfaceGL.h
@@ +110,5 @@
> +        return (SharedSurface_GLTexture*)surf;
> +    }
> +
> +protected:
> +    GLuint mTex;

const GLuint mTex;

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e943472326
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae59369ae4f0

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b5e943472326
https://hg.mozilla.org/mozilla-central/rev/ae59369ae4f0