Bug 111859 – [qt] Webkit fails to compile if EGL headers are not in default INCLUDEPATH

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Floris Bos]

[qt] Webkit fails to compile if EGL headers are not in default INCLUDEPATH

[Floris Bos]

Created attachment 192227 [details]
Patch

[Jocelyn Turcotte]

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?

[Floris Bos]

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.

[Jocelyn Turcotte]

(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.

[Floris Bos]

Created attachment 192482 [details]
Patch

[Floris Bos]

(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.

[Jocelyn Turcotte]

Comment on attachment 192482 [details]
Patch

Great, thanks for the patch.

[Jocelyn Turcotte]

Comment on attachment 192482 [details]
Patch

Cancelling the CQ to figure if we need it also without OpenGLES

[Jocelyn Turcotte]

(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.

[Jocelyn Turcotte]

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.

[Floris Bos]

> 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.

[Floris Bos]

Created attachment 192510 [details]
patch

[Jocelyn Turcotte]

(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.

[Floris Bos]

(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?

[Jocelyn Turcotte]

(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.

[Floris Bos]

(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.

[Floris Bos]

Created attachment 192528 [details]
New patch

[Jocelyn Turcotte]

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.

[Floris Bos]

Created attachment 192533 [details]
Just patching LIBS to CONFIG

[Jocelyn Turcotte]

Comment on attachment 192533 [details]
Just patching LIBS to CONFIG

r=me, thanks for holding on

[WebKit Review Bot]

Comment on attachment 192533 [details]
Just patching LIBS to CONFIG

Clearing flags on attachment: 192533

Committed r145506: <http://trac.webkit.org/changeset/145506>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.