Bug 146511 – [GTK] ENABLE_OPENGL=OFF builds still include GL-specific files

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Emanuele Aina]

A ENABLE_GL=OFF build of trunk@186174 fails on BitmapTextureGL.cpp, which is unguarded and included if USE_TEXTURE_MAPPER is set: https://trac.webkit.org/browser/trunk/Source/WebCore/PlatformGTK.cmake#L392

I'm a cmake noob, but it doesn't look like USE_TEXTURE_MAPPER is something that can be disabled as it is set with SET_AND_EXPOSE_TO_BUILD() in OptionsGTK.cmake, which makes it look like it's not really a flag the user can set, but rather something port-specific: https://trac.webkit.org/browser/trunk/Source/cmake/OptionsGTK.cmake#L299

[Emanuele Aina]

Created attachment 256093 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Emanuele Aina]

Created attachment 256100 [details]
Patch

[Emanuele Aina]

I've reuploaded the patch as it got mixed with bug #146550 before I learnt about the -g flag of `webkit-patch upload`. :/

[Emanuele Aina]

Disabling GL would still leave AC enables, which ultimately leads to LayerTreeHost::create() being called and returning a null pointer which causes the WebProcess to segfault: https://trac.webkit.org/browser/trunk/Source/WebKit2/WebProcess/WebPage/LayerTreeHost.cpp#L49

Would it be appropriate to add an ASSERT_NOT_REACHED() in the "#else" in LayerTreeHost.cpp above, to make the intention more clear? As far as I can tell no LayerTreeHost::create() callsite checks for null, so an assertion would make things more clear.

Relatedly, would it make sense to turn off AC by default with ENABLE_OPENGL=OFF? At the moment AC requires GL (I plan to change that, but I have not even started to work on that), so you only get the segfault above with no GL and AC turned on.

[Zan Dobersek]

The idea for the future is to fold USE_TEXTURE_MAPPER_GL into USE_TEXTURE_MAPPER, i.e. only one variable guarding the TextureMapper implementation (which only supports OpenGL backend at this point).

If you were able to disable USE_TEXTURE_MAPPER (much like USE_TEXTURE_MAPPER_GL is now disabled if there's no OpenGL support), the files you're modifying in the patch wouldn't be built at all.

I'm not sure what's the reason behind enabling USE_TEXTURE_MAPPER at all times though.

[Emanuele Aina]

I guess my (still not particularly well researched) plan for the Raspberry Pi is to implement some sort of TextureMapperCairo to try to composite non-rotated fully-opaque rectangles with Cairo to screen: my specific target is to avoid memory copies as much as possible during video playback (copying video frames to the backing buffer and the blitting it to screen severely slow things down).

This means that having USE_TEXTURE_MAPPER_GL separated from USE_TEXTURE_MAPPER is a useful feature for me, as I'm only interested in avoiding GL (which we can't rely on for the Pi) while I plan to make use of the AC infrastructure.

Does it make any sense? :)

[Emanuele Aina]

Or rather, I should probably look into reviving the TextureMapperImageBuffer, which I guess was the original reason of the USE_TEXTURE_MAPPER_GL/USE_TEXTURE_MAPPER split.

[Martin Robinson]

Here's some context: https://lists.webkit.org/pipermail/webkit-dev/2015-April/027384.html

[Emanuele Aina]

Thanks for the pointer. My plan would be to severely restrict what can be composited to non-rotated fully-opaque rectangles and avoid any kind of transformation during compositing to produce what ultimately is a multi-source memcpy, so it would avoid any problem with non-affine transformations not being available in Cairo. I guess I'm going a bit off-topic from the issue at hand, my only intent is to motivate why I would appreciate to retain the USE_TEXTURE_MAPPER_GL/USE_TEXTURE_MAPPER split at least until I had the chance to bang my head at TextureMapperImageBuffer for a while. :)

I will upload a rebased patch (the current one no longer applies) with the ASSERT_NOT_REACHED() I mentioned a few comments ago.

[Emanuele Aina]

Created attachment 256214 [details]
Patch

[Martin Robinson]

(In reply to comment #11)
> Created attachment 256214 [details]
> Patch

I'm pretty sure TextureMapperImageBuffer was removed a few months ago now: http://trac.webkit.org/changeset/183807

[Emanuele Aina]

> I'm pretty sure TextureMapperImageBuffer was removed a few months ago now: http://trac.webkit.org/changeset/183807

Yup, sorry if I haven't been clear, but my intention is to resurrect it in some form in the next months.

I can't use GL on the Raspberry Pi, but some form of software-only composition would be greatly beneficial there to avoid extra copies. In particular we had some incredibly ugly hacks for WK1 which I would like to get rid of. Those hacks made video playback work acceptably on the Raspberry Pi without any accelerated composition: since those hacks cannot apply with the split process model of WebKit2 I would like to provide a proper implementation this time to accomplish the same goal (smooth 720p <video> playback).

As far as I can tell at the moment, TextureMapperImageBuffer provided a good chunk of what I need. I would first aim at enabling it only when ENABLE_OPENGL=OFF, to avoid the issues that lead to its removal. I'm sorry I missed the request for comments when TextureMapperImageBuffer was to be removed, but I was still stuck with WK1 at that point. :(

[Zan Dobersek]

Comment on attachment 256214 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:382
>  }; // namespace WebCore
> +#endif // USE(TEXTURE_MAPPER_GL)

Could use an empty line in between.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1393
> +#if PLATFORM(X11) && (USE(REDIRECTED_XCOMPOSITE_WINDOW) || USE(TEXTURE_MAPPER_GL))

This needs rebasing, I believe.

[Emanuele Aina]

Created attachment 257245 [details]
Patch

Rebased on current trunk (r187148)

[Emanuele Aina]

Created attachment 257251 [details]
Patch

Rebased on current trunk (r187148) and added a dep from ENABLE(WAYLAND_TARGET) to ENABLE(OPENGL) due to EGL usage.

[Emanuele Aina]

By the way, I've opened bug 147258 with my rough plans for some sort of software-only basic composition, any comment is appreciated. :)

[Michael Catanzaro]

Patch LGTM... this is still a supported configuration, so we should land it.

[Carlos Garcia Campos]

Comment on attachment 257251 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:25
>  #include "config.h"
> +
> +#if USE(TEXTURE_MAPPER_GL)
>  #include "BitmapTextureGL.h"

We don't do this. The header should be guarded too, and then here include config.h and the main header unconditionally, and then we guard the rest of the file.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:24
>  #include "config.h"
> +
> +#if USE(TEXTURE_MAPPER_GL)

Same here. Also it's a bit weird that the header has #if USE(TEXTURE_MAPPER) and the implementation USE(TEXTURE_MAPPER_GL). They should have the same condition. I guess what you want is to change the header in this case.

> Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:26
> +#if USE(TEXTURE_MAPPER_GL)
> +
>  #include "TextureMapperShaderProgram.h"

Same here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1370
> +    Window native;

setNativeSurfaceHandleForCompositing expects a uint64_t, so use that instead of Window. Also, this is not an if but an #if, so you don't need to declare this variable here, you can do it when assigned below on each #if branch. And use a better name, windowID, for example.

[Emanuele Aina]

Sorry for the delay, I will pick the patch up soon but while working on #147258 I found that actually having a non-GL TextureMapper-based compositor helps a lot to figure out the  breakage due to confusion about the different guards.

I guess I will tie the loose ends while cleaning up the local patch I have for #147258.

[Emanuele Aina]

Created attachment 261403 [details]
Patch

[Michael Catanzaro]

Looks like the patch does not apply on master. :(

[Emanuele Aina]

Argh, I was on r189702 and forgot to do a pull before submitting. Let me see...

[Emanuele Aina]

Created attachment 261457 [details]
Patch

[Emanuele Aina]

Patch updated. Not tested on current master (r189944) due to an unrelated build failure. I will try again tomorrow.

[Emanuele Aina]

I've re-tested the patch on top of master and I can run MiniBrowser with no GL involved (the first version only addressed the ability to build without GL but the resulting code just hit an assertion).

For reference, the build failure I encountered yesterday was bug #149342.

[Emanuele Aina]

I'm considering this patch final, if anybody wants to give it a look again and eventually merge it, it would be very appreciated. :)

[WebKit Commit Bot]

Comment on attachment 261457 [details]
Patch

Clearing flags on attachment: 261457

Committed r190615: <http://trac.webkit.org/changeset/190615>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

Comment on attachment 261457 [details]
Patch

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

> Source/WebKit2/UIProcess/gtk/WebPreferencesGtk.cpp:46
> +#if !ENABLE(OPEN_GL)

This is wrong, it should be OPENGL. This disabled accelerated compositing, and I merged this in 2.10 branch before releasing 2.10.1 :-/ I'll make a new release...

[Emanuele Aina]

Aargh, sorry about it! :(