Bug 115803

Please disable accelerated compositing on wayland for WebKitGTK+ 2.2.0 if you can't get this done. If built with both x11 and wayland backends, apps running on wayland compositor fail with traces to accelerated compositing functions. Disabling accelerated compositing makes apps run on wayland compositors just fine, but then X apps are left without accelerated compositing.

This should be covered by r155620, and should be available for testing in the 2.1.92 release (and future ones) that is scheduled for tomorrow. http://trac.webkit.org/changeset/155620

Created attachment 222095 [details] Patch

Created attachment 222096 [details] Patch

Created attachment 222097 [details] Patch

Created attachment 222098 [details] Patch

Created attachment 222099 [details] Patch

Created attachment 222100 [details] Patch

Created attachment 222101 [details] Patch

Created attachment 222102 [details] Patch

Created attachment 222103 [details] Patch

Created attachment 222104 [details] Patch

These patches provide an initial implementation to support Accelerated Compositing under Wayland with WebKit2. This initial implementation is mostly a prototype putting together the necessary pieces for this, but further work is needed to make this ready for production environments. The design is based on feedback from Wayland developers and it involves having the UI Process implement a small nested Wayland compositor to share a Wayland surface with the WebProcess. The implementation of the nested compositor was based on the nested client from Weston. These patches do not provide support for Accelerated Compositing under Wayland with WebKit1. Implementation details and known issues: 1. Patches apply clean on top of commit c69d3281e2f4dc6be7a42ce7ff3d8634a31f5313 from December 10, we need to rebase and test the patches against current webkit and Wayland. 2. Since Wayland support requires EGL and there can be only one EGL target platform configured (this is set at build time), in case that we build for both X11 and Wayland at the same time we need to make sure that only the Wayland path goes through EGL and have the X11 paths use GLX to create the GL context. If we are only building for X11, then we can have it go through the EGL path if that was configured instead of GLX. This together with run-time detection of the windowing system to select automatically between X11 and Wayland paths -see point 7) below- makes code that creates the GL context a bit complex at the moment because there are multiple options to consider. 3. Since Wayland requires EGL and GTK with Wayland support there are guards for the Wayland code in the form #if USE(EGL) && PLATFORM(WAYLAND) && defined(GDK_WINDOWING_WAYLAND) most of the Wayland code paths. I think it would be best that PLATFORM(WAYLAND) is set only if we verify these 3 guards at configure time so we can reduce the guards to PLATFORM(WAYLAND) only or PLATFORM(WAYLAND) && PLATFORM(GTK) depending on the case. Also, as per 2), if we are trying to build for both Wayland and X11 but we lack GLX, then maybe we should disable one of the targets, we could disable either of them, but I guess Wayland would be the right one to drop for now. 4. If testing AC under Wayland with a build configured for both X11 and Wayland targets, --disable-glx must be used, otherwise there is a bug somwehere that makes GTK spit out-of-memory errors when trying to paint the widget with the results of the composition and nothing is painted on the WebView. 5. After visiting a page that enables the AC path, then leaving to another page, and then going back to the first page again using the browsers's back button shows rendering artifacts (composited layers are black). This is fixed if the page is reloaded though. 6. This is only a prototype and it only works with one WebView widget. Multiple tabs or windows are not expected to work. For this to work we will need a way to associate a widget with the wayland surface where we are doing the composition for that widget. This is more difficult in Wayland than in X11 because in Wayland with the nested compositor the shared Wayland surface is created in the WebProcess (LayerTreeHostGtk) where we have no context of the widget (which lives in the UI Process) and the nested compositor (which lives in the UIProcess) does not know for which widget it is creating the surfaces requested from the WebProcess (since the wayland protocol does not include this info). We might need to add a new message between the two processes to fix this limitation or maybe there is something more creative we can do about this. In any case, it needs some thinking. 7. Implementation is intended to support run-time detection of the windowing system (wayland or x11) and select appropriate code paths depending on the case by checking the type of the GdkDisplay. This needs more testing, but it seemed to work well for me in the following scenarios at least: - Build only for X11 then run under X11 - Build for X11 and Wayland, then run under Wayland (*) - Build for X11 and Wayland, then run under X11 (*) But this was not a complete test because as per 4) it required to build with --disable-glx. 8. All tests were done against Weston running inside an X session, so there is the possibility that this needs additional fixes to work in a pure Wayland setup where there is no X server running. I have not tested this. 9. I only used MiniBrowser in my tests so it would be nice to see if this also works with Epiphany/Web as it is or if that would need more work. 10. This does not address the creation of the offscreen context for WebGL under Wayland, Zan was working on that and I think his fix for that should just work fine with my patches. Future Work: There are a number of FIXMEs in the code for things that need more work, but I think addressing 6) and 4) is the top priority. Testing on a pure Wayland setup and also testing with Epiphany/Web are important too.

Created attachment 223102 [details] Patch (multiple surfaces) This patch adds initial support for multiple surfaces/widgets. In order to map surfaces and widgets we implement a custom extension for the wayland protocol that allows to associate a surface with a widget-id. This widget-id is generated by the compositor itself when a WebView is created and is passed to the WebProcess using the same mechanisms we use in X11 to communicate the XWindowID. Then the WebProcess (LayerTreeHostGtk), can use this widget-id when creating the wayland surface to let the compositor know about the widget/surface matching. We need to use wayland-scanner to autogenerate files required by the extension. This patch generates (and expects) the files in the Source tree instead of producing them under DerivedSources. For some reason I was not able to include the autogenerated header files if these were created under DerivedSources, so this probably needs some more work. Since MiniBrowser does not let me create multiple tabs/windows I have not tested this properly yet, but I did check that reloading in MiniBrowser (which creates a new surface for the same widget) does work as expected: the new surface is created, it is assigned to the existing widget and the previous surface is destroyed. The patch applies on top of the previous patches.

Created attachment 223442 [details] Patch (multiple surfaces) New version of the patch to support multiple surfaces. I have tested this with Epiphany as well as current wayland/weston and gtk+. It mostly works, but there are bugs that need more investigation: 1) When having multiple windows/tabs, reloading one of them can cause another one (and only one) to produce rendering artifacts. Usually the same artifacts that I described above for the case when we go back with the browser to a page that activates AC. 2) When having multiple windows/tabs, closing one of them can cause another one (and only one) to produce render artifacts. When one of the tabs/windows is affected by this bug, reloading the page fixes the problem. Another thing I observed (unrelated to this patch) is that AC only seems to work with cairo glesv2, if I use the cairo gl backend instead I see out-of-memory errors every time we try to paint the web view widget (the same errors I got when running AC on Wayladn with --enable-glx set btw).

Status update: (In reply to comment #15) > Created an attachment (id=223442) [details] > Patch (multiple surfaces) > > New version of the patch to support multiple surfaces. I have tested this with Epiphany as well as current wayland/weston and gtk+. > > It mostly works, but there are bugs that need more investigation: > > 1) When having multiple windows/tabs, reloading one of them can cause another one (and only one) to produce rendering artifacts. Usually the same artifacts that I described above for the case when we go back with the browser to a page that activates AC. > 2) When having multiple windows/tabs, closing one of them can cause another one (and only one) to produce render artifacts. These two bugs had the same origin and was fixed by this: https://bugs.webkit.org/show_bug.cgi?id=129244 > When one of the tabs/windows is affected by this bug, reloading the page fixes the problem. > > Another thing I observed (unrelated to this patch) is that AC only seems to work with cairo glesv2, if I use the cairo gl backend instead I see out-of-memory errors every time we try to paint the web view widget (the same errors I got when running AC on Wayladn with --enable-glx set btw). I updated mesa/wayland to master and rebuilt everything from scratch and it all works fine now (both gl and glesv2, both with and without enable-glx), so that's good news :)

Status update: (In reply to comment #13) (...) > Implementation details and known issues: (...) > 4. If testing AC under Wayland with a build configured for both X11 and > Wayland targets, --disable-glx must be used, otherwise there is a bug somwehere > that makes GTK spit out-of-memory errors when trying to paint the widget with the results of the composition and nothing is painted on the WebView. This happens no more. > 5. After visiting a page that enables the AC path, then leaving to another page, and then going back to the first page again using the browsers's back button shows rendering artifacts (composited layers are black). This is fixed if the page is reloaded though. This still happens. The problem is related to WebKit's page cache. Disabling the page cache fixes it. I think the problem is that when going back to the page it tries to re-use the tiles but the underlying texture objects have been destroyed. I came to this conclusion after observing that tiles were trying to use textures with a non-zero ID (m_id in BitmapTextureGL) that had been destroyed. This needs more investigation. > 6. This is only a prototype and it only works with one WebView widget. Multiple > tabs or windows are not expected to work. For this to work we will need a way > to associate a widget with the wayland surface where we are doing the composition for that widget. This is more difficult in Wayland than in X11 because in Wayland with the nested compositor the shared Wayland surface is created in the WebProcess (LayerTreeHostGtk) where we have no context of the widget (which lives in the UI Process) and the nested compositor (which lives in the UIProcess) does not know for which widget it is creating the surfaces > requested from the WebProcess (since the wayland protocol does not include this > info). We might need to add a new message between the two processes to fix > this limitation or maybe there is something more creative we can do about this. > In any case, it needs some thinking. This has already been addressed with the multiple surfaces patch attached to this bug. > 7. Implementation is intended to support run-time detection of the windowing > system (wayland or x11) and select appropriate code paths depending on the case > by checking the type of the GdkDisplay. This needs more testing, but it seemed > to work well for me in the following scenarios at least: > - Build only for X11 then run under X11 > - Build for X11 and Wayland, then run under Wayland (*) > - Build for X11 and Wayland, then run under X11 > > (*) But this was not a complete test because as per 4) it required to > build with --disable-glx. I think all the cases are working fine now. > 8. All tests were done against Weston running inside an X session, so > there is the possibility that this needs additional fixes to work in a pure > Wayland setup where there is no X server running. I have not tested this. > > 9. I only used MiniBrowser in my tests so it would be nice to see if > this also works with Epiphany/Web as it is or if that would need more work. I tested with Epiphany too since I needed it for the multiple surfaces patch and it works well. > 10. This does not address the creation of the offscreen context for WebGL > under Wayland, Zan was working on that and I think his fix for that should > just work fine with my patches. Since this will add support for the sharing context in Wayland I think it might actually solve other problems too. At least I know that bug #129244 should be fixed by this too and I wonder if the page cache issue I mention above is happening because of this as well.

(In reply to comment #16) > Status update: > > (In reply to comment #15) > > Created an attachment (id=223442) [details] [details] > > Patch (multiple surfaces) > > > > New version of the patch to support multiple surfaces. I have tested this with Epiphany as well as current wayland/weston and gtk+. > > > > It mostly works, but there are bugs that need more investigation: > > > > 1) When having multiple windows/tabs, reloading one of them can cause another one (and only one) to produce rendering artifacts. Usually the same artifacts that I described above for the case when we go back with the browser to a page that activates AC. > > 2) When having multiple windows/tabs, closing one of them can cause another one (and only one) to produce render artifacts. > > These two bugs had the same origin and was fixed by this: > https://bugs.webkit.org/show_bug.cgi?id=129244 Well, that's not 100% accurate. We also need this fixed, but the problem has already been detected and a patch should land soon enough: https://bugs.webkit.org/show_bug.cgi?id=129419 Also, merging support for the sharing contextin Wayland should fix the problem too.

(In reply to comment #17) (...) > > 5. After visiting a page that enables the AC path, then leaving to another page, and then going back to the first page again using the browsers's back button shows rendering artifacts (composited layers are black). This is fixed if the page is reloaded though. > > This still happens. The problem is related to WebKit's page cache. Disabling the page cache fixes it. I think the problem is that when going back to the page it tries to re-use the tiles but the underlying texture objects have been destroyed. I came to this conclusion after observing that tiles were trying to use textures with a non-zero ID (m_id in BitmapTextureGL) that had been destroyed. > > This needs more investigation. Indeed, the problem is that GL textures are destroyed when we leave the page, so when we try to reuse the tiles later on it fails. The reason why this works in X11 is the fact that we have a sharing context (again), since that prevents the textures from being destroyed in the end (since they are shared with a context that is not destroyed). Adding a sharing context for Wayland fixes the problem, even though I think this is kind of hiding a real bug that already existed before for X11. I'll file a separate bug for this so we can discuss if want to do something about this.

(In reply to comment #19) > I'll file a separate bug for this so we can discuss if want to do something about this. https://bugs.webkit.org/show_bug.cgi?id=129474

Any update on this? The latest release of webkit-gtk (2.9.2) still doesn't build with egl/gles2/wayland enabled. There is partial support in place, but lots of missing pieces.

*** Bug 144544 has been marked as a duplicate of this bug. ***

it still do not render sites like vimeo, youtube. i am able to play youtube videos but only sound, video part not seen. i attacked a log after playing finished. gtk-3.16.4 webkit-2.8.3 epiphany-3.16.1 mesa-10-6.0 weston-1.8.0 wayland-1.8.1 everythings stock arch-linux with latest upgrade. HW: intel ivybridge (HD 4XXX) weston is running on pure drm-backend even i deleted xwayland.so manually.

Created attachment 255744 [details] video playback ended.

It seems I'll be able to work on this! \o/ Just a quick recap of the current status: * part of attachment #222095 [details] has been merged as bug #136213, introducing WaylandEventSource * attachment #222096 [details] has been merged as bug #136220, introducing WaylandSurface and bug #136216, adding WaylandDisplay (now PlatformDisplayWayland since bug #144517) * attachment #222097 [details] is rather obsolete as it still targets the old autotools-based build system, and the merged patches already add the relevant files to cmake * the custom Wayland protocol extension in attachment #223442 [details] has already been merged as bug #136102 My rough plan is to follow what has been discussed on the mailing list: https://lists.webkit.org/pipermail/webkit-gtk/2016-February/002617.html My rough plan is as follows: * rebase the nested WaylandCompositor on top of master to export a single surface, still leaving compositing in the WebProcess * replace RedirectedXCompositeWindow with the nested WaylandCompositor even under X11 * switch to GtkGLArea to paint the pre-composited surface * export the layer buffers as Wayland surfaces from the WebProcess to the UIProcess * move the threaded compositor from the WebProcess to the UIProcess, painting the Wayland surfaces on the GtkGLArea context I will also investigate the WebKit4Wayland port, even if it uses a different approach to expose buffers across the process boundary I guess there's plenty of useful stuff there, any suggestion from Žan about what could be reused would be greatly appreciated. If anyone wants to provide help, advice or any other kind of input it would be very much welcome! :D

(In reply to comment #25) Thanks for picking this up! A couple notes: > My rough plan is to follow what has been discussed on the mailing list: > https://lists.webkit.org/pipermail/webkit-gtk/2016-February/002617.html > > My rough plan is as follows: > > * rebase the nested WaylandCompositor on top of master to export a single > surface, still leaving compositing in the WebProcess > * replace RedirectedXCompositeWindow with the nested WaylandCompositor even > under X11 I'm not sure we want to do this. We would lose support for cards that don't yet have full Wayland support and make Wayland a new dependency. Better to add a new implementation of RedirectedXComposite window and give it a new name. > * switch to GtkGLArea to paint the pre-composited surface Be sure not to raise our GTK+ dependency when you do this. You might need to have a fallback that follows the current path. > * export the layer buffers as Wayland surfaces from the WebProcess to the > UIProcess > * move the threaded compositor from the WebProcess to the UIProcess, > painting the Wayland surfaces on the GtkGLArea context Please coordinate with Gwang, who is working on preparing Coordinated Graphics for WebKitGTK+.

(In reply to comment #26) > > * replace RedirectedXCompositeWindow with the nested WaylandCompositor even > > under X11 > > I'm not sure we want to do this. We would lose support for cards that don't > yet have full Wayland support and make Wayland a new dependency. Better to > add a new implementation of RedirectedXComposite window and give it a new > name. Without Wayland we don't have any way to efficiently share buffers across the process boundary, so as far as I can tell there's no chance to move compositing in the UIProcess with such setup. We could rely on wl_shm which does not require driver support, but it may be slower than the current GLX + offscreen XComposite window + XRender path. > > * switch to GtkGLArea to paint the pre-composited surface > > Be sure not to raise our GTK+ dependency when you do this. You might need to > have a fallback that follows the current path. Ack. Under Wayland we already require GTK+ 3.12: would be fair to bump only that to 3.16 for GtkGLArea? Otherwise we either need to basically reimplement GtkGLArea or keep compositing in the WebProcess, which in turn would mean that we would end up with three different compositing mechanism (wayland/uiprocess, wayland/webprocess, redirectedxcompositewindow/webprocess).

\o/ I presume you'll be cleaning up the half-finished work (e.g. the unused Wayland protocol)? (In reply to comment #27) > Ack. Under Wayland we already require GTK+ 3.12: would be fair to bump only > that to 3.16 for GtkGLArea? Yes, I think you can safely bump deps as needed in the Wayland case.

Yup, I'm currently basing my work on Iago's compositor which uses the currently unused Wayland stuff (the protocol extension and WaylandEventSource). :)

(In reply to comment #27) > (In reply to comment #26) > > > > * replace RedirectedXCompositeWindow with the nested WaylandCompositor even > > > under X11 > > > > I'm not sure we want to do this. We would lose support for cards that don't > > yet have full Wayland support and make Wayland a new dependency. Better to > > add a new implementation of RedirectedXComposite window and give it a new > > name. > > Without Wayland we don't have any way to efficiently share buffers across > the process boundary, so as far as I can tell there's no chance to move > compositing in the UIProcess with such setup. > > We could rely on wl_shm which does not require driver support, but it may be > slower than the current GLX + offscreen XComposite window + XRender path. I'm not sure I understand what you mean. Obviously for Wayland we wouldn't use X11 redirected windows, but only for X11. In the Wayland case "RedirectedXComposite" window would be using a nested compositor. In any case, you should study GLXSurface.cpp, which I think is what CoordinatedGraphics uses for its UIProcess compositor support. I'm just saying that we shouldn't be using Wayland shm when running under X11, since that would lead to unacceptable performance regressions. In general, I think that when running under X11 we need to be using the X11 API to share surfaces. > > > > * switch to GtkGLArea to paint the pre-composited surface > > > > Be sure not to raise our GTK+ dependency when you do this. You might need to > > have a fallback that follows the current path. > > Ack. Under Wayland we already require GTK+ 3.12: would be fair to bump only > that to 3.16 for GtkGLArea? I think that's fine. Ideally, you should do your work in a way that GtkGLArea is used when it is supported, but otherwise we should fall back to an approach that doesn't use it. One way to do this would be to embed a GtkGLArea into the WebView widget and only use it when compiled against a new enough version of GTK. Maybe we can talk via IRC to work out the details.

I now have a *very* work-in-progress branch at https://github.com/em-/webkit/commits/wayland-ac. At the moment it works more or less like this: WebProcess creates a single wl_surface per widget, paints on it using GL, commits the wl_surface to the nested compositor, the UIProcess gets the committed surface, paints its contents using glEGLImageTargetTexture2DOES() to a GL fbo, copies the fbo contents using glReadPixels() to plain cairo image surface , paints the cairo image surface on GtkWdget::draw(). Not very efficient, but it seems to work.

I've got rid of the glReadPixels() call by relying on GtkGLArea. This needs the GLESv2 support that landed very recently in master and has been released in 3.21.1. For the moment it needs `export GDK_GL=gles` as I wanted to avoid the build-time dependency on fresh GTK+.

Created attachment 286288 [details] New complete patch This is a new attempt to finally fix this, hopefully in time for 2.14. This should obsolete all patches here and all other bugs blocking this one. It's a single big patch, but not that big, I want to avoid landing partial patches again that ended up leaving confusing dead code for long time. It also makes it easier to try it out. It's an initial implementation, there are many things that can be improved, but it works well enough to be used as a base implementation. It's based on all other patches proposed, doing some more refactorings and fixing several issues I found. For now it uses glReadPixels to render the texture into the web view, I have only tested it in my laptop with an intel driver and the performance is as good as the X11 backend in the tests I've done. It would be useful if someone could confirm whether this works with other drivers too. In the future, the glReadPixels approach should be the fallback when no other options are available, I tried to use gdk_cairo_draw_from_gl() but the web view contents are rendered upside down, I guess we need to query the EGL_WAYLAND_Y_INVERTED_WL and flip the Y-axis somehow or something like that, I don't know. We could also try to use cairo-gl if available when the GTK+ version is not recent enough to use gdk_cairo_draw_from_gl(). But as I said the glReadPixels fallback works good enough, so it shouldn't be a blocker for the initial implementation, unless performance is quite bad with other drivers. Things are a lot worse regarding window resize, that is very very slow, we need to figure out why and try to fix it, not sure if we can consider it a blocker though.

*** Bug 136897 has been marked as a duplicate of this bug. ***

*** Bug 136832 has been marked as a duplicate of this bug. ***

*** Bug 118944 has been marked as a duplicate of this bug. ***

*** Bug 136831 has been marked as a duplicate of this bug. ***

*** Bug 136833 has been marked as a duplicate of this bug. ***

Created attachment 286289 [details] Try to fix builds

(In reply to comment #33) > Things are a lot worse > regarding window resize, that is very very slow, we need to figure out why > and try to fix it, not sure if we can consider it a blocker though. Zan, if you could find some time to look into this, it would be much appreciated.

Ouch, sorry for the long silence, as usual. :( Carlos, have you had the chance to look at my branch? It already avoids glReadPixels() and performance have been quite good even on ARM hardware. It still needs some cleanup I finally got some allocated time to do in the next couple of weeks, but I'll check Carlos patch first and report here. Sorry for the duplicated work!

(In reply to comment #41) > Ouch, sorry for the long silence, as usual. :( No problem. > Carlos, have you had the chance to look at my branch? It already avoids > glReadPixels() and performance have been quite good even on ARM hardware. Indeed, I used all previous work for this. Your branch avoids glReadPixels by using gdk_cairo_draw_from_egl_image() which I have no idea where it comes from :-) > It still needs some cleanup I finally got some allocated time to do in the > next couple of weeks, but I'll check Carlos patch first and report here. Cool, thanks! > Sorry for the duplicated work! No problem, hopefully we can get something finally working for 2.14.

> Indeed, I used all previous work for this. Your branch avoids glReadPixels > by using gdk_cairo_draw_from_egl_image() which I have no idea where it comes > from :-) I just submitted it to GTK+ bugtracker[1] but I should have pointed you to the branch using GtkGLArea[2] which is slightly less efficient but more flexible, specially if we want to move compositing in the UIProcess sooner or later. [1] https://bugzilla.gnome.org/show_bug.cgi?id=769739 [2] https://github.com/em-/webkit/tree/wayland-ac-glarea > No problem, hopefully we can get something finally working for 2.14. Yup!

Comment on attachment 286289 [details] Try to fix builds Attachment 286289 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1886063 New failing tests: fast/scrolling/ios/scrollTo-at-page-load.html

Created attachment 286292 [details] Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5

(In reply to comment #43) > > Indeed, I used all previous work for this. Your branch avoids glReadPixels > > by using gdk_cairo_draw_from_egl_image() which I have no idea where it comes > > from :-) > > I just submitted it to GTK+ bugtracker[1] but I should have pointed you to > the branch using GtkGLArea[2] which is slightly less efficient but more > flexible, specially if we want to move compositing in the UIProcess sooner > or later. Let's go step by step, we have just switched to use the threaded compositor and compositing will happen in the web process for now. The goal now is to make wayland also take advantage of the threaded compositor, we'll see if and when we move the compositing to the UI process.

(In reply to comment #44) > Comment on attachment 286289 [details] > Try to fix builds > > Attachment 286289 [details] did not pass ios-sim-ews (ios-simulator-wk2): > Output: http://webkit-queues.webkit.org/results/1886063 > > New failing tests: > fast/scrolling/ios/scrollTo-at-page-load.html I'm pretty sure this is not caused by this patch.

> Let's go step by step, we have just switched to use the threaded compositor > and compositing will happen in the web process for now. The goal now is to > make wayland also take advantage of the threaded compositor, we'll see if > and when we move the compositing to the UI process. Definitely, and I've not even started to think about compositing in UIProcess. I mentioned it because the _draw_from_egl() thingy would not be useful in that case as it assumes that the page is a single big EGLImage, while we'd need to switch to GtkGLArea (or something like that) for compositing in UIProcess because we would end up with several EGLImages for each page. Please consider _draw_from_egl() as a sort of premature optimization that can be safely ignored and give a look at the wayland-ac-glarea branch. :)

(In reply to comment #48) > > Let's go step by step, we have just switched to use the threaded compositor > > and compositing will happen in the web process for now. The goal now is to > > make wayland also take advantage of the threaded compositor, we'll see if > > and when we move the compositing to the UI process. > > Definitely, and I've not even started to think about compositing in > UIProcess. > > I mentioned it because the _draw_from_egl() thingy would not be useful in > that case as it assumes that the page is a single big EGLImage, while we'd > need to switch to GtkGLArea (or something like that) for compositing in > UIProcess because we would end up with several EGLImages for each page. > > Please consider _draw_from_egl() as a sort of premature optimization that > can be safely ignored and give a look at the wayland-ac-glarea branch. :) Sure, I'll take a look. If the wayland approach ends up being more efficient than the redirected XComposite window, we could use it for X11 too if compiled with wayland support. The major problem right now is the poor performance when resizing the window.

Comment on attachment 286289 [details] Try to fix builds View in context: https://bugs.webkit.org/attachment.cgi?id=286289&action=review It seems eglCreateImage() and eglDestroyImage() are only available on EGL 1.5. Which is relatively new. It gives me linking errors with clang, builds fine with GCC. It ends crashing however for me, but it could because i'm running weston over X11 rather than directly, or it could be related with my not very updated graphics drivers. I wonder if instead of calling this two functions directly, it would be an idea to use eglCreateImageKHR/eglDestroyImageKHR via eglGetProcAddress() and abort at runtime in WaylandCompositor::initializeEGL() if they are not defined? > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:114 > +EGLImageKHR WaylandCompositor::Buffer::createImage() const > +{ > + return static_cast<EGLImageKHR*>(eglCreateImage(PlatformDisplay::sharedDisplay().eglDisplay(), EGL_NO_CONTEXT, EGL_WAYLAND_BUFFER_WL, m_resource, nullptr)); eglCreateImage() > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:206 > +bool WaylandCompositor::Surface::commit() > +{ > + EGLDisplay eglDisplay = PlatformDisplay::sharedDisplay().eglDisplay(); > + if (m_image != EGL_NO_IMAGE_KHR) > + eglDestroyImage(eglDisplay, m_image); eglDestroyImage()

(In reply to comment #50) > Comment on attachment 286289 [details] > Try to fix builds > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286289&action=review > > It seems eglCreateImage() and eglDestroyImage() are only available on EGL > 1.5. Which is relatively new. It gives me linking errors with clang, builds > fine with GCC. It ends crashing however for me, but it could because i'm > running weston over X11 rather than directly, or it could be related with my > not very updated graphics drivers. > I wonder if instead of calling this two functions directly, it would be an > idea to use eglCreateImageKHR/eglDestroyImageKHR via eglGetProcAddress() and > abort at runtime in WaylandCompositor::initializeEGL() if they are not > defined? The typical approach in OpenGL is to see that they are defined and to check that the extension is support via the OpenGL APIs. If either of those fail you should abort.

(In reply to comment #50) > Comment on attachment 286289 [details] > Try to fix builds > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286289&action=review > > It seems eglCreateImage() and eglDestroyImage() are only available on EGL > 1.5. Which is relatively new. It gives me linking errors with clang, builds > fine with GCC. Ah, right, I forgot about that. Zan's patch in bug #136897 got eglCreateImage/destroyImage via eglGetProcAddress("eglCreateImageKHR"), but I got compile errors due to redefined stuff, and I removed that with the idea of checking it again once I had everything working, but I forgot about it in the end. > It ends crashing however for me, What the patch does is that if anything required is not available, the nested compositor stops and AC is disabled. In this case we are not doing it because I forgot about eglCreateImage :-P > but it could because i'm > running weston over X11 rather than directly, No, I've tried both with weston on X11, and with a real wayland session, but most of the time I worked with weston, so that shouldn't be a problem. > or it could be related with my > not very updated graphics drivers. More likely. > I wonder if instead of calling this two functions directly, it would be an > idea to use eglCreateImageKHR/eglDestroyImageKHR via eglGetProcAddress() and > abort at runtime in WaylandCompositor::initializeEGL() if they are not > defined? Exactly, yes, but instead of aborting, we just stop the nested compositor, and AC is disabled because WaylandCompositor::singleton().isRunning() will return false. > > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:114 > > +EGLImageKHR WaylandCompositor::Buffer::createImage() const > > +{ > > + return static_cast<EGLImageKHR*>(eglCreateImage(PlatformDisplay::sharedDisplay().eglDisplay(), EGL_NO_CONTEXT, EGL_WAYLAND_BUFFER_WL, m_resource, nullptr)); > > eglCreateImage() what? > > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:206 > > +bool WaylandCompositor::Surface::commit() > > +{ > > + EGLDisplay eglDisplay = PlatformDisplay::sharedDisplay().eglDisplay(); > > + if (m_image != EGL_NO_IMAGE_KHR) > > + eglDestroyImage(eglDisplay, m_image); > > eglDestroyImage() what?

Created attachment 286588 [details] Updated patch Use eglGetProcAddress to get eglCreate/DestroyImage

(In reply to comment #49) > (In reply to comment #48) > > > Let's go step by step, we have just switched to use the threaded compositor > > > and compositing will happen in the web process for now. The goal now is to > > > make wayland also take advantage of the threaded compositor, we'll see if > > > and when we move the compositing to the UI process. > > > > Definitely, and I've not even started to think about compositing in > > UIProcess. > > > > I mentioned it because the _draw_from_egl() thingy would not be useful in > > that case as it assumes that the page is a single big EGLImage, while we'd > > need to switch to GtkGLArea (or something like that) for compositing in > > UIProcess because we would end up with several EGLImages for each page. > > > > Please consider _draw_from_egl() as a sort of premature optimization that > > can be safely ignored and give a look at the wayland-ac-glarea branch. :) > > Sure, I'll take a look. If the wayland approach ends up being more efficient > than the redirected XComposite window, we could use it for X11 too if > compiled with wayland support. The major problem right now is the poor > performance when resizing the window. I've taken a look at the new branch. I don't know much about OpenGL, but it seems that you are using a shader program to flip the Y axis. I wonder if we really need to use a GtkGLArea for that, wouldn't it be possible to use a shader program with the offscreen context to flip the texture and then simply call gdk_cairo_draw_from_gl? I also wonder if we could simplify the shader program creation by using TextureMapperShaderProgram.

Created attachment 286848 [details] Another update Add a way to check the EGL version and only try to get proc address of eglCreateImage/DestroyImage if EGL version is at least 1.5 or if EGL_KHR_image_base extension is present. This way we also return early if there's a dummy implementation of eglCreateImage/DestroyImage like mesa does for the software rasterizer, because in that case nothing will work in AC mode.

Created attachment 286959 [details] Fix poor performance when resizing the window I finally figured out why it was so slow when resizing the window. The main problem is that the resize operation is expected to be synchronous, and it needs to be synchronous in X11, because every resize creates a new pixmap that the UI process need to use for rendering. So, when a web view is resized, the drawing area notfies the web process and waits synchronously until the web process has processed the resize to update the pixmap and continue rendering. In a multitab app this happens after a window resize for every tab open. In the case of wayland, any SwapBuffer operation that happens during the resize while the UI process is waiting for the response is not actually processed, because the nested compositor runs in the UI process that is blocked. That wait is using a timeout of 0.5 seconds, which means that when all the waits finish because of the timeout, the nested compositor processes all pending buffer updates and continues rendering. In practice this meant that every window resize was blocking the Ui process for 0.5 secs * number of tabs open. In the case of wayland, we don't really need to wait until the web process responds to continue rendering, because we always have valid buffers until a new one is committed, and that happens automatically during the resize, even before the web process responds, on the first SwapBuffers operation after the resize. Another problem was that sometimes the SwapBuffers operation blocked for a while because of frame throttling, which also causes the UI process to block on the nested compositor. There's no reason to do any frame sync in the web process when not rendering to the actual screen, because it will happen later anyway, the UI process schedules a redraw after every update, and the parent compositor will take care of syncing to vblank. So, I've also added GLContext::swapInterval() to disable frame sync when rendering offscreen from the web process. With both changes (disable frame sync and not blocking the UI process while waiting for a resize reply) the resize performance is quite good, even better than the X11 one, so we should really consider switching to use the nested wayland compositor on X11 too at some point.

Comment on attachment 286959 [details] Fix poor performance when resizing the window I have tested this. Now it don't segfaults for me when weston don't has proper EGL support (like when running weston over x11 with the mesa software render or with the nvidia propietary drivers). Instead it says that it could not create the nested compositor and launches the minibrowser without AC. Which is good enough given the constrains. Please consider this patch that allows it to build with wayland 1.6: diff --git a/Source/WebCore/platform/graphics/wayland/WlUniquePtr.h b/Source/WebCore/platform/graphics/wayland/WlUniquePtr.h index 6ec3005..3943d7a 100644 --- a/Source/WebCore/platform/graphics/wayland/WlUniquePtr.h +++ b/Source/WebCore/platform/graphics/wayland/WlUniquePtr.h @@ -27,7 +27,7 @@ #if PLATFORM(WAYLAND) #include <wayland-client-protocol.h> -#include <wayland-server-core.h> +#include <wayland-server.h> namespace WebCore { diff --git a/Source/WebKit2/WebProcess/WebPage/gtk/AcceleratedSurfaceWayland.h b/Source/WebKit2/WebProcess/WebPage/gtk/AcceleratedSurfaceWayland.h index aec581f..d250056 100644 --- a/Source/WebKit2/WebProcess/WebPage/gtk/AcceleratedSurfaceWayland.h +++ b/Source/WebKit2/WebProcess/WebPage/gtk/AcceleratedSurfaceWayland.h @@ -30,7 +30,7 @@ #include "AcceleratedSurface.h" #include "WebPage.h" #include <WebCore/WlUniquePtr.h> -#include <wayland-egl-core.h> +#include <wayland-egl.h> namespace WebKit {

Comment on attachment 286959 [details] Fix poor performance when resizing the window Attachment 286959 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1940715 New failing tests: fast/viewport/ios/width-is-device-width-overflowing.html

(In reply to comment #57) > Comment on attachment 286959 [details] > Fix poor performance when resizing the window > > I have tested this. Now it don't segfaults for me when weston don't has > proper EGL support (like when running weston over x11 with the mesa software > render or with the nvidia propietary drivers). Instead it says that it could > not create the nested compositor and launches the minibrowser without AC. > Which is good enough given the constrains. Yes, we can't do more, if we don't have whatever is required to run AC, we can only disable it. > Please consider this patch that allows it to build with wayland 1.6: Sure, I'll update the patch, thanks!

Created attachment 287085 [details] Updated patch to fix the build with wayland 1.6

Tested the patch on a Radeon HD 6000 series, using Mesa drivers (Ubuntu 16.04): * Mesa 11.2, DRM 2.43.0, LLVM 3.8.0 * Weston/Wayland 1.9.0 The window resizing performance problem is not happening with the patch, both running Weston on top of X and on VT. I tested with jellyfish webgl demo and poster circle, and things look ok.

Comment on attachment 286959 [details] Fix poor performance when resizing the window View in context: https://bugs.webkit.org/attachment.cgi?id=286959&action=review Thanks for working on this. > Source/WebCore/platform/graphics/GLContext.cpp:126 > #if PLATFORM(WAYLAND) && USE(EGL) I know you didn't touch this line, but is it really possible to have PLATFORM(WAYLAND) without USE(EGL)? > Source/WebCore/platform/graphics/GLContext.cpp:169 > +#if PLATFORM(WAYLAND) > + if (display.type() == PlatformDisplay::Type::Wayland) { > + if (auto eglContext = GLContextEGL::createSharingContext(display)) > + return WTFMove(eglContext); > + return nullptr; > + } > +#endif It's not the simplest way to write this code. You can delete this entire block, if... > Source/WebCore/platform/graphics/GLContext.cpp:171 > + if (auto glxContext = GLContextGLX::createSharingContext(display)) ...if you simply add a check here for display.type() == PlatformDisplay::Type::X11, and... > Source/WebCore/platform/graphics/GLContext.cpp:174 > +#if USE(EGL) ...and if you change this to #if USE(EGL) || PLATFORM(WAYLAND). > Source/WebCore/platform/graphics/GLContext.h:45 > + WTF_MAKE_NONCOPYABLE(GLContext); WTF_MAKE_FAST_ALLOCATED; I would check with Geoff or Zan before making everything fast allocated. I suspect it's only supposed to be used for types that are frequently-allocated. > Source/WebCore/platform/graphics/PlatformDisplay.cpp:113 > + return s_sharedDisplayForCompositing ? *s_sharedDisplayForCompositing : sharedDisplay(); Why is it right to return sharedDisplay() here if setSharedDisplayForCompositing has never been called? > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:95 > + case GLContextEGL::Surfaceless: Would it be wise to use a default case with ASSERT_NOT_REACHED() here? It's a bit concerning to see a case added to the switch as if it had been forgotten.... > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:142 > - return std::make_unique<GLContextEGL>(context, surface, PbufferSurface); > + return std::unique_ptr<GLContextEGL>(new GLContextEGL(platformDisplay, context, surface, PbufferSurface)); Can't you use std::make_unique here? Is there a visibility issue? > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:152 > + if (!strstr(extensions, "EGL_KHR_surfaceless_context") && !strstr(extensions, "EGL_KHR_surfaceless_opengl")) It's OK to proceed if one or the other extension is present? They are not both necessary? > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:163 > + return std::unique_ptr<GLContextEGL>(new GLContextEGL(platformDisplay, context, EGL_NO_SURFACE, Surfaceless)); Ditto (std::make_unique?) > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:197 > + return std::unique_ptr<GLContextEGL>(new GLContextEGL(platformDisplay, context, surface, WTFMove(pixmap))); Ditto > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:227 > + return std::unique_ptr<GLContextEGL>(new GLContextEGL(platformDisplay, context, surface, WTFMove(wlSurface), window)); Ditto (std::make_unique?) > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:277 > +#if PLATFORM(X11) > + if (!context) { > + if (platformDisplay.type() == PlatformDisplay::Type::X11) > + context = createPixmapContext(platformDisplay); > +#endif > +#if PLATFORM(WAYLAND) > + if (platformDisplay.type() == PlatformDisplay::Type::Wayland) > + context = createWaylandContext(platformDisplay); > #endif > + } Looks like you have the PLATFORM(X11) guard misplaced here. They'll be a stray } if !PLATFORM(X11). And it'd be really confusing to have the PLATFORM(WAYLAND) case nested inside a runtime conditional that might not be compiled. > Source/WebCore/platform/graphics/egl/GLContextEGL.h:47 > + bool makeContextCurrent() override; > + void swapBuffers() override; I do prefer to use override here, but the new style seems to be to use final in an final class.... (override seems better to me since its meaning is very clear.) > Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:36 > +typedef int (*PFNGLXSWAPINTERVALSGIPROC) (int); Impressive. > Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:73 > + return std::unique_ptr<GLContextGLX>(new GLContextGLX(platformDisplay, WTFMove(context), window)); std::make_unique? > Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:105 > + return std::unique_ptr<GLContextGLX>(new GLContextGLX(platformDisplay, WTFMove(context), WTFMove(pbuffer))); std::make_unique? > Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:136 > + return std::unique_ptr<GLContextGLX>(new GLContextGLX(platformDisplay, WTFMove(context), WTFMove(pixmap), WTFMove(glxPixmap))); std::make_unique? > Source/WebCore/platform/graphics/wayland/WlUniquePtr.h:26 > +#pragma once Which reminds me, we're supposed to change the files that we edit to use #pragma once. > Source/WebCore/platform/graphics/wayland/WlUniquePtr.h:55 > + if (ptr) \ Let me be really really clear about this. It is guaranteed by the C++ standard that this deleter function will never be called if ptr is null. You don't need the check here. > Source/WebKit2/ChangeLog:37 > + feature requirted by X11 or Wayland implementations are not available. required > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:45 > +Ref<ThreadedCompositor> ThreadedCompositor::create(Client* client, uint64_t nativeSurfaceHandle, ShouldDoFrameSync doFrameSync) Thank you for not using a bool here! > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:216 > + // the viewport parameters incorrect, means that the content will be misplaced. Thus No comma > Source/WebKit2/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:50 > + return std::unique_ptr<AcceleratedBackingStoreWayland>(new AcceleratedBackingStoreWayland(webPage)); I guess you know by now what I'm going to ask here! > Source/WebKit2/UIProcess/gtk/AcceleratedBackingStoreX11.cpp:108 > + return std::unique_ptr<AcceleratedBackingStoreX11>(new AcceleratedBackingStoreX11(webPage)); . > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:80 > +static PFNEGLBINDWAYLANDDISPLAYWL eglBindWaylandDisplay; > +static PFNEGLUNBINDWAYLANDDISPLAYWL eglUnbindWaylandDisplay; > +static PFNEGLQUERYWAYLANDBUFFERWL eglQueryWaylandBuffer; > +static PFNEGLCREATEIMAGEKHRPROC eglCreateImage; > +static PFNEGLDESTROYIMAGEKHRPROC eglDestroyImage; > +static PFNGLEGLIMAGETARGETTEXTURE2DOESPROC eglImageTargetTexture2D; I'm afraid I have just lost some of my faith in humanity. > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:116 > + WaylandCompositor::Buffer* buffer; > + buffer = wl_container_of(listener, buffer, m_destroyListener); > + delete buffer; The assignment is unnecessary: delete wl_container_of(listener, buffer, m_destroyListener); > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:305 > + auto* surface = static_cast<WaylandCompositor::Surface*>(wl_resource_get_user_data(resource)); > + delete surface; Again, you can get rid of this assignment and do the deletion in one line > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:384 > + [](GSource *source, int *timeout) -> gboolean { Do you really have to use the trailing return type for this to compile? > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:392 > + [](GSource* source, GSourceFunc callback, gpointer userData) -> gboolean { Ditto > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:406 > + nullptr, // finalize > + nullptr, // closure_callback > + nullptr, // closure_marshall So here you use nullptrs for callbacks you don't want to implement. But up above in this file you have lots of empty lambda functions. Be consistent with one style or the other. > Source/WebKit2/WebProcess/WebPage/gtk/AcceleratedSurface.h:42 > + virtual uint64_t window() const { ASSERT_NOT_REACHED(); return 0; } > + virtual uint64_t surfaceID() const { ASSERT_NOT_REACHED(); return 0; }; These should be pure virtual. If that's not possible for some reason, I'd like to know why. > Source/WebKit2/WebProcess/WebPage/gtk/AcceleratedSurfaceWayland.cpp:90 > + return waylandCompositorDisplay() ? std::unique_ptr<AcceleratedSurfaceWayland>(new AcceleratedSurfaceWayland(webPage)) : nullptr; . > Source/WebKit2/WebProcess/WebPage/gtk/AcceleratedSurfaceX11.cpp:46 > + return std::unique_ptr<AcceleratedSurfaceX11>(new AcceleratedSurfaceX11(webPage)); .

(In reply to comment #62) > > Source/WebCore/platform/graphics/GLContext.h:45 > > + WTF_MAKE_NONCOPYABLE(GLContext); WTF_MAKE_FAST_ALLOCATED; > > I would check with Geoff or Zan before making everything fast allocated. I > suspect it's only supposed to be used for types that are > frequently-allocated. > It shouldn't hurt. The downside is there's some extra code generated for the class-specific allocation/deallocation operators, the upside is you don't use the system allocator. The size of the allocation is also relevant, the larger the structure the heavier the effect on the system allocator. GLContext and the derived classes shouldn't be too large though. > > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:406 > > + nullptr, // finalize > > + nullptr, // closure_callback > > + nullptr, // closure_marshall > > So here you use nullptrs for callbacks you don't want to implement. But up > above in this file you have lots of empty lambda functions. Be consistent > with one style or the other. > Depends on the API. GSourceFuncs allows for entries to be null, Wayland callback structures might not.

Comment on attachment 287085 [details] Updated patch to fix the build with wayland 1.6 View in context: https://bugs.webkit.org/attachment.cgi?id=287085&action=review > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:42 > + [](void* data, struct wl_registry*, uint32_t name, const char* interface, uint32_t) { > + static_cast<PlatformDisplayWayland*>(data)->registryGlobal(interface, name); > + }, IMO the curly braces placement should mirror the free-floating function definitions, so the braces would be in their own lines ... > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:46 > + [](void*, struct wl_registry*, uint32_t) > + { > + } ... except in such cases, where the braces should be placed on the same line as the parameter list, with a space in between: [](void*, struct wl_registry*, uint32_t) { } > Source/WebCore/platform/graphics/wayland/WlUniquePtr.h:37 > +template<typename T> > +struct WlPtrDeleter { > + void operator()(T* ptr) const { ASSERT_NOT_REACHED(); } > +}; Tested it out on a simplified test case, and apparently deleting this operator would throw a compile-time error if T didn't have the related WlPtrDeleter specialization available. That's probably preferable to catching missing specializations in a debug build. > Source/WebKit2/UIProcess/gtk/AcceleratedBackingStore.cpp:65 > +AcceleratedBackingStore::~AcceleratedBackingStore() > +{ > +} = default; > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:247 > + [](struct wl_client*, struct wl_resource* resource) { > + wl_resource_destroy(resource); > + }, Ditto re: style. > Source/WebKit2/WebProcess/WebPage/gtk/AcceleratedSurface.cpp:65 > +AcceleratedSurface::~AcceleratedSurface() > +{ > +} = default;

(In reply to comment #62) > Comment on attachment 286959 [details] > Fix poor performance when resizing the window > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286959&action=review > > Thanks for working on this. > > > Source/WebCore/platform/graphics/GLContext.cpp:126 > > #if PLATFORM(WAYLAND) && USE(EGL) > > I know you didn't touch this line, but is it really possible to have > PLATFORM(WAYLAND) without USE(EGL)? Indeed, I changed that in the patch too, the configure fails if wayland target is enabled but EGL disabled. > > Source/WebCore/platform/graphics/GLContext.cpp:169 > > +#if PLATFORM(WAYLAND) > > + if (display.type() == PlatformDisplay::Type::Wayland) { > > + if (auto eglContext = GLContextEGL::createSharingContext(display)) > > + return WTFMove(eglContext); > > + return nullptr; > > + } > > +#endif > > It's not the simplest way to write this code. You can delete this entire > block, if... > > > Source/WebCore/platform/graphics/GLContext.cpp:171 > > + if (auto glxContext = GLContextGLX::createSharingContext(display)) > > ...if you simply add a check here for display.type() == > PlatformDisplay::Type::X11, and... > > > Source/WebCore/platform/graphics/GLContext.cpp:174 > > +#if USE(EGL) > > ...and if you change this to #if USE(EGL) || PLATFORM(WAYLAND). I see, it makes sense, yes. > > Source/WebCore/platform/graphics/GLContext.h:45 > > + WTF_MAKE_NONCOPYABLE(GLContext); WTF_MAKE_FAST_ALLOCATED; > > I would check with Geoff or Zan before making everything fast allocated. I > suspect it's only supposed to be used for types that are > frequently-allocated. > > > Source/WebCore/platform/graphics/PlatformDisplay.cpp:113 > > + return s_sharedDisplayForCompositing ? *s_sharedDisplayForCompositing : sharedDisplay(); > > Why is it right to return sharedDisplay() here if > setSharedDisplayForCompositing has never been called? Because if not explicitly set, the shared display is the one to be used for compositing too. That allows us to return a reference to avoid a null check and also to use the same code even for ports or systems (X11) that don't use a different display for compositing. > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:95 > > + case GLContextEGL::Surfaceless: > > Would it be wise to use a default case with ASSERT_NOT_REACHED() here? It's > a bit concerning to see a case added to the switch as if it had been > forgotten.... This case is new in this patch. > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:142 > > - return std::make_unique<GLContextEGL>(context, surface, PbufferSurface); > > + return std::unique_ptr<GLContextEGL>(new GLContextEGL(platformDisplay, context, surface, PbufferSurface)); > > Can't you use std::make_unique here? > > Is there a visibility issue? GLContext has factory methods, creating instances without using the factory methods could be very problematic, so I prefer to make the constructors all private. > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:152 > > + if (!strstr(extensions, "EGL_KHR_surfaceless_context") && !strstr(extensions, "EGL_KHR_surfaceless_opengl")) > > It's OK to proceed if one or the other extension is present? They are not > both necessary? There's && so it will only proceed if both are present. Note this is not strcmp that returns 0 in case of success, it's strstr that returns nullptr in case of failure. > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:163 > > + return std::unique_ptr<GLContextEGL>(new GLContextEGL(platformDisplay, context, EGL_NO_SURFACE, Surfaceless)); > > Ditto (std::make_unique?) > > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:197 > > + return std::unique_ptr<GLContextEGL>(new GLContextEGL(platformDisplay, context, surface, WTFMove(pixmap))); > > Ditto > > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:227 > > + return std::unique_ptr<GLContextEGL>(new GLContextEGL(platformDisplay, context, surface, WTFMove(wlSurface), window)); > > Ditto (std::make_unique?) > > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:277 > > +#if PLATFORM(X11) > > + if (!context) { > > + if (platformDisplay.type() == PlatformDisplay::Type::X11) > > + context = createPixmapContext(platformDisplay); > > +#endif > > +#if PLATFORM(WAYLAND) > > + if (platformDisplay.type() == PlatformDisplay::Type::Wayland) > > + context = createWaylandContext(platformDisplay); > > #endif > > + } > > Looks like you have the PLATFORM(X11) guard misplaced here. They'll be a > stray } if !PLATFORM(X11). And it'd be really confusing to have the > PLATFORM(WAYLAND) case nested inside a runtime conditional that might not be > compiled. Good catch! > > Source/WebCore/platform/graphics/egl/GLContextEGL.h:47 > > + bool makeContextCurrent() override; > > + void swapBuffers() override; > > I do prefer to use override here, but the new style seems to be to use final > in an final class.... Unless the class is already marked as final which is the case here. > (override seems better to me since its meaning is very clear.) > > > Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:36 > > +typedef int (*PFNGLXSWAPINTERVALSGIPROC) (int); > > Impressive. > > > Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:73 > > + return std::unique_ptr<GLContextGLX>(new GLContextGLX(platformDisplay, WTFMove(context), window)); > > std::make_unique? > > > Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:105 > > + return std::unique_ptr<GLContextGLX>(new GLContextGLX(platformDisplay, WTFMove(context), WTFMove(pbuffer))); > > std::make_unique? > > > Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:136 > > + return std::unique_ptr<GLContextGLX>(new GLContextGLX(platformDisplay, WTFMove(context), WTFMove(pixmap), WTFMove(glxPixmap))); > > std::make_unique? > > > Source/WebCore/platform/graphics/wayland/WlUniquePtr.h:26 > > +#pragma once > > Which reminds me, we're supposed to change the files that we edit to use > #pragma once. > > > Source/WebCore/platform/graphics/wayland/WlUniquePtr.h:55 > > + if (ptr) \ > > Let me be really really clear about this. It is guaranteed by the C++ > standard that this deleter function will never be called if ptr is null. You > don't need the check here. Good point, I'm pretty sure we have more places to fix this. > > Source/WebKit2/ChangeLog:37 > > + feature requirted by X11 or Wayland implementations are not available. > > required > > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:45 > > +Ref<ThreadedCompositor> ThreadedCompositor::create(Client* client, uint64_t nativeSurfaceHandle, ShouldDoFrameSync doFrameSync) > > Thank you for not using a bool here! > > > Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:216 > > + // the viewport parameters incorrect, means that the content will be misplaced. Thus > > No comma > > > Source/WebKit2/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:50 > > + return std::unique_ptr<AcceleratedBackingStoreWayland>(new AcceleratedBackingStoreWayland(webPage)); > > I guess you know by now what I'm going to ask here! > > > Source/WebKit2/UIProcess/gtk/AcceleratedBackingStoreX11.cpp:108 > > + return std::unique_ptr<AcceleratedBackingStoreX11>(new AcceleratedBackingStoreX11(webPage)); > > . > > > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:80 > > +static PFNEGLBINDWAYLANDDISPLAYWL eglBindWaylandDisplay; > > +static PFNEGLUNBINDWAYLANDDISPLAYWL eglUnbindWaylandDisplay; > > +static PFNEGLQUERYWAYLANDBUFFERWL eglQueryWaylandBuffer; > > +static PFNEGLCREATEIMAGEKHRPROC eglCreateImage; > > +static PFNEGLDESTROYIMAGEKHRPROC eglDestroyImage; > > +static PFNGLEGLIMAGETARGETTEXTURE2DOESPROC eglImageTargetTexture2D; > > I'm afraid I have just lost some of my faith in humanity. > > > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:116 > > + WaylandCompositor::Buffer* buffer; > > + buffer = wl_container_of(listener, buffer, m_destroyListener); > > + delete buffer; > > The assignment is unnecessary: > > delete wl_container_of(listener, buffer, m_destroyListener); I'm not sure. Note that wl_container_of is a macro, not a function, I'll try, but I wanted to be use that delete was called with a WaylandCompositor::Buffer*. > > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:305 > > + auto* surface = static_cast<WaylandCompositor::Surface*>(wl_resource_get_user_data(resource)); > > + delete surface; > > Again, you can get rid of this assignment and do the deletion in one line I'll check if that's actually possible. > > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:384 > > + [](GSource *source, int *timeout) -> gboolean { > > Do you really have to use the trailing return type for this to compile? Not to compile, I think, but since we return a number I prefer to tell the compiler this is supposed to return a gboolean instead of assuming it will guess right. > > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:392 > > + [](GSource* source, GSourceFunc callback, gpointer userData) -> gboolean { > > Ditto > > > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:406 > > + nullptr, // finalize > > + nullptr, // closure_callback > > + nullptr, // closure_marshall > > So here you use nullptrs for callbacks you don't want to implement. But up > above in this file you have lots of empty lambda functions. Be consistent > with one style or the other. I know I can do this for GSourceFuncs, but I have no idea about wayland, so I left what the existing code was doing. > > Source/WebKit2/WebProcess/WebPage/gtk/AcceleratedSurface.h:42 > > + virtual uint64_t window() const { ASSERT_NOT_REACHED(); return 0; } > > + virtual uint64_t surfaceID() const { ASSERT_NOT_REACHED(); return 0; }; > > These should be pure virtual. > > If that's not possible for some reason, I'd like to know why. You can build without wayland support and with X11 but without redirected window support. In that case we wouldn't have an implementation of this class, and it won't build if those methods are pure virtual. However, we need to ensure that the factory method doesn't create an instance in that case. > > Source/WebKit2/WebProcess/WebPage/gtk/AcceleratedSurfaceWayland.cpp:90 > > + return waylandCompositorDisplay() ? std::unique_ptr<AcceleratedSurfaceWayland>(new AcceleratedSurfaceWayland(webPage)) : nullptr; > > . > > > Source/WebKit2/WebProcess/WebPage/gtk/AcceleratedSurfaceX11.cpp:46 > > + return std::unique_ptr<AcceleratedSurfaceX11>(new AcceleratedSurfaceX11(webPage)); > > . Same reasoning, we have factory methods, not just create methods that call make_unique, I don't want to allow creating instances using the public constructors, the factory methods should always be used.

(In reply to comment #64) > Comment on attachment 287085 [details] > Updated patch to fix the build with wayland 1.6 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=287085&action=review > > > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:42 > > + [](void* data, struct wl_registry*, uint32_t name, const char* interface, uint32_t) { > > + static_cast<PlatformDisplayWayland*>(data)->registryGlobal(interface, name); > > + }, > > IMO the curly braces placement should mirror the free-floating function > definitions, so the braces would be in their own lines ... I doubted what to do, and checked other similar cases. But I don't have a preference, so I'll just change it. > > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:46 > > + [](void*, struct wl_registry*, uint32_t) > > + { > > + } > > ... except in such cases, where the braces should be placed on the same line > as the parameter list, with a space in between: > > [](void*, struct wl_registry*, uint32_t) { } Ok, same here. > > Source/WebCore/platform/graphics/wayland/WlUniquePtr.h:37 > > +template<typename T> > > +struct WlPtrDeleter { > > + void operator()(T* ptr) const { ASSERT_NOT_REACHED(); } > > +}; > > Tested it out on a simplified test case, and apparently deleting this > operator would throw a compile-time error if T didn't have the related > WlPtrDeleter specialization available. That's probably preferable to > catching missing specializations in a debug build. Ah, yes compile error is much better than runtime one. Thanks for checking this. > > Source/WebKit2/UIProcess/gtk/AcceleratedBackingStore.cpp:65 > > +AcceleratedBackingStore::~AcceleratedBackingStore() > > +{ > > +} > > = default; Ok. > > Source/WebKit2/UIProcess/gtk/WaylandCompositor.cpp:247 > > + [](struct wl_client*, struct wl_resource* resource) { > > + wl_resource_destroy(resource); > > + }, > > Ditto re: style. > > > Source/WebKit2/WebProcess/WebPage/gtk/AcceleratedSurface.cpp:65 > > +AcceleratedSurface::~AcceleratedSurface() > > +{ > > +} > > = default;

Created attachment 287260 [details] Patch for landing

Created attachment 287261 [details] Patch for landing

Committed r205116: <http://trac.webkit.org/changeset/205116>

> Note this is not > strcmp that returns 0 in case of success, it's strstr that returns nullptr > in case of failure. This is correct... (In reply to comment #65) > > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:152 > > > + if (!strstr(extensions, "EGL_KHR_surfaceless_context") && !strstr(extensions, "EGL_KHR_surfaceless_opengl")) > > > > It's OK to proceed if one or the other extension is present? They are not > > both necessary? > > There's && so it will only proceed if both are present. ...but this is not correct. You want to use || there. You're bailing out only if both fail, but you want to bail when either one fails.

(In reply to comment #70) > > Note this is not > > strcmp that returns 0 in case of success, it's strstr that returns nullptr > > in case of failure. > > This is correct... > > (In reply to comment #65) > > > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:152 > > > > + if (!strstr(extensions, "EGL_KHR_surfaceless_context") && !strstr(extensions, "EGL_KHR_surfaceless_opengl")) > > > > > > It's OK to proceed if one or the other extension is present? They are not > > > both necessary? > > > > There's && so it will only proceed if both are present. > > ...but this is not correct. You want to use || there. You're bailing out > only if both fail, but you want to bail when either one fails. Sorry, I misunderstood your question. They are not two extensions, they are the same thing, but the name changed, so we proceed if the old or new name are present.

Thank you so much for driving this to completion! I will just note here the issues I found back when trying to avoid having compositing go through the nested compositor and GDK/GTK+ access the upstream compositor directly from WebProcesses, as the committed code does. I'm not sure they are worth opening a new bug, but I will do that if there's interest. The first issue is that when opening a Wayland connection GDK checks that the xdg-shell protocol is available and returns no display if it is not the case: https://git.gnome.org/browse/gtk+/tree/gdk/wayland/gdkdisplay-wayland.c?id=055ce9f09#n540 Ideally we don't need GDK to talk with the compositor at all: WebProcesses use GTK+ only for theming, and the few settings needed for that are conveyed through GSettings and not through Wayland in any case. However the GDK display is created during gtk_init(), which calls exit(1) if unable to open the display. Calling gtk_init_check() avoids the exit(1) but the whole point of the _init stuff is about connecting to the display system. I tried to skip the gtk_init() call in the WebProcess and (ignoring the fact that WebKit failed to detect it was running under Wayland as it would likely be an easy to fix issue) the theming infrastructure failed because gtk_settings_get_default() doesn't work without an initialized display, and thus connecting to the notify::gtk-theme-name and notify::gtk-color-scheme signals fails. As anticipated, under Wayland those signals are not conveyed by the display system but through GSettings (ie. dconf/D-Bus): however the setup for that is done in _gdk_wayland_screen_new() while opening the display in _gdk_wayland_display_open(). To make everything run we could implement an xdg-shell stub in the nested compositor that really does nothing but makes GDK happy, and with little effort it would be possible to make GDK go through the nested compositor with no changes.

Followup in bug #161530 to get rid of glReadPixels().