Matt Woodrow (:mattwoodrow) Assignee
	Description • 15 years ago

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

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 2 • 15 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 3 • 14 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 4 • 14 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 5 • 14 years ago

Argh, I meant bug 572297.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 7 • 14 years ago

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.

	Timothy B. Terriberry (:derf)
	Comment 9 • 14 years ago

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).

	Timothy B. Terriberry (:derf)
	Comment 10 • 14 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 12 • 14 years ago

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

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 13 • 14 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 14 • 14 years ago

Bug 572295 and bug 570912 just landed.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 15 • 14 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 16 • 14 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 17 • 14 years ago

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

	Karl Tomlinson (:karlt)
	Comment 18 • 14 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 19 • 14 years ago

Ah, good idea, will add that.

	Karl Tomlinson (:karlt)
	Comment 20 • 14 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 21 • 14 years ago

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