GNOME Bugzilla – Bug 720631
Prerequisite cleanups for surface-content / XWayland fixes
Last modified: 2016-07-21 19:42:45 UTC
Most of this is removing old APIs, and moving stuff around. Should be mostly straightforward. These APIs are now unused in gnome-shell, except for position-changed / size-changed, which I have a local patch for.
Created attachment 264436 [details] [review] surface-actor: Move unobscured_region processing here
Created attachment 264437 [details] [review] window-actor: Never unredirect when under Wayland
Created attachment 264438 [details] [review] window-actor: Simplify the unredirected check in cull_out
Created attachment 264439 [details] [review] compositor: Simplify the unredirected window management code
Created attachment 264440 [details] [review] window-actor: Flip set_redirected around I know it's confusing with the triple negative, but unredirected is how we track it elsewhere: we have an 'unredirected' flag, and 'should_unredirect'.
Created attachment 264441 [details] [review] window-actor: Remove old unused APIs
Created attachment 264442 [details] [review] Move position-changed / size-changed signals to the MetaWindow They fit more appropriately over here...
Created attachment 267556 [details] [review] Always map the client and frame windows Traditionally, WMs unmap windows when minimizing them, and map them when restoring them or wanting to show them for other reasons, like upon creation. However, as metacity morphed into mutter, we optionally chose to keep windows mapped for the lifetime of the window under the user option "live-window-previews", which makes the code keep windows mapped so it can show window preview for minimized windows in other places, like Alt-Tab and Expose. I removed this preference two years ago mechanically, by removing all the if statements, but never went through and cleaned up the code so that windows are simply mapped for the lifetime of the window -- the "architecture" of the old code that maps and unmaps on show/hide was still there. Remove this now. The one case we still need to be careful of is shaded windows, in which we do still unmap the client window. Theoretically, we might want to show previews of shaded windows in the overview and Alt-Tab, so we remove the complex unmap tracking for this later.
Created attachment 267557 [details] [review] compositor: Remove meta_compositor_window_[un]mapped We no longer unmap the toplevel windows during normal operation. The toplevel state is tied to the window's lifetime. Call meta_compositor_add_window / meta_compositor_remove_window instead...
Created attachment 267558 [details] [review] compositor: Remove pending_input_region Ever since the change to create the output window synchronously at startup, there hasn't been any time where somebody could set a stage region the output window was ready, so this was effectively dead code.
Created attachment 267559 [details] [review] surface-actor: Move unobscured_region processing here
Created attachment 267560 [details] [review] window-actor: Simplify the unredirected check in cull_out
Created attachment 267561 [details] [review] compositor: Simplify the unredirected window management code
Created attachment 267562 [details] [review] window-actor: Flip set_redirected around I know it's confusing with the triple negative, but unredirected is how we track it elsewhere: we have an 'unredirected' flag, and 'should_unredirect'.
Created attachment 267563 [details] [review] window-actor: Remove old unused APIs
Created attachment 267564 [details] [review] Move position-changed / size-changed signals to the MetaWindow They fit more appropriately over here...
Created attachment 267565 [details] [review] window-actor: Split CLIENT_TYPE into two subclasses of MetaSurfaceActor We now split the rendering logic into two subclasses, which are: * MetaSurfaceActorX11, which handles the X11 compositor case, in that it uses XCompositeNameWindowPixmap to get the backing pixmap, and deal with all the COMPOSITE extension messiness. * MetaSurfaceActorWayland, which handles the Wayland compositor case for both native Wayland clients and XWayland clients. XWayland handles COMPOSITE for us, and handles pushing a surface over through the xf86-video-wayland DDX. Frame sync is still in MetaWindowActor, as it needs to work for both the X11 compositor and XWayland clients. Theoretically, when the video display protocol lands, we can implement a "Wayland" backend for this, using a similar system.
Created attachment 267566 [details] [review] window: Delay the showing of XWayland clients until set_window_id Use our new "surface_mapped" field to delay the showing of XWayland clients until we have associated together the window's XID and the Wayland surface ID. This ensures that when we show this window to the compositor, it will properly use the Wayland surface for rendering, rather than trying to use COMPOSITE and crash.
Review of attachment 267556 [details] [review]: You need to call meta_compositor_window_mapped() on meta_window_new_shared() or so for windows which have attrs->map_state != IsUnmapped. In particular, override redirect windows which come up already mapped never show up right now. ::: src/compositor/meta-window-actor.c @@ +1633,3 @@ + /* We know that when the compositor first adds our window, it will + * be before the toplevel window is mapped. */ + priv->mapped = FALSE; A bit further down there's an if (priv->mapped) which can't ever be true now.
Review of attachment 267557 [details] [review]: Ah gotcha. This looks good. Perhaps squashed with the previous one?
Review of attachment 267558 [details] [review]: Looks fine
Review of attachment 267559 [details] [review]: ::: src/compositor/meta-surface-actor.c @@ +42,3 @@ + MetaSurfaceActorPrivate *priv = self->priv; + + if (!CLUTTER_ACTOR_CLASS (meta_surface_actor_parent_class)->get_paint_volume (actor, volume)) This seems to be always false and we never get to the optimization below. Or am I missing something here?
Review of attachment 267560 [details] [review]: Right
Review of attachment 267561 [details] [review]: Looks good
Review of attachment 267562 [details] [review]: I prefer it like this
Review of attachment 267563 [details] [review]: ++
Review of attachment 267564 [details] [review]: Why do we need this? The semantics aren't exactly the same, e.g. on frozen MetaWindowActors. ::: src/core/window.c @@ +5395,3 @@ save_user_window_placement (window); + if (need_move_frame) ... || need_move_client) ? @@ +5398,3 @@ + g_signal_emit (window, window_signals[POSITION_CHANGED], 0); + + if (need_resize_client) ... || need_resize_frame) ?
(In reply to comment #27) > Why do we need this? The semantics aren't exactly the same, e.g. on frozen > MetaWindowActors. Because the code is going away in the next commit. Hopefully I can get Owen to review this one in more detail... > ::: src/core/window.c > @@ +5395,3 @@ > ... || need_move_client) ? No. This is about the toplevel position changing. If we make the border on the top thicker, and the border on the bottom thinner, then the client has moved, but the toplevel hasn't. > @@ +5398,3 @@ > ... || need_resize_frame) ? This should be need_resize_frame instead of need_resize_client, you're right, e.g. if borders change without the client changing size.
(In reply to comment #28) > > ::: src/core/window.c > > @@ +5395,3 @@ > > ... || need_move_client) ? > > No. This is about the toplevel position changing. If we make the border on the > top thicker, and the border on the bottom thinner, then the client has moved, > but the toplevel hasn't. But the wayland code path above doesn't set need_move_frame so we miss moves on wayland. Perhaps that patch should do need_move_frame = need_move_client; ?
Ugh, yeah, I didn't think of undecorated clients in general. What we really want is a "toplevel_resized" / "toplevel_moved", but an || is a close enough approximation.
Review of attachment 267559 [details] [review]: ::: src/compositor/meta-surface-actor.c @@ +45,3 @@ + return FALSE; + + if (priv->unobscured_region) In light of Adel's finding in bug 721596, this should be if (effective_unobscured_region()) or so, to account for mapped clones. @@ +227,3 @@ + MetaSurfaceActorPrivate *priv = self->priv; + + if (priv->unobscured_region) effective_unobscured_region() here as well I think.
Attachment 267560 [details] pushed as 60d9bee - window-actor: Simplify the unredirected check in cull_out Attachment 267561 [details] pushed as d628271 - compositor: Simplify the unredirected window management code Attachment 267562 [details] pushed as 39fee9f - window-actor: Flip set_redirected around Attachment 267563 [details] pushed as b9755ea - window-actor: Remove old unused APIs Pushing these for now.
Comment on attachment 267558 [details] [review] compositor: Remove pending_input_region Attachment 267558 [details] pushed as d5d5c21 - compositor: Remove pending_input_region
Created attachment 267760 [details] [review] Always map the client and frame windows Traditionally, WMs unmap windows when minimizing them, and map them when restoring them or wanting to show them for other reasons, like upon creation. However, as metacity morphed into mutter, we optionally chose to keep windows mapped for the lifetime of the window under the user option "live-window-previews", which makes the code keep windows mapped so it can show window preview for minimized windows in other places, like Alt-Tab and Expose. I removed this preference two years ago mechanically, by removing all the if statements, but never went through and cleaned up the code so that windows are simply mapped for the lifetime of the window -- the "architecture" of the old code that maps and unmaps on show/hide was still there. Remove this now. The one case we still need to be careful of is shaded windows, in which we do still unmap the client window. Theoretically, we might want to show previews of shaded windows in the overview and Alt-Tab, so we remove the complex unmap tracking for this later. At the same time, simplify the compositor interface by removing meta_compositor_window_[un]mapped API, and instead adding/removing the window on-demand.
Created attachment 267761 [details] [review] surface-actor: Move unobscured_region processing here
Created attachment 267762 [details] [review] Move position-changed / size-changed signals to the MetaWindow They fit more appropriately over here...
Created attachment 267763 [details] [review] window-actor: Split CLIENT_TYPE into two subclasses of MetaSurfaceActor We now split the rendering logic into two subclasses, which are: * MetaSurfaceActorX11, which handles the X11 compositor case, in that it uses XCompositeNameWindowPixmap to get the backing pixmap, and deal with all the COMPOSITE extension messiness. * MetaSurfaceActorWayland, which handles the Wayland compositor case for both native Wayland clients and XWayland clients. XWayland handles COMPOSITE for us, and handles pushing a surface over through the xf86-video-wayland DDX. Frame sync is still in MetaWindowActor, as it needs to work for both the X11 compositor and XWayland clients. Theoretically, when the video display protocol lands, we can implement a "Wayland" backend for this, using a similar system.
Created attachment 267764 [details] [review] window: Delay the showing of XWayland clients until set_window_id Use our new "surface_mapped" field to delay the showing of XWayland clients until we have associated together the window's XID and the Wayland surface ID. This ensures that when we show this window to the compositor, it will properly use the Wayland surface for rendering, rather than trying to use COMPOSITE and crash.
rt # Bug 720631 - Prerequisite cleanups for surface-content / XWayland fixes - UNCONFIRMED
Review of attachment 267761 [details] [review]: Had a quick look over this one. It does not just move stuff around but changes semantics and breaks what the old code intended to do. ::: src/compositor/meta-surface-actor.c @@ +72,3 @@ + cairo_rectangle_int_t bounds; + + /* I hate ClutterPaintVolume so much... */ You can't hate it that much to warrant a comment in the code ;) @@ +145,3 @@ + MetaSurfaceActor *self = META_SURFACE_ACTOR (cullable); + + set_unobscured_region (self, NULL); No. You cannot unset this after painting it is needed to optimize out damage events and for computing the paint volume. ::: src/compositor/meta-window-actor.c @@ +984,3 @@ if (!priv->repaint_scheduled) { + gboolean is_obscured = meta_surface_actor_is_obscured (priv->surface); This checks are not equivalent. You are just checking whether the unobscured region is non empty. Given that the region can be way bigger then the window (almost whole screen for a toplevel window) this does not work. You have to intersect with the window region. ::: src/compositor/meta-window-group.c @@ -134,3 @@ - { - MetaWindowActor *window_actor = META_WINDOW_ACTOR (child); - meta_window_actor_set_unobscured_region (window_actor, NULL); That is actually needed you cannot remove the region after the paint otherwise we have no region between paints (like when we get a damage events) that's why it gets reset before we paint.
(In reply to comment #40) > Had a quick look over this one. It does not just move stuff around but changes > semantics and breaks what the old code intended to do. Can you explain to me simply what the code was supposed to do, then? I figured it would be a nice touch to make it work for subsurfaces, so we don't draw subsurfaces that are culled out, much like what we do for windows. > You can't hate it that much to warrant a comment in the code ;) Look at the amount of code I have to do to intersect two ClutterPaintVolumes. Ugh. > ::: src/compositor/meta-window-actor.c > This checks are not equivalent. You are just checking whether the unobscured > region is non empty. Given that the region can be way bigger then the window > (almost whole screen for a toplevel window) this does not work. You have to > intersect with the window region. We should probably clip the unobscured region to the opaque_region when we set it in set_unobscured_region then. Or maybe we should move this to the ShapedTexture instead, which is the one that actually doesn't cull out? That might be a better fit... > ::: src/compositor/meta-window-group.c > That is actually needed you cannot remove the region after the paint otherwise > we have no region between paints (like when we get a damage events) that's why > it gets reset before we paint. I'm curious here, what's the goal of unsetting it at all? If we traverse all cullables, then each cullable can determine whether it wants an unobscured_region or not in its cull_out vfunc.
(In reply to comment #32) > Attachment 267562 [details] pushed as 39fee9f - window-actor: Flip set_redirected around this broke Mutter's compilation in master, because it references the function meta_window_actor_detach_x11_pixmap() which does not exist.
(In reply to comment #41) > (In reply to comment #40) > > Had a quick look over this one. It does not just move stuff around but changes > > semantics and breaks what the old code intended to do. > > Can you explain to me simply what the code was supposed to do, then? I figured > it would be a nice touch to make it work for subsurfaces, so we don't draw > subsurfaces that are culled out, much like what we do for windows. When the window group gets painted we calculate the unobscured region for each window by iterating over them. Now when we get a damage event we intersect that damage region with the unobscured region. And only queue a clipped redraw for the intersection (if there is any). If you set the unobscured region to NULL directly after paint we lose this information and we end up queing redraws in response to damage events for invisible areas ... not really what we want to do. Then there is the frame sync handling if the window is completely obscured we do not queue any redraw but we need to tell the window that something happened. So we send out frame drawn messages at ~10fps in that case (see the original bug for rationale). > > You can't hate it that much to warrant a comment in the code ;) > > Look at the amount of code I have to do to intersect two ClutterPaintVolumes. > Ugh. > > > ::: src/compositor/meta-window-actor.c > > This checks are not equivalent. You are just checking whether the unobscured > > region is non empty. Given that the region can be way bigger then the window > > (almost whole screen for a toplevel window) this does not work. You have to > > intersect with the window region. > > We should probably clip the unobscured region to the opaque_region when we set > it in set_unobscured_region then. Or maybe we should move this to the > ShapedTexture instead, which is the one that actually doesn't cull out? That > might be a better fit... Yeah maybe shaped texture is a better place. My point was less about where it lives but more about what it does. > > ::: src/compositor/meta-window-group.c > > That is actually needed you cannot remove the region after the paint otherwise > > we have no region between paints (like when we get a damage events) that's why > > it gets reset before we paint. > > I'm curious here, what's the goal of unsetting it at all? If we traverse all > cullables, then each cullable can determine whether it wants an > unobscured_region or not in its cull_out vfunc. If you don't reset it "or not" would leave the old value which can be very wrong (window moved, restacked etc.).
Review of attachment 267762 [details] [review]: Makes sense, but we use both in gnome-shell so do not forgot to update them when landing this.
(In reply to comment #42) > this broke Mutter's compilation in master, because it references the function > meta_window_actor_detach_x11_pixmap() which does not exist. Fixed, thanks.
Comment on attachment 267764 [details] [review] window: Delay the showing of XWayland clients until set_window_id Attachment 267764 [details] pushed as 59c8b94 - window: Delay the showing of XWayland clients until set_window_id Pushing this now since it was ACN'd and it can simply be pushed now. This fixes XWayland in the short term.
Created attachment 268205 [details] [review] cullable: Reset the culling state instead of skipping the traversal When we traversed down to reset the culling state, previously we would just skip any actors that wanted culling. In order to properly reset the unobscured_region before painting, we need to traverse down to these places as well. Do this by calling cull_out with NULL regions for both arguments, and adapt existing cull_out implementations to match.
Created attachment 268206 [details] [review] shaped-texture: Move unobscured_region processing here We want to remove a bunch of auxilliary duties from the MetaWindowActor and MetaSurfaceActor, including some details of how culling is done. Move the unobscured region culling code to the MetaShapedTexture, which helps the actor become "more independent".
Review of attachment 268205 [details] [review]: LG.
Review of attachment 268206 [details] [review]: Looks good to me (other then that one bogos param) but have not tested it. Testing would be to try a fast drawing window like glxgears in the foreground and once in the background and watch the fps (CLUTTER_SHOW_FPS=1) should be <= 5 for so for the hidden case; same with a frame synced app but it should not visibly redraw itself "catch up" once it gets back to the foreground. ::: src/compositor/meta-window-actor.c @@ +1954,3 @@ event->area.width, event->area.height, + event->area.height); Huh what?
Attachment 268205 [details] pushed as 27ab516 - cullable: Reset the culling state instead of skipping the traversal Attachment 268206 [details] pushed as f6db756 - shaped-texture: Move unobscured_region processing here Pushed with fixes. I couldn't test glxgears (Xwayland segfaulted), but I tested with a few other clients, and I couldn't notice any problems.
Review of attachment 267760 [details] [review]: In general, seems fine to me. In the commit message: > Theoretically, we might want to show previews of shaded windows in the overview and Alt-Tab, so we remove the complex unmap tracking for this later. is a bit garbled, maybe: In the future, we might want to show previews of shaded windows in the overview and Alt-Tab. In that we'd also keep shaded windows mapped, and could remove all unmap logic, but we'd need a more complex method of showing the shaded titlebar, such as using a different actor. ::: src/compositor/compositor.c @@ +834,1 @@ + meta_error_trap_push (display); I suspect this error trap is unnecessary, and it's certainly against the philosophy of Mutter to error trap over a large sequence of unknown operations. But not related, and not worth worrying about to change separately. ::: src/compositor/meta-window-actor.c @@ -1585,3 @@ - if (priv->mapped) - meta_window_actor_queue_create_x11_pixmap (self); Why was this removed, instead of always done? ::: src/core/window.c @@ +3183,3 @@ } + meta_compositor_add_window (window->display->compositor, window); This move is not really part of the patch - it's independent and should be broken out or at least called out in the commit message.
Review of attachment 267762 [details] [review]: Other than one typo, seems fine to me. This is definitely going to break extensions - maybe (thinking broader) we should have a wiki page where we document Mutter/gnome-shell API changes that we think will break extensions, or that people have found, and how to rewrite the code. ::: src/core/window.c @@ +627,3 @@ G_TYPE_NONE, 0); + + window_signals[POSITION_CHANGED] = Can you add documentation to these which gives a firm idea of when we are supposed to be emitting these signals? (Maybe when either the size/resp. position of either the frame rect or the client rect changes?) @@ +5410,3 @@ + g_signal_emit (window, window_signals[POSITION_CHANGED], 0); + + if (need_resize_client || need_resize_client) Typo, same check twice
Created attachment 268340 [details] [review] Always map the client and frame windows Traditionally, WMs unmap windows when minimizing them, and map them when restoring them or wanting to show them for other reasons, like upon creation. However, as metacity morphed into mutter, we optionally chose to keep windows mapped for the lifetime of the window under the user option "live-window-previews", which makes the code keep windows mapped so it can show window preview for minimized windows in other places, like Alt-Tab and Expose. I removed this preference two years ago mechanically, by removing all the if statements, but never went through and cleaned up the code so that windows are simply mapped for the lifetime of the window -- the "architecture" of the old code that maps and unmaps on show/hide was still there. Remove this now. The one case we still need to be careful of is shaded windows, in which we do still unmap the client window. In the future, we might want to show previews of shaded windows in the overview and Alt-Tab. In that we'd also keep shaded windows mapped, and could remove all unmap logic, but we'd need a more complex method of showing the shaded titlebar, such as using a different actor. At the same time, simplify the compositor interface by removing meta_compositor_window_[un]mapped API, and instead adding/removing the window on-demand.
Created attachment 268341 [details] [review] compositor: Delay meta_compositor_add_window until the first show In order for the compositor to properly determine whether a client is an X11 client or not, we need to wait until XWayland calls set_window_id to mark the surface as an XWayland client. To prevent the compositor from getting tripped up over this, make sure that the window has been fully initialized by the time we call meta_compositor_add_window.
Created attachment 268342 [details] [review] Move position-changed / size-changed signals to the MetaWindow They fit more appropriately over here...
Created attachment 268343 [details] [review] window-actor: Split into two subclasses of MetaSurfaceActor The rendering logic before was somewhat complex. We had three independent cases to take into account when doing rendering: * X11 compositor. In this case, we're a traditional X11 compositor, not a Wayland compositor. We use XCompositeNameWindowPixmap to get the backing pixmap for the window, and deal with the COMPOSITE extension messiness. In this case, meta_is_wayland_compositor() is FALSE. * Wayland clients. In this case, we're a Wayland compositor managing Wayland surfaces. The rendering for this is fairly straightforward, as Cogl handles most of the complexity with EGL and SHM buffers... Wayland clients give us the input and opaque regions through wl_surface. In this case, meta_is_wayland_compositor() is TRUE and priv->window->client_type == META_WINDOW_CLIENT_TYPE_WAYLAND. * XWayland clients. In this case, we're a Wayland compositor, like above, and XWayland hands us Wayland surfaces. XWayland handles the COMPOSITE extension messiness for us, and hands us a buffer like any other Wayland client. We have to fetch the input and opaque regions from the X11 window ourselves. In this case, meta_is_wayland_compositor() is TRUE and priv->window->client_type == META_WINDOW_CLIENT_TYPE_X11. We now split the rendering logic into two subclasses, which are: * MetaSurfaceActorX11, which handles the X11 compositor case, in that it uses XCompositeNameWindowPixmap to get the backing pixmap, and deal with all the COMPOSITE extension messiness. * MetaSurfaceActorWayland, which handles the Wayland compositor case for both native Wayland clients and XWayland clients. XWayland handles COMPOSITE for us, and handles pushing a surface over through the xf86-video-wayland DDX. Frame sync is still in MetaWindowActor, as it needs to work for both the X11 compositor and XWayland client cases. When Wayland's video display protocol lands, this will need to be significantly overhauled, as it would have to work for any wl_surface, including subsurfaces, so we would need surface-level discretion.
Review of attachment 268343 [details] [review]: The overall structure of the rearrangement. There are various bits and pieces that seem to have gone missing - some probably won't be missed - others likely will. ::: src/compositor/meta-surface-actor-x11.c @@ +83,3 @@ + MetaSurfaceActorX11Private *priv = meta_surface_actor_x11_get_instance_private (self); + + /* Don't do any culling for the unredirected window */ This comment could really have used expansion when you added it and the check in bug 677116 - it means something like: /* We already removed the space occupied by the unredirected window before we started * the culling process, so (as an optimization) we don't need to clip it out here again. * We also don't have to store the clip/unobscured regions since we don't care about * those for an unredirected window. */ Basically, it's just an optimization and may not be worth overriding the interface for. I'm actually a bit uncertain why we even need to special case the unredirected window in MetaWindowGroup - I'd think it would "just work" if we let the logic here handle it and removed the logic there. (Adel might have some idea.) The most conservative thing to do would be just extend the comment as above. ::: src/compositor/meta-surface-actor.c @@ +93,3 @@ +gboolean +meta_surface_actor_redraw_area (MetaSurfaceActor *self, + int x, int y, int width, int height) This doesn't seem like a good new name to me - i think redraw should be reserved for things that actual redraw - what this does is: a) *queue* a redraw b) update the texture tower Might as well call it update_area just like the MetaShapedTexture method ::: src/compositor/meta-window-actor.c @@ -277,3 @@ - if (priv->damage != None) - { - meta_error_trap_push (display); I don't think you managed to actually get the damage object destroyed and recreated - the constructor will *not* create a new surface actor since priv->surface is already non-NULL. @@ -902,3 @@ - - redraw_queued = meta_surface_actor_damage_all (priv->surface); - priv->repaint_scheduled = priv->repaint_scheduled || redraw_queued; You definitely need to keep this tracking of repaint_scheduled - right now you are always hitting the fallback code path that damages an arbitrary 1x1 pixel in the window (most likely increasing the repaint area) if no other damage happens. You can either: A) Track it in the subclasses, and add a virtualized getter B) Rework things to track it in the base class - maybe just add a meta_window_actor_set_repaint_scheduled() I think I like A) better - though either should work. @@ -999,3 @@ - -static void -meta_window_actor_queue_create_x11_pixmap (MetaWindowActor *self) What's the logic for removing this? I'd think that generally we *will get damage around the time that we need to queue pixmap creation - but the leap of faith that we always get damage and this is just unnecessary seems unrelated to this patch. @@ -1198,3 @@ - * you are supposed to be able to free a GLXPixmap after freeing the underlying - * pixmap, but it certainly doesn't work with current DRI/Mesa - */ What happened to this comment and the corresponding ordering/flush? @@ -1356,3 @@ - { - meta_window_actor_queue_create_x11_pixmap (self); - meta_window_actor_update_shape (self); What happened to this call to update_shape? @@ -1823,1 @@ - if (meta_window_is_fullscreen (priv->window) && g_list_last (info->windows)->data == self && !priv->unredirected) What happened to this conditional? You dropped the first part. @@ -1840,3 @@ - /* Drop damage event for unredirected windows */ - if (priv->unredirected) - return; What happened to this? @@ -2182,1 @@ if (is_frozen (self)) This check is duplicated inside meta_surface_actor_x11_pre_paint() @@ -2189,3 @@ - if (!meta_is_wayland_compositor ()) - { - if (priv->unredirected) What happened to this?
Review of attachment 268342 [details] [review]: Looks good
Review of attachment 268340 [details] [review]: Looks good
Review of attachment 268341 [details] [review]: ::: src/core/window.c @@ +1502,1 @@ + meta_compositor_remove_window (window->display->compositor, window); The move of this inside the conditional is not correct if the window has been hidden after being initially shown.
Still going over the review, I'll have updated patches tomorrow, but... (In reply to comment #58) > What's the logic for removing this? I'd think that generally we *will get > damage around the time that we need to queue pixmap creation - but the leap of > faith that we always get damage and this is just unnecessary seems unrelated to > this patch. I don't understand this. update_pixmap is called unconditionally in pre_paint, meaning that whenever the pixmap we have is None, we'll allocate a new one. So I don't see what ties this to Damage. We'll still have a working pixmap, even if we don't get damage.
(In reply to comment #62) > Still going over the review, I'll have updated patches tomorrow, but... > > (In reply to comment #58) > > What's the logic for removing this? I'd think that generally we *will get > > damage around the time that we need to queue pixmap creation - but the leap of > > faith that we always get damage and this is just unnecessary seems unrelated to > > this patch. > > I don't understand this. update_pixmap is called unconditionally in pre_paint, > meaning that whenever the pixmap we have is None, we'll allocate a new one. So > I don't see what ties this to Damage. We'll still have a working pixmap, even > if we don't get damage. If we don't redraw, we wont get pre_paint, and we won't update the pixmap. The code that I added the comment to is that code that queues a redraw when we need to update the pixmap - it looks like to me that in your patch you are counting on "we'll need to redraw anyways" - because, e.g., we got damage.
Comment on attachment 268340 [details] [review] Always map the client and frame windows Attachment 268340 [details] pushed as aec3edb - Always map the client and frame windows
Attachment 268341 [details] pushed as 4efe448 - compositor: Delay meta_compositor_add_window until the first show Attachment 268342 [details] pushed as a0ef7c7 - Move position-changed / size-changed signals to the MetaWindow Pushing these for now, with suggested changes. Will upload a new set of patches tomorrow.
(In reply to comment #44) > Review of attachment 267762 [details] [review]: > > Makes sense, but we use both in gnome-shell so do not forgot to update them > when landing this. Seems like you didn't.
(In reply to comment #66) > (In reply to comment #44) > > Review of attachment 267762 [details] [review] [details]: > > > > Makes sense, but we use both in gnome-shell so do not forgot to update them > > when landing this. > > Seems like you didn't. Not true you did, ignore that.
(In reply to comment #65) > Attachment 268341 [details] pushed as 4efe448 - compositor: Delay > meta_compositor_add_window until the first show This needs to be reverted, as it breaks workspace thumbnails and alt-tab for windows that were never shown because they are in a different workspace. We have a very strong assumption that get_compositor_private() never returns NULL for a valid MetaWindow.
Created attachment 269704 [details] [review] window-actor: Kill off needs_pixmap It's mostly equivalent to the case where we've already detached the pixmap, *except* for the x11_size_changed case. We can simply detach the pixmap at the time the window changes size, though.
Created attachment 269706 [details] [review] window-actor: Split into two subclasses of MetaSurfaceActor The rendering logic before was somewhat complex. We had three independent cases to take into account when doing rendering: * X11 compositor. In this case, we're a traditional X11 compositor, not a Wayland compositor. We use XCompositeNameWindowPixmap to get the backing pixmap for the window, and deal with the COMPOSITE extension messiness. In this case, meta_is_wayland_compositor() is FALSE. * Wayland clients. In this case, we're a Wayland compositor managing Wayland surfaces. The rendering for this is fairly straightforward, as Cogl handles most of the complexity with EGL and SHM buffers... Wayland clients give us the input and opaque regions through wl_surface. In this case, meta_is_wayland_compositor() is TRUE and priv->window->client_type == META_WINDOW_CLIENT_TYPE_WAYLAND. * XWayland clients. In this case, we're a Wayland compositor, like above, and XWayland hands us Wayland surfaces. XWayland handles the COMPOSITE extension messiness for us, and hands us a buffer like any other Wayland client. We have to fetch the input and opaque regions from the X11 window ourselves. In this case, meta_is_wayland_compositor() is TRUE and priv->window->client_type == META_WINDOW_CLIENT_TYPE_X11. We now split the rendering logic into two subclasses, which are: * MetaSurfaceActorX11, which handles the X11 compositor case, in that it uses XCompositeNameWindowPixmap to get the backing pixmap, and deal with all the COMPOSITE extension messiness. * MetaSurfaceActorWayland, which handles the Wayland compositor case for both native Wayland clients and XWayland clients. XWayland handles COMPOSITE for us, and handles pushing a surface over through the xf86-video-wayland DDX. Frame sync is still in MetaWindowActor, as it needs to work for both the X11 compositor and XWayland client cases. When Wayland's video display protocol lands, this will need to be significantly overhauled, as it would have to work for any wl_surface, including subsurfaces, so we would need surface-level discretion.
Created attachment 269708 [details] [review] window-actor: Don't queue a redraw when queueing a new pixmap We guarantee ourselves that a valid pixmap will appear any time that the window is painted. The window actor will be scheduled for a repaint if it's added / removed from the scene graph, like during construction, if the size changes, or if we receive damage, which are the existing use cases where this function is called. So, I can't see any reason that we queue a redraw in here.
Created attachment 269709 [details] [review] window-actor: Kill off needs_pixmap It's mostly equivalent to the case where we've already detached the pixmap, *except* for the x11_size_changed case. We can simply detach the pixmap at the time the window changes size, though.
Created attachment 269710 [details] [review] window-actor: Split into two subclasses of MetaSurfaceActor The rendering logic before was somewhat complex. We had three independent cases to take into account when doing rendering: * X11 compositor. In this case, we're a traditional X11 compositor, not a Wayland compositor. We use XCompositeNameWindowPixmap to get the backing pixmap for the window, and deal with the COMPOSITE extension messiness. In this case, meta_is_wayland_compositor() is FALSE. * Wayland clients. In this case, we're a Wayland compositor managing Wayland surfaces. The rendering for this is fairly straightforward, as Cogl handles most of the complexity with EGL and SHM buffers... Wayland clients give us the input and opaque regions through wl_surface. In this case, meta_is_wayland_compositor() is TRUE and priv->window->client_type == META_WINDOW_CLIENT_TYPE_WAYLAND. * XWayland clients. In this case, we're a Wayland compositor, like above, and XWayland hands us Wayland surfaces. XWayland handles the COMPOSITE extension messiness for us, and hands us a buffer like any other Wayland client. We have to fetch the input and opaque regions from the X11 window ourselves. In this case, meta_is_wayland_compositor() is TRUE and priv->window->client_type == META_WINDOW_CLIENT_TYPE_X11. We now split the rendering logic into two subclasses, which are: * MetaSurfaceActorX11, which handles the X11 compositor case, in that it uses XCompositeNameWindowPixmap to get the backing pixmap, and deal with all the COMPOSITE extension messiness. * MetaSurfaceActorWayland, which handles the Wayland compositor case for both native Wayland clients and XWayland clients. XWayland handles COMPOSITE for us, and handles pushing a surface over through the xf86-video-wayland DDX. Frame sync is still in MetaWindowActor, as it needs to work for both the X11 compositor and XWayland client cases. When Wayland's video display protocol lands, this will need to be significantly overhauled, as it would have to work for any wl_surface, including subsurfaces, so we would need surface-level discretion.
I fixed most of the reivew comments. I did not fix frame sync, even after fiddling with it for a few hours. I'm going to let it slide for now, and I'll fix it when I choose to implement pq's timing protocol, since there's plenty of open questions (at least to me) about how to behave.
Review of attachment 269708 [details] [review]: This logic came from https://bugzilla.gnome.org/show_bug.cgi?id=587251 - https://git.gnome.org/browse/mutter/commit/?id=9244f0f1 I think the basic idea was that when replacing "make changes to an actor immediately" with "make changes to the actor in a paint function" it's more robust and as efficient to simply queue a redraw on the actor than to prove that in every current and future call chain the actor is queued for a redraw. But I do agree that in the places where it is called currently, it's redundant. (And of course, the worse that can happen is that nothing is redrawn until something else happens - incorrect drawing can't occcur.) Can you add to the comment an "In preparation for ..." that explains why in the future queueing a redraw will be difficult. Otherwise, fine to commit.
Review of attachment 269709 [details] [review]: I don't think this patch works. ::: src/compositor/meta-window-actor.c @@ +1330,3 @@ if (priv->x11_size_changed) { + meta_window_actor_detach_x11_pixmap (self); This means that if a frozen window changes size, it will paint blank until unfrozen. This is not good.
Comment on attachment 269708 [details] [review] window-actor: Don't queue a redraw when queueing a new pixmap Attachment 269708 [details] pushed as bc79259 - window-actor: Don't queue a redraw when queueing a new pixmap
Review of attachment 269709 [details] [review]: Missed the is_frozen() above that, so it should be OK - otherwise looks good to commit except for one comment. ::: src/compositor/meta-window-actor.c @@ +1663,3 @@ Window xwindow = meta_window_get_toplevel_xwindow (priv->window); + /* This can happen while we're frozen. */ This comment is confusing to me - I think what you mean is something like: /* If the size changed while we were frozen, the old pixmap will still be attached */ (size_changed == TRUE can also happen in other cases - it's just that the detach() will be a no-op)
Review of attachment 269710 [details] [review]: I haven't followed everything from scratch, but spot-checking against what I pointed out last time, it looks good other than the issue of recreating the damage object when a window is redecorated.
Attachment 269709 [details] pushed as 0b055fa - window-actor: Kill off needs_pixmap Attachment 269710 [details] pushed as 83aca0b - window-actor: Split into two subclasses of MetaSurfaceActor Thanks for the review! I went ahead and made the two changes and fixed frame-sync.