[Cogl] [PATCH 1/2] Don't use eglGetProcAddress to retrieve core functions
Robert Bragg
robert at sixbynine.org
Wed Jun 20 11:39:37 PDT 2012
Cool; this looks good to land to me.
Reviewed-by: Robert Bragg <robert at linux.intel.com>
thanks,
- Robert
On Wed, Jun 20, 2012 at 4:54 PM, Neil Roberts <neil at linux.intel.com> wrote:
> 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.
> ---
> cogl/cogl-feature-private.c | 12 ++++++++++--
> cogl/cogl-renderer-private.h | 3 ++-
> cogl/cogl-renderer.c | 5 +++--
> 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 ++++++++++--
> 12 files changed, 65 insertions(+), 16 deletions(-)
>
> diff --git a/cogl/cogl-feature-private.c b/cogl/cogl-feature-private.c
> index e6f0bd3..7883b3c 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 808102c..72ea9d0 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 fa84625..82e45b6 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/driver/gl/cogl-gl.c b/cogl/driver/gl/cogl-gl.c
> index bc1e5ec..3976d5a 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 b570fea..9e46d12 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 871eea1..17ed67b 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 6650106..c733ec4 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 cd9ca2e..a39cbd6 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 58c177a..39843fb 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 44843ad..f9b94e9 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 02ebb8b..2648622 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 7851404..61cd335 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)
> --
> 1.7.3.16.g9464b
>
> _______________________________________________
> Cogl mailing list
> Cogl at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/cogl
More information about the Cogl
mailing list