572939 - Add CreateForWindow support to GLContextProviderGLX

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Matt Woodrow (:mattwoodrow)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.3) Gecko/20100423 Ubuntu/10.04 (lucid) Firefox/3.6.3
Build Identifier: 

Implement the CreateForWindow function.

Reproducible: Always

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Trying to match GL visual to the current widget

Comment 2

[Matt Woodrow (:mattwoodrow)]

I'm still getting an unload crash using the patch with fglrx because the GLContext is kept alive after the X window is destroyed. Attempting to make this context current with an invalid x id crashes with a BadValue X error.

Comment 3

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

Attachment: Update Matt's patch to work with nvidia drivers

Instead of trying to match the visual, we just use whichever the window already has, even if double-buffered.  This adds a hacky GLContext::SwapBuffers() to account for possible double-buffering.

Comment 4

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

Attachment: Update Matt's patch to work with nvidia drivers (keep glXChooseVisual)

I forgot that I'm using glXChooseVisual in bug 571832.

Comment 5

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

Argh, I meant bug 572297.

Comment 6

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

Attachment: Update Matt's patch to work with nvidia drivers (keep glXChooseVisual, fix glFinish())

Comment 7

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

Matt and I have observed differing behavior with ATI and nvidia drivers wrt double-buffering of visuals.  The three main tests have been 

 (1) Change the visuals of top-level windows to non-double-buffered ones
     - Matt's ATI card: works fine
     - My nvidia card:  works fine
 (2) Request a non-double-buffered fbconfig for a double-buffered window
     - ATI: works fine
     - nvidia: no drawn pixels ever make it to the screen
 (3) Use a double-buffered fbconfig for a double-buffered window, use glXSwapBuffers()
     - ATI: weird painting glitches, apparently timing related (vsync?)
     - nvidia: works fine

The latest plan is to use a mix of (2) and (3) for the time being: (2) for ATI cards and (3) for nvidia.  This is until we have the loose ends tied for implementing (1).

It would be good to know where intel cards fit in this spectrum.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Stick to single buffered visuals for ATI cards (workaround for weird syncing issues)

Comment 9

[Timothy B. Terriberry (:derf)]

This currently fails to work with an Intel GMA HD, a 2.6.34 kernel with the i915 driver, and X.org 1.7.6.

xGetVisualFromFBConfig returns NULL for some configs, causing a SIGSEGV. If the loop is patched to skip those configs, then the result is either a black screen (double-buffered case) or a white screen that doesn't repair its damage (after patching it to use the single-buffered ATI case).

Comment 10

[Timothy B. Terriberry (:derf)]

So, the following change makes this work:
diff -r c7eaf9387b1f gfx/layers/opengl/LayerManagerOGLProgram.h
--- a/gfx/layers/opengl/LayerManagerOGLProgram.h	Mon May 10 21:28:19 2010 -0700
+++ b/gfx/layers/opengl/LayerManagerOGLProgram.h	Sun Jun 20 22:34:16 2010 -0700
@@ -265,7 +265,7 @@
     mGL->fAttachShader(mProgram, mFragmentShader);
 
     // bind common attribs to consistent indices
-    mGL->fBindAttribLocation(mProgram, VertexAttrib, "aVertex");
+    mGL->fBindAttribLocation(mProgram, VertexAttrib, "aVertexCoord");
     mGL->fBindAttribLocation(mProgram, TexCoordAttrib, "aTexCoord");
 
     mGL->fLinkProgram(mProgram);

The reason is that check_valid_to_render() in mesa/main/api_validate.c includes the following check:
   /* For regular OpenGL, only draw if we have vertex positions (regardless
    * of whether or not we have a vertex program/shader).
    */
   if (!ctx->Array.ArrayObj->Vertex.Enabled &&
       !ctx->Array.ArrayObj->VertexAttrib[0].Enabled)
      return GL_FALSE;

However, for most programs the location for aVertexAttrib passed to BindAndDrawQuad winds up being 2. This is because LayerManagerOGLProgram::CreateProgram binds the two attributes "aVertex" and "aTexCoord", while LayerProgram::Initialize binds the single attribute "aVertexCoord". This last one appears to be what is actually used everywhere. Changing the string used in CreateProgram to also be "aVertexCoord" makes this the first attribute, and allows us to pass the Mesa check.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Updated patch to include Tim's Intel changes

Comment 12

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

Thanks Tim.  Latest patch wfm; Matt, would you like to request review of it?

Comment 13

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

Comment on attachment 452654 [details] [diff] [review]
Updated patch to include Tim's Intel changes

looks fine; I don't like the behaviour of the new SwapBuffers function though, because SwapBuffers is defined to be a no-op if the context is not double buffered.  So returning true/false is odd, but we can work that out elsewhere.

Comment 14

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

Attachment: Followup Use X11Util and the already_AddRefed type coercion operator in GLContextProviderGLX and fix a potential leak-on-error

Bug 572295 and bug 570912 just landed.

Comment 15

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

Attachment: Followup: Use X11Util and the already_AddRefed type coercion operator in GLContextProviderGLX and fix a potential leak-on-error, v2

Lost a critical null-check when rebasing, thanks for the catch Matt.  Also updated an assertion message per IRC conversation.

Comment 16

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

http://hg.mozilla.org/mozilla-central/rev/890230e9d5b9

Comment 17

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

890230e9d5b9 omitted Tim's fixes due to an accident on my part, http://hg.mozilla.org/mozilla-central/rev/ba00f1a51ca5 adds them.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 452864 [details] [diff] [review]
Followup: Use X11Util and the already_AddRefed type coercion operator in GLContextProviderGLX and fix a potential leak-on-error, v2

>+private:
>+  void Assign(T* aPtr)
>+  {
>+    if (mPtr)
>+      XFree(mPtr);
>+    mPtr = aPtr;
>+  }

Can we be sure of no self-assignment here?
Could do with an assertion, at least, I assume.

Comment 19

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

Ah, good idea, will add that.

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 452864 [details] [diff] [review]
Followup: Use X11Util and the already_AddRefed type coercion operator in GLContextProviderGLX and fix a potential leak-on-error, v2

>+        nsAutoPtr<GLContextGLX> glContext(new GLContextGLX(display, 
>+                                                           drawable, 
>+                                                           context,
>+                                                           pbuffer,
>+                                                           db));
>         if (!glContext->Init()) {
>             return nsnull;
>         }
> 
>-        return glContext;
>+        return glContext.forget();

Sorry missed that GLContextGLX is ref-counted.
nsRefPtr and returning already_AddRefed<GLContextGLX> would make more sense here, I assume.

Comment 21

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

Pushed http://hg.mozilla.org/mozilla-central/rev/33ad39273c7d with karl's fixes.