Closed Bug 1195359 Opened 9 years ago Closed 8 years ago

Use a separate display connection for the compositor thread

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: acomminos, Assigned: acomminos)

References

(Depends on 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

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.
XInitThreads removal also help to get FF build for Wayland.
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.
Attachment #8648787 - Flags: review?(karlt)
This just adds a Display member to nsShmImage.
Attachment #8648789 - Flags: review?(karlt)
Global context sharing is no longer possible over separate displays.
Attachment #8648790 - Flags: review?(jgilbert)
Again, not possible anymore over separate displays.
Attachment #8648791 - Flags: review?(jgilbert)
Removed unused variable.
Attachment #8648787 - Attachment is obsolete: true
Attachment #8648787 - Flags: review?(karlt)
Attachment #8648809 - Flags: review?(karlt)
Attachment #8648809 - Attachment description: Use separate display connection for the compositor thread on X11. → Part 1 - Use separate display connection for the compositor thread on X11.
Attachment #8648789 - Attachment description: Use compositor display for X11 SHM. → Part 2 - Use compositor display for X11 SHM.
Attachment #8648790 - Attachment description: Disable global GLX context sharing, no longer functional after implementation of separate X11 display connections. → Part 3 - Disable global GLX context sharing, no longer functional after implementation of separate X11 display connections.
Attachment #8648791 - Attachment description: Remove GLXPixmap sharing in-process, no longer possible without context sharing. → Part 4 - Remove GLXPixmap sharing in-process, no longer possible without context sharing.
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.
Attachment #8648809 - Flags: review?(karlt)
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.
Attachment #8648790 - Flags: review?(jgilbert) → review+
Attachment #8648791 - Flags: review?(jgilbert) → review+
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.
Attachment #8648789 - Flags: review?(karlt)
(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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Let's bring this back for bug 1272485 (and possibly bug 1260559).
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
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/
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 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 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
Attachment #8769893 - Flags: review?(lsalzman) → review+
Pushed by acomminos@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/c7e78e0ddba7
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Summary: Replace XInitThreads with a separate display connection for the compositor thread → Use a separate display connection for the compositor thread
Depends on: 1470620
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: