Use a separate display connection for the compositor thread

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: acomminos, Assigned: acomminos)

Tracking

(Depends on: 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 2

3 years ago
Created attachment 8648787 [details] [diff] [review]
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.
Attachment #8648787 - Flags: review?(karlt)
(Assignee)

Comment 3

3 years ago
Created attachment 8648789 [details] [diff] [review]
Part 2 - Use compositor display for X11 SHM.

This just adds a Display member to nsShmImage.
Attachment #8648789 - Flags: review?(karlt)
(Assignee)

Comment 4

3 years ago
Created attachment 8648790 [details] [diff] [review]
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.
Attachment #8648790 - Flags: review?(jgilbert)
(Assignee)

Comment 5

3 years ago
Created attachment 8648791 [details] [diff] [review]
Part 4 - Remove GLXPixmap sharing in-process, no longer possible without context sharing.

Again, not possible anymore over separate displays.
Attachment #8648791 - Flags: review?(jgilbert)
(Assignee)

Comment 6

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

Removed unused variable.
Attachment #8648787 - Attachment is obsolete: true
Attachment #8648787 - Flags: review?(karlt)
Attachment #8648809 - Flags: review?(karlt)
(Assignee)

Updated

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8648789 - Attachment description: Use compositor display for X11 SHM. → Part 2 - Use compositor display for X11 SHM.
(Assignee)

Updated

3 years ago
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.
(Assignee)

Updated

3 years ago
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.
(Assignee)

Comment 7

3 years ago
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)
(Assignee)

Comment 10

3 years ago
(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
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 11

2 years ago
Let's bring this back for bug 1272485 (and possibly bug 1260559).
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Comment 12

2 years ago
Created attachment 8769893 [details]
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/
Attachment #8769893 - Flags: review?(lsalzman)
(Assignee)

Comment 13

2 years ago
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.
(Assignee)

Comment 15

2 years ago
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+

Comment 17

2 years ago
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

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c7e78e0ddba7
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Updated

2 years ago
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.