Bug 146511 - [GTK] ENABLE_OPENGL=OFF builds still include GL-specific files
Summary: [GTK] ENABLE_OPENGL=OFF builds still include GL-specific files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-01 07:05 PDT by Emanuele Aina
Modified: 2015-10-15 01:39 PDT (History)
20 users (show)

See Also:


Attachments
Patch (23.97 KB, patch)
2015-07-03 05:08 PDT, Emanuele Aina
no flags Details | Formatted Diff | Diff
Patch (5.53 KB, patch)
2015-07-03 06:48 PDT, Emanuele Aina
no flags Details | Formatted Diff | Diff
Patch (6.16 KB, patch)
2015-07-06 08:33 PDT, Emanuele Aina
no flags Details | Formatted Diff | Diff
Patch (18.81 KB, patch)
2015-07-22 00:23 PDT, Emanuele Aina
no flags Details | Formatted Diff | Diff
Patch (7.50 KB, patch)
2015-07-22 00:42 PDT, Emanuele Aina
no flags Details | Formatted Diff | Diff
Patch (18.35 KB, patch)
2015-09-17 11:18 PDT, Emanuele Aina
no flags Details | Formatted Diff | Diff
Patch (18.36 KB, patch)
2015-09-17 16:50 PDT, Emanuele Aina
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emanuele Aina 2015-07-01 07:05:29 PDT
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
Comment 1 Emanuele Aina 2015-07-03 05:08:48 PDT
Created attachment 256093 [details]
Patch
Comment 2 WebKit Commit Bot 2015-07-03 05:09:45 PDT
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
Comment 3 Emanuele Aina 2015-07-03 06:48:57 PDT
Created attachment 256100 [details]
Patch
Comment 4 Emanuele Aina 2015-07-03 06:51:06 PDT
I've reuploaded the patch as it got mixed with bug #146550 before I learnt about the -g flag of `webkit-patch upload`. :/
Comment 5 Emanuele Aina 2015-07-03 13:28:58 PDT
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.
Comment 6 Zan Dobersek 2015-07-06 00:52:42 PDT
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.
Comment 7 Emanuele Aina 2015-07-06 01:31:04 PDT
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? :)
Comment 8 Emanuele Aina 2015-07-06 02:17:33 PDT
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.
Comment 9 Martin Robinson 2015-07-06 07:18:26 PDT
Here's some context: https://lists.webkit.org/pipermail/webkit-dev/2015-April/027384.html
Comment 10 Emanuele Aina 2015-07-06 08:21:52 PDT
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.
Comment 11 Emanuele Aina 2015-07-06 08:33:42 PDT
Created attachment 256214 [details]
Patch
Comment 12 Martin Robinson 2015-07-06 08:53:54 PDT
(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
Comment 13 Emanuele Aina 2015-07-08 11:51:26 PDT
> 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. :(
Comment 14 Zan Dobersek 2015-07-21 04:28:14 PDT
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.
Comment 15 Emanuele Aina 2015-07-22 00:23:54 PDT
Created attachment 257245 [details]
Patch

Rebased on current trunk (r187148)
Comment 16 Emanuele Aina 2015-07-22 00:42:20 PDT
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.
Comment 17 Emanuele Aina 2015-07-24 01:46:40 PDT
By the way, I've opened bug 147258 with my rough plans for some sort of software-only basic composition, any comment is appreciated. :)
Comment 18 Michael Catanzaro 2015-08-04 09:51:04 PDT
Patch LGTM... this is still a supported configuration, so we should land it.
Comment 19 Carlos Garcia Campos 2015-08-04 23:49:38 PDT
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.
Comment 20 Emanuele Aina 2015-09-04 09:05:52 PDT
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.
Comment 21 Emanuele Aina 2015-09-17 11:18:09 PDT
Created attachment 261403 [details]
Patch
Comment 22 Michael Catanzaro 2015-09-17 13:25:52 PDT
Looks like the patch does not apply on master. :(
Comment 23 Emanuele Aina 2015-09-17 13:54:32 PDT
Argh, I was on r189702 and forgot to do a pull before submitting. Let me see...
Comment 24 Emanuele Aina 2015-09-17 16:50:21 PDT
Created attachment 261457 [details]
Patch
Comment 25 Emanuele Aina 2015-09-17 16:52:12 PDT
Patch updated. Not tested on current master (r189944) due to an unrelated build failure. I will try again tomorrow.
Comment 26 Emanuele Aina 2015-09-18 04:16:57 PDT
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.
Comment 27 Emanuele Aina 2015-10-06 05:17:13 PDT
I'm considering this patch final, if anybody wants to give it a look again and eventually merge it, it would be very appreciated. :)
Comment 28 WebKit Commit Bot 2015-10-06 09:21:37 PDT
Comment on attachment 261457 [details]
Patch

Clearing flags on attachment: 261457

Committed r190615: <http://trac.webkit.org/changeset/190615>
Comment 29 WebKit Commit Bot 2015-10-06 09:21:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Carlos Garcia Campos 2015-10-15 01:13:32 PDT
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...
Comment 31 Emanuele Aina 2015-10-15 01:39:53 PDT
Aargh, sorry about it! :(