GNOME Bugzilla – Bug 695737
Add wayland support
Last modified: 2014-04-25 22:51:29 UTC
GtkClutterEmbed doesn't work on wayland because it uses X11 specific calls, and wayland doesn't have support for sub-surfaces that are needed to get an equivalent to the child windows we use in X11.
Created attachment 238749 [details] [review] util: Add Wayland support to init Pretty useless, but gets the code running a little bit further.
The above patch allows to do this: https://twitter.com/hadessuk/status/311844151337562112
This patch helped to get quadrapassel to at least start on wayland. The other games that use clutter-gtk did start but they call gtk_clutter_init. Quadrapassel instead calls gtk_clutter_init_with_args which calls post_parse_hook where clutter-gtk fails on wayland.
Created attachment 253979 [details] [review] wayland: Import a copy of the subsurface protocol extension
Created attachment 253980 [details] [review] wayland: Generate client side binding for the subsurface protocol This adds a non-optional dependency on Clutter Wayland and GTK Wayland as well as the Wayland client library itself.
Created attachment 253981 [details] [review] gitignore: Update for generated subsurface code
Created attachment 253982 [details] [review] wayland: Integrate GTK+ and Clutter's display Tell Clutter to use the display from GTK and in order to avoid deadlocks on the Wayland socket disable Clutter's polling on that socket.
Created attachment 253983 [details] [review] wayland: Use the subsurface protocol to implement GtkClutterEmbed The subsurface protocol lets us "embed" one surface within another. The compositor will compose the two surfaces together to create a single window. We use it here to take the surface we get from Clutter and make that into a subsurface and then associate that subsurface with the main surface coming from GTK+.
Review of attachment 253982 [details] [review]: ::: clutter-gtk/gtk-clutter-util.c @@ +107,3 @@ /* let GTK+ be in charge of the event handling */ + clutter_wayland_disable_event_retrieval (); + clutter_wayland_set_display (gdk_wayland_display_get_wl_display (display)); You're missing the same fix in gtk_clutter_init()
Created attachment 254364 [details] [review] wayland: Integrate GTK+ and Clutter's display Tell Clutter to use the display from GTK and in order to avoid deadlocks on the Wayland socket disable Clutter's polling on that socket.
Review of attachment 253980 [details] [review]: This seems to fail to create the files when using parallel builds, I had to "make subsurface-client-protocol.h" ::: clutter-gtk/Makefile.am @@ +22,3 @@ lib_LTLIBRARIES = libclutter-gtk-@CLUTTER_GTK_API_VERSION@.la +source_c = $(srcdir)/subsurface-protocol.c $(srcdir)/subsurface-client-protocol.h Those should be $(builddir), not $(srcdir), as they're generated files.
Review of attachment 253981 [details] [review]: Looks good.
Created attachment 254366 [details] [review] wayland: Integrate GTK+ and Clutter's display Tell Clutter to use the display from GTK and in order to avoid deadlocks on the Wayland socket disable Clutter's polling on that socket.
Created attachment 254367 [details] [review] util: Add Wayland support to init Pretty useless, but gets the code running a little bit further.
Review of attachment 254366 [details] [review]: The original fix was missing an include, and the fix in gtk_clutter_init().
Review of attachment 254367 [details] [review]: Old and incomplete, maybe should be merged into your other patch? This is rebased on current master.
Review of attachment 253979 [details] [review]: This is easy enough.
Review of attachment 253983 [details] [review]: I don't know whether this code is responsible for the problem, but I only see between 1 and zero floating windows in wayland, whereas I see between 3 and 4 with X11, with the gtk-clutter-test-actor example. $ GDK_BACKEND=wayland CLUTTER_BACKEND=wayland ./gtk-clutter-test-actor <one or no floating windows) $ ./gtk-clutter-test-actor <three or 4 floating windows>
I also had to apply the patch from https://bugzilla.gnome.org/show_bug.cgi?id=707704 to prevent the above X11 test case from crashing (GTK+ would prefer wayland, and Clutter X11, thus, assertion).
Seems to be a problem with positioning of the subsurface within scrolled windows. In those cases, this works better than using the allocation: gtk_widget_translate_coordinates (widget, toplevel, 0, 0, &x, &y); But sizing is still wrong.
Created attachment 255417 [details] [review] wayland: Generate client side binding for the subsurface protocol This adds a non-optional dependency on Clutter Wayland and GTK Wayland as well as the Wayland client library itself. v1: Integrate fix from Bastien to use $(builddir)
Created attachment 255418 [details] [review] wayland: Integrate GTK+ and Clutter's display Tell Clutter to use the display from GTK and in order to avoid deadlocks on the Wayland socket disable Clutter's polling on that socket. v1: Integrates Bastien's original enabling patch: attachment#254367 [details]
Review of attachment 255417 [details] [review]: ::: clutter-gtk/Makefile.am @@ +3,3 @@ NULL = +CLEANFILES = subsurface-client-protocol.h subsurface-protocol.c these should go below, and conditionally added i.e. if BUILD_WAYLAND CLEANFILES += subsurface-client-protocol.h subsurface-protocol.c endif @@ +22,3 @@ lib_LTLIBRARIES = libclutter-gtk-@CLUTTER_GTK_API_VERSION@.la +source_c = $(builddir)/subsurface-protocol.c $(builddir)/subsurface-client-protocol.h same as above, plus builddir is never used. if BUILD_WAYLAND source_c += \ $(top_builddir)/clutter-gtk/subsurface-protocol.c \ $(top_builddir)/clutter-gtk/subsurface-protocol.h \ $(NULL) endif @@ +64,3 @@ + +@wayland_scanner_rules@ should be conditionally expanded. ::: configure.ac @@ +89,3 @@ AC_SUBST([CLUTTER_GTK_DEPS_CFLAGS]) AC_SUBST([CLUTTER_GTK_DEPS_LIBS]) +PKG_CHECK_MODULES([WAYLAND_DEPS], [wayland-client gtk+-wayland-3.0 clutter-wayland-1.0]) this should really be conditional. PKG_CHECK_MODULES([WAYLAND_DEPS], [wayland-client gtk+-wayland-3.0 clutter-wayland-1.0], [build_wayland=yes], [build_wayland=no]) AM_CONDITIONAL(BUILD_WAYLAND, [test "x$build_wayland" = "xyes"]) you're also not using WAYLAND_DEPS_CFLAGS and WAYLAND_DEPS_LIBS anywhere. @@ +204,3 @@ GOBJECT_INTROSPECTION_CHECK([0.9.12]) +WAYLAND_SCANNER_RULES(['$(top_srcdir)/protocol']) again, conditional, under an AS_IF([test "x$build_wayland" = "xyes"],...)
Review of attachment 255418 [details] [review]: ::: clutter-gtk/gtk-clutter-util.c @@ +219,3 @@ +#if defined(GDK_WINDOWING_WAYLAND) && defined(CLUTTER_WINDOWING_WAYLAND) + if (clutter_check_windowing_backend (CLUTTER_WINDOWING_WAYLAND)) needs run-time check on the GdkDisplay.
Review of attachment 253983 [details] [review]: ::: clutter-gtk/gtk-clutter-embed.c @@ +389,3 @@ + struct wl_compositor *compositor; + + display = gdk_display_get_default (); should probably get the display from the GdkWindow, to ensure consistency. @@ +392,3 @@ + compositor = gdk_wayland_display_get_wl_compositor (display); + clutter_surface = wl_compositor_create_surface (compositor); + struct wl_surface *clutter_surface, *gtk_surface; coding style: missing space between function and arguments. @@ +396,3 @@ + clutter_surface); + priv->subsurface = + coding style: indentation level. @@ +576,3 @@ +#if defined(GDK_WINDOWING_WAYLAND) && defined(CLUTTER_WINDOWING_WAYLAND) + if (priv->subsurface) + { coding style: indentation. @@ +999,3 @@ + GtkClutterEmbedPrivate *priv = embed->priv; + + uint32_t name, coding style: braces on a different indentation level. @@ +1011,3 @@ + struct wl_registry *registry, + uint32_t name) + shouldn't priv->subcompositor be NULL-ified here? do we need to care? @@ +1075,3 @@ + if (clutter_check_windowing_backend (CLUTTER_WINDOWING_WAYLAND) && + GDK_IS_WAYLAND_DISPLAY (gdk_display)) + { coding style: indentation level.
(In reply to comment #18) > Review of attachment 253983 [details] [review]: > > I don't know whether this code is responsible for the problem, but I only see > between 1 and zero floating windows in wayland, whereas I see between 3 and 4 > with X11, with the gtk-clutter-test-actor example. > > $ GDK_BACKEND=wayland CLUTTER_BACKEND=wayland ./gtk-clutter-test-actor > <one or no floating windows) > $ ./gtk-clutter-test-actor > <three or 4 floating windows> Isn't this GTK embedding Clutter rather than Clutter embedding in GTK? This might be the same offscreen window problem as: https://bugzilla.gnome.org/show_bug.cgi?id=695494 ?
Created attachment 256487 [details] [review] examples: Add a GtkStack to the examples Clicking on the "clickety" button in the gtk-clutter-test will toggle, in a GtkStack, between a label, and a clutter-gtk stage. It doesn't take very long to see the display artifacts after that.
Sadly, it looks like this won't happen for 3.12
Trying the clutter-gtk tests here with 3.12 and these patches here, the first impression is that I don't get any events - no clicking or typing...
Created attachment 274495 [details] [review] gtk-clutter-util: Remove extraneous set to ARGB32 visuals This is already done in the post-parse hook
Created attachment 274496 [details] [review] gtk-clutter-util: Use the GDK display when using the GDK backend This is what the post-parse hook does.
Created attachment 274497 [details] [review] gtk-clutter-util: Don't error out for an unknown backend We could be just fine with e.g. the Wayland backend.
Created attachment 274498 [details] [review] gtk-clutter-util: Merge fixes from post_parse_hook Get these two methods back in sync.
Created attachment 274499 [details] [review] gtk-clutter-util: Disable accessibility in the post_parse_hook as well
Created attachment 274500 [details] [review] gtk-clutter-util: Put the shared initializion logic in a shared function
Created attachment 274501 [details] [review] gdk-clutter-util: Add Wayland support to init Pretty useless, but gets the code running a little bit further.
Created attachment 274502 [details] [review] wayland: Use the subsurface protocol to implement GtkClutterEmbed The subsurface protocol lets us "embed" one surface within another. The compositor will compose the two surfaces together to create a single window. We use it here to take the surface we get from Clutter and make that into a subsurface and then associate that subsurface with the main surface coming from GTK+.
Review of attachment 274495 [details] [review]: okay.
Review of attachment 274496 [details] [review]: okay.
Review of attachment 274497 [details] [review]: I'd actually add a Wayland windowing system backend check, and still `g_error()` out for unrecognised backends.
Review of attachment 274498 [details] [review]: okay.
Review of attachment 274499 [details] [review]: okay.
Review of attachment 274500 [details] [review]: okay.
Review of attachment 274501 [details] [review]: okay.
Review of attachment 274502 [details] [review]: looks good, with one minor issue that can be fixed prior to pushing. ::: clutter-gtk/gtk-clutter-embed.c @@ +385,3 @@ + struct wl_compositor *compositor; + + display = gdk_display_get_default (); we may want to get the GdkDisplay from the widget, in case we end up supporting multiple display connections.
Attachment 274495 [details] pushed as 95c004c - gtk-clutter-util: Remove extraneous set to ARGB32 visuals Attachment 274496 [details] pushed as e56061b - gtk-clutter-util: Use the GDK display when using the GDK backend Attachment 274499 [details] pushed as a160c1e - gtk-clutter-util: Disable accessibility in the post_parse_hook as well
Created attachment 274507 [details] [review] gtk-clutter-util: Merge fixes from post_parse_hook Get these two methods back in sync.
Created attachment 274508 [details] [review] gtk-clutter-util: Put the shared initializion logic in a shared function
Created attachment 274509 [details] [review] gdk-clutter-util: Add Wayland support to init Pretty useless, but gets the code running a little bit further.
Created attachment 274510 [details] [review] wayland: Use the subsurface protocol to implement GtkClutterEmbed The subsurface protocol lets us "embed" one surface within another. The compositor will compose the two surfaces together to create a single window. We use it here to take the surface we get from Clutter and make that into a subsurface and then associate that subsurface with the main surface coming from GTK+.
Review of attachment 274507 [details] [review]: okay
Review of attachment 274508 [details] [review]: okay
Review of attachment 274509 [details] [review]: okay
Review of attachment 274510 [details] [review]: okay ::: clutter-gtk/gtk-clutter-embed.c @@ +1004,3 @@ + struct wl_registry *registry, + uint32_t name) + if (strcmp (interface, "wl_subcompositor") == 0) should we unset priv->subcompositor here? can it even happen that the interface may go away? I doubt it...
Review of attachment 274510 [details] [review]: It's theoretically possible (that's what the global_removed hook is for), but I can't imagine it happening on a real compositor in practice.
Attachment 274507 [details] pushed as fbd8889 - gtk-clutter-util: Merge fixes from post_parse_hook Attachment 274508 [details] pushed as 7c143f8 - gtk-clutter-util: Put the shared initializion logic in a shared function Attachment 274509 [details] pushed as a117b16 - gdk-clutter-util: Add Wayland support to init Attachment 274510 [details] pushed as 25db4f8 - wayland: Use the subsurface protocol to implement GtkClutterEmbed
Jasper, Emmanuele, Bastien, Matthias, thanks for pursuing this after I ran out of time to work on this.