Closed Bug 761155 Opened 12 years ago Closed 12 years ago

Extend Mesa LLVMpipe preference to GLX


(Core :: Graphics: CanvasWebGL, defect)

Not set





(Reporter: bjacob, Assigned: drexler)



(Whiteboard: webgl-next)


(1 file, 1 obsolete file)

Bug 731836 added a preference, currently only used on WGL, to use a custom Mesa LLVMpipe library instead of the system OpenGL. This would be useful on linux as well, as there will realistically remain a proportion of Linux users without good OpenGL support for a long time, despite the majority of Linux users now enjoying good OpenGL drivers ( ).

This is probably a prerequisite before we can drop OSMesa support.

On the other hand, I'm not filing a similar bug for Mac OSX (CGL) because that would only be useful for Mac OS 10.5 users and we're going to stop supporting 10.5 rather soon. But if someone wants to write the patch for CGL, I'd r+ it too.
Whiteboard: webgl-next
Assignee: nobody → andrew.quartey
Setting the preference for GLX appears to be irrelevant as Mesa LLVMPipe is the default WebGLRenderer on two different Linux variants i tested a patch for this on - Fedora and Ubuntu.
I didn't know that Ubuntu and Fedora automatically fell back to LLVMpipe when no hardware accelerated OpenGL was possible. Together with the fact that WebGL success rates are particularly high, and increasing fast, on Linux, I can agree with saying that Linux doesn't need us to do anything for software fallbacks.
On the other hand, there will always be a certain portion of users on distros that don't correctly set the LLVMpipe fallback, so a patch here would still be useful. But the fact that it's not needed on Ubuntu and Fedora makes it a bit less useful.
Attached patch patch (obsolete) — Splinter Review
This implements dynamic switching between the default OpenGL library and LLVMpipe for WebGL rendering.
Attachment #655429 - Flags: review?(bjacob)
Comment on attachment 655429 [details] [diff] [review]

Review of attachment 655429 [details] [diff] [review]:

r+, but some nits, and at least the .so.1 nit is 'mandatory' for landing. Thanks!

::: gfx/gl/GLContextProviderGLX.cpp
@@ +93,2 @@
>  #else
> +        const char *libGLfilename = aUseMesaLLVMPipe? "" : "";

Why Since we control the filename here, we may as well have it be unconditionally.

For libGL, we didn't control the filename, and neither nor would work in all cases, whence this ifdef.

@@ +1137,5 @@
>  already_AddRefed<GLContext>
>  GLContextProviderGLX::CreateForWindow(nsIWidget *aWidget)
>  {
> +    LibType libToUse = GLXLibrary::OPENGL_LIB;
> +    if (!sGLXLibrary[libToUse].EnsureInitialized(false)) {

To simplify code, I would define a reference, once and for all, to sGLXLibrary[libToUse].  But make sure to make it a reference to avoid a deep copy / relying on a copy ctor.

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +348,5 @@
>  #if defined(MOZ_WIDGET_GTK2) && !defined(MOZ_PLATFORM_MAEMO)
>      GLXPixmap pixmap;
>      if (cairoImage->mSurface) {
> +        pixmap = sGLXLibrary[GLXLibrary::OPENGL_LIB].CreatePixmap(cairoImage->mSurface);

Likewise, maybe you could find a place to add a shortcut to avoid typing sGLXLibrary[GLXLibrary::OPENGL_LIB] everytime.
Attachment #655429 - Flags: review?(bjacob) → review+
Attached patch patchSplinter Review
Thought to clear these changes with you before landing it.
Attachment #655429 - Attachment is obsolete: true
Attachment #658207 - Flags: review?(bjacob)
Comment on attachment 658207 [details] [diff] [review]

Review of attachment 658207 [details] [diff] [review]:


::: gfx/gl/GLXLibrary.h
@@ +32,5 @@
> +    {
> +      OPENGL_LIB = 0,
> +      MESA_LLVMPIPE_LIB = 1,
> +      LIBS_MAX
> +    };   

Trailing whitespace (according to patch review tool).
Attachment #658207 - Flags: review?(bjacob) → review+
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 789983
Depends on: 868787
You need to log in before you can comment on or make changes to this bug.