575469 - extend GLContext interface to understand offscreen & sharing more

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

In bug 575032, we create a generic CreateOffscreen with the assumption that we'd always be able to share resources amongst all contexts, and so offscreen rendering can happen just via passing the texture for the color attachment of a FBO in the offscreen context.

However, on some Android devices, context sharing does not seem to be supported.  In addition, for WebGL, context sharing isn't /really/ what we want -- an entirely separate context would be much better, if we could export just the color buffer.  Here's a grid of what's supported where -- "pbuffers" means specifically pbuffer support with bind-to-texture, not just pbuffer.  Sharing is context sharing.  Other is if there's another mechanism available, see below.

                | pbuffers | sharing | other
Desktop - WGL   |   YES    |   YES   |
Desktop - GLX   |    no    |   YES   |   YES
Desktop - CGL   |   YES    |   YES   |   YES
Maemo - N900    |   YES    |   YES   |   YES
Maemo - Future  |    no    |   YES   |   YES
Android - Tegra |    no    |   YES   |   YES
Android - N1    |   YES    |    no   |
Android - Droid |   YES    |    no   |
Android - EVO4G |   YES    |    no   |

Desktop - GLX - Other: X11 Pixmaps can be rendered to and can be bound as textures
Desktop - CGL - Other: I think there's some CoreAnimation thing that can be used here?
Maemo - N900/Future - Other: X11 Pixmaps, potentially EGL_KHR_gl_texture_2D_image
Android - Tegra - Other: EGL_KHR_gl_texture_2D_image is supported; this allows exporting a 2D texture from one context as an EGLImage, which can be bound as a texture in another context

Unfortunately that means there's no one-size-fits-all for our two use cases:

1. Share resources amongst internal layer managers and their details, so that we can do things like share resources amongst windows, do resource deletion with a generic context, and do texture upload on another thread.

2. WebGL, which just wants the ability to bind its color framebuffer as a texture in the GL Layer Manager's context.

I propose that we make GLContext a little smarter, because we'll have to handle both sharing available and not available, and various forms of offscreen rendering for WebGL.

On GLContext:

- there should be a boolean getter that indicates whether this GLContext shares resources with a global context or not.  LayerManager and others can then use this to decide what to do with its implementation.

- add IsOffscreen() and ResizeOffscreen().  Only would work on GLContexts returned by CreateOffscreen, and would do what their name suggests.

- add MakeOffscreenColorBufferActive() (or something similar) -- this would serve the equivalent of BindFramebuffer(GL_FRAMEBUFFER, 0), except that if under the hood our offscreen context is actually backed by a FBO, that 0 would be whatever the FBO id is.  This would hide that from the user.  Another option is to add a small wrapper to fBindFramebuffer and trap binding 0 and do the right thing.  This would require wrapping some getters and a few other misc functions as well, but is not a big deal to do. 

- add BindOffscreenAsTexture(GLenum texTarget, GLContext *context_to_bind).  This would do whatever magic is necessary to bind the given offscreen GLContext as the given texture target.  This might need to be a little smarter -- for example, it might need to return a texture ID or some other kind of resource, but we can figure out those details.

On GLContextProvider:

- keep the CreateOffscreen API, but give it back a ContextFormat argument (so that if we have to use a pbuffer as the render offscreen render target, we can create it correctly).

For use case #2, the different ways of binding a GLContext as a texture that I propose that we support:

- no sharing & [any] context & FBO & EGL_KHR_gl_texture_2d_image
  [Android/Tegra, Maemo]

- no sharing & pbuffer context & pbuffer binding
  [Android/Other, maybe WGL & CGL]

- (maybe) no sharing & pixmap context & texture_from_pixmap
  [GLX?]

- full sharing & FBO as backing store
  [wherever it's supported]

this is basically the full gamut, which unfortunately seems to be necessary; Android/Tegra requires the first (or the last, but the first is preferred), Android/Other requires the second, X11 the third or the last.  I think the above would also be order of preference.

This complicates the GL layers implementation, especially with the ability for sharing to be on or off.  The code to do sharing isn't large, so the question is whether there's enough benefit to sharing when it's available to have code paths for it.  I /think/ the answer is yes.

Comment 1

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

Attachment: add sharing/offscreen support to GLContexts

The majority of the work here.  Includes the context sharing work from bug 575032, as well as a revamped offscreen api.  Not implemented yet on CGL, will do that soon.

I need some help on GLX here; the GLX backend before was only accidentally working as it was, because it relied on GLX 1.3 functionality without checking for GLX 1.3 (FBConfigs and CreateNewContext specifically).  Unfortunately, I only have a VirtualBox instance, which only provides GLX 1.2.  Would be nice if we could support GLX 1.2 here.

Comment 2

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

Attachment: use offscreen API in layers

Comment 3

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

Attachment: 1 - add sharing/offscreen support to GLContexts (updated to trunk)

Comment 4

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

Attachment: 2 - use offscreen API in layers (updated to trunk)

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 455606 [details] [diff] [review]
1 - add sharing/offscreen support to GLContexts (updated to trunk)

>+    int serverVersion = 0, clientVersion = 0;
>+    if (serverVersionStr &&
>+        strlen(serverVersionStr) > 3 &&

>= 3 or == 3 maybe? My version string is "1.4"

>+
>+    if (clientVersion &&

s/clientVersion/clientVersionStr/

>+        strlen(clientVersionStr) > 3 &&

as above.

WFM with these changes.

Comment 6

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

Attachment: 1 - add sharing/offscreen support to GLcontexts

Updated again, tested on windows, mac, and a linux.

Comment 7

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

Attachment: 2 - use offscreen API in layers

updated again after testing on Mac.

Comment 8

[Bas Schouten (:bas.schouten)]

Comment on attachment 458056 [details] [diff] [review]
1 - add sharing/offscreen support to GLcontexts

This looks good to me for now. I've got a couple of question marks but they can be addressed in follow-ups.

>@@ -199,11 +202,9 @@ public:
>   }
> 
> protected:
>-  GLContext *mGL;
>+  nsRefPtr<GLContext> mGL;

We should probably make sure we note, that a GL context cannot hold a reference to its widget with this in place. Since this would mean a reference cycle Widget->LayerManager->GLContext->widget.

>+        fTexImage2D(LOCAL_GL_TEXTURE_2D,
>+                    0,
>+                    LOCAL_GL_RGB,
>+                    aSize.width, aSize.height,
>+                    0,
>+                    LOCAL_GL_RGB,
>+                    isMobile ? LOCAL_GL_UNSIGNED_SHORT_5_6_5

Do we know if the UNSIGNED_SHORT_5_6_5 format is supported on all our mobile devices?

>+void
>+GLContext::CreatedRenderbuffers(GLContext *aOrigin, GLsizei aCount, GLuint *aNames)
>+{
>+    for (GLsizei i = 0; i < aCount; ++i) {
>+        mTrackedRenderbuffers.AppendElement(NamedResource(aOrigin, aNames[i]));
>+    }
>+}

I think maybe some macros here could help.

Macro's for all the following stuff maybe?

>+#ifndef DEBUG
>+    GLuint GLAPIENTRY fCreateProgram() {
>+        return priv_fCreateProgram();
>+    }
>+


>diff --git a/gfx/thebes/GLContextProviderWGL.cpp b/gfx/thebes/GLContextProviderWGL.cpp
>--- a/gfx/thebes/GLContextProviderWGL.cpp
>+++ b/gfx/thebes/GLContextProviderWGL.cpp
>@@ -49,9 +49,62 @@ namespace gl {

Do we -really- need to create a window to create our GLContext? Is it not possible to create simply a DDB and create a context around that DC?

>+    // This is ridiculous -- we have to actually create a context to
>+    // get the OpenGL ICD to load.

Wouldn't a LoadLibrary call be able to do that?

>-        // XXX fixme after bug 571092 lands and we have the format available

The patch on the bug was r+'ed.

>+    HDC dc;
>+    HWND win = CreateDummyWindow(&dc);
>+    if (!win) {
>+        return nsnull;
>+    }
>+    
>+    HGLRC context = sWGLLibrary.fCreateContext(dc);
>+    if (!context) {
>+        return nsnull;
>+    }
>+
>+    if (!sWGLLibrary.fShareLists(shareContext->Context(), context)) {
>+        NS_WARNING("wglShareLists failed!");
>+
>+        sWGLLibrary.fDeleteContext(context);
>+        DestroyWindow(win);
>+        return nsnull;
>+    }
>+
>+    nsRefPtr<GLContextWGL> glContext = new GLContextWGL(aFormat, shareContext,
>+                                                        dc, context, win, PR_TRUE);
>+
>+    return glContext.forget();
>+}

I wonder again if we couldn't do with a simpler DC than a window.

Comment 9

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

(In reply to comment #8)
> >@@ -199,11 +202,9 @@ public:
> >   }
> > 
> > protected:
> >-  GLContext *mGL;
> >+  nsRefPtr<GLContext> mGL;
> 
> We should probably make sure we note, that a GL context cannot hold a reference
> to its widget with this in place. Since this would mean a reference cycle
> Widget->LayerManager->GLContext->widget.

Will do, I'll double check that -- the current ones don't, they just create a GL context and hold on to that.

> Do we know if the UNSIGNED_SHORT_5_6_5 format is supported on all our mobile
> devices?

Yep, it's required.

> >+void
> >+GLContext::CreatedRenderbuffers(GLContext *aOrigin, GLsizei aCount, GLuint *aNames)
> 
> I think maybe some macros here could help.
> 
> Macro's for all the following stuff maybe?

Yeah, maybe; I'll take a look.  Some take different args, so was easier to just write them out.. especially given that there won't be any more anytime soon.

> >--- a/gfx/thebes/GLContextProviderWGL.cpp
> >+++ b/gfx/thebes/GLContextProviderWGL.cpp
> >@@ -49,9 +49,62 @@ namespace gl {
> 
> Do we -really- need to create a window to create our GLContext? Is it not
> possible to create simply a DDB and create a context around that DC?

Sadly, yes; creating a GL context for a DDB is wonky from the last time I tried it.  Works some places, doesn't work others, and often gets you the software renderer.  I could use a pbuffer here, but that's not any simpler.  However, we could use the "hidden window" we already have sitting around, but that can be done as a followup...

> >+    // This is ridiculous -- we have to actually create a context to
> >+    // get the OpenGL ICD to load.
> 
> Wouldn't a LoadLibrary call be able to do that?

Nope, the ICD is the driver-vendor provided opengl runtime; we load the generic microsoft opengl32.dll, which in turn selects the appropriate ICD to load when a context is first created.  If we don't do this, we have no way of querying the right GL extension functions, since we'll end up querying those that come from microsoft's software impl (most of which will be null, since it doesn't support much).  We have to make the right context current before we can query the right driver's functions.

> >-        // XXX fixme after bug 571092 lands and we have the format available
> 
> The patch on the bug was r+'ed.

Yeah, I need to revisit it after this.

Thanks!

Comment 10

[Bas Schouten (:bas.schouten)]

Comment on attachment 458057 [details] [diff] [review]
2 - use offscreen API in layers

>diff --git a/gfx/thebes/GLContext.h b/gfx/thebes/GLContext.h
>--- a/gfx/thebes/GLContext.h
>+++ b/gfx/thebes/GLContext.h
>@@ -1113,10 +1113,10 @@ public:
> inline void
> GLDebugPrintError(GLContext* aCx, const char* const aFile, int aLine)
> {
>-  GLenum err = aCx->fGetError();
>-  if (err) {
>-    fprintf(stderr, "GL ERROR: 0x%04x at %s:%d\n", err, aFile, aLine);
>-  }
>+    GLenum err = aCx->fGetError();
>+    if (err) {
>+        printf_stderr("GL ERROR: 0x%04x at %s:%d\n", err, aFile, aLine);
>+    }
> }
> 
> #ifdef DEBUG

This whitespace change is a bit out of context for this patch? Although obviously it's a correct change.

Comment 11

[Oleg Romashin (:romaxa)]

GTK port always fail to initialize GLContext, because  GLX version check is broken.
I have nvidia and "1.4", and that never parsed by 
******************************
    int serverVersion = 0, clientVersion = 0;
    if (serverVersionStr &&
        strlen(serverVersionStr) > 3 &&
        serverVersionStr[1] == '.')
    {
        serverVersion = (serverVersionStr[0] - '0') << 8 | (serverVersionStr[2] - '0');
    }
******************************
because strlen(serverVersionStr) == 3...

Comment 12

[Oleg Romashin (:romaxa)]

Also if that check disabled it still does not work on latest nightly...  MOZ_ACCELERATED=1, I see only black screen on GTK build. 

And GTK GL works fine on m-c 47857:b00bb3a115de

Comment 14

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

http://hg.mozilla.org/mozilla-central/rev/e235ebdbef50
http://hg.mozilla.org/mozilla-central/rev/23b8e7fd1794

Let's do the GLX fixups in another bug, this one's gotten a little crufty