[PATCH wayland] protocol: make get_subsurface double-buffered

[Jonas Ådahl]

On Tue, May 10, 2016 at 11:19:29AM +0300, Pekka Paalanen wrote:
> On Mon, 9 May 2016 07:58:32 -0500
> Yong Bakos <junk at humanoriented.com> wrote:
> 
> > Hi Pekka,
> > Two minor nits in the description, noted inline below.
> > 
> > yong
> > 
> > On May 9, 2016, at 6:45 AM, Pekka Paalanen <ppaalanen at gmail.com> 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.
> > > 
> > > 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.
> > > 
> > > 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  
> > 
> > visible the next time
> > 
> 
> Yes.
> 
> > > +	applied.
> > > +
> > > +	This request modifies the behaviour of wl_surface.commit request on  
> > 
> > requests
> 
> "request" is used as a noun here and there is only one, so I think it's
> correct as is.
> 
> Or which one did you mean? I think the latter is also singular, but I
> was thinking in interface terms, not as "from now on commits will
> behave differently" which would be true too, thinking in runtime terms.
> 
> Alternative suggestions?

It seems to be missing a 'the', as in, it should be:

"This request modifies the behaviour of the wl_surface.commit request
on the sub-surface.."

or simply without the second "request" at all, as in:

"This request modifies the behaviour of wl_surface.commit on the
sub-surface, ..."


Jonas

> 
> 
> Thanks,
> pq
> 
> > > +	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>
> > > 
> > > -- 
> > > 2.7.3
> > > 
> > > _______________________________________________
> > > wayland-devel mailing list
> > > wayland-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/wayland-devel  
> > 
>