1275725 - Improve and stabilize surface sharing support with GLX

Status: RESOLVED WONTFIX

(Core :: Graphics, defect, P3)

Description

[Andrew Comminos [:acomminos]]

Right now, our surface sharing support on GLX with the GL compositor has a variety of issues related to FBConfig selection, OOP pixmap sharing, and auxiliary buffers. I have developed a series of patches that aim to address these issues;

- Use of gfxXlibSurface and SurfaceDescriptorX11 for surface sharing has been replaced by GLXSurface and SurfaceDescriptorGLX to abstract away the underlying X11 pixmap (more details on why we shouldn't touch that below).
- GLXPixmaps used for sharing are now encapsulated within a GLXSurface, which enforces FBConfig restrictions. GLXDrawable compatibility is now defined as strictly as possible as having the same GLXFBConfigID as the context.
- Use of X11TextureHost has been removed and replaced with GLXTextureHostOGL. GLXPixmaps are not guaranteed to expose the drawable's alpha channel in the pixmap, and hence should not be used in the X11 basic composition pipeline (most visual configs with an alpha channel are represented with an X11 visual of depth 24).

This patch has been tested on an intel DDX with Mesa 11.2, as well as NVIDIA 310.32. I feel that this implementation of surface sharing is strict enough to be supported by any reasonable GLX implementation.

The existing code for creating a GLXPixmap from a gfxXlibSurface remains relevant for the X11 basic composition pipeline.

Comment 1

[Andrew Comminos [:acomminos]]

Attachment: MozReview Request: Bug 1275725 - Remove GLXPixmap sharing support from gfxXlibSurface. r?jgilbert

Review commit: https://reviewboard.mozilla.org/r/55242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55242/

Comment 2

[Andrew Comminos [:acomminos]]

Attachment: MozReview Request: Bug 1275725 - Implement GLXSurface for encapsulating a GLXPixmap target. r?jgilbert

Review commit: https://reviewboard.mozilla.org/r/55244/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55244/

Comment 3

[Andrew Comminos [:acomminos]]

Attachment: MozReview Request: Bug 1275725 - Add GLXTextureHostOGL for rendering GLXSurfaces on the GL compositor. r?nical

Review commit: https://reviewboard.mozilla.org/r/55246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55246/

Comment 4

[Andrew Comminos [:acomminos]]

Attachment: MozReview Request: Bug 1275725 - Enable texture from pixmap by default. r?jgilbert

Review commit: https://reviewboard.mozilla.org/r/55248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55248/

Comment 5

[Andrew Comminos [:acomminos]]

Comment on attachment 8756559 [details]
MozReview Request: Bug 1275725 - Implement GLXSurface for encapsulating a GLXPixmap target. r?jgilbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55244/diff/1-2/

Comment 6

[Andrew Comminos [:acomminos]]

Comment on attachment 8756560 [details]
MozReview Request: Bug 1275725 - Add GLXTextureHostOGL for rendering GLXSurfaces on the GL compositor. r?nical

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55246/diff/1-2/

Comment 7

[Andrew Comminos [:acomminos]]

Comment on attachment 8756561 [details]
MozReview Request: Bug 1275725 - Enable texture from pixmap by default. r?jgilbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55248/diff/1-2/

Comment 8

[Nicolas Silva [:nical]]

Comment on attachment 8756560 [details]
MozReview Request: Bug 1275725 - Add GLXTextureHostOGL for rendering GLXSurfaces on the GL compositor. r?nical

https://reviewboard.mozilla.org/r/55246/#review52166

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8756558 [details]
MozReview Request: Bug 1275725 - Remove GLXPixmap sharing support from gfxXlibSurface. r?jgilbert

https://reviewboard.mozilla.org/r/55242/#review52272

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8756561 [details]
MozReview Request: Bug 1275725 - Enable texture from pixmap by default. r?jgilbert

https://reviewboard.mozilla.org/r/55248/#review52274

Comment 11

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8756561 [details]
MozReview Request: Bug 1275725 - Enable texture from pixmap by default. r?jgilbert

So this is fine, but we need a way to blacklist this if necessary.

I'm not sure if we can blacklist prefs directly, but if not, you'll need to add a blacklist entry and check.

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8756559 [details]
MozReview Request: Bug 1275725 - Implement GLXSurface for encapsulating a GLXPixmap target. r?jgilbert

https://reviewboard.mozilla.org/r/55244/#review52278

Cool, thanks for working on this.

::: gfx/gl/GLContextGLX.h:25
(Diff revision 2)
> +// required for texture from pixmap.
> +//
> +// A primary goal of a GLXSurface is to treat the underlying X11 pixmap as an
> +// opaque blob only accessible within GLX and the GL. This is because not all
> +// buffers (particularly alpha) are always present in the X11 pixmap.
> +class GLXSurface : public GenericAtomicRefCounted

Why is this AtomicRefCounted and not just RefCounted?

::: gfx/gl/GLContextGLX.h:39
(Diff revision 2)
> +    // with the depth of the provided GLXFBConfig.
> +    static already_AddRefed<GLXSurface> Create(Display* aDisplay,
> +                                               GLXFBConfig& aConfig,
> +                                               const gfx::IntSize& aSize);
> +
> +    // Creates a new GLXSurface by binds a new GLXPixmap to the provided pixmap.

s/binds/binding/

::: gfx/gl/GLContextGLX.h:62
(Diff revision 2)
> +
> +    // Creates a compatible (with the same GLXFBConfig) GLXSurface.
> +    already_AddRefed<GLXSurface> CreateCompatible(const gfx::IntSize& aSize);
> +
> +    // Queries this surface's GLXFBConfig for the given attribute value.
> +    Maybe<int> GetFBConfigAttrib(int attrib);

Use out-params here (instead of Maybe) unless there's a reason to use Maybe here. Out-params (`T* const out_foo`) should be cleaner at the call-site and are already in use by a bunch of related APIs.

Also, if this is only used internally, it should be private.

::: gfx/gl/GLContextGLX.h:69
(Diff revision 2)
> +    GLXPixmap GetGLXPixmap() { return mGLXPixmap; }
> +    GLXFBConfigID GetFBConfigID() { return mFBConfigID; }
> +
> +    bool ToSurfaceDescriptor(bool aInProcess,
> +                             layers::SurfaceDescriptor* const out_descriptor)
> +    {

Avoid non-trivial member function implementations in the header. Move it to the source file.

::: gfx/gl/GLContextGLX.h:71
(Diff revision 2)
> +
> +    bool ToSurfaceDescriptor(bool aInProcess,
> +                             layers::SurfaceDescriptor* const out_descriptor)
> +    {
> +        Maybe<int> hasAlpha = GetFBConfigAttrib(LOCAL_GLX_ALPHA_SIZE);
> +        if (!hasAlpha)

This is pretty misleading. `maybeAlphaBits` is clearer.

::: gfx/gl/GLContextGLX.h:100
(Diff revision 2)
> +    Display* mDisplay;
> +    Pixmap mPixmap;
> +    GLXPixmap mGLXPixmap;
> +    GLXFBConfigID mFBConfigID;
> +    gfx::IntSize mSize;

These should all be const, and if they have getters, made public instead.

::: gfx/gl/GLContextGLX.h:156
(Diff revision 2)
> -    bool OverrideDrawable(GLXDrawable drawable);
> +    bool OverrideSurface(GLXSurface& aSurface);
>  
> -    // Undoes the effect of a drawable override.
> -    bool RestoreDrawable();
> +    // Unredirects the active GLXSurface.
> +    bool RestoreSurface();

Don't use a reference for this. It doesn't really work for trying to ensure something is non-null.

Just combine these into a SetOverrideSurface that treats nullptr as 'restore'/'remove override'.

::: gfx/gl/GLContextGLX.h:185
(Diff revision 2)
>                   ContextProfile profile);
>  
>      GLXContext mContext;
>      Display *mDisplay;
>      GLXDrawable mDrawable;
> +    GLXFBConfigID mFBConfigID;

const

::: gfx/gl/GLContextProviderGLX.cpp:963
(Diff revision 2)
> +    MOZ_ASSERT(mIsOffscreen, "Only offscreen contexts may render to a GLXPixmap.");
> +    MOZ_RELEASE_ASSERT(aSurface.GetFBConfigID() == mFBConfigID, "Incompatible FBConfig!");
> +
>      if (Screen())
>          Screen()->AssureBlitted();
> -    Bool result = mGLX->xMakeCurrent(mDisplay, drawable, mContext);
> +    Bool result = mGLX->xMakeCurrent(mDisplay, aSurface.GetGLXPixmap(), mContext);

s/Bool/bool/

::: gfx/gl/GLContextProviderGLX.cpp:1051
(Diff revision 2)
>          RefPtr<GLContextGLX> glContext =
>              new GLContextGLX(caps,
>                               nullptr, // SharedContext
>                               false, // Offscreen
>                               (Display*)DefaultXDisplay(), // Display
> -                             (GLXDrawable)aSurface, (GLXContext)aContext,
> +                             (GLXDrawable)aSurface, None,

It's not clear what None is, so pull it out into a local with a descriptive name.

::: gfx/gl/GLContextProviderGLX.cpp:1123
(Diff revision 2)
>      ScopedXFree<GLXFBConfig>& scopedConfigArr = *out_scopedConfigArr;
>  
>      if (minCaps.antialias)
>          return false;
>  
> -    int attribs[] = {
> +    AutoTArray<int, 17> attribs;

s/17/MOZ_ARRAY_LENGTH(baseAttribs)/

::: gfx/gl/GLContextProviderGLX.cpp:1133
(Diff revision 2)
>          LOCAL_GLX_GREEN_SIZE, 8,
>          LOCAL_GLX_BLUE_SIZE, 8,
>          LOCAL_GLX_ALPHA_SIZE, minCaps.alpha ? 8 : 0,
>          LOCAL_GLX_DEPTH_SIZE, minCaps.depth ? 16 : 0,
>          LOCAL_GLX_STENCIL_SIZE, minCaps.stencil ? 8 : 0,
> -        0
> +        None

Don't put None here, since it will (should) prevent the tfpAttribs from being read when they're appended later.

Also, is there a namespace for this None that we can use?

::: gfx/gl/GLContextProviderGLX.cpp:1354
(Diff revision 2)
>  {
>      gGlobalContext = nullptr;
>  }
>  
> +/*static*/ bool
> +GLXSurface::GetFBConfigWithID(Display* aDisplay, GLXFBConfigID aFBConfigID,

s/With/By/

::: gfx/gl/GLContextProviderGLX.cpp:1375
(Diff revision 2)
> +                   const gfx::IntSize& aSize)
> +{
> +    int visid = 0;
> +    if (sGLXLibrary.xGetFBConfigAttrib(aDisplay, aConfig, LOCAL_GLX_VISUAL_ID,
> +                                       &visid) != Success)
> +        return nullptr;

Multi-line conditionals should drop opening brace to its own line:

if (foo &&
    bar)
{
    return;
}

::: gfx/gl/GLContextProviderGLX.cpp:1378
(Diff revision 2)
> +    if (sGLXLibrary.xGetFBConfigAttrib(aDisplay, aConfig, LOCAL_GLX_VISUAL_ID,
> +                                       &visid) != Success)
> +        return nullptr;
> +
> +    // Use the associated X11 visual's depth for the created pixmap.
> +    int depth = 0;

Don't initialize `depth` iff you don't initialize `visual`. Since these are outparams, you don't need to initialize them as long as we're catching the failure case.

However, it doesn't look like we check if FindVisualAndDepth succeeds. Check if it succeeds and/or commment detailing how we detect it failed.
If it's infallible, don't initialize depth or visual. (and/or comment that FindVisualAndDepth is infallible)

::: gfx/gl/GLContextProviderGLX.cpp:1379
(Diff revision 2)
> +                                       &visid) != Success)
> +        return nullptr;
> +
> +    // Use the associated X11 visual's depth for the created pixmap.
> +    int depth = 0;
> +    Visual *visual;

Star to the left.

::: gfx/gl/GLContextProviderGLX.cpp:1396
(Diff revision 2)
> +GLXSurface::Bind(Display* aDisplay, Pixmap aPixmap, GLXFBConfig& aConfig,
> +                 const gfx::IntSize& aSize, bool aOwnsPixmap)
> +{
> +    int alphaSize = 0;
> +    if (sGLXLibrary.xGetFBConfigAttrib(aDisplay, aConfig, LOCAL_GLX_ALPHA_SIZE,
> +                                       &alphaSize) != Success)

This is generally better as:

const auto result = sGLXLibrary.xGetFBConfigAttrib(aDisplay, aConfig,
                                                   LOCAL_GLX_ALPHA_SIZE, &alphaSize);
if (result != Succeeded)
    return nullptr;

::: gfx/gl/GLContextProviderGLX.cpp:1397
(Diff revision 2)
> +                 const gfx::IntSize& aSize, bool aOwnsPixmap)
> +{
> +    int alphaSize = 0;
> +    if (sGLXLibrary.xGetFBConfigAttrib(aDisplay, aConfig, LOCAL_GLX_ALPHA_SIZE,
> +                                       &alphaSize) != Success)
> +        return nullptr;

Multiline conditional means use braces.

::: gfx/gl/GLContextProviderGLX.cpp:1403
(Diff revision 2)
> +
> +    int tfpAttribs[] = {
> +        LOCAL_GLX_TEXTURE_TARGET_EXT, LOCAL_GLX_TEXTURE_2D_EXT,
> +        LOCAL_GLX_TEXTURE_FORMAT_EXT,
> +        (alphaSize ? LOCAL_GLX_TEXTURE_FORMAT_RGBA_EXT
> +                   : LOCAL_GLX_TEXTURE_FORMAT_RGB_EXT),

Try not to embed these trinaries so much. Use locals to keep things clean. Trinaries almost always incur weird line wrapping and formatting.

::: gfx/gl/GLContextProviderGLX.cpp:1408
(Diff revision 2)
> +                   : LOCAL_GLX_TEXTURE_FORMAT_RGB_EXT),
> +        None
> +    };
> +
> +    GLXPixmap glxPixmap = sGLXLibrary.xCreatePixmap(aDisplay, aConfig, aPixmap,
> +        sGLXLibrary.UseTextureFromPixmap() ? tfpAttribs : nullptr);

Pull this trinary out into a local.

::: gfx/gl/GLContextProviderGLX.cpp:1413
(Diff revision 2)
> +        sGLXLibrary.UseTextureFromPixmap() ? tfpAttribs : nullptr);
> +    if (!glxPixmap)
> +        return nullptr;
> +
> +    return GLXSurface::Wrap(aDisplay, aPixmap, glxPixmap, aConfig, aSize,
> +                            aOwnsPixmap, true);

Avoid passing `true` in larger arg lists like this.

const bool doesFoo = true;
return Bar(..., doesFoo, ...)

::: gfx/gl/GLContextProviderGLX.cpp:1448
(Diff revision 2)
> +}
> +
> +GLXSurface::~GLXSurface()
> +{
> +  if (mOwnsGLXPixmap)
> +      sGLXLibrary.xDestroyPixmap(mDisplay, mGLXPixmap);

Generally brace `if` bodies unless they are single lines control flow statements. (return/break/continue)

::: gfx/gl/GLContextProviderGLX.cpp:1452
(Diff revision 2)
> +  if (mOwnsGLXPixmap)
> +      sGLXLibrary.xDestroyPixmap(mDisplay, mGLXPixmap);
> +
> +  if (mOwnsPixmap)
> +      XFreePixmap(mDisplay, mPixmap);
> +

Drop this blank line.

::: gfx/gl/SharedSurfaceGLX.cpp:29
(Diff revision 2)
>                                    const gfx::IntSize& size,
>                                    bool deallocateClient,
>                                    bool inSameProcess)
>  {
> +    GLContextGLX* glxContext = GLContextGLX::Cast(prodGL);
>      UniquePtr<SharedSurface_GLXDrawable> ret;

Move `ret` to near where it's used.

::: gfx/gl/SharedSurfaceGLX.cpp:71
(Diff revision 2)
>  void
>  SharedSurface_GLXDrawable::LockProdImpl()
>  {
>      mGL->Screen()->SetReadBuffer(LOCAL_GL_FRONT);
> -    GLContextGLX::Cast(mGL)->OverrideDrawable(mXlibSurface->GetGLXPixmap());
> +    if (!GLContextGLX::Cast(mGL)->OverrideSurface(*mGLXSurface))
> +        gfxWarning() << "Failed to redirect to GLXSurface.";

Brace this.

::: gfx/gl/SharedSurfaceGLX.cpp:97
(Diff revision 2)
>  SurfaceFactory_GLXDrawable::Create(GLContext* prodGL,
>                                     const SurfaceCaps& caps,
>                                     const RefPtr<layers::ClientIPCAllocator>& allocator,
>                                     const layers::TextureFlags& flags)
>  {
> -    MOZ_ASSERT(caps.alpha, "GLX surfaces require an alpha channel!");
> +    MOZ_ASSERT(caps.alpha == prodGL->Caps().alpha &&

Surround the && chain with parens. This will incidentally make the indentation obvious that the string is arg 2.

::: gfx/gl/SharedSurfaceGLX.cpp:101
(Diff revision 2)
>  {
> -    MOZ_ASSERT(caps.alpha, "GLX surfaces require an alpha channel!");
> -
> +    MOZ_ASSERT(caps.alpha == prodGL->Caps().alpha &&
> +               caps.depth == prodGL->Caps().depth &&
> +               caps.stencil == prodGL->Caps().stencil,
> +               "GLX requires the same buffer depths as the context.");
> +    MOZ_ASSERT(prodGL->Caps().textureBinding, "Context must be initialized to support TFP.");

MOZ_RELEASE_ASSERT

::: gfx/gl/SurfaceTypes.h:29
(Diff revision 2)
>      bool bpp16;
>      bool depth, stencil;
>      bool antialias;
>      bool premultAlpha;
>      bool preserve;
> +    bool textureBinding;

Update Clear().

Optionally, update clear to memset(0) the struct, and exceptionally set premultAlpha:true.

Comment 13

[Andrew Comminos [:acomminos]]

Comment on attachment 8756559 [details]
MozReview Request: Bug 1275725 - Implement GLXSurface for encapsulating a GLXPixmap target. r?jgilbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55244/diff/2-3/

Comment 14

[Andrew Comminos [:acomminos]]

https://reviewboard.mozilla.org/r/55244/#review52278

> Why is this AtomicRefCounted and not just RefCounted?

You're right, it doesn't need to be. I figured it might be a good idea for consistency with GLContext, but let's scrap it.

> Don't put None here, since it will (should) prevent the tfpAttribs from being read when they're appended later.
> 
> Also, is there a namespace for this None that we can use?

I'm not aware of any explicitly X11-related namespace, no.

Comment 15

[Andrew Comminos [:acomminos]]

Attachment: MozReview Request: Bug 1275725 - Remove GLXPixmap sharing support from gfxXlibSurface. r=jgilbert

Review commit: https://reviewboard.mozilla.org/r/55862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55862/

Comment 16

[Andrew Comminos [:acomminos]]

Attachment: MozReview Request: Bug 1275725 - Add GLXTextureHostOGL for rendering GLXSurfaces on the GL compositor. r=nical

Review commit: https://reviewboard.mozilla.org/r/55864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55864/

Comment 17

[Andrew Comminos [:acomminos]]

Attachment: MozReview Request: Bug 1275725 - Enable texture from pixmap by default. r?jrmuizel

Review commit: https://reviewboard.mozilla.org/r/55866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55866/

Comment 18

[Andrew Comminos [:acomminos]]

Comment on attachment 8756559 [details]
MozReview Request: Bug 1275725 - Implement GLXSurface for encapsulating a GLXPixmap target. r?jgilbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55244/diff/3-4/

Comment 19

[Andrew Comminos [:acomminos]]

Comment on attachment 8757429 [details]
MozReview Request: Bug 1275725 - Add GLXTextureHostOGL for rendering GLXSurfaces on the GL compositor. r=nical

Sorry for the spam, MozReview doesn't like carrying r+ on reviewed patches it seems.

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8757430 [details]
MozReview Request: Bug 1275725 - Enable texture from pixmap by default. r?jrmuizel

https://reviewboard.mozilla.org/r/55866/#review52940

Andrew's going to file a bug about adding a gfxConfig feature for this. If we need to disable it before that gets in, we can just disable it completely.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8756559 [details]
MozReview Request: Bug 1275725 - Implement GLXSurface for encapsulating a GLXPixmap target. r?jgilbert

https://reviewboard.mozilla.org/r/55244/#review57744

We decided to address this more simply than this patch does.

Comment 22

[BugBot [:suhaib / :marco]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.