569236 - A couple of OSMesa improvements

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

The attached patch does a couple of things OSMesa-related:
1) it allows to detect at runtime whether the correct prefix is "mgl" or "gl". Indeed, it seems that using "gl" is not convenient for Windows users, see

http://learningwebgl.com/blog/?p=2266

2) But the console output caused by doing this was awkward so I took this occasion to rework it a bit. The only not-OSMesa-specific change is that LibrarySymbolLoader now also prints the prefix when printing the name of a symbol that failed to load. The rest is OSMesa specific, it consists in adding a static LogMessage() function there (inspired by WebGLContext's) to make message output more consistent/friendly for the user.

Comment 1

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

Attachment: a couple of OSMesa-related improvements

Comment 2

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

Attachment: a couple of OSMesa-related improvements

sorry, the previous patch has unrelated whitespace changes.

Comment 3

[Giles Thomas]

Added myself to cc list

Comment 4

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

Comment on attachment 448391 [details] [diff] [review]
a couple of OSMesa-related improvements

Actually, I don't want to support the mgl prefixed stuff at all -- it only makes things "harder" for win32 users because the DLLs that I built for OSMesa use mangled mgl names (and from an ancient mesa version, too!).  I can rebuild them to use the standard gl prefixes, which should resolve that issue.

Logging message changes look fine though, but let's do those separately.

Comment 5

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

As you like! If you make a gl-prefixed build available, I guess that windows users will be happy.

As you can see, the mgl prefix support is only 1 line in that patch (search for mgl) so if you want to apply only the rest, can you manually edit it? Or tell me if you want an updated patch.

Comment 6

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

Update: what I hadn't understood is that the "gl" prefix _is_ the default of a OSMesa build, and there is no need at all to support other prefixes. The "mgl" thing was a hack, specific to Vlad's old OSMesa/windows build.

Comment 7

[Giles Thomas]

@Benoit -- that's not quite right, "mgl" is a supported alternative prefix -- a normal part of OSMesa rather than a Minefield-specific hack.  It's there so that people who are linking against other GL libraries can compile OSMesa in a way that avoids namespace collisions.  

However, Vladimir's idea of hosting a compiled Windows binary with "gl" prefixes is a great way of supporting OSMesa in Minefield, and if he does that then there's no need at all to support both kinds of prefix, because "gl" is indeed the OSMesa default.  So the plan sounds good :-)

Comment 8

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

Attachment: better OSMesa messages

Here's an version of the patch without the mgl prefix support, only improving messages.

Letting Vlad decide between the 2 patches :-)

Comment 9

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

Comment on attachment 448761 [details] [diff] [review]
better OSMesa messages

This looks good, though now that I read it over, I think we should change the messages some more:

Let's get rid of the "Creating PBuffer..." message entirely and the "Canvas 3D: Ready" message.

>-        LogMessage("Canvas 3D: can't get a native PBuffer, trying OSMesa...");
>+        LogMessage("Canvas 3D: can't get a native PBuffer.");

This is good; no need to mention OSMesa.

>         gl = gl::GLContextProviderOSMesa::CreatePBuffer(gfxIntSize(width, height), format);
>-        if (!gl) {
>-            LogMessage("Canvas 3D: can't create a OSMesa pseudo-PBuffer.");
>+        if (!gl)
>             return NS_ERROR_FAILURE;
>-        }

If OSMesa context creation succeeds, let's log "WebGL: Using software rendering via OSMesa".  (But do it here, not in the OSMesa context provider -- that could well be used outside of webgl.)

>+    LogMessage("Creating OSMesa pseudo-pbuffer...");

Let's get rid of this; no need to spam.

>     mOSMesaLibrary = PR_LoadLibrary(osmesalib.get());
> 
>     if (!mOSMesaLibrary) {
>-        NS_WARNING("Canvas 3D: Couldn't open OSMesa lib -- webgl.osmesalib path is incorrect, or not a valid shared library");
>+        LogMessage("Couldn't open OSMesa lib -- webgl.osmesalib path is incorrect, or not a valid shared library");

maybe say "OSMesa for software rendering"

> 
>     if (!ValidateGL()) {
>         LogMessage("Canvas 3D: Couldn't validate OpenGL implementation; is everything needed present?");

^ Change this to say "WebGL: OpenGL implementation doesn't provide all needed functionality".  I can't remember how the control flow goes here; if we're trying a hardware pbuffer, it might be worth jumping back up to the top and trying OSMesa first, before dumping the error.

>         return NS_ERROR_FAILURE;
>     }
> 
>     LogMessage("Canvas 3D: ready");

^ Nuke this

Comment 10

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

Attachment: better OSMesa messages, updated

Comment 11

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

The new patch applies all your comments. For this one:

>>     if (!ValidateGL()) {
>>         LogMessage("Canvas 3D: Couldn't validate OpenGL implementation; is > everything needed present?");
>
> ^ Change this to say "WebGL: OpenGL implementation doesn't provide all needed
> functionality".  I can't remember how the control flow goes here; if we're
> trying a hardware pbuffer, it might be worth jumping back up to the top and
> trying OSMesa first, before dumping the error.

I changed the flow as you suggested: first validate native GL, if fail then try OSMesa. I thought there was no need to validate OSMesa, on the other hand.

Comment 12

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

I am stupid, ValidateGL() is actually doing nontrivial initialization work. Updated patch coming...

Comment 13

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

Attachment: better OSMesa messages, updated

Ok, tested, works.

Comment 14

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

Comment on attachment 448807 [details] [diff] [review]
better OSMesa messages, updated

Close!  r+ me with the below changes:

>+        LogMessage("WebGL: can't get a native PBuffer.");

Don't print this -- we don't want it to be chatty in the normal case.

>     }
> 
>-    if (!ValidateGL()) {
>-        LogMessage("Canvas 3D: Couldn't validate OpenGL implementation; is everything needed present?");
>-        return NS_ERROR_FAILURE;
>+    if (gl && !ValidateGL()) {
>+        LogMessage("WebGL: OpenGL implementation doesn't provide all needed functionality");
>+        gl = nsnull;
>     }
> 
>-    LogMessage("Canvas 3D: ready");
>+    if (!gl) {
>+        gl = gl::GLContextProviderOSMesa::CreatePBuffer(gfxIntSize(width, height), format);
>+        if (gl && ValidateGL())
>+            LogMessage("WebGL: Using software rendering via OSMesa");
>+        else
>+            return NS_ERROR_FAILURE;

^ here would be a good place to log that we failed to create any pbuffer.

Comment 15

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

Attachment: better OSMesa messages, updated

Note: I'm not sure by what is meant by "r+ me". If I select "+", I can't select you in the "requestee" field.

Comment 16

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

Ugh. The patch doesn't apply well anymore. Need to redo it against the current code.

Comment 17

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

Attachment: Better webgl messages

Here's an updated patch to compile against current moz-central; also I have renamed ValidateGL() to InitAndValidateGL() since that function was already doing some initialization work; and I moved more (re-)initialization code to it, which I had to in order to be able to keep this patch clean.

Comment 18

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

Comment on attachment 451039 [details] [diff] [review]
Better webgl messages

Looks good, with one small change:

>     gl = gl::sGLContextProvider.CreatePBuffer(gfxIntSize(width, height), format);
> 
>-    if (!gl) {
>-        LogMessage("Canvas 3D: can't get a native PBuffer, trying OSMesa...");
>+    if (!gl || !InitAndValidateGL()) {
>         gl = gl::GLContextProviderOSMesa::CreatePBuffer(gfxIntSize(width, height), format);
>-        if (!gl) {
>-            LogMessage("Canvas 3D: can't create a OSMesa pseudo-PBuffer.");
>+        if (!gl || !InitAndValidateGL()) {
>+            LogMessage("WebGL: Can't get a usable OpenGL context.");
>             return NS_ERROR_FAILURE;
>         }
>+        else {
>+            LogMessage("WebGL: Using software rendering via OSMesa");
>+        }

In the above blocks, no need for !gl, because...

> PRBool
>-WebGLContext::ValidateGL()
>+WebGLContext::InitAndValidateGL()
> {
>+    if(!gl) return PR_FALSE;

You check for it here.

So get rid of one or the other check -- also space after the "if" here, and stick the return on a separate line.

Comment 19

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

http://hg.mozilla.org/mozilla-central/rev/4835fc661241

and sorry, forgot to put the return on a separate line.