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.
Created attachment 161664 [details] Patch
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?
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".
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.
Created attachment 162748 [details] Patch
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.
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.
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!
Created attachment 162773 [details] Patch
Created attachment 162776 [details] Patch Updated patch with doc fixes
Committed r127874: <http://trac.webkit.org/changeset/127874>
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.