GNOME Bugzilla – Bug 744453
wayland: Send wl_surface.enter and wl_surface.leave
Last modified: 2015-07-15 09:12:01 UTC
Support sending information about what output the surface is displayed on. So far only supports top level surfaces and subsurfaces, and potential clones are completely ignored. These patches depend on the patches in https://bugzilla.gnome.org/show_bug.cgi?id=744452 .
Created attachment 296744 [details] [review] wayland: Put MetaWaylandOutput struct in header file We need this in MetaWaylandSurface to be able to send wl_surface.enter/leave.
Created attachment 296745 [details] [review] wayland: Send wl_surface.enter and wl_surface.leave when moving window Current known issues: * Resizing a window doesn't send enter/leave events accordingly * Attaching subsurfaces after the parent window's first position event causes those subsurfaces not to receive enter/leave events after being mapped. * Clones are ignored. * Cursor and DND surfaces not supported.
Created attachment 296746 [details] [review] wayland: Use surface role when special casing surface commits
Created attachment 296747 [details] [review] wayland: Send wl_surface.enter/leave when attaching buffer If the surface is mapped, send wl_surface.enter/leave depending on what output the surface is one, given the new buffer size.
Created attachment 296748 [details] [review] wayland: Send wl_surface.enter/leave after creating xdg surface After having created an xdg surface, update the surface output list and send out wl_surface.enter/leave accordingly.
Review of attachment 296745 [details] [review]: ::: src/wayland/meta-wayland-surface.c @@ +768,3 @@ + { + g_hash_table_remove (surface->outputs, wayland_output); + surface_left_output (surface, wayland_output); Do you need to remove the output_destroy_listener from the signal here?
Review of attachment 296745 [details] [review]: ::: src/wayland/meta-wayland-surface.c @@ +768,3 @@ + { + g_hash_table_remove (surface->outputs, wayland_output); + surface_left_output (surface, wayland_output); It's removed implicitly by g_hash_table_remove (surface->outputs, wayland_output); as the hash table remove function both removes the listener and frees the memory. Might help to add a comment about that I suppose.
Created attachment 298597 [details] [review] wayland: Send wl_surface.enter and wl_surface.leave when moving window Whenever the 'position' parameter signal of the window actor is emitted update a list of outputs the surface is on and notify the client which outputs was entered and left. The 'position' parameter signal is emitted when the window actor is moved or resized Current known issues: * Clones are ignored. * Cursor and DND surfaces not supported.
Created attachment 298598 [details] [review] wayland: Send wl_surface.enter/leave when attaching buffer If the surface is mapped, send wl_surface.enter/leave depending on what output the surface is one, given the new buffer size.
Review of attachment 296748 [details] [review]: This patch is not needed any more.
Attached a simplified series that doesn't calculate the positions itself, but relies on clutter for doing that.
Created attachment 298600 [details] [review] wayland: Send wl_surface.enter and wl_surface.leave when moving window Whenever the 'position' parameter signal of the window actor is emitted update a list of outputs the surface is on and notify the client which outputs was entered and left. The 'position' parameter signal is emitted when the window actor is moved or resized Current known issues: * Clones are ignored. * Cursor and DND surfaces not supported. --- Changes since the recently uploaded patch is that a weak ref is kept to the window actor while listening to the signal; the window actor had already removed itself from the window but could still move because of effects.
Review of attachment 296746 [details] [review]: Is this related to the patch series? Can you include a more informative commit message that gives a motiviation for the change?
Review of attachment 298598 [details] [review]: it looks like either the patches were attached out of order or this patch doesn't compile without the next patch.
(In reply to Owen Taylor from comment #14) > Review of attachment 298598 [details] [review] [review]: > > it looks like either the patches were attached out of order or this patch > doesn't compile without the next patch. The order I have them on my branch is: wayland: Put MetaWaylandOutput struct in header file wayland: Send wl_surface.enter and wl_surface.leave when moving window wayland: Use surface role when special casing surface commits wayland: Send wl_surface.enter/leave when attaching buffer Can I change the order of attached patches? The order became a bit mangled after I attached a new version of a couple of them I think.
Created attachment 299777 [details] [review] wayland: Use surface role when special casing surface commits Lets use the role when doing role specific commit actions. The conditions effectively do that anyway, and this way we will get a compiler warning here whenever we add a new role, as well as we avoid having different variants of role-determination checks in different places.
(In reply to Owen Taylor from comment #13) > Review of attachment 296746 [details] [review] [review]: > > Is this related to the patch series? Can you include a more informative > commit message that gives a motiviation for the change? It is not really related, but was part of an earlier version of the series, but since the change itself makes sense I kept it around. I attached a new version with a better commit message.
Review of attachment 299777 [details] [review]: ::: src/wayland/meta-wayland-surface.c @@ +491,3 @@ + case META_WAYLAND_SURFACE_ROLE_NONE: + + break; Remove the funky whitespace before breaks, otherwise fine.
Created attachment 299780 [details] [review] wayland: Use surface role when special casing surface commits Lets use the role when doing role specific commit actions. The conditions effectively do that anyway, and this way we will get a compiler warning here whenever we add a new role, as well as we avoid having different variants of role-determination checks in different places.
Review of attachment 299780 [details] [review]: good.
Review of attachment 296744 [details] [review]: Seems OK
Review of attachment 298600 [details] [review]: The code looks like it should basically work, and I've added some small-scale comments below, but I also have some big-picture questions: I think the thing that I'm most uncertain about here is tracking what outputs a surface is on by looking at the actor. It has the potential advantage of automatically handling subsurfaces, though that doesn't seem to be handled by your patch? But there are some issues as well: First, it's pretty hard to get notification when the set of pixels an actor touches changes - notify::position will catch some cases, but it isn't emitted when the transform of an actor or a parent changes, and I don't think it will even be emitted when the position of a parent actor changes - what sort of testing did you do with subsurfaces? I think the most reliably way to keep track of what outputs a surface is *actually* painted on is to check at paint time. Secondly, I'm not sure we actually want clones (which you mark as unhandled in your commit message) or animated effects to cause rerendering of an application - a thumbnail on a high-resolution output shouldn't cause an app to rerender itself at a high resolution. I think the idea of "on an output" really is meant the outputs that a window touches, permanently, and unscaled. So, I'd think that it would be better to base the set of outputs off of window->rect - which has a simple interpretation and is fully under the control of the mutter code. To handle subsurfaces will mean iterating over subsurfaces at various places, but that doesn't seem too hard. Consider whether this should be handled as something queued to be done at META_LATER_BEFORE_REDRAW - trying to do it synchronously seems like it might have issues, for, say, attaching a new buffer with a dx/dy - resizing a window that is on the edge of the screen from the left might cause us to send enter/leave signals for the next monitor over. What about changes in output configuration? ::: src/compositor/meta-surface-actor-wayland.c @@ +208,3 @@ + .y = (int)(y + 0.5f), + .width = (int)(width + 0.5f), + .height = (int)(height + 0.5f), Generally speaking, this is not the right way to round rectangles - e.g: x=0.7,y=0.7,width=1.6,height=1.6 is rounded to: x=1,y=1,width=2,height=2 Which extends 0.7 to the right of the original rectangle. It's better to round the bounding coordinates. (This also preserves tiling of adjacent rectangles). actor_rect.x = (int)(0.5 + x); actor_rect.y = (int)(0.5 + y); actor_rect.width = (int)(0.5 + x + width) - actor_rect.x; actor_rect.height = (int)(0.5 + y + height) - actor_rect.y; Also possible is to do a bounding box in integral coordinates to get all affected pixels, but I think that's worse here, since we'd prefer a window that is only 0.001f onto a monitor through a rounding error to be *not* counted as on the monitor. I say this not because it matters a lot here (we expect actors to be nearly pixel aligned), but because rounding rectangles like above should set off warning bells in a graphics programmer's head. The other caution is that (int)(x + 5) for round(x) only works for positive x. Here we can be sure that the coordinates are positive, so it's not a concern, but often in graphics that isn't the case. @@ +227,3 @@ + cairo_region_destroy (region); + + return !cairo_region_is_empty (region); Use after free. ::: src/wayland/meta-wayland-outputs.c @@ +101,3 @@ GList *resources; + wl_signal_emit (&wayland_output->destroy_signal, wayland_output); Not crazy about talking to ourselves via the libwayland signal mechanism. Definitely inconsistent with the rest of Mutter and makes it harder for someone learning or maintaining the code. But I know that MetaOutput => GObject conversion is quite a bit further down your patch pile - so not worth a lot of rebasing to avoid this. But maybe it's easier to just iterate through all surfaces and subsurfaces when an output is destroyed? - the code for handling output destruction handling in meta-wayland-surface.c is not easy to follow - though it seems correct as far as I can determine. ::: src/wayland/meta-wayland-surface.c @@ +820,3 @@ + MetaWaylandSurface *surface; + cairo_rectangle_int_t buffer_rect; +} UpdateOutputStateData; You don't use this? @@ +880,3 @@ + g_signal_handlers_disconnect_by_func ( + surface->window_actor, + G_CALLBACK (window_actor_position_changed), Weird historical thing is that g_signal_handlers_disconnect_by_func() doesn't take a GCallback, but rather a gpointer. Casts between void * and function pointers aren't allowed *at all* in standard C, whether implicit or explicit, but our standard is to write (gpointer)window_actor_position_changed - there's no point in casting to GCallback in any case. @@ +968,3 @@ surface->surface_actor = g_object_ref_sink (meta_surface_actor_wayland_new (surface)); + surface->outputs = g_hash_table_new_full (NULL, NULL, Huh, I thought you had to write g_direct_hash, NULL, but looking at glib, I see that support for NULL, NULL was slipped in only, uh, 17 years ago. :-)
Review of attachment 298598 [details] [review]: See my comment about queueing surface output updates on the big patch - this in particular is a case where we need to do the update for the move and the attach atomically.
(In reply to Owen Taylor from comment #23) > Review of attachment 298600 [details] [review] [review]: > > The code looks like it should basically work, and I've added some > small-scale comments below, but I also have some big-picture questions: > > I think the thing that I'm most uncertain about here is tracking what > outputs a surface is on by looking at the actor. It has the potential > advantage of automatically handling subsurfaces, though that doesn't seem to > be handled by your patch? But there are some issues as well: Subsurfaces are handled already. > > First, it's pretty hard to get notification when the set of pixels an actor > touches changes - notify::position will catch some cases, but it isn't > emitted when the transform of an actor or a parent changes, and I don't > think it will even be emitted when the position of a parent actor changes - > what sort of testing did you do with subsurfaces? I think the most reliably > way to keep track of what outputs a surface is *actually* painted on is to > check at paint time. I tested with the subsurface demo from weston (requires a couple of patches to weston to make it start) and it works (moving and resizing). > > Secondly, I'm not sure we actually want clones (which you mark as unhandled > in your commit message) or animated effects to cause rerendering of an > application - a thumbnail on a high-resolution output shouldn't cause an app > to rerender itself at a high resolution. I think the idea of "on an output" > really is meant the outputs that a window touches, permanently, and unscaled. Technically, "on an output" means when any part of the surface is displayed on that output. There is nothing about "windows" in that part of the Wayland protocol. weston interprets this as "any view" (i.e. clone and main actor for us), but I think its fine to interpret it as "only main view" as well, so sure lets ignore clones for now. > > So, I'd think that it would be better to base the set of outputs off of > window->rect - which has a simple interpretation and is fully under the > control of the mutter code. To handle subsurfaces will mean iterating over > subsurfaces at various places, but that doesn't seem too hard. Iterating over subsurfaces is what this patch does already. Anyway, using window_rect would mean we don't follow the spec. We could use the surface rect though, calculating the toplevel position from the window though. > > Consider whether this should be handled as something queued to be done at > META_LATER_BEFORE_REDRAW - trying to do it synchronously seems like it might > have issues, for, say, attaching a new buffer with a dx/dy - resizing a > window that is on the edge of the screen from the left might cause us to > send enter/leave signals for the next monitor over. Doing it just before redraw is probably better since we'd indeed avoid the potential temporary invalid state if one attached with a (dx, dy). (Though I don't think we ever do attach with (dx, dy) that is not (0, 0), not in GTK+ at least). > > What about changes in output configuration? Good point. Probably not handled properly (at least when moving outputs). Queing an update before redraw should fix that I suppose.
Review of attachment 298600 [details] [review]: ::: src/compositor/meta-surface-actor-wayland.c @@ +208,3 @@ + .y = (int)(y + 0.5f), + .width = (int)(width + 0.5f), + .height = (int)(height + 0.5f), > Here we can be sure that the coordinates are positive, so it's not a concern, but often > in graphics that isn't the case. Actually they can be negative (I suppose, since a surface can have its (x, y) outside of the screen), so its even more wrong than you say :P but yea, silly mistake. ::: src/wayland/meta-wayland-outputs.c @@ +101,3 @@ GList *resources; + wl_signal_emit (&wayland_output->destroy_signal, wayland_output); I don't think I have a patch that GObject:ifies MetaOutput. Anyway, not a fan of having output find all surfaces to notify itself, doesn't feel like its the job of the output. I'd prefer GObject:ifying MetaOutput more than not having a destroy signal. ::: src/wayland/meta-wayland-surface.c @@ +820,3 @@ + MetaWaylandSurface *surface; + cairo_rectangle_int_t buffer_rect; +} UpdateOutputStateData; Nope. Is a left over. Consider it gone.
Created attachment 301966 [details] [review] wayland: Make MetaWaylandOutput a GObject This way we can later add signals to it.
Created attachment 301967 [details] [review] wayland: Send wl_surface.enter and wl_surface.leave Whenever a MetaSurfaceActor is painted, update the list of what outputs the surface is being drawed upon. Since we do this on paint, we effectively avoids this whenever the surface is not drawn, for example being minimized, on a non-active workspace, or simply outside of the damage region of a frame. Clones, DND icons and pointer cursors are not affected by this patch, as they are not drawn as MetaSurfaceActor's. ------ What do you think of this approach? We completely avoid need to queue a recalculation every time some aspect of the surface change and rely on the repaint mechanics to do it for us. Downsides would be unnecessary recalculations when something causse the surface to be painted (cursor moving above, ...), or missed recalculations if it was culled out. I still use the clutter scene graph to calculate whether a surface is on a CRTC, but if you still prefer to calculate the surface rect from the corresponding toplevel window position, I can do that.
Review of attachment 301966 [details] [review]: Looks fine other than one minor style point. ::: src/wayland/meta-wayland-outputs.c @@ +167,3 @@ + + object = g_object_new (META_TYPE_WAYLAND_OUTPUT, NULL); + wayland_output = META_WAYLAND_OUTPUT (object); g_object_new() returns a void * rather than a GObject to avoid having to write this type of thing.
Review of attachment 301967 [details] [review]: I'm OK with this approach. The big advantage of it is simplicity and that we're *sure* that it works. There is no way for a surface to be painted on a new output without it being painted :-) The disadvantages are: - The notifications occur depending on the details of what the compositor is doing, so it might be a little confusing to someone debugging an application - There a non-trivial amount of work for every surface that we paint to retransform coordinates. If clutter can't cull and paints the entire scene, then we need to do the work of computing the actors transform for [each surface] x [1 + number of clones] x [number of monitors]. I honestly have no intuition about whether this matters. The overviewRedrawTime() stat from gnome-shell/js/perf/hwtest.js would be the interesting before/after, but getting the perf framework to work under wayland would be a project in itself. Let's hope for the best :-) I'm not sure what you mean by "Clones" in the commit message - was that meant to be something else? In terms of clones: It seems like meta_wayland_surface_update_outputs() will be called for every clone of the actor - so e.g., when in the GNOME Shell overview, it will be called once for the main view of the surface and once for the view in the workspace thumbnail. clutter_actor_is_in_clone_paint() doesn't work since it isn't recursive, so there's no obvious way to avoid this without a clutter API addition that exposes clutter-actor.c:in_clone_paint(). ::: src/compositor/meta-surface-actor-wayland.c @@ +220,3 @@ + .width = crtc->rect.width, + .height = crtc->rect.height, + })) != CAIRO_STATUS_SUCCESS) The general cairo error handling strategy is try to make code that ignores errors do *something* and be correct for memory handling - statuses are really for language bindings which can throw exceptions. Here if the return status is NO_MEMORY, then that status will be set on the region. Calling cairo_region_is_empty() on a region with a status set returns TRUE, so it would end up doing the same thing as if you had omitted doing the error handling. I don't think we *really* care whether runnning out of memory at this point makes us decide that the window is on all monitors or no monitors - either is likely to have serious side effects (and points out the ridiculousess of trying to handle OOM in such a fine-grained fashion), so I'd suggest omitting the check, as we do elsewhere in Mutter. ::: src/wayland/meta-wayland-outputs.c @@ +242,3 @@ GList *resources; + g_signal_emit (wayland_output, signals[OUTPUT_DESTROYED], 0); This isn't going to work - finalize() is called *after* signals are disconnected. This needs to be done in wayland_output_destroy_notify(). Once you fix this, can you figure out how to test the code path? @@ +263,3 @@ + signals[OUTPUT_DESTROYED] = g_signal_new ("output-destroyed", + G_TYPE_FROM_CLASS (object_class), + G_SIGNAL_RUN_FIRST, Minor style point is that G_SIGNAL_RUN_FIRST should be avoided because it breaks g_signal_connect_after(). It doesn't matter here since there is no default handler, but I'd suggest just always writing G_SIGNAL_RUN_LAST. ::: src/wayland/meta-wayland-surface.c @@ +790,3 @@ + { + g_signal_connect (wayland_output, "output-destroyed", + G_CALLBACK (surface_handle_output_destroy), See note in a previous review comment about using (gpointer) here
(In reply to Owen Taylor from comment #30) > Review of attachment 301967 [details] [review] [review]: > > I'm OK with this approach. The big advantage of it is simplicity and that > we're *sure* that it works. There is no way for a surface to be painted on a > new output without it being painted :-) The disadvantages are: > > - The notifications occur depending on the details of what the compositor > is doing, so it might be a little confusing to someone debugging an > application > - There a non-trivial amount of work for every surface that we paint to > retransform coordinates. If clutter can't cull and paints the entire scene, > then we need to do the work of computing the actors transform for [each > surface] x [1 + number of clones] x [number of monitors]. I honestly have no > intuition about whether this matters. The overviewRedrawTime() stat from > gnome-shell/js/perf/hwtest.js would be the interesting before/after, but > getting the perf framework to work under wayland would be a project in > itself. Let's hope for the best :-) > > I'm not sure what you mean by "Clones" in the commit message - was that > meant to be something else? No, I meant clones as in clutter screne graph clones. They are not handled as in if a clone is on output 1 but the real actor is only on output 2, it'll be considered only part of output 2. As you mentioned, it might be necessary to handle that in that way, but the commit message only describes the fact that clones won't affect what output the wl_surface is considered being on. > > In terms of clones: It seems like meta_wayland_surface_update_outputs() > will be called for every clone of the actor - so e.g., when in the GNOME > Shell overview, it will be called once for the main view of the surface and > once for the view in the workspace thumbnail. > > clutter_actor_is_in_clone_paint() doesn't work since it isn't recursive, so > there's no obvious way to avoid this without a clutter API addition that > exposes clutter-actor.c:in_clone_paint(). Couldn't we just check clutter_actor_is_in_clone_paint on parents? If there is a parent anywhere higher up in the tree, then skip updating. > ::: src/wayland/meta-wayland-surface.c > @@ +790,3 @@ > + { > + g_signal_connect (wayland_output, "output-destroyed", > + G_CALLBACK (surface_handle_output_destroy), > > See note in a previous review comment about using (gpointer) here IIRC that was about disconnecting, which does indeed take a gpointer. The connect function does, however, take a "GCallback". AFAICS every time a signal is connected in the mutter source tree, it's casted the same way as above. Are you sure it should be casted to a gpointer?
(In reply to Jonas Ådahl from comment #31) > (In reply to Owen Taylor from comment #30) > > > > > In terms of clones: It seems like meta_wayland_surface_update_outputs() > > will be called for every clone of the actor - so e.g., when in the GNOME > > Shell overview, it will be called once for the main view of the surface and > > once for the view in the workspace thumbnail. > > > > clutter_actor_is_in_clone_paint() doesn't work since it isn't recursive, so > > there's no obvious way to avoid this without a clutter API addition that > > exposes clutter-actor.c:in_clone_paint(). > > Couldn't we just check clutter_actor_is_in_clone_paint on parents? If there > is a parent anywhere higher up in the tree, then skip updating. > I tried adding some logging and for what I can see, with this patch, for every stage paint, every MetaSurfaceActor was painted only once (i.e. update_output was invoked only once) for every frame, even when showing the alt-tab popup or the overview workspace preview. Are you sure that clutter invokes the actual paint for all the actors in the tree even when drawing a cloned (MetaWindowActor) actor?
(In reply to Jonas Ådahl from comment #31) > (In reply to Owen Taylor from comment #30) > > Review of attachment 301967 [details] [review] [review] [review]: > > > > I'm OK with this approach. The big advantage of it is simplicity and that > > we're *sure* that it works. There is no way for a surface to be painted on a > > new output without it being painted :-) The disadvantages are: > > > > - The notifications occur depending on the details of what the compositor > > is doing, so it might be a little confusing to someone debugging an > > application > > - There a non-trivial amount of work for every surface that we paint to > > retransform coordinates. If clutter can't cull and paints the entire scene, > > then we need to do the work of computing the actors transform for [each > > surface] x [1 + number of clones] x [number of monitors]. I honestly have no > > intuition about whether this matters. The overviewRedrawTime() stat from > > gnome-shell/js/perf/hwtest.js would be the interesting before/after, but > > getting the perf framework to work under wayland would be a project in > > itself. Let's hope for the best :-) > > > > I'm not sure what you mean by "Clones" in the commit message - was that > > meant to be something else? > > No, I meant clones as in clutter screne graph clones. They are not handled > as in if a clone is on output 1 but the real actor is only on output 2, > it'll be considered only part of output 2. As you mentioned, it might be > necessary to handle that in that way, but the commit message only describes > the fact that clones won't affect what output the wl_surface is considered > being on. Then your commit message needs improvement: "Clones, DND icons and pointer cursors are not affected by this patch, as they are not drawn as MetaSurfaceActor's" - clones are definitely drawn as MetaSurfaceActor. > > In terms of clones: It seems like meta_wayland_surface_update_outputs() > > will be called for every clone of the actor - so e.g., when in the GNOME > > Shell overview, it will be called once for the main view of the surface and > > once for the view in the workspace thumbnail. > > > > clutter_actor_is_in_clone_paint() doesn't work since it isn't recursive, so > > there's no obvious way to avoid this without a clutter API addition that > > exposes clutter-actor.c:in_clone_paint(). > > Couldn't we just check clutter_actor_is_in_clone_paint on parents? If there > is a parent anywhere higher up in the tree, then skip updating. I don't think we should, to avoid walking all the way up the tree accumulating translation matrices, walk all the way up the tree checking clutter_actor_is_in_clone_paint(). Yes, it might be a bit faster, but it's still doing a similar amount of work. > > ::: src/wayland/meta-wayland-surface.c > > @@ +790,3 @@ > > + { > > + g_signal_connect (wayland_output, "output-destroyed", > > + G_CALLBACK (surface_handle_output_destroy), > > > > See note in a previous review comment about using (gpointer) here > > IIRC that was about disconnecting, which does indeed take a gpointer. The > connect function does, however, take a "GCallback". AFAICS every time a > signal is connected in the mutter source tree, it's casted the same way as > above. Are you sure it should be casted to a gpointer? Yeah, just ignore my comment - I was reading too fast.
(In reply to Jonas Ådahl from comment #32) > > I tried adding some logging and for what I can see, with this patch, for > every stage paint, every MetaSurfaceActor was painted only once (i.e. > update_output was invoked only once) for every frame, even when showing the > alt-tab popup or the overview workspace preview. Are you sure that clutter > invokes the actual paint for all the actors in the tree even when drawing a > cloned (MetaWindowActor) actor? I'm quite sure - in what other way would clones get painted? There are reasons an actor might not get painted for a particular frame - culling of actors outside a partial fame paint, or in some cases when effects are applied to a portion of a tree, caching in an offscreen framebuffer, but there is no way to share painting of an actor between different portions of the scene graph - potentially with different scales and rotations. You can try CLUTTER_DEBUG=disable-culling and see if that makes the debug output clearer.
(In reply to Owen Taylor from comment #34) > (In reply to Jonas Ådahl from comment #32) > > > > I tried adding some logging and for what I can see, with this patch, for > > every stage paint, every MetaSurfaceActor was painted only once (i.e. > > update_output was invoked only once) for every frame, even when showing the > > alt-tab popup or the overview workspace preview. Are you sure that clutter > > invokes the actual paint for all the actors in the tree even when drawing a > > cloned (MetaWindowActor) actor? > > I'm quite sure - in what other way would clones get painted? > > There are reasons an actor might not get painted for a particular frame - > culling of actors outside a partial fame paint, or in some cases when > effects are applied to a portion of a tree, caching in an offscreen > framebuffer, but there is no way to share painting of an actor between > different portions of the scene graph - potentially with different scales > and rotations. > > You can try CLUTTER_DEBUG=disable-culling and see if that makes the debug > output clearer. CLUTTER_PAINT=disable-culling has no effect. I did discover that while meta_surface_actor_wayland_paint only gets called once per frame, meta_shaped_texture_paint gets called multiple times when there are clones. AFAICS gnome-shell creates it clones from the MetaWindowActor, but the surface actor gets skipped anyway for some reason. It'd be good to know why this happens, but I haven't looked any further yet.
Created attachment 306584 [details] [review] wayland: Make MetaWaylandOutput a GObject This way we can later add signals to it.
Created attachment 306585 [details] [review] wayland: Send wl_surface.enter and wl_surface.leave Whenever a MetaSurfaceActor is painted, update the list of what outputs the surface is being drawed upon. Since we do this on paint, we effectively avoids this whenever the surface is not drawn, for example being minimized, on a non-active workspace, or simply outside of the damage region of a frame. Actor clones won't affect what outputs the wl_surface is notified on having entered or leaved. Sending enter/leave events to DND icon and pointer cursor surfaces are not implemented in this patch. --- This new version does nothing regarding _paint being called more than once. It only fixes the other brought up review issues.
Created attachment 306586 [details] [review] wayland: Don't crash if wl_output resource is destroyed after being removed Previously a MetaWaylandOutput could be removed from the current outputs table (by being unplugged for example). This would result in the global object being removed and the MetaWaylandOutput instance free:ed, but the wl_resource destructor would still try to remove itself from the list of resources. Trying to do this, it'd try to access its user data pointer which would point to the free:ed MetaWaylandOutput instance, and as a result crash when trying to manipulate the free:ed data. ----- This fix is just semi related. It looks like it could crash prior to the other patches here; the difference mostly being it'd access a free:ed slice before, and a free:ed object now.
Review of attachment 306584 [details] [review]: Looks good
(In reply to Jonas Ådahl from comment #35) > CLUTTER_PAINT=disable-culling has no effect. I did discover that while > meta_surface_actor_wayland_paint only gets called once per frame, > meta_shaped_texture_paint gets called multiple times when there are clones. > AFAICS gnome-shell creates it clones from the MetaWindowActor, but the > surface actor gets skipped anyway for some reason. > > It'd be good to know why this happens, but I haven't looked any further yet. Ah, yeah, looking at what we clone: Workspace thumbnail: the MetaWindowActor Main overview windows: the MetaShapedTexture Alt-tab: the MetaShapedTexture So that's why you are getting it called exactly once in the overview, even though there are no visible non-clones. I think the idea of cloning the MetaWindowActor for the workspace thumbnail was to get the shadow, though obviously that's not going to be *very* visible, and matters less as we get more client-side-shadows. (Who draws the shadow for Xwayland windows?) I'm not particularly worried about the clone issue; with that respect, can you just clarify the commit message: > Clones, DND icons and pointer cursors are not affected by this patch, as > they are not drawn as MetaSurfaceActor's. DND icons and cursors are not affected by this patch, since they are not drawn as MetaSurfaceActors. If a MetaSurfaceActor or a parent is cloned, then we'll check the position of the original actor again when the clone is drawn, which is slightly expensive, but harmless. If the MetaShapedTexture instead is cloned, as GNOME Shell does in many cases, then these clones will not cause duplicate position checks.
Review of attachment 306585 [details] [review]: Looks fine, see separate note about commit message update.
(In reply to Owen Taylor from comment #41) > Review of attachment 306585 [details] [review] [review]: > > Looks fine, see separate note about commit message update. There is one issue I discovered with this approach. We still paint the actor after the MetaWaylandSurface is destroyed (the fade out effect), so need to unset the priv->surface pointer when the MetaWaylandSurface is destroyed. I'll make that change and upload a new version. I think that sooner or later its probably useful to make more MetaWayland* objects GObjects but I think that can be done after having landed this.
(In reply to Owen Taylor from comment #34) > (In reply to Jonas Ådahl from comment #32) > > > > I tried adding some logging and for what I can see, with this patch, for > > every stage paint, every MetaSurfaceActor was painted only once (i.e. > > update_output was invoked only once) for every frame, even when showing the > > alt-tab popup or the overview workspace preview. Are you sure that clutter > > invokes the actual paint for all the actors in the tree even when drawing a > > cloned (MetaWindowActor) actor? > > I'm quite sure - in what other way would clones get painted? > > There are reasons an actor might not get painted for a particular frame - > culling of actors outside a partial fame paint, or in some cases when > effects are applied to a portion of a tree, caching in an offscreen > framebuffer, but there is no way to share painting of an actor between > different portions of the scene graph - potentially with different scales > and rotations. > > You can try CLUTTER_DEBUG=disable-culling and see if that makes the debug > output clearer. (In reply to Owen Taylor from comment #40) > (In reply to Jonas Ådahl from comment #35) > > > CLUTTER_PAINT=disable-culling has no effect. I did discover that while > > meta_surface_actor_wayland_paint only gets called once per frame, > > meta_shaped_texture_paint gets called multiple times when there are clones. > > AFAICS gnome-shell creates it clones from the MetaWindowActor, but the > > surface actor gets skipped anyway for some reason. > > > > It'd be good to know why this happens, but I haven't looked any further yet. > > Ah, yeah, looking at what we clone: > > Workspace thumbnail: the MetaWindowActor > Main overview windows: the MetaShapedTexture > Alt-tab: the MetaShapedTexture > > So that's why you are getting it called exactly once in the overview, even > though there are no visible non-clones. Ah, I see. So I'm seing the _paint via the workspace preview, and the overview is just the texture. That explains it. Without checking, I just assumed the actual windows actors were just transformed or something. Thanks for the gnome-shell insight. > > I think the idea of cloning the MetaWindowActor for the workspace thumbnail > was to get the shadow, though obviously that's not going to be *very* > visible, and matters less as we get more client-side-shadows. (Who draws the > shadow for Xwayland windows?) AFAIK mutter does that in the same way as when in X. xterm for example has shadows when shown via Xwayland, so we draw the frame, and add the shadow like normal. Not sure whether GTK+ clients do something smart though.
Created attachment 306677 [details] [review] MetaSurfaceActorWayland: Unset the MetaWaylandSurface pointer when it goes away We may access it during painting even if it has been free:ed. For now, manually unset it during the MetaWaylandSurface cleanup; in the future make MetaWaylandSurface a GObject and make the surface pointer a weak reference. --- The unsetting as a separate commit. We seem to access the scale potentially via free:d memory.
Created attachment 306678 [details] [review] wayland: Send wl_surface.enter and wl_surface.leave Whenever a MetaSurfaceActor is painted, update the list of what outputs the surface is being drawed upon. Since we do this on paint, we effectively avoids this whenever the surface is not drawn, for example being minimized, on a non-active workspace, or simply outside of the damage region of a frame. DND icons and cursors are not affected by this patch, since they are not drawn as MetaSurfaceActors. If a MetaSurfaceActor or a parent is cloned, then we'll check the position of the original actor again when the clone is drawn, which is slightly expensive, but harmless. If the MetaShapedTexture instead is cloned, as GNOME Shell does in many cases, then these clones will not cause duplicate position checks. ----- Changed to your explanation in the commit message. Added NULL check in _paint.
Review of attachment 306677 [details] [review]: In terms of moving to weak references - what I would suggest is that if you do that, run g_object_run_dispose() in the destructor, instead of letting it happen at finalization time, so that actor->surface never points to a "destroyed" wayland surface even something else (like a not-yet-collected interpreted language reference) is keeping the surface object alive. ::: src/compositor/meta-window-actor.c @@ +619,3 @@ + META_SURFACE_ACTOR_WAYLAND (priv->surface); + + if (surface_actor && You've already checked META_IS_SURFACE_ACTOR_WAYLAND (priv->surface)) - so priv->surface cannot be NULL @@ +622,3 @@ + meta_surface_actor_wayland_get_surface (surface_actor)) + { + double scale = meta_surface_actor_wayland_get_scale (surface_actor); Weird dependency between methods if you need to call meta_surface_actor_wayland_get_surface() to know if it's safe to call meta_surface_actor_wayland_get_scale (surface_actor) - maybe just make the latter return 1 if there's no surface?
Review of attachment 306586 [details] [review]: Looks fine, couple of tiny points to fix before committing. ::: src/wayland/meta-wayland-outputs.c @@ +238,3 @@ { MetaWaylandOutput *wayland_output = META_WAYLAND_OUTPUT (object); + GList *it; Eek - more different conventions! Mutter currently has a 50-50 or so mix of 'GList *tmp' and 'GList *l' - pick one or the other - I suggest '*l' as easier to read. @@ +243,3 @@ + + /* Make sure the wl_output destructor doesn't try to access MetaWaylandOutput + * after we have free:ed it. I'd simply write "freed". For cases where that doesn't work, the standard would be to use an apostrophe. "I rm'ed the file from that directory." (Same thing for the commit message)
Review of attachment 306678 [details] [review]: Looks fine
Created attachment 306984 [details] [review] wayland: Don't crash if wl_output resource is destroyed after being removed Previously a MetaWaylandOutput could be removed from the current outputs table (by being unplugged for example). This would result in the global object being removed and the MetaWaylandOutput instance freed, but the wl_resource destructor would still try to remove itself from the list of resources. Trying to do this, it'd try to access its user data pointer which would point to the free:ed MetaWaylandOutput instance, and as a result crash when trying to manipulate the free:ed data.
Created attachment 306985 [details] [review] MetaSurfaceActorWayland: Unset the MetaWaylandSurface pointer when it goes away We may access it during painting even if it has been free:ed. For now, manually unset it during the MetaWaylandSurface cleanup; in the future make MetaWaylandSurface a GObject and make the surface pointer a weak reference.
Review of attachment 306984 [details] [review]: Looks good
Review of attachment 306985 [details] [review]: free:ed in the commit message ::: src/compositor/meta-surface-actor-wayland.c @@ +111,3 @@ int output_scale = 1; + g_return_val_if_fail (priv->surface, 1); g_return_val_if_fail() must never be used for "can happen" circumstances: A) It's possible to set compile flags so that g_return_val_if_fail() is omitted B) It warns with a critical warning that can be set with an environment to crash the program C) It warns with a critical warning that indicates a fault that needs to be debugged this should simply be: if (priv->surface == NULL) return;
Created attachment 307015 [details] [review] wayland: Don't crash if wl_output resource is destroyed after being removed Previously a MetaWaylandOutput could be removed from the current outputs table (by being unplugged for example). This would result in the global object being removed and the MetaWaylandOutput instance freed, but the wl_resource destructor would still try to remove itself from the list of resources. Trying to do this, it'd try to access its user data pointer which would point to the freed MetaWaylandOutput instance, and as a result crash when trying to manipulate the freed data.
Created attachment 307016 [details] [review] MetaSurfaceActorWayland: Unset the MetaWaylandSurface pointer when it goes away We may access it during painting even if it has been freed. For now, manually unset it during the MetaWaylandSurface cleanup; in the future make MetaWaylandSurface a GObject and make the surface pointer a weak reference.
(In reply to Jonas Ådahl from comment #53) > Created attachment 307015 [details] [review] [review] > wayland: Don't crash if wl_output resource is destroyed after being removed > > Previously a MetaWaylandOutput could be removed from the current outputs > table (by being unplugged for example). This would result in the global > object being removed and the MetaWaylandOutput instance freed, but the > wl_resource destructor would still try to remove itself from the list of > resources. Trying to do this, it'd try to access its user data pointer > which would point to the freed MetaWaylandOutput instance, and as a > result crash when trying to manipulate the freed data. This new version only does s/free:ed/freed/ in the commit message. No need to re-review the code.
Attachment 296744 [details] pushed as 1576b7d - wayland: Put MetaWaylandOutput struct in header file Attachment 299780 [details] pushed as 6ec7fa2 - wayland: Use surface role when special casing surface commits Attachment 306584 [details] pushed as dc99af4 - wayland: Make MetaWaylandOutput a GObject Attachment 306678 [details] pushed as eb023ff - wayland: Send wl_surface.enter and wl_surface.leave Attachment 307015 [details] pushed as f295349 - wayland: Don't crash if wl_output resource is destroyed after being removed Attachment 307016 [details] pushed as ba7c524 - MetaSurfaceActorWayland: Unset the MetaWaylandSurface pointer when it goes away
Created attachment 307454 [details] [review] MetaSurfaceActorWayland: Don't dereference surface before NULL check Fixes regression introduced in ba7c524a18d73679ba1e2b8835254774f7726e8d.
Attachment 307454 [details] pushed as 5d10196 - MetaSurfaceActorWayland: Don't dereference surface before NULL check