565417 - WebGL: add ability to use OSMesa rendering

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, enhancement)

Description

[Benoit Jacob [:bjacob] (mostly away)]

The attached patch adds the ability for the 3D Canvas to use the OSMesa rendering library. This means software-only off-screen rendering.

OSMesa is used to render to the underlying bitmap of a gfxImageSurface, and WebGLContext::GetCanvasLayer() then uses this image directly.

This is disabled by default. In order to test it, go to about:config and set webgl.osmesalib to the path to your OSMesa library, for example /usr/lib/libOSMesa.so

This may be useful for testing / debugging and perhaps also as a possible fall-back when nothing better is available.

This doesn't add any dependency (buildtime or runtime) on OSMesa. The library is dlopen'd at runtime.

Some questions:

1) The struct GLContextProviderOSMesa::ContextFormat was copied from GLContextProvider::ContextFormat. So some stuff here doesn't actually apply to OSMesa, which doesn't have a ChoosePixelFormat function and only knows a few pixel formats. Should we modify that struct to make it more OSMesa-ish at the expense of making it more different from GLContextProviderOSMesa::ContextFormat? Is it important to keep a uniform API?

2) The static member BasicRGBA32Format is created in GLContext.cpp, which is probably not the right place for such OSMesa-specific stuff. Feel free to move it. I just had a strange linker error when trying to put it in GLContextProviderOSMesa.cpp.

3) Is it really a good idea to implement stuff like BasicRGBA32Format as static data members? Doesn't that mean that such object get constructed at firefox startup, even if the user never actually uses it? Shouldn't we instead implement e.g. a static method BasicRGBA32Format() returning such an object?

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Patch adding OSMesa support

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Ah yes, more questions. LogMessage wasn't available everywhere I needed it, so I tended to use NS_WARNING when I guess I shouldn't have. As a result the logging is a bit erratic. For example, when the user just hasn't enabled osmesa in about:config, it complains "can't create a OSMesa PBuffer", etc. Need to learn more about LogMessage()...

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Patch adding OSMesa support, take 2

(was mistakenly incorporating the patch for bug https://bugzilla.mozilla.org/show_bug.cgi?id=565287)

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 444947 [details] [diff] [review]
Patch adding OSMesa support

># HG changeset patch
># Parent 501e0cf295a0af7ec35a214b37e810186807fa1d
>
>diff --git a/content/canvas/src/WebGLContext.cpp b/content/canvas/src/WebGLContext.cpp
>--- a/content/canvas/src/WebGLContext.cpp
>+++ b/content/canvas/src/WebGLContext.cpp
>@@ -132,6 +132,17 @@
> 
>     gl = gl::sGLContextProvider.CreatePBuffer(gfxIntSize(width, height));
> 
>+    if(!gl)
>+    {

style: one space between if and the condition, and brace on same line if the condition fits on a single line.  (Same thing in various places elsewhere in the patch.)

>@@ -280,7 +291,22 @@
> 
>     CanvasLayer::Data data;
> 
>-    data.mGLContext = gl.get();
>+    // the gl context may either provide a native PBuffer, in which case we want to initialize
>+    // data with the gl context directly, or may provide a surface to which it renders (this is the case
>+    // of OSMesa contexts), in which case we want to initialize data with that surface.
>+
>+    void* native_pbuffer = gl->GetNativeData(gl::GLContext::NativePBuffer);
>+    void* native_surface = gl->GetNativeData(gl::GLContext::NativeImageSurface);
>+
>+    if(native_pbuffer)
>+        data.mGLContext = gl.get();
>+    else if(native_surface)
>+        data.mSurface = static_cast<gfxASurface*>(native_surface);
>+    else {
>+        NS_WARNING("The GLContext has neither a native PBuffer nor a native surface!");
>+        return nsnull;
>+    }

style: braces around every clause if any of them has braces; the else has them, so need { } everywhere.

>diff --git a/gfx/thebes/public/GLContext.h b/gfx/thebes/public/GLContext.h
>--- a/gfx/thebes/public/GLContext.h
>+++ b/gfx/thebes/public/GLContext.h
>@@ -121,7 +121,8 @@
>       NativeGLContext,
>       NativeCGLContext,
>       NativePBuffer,
>-      NativeDataTypeMax
>+      NativeDataTypeMax,
>+      NativeImageSurface

Wrong way around -- NativeDataTypeMax should be the last element in the enum, or it's no longer the Max :-)

>+/** Same as GLContextProvider but for off-screen Mesa rendering */
>+class THEBES_API GLContextProviderOSMesa
>+{
>+public:
>+    struct ContextFormat {

Don't duplicate this -- just use GLContextProvider::ContextFormat directly.  You can add a typedef inside this class so that you can continue to use ContextFormat as the type elsewhere.

>+
>+    /**
>+     * Creates a PBuffer.
>+     *
>+     * @param aSize Size of the pbuffer to create
>+     * @param aFormat A ContextFormat describing the desired context attributes.  Defaults to a basic RGBA32 context.
>+     *
>+     * @return Context to use for this Pbuffer
>+     */
>+    static already_AddRefed<GLContext> CreatePBuffer(const gfxIntSize &aSize,
>+                                              const ContextFormat& aFormat = ContextFormat::BasicRGBA32Format);
>+
>+    /**
>+     * Create a context that renders to the surface of the widget that is
>+     * passed in.
>+     *
>+     * @param Widget whose surface to create a context for
>+     * @return Context to use for this window
>+     */
>+    static already_AddRefed<GLContext> CreateForWindow(nsIWidget *aWidget);
>+};
>+

>+// XXX had to put that here, as putting it in GLContextProviderOSMesa.cpp gave me a linking error
>+typedef GLContextProviderOSMesa::ContextFormat ContextFormatOSMesa;
>+const ContextFormatOSMesa ContextFormatOSMesa::BasicRGBA32Format(ContextFormatOSMesa::BasicRGBA32);

See above; just get rid of it, no need.

>diff --git a/gfx/thebes/src/GLContextProviderCGL.mm b/gfx/thebes/src/GLContextProviderCGL.mm
>--- a/gfx/thebes/src/GLContextProviderCGL.mm
>+++ b/gfx/thebes/src/GLContextProviderCGL.mm
>@@ -49,7 +49,7 @@
> class CGLLibrary
> {
> public:
>-    CGLLibrary() : mInitialized(PR_FALSE) {}
>+    CGLLibrary() : mInitialized(PR_FALSE), mOGLLibrary(nsnull) {}
> 
>     PRBool EnsureInitialized()
>     {

This is from a different patch -- better to remove it from yours or it'll conflict.

>diff --git a/gfx/thebes/src/GLContextProviderWGL.cpp b/gfx/thebes/src/GLContextProviderOSMesa.cpp
>copy from gfx/thebes/src/GLContextProviderWGL.cpp
>copy to gfx/thebes/src/GLContextProviderOSMesa.cpp

Copying this file was probably a bad idea in retrospect -- the bare structure is the same, but very little of the code is.  Trying to review it as a diff is just more confusing.  (I didn't realize that it would generate a diff against the WGL file... I guess that makes sense, but only when you really did mean to copy and change /some/ things, as opposed to copy and rewrite :-)  Doesn't matter for now, but something to keep in mind for future patches.


>+    if (NS_FAILED(rv) ||
>+        osmesalib.Length() == 0)
>+    {
>+        // XXX should not be a warning, just a normal log message, what should I use here?
>+        NS_WARNING("Canvas 3D: Not trying OSMesa, since it's not specified in webgl.osmesalib");
>+        return PR_FALSE;

I would just remove this warning and return false; no need to warn or even log.  (We need to get rid of a bunch of the log messages anyway, webgl is too chatty right now.)


>+            if (aFormat.alpha == 8)
>+                osmesa_format = OSMESA_BGRA;
>+            else if (aFormat.alpha == 0)
>+                osmesa_format = OSMESA_BGR;

is OSMesa's BGR format packed-pixel or still 32 bits per pixel?  I can't remember, if it's packed then we can't use it (it won't match the cairo/Thebes format).

>+        // XXX without the next line, the image is rendered upside down
>+        sOSMesaLibrary.fPixelStore (OSMESA_Y_UP, 0);

Nuke this for landing, since the wrong-way-up flip happens earlier.  Though if we're using osmesa we can probably get a perf boost by understanding this and not doing the flip anywhere.. something we can fix in the future.

Looks great otherwise -- can you fix up the above, and I'll r+ the next patch?

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Patch adding OSMesa support, take 3

I think this addresses all your comments!

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 444986 [details] [diff] [review]
Patch adding OSMesa support, take 3

Yep, looks great, thanks!  I'll land it for you when I land the layers code.  Will mark that bug as a dependency.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Just one more question. Currently OSMesa is only used as a fallback when native GL PBuffers aren't available. But if we say that OSMesa is useful for testing / debugging, then it should be possible to use it even when actual GL is available. Perhaps a new config option? That would make it easy to switch between OSMesa and actual GL.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Patch adding OSMesa support, without the bad hg cp

carrying forward r+ from previous patch.
This new patch should have exactly the same code, just without the bad hg cp. Tested here.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/9b8c84009138