Bug 172104 – [WPE] Use libepoxy

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

[Zan Dobersek]

[GTK][WPE] Use libepoxy

[Zan Dobersek]

Created attachment 310114 [details]
WIP

[Zan Dobersek]

Created attachment 311580 [details]
WIP

Works for WPE, GTK+ next.

[Zan Dobersek]

Created attachment 312662 [details]
WIP patch

Also tested on the GTK+ port.

[Zan Dobersek]

Created attachment 313394 [details]
WIP

Cleaned up WPE CMake files.

[Build Bot]

Attachment 313394 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:45:  "EpoxyShims.h" already included at Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:30  [build/include] [4]
ERROR: Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:38:  "epoxy/egl.h" already included at Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:28  [build/include] [4]
ERROR: Tools/WebKitTestRunner/wpe/HeadlessViewBackend.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/wpe/HeadlessViewBackend.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/EpoxyShims.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:106:  "epoxy/gl.h" already included at Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:66  [build/include] [4]
Total errors found: 7 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Change of plans, I'd only enable libepoxy for the WPE port, for starters.  It's simpler there since only the EGL + OpenGL ES 2.0 combo is supported.  We'd approach the GTK+ port afterwards, once libepoxy is shown to work well.

[Zan Dobersek]

Created attachment 314638 [details]
WIP patch

[Build Bot]

Attachment 314638 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:44:  "EpoxyShims.h" already included at Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:30  [build/include] [4]
ERROR: Tools/WebKitTestRunner/wpe/HeadlessViewBackend.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/wpe/HeadlessViewBackend.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:103:  "epoxy/gl.h" already included at Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:64  [build/include] [4]
Total errors found: 5 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Looks like the style checker errors are valid.

[Zan Dobersek]

Created attachment 314826 [details]
WIP patch

[Zan Dobersek]

Created attachment 314967 [details]
Patch

[Build Bot]

Attachment 314967 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/wpe/HeadlessViewBackend.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/wpe/HeadlessViewBackend.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Nice!

[Michael Catanzaro]

Comment on attachment 314967 [details]
Patch

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

Nice! r=me, conditional on getting approval from Miguel as well.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:69
> +#if USE(LIBEPOXY)
> +// Include the <epoxy/gl.h> header before <gst/gl/gl.h>.
> +// Define GL_EXT_blend_func_extended since libepoxy headers don't, despite
> +// providing all the GL entrypoints and constants relevant to that extension.
> +#include <epoxy/gl.h>
> +#define GL_EXT_blend_func_extended 1
> +#endif

Did you report a bug at https://github.com/anholt/libepoxy? Our friends at Endless are maintaining libepoxy nowadays, so it should be easy to get changes upstreamed. Let me know if you need me to ping Emmanuele.

> Source/cmake/OptionsWPE.cmake:78
> +find_package(LibEpoxy REQUIRED)

Do you know if a particular minimum version is required? That would be good to research.

If 1.3.1 is new enough, then I think we could even make it a mandatory dependency for WebKitGTK+. If you need 1.4, then it will have to remain optional for a long time, because we need to support Ubuntu 16.04.

[Zan Dobersek]

Comment on attachment 314967 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:69
>> +#endif
> 
> Did you report a bug at https://github.com/anholt/libepoxy? Our friends at Endless are maintaining libepoxy nowadays, so it should be easy to get changes upstreamed. Let me know if you need me to ping Emmanuele.

This was actually a problem only with the 1.8 version of gst-plugins-bad, where these headers originate from. This isn't an issue anymore with the 1.10 version of GStreamer packages to which we upgraded recently.

The main problem here was that GSTREAMER_GL was getting enabled for WPE even with the 1.8 version, which should not happen. The GTK+ port only enables the feature if version 1.10 or newer is present.

>> Source/cmake/OptionsWPE.cmake:78
>> +find_package(LibEpoxy REQUIRED)
> 
> Do you know if a particular minimum version is required? That would be good to research.
> 
> If 1.3.1 is new enough, then I think we could even make it a mandatory dependency for WebKitGTK+. If you need 1.4, then it will have to remain optional for a long time, because we need to support Ubuntu 16.04.

1.3.1 works fine, and it might be enough for all the functionality we want from the library.

[Zan Dobersek]

Created attachment 315233 [details]
Patch for landing

[Build Bot]

Attachment 315233 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/wpe/HeadlessViewBackend.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/wpe/HeadlessViewBackend.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 315233 [details]
Patch for landing

Clearing flags on attachment: 315233

Committed r219391: <http://trac.webkit.org/changeset/219391>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Timothy Hatcher]

Comment on attachment 315233 [details]
Patch for landing

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

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:39
> +#elif USE(OPENGL_ES2)

This is a typo that broke the non USE_LIBEPOXY build. See bug 174442.