GNOME Bugzilla – Bug 739163
wayland: Only call frame callbacks when a surface gets drawn on screen
Last modified: 2015-08-09 17:26:16 UTC
See patch.
Created attachment 289309 [details] [review] wayland: Only call frame callbacks when a surface gets drawn on screen The spec says: "A server should avoid signalling the frame callbacks if the surface is not visible in any way, e.g. the surface is off-screen, or completely obscured by other opaque surfaces." We actually do have the information to do that but we are always calling the frame callbacks in after_stage_paint. So fix that to only call when when the surface gets drawn on screen.
Review of attachment 289309 [details] [review]: ::: src/compositor/meta-window-actor.c @@ +1960,3 @@ + if (meta_is_wayland_compositor () && priv->repaint_scheduled) + meta_surface_actor_wayland_prepare_frame_callbacks (META_SURFACE_ACTOR_WAYLAND (priv->surface)); Can't you just add a meta_surface_actor_wayland_paint? That way you don't need to have this function traverse the subsurface tree (which seems missing in this patch). ::: src/wayland/meta-wayland-surface.c @@ +419,2 @@ /* wl_surface.frame */ + meta_surface_actor_wayland_add_frame_callbacks (META_SURFACE_ACTOR_WAYLAND (surface->surface_actor), &pending->frame_callback_list); nit: This line is so long I can't see it in the review window. Could you make it a bit shorter?
Created attachment 306530 [details] [review] [PATCH] wayland: Only call frame callbacks when a surface gets drawn on screen The spec says: "A server should avoid signalling the frame callbacks if the surface is not visible in any way, e.g. the surface is off-screen, or completely obscured by other opaque surfaces." We actually do have the information to do that but we are always calling the frame callbacks in after_stage_paint. So fix that to only call when when the surface gets drawn on screen. ---- 1) Moved it to _paint to fix subsurface handling 2) Splitted the long line
Created attachment 306531 [details] [review] wayland: Only call frame callbacks when a surface gets drawn on screen The spec says: "A server should avoid signalling the frame callbacks if the surface is not visible in any way, e.g. the surface is off-screen, or completely obscured by other opaque surfaces." We actually do have the information to do that but we are always calling the frame callbacks in after_stage_paint. So fix that to only call when when the surface gets drawn on screen.
Created attachment 306532 [details] [review] wayland: Only call frame callbacks when a surface gets drawn on screen The spec says: "A server should avoid signalling the frame callbacks if the surface is not visible in any way, e.g. the surface is off-screen, or completely obscured by other opaque surfaces." We actually do have the information to do that but we are always calling the frame callbacks in after_stage_paint. So fix that to only call when when the surface gets drawn on screen. --- Readded accidently removed empty line.
Review of attachment 306532 [details] [review]: ::: src/wayland/meta-wayland-surface.c @@ +527,3 @@ /* wl_surface.frame */ + meta_surface_actor_wayland_add_frame_callbacks (META_SURFACE_ACTOR_WAYLAND (surface->surface_actor), + &pending->frame_callback_list); Only toplevel surfaces and subsurfaces will have a surface->surface_actor (at least a valid one), so you might want to do this only for the roles that has uses an actor. Maybe add to their commit functions instead?
Created attachment 308969 [details] [review] wayland: Only call frame callbacks when a surface gets drawn on screen The spec says: "A server should avoid signalling the frame callbacks if the surface is not visible in any way, e.g. the surface is off-screen, or completely obscured by other opaque surfaces." We actually do have the information to do that but we are always calling the frame callbacks in after_stage_paint. So fix that to only call when when the surface gets drawn on screen. --- Went with the simple way of checking wheter the surface has an actor.
Attachment 308969 [details] pushed as 070cd27 - wayland: Only call frame callbacks when a surface gets drawn on screen
Review of attachment 308969 [details] [review]: ::: src/wayland/meta-wayland-surface.c @@ +546,3 @@ + &pending->frame_callback_list); + else + wl_list_insert_list (&compositor->frame_callbacks, &pending->frame_callback_list); This will break the DND surfaces and cursor surfaces, as they will never be painted as actors and the frame callbacks will end up never be replied to. Given that frame callbacks for such surfaces will now stay in the pending callback list in the actor, did you consider how to deal with destruction? I believe client object destruction has no guaranteed order, so the wl_surface may be destroyed before the wl_callback, meaning the actor is unreferenced before the actor. If that happens, it looks like we might crash when removing the callback if we try to unlink from the list in destroyed actor. I think we either need to check what surface role we are, and depending on that add to the surface actor. Or we need to change MetaWaylandSurface to only create a surface actor when we actually will need one. This might be more reasonably actually.
(In reply to Jonas Ådahl from comment #9) > Review of attachment 308969 [details] [review] [review]: > > ::: src/wayland/meta-wayland-surface.c > @@ +546,3 @@ > + > &pending->frame_callback_list); > + else > + wl_list_insert_list (&compositor->frame_callbacks, > &pending->frame_callback_list); > > This will break the DND surfaces and cursor surfaces, as they will never be > painted as actors and the frame callbacks will end up never be replied to. > > Given that frame callbacks for such surfaces will now stay in the pending > callback list in the actor, did you consider how to deal with destruction? I > believe client object destruction has no guaranteed order, so the wl_surface > may be destroyed before the wl_callback, meaning the actor is unreferenced > before the actor. If that happens, it looks like we might crash when > removing the callback if we try to unlink from the list in destroyed actor. > > I think we either need to check what surface role we are, and depending on > that add to the surface actor. Or we need to change MetaWaylandSurface to > only create a surface actor when we actually will need one. This might be > more reasonably actually. > I think we either need to check what surface role we are, and depending on > that add to the surface actor. Or we need to change MetaWaylandSurface to > only create a surface actor when we actually will need one. This might be > more reasonably actually. Your comment about them not having a valid surface actor kinda implied the later? But looking at the code it seems we always have an actor (and assume that there is one). So easiest would be to do it based on the role.
Created attachment 308970 [details] [review] wayland: Add frame callbacks to the actor based on the role Checking for the presense of the actor is wrong because we always create one.
Created attachment 308971 [details] [review] SurfaceActorWayland: Destroy frame callbacks when the surface gets destroyed
Review of attachment 308971 [details] [review]: Sure.
Review of attachment 308970 [details] [review]: Ugh, I don't like this at all. I sort of made a conscious decision for a surface to always have an actor, just in case, but I'm sort of regretting that now. Let's go with this patch for now, and then I'll perhaps make these other surfaces not have an actor.
Attachment 308970 [details] pushed as 4dc5882 - wayland: Add frame callbacks to the actor based on the role Attachment 308971 [details] pushed as 038f828 - SurfaceActorWayland: Destroy frame callbacks when the surface gets destroyed