Fix EGL bugs (#5730)

* Fix two EGL bugs

* Ensure float precision is always given for fragment shaders.

* Try multiple displays to find appropriate config.

* Use only two return values for halide_opengl_create_context

* Detect GL_EXT_color_buffer_float

* Improve color-renderable error reporting for OpenGL ES < 3.2

* Use KHR_debug (when available) in the debug runtime

* Add skips for RGB tests when EGL enabled.

apps/glsl/opengl_test.cpp

@@ -55,4 +55,9 @@ int main(int argc, char *argv[]) {
     test_blur();
     test_ycc();
     test_device_sync();
+
+    // This is supposed to be called as an __attribute__((destructor)), but
+    // some EGL implementations can start to unload their libraries before those
+    // destructors run. On such systems, doing this outside of main() segfaults.
+    halide_device_release(nullptr, halide_opengl_device_interface());
 }

src/CodeGen_OpenGL_Dev.cpp

@@ -1045,6 +1045,8 @@ void CodeGen_GLSL::add_kernel(const Stmt &stmt, const string &name,
     if (is_opengl_es(target)) {
         stream << "#ifdef GL_FRAGMENT_PRECISION_HIGH\n"
                << "precision highp float;\n"
+               << "#else\n"
+               << "precision mediump float;\n"
                << "#endif\n";
     }
 

src/Target.cpp

@@ -994,9 +994,10 @@ bool Target::get_runtime_compatible_target(const Target &other, Target &result)
     // clang-format on
 
     // clang-format off
-    const std::array<Feature, 12> matching_features = {{
+    const std::array<Feature, 10> matching_features = {{
         ASAN,
         Debug,
+        EGL,
         HexagonDma,
         HVX,
         HVX_shared_object,

src/runtime/mini_opengl.h

@@ -218,4 +218,25 @@ typedef void (*PFNGLDELETEBUFFERSPROC)(GLsizei n, const GLuint *buffers);
 typedef void (*PFNGLGETACTIVEUNIFORM)(GLuint program, GLuint index, GLsizei bufSize, GLsizei *length, GLint *size, GLenum *type, GLchar *name);
 typedef GLint (*PFNGLGETUNIFORMLOCATION)(GLuint program, const GLchar *name);
 
+// ---------- KHR_debug macros ----------
+
+// clang-format off
+#define GL_DEBUG_OUTPUT                   0x92E0
+#define GL_DEBUG_OUTPUT_SYNCHRONOUS       0x8242
+#define GL_DEBUG_SEVERITY_HIGH            0x9146
+#define GL_DEBUG_SEVERITY_MEDIUM          0x9147
+#define GL_DEBUG_SEVERITY_LOW             0x9148
+#define GL_DEBUG_SEVERITY_NOTIFICATION    0x826B
+#define GL_DEBUG_TYPE_DEPRECATED_BEHAVIOR 0x824D
+#define GL_DEBUG_TYPE_ERROR               0x824C
+#define GL_DEBUG_TYPE_MARKER              0x8268
+#define GL_DEBUG_TYPE_OTHER               0x8251
+#define GL_DEBUG_TYPE_PERFORMANCE         0x8250
+#define GL_DEBUG_TYPE_PORTABILITY         0x824F
+#define GL_DEBUG_TYPE_UNDEFINED_BEHAVIOR  0x824E
+// clang-format on
+
+typedef void (*DEBUGPROC)(GLenum source, GLenum type, GLuint id, GLenum severity, GLsizei length, const GLchar *message, void *userParam);
+typedef void (*PFNGLDEBUGMESSAGECALLBACK)(DEBUGPROC callback, void *userParam);
+
 #endif  // MINI_OPENGL_H

src/runtime/opengl.cpp

@@ -73,11 +73,12 @@
 // List of all OpenGL functions used by the runtime, which may not
 // exist due to an older or less capable version of GL. In using any
 // of these functions, code must test if they are nullptr.
-#define OPTIONAL_GL_FUNCTIONS                            \
-    GLFUNC(PFNGLGENVERTEXARRAYS, GenVertexArrays);       \
-    GLFUNC(PFNGLBINDVERTEXARRAY, BindVertexArray);       \
-    GLFUNC(PFNGLDELETEVERTEXARRAYS, DeleteVertexArrays); \
-    GLFUNC(PFNDRAWBUFFERS, DrawBuffers)
+#define OPTIONAL_GL_FUNCTIONS                                \
+    GLFUNC(PFNGLDEBUGMESSAGECALLBACK, DebugMessageCallback); \
+    GLFUNC(PFNGLGENVERTEXARRAYS, GenVertexArrays);           \
+    GLFUNC(PFNGLBINDVERTEXARRAY, BindVertexArray);           \
+    GLFUNC(PFNGLDELETEVERTEXARRAYS, DeleteVertexArrays);     \
+    GLFUNC(PFNDRAWBUFFERS, DrawBuffers);
 
 // ---------- Types ----------
 
@@ -127,6 +128,69 @@ WEAK const char *gl_error_name(int32_t err) {
     return result;
 }
 
+#ifdef DEBUG_RUNTIME
+static void halide_gl_debug_callback(GLenum source,
+                                     GLenum type,
+                                     GLuint id,
+                                     GLenum severity,
+                                     GLsizei length,
+                                     const GLchar *message,
+                                     void *userParam) {
+    const char *type_str;
+    switch (type) {
+    case GL_DEBUG_TYPE_ERROR:
+        type_str = "Error ";
+        break;
+    case GL_DEBUG_TYPE_DEPRECATED_BEHAVIOR:
+        type_str = "Deprecated Behaviour ";
+        break;
+    case GL_DEBUG_TYPE_UNDEFINED_BEHAVIOR:
+        type_str = "Undefined Behaviour ";
+        break;
+    case GL_DEBUG_TYPE_PORTABILITY:
+        type_str = "Portability Issue ";
+        break;
+    case GL_DEBUG_TYPE_PERFORMANCE:
+        type_str = "Performance Issue ";
+        break;
+    case GL_DEBUG_TYPE_OTHER:
+        type_str = "Misc Message ";
+        break;
+    case GL_DEBUG_TYPE_MARKER:
+        type_str = "Marker Hit ";
+        break;
+    default:
+        type_str = "Unknown Message Type ";
+        break;
+    }
+
+    const char *severity_str;
+    switch (severity) {
+    case GL_DEBUG_SEVERITY_NOTIFICATION:
+        severity_str = "(Notification): ";
+        break;
+
+    case GL_DEBUG_SEVERITY_LOW:
+        severity_str = "(Low Severity): ";
+        break;
+
+    case GL_DEBUG_SEVERITY_MEDIUM:
+        severity_str = "(Medium Severity): ";
+        break;
+
+    case GL_DEBUG_SEVERITY_HIGH:
+        severity_str = "(High Severity): ";
+        break;
+
+    default:
+        severity_str = "(Unknown Severity): ";
+        break;
+    }
+
+    debug(userParam) << type_str << severity_str << message << "\n";
+}
+#endif
+
 struct HalideMalloc {
     ALWAYS_INLINE HalideMalloc(void *user_context, size_t size)
         : user_context(user_context), ptr(halide_malloc(user_context, size)) {
@@ -198,6 +262,7 @@ struct GlobalState {
     bool have_vertex_array_objects;
     bool have_texture_rg;
     bool have_texture_float;
+    bool have_color_buffer_float;
     bool have_texture_rgb8_rgba8;
 
     // Various objects shared by all filter kernels
@@ -621,6 +686,14 @@ WEAK void init_extensions(void *user_context) {
     }
     load_gl_func(user_context, "glDrawBuffers", (void **)&global_state.DrawBuffers, false);
 
+#ifdef DEBUG_RUNTIME
+    load_gl_func(user_context, "glDebugMessageCallback", (void **)&global_state.DebugMessageCallback, false);
+    if (global_state.DebugMessageCallback) {
+        global_state.Enable(GL_DEBUG_OUTPUT_SYNCHRONOUS);
+        global_state.DebugMessageCallback(halide_gl_debug_callback, user_context);
+    }
+#endif
+
     global_state.have_texture_rg =
         global_state.major_version >= 3 ||
         (global_state.profile == OpenGL &&
@@ -639,6 +712,11 @@ WEAK void init_extensions(void *user_context) {
          extension_supported(user_context, "GL_ARB_texture_float")) ||
         (global_state.profile == OpenGLES &&
          extension_supported(user_context, "GL_OES_texture_float"));
+
+    global_state.have_color_buffer_float =
+        global_state.profile == OpenGL ||
+        extension_supported(user_context, "GL_EXT_color_buffer_float") ||
+        extension_supported(user_context, "GL_ARB_color_buffer_float");
 }
 
 WEAK const char *parse_int(const char *str, int *val) {
@@ -711,7 +789,8 @@ WEAK int halide_opengl_init(void *user_context) {
         << "  vertex_array_objects: " << (global_state.have_vertex_array_objects ? "yes\n" : "no\n")
         << "  texture_rg: " << (global_state.have_texture_rg ? "yes\n" : "no\n")
         << "  have_texture_rgb8_rgba8: " << (global_state.have_texture_rgb8_rgba8 ? "yes\n" : "no\n")
-        << "  texture_float: " << (global_state.have_texture_float ? "yes\n" : "no\n");
+        << "  texture_float: " << (global_state.have_texture_float ? "yes\n" : "no\n")
+        << "  color_buffer_float: " << (global_state.have_color_buffer_float ? "yes\n" : "no\n");
 
     // Initialize framebuffer.
     global_state.GenFramebuffers(1, &global_state.framebuffer_id);
@@ -825,32 +904,37 @@ WEAK bool get_texture_format(void *user_context, halide_buffer_t *buf,
         return false;
     }
 
-    switch (global_state.profile) {
-    case OpenGLES:
-        // For OpenGL ES, the texture format has to match the pixel format
-        // since there no conversion is performed during texture transfers.
-        // See OES_texture_float.
-        *internal_format = *format;
-        break;
-    case OpenGL:
-        // For desktop OpenGL, the internal format specifiers include the
-        // precise data type, see ARB_texture_float.
-        if (*type == GL_FLOAT) {
+    if (*type == GL_FLOAT) {
+        if (!global_state.have_color_buffer_float) {
+            error(user_context) << "This system lacks support for float framebuffers";
+            return false;
+        }
+
+        if (global_state.profile == OpenGL) {
+            *internal_format = GL_RGBA32F;
+        } else {
+            // See: https://www.khronos.org/registry/OpenGL/specs/es/3.2/es_spec_3.2.pdf
+            // Refer to table 8.10
             switch (*format) {
             case GL_RED:
+                *internal_format = GL_R32F;
+                break;
             case GL_RG:
+                *internal_format = GL_RG32F;
+                break;
             case GL_RGB:
+                error(user_context) << "OpenGL ES does not allow rendering to RGB (only R, RG, RGBA supported)";
+                return false;
             case GL_RGBA:
                 *internal_format = GL_RGBA32F;
                 break;
             default:
-                error(user_context) << "OpenGL: Cannot select internal format for format " << *format;
-                return false;
+                error(user_context) << "Invalid format " << (*format);
+                break;
             }
-        } else {
-            *internal_format = *format;
         }
-        break;
+    } else {
+        *internal_format = *format;
     }
 
     return true;

src/runtime/opengl_egl_context.cpp

@@ -62,18 +62,51 @@ EGLAPI EGLSurface EGLAPIENTRY eglCreatePbufferSurface(EGLDisplay dpy, EGLConfig
                                                       const EGLint *attrib_list);
 EGLAPI EGLBoolean EGLAPIENTRY eglMakeCurrent(EGLDisplay dpy, EGLSurface draw,
                                              EGLSurface read, EGLContext ctx);
+EGLAPI EGLBoolean EGLAPIENTRY eglTerminate(EGLDisplay display);
 
 EGLAPI void *eglGetProcAddress(const char *procname);
 
 extern int strcmp(const char *, const char *);
 
+static bool halide_egl_try_display(EGLDisplay display, EGLConfig *config) {
+    if (display == EGL_NO_DISPLAY) {
+        return false;
+    }
+
+    if (!eglInitialize(display, nullptr, nullptr)) {
+        return false;
+    }
+
+    // clang-format off
+    EGLint attribs[] = {
+        EGL_SURFACE_TYPE,    EGL_PBUFFER_BIT,
+        EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
+        EGL_RED_SIZE,        8,
+        EGL_GREEN_SIZE,      8,
+        EGL_BLUE_SIZE,       8,
+        EGL_ALPHA_SIZE,      8,
+        EGL_NONE
+    };
+    // clang-format on
+
+    int numconfig;
+    EGLBoolean result = eglChooseConfig(display, attribs, config, 1, &numconfig);
+    bool success = result == EGL_TRUE && numconfig == 1;
+    if (!success) {
+        eglTerminate(display);
+    }
+    return success;
+}
+
 WEAK int halide_opengl_create_context(void *user_context) {
     if (eglGetCurrentContext() != EGL_NO_CONTEXT) {
         return 0;
     }
 
     EGLDisplay display = eglGetDisplay(EGL_DEFAULT_DISPLAY);
-    if (display == EGL_NO_DISPLAY || !eglInitialize(display, nullptr, nullptr)) {
+    EGLConfig config;
+
+    if (!halide_egl_try_display(display, &config)) {
         PFNEGLQUERYDEVICESEXTPROC eglQueryDevicesEXT =
             reinterpret_cast<PFNEGLQUERYDEVICESEXTPROC>(
                 eglGetProcAddress("eglQueryDevicesEXT"));
@@ -92,83 +125,43 @@ WEAK int halide_opengl_create_context(void *user_context) {
         EGLDeviceEXT egl_devices[kMaxDevices];
         EGLint num_devices = 0;
         EGLint egl_error = eglGetError();
-        if (!eglQueryDevicesEXT(kMaxDevices, egl_devices, &num_devices) ||
-            egl_error != EGL_SUCCESS) {
+        if (!eglQueryDevicesEXT(kMaxDevices, egl_devices, &num_devices) || egl_error != EGL_SUCCESS) {
             return 1;
         }
 
         EGLBoolean initialized = EGL_FALSE;
         for (EGLint i = 0; i < num_devices; ++i) {
-            display = eglGetPlatformDisplayEXT(EGL_PLATFORM_DEVICE_EXT,
-                                               egl_devices[i], nullptr);
-            if (eglGetError() == EGL_SUCCESS && display != EGL_NO_DISPLAY) {
-                int major, minor;
-                initialized = eglInitialize(display, &major, &minor);
-                if (eglGetError() == EGL_SUCCESS && initialized == EGL_TRUE) {
-                    break;
-                }
+            display = eglGetPlatformDisplayEXT(EGL_PLATFORM_DEVICE_EXT, egl_devices[i], nullptr);
+            if (halide_egl_try_display(display, &config)) {
+                initialized = EGL_TRUE;
+                break;
             }
         }
 
-        if (eglGetError() != EGL_SUCCESS || initialized != EGL_TRUE) {
-            error(user_context) << "Could not initialize EGL display: "
-                                << eglGetError();
+        if (initialized != EGL_TRUE) {
+            error(user_context) << "Could not initialize EGL display: " << eglGetError();
             return 1;
         }
     }
 
-    EGLint attribs[] = {
-        EGL_SURFACE_TYPE,
-        EGL_PBUFFER_BIT,
-        EGL_RENDERABLE_TYPE,
-        EGL_OPENGL_ES2_BIT,
-        EGL_RED_SIZE,
-        8,
-        EGL_GREEN_SIZE,
-        8,
-        EGL_BLUE_SIZE,
-        8,
-        EGL_ALPHA_SIZE,
-        8,
-        EGL_NONE,
-    };
-    EGLConfig config;
-    int numconfig;
-    EGLBoolean result = eglChooseConfig(display, attribs, &config, 1, &numconfig);
-    if (result != EGL_TRUE || numconfig != 1) {
-        error(user_context) << "eglChooseConfig(): config not found: "
-                            << " result=" << (int)result
-                            << " eglGetError=" << eglGetError()
-                            << " numConfig=" << numconfig;
-        return -1;
-    }
-
-    EGLint context_attribs[] = {
-        EGL_CONTEXT_CLIENT_VERSION, 2,
-        EGL_NONE};
-    EGLContext context = eglCreateContext(display, config, EGL_NO_CONTEXT,
-                                          context_attribs);
+    EGLint context_attribs[] = {EGL_CONTEXT_CLIENT_VERSION, 2, EGL_NONE};
+    EGLContext context = eglCreateContext(display, config, EGL_NO_CONTEXT, context_attribs);
     if (context == EGL_NO_CONTEXT) {
         error(user_context) << "Error: eglCreateContext failed: " << eglGetError();
-        return -1;
+        return 1;
     }
 
-    EGLint surface_attribs[] = {
-        EGL_WIDTH, 1,
-        EGL_HEIGHT, 1,
-        EGL_NONE};
+    EGLint surface_attribs[] = {EGL_WIDTH, 1, EGL_HEIGHT, 1, EGL_NONE};
     EGLSurface surface = eglCreatePbufferSurface(display, config, surface_attribs);
     if (surface == EGL_NO_SURFACE) {
         error(user_context) << "Error: Could not create EGL window surface: " << eglGetError();
-        return -1;
+        return 1;
     }
 
-    result = eglMakeCurrent(display, surface, surface, context);
+    EGLBoolean result = eglMakeCurrent(display, surface, surface, context);
     if (result != EGL_TRUE) {
-        error(user_context) << "eglMakeCurrent fails: "
-                            << " result=" << (int)result
-                            << " eglGetError=" << eglGetError();
-        return -1;
+        error(user_context) << "eglMakeCurrent fails: result=" << (int)result << " eglGetError=" << eglGetError();
+        return 1;
     }
     return 0;
 }