Code review comment for lp:~marcoil/glproxy/extensions-cleanup

Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

> 472 + static char const *gl_extensions = "";

It's not safe to cache gl_extensions in this manner, as the extensions are context-dependent. For example, glProxyGetExtProcAddr() could be called again after a user has changed GL context or by two different threads having different GL contexts.

I would argue that we don't need to cache the extension strings anyway as 1. getting the extension string is fast enough and 2. extension checking isn't usually in a performance-critical path.

> 501 + if (!strstr(backend_extensions, extension) &&
> 502 + !strstr(gl_extensions, extension))
> 503 + return NULL;

There are extension names that are prefixes of other extension names (eg EGL_KHR_image and EGL_KHR_image_base), so a simple strstr() check is not enough. You can check cairo-gl-info.c in the cairo sources for a more robust implementation of an extension check.

By the way, this complication is the reason glGetString(GL_EXTENSIONS) was deprecated in GL 3.0 (and removed in GL 3.1) in favor of glGetStringi(GL_EXTENSIONS, i).

review: Needs Fixing

« Back to merge proposal