Bug 95556

Summary: Build of OpenGLShims.cpp against EGL/GLES2 platforms is broken
Product: WebKit Reporter: Milian Wolff <milian.wolff>
Component: WebKit QtAssignee: Milian Wolff <milian.wolff>
Status: RESOLVED FIXED    
Severity: Normal CC: hausmann, jturcotte, noam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 90879, 95466    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch jturcotte: review+

Milian Wolff
Reported 2012-08-31 04:38:08 PDT
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.
Attachments
Patch (1.98 KB, patch)
2012-08-31 04:41 PDT, Milian Wolff 		no flags
DetailsFormatted DiffDiff
Patch (4.85 KB, patch)
2012-09-07 05:17 PDT, Simon Hausmann 		no flags
DetailsFormatted DiffDiff
Patch (5.90 KB, patch)
2012-09-07 07:59 PDT, Simon Hausmann 		no flags
DetailsFormatted DiffDiff
Patch (5.88 KB, patch)
2012-09-07 08:11 PDT, Simon Hausmann 		jturcotte: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Milian Wolff
Comment 1 2012-08-31 04:41:27 PDT
Created attachment 161664 [details] Patch
Simon Hausmann
Comment 2 2012-08-31 05:56:28 PDT
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
Comment 3 2012-08-31 06:14:00 PDT
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 4 2012-09-07 00:08:13 PDT
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
Comment 5 2012-09-07 05:17:36 PDT
Created attachment 162748 [details] Patch
Jocelyn Turcotte
Comment 6 2012-09-07 06:32:02 PDT
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 7 2012-09-07 06:44:18 PDT
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 8 2012-09-07 07:10:01 PDT
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
Comment 9 2012-09-07 07:59:01 PDT
Created attachment 162773 [details] Patch
Simon Hausmann
Comment 10 2012-09-07 08:11:28 PDT
Created attachment 162776 [details] Patch Updated patch with doc fixes
Simon Hausmann
Comment 11 2012-09-07 09:05:25 PDT
Committed r127874: <http://trac.webkit.org/changeset/127874>
Joseph Pecoraro
Comment 12 2015-04-20 14:01:16 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.