Closed Bug 1193015 Opened 6 years ago Closed 6 years ago

Binding shared GLX pixmap when sharing WebGL surfaces fails on NVIDIA drivers

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 --- fixed
firefox43 --- fixed
firefox44 --- fixed

People

(Reporter: acomminos, Assigned: acomminos)

References

Details

Attachments

(2 files, 1 obsolete file)

We encounter a BadMatch when calling glXMakeCurrent on shared GLXPixmaps using the NVIDIA drivers on the test server. I can reproduce locally with NVIDIA driver 310.32.
This patch ensures that we always create RGBA visuals with a headless context so we can attach RGBA pixmaps (in order for a context and drawable to be compatible, they must have the same buffer depths).

Thanks!
Attachment #8647551 - Flags: review?(jgilbert)
This is the same issue we're fixing in EGL. It looks like we need to largely revert from the idea of headless contexts, since at least both GLX and EGL don't actually allow attaching surfaces with configs differing from the context creation config.

I think it'll be a week before I can get a pervasive fix for this, so let's look at taking this in the mean time.
Actually, before we check this patch, a question:
Does GLX require matching depth/stencil channels as well? EGL does. If GLX does, I'm going to recommend you look at the EGL patch we have.
Flags: needinfo?(acomminos)
The other bug is bug 1191042.
See Also: → 1191042
Yes, the spec says it does; all buffers must have the same depth. I'll take a look at that EGL patch.
Flags: needinfo?(acomminos)
(In reply to Andrew Comminos [:acomminos] from comment #5)
> Yes, the spec says it does; all buffers must have the same depth. I'll take
> a look at that EGL patch.

Ok. We'll want to uplift this, since I think GLX sharing made 42. (now Dev edition)
We'll also want to uplift bug 1194472 if we want to fix GLX surface sharing with e10s.
(In reply to Jeff Gilbert [:jgilbert] from comment #3)
> I'm going to recommend you look at the EGL patch we have.

I think the approach with the EGL patch is the right way to go; we can uplift this patch to 42, and I'll whip up a GLX implementation of the fix for bug 1191042 when it lands.
Following up on our discussion from IRC about rendering directly to a texture in the pixmap's context- X pixmaps are unfortunately just dumb bitmap data that may be stored on the GPU with some XRender implementations. We actually have to write to the pixmap to share surfaces with X11, so the context definitely needs to know what buffers to expect beforehand (like EGL).
Comment on attachment 8647551 [details] [diff] [review]
Ensure we use RGBA surfaces with offscreen GLX.

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

Let's do something similar to what the EGL patch was doing.
Attachment #8647551 - Flags: review?(jgilbert) → review-
(In reply to Andrew Comminos [:acomminos] from comment #9)
> Following up on our discussion from IRC about rendering directly to a
> texture in the pixmap's context- X pixmaps are unfortunately just dumb
> bitmap data that may be stored on the GPU with some XRender implementations.
> We actually have to write to the pixmap to share surfaces with X11, so the
> context definitely needs to know what buffers to expect beforehand (like
> EGL).

Ugh, ok. It's way messier, and such an annoying and unnecessary aspect of the spec.
Depends on: 1191042
Looks like NVIDIA's GLX implementation may be even more strict and mandate that we use the exact same GLX Visual / GLXFBConfig for the drawable and the context: http://blog.sukimashita.com/2015/04/13/gnome-3-16-and-nvidia-binary-driver-crash-with-totem-gnome-maps-cheese-and-others/

Not too big of an issue though- we'll just have to put in compatible GLXDrawable semantics into GLContextGLX (i.e. store the context's visual and create a new pixmap with it). As long as we create the initial context with the correct capabilities, we should be fine with that approach.
Here's a patch putting GLX surface sharing behind the envvar MOZ_GLX_USE_SURFACE_SHARING. We should probably uplift this to aurora and beta.
Attachment #8665583 - Flags: review?(jgilbert)
Comment on attachment 8665583 [details] [diff] [review]
Require MOZ_GLX_USE_SURFACE_SHARING to enable WebGL surface sharing on GLX.

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

Change this over to the bool version of GetEnv, and we're GTG.

::: gfx/gl/GLXLibrary.h
@@ +111,5 @@
>      bool GLXVersionCheck(int aMajor, int aMinor);
> +    bool UseSurfaceSharing() {
> +      // Disable surface sharing due to issues with compatible FBConfigs on
> +      // NVIDIA drivers as described in bug 1193015.
> +      const char* useSharing = PR_GetEnv("MOZ_GLX_USE_SURFACE_SHARING");

We generally have been using:
    static bool foo = PR_GetEnv("MOZ_FOO");

instead of actually reading the string.
Attachment #8665583 - Flags: review?(jgilbert) → review+
Comment on attachment 8665657 [details] [diff] [review]
Require MOZ_GLX_USE_SURFACE_SHARING to enable WebGL surface sharing on GLX. Carry r=jgilbert

Approval Request Comment
[Feature/regressing bug #]: 1187440
[User impact if declined]: Users with NVIDIA cards who force-enabled the OpenGL compositor on Linux may experience crashes with WebGL.
[Describe test coverage new/current, TreeHerder]: The OpenGL compositor on Linux is not tested on the current infrastructure.
[Risks and why]: No known risks.
[String/UUID change made/needed]: N/A
Attachment #8665657 - Flags: approval-mozilla-beta?
Attachment #8665657 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/11b3778d51e0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8665657 [details] [diff] [review]
Require MOZ_GLX_USE_SURFACE_SHARING to enable WebGL surface sharing on GLX. Carry r=jgilbert

Fix a crash, taking it. Should be in 42 beta 2.
Attachment #8665657 - Flags: approval-mozilla-beta?
Attachment #8665657 - Flags: approval-mozilla-beta+
Attachment #8665657 - Flags: approval-mozilla-aurora?
Attachment #8665657 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.