[qt] Webkit fails to compile if EGL headers are not in default INCLUDEPATH
Created attachment 192227 [details] Patch
Comment on attachment 192227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192227&action=review > Source/WebCore/ChangeLog:3 > + [qt] Webkit fails to compile if EGL headers are not in default INCLUDEPATH qt -> Qt Webkit -> WebKit > Source/WebCore/ChangeLog:13 > + Reviewed by NOBODY (OOPS!). This line should normally be kept one empty line after the bug URL. > Source/WebCore/Target.pri:4153 > contains(QT_CONFIG, opengl) | contains(QT_CONFIG, opengles2) { I think that what you are trying to fix could all be done by qmake if you add "CONFIG += opengl egl" inside this block. Could you try?
Thanks for the feedback. >I think that what you are trying to fix could all be done by qmake if you add "CONFIG += opengl egl" inside this block. Could you try? A potential problem I see with that, is that it could interfere with this section in WebCore.pri: == use?(3D_GRAPHICS) { win32: { win32-g++: { # Make sure OpenGL libs are after the webcore lib so MinGW can resolve symbols contains(QT_CONFIG, opengles2) { CONFIG(debug, debug|release):contains(QT_CONFIG, angle) { LIBS += $$QMAKE_LIBS_OPENGL_ES2_DEBUG } else { LIBS += $$QMAKE_LIBS_OPENGL_ES2 } } else { LIBS += $$QMAKE_LIBS_OPENGL } } } else { contains(QT_CONFIG, opengles2): LIBS += -lEGL } } == Think that if I do "CONFIG += opengl" it will not only handle the INCLUDEPATH, but also will also add the libraries, and that might render the MinGW workaround here invalid.
(In reply to comment #3) > Think that if I do "CONFIG += opengl" it will not only handle the INCLUDEPATH, but also will also add the libraries, and that might render the MinGW workaround here invalid. That's a good point, I didn't think about it. To be sure I just tried with MinGW and it doesn't break. The libs were already duplicated on the link command line with the MinGW hack, so adding CONFIG += opengl is fine. You could probably just add it together with the current "CONFIG += opengl-shims" to fix your issue.
Created attachment 192482 [details] Patch
(In reply to comment #4) > (In reply to comment #3) > You could probably just add it together with the current "CONFIG += opengl-shims" to fix your issue. Didn't notice that before, but opengl-shims also provides generic OpenGL ES2 headers. So adding the OGLES2 headers in my earlier patch was redundant, the only thing that was actually necessary is the EGL headers. Can confirm that adding just "egl" to CONFIG fixes my problem. Do wonder if it wouldn't be a better long term solution to add the EGL functions to opengl-shims as well, so runtime detection of the presense of the library could be done. But think that's outside the scope of this bug report, so this will do.
Comment on attachment 192482 [details] Patch Great, thanks for the patch.
Comment on attachment 192482 [details] Patch Cancelling the CQ to figure if we need it also without OpenGLES
(In reply to comment #6) > Do wonder if it wouldn't be a better long term solution to add the EGL functions to opengl-shims as well, > so runtime detection of the presense of the library could be done. > But think that's outside the scope of this bug report, so this will do. I'm not an expert in this area, but to me it feels like the shims are wrapping APIs needing to go through getProcAddress to be resolved dynamically. All this to handle the various function availability differences given by the installed OpenGL library on the machine. If all the EGL calls we need can be linked at compile time through normal library linking, I think that we should keep it that way as long as possible.
I didn't want to bother you with this, so we can just push this patch and we'll fix the rest later, but the best way to fix this would be to remove those lines from WebCore.pri: > win32: { > ... > } else { > contains(QT_CONFIG, opengles2): LIBS += -lEGL > } And replace them with this line in Target.pri (not merged with the opengl-shims config): > contains(QT_CONFIG, opengles2): CONFIG += egl So tell me if you would like to do that change properly. Else I can do that patch afterward, but that would make you patch relatively pointless so you might as well do the final fix yourself. Sorry going back and forth with this, Allan just pointed me that we currently don't want EGL without OpenGL ES.
> If all the EGL calls we need can be linked at compile time through normal >library linking, I think that we should keep it that way as long as possible. Well, I'm not sure if all ARM devices have OpenGL ES/EGL libraries installed at all. Some vendors might not supply GPU accelerated ones, and having ones doing software rendering makes little sense on such slow CPUs. Naturally, if you are creating a build for a specific device, 3D functionality can simply be disabled at compile time. But it would be nice if generic Linux/ARM distributions could provide a single generic QtWebkit package that works on multiple devices, and resorts to disabling 3D functionality at runtime if libEGL is not present. >I didn't want to bother you with this, so we can just push this patch and we'll >fix the rest later, but the best way to fix this would be to remove those lines >from WebCore.pri: > >> win32: { >> ... >> } else { >> contains(QT_CONFIG, opengles2): LIBS += -lEGL >> } Do you mean just the non-win32 EGL section, or including the win32 section dealing with OpenGL libs? Linking OpenGL libs there should not be necessary if shims take care of that at runtime, but do not have a Windows box to test on.
Created attachment 192510 [details] patch
(In reply to comment #11) > Well, I'm not sure if all ARM devices have OpenGL ES/EGL libraries installed at all. > Some vendors might not supply GPU accelerated ones, and having ones doing software rendering makes little sense on such slow CPUs. > > Naturally, if you are creating a build for a specific device, 3D functionality can simply be disabled at compile time. > But it would be nice if generic Linux/ARM distributions could provide a single generic QtWebkit package that works on multiple devices, and resorts to disabling 3D functionality at runtime if libEGL is not present. I don't know this stuff enough to answer you, but if you face a concrete problem that is worth fixing that way we can work on shaping some patch. > >> win32: { > >> ... > >> } else { > >> contains(QT_CONFIG, opengles2): LIBS += -lEGL > >> } > > > Do you mean just the non-win32 EGL section, or including the win32 section dealing with OpenGL libs? > Linking OpenGL libs there should not be necessary if shims take care of that at runtime, but do not have a Windows box to test on. Some OpenGL function that are available on all implementations are linked at compile time so we still need to link with it. You can remove the surrounding win32 block, win32-g++ is only available on win32, as its name states, so no need for the extra scope check.
(In reply to comment #10) > I didn't want to bother you with this, so we can just push this patch and we'll fix the rest later, but the best way to fix this would be to remove those lines from WebCore.pri: > > > win32: { > > ... > > } else { > > contains(QT_CONFIG, opengles2): LIBS += -lEGL > > } > > And replace them with this line in Target.pri (not merged with the opengl-shims config): > > contains(QT_CONFIG, opengles2): CONFIG += egl Hmm, after removing the "LIBS += -lEGL": == Source/WebCore/release/libWebCore.a(Extensions3DOpenGLES.o): In function `WebCore::Extensions3DOpenGLES::supportsExtension(WTF::String const&)': Extensions3DOpenGLES.cpp:(.text._ZN7WebCore20Extensions3DOpenGLES17supportsExtensionERKN3WTF6StringE+0x14c): undefined reference to `eglGetProcAddress' Extensions3DOpenGLES.cpp:(.text._ZN7WebCore20Extensions3DOpenGLES17supportsExtensionERKN3WTF6StringE+0x160): undefined reference to `eglGetProcAddress' Extensions3DOpenGLES.cpp:(.text._ZN7WebCore20Extensions3DOpenGLES17supportsExtensionERKN3WTF6StringE+0x174): undefined reference to `eglGetProcAddress' Extensions3DOpenGLES.cpp:(.text._ZN7WebCore20Extensions3DOpenGLES17supportsExtensionERKN3WTF6StringE+0x188): undefined reference to `eglGetProcAddress' Extensions3DOpenGLES.cpp:(.text._ZN7WebCore20Extensions3DOpenGLES17supportsExtensionERKN3WTF6StringE+0x22c): undefined reference to `eglGetProcAddress' == Should I have the "CONFIG += egl" in WebCore.pri as well in addition to in Target.pri?
(In reply to comment #14) > Should I have the "CONFIG += egl" in WebCore.pri as well in addition to in Target.pri? Target.pri includes WebCore.pri, so after all maybe the fix should be to replace "LIBS += -lEGL" by "CONFIG += egl" in WebCore.pri.
(In reply to comment #15) >Target.pri includes WebCore.pri, so after all maybe the fix should be to replace >"LIBS += -lEGL" by "CONFIG += egl" in WebCore.pri. Thanks for the hint. Yes, that does work fine.
Created attachment 192528 [details] New patch
Comment on attachment 192528 [details] New patch View in context: https://bugs.webkit.org/attachment.cgi?id=192528&action=review > Source/WebCore/WebCore.pri:-230 > - win32: { We should probably keep that block now that we won't be doing that logic in Target.pri. This should avoid having different behaviors between win32-g++ and win32-msvc* regarding EGL. Sorry I should have mentioned it.
Created attachment 192533 [details] Just patching LIBS to CONFIG
Comment on attachment 192533 [details] Just patching LIBS to CONFIG r=me, thanks for holding on
Comment on attachment 192533 [details] Just patching LIBS to CONFIG Clearing flags on attachment: 192533 Committed r145506: <http://trac.webkit.org/changeset/145506>
All reviewed patches have been landed. Closing bug.