Bug 95556 – Build of OpenGLShims.cpp against EGL/GLES2 platforms is broken

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Milian Wolff]

When trying to build QtWebKit with OpenGL support for QNX (see bug 95466), you will run into compilation errors in OpenGLShims.cpp:

/home/milian/projects/qt5/qtwebkit/Source/WebCore/platform/graphics/OpenGLShims.cpp: In function 'bool WebCore::initializeOpenGLShims()':
/home/milian/projects/qt5/qtwebkit/Source/WebCore/platform/graphics/OpenGLShims.cpp:117: error: expected id-expression before numeric constant
/home/milian/projects/qt5/qtwebkit/Source/WebCore/platform/graphics/OpenGLShims.cpp:117: error: expected ';' before numeric constant
/home/milian/projects/qt5/qtwebkit/Source/WebCore/platform/graphics/OpenGLShims.cpp:173: error: '::glRenderbufferStorageMultisampleAPPLE' has not been declared

The reason is that OpenGLShims.h includes qopenglfunctions.h which is the khronos official header that includes all possible extensions. According to a colleague of mine, consumers should be doing runtime checks in addition to compile time checks. An easier fix for QNX at least though would be the following: Undefining the defines that are known to be non-existant on QNX, in our case: GL_ANGLE_framebuffer_blit, GL_APPLE_framebuffer_multisample and GL_ANGLE_framebuffer_multisample.

Unrelated to this, I think the following line is completely wrong:

openGLFunctionTable()->glBlitFramebuffer = ::GL_ANGLE_framebuffer_blit;

GL_ANGLE_framebuffer_blit is a define of "1" and not a function pointer. I *think* the correct version would be

openGLFunctionTable()->glBlitFramebuffer = ::glBlitFramebufferANGLE;

Anyhow, I'll include this in a patch and you can decide on whether that should be included as well.

[Milian Wolff]

Created attachment 161664 [details]
Patch

[Simon Hausmann]

Comment on attachment 161664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161664&action=review

> Source/WebCore/platform/graphics/OpenGLShims.cpp:34
> +#if OS(QNX)
> +// undefine known unimplemented extensions, see https://bugs.webkit.org/show_bug.cgi?id=95556
> +#undef GL_ANGLE_framebuffer_blit
> +#undef GL_APPLE_framebuffer_multisample
> +#undef GL_ANGLE_framebuffer_multisample
> +#endif

Hm, does this mean that there are angle headers clashing with QNX GLES headers?

[Milian Wolff]

No, the ANGLE headers are not included here at all (at least the headers in ANGLE/include/{GLES2,KHR,EGL}). See also bug 95560.

The issue here is the code in WebKit, it assumes that e.g. GL_APPLE_framebuffer_multisample is only defined if the function glRenderbufferStorageMultisampleAPPLE exists. This is not the case, as you can see by looking at $(qmake -query QT_INSTALL_HEADERS)/Qt/qopengles2ext.h (sorry, I mentioned qopenglfunctions.h but meant qopengles2ext.h which is included by qopengl.h which is included by qopenglfunctions.h). qopengles2ext.h is what I said above:

"the khronos official header that includes all possible extensions. According to a colleague of mine, consumers should be doing runtime checks in addition to compile time checks".

[Simon Hausmann]

Comment on attachment 161664 [details]
Patch

I think you're right in the sense that functions like glBlitFramebufferANGLE and other extensions should only be looked up at run-time. That means we should _not_ try to access ::glBlitFramebufferANGLE directly and we should _not_ #undef GL_ANGLE_framebuffer_blit but instead use ASSIGN_FUNCTION_TABLE_ENTRY() to look it up. If there is a fallback for example, then we should fall back.

[Simon Hausmann]

Created attachment 162748 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 162748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162748&action=review

> Source/WebCore/ChangeLog:20
> +        Qt (and there it it breaks the Windows build)

"it it"

> Source/WebCore/ChangeLog:23
> +        This patch implements the dynamic lookup of glBlitFramebufferANGLE
> +        (used in GraphicsContext3DOpenGLES.cpp) as well as the dynamic lookup of the

Humm couldn't find it in GraphicsContext3DOpenGLES.cpp, no glBlitFramebuffer there.

> Source/WebCore/platform/graphics/OpenGLShims.cpp:105
> +#define ASSIGN_FUNCTION_TABLE_ENTRY_EXT(FunctionName, ExtensionName) \
> +    openGLFunctionTable()->FunctionName = reinterpret_cast<FunctionName##Type>(getProcAddress(ExtensionName))

It would be nice to get a success parameter like the other macro, and set it to false if getProcAddress returns 0;
It would also be nice if ExtensionName was concatenated to FunctionName and just pass "EXT", "ANGLE" or "APPLE" instead of duplicating the function name and risk copy-paste bugs.

[Jocelyn Turcotte]

Comment on attachment 162748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162748&action=review

>> Source/WebCore/platform/graphics/OpenGLShims.cpp:105
>> +    openGLFunctionTable()->FunctionName = reinterpret_cast<FunctionName##Type>(getProcAddress(ExtensionName))
> 
> It would be nice to get a success parameter like the other macro, and set it to false if getProcAddress returns 0;
> It would also be nice if ExtensionName was concatenated to FunctionName and just pass "EXT", "ANGLE" or "APPLE" instead of duplicating the function name and risk copy-paste bugs.

Since you only want a separate macro to allow using compile-time resolving, maybe you could also just add "ANGLE" to the list of tested suffixes in lookupOpenGLFunctionAddress and let ASSIGN_FUNCTION_TABLE_ENTRY_EXT use lookupOpenGLFunctionAddress too.

[Simon Hausmann]

Comment on attachment 162748 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162748&action=review

>> Source/WebCore/ChangeLog:20
>> +        Qt (and there it it breaks the Windows build)
> 
> "it it"

Ooops, will fix.

>> Source/WebCore/ChangeLog:23
>> +        (used in GraphicsContext3DOpenGLES.cpp) as well as the dynamic lookup of the
> 
> Humm couldn't find it in GraphicsContext3DOpenGLES.cpp, no glBlitFramebuffer there.

For fun it's redefined to glBlitFramebufferEXT in OpenGLESShims.h.

The shims stuff is quite messy ;(

>>> Source/WebCore/platform/graphics/OpenGLShims.cpp:105
>>> +    openGLFunctionTable()->FunctionName = reinterpret_cast<FunctionName##Type>(getProcAddress(ExtensionName))
>> 
>> It would be nice to get a success parameter like the other macro, and set it to false if getProcAddress returns 0;
>> It would also be nice if ExtensionName was concatenated to FunctionName and just pass "EXT", "ANGLE" or "APPLE" instead of duplicating the function name and risk copy-paste bugs.
> 
> Since you only want a separate macro to allow using compile-time resolving, maybe you could also just add "ANGLE" to the list of tested suffixes in lookupOpenGLFunctionAddress and let ASSIGN_FUNCTION_TABLE_ENTRY_EXT use lookupOpenGLFunctionAddress too.

Hmm, it's tempting to do so. Hmmm. OK!

[Simon Hausmann]

Created attachment 162773 [details]
Patch

[Simon Hausmann]

Created attachment 162776 [details]
Patch

Updated patch with doc fixes

[Simon Hausmann]

Committed r127874: <http://trac.webkit.org/changeset/127874>

[Joseph Pecoraro]

Comment on attachment 162776 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162776&action=review

> Source/WebCore/platform/graphics/OpenGLShims.cpp:86
>      fullFunctionName.append("EXT");
>      target = getProcAddress(fullFunctionName.utf8().data());
>  
> +#if defined(GL_ES_VERSION_2_0)
> +    fullFunctionName = functionName;
> +    fullFunctionName.append("ANGLE");
> +    target = getProcAddress(fullFunctionName.utf8().data());

Looks like this will always overwrite the "target" assigned in line 81. I'll file a new bug about cleaning this up.