[PATCH wayland] protocol: make get_subsurface double-buffered

[Pekka Paalanen]

On Mon, 09 May 2016 11:48:07 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> 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?

I think I did, yes.

But weston also has another bug:
https://bugs.freedesktop.org/show_bug.cgi?id=94735

Weston needs fixing and verifying in any case.
https://phabricator.freedesktop.org/T7359

> > 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. :)

Yeah, and I'd love to write that test, but we'll see when I can get to
it and fixing weston. It will need to use screenshot-based testing,
where the baseline test is racy (cursor may or may not be in the
picture).

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


Thanks,
pq

> > 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>
> >  
> >   
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160510/14a8bead/attachment.sig>