From df77e8565e9822bfcdeb88b380f64f0f303aaef6 Mon Sep 17 00:00:00 2001 From: Neil Roberts Date: Wed, 20 Jun 2012 12:42:31 +0100 Subject: [PATCH] Don't use eglGetProcAddress to retrieve core functions According to the EGL spec, eglGetProcAddress should only be used to retrieve extension functions. It also says that returning non-NULL does not mean the extension is available so you could interpret this as saying that the function is allowed to return garbage for core functions. This seems to happen at least for the Android implementation of EGL. To workaround this the winsys's are now passed down a flag to say whether the function is from the core API. This information is already in the gl-prototypes headers as the minimum core GL version and as a pair of flags to specify whether it is available in core GLES1 and GLES2. If the function is in core the EGL winsys will now avoid using eglGetProcAddress and always fallback to querying the library directly with the GModule API. The GLX winsys is left alone because glXGetProcAddress apparently supports querying core API and extension functions. The WGL winsys could ideally be changed because wglGetProcAddress should also only be used for extension functions but the situation is slightly different because WGL considers anything from GL > 1.1 to be an extension so it would need a bit more information to determine whether to query the function directly from the library. The SDL winsys is also left alone because it's not as easy to portably determine which GL library SDL has chosen to load in order to resolve the symbols directly. Reviewed-by: Robert Bragg (cherry picked from commit 72089730ad06ccdd38a344279a893965ae68cec1) Since we aren't able to break API on the 1.12 branch cogl_get_proc_address is still supported but isn't easily able to determine whether the given name corresponds to a core symbol or not. For now we just assume the symbol being queried isn't part of the core GL api and update the documentation accordingly. --- cogl/cogl-feature-private.c | 12 ++++++++++-- cogl/cogl-renderer-private.h | 3 ++- cogl/cogl-renderer.c | 5 +++-- cogl/cogl.c | 2 +- cogl/cogl1-context.h | 4 ++++ cogl/driver/gl/cogl-gl.c | 3 ++- cogl/driver/gles/cogl-gles.c | 3 ++- cogl/winsys/cogl-winsys-egl.c | 8 +++++--- cogl/winsys/cogl-winsys-glx.c | 7 ++++++- cogl/winsys/cogl-winsys-private.h | 3 ++- cogl/winsys/cogl-winsys-sdl.c | 10 ++++++++++ cogl/winsys/cogl-winsys-sdl2.c | 12 +++++++++++- cogl/winsys/cogl-winsys-stub.c | 3 ++- cogl/winsys/cogl-winsys-wgl.c | 12 ++++++++++-- 14 files changed, 70 insertions(+), 17 deletions(-) diff --git a/cogl/cogl-feature-private.c b/cogl/cogl-feature-private.c index e6f0bd3fc..7883b3cbb 100644 --- a/cogl/cogl-feature-private.c +++ b/cogl/cogl-feature-private.c @@ -45,6 +45,7 @@ _cogl_feature_check (CoglRenderer *renderer, { const char *suffix = NULL; int func_num; + CoglBool in_core; /* First check whether the functions should be directly provided by GL */ @@ -55,7 +56,10 @@ _cogl_feature_check (CoglRenderer *renderer, (data->gles_availability & COGL_EXT_IN_GLES)) || (driver == COGL_DRIVER_GLES2 && (data->gles_availability & COGL_EXT_IN_GLES2))) - suffix = ""; + { + suffix = ""; + in_core = TRUE; + } else { /* Otherwise try all of the extensions */ @@ -107,6 +111,8 @@ _cogl_feature_check (CoglRenderer *renderer, break; } } + + in_core = FALSE; } /* If we couldn't find anything that provides the functions then @@ -122,7 +128,9 @@ _cogl_feature_check (CoglRenderer *renderer, full_function_name = g_strconcat (data->functions[func_num].name, suffix, NULL); - func = _cogl_renderer_get_proc_address (renderer, full_function_name); + func = _cogl_renderer_get_proc_address (renderer, + full_function_name, + in_core); g_free (full_function_name); if (func == NULL) diff --git a/cogl/cogl-renderer-private.h b/cogl/cogl-renderer-private.h index 808102c56..72ea9d05b 100644 --- a/cogl/cogl-renderer-private.h +++ b/cogl/cogl-renderer-private.h @@ -92,6 +92,7 @@ _cogl_renderer_remove_native_filter (CoglRenderer *renderer, void * _cogl_renderer_get_proc_address (CoglRenderer *renderer, - const char *name); + const char *name, + CoglBool in_core); #endif /* __COGL_RENDERER_PRIVATE_H */ diff --git a/cogl/cogl-renderer.c b/cogl/cogl-renderer.c index fa84625ff..82e45b685 100644 --- a/cogl/cogl-renderer.c +++ b/cogl/cogl-renderer.c @@ -509,11 +509,12 @@ cogl_renderer_get_winsys_id (CoglRenderer *renderer) void * _cogl_renderer_get_proc_address (CoglRenderer *renderer, - const char *name) + const char *name, + CoglBool in_core) { const CoglWinsysVtable *winsys = _cogl_renderer_get_winsys (renderer); - return winsys->renderer_get_proc_address (renderer, name); + return winsys->renderer_get_proc_address (renderer, name, in_core); } int diff --git a/cogl/cogl.c b/cogl/cogl.c index a1fa5a724..6ab38c5ab 100644 --- a/cogl/cogl.c +++ b/cogl/cogl.c @@ -94,7 +94,7 @@ cogl_get_proc_address (const char* name) { _COGL_GET_CONTEXT (ctx, NULL); - return _cogl_renderer_get_proc_address (ctx->display->renderer, name); + return _cogl_renderer_get_proc_address (ctx->display->renderer, name, FALSE); } CoglBool diff --git a/cogl/cogl1-context.h b/cogl/cogl1-context.h index 49744ac4d..f9cf94606 100644 --- a/cogl/cogl1-context.h +++ b/cogl/cogl1-context.h @@ -88,6 +88,10 @@ cogl_features_available (CoglFeatureFlags features); * as a wrapper around glXGetProcAddress() or whatever is the * appropriate function for the current backend. * + * This function should not be used to query core opengl API + * symbols since eglGetProcAddress for example doesn't allow this and + * and may return a junk pointer if you do. + * * Return value: a pointer to the requested function or %NULL if the * function is not available. */ diff --git a/cogl/driver/gl/cogl-gl.c b/cogl/driver/gl/cogl-gl.c index bc1e5ec73..3976d5a6f 100644 --- a/cogl/driver/gl/cogl-gl.c +++ b/cogl/driver/gl/cogl-gl.c @@ -321,7 +321,8 @@ _cogl_driver_update_features (CoglContext *ctx, can expect */ ctx->glGetString = (void *) _cogl_renderer_get_proc_address (ctx->display->renderer, - "glGetString"); + "glGetString", + TRUE); if (!check_gl_version (ctx, error)) return FALSE; diff --git a/cogl/driver/gles/cogl-gles.c b/cogl/driver/gles/cogl-gles.c index b570feab4..9e46d12da 100644 --- a/cogl/driver/gles/cogl-gles.c +++ b/cogl/driver/gles/cogl-gles.c @@ -171,7 +171,8 @@ _cogl_driver_update_features (CoglContext *context, can expect */ context->glGetString = (void *) _cogl_renderer_get_proc_address (context->display->renderer, - "glGetString"); + "glGetString", + TRUE); COGL_NOTE (WINSYS, "Checking features\n" diff --git a/cogl/winsys/cogl-winsys-egl.c b/cogl/winsys/cogl-winsys-egl.c index 289bc808c..800ebce3d 100644 --- a/cogl/winsys/cogl-winsys-egl.c +++ b/cogl/winsys/cogl-winsys-egl.c @@ -118,11 +118,13 @@ get_error_string (void) static CoglFuncPtr _cogl_winsys_renderer_get_proc_address (CoglRenderer *renderer, - const char *name) + const char *name, + CoglBool in_core) { - void *ptr; + void *ptr = NULL; - ptr = eglGetProcAddress (name); + if (!in_core) + ptr = eglGetProcAddress (name); /* eglGetProcAddress doesn't support fetching core API so we need to get that separately with GModule */ diff --git a/cogl/winsys/cogl-winsys-glx.c b/cogl/winsys/cogl-winsys-glx.c index 665010664..c733ec406 100644 --- a/cogl/winsys/cogl-winsys-glx.c +++ b/cogl/winsys/cogl-winsys-glx.c @@ -129,10 +129,15 @@ static const CoglFeatureData winsys_feature_data[] = static CoglFuncPtr _cogl_winsys_renderer_get_proc_address (CoglRenderer *renderer, - const char *name) + const char *name, + CoglBool in_core) { CoglGLXRenderer *glx_renderer = renderer->winsys; + /* The GLX_ARB_get_proc_address extension documents that this should + * work for core functions too so we don't need to do anything + * special with in_core */ + return glx_renderer->glXGetProcAddress ((const GLubyte *) name); } diff --git a/cogl/winsys/cogl-winsys-private.h b/cogl/winsys/cogl-winsys-private.h index cd9ca2e66..a39cbd61e 100644 --- a/cogl/winsys/cogl-winsys-private.h +++ b/cogl/winsys/cogl-winsys-private.h @@ -70,7 +70,8 @@ typedef struct _CoglWinsysVtable CoglFuncPtr (*renderer_get_proc_address) (CoglRenderer *renderer, - const char *name); + const char *name, + CoglBool in_core); CoglBool (*renderer_connect) (CoglRenderer *renderer, GError **error); diff --git a/cogl/winsys/cogl-winsys-sdl.c b/cogl/winsys/cogl-winsys-sdl.c index 58c177a40..39843fb69 100644 --- a/cogl/winsys/cogl-winsys-sdl.c +++ b/cogl/winsys/cogl-winsys-sdl.c @@ -54,6 +54,16 @@ static CoglFuncPtr _cogl_winsys_renderer_get_proc_address (CoglRenderer *renderer, const char *name) { + /* XXX: It's not totally clear whether it's safe to call this for + * core functions. From the code it looks like the implementations + * will fall back to using some form of dlsym if the winsys + * GetProcAddress function returns NULL. Presumably this will work + * in most cases apart from EGL platforms that return invalid + * pointers for core functions. It's awkward for this code to get a + * handle to the GL module that SDL has chosen to load so just + * calling SDL_GL_GetProcAddress is probably the best we can do + * here. */ + #ifdef COGL_HAS_SDL_GLES_SUPPORT if (renderer->driver != COGL_DRIVER_GL) return SDL_GLES_GetProcAddress (name); diff --git a/cogl/winsys/cogl-winsys-sdl2.c b/cogl/winsys/cogl-winsys-sdl2.c index 44843ade1..f9b94e97e 100644 --- a/cogl/winsys/cogl-winsys-sdl2.c +++ b/cogl/winsys/cogl-winsys-sdl2.c @@ -61,8 +61,18 @@ typedef struct _CoglOnscreenSdl2 static CoglFuncPtr _cogl_winsys_renderer_get_proc_address (CoglRenderer *renderer, - const char *name) + const char *name, + CoglBool in_core) { + /* XXX: It's not totally clear whether it's safe to call this for + * core functions. From the code it looks like the implementations + * will fall back to using some form of dlsym if the winsys + * GetProcAddress function returns NULL. Presumably this will work + * in most cases apart from EGL platforms that return invalid + * pointers for core functions. It's awkward for this code to get a + * handle to the GL module that SDL has chosen to load so just + * calling SDL_GL_GetProcAddress is probably the best we can do + * here. */ return SDL_GL_GetProcAddress (name); } diff --git a/cogl/winsys/cogl-winsys-stub.c b/cogl/winsys/cogl-winsys-stub.c index 02ebb8bab..264862272 100644 --- a/cogl/winsys/cogl-winsys-stub.c +++ b/cogl/winsys/cogl-winsys-stub.c @@ -47,7 +47,8 @@ static int _cogl_winsys_stub_dummy_ptr; static CoglFuncPtr _cogl_winsys_renderer_get_proc_address (CoglRenderer *renderer, - const char *name) + const char *name, + CoglBool in_core) { static GModule *module = NULL; diff --git a/cogl/winsys/cogl-winsys-wgl.c b/cogl/winsys/cogl-winsys-wgl.c index 785140428..61cd335c2 100644 --- a/cogl/winsys/cogl-winsys-wgl.c +++ b/cogl/winsys/cogl-winsys-wgl.c @@ -127,14 +127,22 @@ static const CoglFeatureData winsys_feature_data[] = static CoglFuncPtr _cogl_winsys_renderer_get_proc_address (CoglRenderer *renderer, - const char *name) + const char *name, + CoglBool in_core) { CoglRendererWgl *wgl_renderer = renderer->winsys; void *proc = wglGetProcAddress ((LPCSTR) name); /* The documentation for wglGetProcAddress implies that it only returns pointers to extension functions so if it fails we'll try - resolving the symbol directly from the the GL library */ + resolving the symbol directly from the the GL library. We could + completely avoid using wglGetProcAddress if in_core is TRUE but + on WGL any function that is in GL > 1.1 is considered an + extension and is not directly exported from opengl32.dll. + Therefore we currently just assume wglGetProcAddress will return + NULL for GL 1.1 functions and we can fallback to querying them + directly from the library */ + if (proc == NULL) { if (wgl_renderer->gl_module == NULL)