Compiz FBConfig selection criteria

[Ben Gamari]

Hey All,

Recently I (perhaps stupidly) decided to try using git master of
xf86-video-intel, drm, xorg-server, and mesa. Unfortunately, something
in this update broke my fbconfigs badly, causing compiz to give me the
error:

        ben at mercury ~ $ LIBGL_ALWAYS_INDIRECT=1 compiz --replace ccp
        compiz (core) - Fatal: No GLXFBConfig for default depth, this isn't going to work.
        compiz (core) - Error: Failed to manage screen: 0
        compiz (core) - Fatal: No manageable screens found on display :0.0

To make matters worse, I've been unable to reproduce the original
working conditions. In trying to debug the issue, I've been looking at
compiz's source, attempting to figure out how it chooses fbconfigs.

First, in poking around in compiz, it seems like there is quite a
sophisticated criteria for FBConfig selection (starting at
src/screen.c:2015). While I'm by no means well-versed in FBConfigs,
AIGLX, or the GL stack as a whole, I have taken a stab at identifying
the various checks that are executed for each FBConfig which I have
included below.

Despite my efforts, there are a few facets I have been unable to figure
out. First, what does the check ((value - alpha) == i) accomplish? It
seems to me that the only way that (value == i) and ((value - alpha) ==
i) could be simultaneously true is if alpha == 0. Is this true? Is
compiz looking for a FBConfig with no alpha channel? Perhaps the source
of my difficulty here is just my general lack of understanding of
FBConfigs.

Secondly, it appears to me that the GLX_BIND_TO_MIPMAP_TEXTURE_EXT and
GLX_DOUBLEBUFFER attributes are booleans. This, however, is inconsistent
with the code, which both declares and compares their respective
tracking variables (db and mipmap respectively) as ints.

Below I have attached a patch (not intended for merging) which
represents my present understanding of the FBConfig selection code by
commenting and restructuring some of it to be a little clearer. Comments
on its accuracy would be greatly appreciated.

Thank you very much for your time,

- Ben


diff --git a/src/screen.c b/src/screen.c
index e3543f3..e09c583 100644
--- a/src/screen.c
+++ b/src/screen.c
@@ -2011,6 +2011,7 @@ addScreen (CompDisplay *display,
 				    screenNum,
 				    &nElements);
 
+    // Iterate over all depths
     for (i = 0; i <= MAX_DEPTH; i++)
     {
 	int j, db, stencil, depth, alpha, mipmap, rgba;
@@ -2026,12 +2027,14 @@ addScreen (CompDisplay *display,
 	depth   = MAXSHORT;
 	mipmap  = 0;
 	rgba    = 0;
-
+        
+        // For each depth, iterate over all FBConfigs
 	for (j = 0; j < nElements; j++)
 	{
 	    XVisualInfo *vi;
 	    int		visualDepth;
 
+            // We can't use an FBConfig unless it has an associated visual
 	    vi = glXGetVisualFromFBConfig (dpy, fbConfigs[j]);
 	    if (vi == NULL)
 		continue;
@@ -2039,7 +2042,8 @@ addScreen (CompDisplay *display,
 	    visualDepth = vi->depth;
 
 	    XFree (vi);
-
+            
+            // We're only looking at FBConfigs of depth i
 	    if (visualDepth != i)
 		continue;
 
@@ -2051,9 +2055,14 @@ addScreen (CompDisplay *display,
 				     fbConfigs[j],
 				     GLX_BUFFER_SIZE,
 				     &value);
-	    if (value != i && (value - alpha) != i)
+            // Check that the buffer size matches our desired depth
+	    if (value != i)
+	        continue;
+	    // Check that ???
+	    if ((value - alpha) != i)
 		continue;
 
+            // Check if we can bind to RGBA textures
 	    value = 0;
 	    if (i == 32)
 	    {
@@ -2061,41 +2070,44 @@ addScreen (CompDisplay *display,
 					 fbConfigs[j],
 					 GLX_BIND_TO_TEXTURE_RGBA_EXT,
 					 &value);
-
-		if (value)
-		{
-		    rgba = 1;
-
-		    s->glxPixmapFBConfigs[i].textureFormat =
-			GLX_TEXTURE_FORMAT_RGBA_EXT;
-		}
+            }
+            
+            if (value)
+	    {
+                // We have RGBA binding capability with this FBConfig
+	        rgba = 1;
+	        s->glxPixmapFBConfigs[i].textureFormat =  GLX_TEXTURE_FORMAT_RGBA_EXT;
 	    }
-
-	    if (!value)
+	    else
 	    {
+	        // We don't have RGBA binding capability on this FBConfig
+	        // but we've already found a config with an alpha channel, ignore
 		if (rgba)
 		    continue;
 
+                // If we don't have an alpha channel, make sure we have BIND_TO_TEXTURE_RGB_EXT
 		(*s->getFBConfigAttrib) (dpy,
 					 fbConfigs[j],
 					 GLX_BIND_TO_TEXTURE_RGB_EXT,
 					 &value);
-		if (!value)
+		if (value != GL_TRUE)
 		    continue;
 
 		s->glxPixmapFBConfigs[i].textureFormat =
 		    GLX_TEXTURE_FORMAT_RGB_EXT;
 	    }
-
+            
+            // Only look at FBConfigs with double-buffers
 	    (*s->getFBConfigAttrib) (dpy,
 				     fbConfigs[j],
 				     GLX_DOUBLEBUFFER,
 				     &value);
-	    if (value > db)
+	    if (value != GL_TRUE)
 		continue;
 
 	    db = value;
-
+            
+            // Look for smallest stencil buffer
 	    (*s->getFBConfigAttrib) (dpy,
 				     fbConfigs[j],
 				     GLX_STENCIL_SIZE,
@@ -2105,6 +2117,7 @@ addScreen (CompDisplay *display,
 
 	    stencil = value;
 
+            // Look for smallest depth buffer
 	    (*s->getFBConfigAttrib) (dpy,
 				     fbConfigs[j],
 				     GLX_DEPTH_SIZE,
@@ -2116,11 +2129,12 @@ addScreen (CompDisplay *display,
 
 	    if (s->fbo)
 	    {
+	        // Look for mipmapped texture support
 		(*s->getFBConfigAttrib) (dpy,
 					 fbConfigs[j],
 					 GLX_BIND_TO_MIPMAP_TEXTURE_EXT,
 					 &value);
-		if (value < mipmap)
+		if (value != GL_TRUE)
 		    continue;
 
 		mipmap = value;