[PATCH wayland] protocol: make get_subsurface double-buffered

[Derek Foreman]

On 09/05/16 06:45 AM, Pekka Paalanen wrote:
> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> 
> The existing specification was not explicitly clear on when
> wl_subcompositor.get_subsurface request actually adds the sub-surface to
> the parent in the compositor's scenegraph. The implicit assumption was
> that this happens immediately, but it was not written anywhere.

Was weston implemented with this assumption?

> If it happens immediately, the client doing things in a wrong order may
> cause a glitch on screen. Particularly, if the wl_surface B that is
> going to be a sub-surface for wl_surface A (the parent) already has a
> buffer committed, and the parent surface is mapped, then get_subsurface
> will (may?) cause wl_surface B to become mapped immediately. That leaves
> no time to set up the sub-surface z-order or position before mapping,
> hence there can be a visible glitch.

It would be really cool to have a test for this glitch in weston. :)

Anyway,
Reviewed-by: Derek Foreman <derekf at osg.samsung.com>

> The way to avoid that, given that the parent surface is mapped, is to
> not commit a buffer to wl_surface B until all the sub-surface setup is
> done.
> 
> However, doing the sub-surface setup always requires a wl_surface.commit
> on the parent surface unless the defaults happen to be correct.
> 
> To make setting up a subsurface slightly easier by removing one
> possibility for a glitch, this patch amends the specification to require
> a wl_surface.commit on the parent surface for get_subsurface to
> complete. The sub-surface cannot become mapped before a parent commit.
> 
> This change may break existing clients that relied on the glitchy
> sequence to not need a parent surface commit to map the sub-surface.
> However, presumably all uses would at least issue a
> wl_subsurface.set_position, which requires a parent surface commit to
> apply. That would guarantee that there is a parent surface commit after
> get_subsurface, and so reduces the chances of breaking anything.
> 
> In other cases, this change may simply remove a possibility for the
> glitch.
> 
> This patch also adds a note about changing wl_surface.commit behaviour
> on wl_subcompositor.get_subsurface. (That could be a separate patch.)
> 
> The behaviour of wl_subsurface.destroy remains as specified, even though
> it is now slightly asymmetrical to get_subsurface. This is emphasized by
> adding the word "immediately". The effects of destruction were already
> explicitly documented, as is the way to achieve synchronized unmapping,
> so changing destruction behaviour would likely be more disruptive, and
> also open up more corner cases (what would happen between destroy and
> unmapping?).
> 
> Bug: https://phabricator.freedesktop.org/T7358
> Cc: Martin Gräßlin <mgraesslin at kde.org>
> Cc: Jonas Ådahl <jadahl at gmail.com>
> Cc: Chris Michael <cpmichael at comcast.net>
> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> ---
>  protocol/wayland.xml | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> index 92e3f43..1546b46 100644
> --- a/protocol/wayland.xml
> +++ b/protocol/wayland.xml
> @@ -2487,6 +2487,14 @@
>  	The to-be sub-surface must not already have another role, and it
>  	must not have an existing wl_subsurface object. Otherwise a protocol
>  	error is raised.
> +
> +	Adding sub-surfaces to a parent is a double-buffered operation on the
> +	parent (see wl_surface.commit). The effect of adding a sub-surface
> +	becomes visible on the next time the state of the parent surface is
> +	applied.
> +
> +	This request modifies the behaviour of wl_surface.commit request on
> +	the sub-surface, see the documentation on wl_subsurface interface.
>        </description>
>  
>        <arg name="id" type="new_id" interface="wl_subsurface"
> @@ -2557,7 +2565,7 @@
>  	that was turned into a sub-surface with a
>  	wl_subcompositor.get_subsurface request. The wl_surface's association
>  	to the parent is deleted, and the wl_surface loses its role as
> -	a sub-surface. The wl_surface is unmapped.
> +	a sub-surface. The wl_surface is unmapped immediately.
>        </description>
>      </request>
>  
>