565833 - Add a GLX backend to GLContextProvider

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Partially working attempt 1

This appears to render video for my via opengl, but suffers from horrible performance (0.1fps approximately).
Basic profiling suggests most of the CPU time is being spent within libGLcore.so.

Attempting to run this on karl machine required adding doublebuffering support (only double buffered visuals were available) and now errors inside LayerManagerOGL::Initialize().

Line 157 - glTextImage2D throws GL_INVALID_VALUE on the first iteration (GL_TEXTURE_2D).
Stepping through this and clearing the error flags lets the second iteration complete, but the frame buffer status check (line 174) fails both times.

Could someone please try this on their hardware and see if any of the problems are resolved?
Don't know enough about gl coding to track the issues down currently, any suggestions would be appreciated.

Comment 2

[Matt Woodrow (:mattwoodrow)]

glCheckFramebufferStatus appears to be returning 0 on both iterations, which isn't one of the documented return values for it.

Comment 3

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

What hardware and drivers are you testing this on?

Comment 4

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

Also, on karl's and your machines, what versions of GL are actually supported?  Pasting the output of glxinfo would help.

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: glxinfo with radeon 6.12.2 for Mobility X1400

(II) Module radeon: vendor="X.Org Foundation"
        compiled for 1.5.3, module version = 6.12.2
        Module class: X.Org Video Driver
        ABI class: X.Org Video Driver, version 4.1

(--) RADEON(0): Chipset: "ATI Mobility Radeon X1400" (ChipID = 0x7145)

(II) RADEON(0): Framebuffer space used by Firmware (kb): 20
(II) RADEON(0): Start of VRAM area used by Firmware: 0x7ffb000
(II) RADEON(0): AtomBIOS requests 20kB of VRAM scratch space
(II) RADEON(0): AtomBIOS VRAM scratch base: 0x7ffb000
(II) RADEON(0): Cannot get VRAM scratch space. Allocating in main memory instead

(II) RADEON(0): [dri] Found DRI library version 1.3.0 and kernel module version 1.29.0

This might be relevant:

(**) RADEON(0): Option "FBTexPercent" "0"

             "Specifying 0 results in all offscreen video
              RAM being reserved for EXA and only GART memory being  available
              for  OpenGL  textures."

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Glxinfo output

Comment 7

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

Karl's driver only supports OpenGL 1.4, and does not explicitly export GL_EXT_framebuffer_object -- mesa frequently provides dummy implementations of entries, so checking whether the function entries exist isn't sufficient to determine whether a feature is present and/or available.  We need to add explicit version and extension checks in the GL context providers.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Alright, I'll add a check for GL_EXT_framebuffer_object.

Regarding the terrible performance on my machine, I've narrowed it down to massive amounts of time being spent inside calls to glDrawArrays (3 seconds plus per call). oprofile shows 80%+ of the CPU time being spent inside libGLcore.so

This sounds like some sort of software fallback, but I can't find any information on how to detect if this is happening.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Updated patch

Added gl version string and gl extension checking.

This definitely needs testing on other hardware, appears to work for me though.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Added CreatePBuffer support.

Rebased against tip (vlad's Canvas layers changes) and added support for CreatePBuffer.

Tested with WebGL examples and works.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #10)
> Created an attachment (id=446135) [details]

Building this patch against 1e9af0857103, I get

GLContextProviderGLX.cpp
In file included from /usr/include/GL/glx.h:351,
                 from ../../../dist/include/GLXLibrary.h:41,
                 from /home/cjones/mozilla/mozilla-central/gfx/thebes/src/GLContextProviderGLX.cpp:41:
/usr/include/GL/glxext.h:803: error: ‘GLboolean’ has not been declared
make[6]: *** [GLContextProviderGLX.o] Error 1
make[5]: *** [libs] Error 2
make[4]: *** [libs] Error 2
make[3]: *** [libs_tier_platform] Error 2
make[2]: *** [tier_platform] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2

on this machine

$ uname -a && glxinfo 
Linux beast 2.6.31-21-generic #59-Ubuntu SMP Wed Mar 24 07:28:27 UTC 2010 x86_64 GNU/Linux
name of display: :0.0
display: :0  screen: 0
direct rendering: Yes
server glx vendor string: NVIDIA Corporation
server glx version string: 1.4
[snip]
OpenGL vendor string: NVIDIA Corporation
OpenGL renderer string: GeForce GT 220/PCI/SSE2
OpenGL version string: 3.0.0 NVIDIA 185.18.36
OpenGL shading language version string: 1.30 NVIDIA via Cg compiler
[snip]

Comment 12

[Karl Tomlinson (:karlt)]

Chris, I suspect the solution is to not define __gl_h_ here:
http://hg.mozilla.org/mozilla-central/annotate/6d70525ff299/gfx/thebes/public/GLDefs.h#l43

because gl.h hasn't been included.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Fixed compile issue on some systems

Comment 14

[Bas Schouten (:bas.schouten)]

Comment on attachment 446380 [details] [diff] [review]
Fixed compile issue on some systems

In general it would be a good idea to let someone else look at the widget part of this patch, and some of the linux stuff. Vlad suggested karlt. Here's my comments. It should probably be split up into a Widget patch and a GFx patch.

>@@ -190,6 +193,7 @@
>     return false;
>   }
> 
>+  mGLContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0);

Why is this needed?

>+
>+#if defined LINUX
>+    /**
>+     * Creates an appropriate XVisualInfo setup for the current system.
>+     *
>+     * @return XVisualInfo pointer. Must not be freed.
>+     */
>+    static XVisualInfo *CreateVisualInfo();
>+#endif

Will this not also build in some Linux situations where we won't have XVisualInfo ? We probably need to check for some GDK specific thing? Should ask karlt about this or something. Also I'm really not happy about having this in here, but I don't see a better way for it right now sadly.

>     PFNWGLCREATECONTEXTPROC fCreateContext;
>diff -r f92702311196 gfx/thebes/src/GLContext.cpp
>--- a/gfx/thebes/src/GLContext.cpp	Wed May 19 09:56:42 2010 +1200
>+++ b/gfx/thebes/src/GLContext.cpp	Thu May 20 11:23:52 2010 +1200
>@@ -315,10 +315,52 @@
> 
>     };
> 
>-    mInitialized = LoadSymbols(&symbols[0], trygl, prefix);
>+    if (!LoadSymbols(&symbols[0], trygl, prefix)) {
>+        return PR_FALSE;
>+    }
>+    
>+    MakeCurrent();
>+    if (atof((const char *)fGetString(LOCAL_GL_VERSION)) < 2.0) {
>+        return PR_FALSE;
>+    }

I thought a bunch of the functions we look for don't exist on GL versions smaller than 2.0 anyway? So I don't think this is needed. In any case I'm not sure I'm happy about just atof here.. atof(NULL) is not allowed iirc.

>+PRBool GLContext::IsExtensionSupported(const char *extension)

Good idea to have this, if we end up using this more we may want to store it in some more clever way for quicker repeated use.

>+    if (!LibrarySymbolLoader::LoadSymbols(mOGLLibrary, &symbols[0])) {
>+        NS_WARNING("Couldn't find required entry point in OpenGL DLL");

Technically you're not looking in a DLL ;-)

>+already_AddRefed<GLContext>
>+GLContextProvider::CreatePBuffer(const gfxIntSize &aSize, const ContextFormat& aFormat)
>+{
>+    if (!sGLXLibrary.EnsureInitialized()) {
>+        return nsnull;
>+    }
>+
>+    nsTArray<int> attribs;
>+
>+#define A1_(_x)  do { attribs.AppendElement(_x); } while(0)
>+#define A2_(_x,_y)  do {                                                \
>+        attribs.AppendElement(_x);                                      \
>+        attribs.AppendElement(_y);                                      \
>+    } while(0)

Personally I'd prefer if these are defined outside of the function body.

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to comment #14)
> >+#if defined LINUX
> >+    /**
> >+     * Creates an appropriate XVisualInfo setup for the current system.
> >+     *
> >+     * @return XVisualInfo pointer. Must not be freed.
> >+     */
> >+    static XVisualInfo *CreateVisualInfo();
> >+#endif
> 
> Will this not also build in some Linux situations where we won't have
> XVisualInfo ? We probably need to check for some GDK specific thing?

Xlib.h will pull in XVisualInfo and I expect X11/Qt builds will want this too, so MOZ_X11 is probably the appropriate thing to test here.

(I don't know whether or not MOZ_DFB will use this file nor how GL works with DirectFB.)

Comment 16

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

(In reply to comment #14)
> >+    MakeCurrent();
> >+    if (atof((const char *)fGetString(LOCAL_GL_VERSION)) < 2.0) {
> >+        return PR_FALSE;
> >+    }
> 
> I thought a bunch of the functions we look for don't exist on GL versions
> smaller than 2.0 anyway? So I don't think this is needed. In any case I'm not
> sure I'm happy about just atof here.. atof(NULL) is not allowed iirc.

Mesa /always/ returns symbols (as do many other opengl implementations), even if the underlying hardware/driver/whatever combo does not support a particular extension.  So we need a check beyond just looking for the symbols; that's where the GL version check came from.

Checking for GL 2.0 is ok, but there might well be some systems out there that support the set of functionality we want via the extensions, but don't advertise OpenGL 2.0.  Not sure we care.  However, if we do go with just a GL version check, then we need to make sure that what we're using really is just GL 2.0 (and not stuff that was made core in 2.1 or whenever).  Otherwise we have to check GL_EXTENSIONS directly for the set of things that we need.

Comment 17

[Karl Tomlinson (:karlt)]

(In reply to comment #16)
> Checking for GL 2.0 is ok, but there might well be some systems out there that
> support the set of functionality we want via the extensions, but don't
> advertise OpenGL 2.0.  Not sure we care.

A useful data point might be the implication at http://xorg.freedesktop.org/wiki/RadeonFeature that because ATI cards older than R600 don't support ARB NPOT textures fully, they don't fully support OpenGL 2.0.  I don't know whether the missing features can or will be done in software one day such that the driver may advertise OpenGL 2.0 support.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: Rebased to tip, fixed maemo compile errors and separated widget patch

Comment 19

[Matt Woodrow (:mattwoodrow)]

Attachment: Widget changes

Could someone please submit these two patches to the try server for me?

I've tried submitting using the web form and only got results back for the maemo platform.

Comment 20

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/try/rev/411bcebedaeb
http://hg.mozilla.org/try/rev/6f9c57b46de4

Comment 21

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

Hi, I just tried this patch on a new linux setup, and got a crash very early at firefox startup. I am attaching a backtrace obtained using MOZ_X_SYNC=1 as per the recommendation I got from the console output.

The linux system is Fedora 13 / x86-64 with the proprietary nvidia driver.

Comment 22

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

Attachment: crash backtrace (fedora 13, nvidia proprietary driver)

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 447475 [details] [diff] [review]
Widget changes

>+            cachedVisual = gdk_x11_screen_lookup_visual(gdkscreen, xvisual->visualid);
>+            if (visual) {
owAttr attributes;

Change this to if (cachedVisual).

Comment 24

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

I retried with the fix given in comment 23 (and another difference was that this time I had the patch from bug 569775 applied) and this fixes it. Moreover, I can now enjoy hw-accelerated WebGL for the first time. Thanks!

Comment 25

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

Can we get this checked in in its current state, and do followup fixes for various drivers/etc?  It's not something that's enabled by default (and we can even add a pref for fullscreen video if needed), but it would make WebGL work again on Linux (where it works, at least).

Comment 26

[Matt Woodrow (:mattwoodrow)]

Attachment: Fixed unitialized variable

Comment 27

[Matt Woodrow (:mattwoodrow)]

Attachment: Moved visual/colormap creation into top level window (mozcontainer)

Comment 28

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

Comment on attachment 447474 [details] [diff] [review]
Rebased to tip, fixed maemo compile errors and separated widget patch

This looks fine to me.  We should probably do a gl version check as well as looking for the extension, for whatever version FBOs became core -- as is, if we were to create a forward-looking GL4 context, we'd fail that extension check even though everything needed is supported.  But we can fix that up later.

Comment 29

[Oleg Romashin (:romaxa)]

Attachment: glxinfo from my environment

I've tried these patches, and it seems freeze forever or rendering very slow, and gdb always show that browser stay in libGL library.

Comment 30

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

This is funny, because I am using the nVidia proprietary driver like Oleg, and am not seeing any freeze/slowdown.

Comment 31

[Karl Tomlinson (:karlt)]

Comment on attachment 449188 [details] [diff] [review]
Moved visual/colormap creation into top level window (mozcontainer)

There are two big changes here:

1) Changing the visual on the windows.

   There is a risk here that some window managers may get confused.  I hope
   this we be fine, but I think it is worth making this change conditional on
   mozilla.widget.accelerated-layers.  That gives people the option to revert
   if necessary, and it makes sense to continue to use the system visual if
   there is no need for a GL-chosen visual.

   This visual should be set on the (toplevel) mShell.  (Sorry, I misadvised
   re mozcontainer here, but the existing colormap setting code in
   mozcontainer is in the wrong place.)  Setting the colormap on the mShell
   should also make it easier to release the colormap in
   nsWindow::ReleaseGlobals().

2) Use of LayerManager::LAYERS_OPENGL.

   There have been two reports of a big regression with NVIDIA drivers, as
   well as one positive report, but not much other testing.

   It doesn't seem right to cause a known big regression here, so I'd suggest
   defaulting mozilla.widget.accelerated-layers to off for now until we can
   come up with a workaround for this issue.

Comment 32

[Karl Tomlinson (:karlt)]

(In reply to comment #25)
> Can we get this checked in in its current state, and do followup fixes for
> various drivers/etc?  It's not something that's enabled by default (and we can
> even add a pref for fullscreen video if needed), but it would make WebGL work
> again on Linux (where it works, at least).

We can probably land attachment 447474 [details] [diff] [review] (thebes/layers parts) if/when it has
had enough review.

One thing I notice is that GLContextProvider::CreateForWindow should use the
visual from the window rather than re-choosing a visual and expecting it to be
the same.  (Maybe that won't matter in existing code - I haven't checked
thoroughly.)

Comment 33

[Matt Woodrow (:mattwoodrow)]

Thanks Karl, I'll organize getting a linux box set up at home to address these comments.

Landing attachment 447474 [details] [diff] [review] probably requires the CreateForWindow section to be replace with 'return nsnull'. Seems worthwhile to get WebGL support added sooner rather than later.

Comment 34

[Oleg Romashin (:romaxa)]

Comment on attachment 447474 [details] [diff] [review]
Rebased to tip, fixed maemo compile errors and separated widget patch


>--- /dev/null
>+++ b/gfx/thebes/public/GLXLibrary.h
...
>+#include "GLContext.h"
>+#include <gdk/gdkx.h>
>+typedef realGLboolean GLboolean;
>+#include <GL/glx.h>

>+#include <gtk/gtk.h>
why do we need this header here?

Comment 35

[Oleg Romashin (:romaxa)]

Attachment: Additional patch for building GLX on linux Qt desktop

Comment 36

[Karl Tomlinson (:karlt)]

There are some other issues that changing the visual will cause.  They are
significant enough that they should preclude this visual change happening by
default in the GTK port.  I expect a fix would be a bit involved, so should happen in another bug.  (The changes here can still be made but with the pref defaulting to off.)

1. GTK Native theme rendering assumes that themes only understand the system
   visual and so things will get very slow with another visual.
   http://hg.mozilla.org/mozilla-central/annotate/f21132993dc2/widget/src/gtk2/nsNativeThemeGTK.cpp#l780
   http://hg.mozilla.org/mozilla-central/annotate/f21132993dc2/gfx/thebes/src/cairo-xlib-utils.c#l300

   I'm guessing (but not certain) that most themes will probably draw fine
   to non-system visuals that do not have an alpha channel
   (though this is not what the code currently tests for).

2. LookupGdkColormapForVisual won't know a colormap for the new visual:
   http://hg.mozilla.org/mozilla-central/annotate/f21132993dc2/gfx/thebes/src/gfxPlatformGtk.cpp#l803

   I expect this will be needed in some situations here:
   http://hg.mozilla.org/mozilla-central/annotate/f21132993dc2/gfx/thebes/src/gfxGdkNativeRenderer.cpp#l60

Comment 37

[Oleg Romashin (:romaxa)]

Comment on attachment 447474 [details] [diff] [review]
Rebased to tip, fixed maemo compile errors and separated widget patch


>-    mInitialized = LoadSymbols(&symbols[0], trygl, prefix);
>+    if (!LoadSymbols(&symbols[0], trygl, prefix)) {
>+        return PR_FALSE;
>+    }
>+    
>+    mInitialized = IsExtensionSupported("GL_EXT_framebuffer_object");
>+    return mInitialized;

By some reason this GL_EXT_framebuffer_object extension not available on EGL... and it always fails here...

Comment 38

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

This is a GLX backend, not an EGL backend.  Why is this code being used on EGL at all?

Comment 39

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

The title of this bug has been bugging me for a while... fixing it :)

Comment 40

[Oleg Romashin (:romaxa)]

(In reply to comment #38)
> This is a GLX backend, not an EGL backend.  Why is this code being used on EGL
> at all?

Because this is inside of GLContext.cpp... and that part is common for all backends

Comment 41

[Oleg Romashin (:romaxa)]

EGL is calling GLContext::InitWithPrefix

Comment 42

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

Oh wow, sorry -- didn't realize that was in the common code.  That should absolutely not be there, needs to get moved out to the platform-specific GL context code.

Comment 43

[Matt Woodrow (:mattwoodrow)]

It looks like changing the visual of widgets/windows is going to be more trouble than it's worth, so maybe it would be a better solution to get GL to match the existing visual.

I think the best method would be to switch to using glxChooseFBConfig, and iterate the results, looking for one that matches the current widgets visual.

I've setup a quick test and it appears that the current visual is a double buffered one (not sure what this actually means in terms of a X visual). Hard coding the check to pick the FBConfig with the closest matching visual (identical except for double buffered flag) appears to work fine on my system, not sure if this is really an acceptable solution though.

Also, since this is unnecessarily blocking WebGL, maybe it should be separated into two bugs and we can get the requirements for WebGL landed asap.

Comment 44

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

(In reply to comment #43)
> Also, since this is unnecessarily blocking WebGL, maybe it should be separated
> into two bugs and we can get the requirements for WebGL landed asap.

Would be much appreciated.

Comment 45

[Matt Woodrow (:mattwoodrow)]

Attachment: Context provider with only PBuffer support

I've removed the code to create a context for a window until we find a suitable solution to the visual problem.

I added romaxa's changes for Qt (untested) and moved the extension check into platform specific code.

Comment 46

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

My NVIDIA system runs this patch without crashing (OpenGL renderer string: GeForce GT 220/PCI/SSE2 | OpenGL version string: 3.0.0 NVIDIA 185.18.36).

Comment 47

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

Landed the pbuffers patch as http://hg.mozilla.org/mozilla-central/rev/3b3e795a1c2e

Comment 48

[Matt Woodrow (:mattwoodrow)]

Attachment: Required changes to the GTK widget to support GL accelerated rendering

Separated the GTK widget changes into a separate patch. This should be all that is required for EGL to work.

Comment 49

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/624440087028

We should close this bug when the plugin fix lands.

Comment 50

[Matt Woodrow (:mattwoodrow)]

We aren't dependant on the plugin change because we don't need to create any Colormap/Visuals any more.