DisplayGLX: rework the GLXFBConfig selection

src/libANGLE/renderer/gl/glx/DisplayGLX.cpp

@@ -96,17 +96,18 @@ egl::Error DisplayGLX::initialize(egl::Display *display)
     }
 
     GLXFBConfig contextConfig;
-    // When glXMakeCurrent is called the visual of the context FBConfig and of
-    // the drawable must match. This means that when generating the list of EGL
-    // configs, they must all have the same visual id as our unique GL context.
-    // Here we find a GLX framebuffer config we like to create our GL context
-    // so that we are sure there is a decent config given back to the application
-    // when it queries EGL.
+    // When glXMakeCurrent is called, the context and the surface must be
+    // compatible which in glX-speak means that their config have the same
+    // color buffer type, are both RGBA or ColorIndex, and their buffers have
+    // the same depth, if they exist.
+    // Since our whole EGL implementation is backed by only one GL context, this
+    // context must be compatible with all the GLXFBConfig corresponding to the
+    // EGLconfigs that we will be exposing.
     {
         int nConfigs;
         int attribList[] =
         {
-            // We want at least RGBA8 and DEPTH24_STENCIL8
+            // We want RGBA8 and DEPTH24_STENCIL8
             GLX_RED_SIZE, 8,
             GLX_GREEN_SIZE, 8,
             GLX_BLUE_SIZE, 8,
@@ -132,7 +133,6 @@ egl::Error DisplayGLX::initialize(egl::Display *display)
         contextConfig = candidates[0];
         XFree(candidates);
     }
-    mContextVisualId = getGLXFBConfigAttrib(contextConfig, GLX_VISUAL_ID);
 
     mContext = mGLX.createContextAttribsARB(contextConfig, nullptr, True, nullptr);
     if (!mContext)
@@ -232,8 +232,16 @@ egl::ConfigSet DisplayGLX::generateConfigs() const
     // GLX_EXT_texture_from_pixmap is required for the "bind to rgb(a)" attributes
     bool hasTextureFromPixmap = mGLX.hasExtension("GLX_EXT_texture_from_pixmap");
 
+    int attribList[] =
+    {
+        GLX_RENDER_TYPE, GLX_RGBA_BIT,
+        GLX_X_RENDERABLE, True,
+        GLX_DOUBLEBUFFER, True,
+        None,
+    };
+
     int glxConfigCount;
-    GLXFBConfig *glxConfigs = mGLX.getFBConfigs(&glxConfigCount);
+    GLXFBConfig *glxConfigs = mGLX.chooseFBConfig(attribList, &glxConfigCount);
 
     for (int i = 0; i < glxConfigCount; i++)
     {
@@ -241,13 +249,7 @@ egl::ConfigSet DisplayGLX::generateConfigs() const
         egl::Config config;
 
         // Native stuff
-        int visualId = getGLXFBConfigAttrib(glxConfig, GLX_VISUAL_ID);
-        if (visualId != mContextVisualId)
-        {
-            // Filter out the configs that are incompatible with our GL context
-            continue;
-        }
-        config.nativeVisualID = visualId;
+        config.nativeVisualID = getGLXFBConfigAttrib(glxConfig, GLX_VISUAL_ID);
         config.nativeVisualType = getGLXFBConfigAttrib(glxConfig, GLX_X_VISUAL_TYPE);
         config.nativeRenderable = EGL_TRUE;
 
@@ -259,6 +261,16 @@ egl::ConfigSet DisplayGLX::generateConfigs() const
         config.depthSize = getGLXFBConfigAttrib(glxConfig, GLX_DEPTH_SIZE);
         config.stencilSize = getGLXFBConfigAttrib(glxConfig, GLX_STENCIL_SIZE);
 
+        // We require RGBA8 and the D24S8 (or no DS buffer)
+        if (config.redSize != 8 || config.greenSize != 8 || config.blueSize != 8 || config.alphaSize != 8)
+        {
+            continue;
+        }
+        if (!(config.depthSize == 24 && config.stencilSize == 8) && !(config.depthSize == 0 && config.stencilSize == 0))
+        {
+            continue;
+        }
+
         config.colorBufferType = EGL_RGB_BUFFER;
         config.luminanceSize = 0;
         config.alphaMaskSize = 0;

src/libANGLE/renderer/gl/glx/DisplayGLX.h

@@ -66,8 +66,6 @@ class DisplayGLX : public DisplayGL
     //TODO(cwallez) yuck, change generateConfigs to be non-const or add a userdata member to egl::Config?
     mutable std::map<int, GLXFBConfig> configIdToGLXConfig;
 
-    // The ID of the visual used to create the context
-    int mContextVisualId;
     GLXContext mContext;
     // A pbuffer the context is current on during ANGLE initialization
     GLXPbuffer mDummyPbuffer;

src/libANGLE/renderer/gl/glx/FBConfigCompatibility.md

+GLX Framebuffer Compatibility investigation
+===========================================
+
+In GLX and EGL, contexts are created with respect to a config that
+describes the type of surfaces they will be used to render to.
+Likewise surfaces are created with respect to a config and for
+a context to be able to render to a surface, both their configs
+must be compatible. Compatibility is losely described in both
+the GLX and EGL specs but the following is clear:
+ * In GLX the config's color buffer must have the same type, including
+RGBA vs. ColorIndex and the buffers must have the same depth, if they
+exist.
+ * In EGL the config's color buffer must have the same type and the
+buffers must have the same depth (not clear if it is only if they exist)
+
+Obviously the EGLconfig we will expose will have a one-to-one
+correspondance with GLXFBConfigs.
+
+Our EGL implementation uses a single OpenGL context to back all
+the EGLcontexts created by the application. Since our GL context
+and GLXContext are the same object but in two APIs, we will make
+the confusion and call the GLX context our backing context.
+
+The problem we have is that the the GLX context is created before
+the application can choose what type of context it wants to use,
+that means we have to expose EGLconfigs whose respective GLXFBConfigs
+are compatible with the GLXFBConfig of our GLX context; we also need
+to choose the GLXFBConfig of our GLX context so that it matches the
+most common needs of application.
+
+Choice of the GLX context GLXFBConfig
+-------------------------------------
+
+We decided that our GLX context's configuration must satisfy the following:
+ * Have a RGBA8 color buffer and D24S8 depth-stencil buffer which is what
+the vast majority of applications use.
+ * It must render in direct colors, i.e. not in a color indexed format.
+ * It must be double-buffered (see later)
+ * It must support rendering to all the types of GLX surfaces so that we can
+use it for all types of EGL surfaces
+ * It must have an associated visual ID so that we can use it with X, it seems
+like this would be strongly tied to it having the ```WINDOW_BIT``` set.
+ * We would like a conformat context.
+
+Study of compatible GLXFBConfigs
+--------------------------------
+
+When using the condition of compatibility defined in the GLX spec and filtering
+out the non-conformant GLXFBConfig we got the following list (see function
+```print_visual_attribs_short``` in [glxinfo's source code](http://cgit.freedesktop.org/mesa/demos/tree/src/xdemos/glxinfo.c)
+to understand how to read the table):
+
+```
+    visual  x   bf lv rg d st  colorbuffer  sr ax dp st accumbuffer  ms  cav
+  id dep cl sp  sz l  ci b ro  r  g  b  a F gb bf th cl  r  g  b  a ns b eat  Result
+----------------------------------------------------------------------------
+0x02e 24 tc  0  32  0 r  . .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None Fail
+0x0e4 32 tc  0  32  0 r  . .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None BadMatch
+0x02c 24 tc  0  32  0 r  y .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None Pass
+0x0e2 32 tc  0  32  0 r  y .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None BadMatch
+0x089 24 dc  0  32  0 r  . .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None Fail
+0x087 24 dc  0  32  0 r  y .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None Pass
+0x026 24 tc  0  32  0 r  . .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None Fail
+0x0dc 32 tc  0  32  0 r  . .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None BadMatch
+0x024 24 tc  0  32  0 r  y .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None Pass
+0x0da 32 tc  0  32  0 r  y .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None BadMatch
+0x081 24 dc  0  32  0 r  . .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None Fail
+0x07f 24 dc  0  32  0 r  y .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None Pass
+```
+
+The last column shows the result of trying to render on a window using the config,
+with a GLX context using config 0x024. The first thing we see is that BadMatch is
+thrown by the X server when creating the subwindow for rendering. This was because
+we didn't set the border pixel of the subwindow *shake fist at X11* (see this [StackOverflow question](http://stackoverflow.com/questions/3645632/how-to-create-a-window-with-a-bit-depth-of-32)).
+The result updated with this fix give:
+
+```
+    visual  x   bf lv rg d st  colorbuffer  sr ax dp st accumbuffer  ms  cav
+  id dep cl sp  sz l  ci b ro  r  g  b  a F gb bf th cl  r  g  b  a ns b eat
+----------------------------------------------------------------------------
+0x02e 24 tc  0  32  0 r  . .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None Fail
+0x0e4 32 tc  0  32  0 r  . .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None Fail
+0x02c 24 tc  0  32  0 r  y .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None Pass
+0x0e2 32 tc  0  32  0 r  y .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None Pass
+0x089 24 dc  0  32  0 r  . .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None Fail
+0x087 24 dc  0  32  0 r  y .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None Pass
+0x026 24 tc  0  32  0 r  . .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None Fail
+0x0dc 32 tc  0  32  0 r  . .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None Fail
+0x024 24 tc  0  32  0 r  y .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None Pass
+0x0da 32 tc  0  32  0 r  y .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None Pass
+0x081 24 dc  0  32  0 r  . .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None Fail
+0x07f 24 dc  0  32  0 r  y .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None Pass
+```
+
+From this we see that our rendering test passed if and only if the config was double
+buffered like 0x024 which is our GLX context config. The compatible configs are then:
+
+```
+    visual  x   bf lv rg d st  colorbuffer  sr ax dp st accumbuffer  ms  cav
+  id dep cl sp  sz l  ci b ro  r  g  b  a F gb bf th cl  r  g  b  a ns b eat
+----------------------------------------------------------------------------
+0x02c 24 tc  0  32  0 r  y .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None
+0x0e2 32 tc  0  32  0 r  y .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None
+0x087 24 dc  0  32  0 r  y .   8  8  8  8 .  s  4  0  0 16 16 16 16  0 0 None
+0x024 24 tc  0  32  0 r  y .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None
+0x0da 32 tc  0  32  0 r  y .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None
+0x07f 24 dc  0  32  0 r  y .   8  8  8  8 .  s  4 24  8 16 16 16 16  0 0 None
+```
+
+We can see two dimensions, with our without a depth-stencil buffer and with TrueColor
+or DirectColor. The depth-stencil will be useful to expose to application.
+
+Future investigation
+--------------------
+At that point we don't know if there is a special trick that could allow us to use
+the non-doublebuffered configs but it would be nice to be able to use them so that
+we can expose the non-double buffered ones in an extension.
+
+All the non-conformant configs have a multisampled buffer, so it could be interesting
+to see if we can use them to expose another EGL extension.
+
+Finally all this document is written with respect to the proprietary NVIDIA driver,
+before using the GLX EGL implementation in the wild it would be good to test it on
+other drivers and hardware.