Check more GLES versions when creating context

tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp

Index: tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp
diff --git a/tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp b/tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp
index a3069dc0285f4ea377857ef55f56fb4d8ef28aa6..9eb3d88b99840f26de87ffe9839d6325cce2c98d 100644
--- a/tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp
+++ b/tools/gpu/gl/glx/CreatePlatformGLTestContext_glx.cpp
@@ -1,4 +1,3 @@
-
 /*
  * Copyright 2011 Google Inc.
  *
@@ -11,12 +10,15 @@
 #include <GL/glx.h>
 #include <GL/glu.h>
 
+#include <vector>
+#include <utility>
+
 namespace {
 
 /* Note: Skia requires glx 1.3 or newer */
 
 /* This struct is taken from a mesa demo.  Please update as required */
-static const struct { int major, minor; } gl_versions[] = {
+static const std::vector<std::pair<int, int>> gl_versions = {
    {1, 0},
    {1, 1},
    {1, 2},
@@ -34,9 +36,12 @@ static const struct { int major, minor; } gl_versions[] = {
    {4, 2},
    {4, 3},
    {4, 4},
-   {0, 0} /* end of list */
 };
-#define NUM_GL_VERSIONS SK_ARRAY_COUNT(gl_versions)
+
+static const std::vector<std::pair<int, int>> gles_versions = {
+    {2, 0},
+    {3, 0},
+};
 
 static bool ctxErrorOccurred = false;
 static int ctxErrorHandler(Display *dpy, XErrorEvent *ev) {
@@ -51,6 +56,8 @@ public:
 
 private:
     void destroyGLContext();

[robertphillips, 2016/08/03 17:31:55]

is_es should be isES
the "f" prefix is reserved for member variables so "fDisplay" should just be
"display"

[mkollaro, 2016/08/04 13:26:24]

Done.

+    GLXContext createBestContext(bool is_es, Display* fDisplay, GLXFBConfig bestFbc,

[robertphillips, 2016/08/03 17:31:55]

Line this last parameter up with the opening '(' ?

[mkollaro, 2016/08/04 13:26:24]

Done.

+            GLXContext glxSharedContext);
 
     void onPlatformMakeCurrent() const override;
     void onPlatformSwapBuffers() const override;
@@ -149,25 +156,10 @@ GLXGLTestContext::GLXGLTestContext(GrGLStandard forcedGpuAPI, GLXGLTestContext*
     // Done with the visual info data
     XFree(vi);
 
-    // Create the context
-
-    // Install an X error handler so the application won't exit if GL 3.0
-    // context allocation fails.
-    //
-    // Note this error handler is global.
-    // All display connections in all threads of a process use the same
-    // error handler, so be sure to guard against other threads issuing
-    // X commands while this code is running.
-    ctxErrorOccurred = false;
-    int (*oldHandler)(Display*, XErrorEvent*) =
-        XSetErrorHandler(&ctxErrorHandler);
-
     // Get the default screen's GLX extension list
     const char *glxExts = glXQueryExtensionsString(
         fDisplay, DefaultScreen(fDisplay)
     );
-
-
     // Check for the GLX_ARB_create_context extension string and the function.
     // If either is not present, use GLX 1.3 context creation method.
     if (!gluCheckExtension(reinterpret_cast<const GLubyte*>("GLX_ARB_create_context"),
@@ -176,85 +168,19 @@ GLXGLTestContext::GLXGLTestContext(GrGLStandard forcedGpuAPI, GLXGLTestContext*
             fContext = glXCreateNewContext(fDisplay, bestFbc, GLX_RGBA_TYPE, 0, True);
         }
     } else {
-        //SkDebugf("Creating context.\n");
-        PFNGLXCREATECONTEXTATTRIBSARBPROC glXCreateContextAttribsARB =
-            (PFNGLXCREATECONTEXTATTRIBSARBPROC) glXGetProcAddressARB((GrGLubyte*)"glXCreateContextAttribsARB");
-
         if (kGLES_GrGLStandard == forcedGpuAPI) {
             if (gluCheckExtension(
                     reinterpret_cast<const GLubyte*>("GLX_EXT_create_context_es2_profile"),
                     reinterpret_cast<const GLubyte*>(glxExts))) {

[robertphillips, 2016/08/03 17:31:55]

line the last parameter up with the opening '(' ?
Can it all fit on one line ?
Member function calls are prefixed with "this->"

[mkollaro, 2016/08/04 13:26:24]

Done.

-                const int context_attribs_gles[] = {
-                    GLX_CONTEXT_MAJOR_VERSION_ARB, 2,
-                    GLX_CONTEXT_MINOR_VERSION_ARB, 0,
-                    GLX_CONTEXT_PROFILE_MASK_ARB, GLX_CONTEXT_ES2_PROFILE_BIT_EXT,
-                    None
-                };
-                fContext = glXCreateContextAttribsARB(fDisplay, bestFbc, glxShareContext, True,
-                                                      context_attribs_gles);
+                fContext = createBestContext(true, fDisplay, bestFbc,
+                        glxShareContext);
             }
         } else {

[robertphillips, 2016/08/03 17:31:56]

Can this fit on one line?

[mkollaro, 2016/08/04 13:26:25]

Done.

-            // Well, unfortunately GLX will not just give us the highest context so instead we have
-            // to do this nastiness
-            for (i = NUM_GL_VERSIONS - 2; i > 0 ; i--) {
-                /* don't bother below GL 3.0 */
-                if (gl_versions[i].major < 3) {
-                    break;
-                }
-                // On Nvidia GPUs, to use Nv Path rendering we need a compatibility profile for the
-                // time being.
-                // TODO when Nvidia implements NVPR on Core profiles, we should start requesting
-                // core here
-                // Warning: This array should not be set to static. The
-                // glXCreateContextAttribsARB call writes to it upon failure and
-                // the next call would fail too.
-                const int context_attribs_gl[] = {
-                      GLX_CONTEXT_MAJOR_VERSION_ARB, gl_versions[i].major,
-                      GLX_CONTEXT_MINOR_VERSION_ARB, gl_versions[i].minor,
-                      GLX_CONTEXT_PROFILE_MASK_ARB, GLX_CONTEXT_COMPATIBILITY_PROFILE_BIT_ARB,
-                      None
-                };
-                fContext =
-                        glXCreateContextAttribsARB(fDisplay, bestFbc, glxShareContext, True,
-                                                   context_attribs_gl);
-
-                // Sync to ensure any errors generated are processed.
-                XSync(fDisplay, False);
-
-                if (!ctxErrorOccurred && fContext) {
-                    break;
-                }
-                // try again
-                ctxErrorOccurred = false;
-            }
-
-            // Couldn't create GL 3.0 context.
-            // Fall back to old-style 2.x context.
-            // When a context version below 3.0 is requested,
-            // implementations will return the newest context version
-            // compatible with OpenGL versions less than version 3.0.
-            if (ctxErrorOccurred || !fContext) {
-                const int context_attribs_gl_fallback[] = {
-                    GLX_CONTEXT_MAJOR_VERSION_ARB, 1,
-                    GLX_CONTEXT_MINOR_VERSION_ARB, 0,
-                    None
-                };
-
-                ctxErrorOccurred = false;
-
-                fContext = glXCreateContextAttribsARB(fDisplay, bestFbc, glxShareContext, True,
-                                                      context_attribs_gl_fallback);
-            }
+            fContext = createBestContext(false, fDisplay, bestFbc,
+                    glxShareContext);
         }
     }
-
-    // Sync to ensure any errors generated are processed.
-    XSync(fDisplay, False);
-
-    // Restore the original error handler
-    XSetErrorHandler(oldHandler);
-
-    if (ctxErrorOccurred || !fContext) {
+    if (!fContext) {
         SkDebugf("Failed to create an OpenGL context.\n");
         this->destroyGLContext();
         return;
@@ -320,6 +246,72 @@ void GLXGLTestContext::destroyGLContext() {
     }
 }
 
+/* Create a context with the highest possible version.
+ *
+ * Disable Xlib errors for the duration of this function (by default they abort
+ * the program) and try to get a context starting from the highest version
+ * number - there is no way to just directly ask what the highest supported
+ * version is.
+ *
+ * Return nullptr upon failure.
+ */

[robertphillips, 2016/08/03 17:31:55]

is_es & fDisplay again

Can we make this a static method? It is odd to be assigning to the fContext
member variable inside this method and then return it so that the caller can
assigned it to fContext.

[mkollaro, 2016/08/04 13:26:25]

I've made it a normal member and directly set fContext. Is that ok or would a
static member be preferable?

[robertphillips, 2016/08/04 19:42:13]

I think having it be a static method would be clearer b.c. it would centralize
the setting of fContext to the GLXGLTestContext ctor.

+GLXContext GLXGLTestContext::createBestContext(bool is_es, Display* fDisplay,
+        GLXFBConfig bestFbc, GLXContext glxShareContext) {
+    auto glXCreateContextAttribsARB = (PFNGLXCREATECONTEXTATTRIBSARBPROC)
+        glXGetProcAddressARB((GrGLubyte*)"glXCreateContextAttribsARB");

[robertphillips, 2016/08/03 17:31:56]

either:

nullptr == glXCreateContextAttribsARB

or

!glXCreateContexAttribsARB

[mkollaro, 2016/08/04 13:26:24]

Done.

+    if (glXCreateContextAttribsARB == nullptr) {
+        SkDebugf("Failed to get address of glXCreateContextAttribsARB");
+        return nullptr;
+    }
+    // Install Xlib error handler that will set ctxErrorOccurred.
+    // WARNING: It is global for all threads.
+    ctxErrorOccurred = false;

[robertphillips, 2016/08/03 17:31:56]

Can this fit on one line ?

[mkollaro, 2016/08/04 13:26:25]

Done.

+    int (*oldHandler)(Display*, XErrorEvent*) =
+        XSetErrorHandler(&ctxErrorHandler);
+

[robertphillips, 2016/08/03 17:31:56]

The 'f' prefix is reserved for member variables. This should be "context"

[mkollaro, 2016/08/04 13:26:24]

Done.

+    GLXContext fContext = nullptr;
+    auto versions = is_es ? gles_versions : gl_versions;
+    // Well, unfortunately GLX will not just give us the highest context so
+    // instead we have to do this nastiness
+    for (int i = versions.size() - 1; i >= 0 ; i--) {

[robertphillips, 2016/08/03 17:31:55]

I believe the nVidia specific comment should be moved down to where we add the
COMPATIBILITY_PROFILE_BIT

[mkollaro, 2016/08/04 13:26:25]

Done.

+        // TODO When Nvidia implements NVPR on Core profiles, we should start
+        // requesting core here - currently Nv Path rendering on Nvidia
+        // requires a compatibility profile.
+        // WARNING: Don't try to optimize this and make this array static. The
+        // glXCreateContextAttribsARB call writes to it upon failure and the
+        // next call would fail too.
+        std::vector<int> flags = {
+            GLX_CONTEXT_MAJOR_VERSION_ARB, versions[i].first,
+            GLX_CONTEXT_MINOR_VERSION_ARB, versions[i].second,
+        };
+        if (is_es) {
+            flags.push_back(GLX_CONTEXT_PROFILE_MASK_ARB);
+            // the ES2 flag should work even for higher versions
+            flags.push_back(GLX_CONTEXT_ES2_PROFILE_BIT_EXT);
+        } else if (versions[i].first > 2) {
+            flags.push_back(GLX_CONTEXT_PROFILE_MASK_ARB);
+            flags.push_back(GLX_CONTEXT_COMPATIBILITY_PROFILE_BIT_ARB);
+        }
+        flags.push_back(0);
+        fContext = glXCreateContextAttribsARB(fDisplay, bestFbc,
+                                              glxShareContext, true, &flags[0]);
+        // Sync to ensure any errors generated are processed.
+        XSync(fDisplay, False);
+
+        if (!ctxErrorOccurred && fContext) {

[robertphillips, 2016/08/03 17:31:55]

Remove this commented out code

[mkollaro, 2016/08/04 13:26:25]

Done.

+            //SkDebugf("Created %d.%d context\n", versions[i].first,
+                    //versions[i].second);
+            break;
+        }
+        // try again
+        ctxErrorOccurred = false;
+    }
+    // Restore the original error handler.
+    XSetErrorHandler(oldHandler);
+
+    return fContext;
+}
+
 void GLXGLTestContext::onPlatformMakeCurrent() const {
     if (!glXMakeCurrent(fDisplay, fGlxPixmap, fContext)) {
         SkDebugf("Could not set the context.\n");