GNOME Bugzilla – Bug 671741
[RFC] Add basic hybrid wayland support
Last modified: 2013-09-11 14:27:09 UTC
Within Intel we have been working quite heavily on Wayland recently and Neil Roberts and myself have been working on showing that it's possible to incrementally adapt an existing, mature, X based compositor into a hybrid Wayland compositor, and maybe even one day into a standalone Wayland compositor too. This gives us a good way to validate Wayland protocols and we also think this could be the most practical way of making Wayland into a reality for full desktop environments not least because we can have an excellent compatibility story for X applications which will continue to need a window manager. We would love to get some early feedback to see if others would be open to the idea of us integrating patches for mutter adding hybrid Wayland support in a way that still preserves the ability to build Mutter without Wayland support and it will work as it does today, unchanged. We are hopeful that it should be possible to work on this integration in master so we can avoid needing to maintain a fork of mutter. This first set of patches are a bare minimum to add hybrid compositing support to Mutter. The main patch is quite big since the addition of src/wayland/meta-wayland.[ch] is a fairly monolithic addition but I think although I hope people will be interested by that code, for this discussion the changes that affect the core of mutter are probably more important and those are much more minimal. For reference; to follow on from these patches later we do have two other branches wip/wayland-input and wip/wayland-kms which people may be interested in looking at too. The hope is that if we can get this first set of patches integrated first then we can look at reviewing this work next. This first set of patches doesn't include any input support and just provides the ability to have X11 as well as Wayland clients. Since they don't add Kernel Mode Setting support or tty support the only way to test is by running mutter using Clutter's X11 backend, so it's a bit like running mutter hosted under X using Xephyr. Any feedback welcome, thanks
Created attachment 209349 [details] [review] screen: Don't ref workspace->screen after freeing workspace In meta_screen_remove_workspace its possible that the workspace will be disposed before we call meta_screen_set_active_workspace_hint to update the active workspace index so we make sure to use the screen pointer passed as an argument to meta_screen_remove_workspace() instead of referring to the workspace->screen pointer which might cause a crash if workspace was indeed freed.
Created attachment 209350 [details] [review] configure: Adds wayland config options This adds a --enable-wayland configure option to enable building mutter as a hybrid X and Wayland compositor. By default the option is disabled. If enabled HAVE_WAYLAND is defined for C code and as an automake conditional. In addition a --with-wayland-protocols option has been added since wayland support will depend on the xserver.xml extension protocol that's currently part of the Weston project which we need the location of.
Created attachment 209351 [details] [review] configure: Adds --with-xwayland-path option This adds a --with-xwayland-path configure option that can be used to specify the absolute path of a headless X server binary supporting the wayland xserver protocol.
Created attachment 209352 [details] [review] wayland: Adds basic hybrid X + Wayland support This adds support for running mutter as a hybrid X and Wayland compositor. It runs a headless XWayland server for X applications that presents wayland surfaces back to mutter which mutter can then composite. At this point no input is supported The following specific dependencies are required to run this: repo: git://anongit.freedesktop.org/wayland/wayland commit: e0b6af03cad9cb9700 (Don't try wayland master since there have been interface changes we need to update for) repo: git://anongit.freedesktop.org/~krh/xserver branch: wayland-1.10
Review of attachment 209349 [details] [review]: Dup of the patch in https://bugzilla.gnome.org/show_bug.cgi?id=671601
Comment on attachment 209349 [details] [review] screen: Don't ref workspace->screen after freeing workspace I've marked the patch to fix the meta_screen_remove_workspace as obsolete, since its duplicated in another bug and not actually specific to adding wayland support.
Created attachment 209875 [details] [review] wayland: Adds basic hybrid X + Wayland support I've updated the patch with a minor tweak to reinstate meta_{enable,disable}_unredirect_for_screen when running under Wayland so that it will work with Gnome Shell. I've also updated the three branches with this squashed in.
Created attachment 210104 [details] [review] wayland: Adds basic hybrid X + Wayland support I've squashed in a fix from Rico Tzschichholz to fix the generation of the wayland xserver protocol files and rebased the wip/wayland* branches too.
Created attachment 210190 [details] [review] wayland: Adds basic hybrid X + Wayland support I've rebased on master and fixed the filenames listed for the generated xserver protocol files.
Just to note that I've rebased the patches on master and for reference there is now also a wip/wayland-stacking branch that builds on the input branch adding core support in the mutter stacking code for stacking Wayland clients along with X11 clients. I think this is probably one of the most awkward parts of mutter to integrate Wayland support into so I'm happy to have got that working.
Last year I spent some days starting to port mutter to Wayland 1.0. I came to a point where I could launch mutter inside weston (my main focus was to use weston as a compositor talking to the drm layer and have mutter as a nested compositor) and it could display simple-shm. Since then I have not worked on it more, and it is not in a very pretty state at the moment, but that can improve. Anyhow, it's based on wip/wayland-stacking and I just put it here: https://github.com/jadahl/mutter/commits/wip/wayland-1.0 so people can look at it.
Created attachment 248007 [details] [review] Track the X Shape input region and use it for picking We now track whether a window has an input shape specified via the X Shape extension. Intersecting that with the bounding shape (as required by the X Shape extension) we use the resulting rectangles to paint window silhouettes when picking. As well as improving the correctness of picking this should also be much more efficient because typically when only picking solid rectangles then the need to actually render and issue a read_pixels request can be optimized away and instead the picking is done on the cpu.
Created attachment 248008 [details] [review] configure: Adds wayland config options This adds a --enable-wayland configure option to enable building mutter as a hybrid X and Wayland compositor. By default the option is disabled. If enabled HAVE_WAYLAND is defined for C code and as an automake conditional. In addition a --with-wayland-protocols option has been added since wayland support will depend on the xserver.xml extension protocol that's currently part of the Weston project which we need the location of.
Created attachment 248009 [details] [review] configure: Adds --with-xwayland-path option This adds a --with-xwayland-path configure option that can be used to specify the absolute path of a headless X server binary supporting the wayland xserver protocol.
Created attachment 248010 [details] [review] Adds a --nested option This adds a --nested option to request that mutter no longer run as a classic X compositor with an output window mapped on the X Composite Overlay Window and also not assume it is running directly under X. The intention is that in this mode Mutter will itself launch a headless X server and display output will be handled by Clutter and Cogl. This will enable running Mutter nested as an application within an X session. This patch introduces the internal idea that mutter is running as a "display server" as a means to condition the way mutter operates in various ways throughout the code base. Later we also expect to add a --kms option as another way of enabling this display server mode that will assume full control of the display hardware instead of running as a nested application.
Created attachment 248011 [details] [review] wayland: Adds basic hybrid X + Wayland support This adds support for running mutter as a hybrid X and Wayland compositor. It runs a headless XWayland server for X applications that presents wayland surfaces back to mutter which mutter can then composite. This aims to not break Mutter's existing support for the traditional X compositing model which means a single build of Mutter can be distributed supporting the traditional model and the new Wayland based compositing model. TODO: although building with --disable-wayland has at least been tested, I still haven't actually verified that running as a traditional compositor isn't broken currently. At this point no input is supported Note: This patch is still a work in progress Additional authors: Neil Roberts <neil@linux.intel.com> Rico Tzschichholz.
Created attachment 248012 [details] [review] Add support for stacking X and Wayland windows together This breaks down the assumptions in stack-tracker.c and stack.c that Mutter is only stacking X windows. The stack tracker now tracks windows using a MetaStackWindow structure which is a union with a type member so that X windows can be distinguished from Wayland windows. Some notable changes are: Queued stack tracker operations that affect Wayland windows will not be associated with an X serial number. If an operation only affects a Wayland window and there are no queued stack tracker operations ("unvalidated predictions") then the operation is applied immediately since there is no server involved with changing the stacking for Wayland windows. The stack tracker can no longer respond to X events by turning them into stack operations and discarding the predicted operations made prior to that event because operations based on X events don't know anything about the stacking of Wayland windows. Instead of discarding old predictions the new approach is to trust the predictions but whenever we receive an event from the server that affects stacking we cross-reference with the predicted stack and check for consistency. So e.g. if we have an event that says ADD window A then we apply the predictions (up to the serial for that event) and verify the predicted state includes a window A. Similarly if an event says RAISE_ABOVE(B, C) we can apply the predictions (up to the serial for that event) and verify that window B is above C. If we ever receive spurious stacking events (with a serial older than we would expect) or find an inconsistency (some things aren't possible to predict from the compositor) then we hit a re-synchronization code-path that will query the X server for the full stacking order and then use that stack to walk through our combined stack and force the X windows to match the just queried stack but avoiding disrupting the relative stacking of Wayland windows. This will be relatively expensive but shouldn't be hit for compositor initiated restacking operations where our predictions should be accurate. The code in core/stack.c that deals with synchronizing the window stack with the X server had to be updated quite heavily. In general the patch avoids changing the fundamental approach being used but most of the code did need some amount of re-factoring to consider what re-stacking operations actually involve X or not and when we need to restack X windows we sometimes need to search for a suitable X sibling to restack relative too since the closest siblings may be Wayland windows.
Created attachment 248013 [details] [review] wayland: Add basic input support This copies the basic input support from the Clayland demo compositor. It adds a basic wl_seat implementation which can convert Clutter mouse events to Wayland events. For this to work all of the wayland surface actors need to be made reactive. The wayland keyboard input focus surface is updated whenever Mutter sees a FocusIn event so that it will stay in synch with whatever surface Mutter wants as the focus. Wayland surfaces don't get this event so for now it will just give them focus whenever they are clicked as a hack to test the code.
Created attachment 248014 [details] [review] wayland: Add an actor for the cursor When running Mutter under Cogl's KMS backend no cursor will be provided so instead this makes it so the cursor will be painted as a CoglTexture that gets moved in response to mouse motion events. The painting is done in a subclass of ClutterStage so that we can guarantee that the cursor will be painted on top of everything else. This patch adds support for the set_cursor method on the pointer interface so that clients can change the cursor image. The set_pointer method sets a surface and a hotspot position to use for the cursor image. The surface's buffer is converted to a CoglTexture and attached to a pipeline to paint directly via Cogl. If a new buffer is attached to the surface the image will be updated. The cursor reverts back to the default image whenever to the pointer focus is moved off of any surface. The image for the pointer is taken from X. It gets installed into a fixed data location for mutter.
Created attachment 248015 [details] [review] wayland: support left click to raise wayland surfaces This adds support for raising wayland surfaces when clicked with the left mouse button.
Created attachment 248016 [details] [review] wayland: implement shell surface move interface This implements the shell surface move interface so now it's possible to use the mouse to interactively move wayland based windows around the screen.
Ok, I've attached an updated set of patches to get mutter working with Wayland 1.1. Dependencies: Since there is quite a bit of churn going on in wayland master a.t.m anyone testing these patches should checkout this wayland commit: 7094441b1d1ad15e7e0121410d23c3e94731a805 You should fetch the latest xwayland from: git://anongit.freedesktop.org/xorg/xserver You should fetch the latest libxkbcommon from: https://github.com/xkbcommon/libxkbcommon.git (Note don't use the repo on freedesktop.org) You should use the 1.16 branches of clutter and cogl Due to the churn in libwayland a.t.m it's likely we will push various updates to the cogl-1.16 branch to adapt to the server api changes but when we do that you will then need to fetch the latest wayland instead of the commit listed above. Compared to the original patches made for wayland 0.85 this series is designed to be able to support choosing at runtime whether mutter is run as a wayland compositor or as a traditional X compositor, the hope being that we can land the work in master as soon as possible to avoid having to continually rebase this work as mutter evolves.
Review of attachment 248007 [details] [review]: Hm. I'd prefer if this was kept to MetaWindowActor, and simply having MetaShapedTexture expose a rectangular pick. Nowadays, MetaShapedTexture is more of a "MetaMaskedTexture", and there's only a few cases where we actually use it alone in a reactive setting: in the overview of gnome-shell. I'm not sure you'd want to need to click on the center part in oclock to select it. Simply falling back to a basic quad for MetaShapedTexture, and adding all the fancy picking support to MetaWindowActor seems better to me...
Review of attachment 248008 [details] [review]: ::: configure.ac @@ +122,3 @@ +## repository but since there aren't currently established conventions for +## installing and discovering these we simply require a location to be given +## explicitly... Want to see if we can have a standard /usr/share/waylandproto/ dir or something? Or make it a .pc variable of wayland.pc? I'd prefer if we worked with upstream on this sooner rather than later.
Review of attachment 248007 [details] [review]: (No idea why this was marked as ACN)
Review of attachment 248010 [details] [review]: It seems that occasionally in later patches you use "is_display_server" to mean "is_wayland", making Wayland-specific judgment calls. I also don't like the if statements littered all over the place. Obviously, it won't be easy, but would it be possible to have something like MetaOutput vtable? So we'd have MetaX11Output for a traditional X11 compositor, and then MetaNestedOutput to behave like a "Clutter app", and MetaKmsOutput. I think that would also help us understand the API boundaries more, and resist the temptation to do "if (is_display_server ()) wayland_hack ();"
Review of attachment 248011 [details] [review]: ::: src/compositor/meta-plugin-manager.c @@ +320,3 @@ + + if (meta_is_display_server ()) + return FALSE; You're gonna have to explain this one to me. (Also, either use "else if", or delete the else below) ::: src/compositor/meta-shaped-texture.c @@ +73,2 @@ G_DEFINE_TYPE (MetaShapedTexture, meta_shaped_texture, CLUTTER_TYPE_ACTOR); Doesn't match the parent class struct in the header? Is that a leftover? @@ -66,3 @@ { MetaTextureTower *paint_tower; - Pixmap pixmap; As a quick note, I just want to point out that the Pixmap here isn't actually necessary in the slightest. We only use it to make the CoglTexturePixmapX11, so a cleanup would be to remove it and just have the MetaShapedTexture take a CoglTexture directly, punting the other responsibilities (update_area, texture tower, etc.) over to MetaWindowActor. I think I like the idea of MetaShapedTexture becoming oblivious to the underlying window system, and simply being a thing that MetaWindowActor "controls". @@ +91,3 @@ + MetaWaylandSurface *surface; +#warning "FIXME: now that we track the surface we don't need to track the buffer separately" + //struct wl_buffer *buffer; uh? I assume this is no longer necessary? @@ +616,2 @@ static void +queue_damage_redraw_with_clip (MetaShapedTexture *stex, Perhaps this optimization should be split out into a separate patch? It seems quite handy to have, even without the Wayland stuff. ::: src/compositor/meta-window-actor.c @@ +68,3 @@ MetaShadow *unfocused_shadow; + Pixmap back_pixmap; /* Not used in display server mode */ In Wayland mode, all of the X11 compositor stuff isn't really used. Instead of trying to shove the Wayland stuff into the X11 compositor class, how about MetaWindowActorX11 / MetaWindowActorWayland (and perhaps MetaWindowActorXwayland for special support? Not sure if that's needed), inheriting from a common MetaWindowActor which handles the common APIs like effects? ::: src/compositor/meta-window-group.c @@ +205,3 @@ + /* Currently wayland clients have no way to report opaque + * window regions so for now we assume that all wayland + * clients are transparent... */ Are you sure about that? I'm quite sure krh was working on something like this. ::: src/core/display.c @@ +942,3 @@ + * We still want a guard window so we can avoid + * unmapping/withdrawing minimized windows for live + * thumbnails... Do we? As a Wayland compositor we can handle this all in-house: don't pass any events to the surface, and don't show the buffer on the screen. If you want to keep it for simplicity for now, perhaps create it outside of manage_all_windows ? @@ +3849,3 @@ +#ifdef HAVE_WAYLAND + if (xwindow == None) + return; ??? ::: src/core/window.c @@ +834,3 @@ meta_verbose ("Attempting to manage 0x%lx\n", xwindow); + if (client_type == META_WINDOW_CLIENT_TYPE_X11 && Split all the X11 stuff into a separate function if you can. ::: src/meta/meta-shaped-texture.h @@ +53,3 @@ /*< private >*/ +#ifdef HAVE_WAYLAND + ClutterWaylandSurfaceClass parent_class; yuck ::: src/wayland/meta-wayland.c @@ +801,3 @@ + shell_surface_set_maximized, + shell_surface_set_title, + shell_surface_set_class I assume all of these implementations will be filled in at some point. @@ +852,3 @@ + shell_surface->resource.object.interface = &wl_shell_surface_interface; + shell_surface->resource.object.implementation = + (void (**) (void)) &meta_wayland_shell_surface_interface; Isn't this a cast for a function pointer? @@ +1001,3 @@ + +static int +bind_to_unix_socket (int display) What's the difference between this and bind_to_abstract_socket? One unlinks, the other doesn't, one has a NUL at the start, the other doesn't? It doesn't make sense to me. @@ +1138,3 @@ + +static void +stop_xwayland (MetaWaylandCompositor *compositor) Could you move all of this messy Xwayland spawning code to a separate file? @@ +1355,3 @@ + /* The simplest measure to avoid infinitely re-spawning a crashing + * X server */ + if (pid == compositor->xwayland_pid) Just curious, what would auto-respawn the X server? @@ +1365,3 @@ + * In the future X will only be loaded lazily for legacy X support + * but for now it's a hard requirement. */ + g_critical ("Spurious exit of X Wayland server"); g_critical is not an abort. You want g_error, perhaps.
Review of attachment 248012 [details] [review]: I'd prefer this to pass an Owen review, but I just want to point out that historically, stack.c was more like stack-tracker.c, and that it was changed to be the model it is today, and stack-tracker.c was written for compositor support. It's possible that we should aggregate or merge the two, considering you're changing the stack.c model again. ::: src/core/stack-tracker.c @@ +164,3 @@ +{ + if (window->any.type == META_WINDOW_CLIENT_TYPE_X11) + return window->x11.xwindow == None ? FALSE : TRUE; return window->x11.xwindow != None @@ +166,3 @@ + return window->x11.xwindow == None ? FALSE : TRUE; + else + return window->wayland.meta_window ? TRUE : FALSE; return window->wayland.meta_window != NULL @@ +176,3 @@ + { + if (a->any.type == META_WINDOW_CLIENT_TYPE_X11) + return a->x11.xwindow == b->x11.xwindow; If we have a sibling field, shouldn't we compare those as well? @@ -517,2 @@ { - meta_stack_tracker_record_raise_above (tracker, window, None, serial); I just noticed this. This is a bug in the existing code, but I'm quite sure this should be lower_below, not raise_above. If something actually exercises this code path, please write a separate patch. If nothing calls lower (and/or lower_below), please remove it in a separate patch.
Review of attachment 248013 [details] [review]: Hm, wasn't the plan to use the evdev support from Cogl/Clutter, rather than connecting to the kernel directly? ::: src/compositor/meta-window-actor.c @@ +386,3 @@ + if (meta_is_display_server ()) + clutter_actor_set_reactive (priv->actor, TRUE); Hm. Originally, I had the idea that MetaWindowActor would become a public API, so parts of the shell would do "new Meta.WindowActor" at various parts, when they currently use a ClutterClone of the stex. In this case, the user of MetaWindowActor would mark it as reactive. That is, compositor.c would set the reactive bit. If we don't want to pursue that path, then we should always make it reactive. @@ +2773,3 @@ +#ifdef HAVE_WAYLAND +ClutterActor * +meta_window_actor_get_shaped_texture (MetaWindowActor *self) Doesn't this already exist? For screenshots? And why the HAVE_WAYLAND? ::: src/core/display.c @@ +2639,3 @@ + MetaWaylandCompositor *compositor = + meta_wayland_compositor_get_default (); + meta_wayland_compositor_set_input_focus (compositor, window); Does HAVE_WAYLAND guarantee we have a Wayland compositor? @@ +3207,3 @@ + translation altogether by directly using the Clutter events */ +#ifdef HAVE_WAYLAND + if (event->type == MotionNotify) So, first off, this shouldn't happen when just building with Wayland support. This should happen only when is_display_server. Second, we won't get MotionNotify events since the port to XI2. ::: src/wayland/meta-wayland-data-device.c @@ +1,2 @@ +/* + * Copyright © 2011 Kristian Høgsberg Where does this come from? Clayland? Add a "based on" comment below, please. ::: src/wayland/meta-wayland-keyboard.c @@ +98,3 @@ + +#ifdef HAVE_MKOSTEMP + fd = mkostemp (tmpname, O_CLOEXEC); I thought we had a wrapper for this in glib, but if not, I think it should go in libgsystem. @@ +117,3 @@ + GError **error) +{ + static const char template[] = "weston-shared-XXXXXX"; "mutter-shared-XXXXXX" ? @@ +187,3 @@ + xkb_info->keymap_area = mmap (NULL, xkb_info->keymap_size, + PROT_READ | PROT_WRITE, + MAP_SHARED, xkb_info->keymap_fd, 0); Not using libxkbcommon, I see... @@ +544,3 @@ + xkb_context_unref (keyboard->xkb_context); + + /* XXX: What about keyboard->resource_list? */ What about it? ::: src/wayland/meta-wayland-pointer.c @@ +2,3 @@ + * MetaWayland + * + * An example Wayland compositor using Clutter I hope it's more than that. :) ::: src/wayland/meta-wayland.c @@ +1318,3 @@ + + /* We want to synthesize X events for mouse motion events so that we + don't have to rely on the X server's window position being I don't quite get this. I guess we'll talk on IRC about it. @@ +1319,3 @@ + /* We want to synthesize X events for mouse motion events so that we + don't have to rely on the X server's window position being + synched with the surface positoin. See the comment in "synced", "position" @@ +1320,3 @@ + don't have to rely on the X server's window position being + synched with the surface positoin. See the comment in + event_callback() in display.c */ ... which you renamed in this very patch :) @@ +1526,3 @@ + * signal won't get emitted but we still want to see these events so + * we can update the cursor position. To make sure we see all events + * we also install an emission hook on the event signal */ Holy hacks, Batman. You can't do this with an "event" signal?
Review of attachment 248014 [details] [review]: The commit message is misleading. There is no actor for the cursor. I'm not sure I like the whole "paint on top of the stage" thing, because it doesn't seem like makes us up for using hardware cursors under kms in the future, as we're tied to the Clutter paint loop. Having an actor system seems like it would be better, but that's just gut intuition -- I can easily be swayed. The image for the pointer is actually taken from Adwaita. While I'm sure we don't want to implement the entire cursor theme system, I think we should probably (hardcode?) /usr/share/themes/Adwaita/cursors/24x24/left_ptr.png for now.
Review of attachment 248015 [details] [review]: ::: src/wayland/meta-wayland-seat.c @@ +355,3 @@ pointer->grab_y = pointer->y; + + if (button == BTN_LEFT && Is there no way to tie this into the event handler in display.c, where we implement focus-follows-mouse, etc.? Having it in here just seems entirely wrong.
Review of attachment 248016 [details] [review]: This should tie into the meta_display_begin_grab_op system. ::: src/wayland/meta-wayland.c @@ +689,3 @@ + MetaFrameBorders borders; + *rect = window->frame->rect; + meta_frame_calc_borders (window->frame, &borders); What's the borders for? You don't use them. In which case this looks like it would be meta_window_get_outer_rect.
Review of attachment 248009 [details] [review]: This should, again, be a .pc variable in the xwayland.pc file.
(In reply to comment #23) > Review of attachment 248007 [details] [review]: > > Hm. I'd prefer if this was kept to MetaWindowActor, and simply having > MetaShapedTexture expose a rectangular pick. Nowadays, MetaShapedTexture is > more of a "MetaMaskedTexture", and there's only a few cases where we actually > use it alone in a reactive setting: in the overview of gnome-shell. I'm not > sure you'd want to need to click on the center part in oclock to select it. > > Simply falling back to a basic quad for MetaShapedTexture, and adding all the > fancy picking support to MetaWindowActor seems better to me... Evolving the design and responsibilities of MetaWindowActor and MetaShapedTexture seems reasonable enough but I think it's also independent from this work to integrate Wayland support, so I think we should avoid conflating this. Wayland doesn't really change whether or not this is a good idea and currently MetaWindowActor doesn't have any picking code so we just modified the existing ShapedTexture picking code. If you think it would be a better design then you probably have a more in-depth understanding of why that is and so it would probably make more sense for you to make that change because the idea probably relates to other things too. From the pov of this work I would prefer if the change was done after we land this, but we can potentially rebase the patches if were done first too.
(In reply to comment #24) > Review of attachment 248008 [details] [review]: > > ::: configure.ac > @@ +122,3 @@ > +## repository but since there aren't currently established conventions for > +## installing and discovering these we simply require a location to be given > +## explicitly... > > Want to see if we can have a standard /usr/share/waylandproto/ dir or > something? Or make it a .pc variable of wayland.pc? > > I'd prefer if we worked with upstream on this sooner rather than later. Right. I did once look at this back when we made the patches for wayland 0.85 but in general back then no one was particularly concerned about coming up with a standard scheme and up until now the various projects dealing wayland extensions have actually just been copying the xml specs between projects (e.g. xserver.xml is copied in weston and xwayland). Talking about this today with krh and others we agreed if would be simple enough to have the xserver-xwayland project install its copy of the xserver.xml spec and also an xwayland.pc file which we can use to find the xwayland binary and the xml spec too.
(In reply to comment #25) > Review of attachment 248007 [details] [review]: > > (No idea why this was marked as ACN) strange, no idea either - I just used git bz attach
(In reply to comment #26) > Review of attachment 248010 [details] [review]: > > It seems that occasionally in later patches you use "is_display_server" to mean > "is_wayland", making Wayland-specific judgment calls. I also don't like the if > statements littered all over the place. > > Obviously, it won't be easy, but would it be possible to have something like > MetaOutput vtable? So we'd have MetaX11Output for a traditional X11 compositor, > and then MetaNestedOutput to behave like a "Clutter app", and MetaKmsOutput. > > I think that would also help us understand the API boundaries more, and resist > the temptation to do "if (is_display_server ()) wayland_hack ();" Yeah, there are a few things to talk about here. Firstly it was quite intentional to only make the minimal design changes to mutter to enable integration and generally start of with a fairly self contained set of src/wayland files. This has helped ease rebasing the work during early development and also minimizes the risk of regressing X compositing support. Really, the whole approach of this integration approach is a trade of in favour of practicality over design ideals since mutter was obviously never designed as a wayland compositor. We should keep in mind that these is_display_server() conditionals are only a short term requirement, because going forward the expectation is that we'll be making display server mode the only mode for running mutter. I think the extra work that would be involved in adding some form of vtable abstraction will be fairly wasted effort if anyway it shouldn't be that long before we just remove support for running as a traditional X compositor. The tentative plan was to be able to switch by Gnome 3.12 to using display server mode by default. Considering that we're starting with a codebase that is so utterly X dependent, to the bone, we haven't really been too concerned with trying to draw clear lines between X and anything else - I think that kind of design will inevitably come with time, but it might also be easier once we get to delete some of the X code we don't need any more. I think a final thing to note here is that adding kms support isn't really expected to affect this conditional code, so we wouldn't expect to have a third set of code paths in the way you suggest. In the kms case it will essentially just be explicitly making cogl use the egl_kms renderer with some vt handling code, but for everything else it's just running as a display server the same as when running nested.
(In reply to comment #27) > Review of attachment 248011 [details] [review]: > > ::: src/compositor/meta-plugin-manager.c > @@ +320,3 @@ > + > + if (meta_is_display_server ()) > + return FALSE; > > You're gonna have to explain this one to me. When mutter is running as a display server, things like input events just come directly from clutter so it won't have disabled clutter's event retrieval and won't need to forward it events (if it did it would lead to recursion). Also when running as a display server we shouldn't be assuming that we're running with the clutter x11 backend. Adding a decent comment here make sense. Yeah, making the if else style be consistent here seems fine. > > (Also, either use "else if", or delete the else below) > > ::: src/compositor/meta-shaped-texture.c > @@ +73,2 @@ > G_DEFINE_TYPE (MetaShapedTexture, meta_shaped_texture, > CLUTTER_TYPE_ACTOR); > > Doesn't match the parent class struct in the header? Is that a leftover? yeah sorry, that was just a left over and I have a patch to clean this up which I can attach > > @@ -66,3 @@ > { > MetaTextureTower *paint_tower; > - Pixmap pixmap; > > As a quick note, I just want to point out that the Pixmap here isn't actually > necessary in the slightest. We only use it to make the CoglTexturePixmapX11, so > a cleanup would be to remove it and just have the MetaShapedTexture take a > CoglTexture directly, punting the other responsibilities (update_area, texture > tower, etc.) over to MetaWindowActor. > > I think I like the idea of MetaShapedTexture becoming oblivious to the > underlying window system, and simply being a thing that MetaWindowActor > "controls". Similar to earlier, as a general design idea then moving more responsibility into MetaWindowActor could be a good idea but I also think we should avoid conflating design changes like this that aren't strictly necessary for the wayland integration. This change could be reasonable enough to do with or without wayland it seems. As with the earlier idea, it would be more awkward for us if we made the change before landing this work, but potentially we could rebase the patches. > > @@ +91,3 @@ > + MetaWaylandSurface *surface; > +#warning "FIXME: now that we track the surface we don't need to track the > buffer separately" > + //struct wl_buffer *buffer; > > uh? I assume this is no longer necessary? oops yeah, that's just a leftover and I have a patch I can attach to remove that > > @@ +616,2 @@ > static void > +queue_damage_redraw_with_clip (MetaShapedTexture *stex, > > Perhaps this optimization should be split out into a separate patch? It seems > quite handy to have, even without the Wayland stuff. Yeah, I suppose if it's not too much effort to split out then this could be good to land separately > > ::: src/compositor/meta-window-actor.c > @@ +68,3 @@ > MetaShadow *unfocused_shadow; > > + Pixmap back_pixmap; /* Not used in display server mode */ > > In Wayland mode, all of the X11 compositor stuff isn't really used. Instead of > trying to shove the Wayland stuff into the X11 compositor class, how about > MetaWindowActorX11 / MetaWindowActorWayland (and perhaps > MetaWindowActorXwayland for special support? Not sure if that's needed), > inheriting from a common MetaWindowActor which handles the common APIs like > effects? That sounds like a lot of code churn at this stage, considering having to maintain and rebase this work until we can get it integrated. It could be something to consider later though. Since it sounds like you have quite a few ideas about clarifying the role of ShapeTexture vs WindowActor maybe any re-work should take into account those other ideas too. > > ::: src/compositor/meta-window-group.c > @@ +205,3 @@ > + /* Currently wayland clients have no way to report opaque > + * window regions so for now we assume that all wayland > + * clients are transparent... */ > > Are you sure about that? I'm quite sure krh was working on something like this. Yeah, this is just an old comment because this work was originally done for the wayland 0.85 protocol back in 2011. This can be updated to a TODO to consider the wl_surface set_opaque_region api. > > ::: src/core/display.c > @@ +942,3 @@ > + * We still want a guard window so we can avoid > + * unmapping/withdrawing minimized windows for live > + * thumbnails... > > Do we? As a Wayland compositor we can handle this all in-house: don't pass any > events to the surface, and don't show the buffer on the screen. > > If you want to keep it for simplicity for now, perhaps create it outside of > manage_all_windows ? Yeah, we should be able to use the MetaWindow::hidden property so we can ignore windows that would be under the guard window. For initial versions of this work it probably would have been easiest to continue using the guard window or maybe we just didn't realize at the time that there was a ::hidden property we could use to distinguish which X windows should be ignored for input delivery. I suppose it might be good to start this way and then have a separate patch that removes the need for the guard window since that will probably touch the stacking code I guess which might be nice to avoid in this initial commit. > > @@ +3849,3 @@ > +#ifdef HAVE_WAYLAND > + if (xwindow == None) > + return; > > ??? Having an xwindow of None essentially implies that this is actually a Wayland client. It would be better to check the client_type before calling meta_display_unregister_x_window() though. > > ::: src/core/window.c > @@ +834,3 @@ > meta_verbose ("Attempting to manage 0x%lx\n", xwindow); > > + if (client_type == META_WINDOW_CLIENT_TYPE_X11 && > > Split all the X11 stuff into a separate function if you can. Yeah, actually I'd been meaning to do that at some point. > > ::: src/meta/meta-shaped-texture.h > @@ +53,3 @@ > /*< private >*/ > +#ifdef HAVE_WAYLAND > + ClutterWaylandSurfaceClass parent_class; > > yuck oops, yeah that's just a left over that's not necessary any more, I have a patch I can attach to remove this. > > ::: src/wayland/meta-wayland.c > @@ +801,3 @@ > + shell_surface_set_maximized, > + shell_surface_set_title, > + shell_surface_set_class > > I assume all of these implementations will be filled in at some point. Actually that's not really a given at this point. Discussing the shell interface with krh and others today the consensus is that the wl_shell interface probably won't be developed much further as part of the core protocol and we should instead be looking at defining our own wl_gnome_shell protocol with a eye to creating a sharable wl_xdg_shell interface in the future once it's clear what parts are sharable with other desktop environments. Developing the shell capabilities is generally work for the future, according to real world toolkit requirements and we are only bootstrapping wayland support with this patch series. > > @@ +852,3 @@ > + shell_surface->resource.object.interface = &wl_shell_surface_interface; > + shell_surface->resource.object.implementation = > + (void (**) (void)) &meta_wayland_shell_surface_interface; > > Isn't this a cast for a function pointer? It's a redundant cast to an array of function pointers (following the convention in weston probably), but anyway this about to change because the libwayland server interface has changed in the last week which means resources are no longer embedded. E.g. you can see the corresponding changes made to cogland: https://git.gnome.org/browse/cogl/commit/?id=adb971954757dffb9ddecf8c9091b96756424800 > > @@ +1001,3 @@ > + > +static int > +bind_to_unix_socket (int display) > > What's the difference between this and bind_to_abstract_socket? One unlinks, > the other doesn't, one has a NUL at the start, the other doesn't? > > It doesn't make sense to me. That's just the difference between creating an abstract or unix socket and an abstract socket doesn't have a filesystem entry so it doesn't need unlinking. It would be straight forward enough to have a shared function though I suppose for whether it's abstract or not. On closer inspection of the start_xwayland api that uses these apis it looks like there are some other curious logic issues that need double checking too actually. > > @@ +1138,3 @@ > + > +static void > +stop_xwayland (MetaWaylandCompositor *compositor) > > Could you move all of this messy Xwayland spawning code to a separate file? Yeah that would probably be good > > @@ +1355,3 @@ > + /* The simplest measure to avoid infinitely re-spawning a crashing > + * X server */ > + if (pid == compositor->xwayland_pid) > > Just curious, what would auto-respawn the X server? When this code would have originally been written and tested outside of mutter (in test-wayland-surface) it was probably able to handle xserver restarts. In the case of mutter there's no way we could handle the server exiting so currently this consideration is unnecessary. The long term aim would be that the xserver should only be started lazily for X compatibility, but it'll be a while before we're able to do that :-) > > @@ +1365,3 @@ > + * In the future X will only be loaded lazily for legacy X support > + * but for now it's a hard requirement. */ > + g_critical ("Spurious exit of X Wayland server"); > > g_critical is not an abort. You want g_error, perhaps. right
(In reply to comment #28) > Review of attachment 248012 [details] [review]: > > I'd prefer this to pass an Owen review, but I just want to point out that > historically, stack.c was more like stack-tracker.c, and that it was changed to > be the model it is today, and stack-tracker.c was written for compositor > support. It's possible that we should aggregate or merge the two, considering > you're changing the stack.c model again. I think the algorithms of stack.c (in particular for stack_sync_to_server) are fairly original (as in written by Havoc) and weren't really changed when the stack-tracker was added. I definitely want to avoid changing them now and risk breaking stacking for x windows. The stacking code in general is an area where I really don't think we should conflate changing the model for tracking window stacking with integrating wayland support. The model is working well enough for tracking X windows and the maturity of this code is one of mutter's biggest assets so I don't really want to risk breaking it at the same time as adding wayland support. Adding wayland support doesn't change the requirements for reliably tracking the stack of X windows (we are making mutter a hybrid compositor not a wayland only compositor) so any ideas so re-work the model used for stacking should probably either be done separately before we land this integration (that wouldn't be ideal for us having to rebase these patches since this was possibly the trickiest part of the integration work so far) or done afterwards. > > ::: src/core/stack-tracker.c > @@ +164,3 @@ > +{ > + if (window->any.type == META_WINDOW_CLIENT_TYPE_X11) > + return window->x11.xwindow == None ? FALSE : TRUE; > > return window->x11.xwindow != None > > @@ +166,3 @@ > + return window->x11.xwindow == None ? FALSE : TRUE; > + else > + return window->wayland.meta_window ? TRUE : FALSE; > > return window->wayland.meta_window != NULL yeah - not really sure why I used the ternary operator there > > @@ +176,3 @@ > + { > + if (a->any.type == META_WINDOW_CLIENT_TYPE_X11) > + return a->x11.xwindow == b->x11.xwindow; > > If we have a sibling field, shouldn't we compare those as well? I'm not really sure what you mean here; a MetaStackWindow doesn't have a sibling, it's the stack operations that have siblings? > > @@ -517,2 @@ > { > - meta_stack_tracker_record_raise_above (tracker, window, None, serial); > > I just noticed this. This is a bug in the existing code, but I'm quite sure > this should be lower_below, not raise_above. > > If something actually exercises this code path, please write a separate patch. > If nothing calls lower (and/or lower_below), please remove it in a separate > patch. The semantics of raising above None/NULL is that the window is lowered to the bottom so I think this is fine.
(In reply to comment #34) > From the pov of this work I > would prefer if the change was done after we land this, but we can potentially > rebase the patches if were done first too. It's just that we made the Stex blind to rectangles recently, so I'd rather not have "here's a mask texture for painting, and a set of rectangles for picking" -- that just seems like a confusing API altogether. (In reply to comment #35) > up until now the various projects dealing wayland > extensions have actually just been copying the xml specs between projects (e.g. > xserver.xml is copied in weston and xwayland). I'm fine with copy-in-tree too, like we do with DBus protocols. As long as there's a standard location for them.
Sorry for the late reply. (In reply to comment #29) > Hm, wasn't the plan to use the evdev support from Cogl/Clutter, rather than > connecting to the kernel directly? I'm not sure what you mean here. It *is* reading events via Clutter and converting them into Wayland events. It doesn't read from evdev directly. > If we don't want to pursue that path, then we should always make it reactive. Ok, I'm not exactly sure what the best thing to do there is then, but maybe making it always reactive would be fine. > @@ +2773,3 @@ > +#ifdef HAVE_WAYLAND > +ClutterActor * > +meta_window_actor_get_shaped_texture (MetaWindowActor *self) > > Doesn't this already exist? For screenshots? And why the HAVE_WAYLAND? Yes, you're right. I have removed it in this patch: https://git.gnome.org/browse/mutter/commit/?h=wip/wayland&id=befd43a30e045 > ::: src/core/display.c > @@ +2639,3 @@ > + MetaWaylandCompositor *compositor = > + meta_wayland_compositor_get_default (); > + meta_wayland_compositor_set_input_focus (compositor, window); > > Does HAVE_WAYLAND guarantee we have a Wayland compositor? Yes, good catch. This got missed in the change to using a runtime switch. Giovanni has fixed this in a patch on his wip/wayland-display branch and I have also cherry-picked it into the wip/wayland branch: https://git.gnome.org/browse/mutter/commit/?h=wip/wayland&id=078e0a5b29c5 > @@ +3207,3 @@ > + translation altogether by directly using the Clutter events */ > +#ifdef HAVE_WAYLAND > + if (event->type == MotionNotify) > > So, first off, this shouldn't happen when just building with Wayland support. > This should happen only when is_display_server. > > Second, we won't get MotionNotify events since the port to XI2. Ah yes, this code was written before Mutter was using XI2. I have made a patch to make it synthesize XI2 events instead: https://git.gnome.org/browse/mutter/commit/?h=wip/wayland&id=8946889712b > ::: src/wayland/meta-wayland-data-device.c > @@ +1,2 @@ > +/* > + * Copyright © 2011 Kristian Høgsberg > > Where does this come from? Clayland? Add a "based on" comment below, please. Ok, I have added a “based on” comment here: https://git.gnome.org/browse/mutter/commit/?h=wip/wayland&id=7bc7ba0c0164 > ::: src/wayland/meta-wayland-keyboard.c > @@ +98,3 @@ > + > +#ifdef HAVE_MKOSTEMP > + fd = mkostemp (tmpname, O_CLOEXEC); > > I thought we had a wrapper for this in glib, but if not, I think it should go > in libgsystem. It looks like glib has a wrapper for making temporary files but it doesn't use mkostemp. mkostemp has the advantage that you can atomically set the O_CLOEXEC flag so that there is no race condition where another thread might fork before the process gets a chance to set the flag. However I think this isn't a big deal for Mutter so I've made a patch to use the glib wrapper: https://git.gnome.org/browse/mutter/commit/?h=wip/wayland&id=ec36cffaba52 > > @@ +117,3 @@ > + GError **error) > +{ > + static const char template[] = "weston-shared-XXXXXX"; > > "mutter-shared-XXXXXX" ? Oops, right you are: https://git.gnome.org/browse/mutter/commit/?h=wip/wayland&id=4f507d87 > @@ +187,3 @@ > + xkb_info->keymap_area = mmap (NULL, xkb_info->keymap_size, > + PROT_READ | PROT_WRITE, > + MAP_SHARED, xkb_info->keymap_fd, 0); > > Not using libxkbcommon, I see... This is code that is directly copied from Weston. I'm not sure what you mean about it using libxkbcommon. In general the code is using it and I'm not sure how it could help in this particular place. This is copying the keymap data into the buffer for the temporary file so that the data can be passed to the client by passing the fd. > @@ +544,3 @@ > + xkb_context_unref (keyboard->xkb_context); > + > + /* XXX: What about keyboard->resource_list? */ > > What about it? This comment is copied from Weston. I guess it's suggesting that the list should be freed? This code is all about to change in Weston anyway thanks to patches from Robert Bradford so perhaps we can update it when eventually copy in those changes: http://lists.freedesktop.org/archives/wayland-devel/2013-July/010342.html > ::: src/wayland/meta-wayland-pointer.c > @@ +2,3 @@ > + * MetaWayland > + * > + * An example Wayland compositor using Clutter > > I hope it's more than that. :) Ah yes, sorry, I've fixed that too. > ::: src/wayland/meta-wayland.c > @@ +1318,3 @@ > + > + /* We want to synthesize X events for mouse motion events so that we > + don't have to rely on the X server's window position being > > I don't quite get this. I guess we'll talk on IRC about it. This was code was written because we were encountering a problem where when you drag a window around it will jitter about randomly. This happens because of the complicated way X events get into Mutter. All events initially come from Clutter and the Wayland code within Mutter takes these and converts them to Wayland events and passes them on to all the clients. The X server is a Wayland client so it will get its events from Mutter this way and pass them on to any X clients. Mutter is also an X client so it will receive the events again as X events, and that is what is used for most of the event handling code. The problem arises because Wayland events are surface relative but the X server wants a global position. Therefore it uses the position of the window to convert these events back into a global position. However the position of the window and the events are sent via two different streams (the X connection and the Wayland connection) which means they can be out of sync. This will happen while the window is being dragged around which means the X events that Mutter sees will end up with the wrong position. To fix it we just made it so Mutter will handle dragging the window around based directly on Clutter events via synthesized X events so that it avoids the translation altogether. Ideally Mutter would directly use Clutter events for all input and not worry about the X events to avoid the complicated routing. > @@ +1320,3 @@ > + don't have to rely on the X server's window position being > + synched with the surface positoin. See the comment in > + event_callback() in display.c */ > > ... which you renamed in this very patch :) event_callback() is still there and it still has the comment. It is now just a thin wrapper around a new function called meta_display_handle_event() which contains the previous contents of event_callback(). > @@ +1526,3 @@ > + * signal won't get emitted but we still want to see these events so > + * we can update the cursor position. To make sure we see all events > + * we also install an emission hook on the event signal */ > > Holy hacks, Batman. You can't do this with an "event" signal? Could you explain this a bit more? Where would the “event” signal live? I think the problem is we want to catch Clutter events at a lower level than the actor event signal mechanism but there isn't any API to do that so we end up with this hack. Maybe we could add something like the GDK event filter to Clutter which this code could take advantage of.
Ok, I've just force pushed an updated wip/wayland with lots of clean up patches by Neil and myself squashed back as appropriate. I haven't pushed all of the separate patches since Jasper already reviewed most of these while we were face to face at Guadec. Except for the suggestions that involved more significant re-working of code I *think* we've addressed everything that Jasper commented on, plus a few extra minor things we discussed at Guadec.
I think this bug has served its purpose. Things are moving forward on the wayland branch of mutter, and in various other bugs that can be found by looking for 'wayland' in the whiteboard