Closed Bug 572939 Opened 11 years ago Closed 11 years ago

Add CreateForWindow support to GLContextProviderGLX


(Core :: Graphics, enhancement)

Not set





(Reporter: mattwoodrow, Assigned: mattwoodrow)




(3 files, 5 obsolete files)

12.60 KB, patch
Details | Diff | Splinter Review
15.78 KB, patch
: review+
Details | Diff | Splinter Review
9.73 KB, patch
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/20100423 Ubuntu/10.04 (lucid) Firefox/3.6.3
Build Identifier: 

Implement the CreateForWindow function.

Reproducible: Always
Blocks: 565833
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.
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.
I forgot that I'm using glXChooseVisual in bug 571832.
Attachment #452202 - Attachment is obsolete: true
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.
This currently fails to work with an Intel GMA HD, a 2.6.34 kernel with the i915 driver, and 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).
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");

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 &&
      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.
Attachment #452409 - Attachment is obsolete: true
Thanks Tim.  Latest patch wfm; Matt, would you like to request review of it?
Attachment #452654 - Flags: review?(vladimir)
Ever confirmed: true
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.
Attachment #452654 - Flags: review?(vladimir) → review+
Lost a critical null-check when rebasing, thanks for the catch Matt.  Also updated an assertion message per IRC conversation.
Attachment #452842 - Attachment is obsolete: true
Attachment #452864 - Flags: review?(karlt)
Attachment #452842 - Flags: review?(karlt)
Closed: 11 years ago
Resolution: --- → FIXED
890230e9d5b9 omitted Tim's fixes due to an accident on my part, adds them.
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

>+  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.
Attachment #452864 - Flags: review?(karlt) → review+
Ah, good idea, will add that.
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.
Attachment #452864 - Flags: review+ → review?(karlt)
Attachment #452864 - Flags: review?(karlt) → review+
Assignee: nobody → matt.woodrow+bugzilla
See Also: → 1478454
You need to log in before you can comment on or make changes to this bug.