1195359 - Use a separate display connection for the compositor thread

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Andrew Comminos [:acomminos]]

XInitThreads has the unfortunate effect of freezing some GL drivers that don't expect a thread-safe X11 environment. We should remove it and replace it with a separate display connection on the compositor thread, at least on GTK.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

XInitThreads removal also help to get FF build for Wayland.

Comment 2

[Andrew Comminos [:acomminos]]

Attachment: Use separate display connection for the compositor thread on X11.

This adds a separate compositor display connection managed by gfxPlatformGtk, and ensures that the main thread's display connection is not used in any other thread.

Necessary flushing is also added for child textures.

Comment 3

[Andrew Comminos [:acomminos]]

Attachment: Part 2 - Use compositor display for X11 SHM.

This just adds a Display member to nsShmImage.

Comment 4

[Andrew Comminos [:acomminos]]

Attachment: Part 3 - Disable global GLX context sharing, no longer functional after implementation of separate X11 display connections.

Global context sharing is no longer possible over separate displays.

Comment 5

[Andrew Comminos [:acomminos]]

Attachment: Part 4 - Remove GLXPixmap sharing in-process, no longer possible without context sharing.

Again, not possible anymore over separate displays.

Comment 6

[Andrew Comminos [:acomminos]]

Attachment: Part 1 - Use separate display connection for the compositor thread on X11.

Removed unused variable.

Comment 7

[Andrew Comminos [:acomminos]]

Comment on attachment 8648809 [details] [diff] [review]
Part 1 - Use separate display connection for the compositor thread on X11.

Holding off review on this one for now, there's still some places we make GTK/GDK/X11 calls off the main thread.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8648790 [details] [diff] [review]
Part 3 - Disable global GLX context sharing, no longer functional after implementation of separate X11 display connections.

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

::: gfx/gl/GLContextProviderGLX.cpp
@@ +1122,3 @@
>      SurfaceCaps caps = SurfaceCaps::Any();
>      nsRefPtr<GLContextGLX> glContext = GLContextGLX::CreateGLContext(caps,
> +                                                                     nullptr,

If this is always nullptr, remove the arg from the callee.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8648789 [details] [diff] [review]
Part 2 - Use compositor display for X11 SHM.

I guess we're holding off on this one too.

I think we need a good reason to use a different display.  It could make synchronization harder for Gecko and just move any contention to the X server.
With the situation in bug 1189132, we don't have a good reason AFAIK.

Comment 10

[Andrew Comminos [:acomminos]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #9)
> Comment on attachment 8648789 [details] [diff] [review]
> Part 2 - Use compositor display for X11 SHM.
> 
> I guess we're holding off on this one too.
> 
> I think we need a good reason to use a different display.  It could make
> synchronization harder for Gecko and just move any contention to the X
> server.
> With the situation in bug 1189132, we don't have a good reason AFAIK.

Right. Marking as WONTFIX for now, we can reopen if a good reason to go this route arises.

Comment 11

[Andrew Comminos [:acomminos]]

Let's bring this back for bug 1272485 (and possibly bug 1260559).

Comment 12

[Andrew Comminos [:acomminos]]

Attachment: Bug 1195359 - Use a new display on the compositor thread when using GLX and the GL compositor.

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

Comment 13

[Andrew Comminos [:acomminos]]

Comment on attachment 8769893 [details]
Bug 1195359 - Use a new display on the compositor thread when using GLX and the GL compositor.

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

Comment 14

[Lee Salzman [:lsalzman]]

https://reviewboard.mozilla.org/r/63570/#review60482

::: gfx/thebes/gfxPlatformGtk.h:20
(Diff revision 2)
>      typedef struct _GdkDrawable GdkDrawable;
>  }
>  #endif
>  
> +#ifdef MOZ_X11
> +typedef struct _XDisplay Display;

Should forward declare struct _XDisplay before doing typedef.

::: gfx/thebes/gfxPlatformGtk.cpp:122
(Diff revision 2)
>          sFontconfigUtils = nullptr;
>          gfxPangoFontGroup::Shutdown();
>      }
> +
> +#ifdef MOZ_X11
> +    if (mCompositorDisplay)

Needs brackets.

Comment 15

[Andrew Comminos [:acomminos]]

Comment on attachment 8769893 [details]
Bug 1195359 - Use a new display on the compositor thread when using GLX and the GL compositor.

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

Comment 16

[Lee Salzman [:lsalzman]]

Comment on attachment 8769893 [details]
Bug 1195359 - Use a new display on the compositor thread when using GLX and the GL compositor.

https://reviewboard.mozilla.org/r/63570/#review60730

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7e78e0ddba7
Use a new display on the compositor thread when using GLX and the GL compositor. r=lsalzman

Comment 18

[Carsten Book [:Tomcat]]

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