1187440 - Implement GLX surface sharing with OpenGL layers

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Andrew Comminos [:acomminos]]

WebGL should make use of GLXPixmaps to share surfaces with the OpenGL compositor using EXT_texture_from_pixmap.

Comment 1

[Andrew Comminos [:acomminos]]

Attachment: Implement GLX shared surfaces on the OpenGL compositor.

This is an initial implementation of surface sharing using GLXPixmaps. As GLX forbids multiple GLXPixmap bindings to a Pixmap in the same address space, gfxXlibSurface will carry a GLXPixmap in the single process OMTC case. Thanks!

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8638729 [details] [diff] [review]
Implement GLX shared surfaces on the OpenGL compositor.

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

First off, thanks for writing this!

:nical should additionally review the gfx/layers parts.

::: gfx/gl/GLContextProviderGLX.cpp
@@ +924,5 @@
> +
> +void
> +GLContextGLX::RestoreDrawable()
> +{
> +    mGLX->xMakeCurrent(mDisplay, mDrawable, mContext);

Either `MOZ_ALWAYS_TRUE( ... );` or return the result.

::: gfx/gl/GLScreenBuffer.cpp
@@ +387,5 @@
>  {
>      MOZ_ASSERT(newFactory);
>      mFactory = Move(newFactory);
> +    // Destroy DrawBuffer in case the factory has a different color format.
> +    mDraw = nullptr;

You can't destroy this here, as it may be in use. (r-)
Explain more please.

::: gfx/gl/SharedSurfaceGLX.cpp
@@ +41,5 @@
> +
> +SharedSurface_GLXDrawable::SharedSurface_GLXDrawable(GLContext* gl,
> +                                                     const gfx::IntSize& size,
> +                                                     bool inSameProcess,
> +                                                     RefPtr<gfxXlibSurface> surface)

`const RefPtr<gfxXlibSurface>& xlibSurface`
or
`gfxXlibSurface* xlibSurface`

@@ +48,5 @@
> +                    gl,
> +                    size,
> +                    true,
> +                    true)
> +    , mSurface(surface)

mXSurface or mXlibSurface. We have a bunch of different types of surfaces, try to be explicit.

@@ +91,5 @@
> +                                   const layers::TextureFlags& flags)
> +{
> +    // Always use RGBA visuals on GLX- we can't request the context to provide
> +    // us with an internal format of GL_RGB, which will cause errors blitting
> +    // from a GL_RGB framebuffer.

This tricky, but we already handle this with ANGLE. You'll need to go into WebGLContext.cpp and find these lines:
    if (gl->WorkAroundDriverBugs() && gl->IsANGLE()) {
        if (!mOptions.alpha && gl->Caps().alpha)
            mNeedsFakeNoAlpha = true;

You'll need to remove the `&& gl->IsANGLE()` part.

@@ +96,5 @@
> +    SurfaceCaps newCaps = caps;
> +    newCaps.alpha = true;
> +    return UniquePtr<SurfaceFactory_GLXDrawable>(
> +        new SurfaceFactory_GLXDrawable(prodGL, newCaps, allocator,
> +          flags & ~layers::TextureFlags::ORIGIN_BOTTOM_LEFT));

Why do you drop the ORIGIN_BOTTOM_LEFT flag? It's generally true for GL-sourced data.

@@ +103,5 @@
> +UniquePtr<SharedSurface>
> +SurfaceFactory_GLXDrawable::CreateShared(const gfx::IntSize& size)
> +{
> +    return SharedSurface_GLXDrawable::Create(mGL, mCaps, size,
> +        !!(mFlags & layers::TextureFlags::DEALLOCATE_CLIENT),

Pull non-trivial calculations into local variables.
It'd be really obvious what's going on if it were:

bool deallocateClient = !!(mFlags & layers::TextureFlags::DEALLOCATE_CLIENT);
return SharedSurface_GLXDrawable::Create(mGL, mCaps, size, deallocateClient,
                                         mAllocator->IsSameProcess());

::: gfx/gl/SharedSurfaceGLX.h
@@ +37,5 @@
> +                              const gfx::IntSize& size,
> +                              bool inSameProcess,
> +                              RefPtr<gfxXlibSurface> surface);
> +
> +    RefPtr<gfxXlibSurface> mSurface;

mXSurface or mXlibSurface.

Also try to put members near the top of the class, so they're easier to find.

::: gfx/layers/ipc/ShadowLayerUtilsX11.cpp
@@ +88,5 @@
>    : mId(aDrawable)
>    , mFormat(aFormatID)
>    , mSize(aSize)
> +#ifdef GL_PROVIDER_GLX
> +  , mGLXPixmap(None)

Why #ifdef this? Why not just let it be None when #ifndef?
Fewer ifdefs is better.

::: gfx/layers/ipc/ShadowLayerUtilsX11.h
@@ +18,5 @@
>  typedef unsigned long XID;
>  typedef XID Drawable;
>  
> +#ifdef GL_PROVIDER_GLX
> +typedef XID GLXPixmap;

Shouldn't we just use the `Drawable` type above?

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +99,5 @@
> +
> +#ifdef GL_PROVIDER_GLX
> +    case SurfaceDescriptor::TSurfaceDescriptorX11: {
> +      const SurfaceDescriptorX11& desc =
> +        aDesc.get_SurfaceDescriptorX11();

It fits on one line:
      const SurfaceDescriptorX11& desc = aDesc.get_SurfaceDescriptorX11();

You can probable use `const auto&` to shorten it further.

Comment 3

[Andrew Comminos [:acomminos]]

Thanks for the review!

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> ::: gfx/gl/GLScreenBuffer.cpp
> @@ +387,5 @@
> >  {
> >      MOZ_ASSERT(newFactory);
> >      mFactory = Move(newFactory);
> > +    // Destroy DrawBuffer in case the factory has a different color format.
> > +    mDraw = nullptr;
> 
> You can't destroy this here, as it may be in use. (r-)
> Explain more please.

This was necessary as we were attempting to blit a GL_RGB buffer (from before the shared surface becomes active) onto the shared surface's default FB, which is of format GL_RGBA. We shouldn't need it anymore if we always use GL_RGBA buffers like on ANGLE.

Comment 4

[Andrew Comminos [:acomminos]]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> @@ +96,5 @@
> > +    SurfaceCaps newCaps = caps;
> > +    newCaps.alpha = true;
> > +    return UniquePtr<SurfaceFactory_GLXDrawable>(
> > +        new SurfaceFactory_GLXDrawable(prodGL, newCaps, allocator,
> > +          flags & ~layers::TextureFlags::ORIGIN_BOTTOM_LEFT));
> 
> Why do you drop the ORIGIN_BOTTOM_LEFT flag? It's generally true for
> GL-sourced data.

X drawables use a different coordinate system (origin @ top-left) than OpenGL. The GLX pixmap itself is an X drawable, and thus should be noted as having its origin at the top left.

Comment 5

[Andrew Comminos [:acomminos]]

Attachment: Implement GLX shared surfaces on the OpenGL compositor.

Updated with review comments.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8640522 [details] [diff] [review]
Implement GLX shared surfaces on the OpenGL compositor.

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

::: gfx/gl/SharedSurfaceGLX.cpp
@@ +92,5 @@
> +{
> +    MOZ_ASSERT(caps.alpha, "GLX surfaces require an alpha channel!");
> +    return UniquePtr<SurfaceFactory_GLXDrawable>(
> +        new SurfaceFactory_GLXDrawable(prodGL, caps, allocator,
> +          flags & ~layers::TextureFlags::ORIGIN_BOTTOM_LEFT));

Pull this into a local.

::: gfx/thebes/gfxXlibSurface.cpp
@@ +83,3 @@
>      // gfxASurface's destructor calls RecordMemoryFreed().
>      if (mPixmapTaken) {
> +#if defined(GL_PROVIDER_GLX)

I see you just moved this, but shouldn't it be able to be non-ifdef'd?

Comment 7

[Andrew Comminos [:acomminos]]

(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> ::: gfx/thebes/gfxXlibSurface.cpp
> @@ +83,3 @@
> >      // gfxASurface's destructor calls RecordMemoryFreed().
> >      if (mPixmapTaken) {
> > +#if defined(GL_PROVIDER_GLX)
> 
> I see you just moved this, but shouldn't it be able to be non-ifdef'd?

mGLXPixmap is guarded with a GL_PROVIDER_GLX ifdef (as it is of type GLXPixmap), so it's not possible without changing mGLXPixmap to a standard X11 drawable.

Comment 8

[Andrew Comminos [:acomminos]]

Attachment: Implement GLX shared surfaces on the OpenGL compositor. Carry r=jgilbert,nical

Skipping try push as our test infrastructure doesn't use the GL compositor with the WebGL tests.

Comment 9

[Pulsebot]

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

Comment 10

[Andrew Comminos [:acomminos]]

Attachment: Fix curly brace bustage in TextureHostOGL.

Comment 11

[Pulsebot]

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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/575357639610
https://hg.mozilla.org/mozilla-central/rev/b0d4af6d3d2a