[PATCH wayland] protocol: make get_subsurface double-buffered

[Jonas Ådahl]

On Tue, May 10, 2016 at 11:27:25AM +0300, Pekka Paalanen wrote:
> On Tue, 10 May 2016 11:43:04 +0800
> Jonas Ådahl <jadahl at gmail.com> wrote:
> 
> > On Mon, May 09, 2016 at 02:45:05PM +0300, 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.
> > > 
> > > 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>  
> > 
> > Reviewed-by: Jonas Ådahl <jadahl at gmail.com>
> > 
> > It would be very nice to have a test for this somewhere. It's hard to
> > test automatically, since its the output from the renderer that this
> > effects, and we can't easily test that, but at least a manually
> > checked test.
> 
> Yes, I will need to write one for fixing weston anyway.
> 
> 
> Considering that the weston fixes and tests might not make it in 1.11
> release, should this patch be merged for Wayland 1.11 or not?

That is a good question. Considering it is unclear (while still
unlikely) if this will actually break anything, it might be a good idea
to be a bit conservative here, and it would be good with an
implementation to just have tested, while having weston implement it
according to the specification released along side it.

It is not a strong opinion though.


Jonas

> 
> 
> Thanks,
> pq
> 
> > > ---
> > >  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>
> > >  
> > > -- 
> > > 2.7.3
> > >   
>