Semantics of wl_subcompositor::get_subsurface

[Pekka Paalanen]

On Thu, 24 Mar 2016 08:53:11 +0100
Martin Graesslin <mgraesslin at kde.org> wrote:

> On Monday, March 21, 2016 11:07:39 AM CET you wrote:
> > On Mon, 21 Mar 2016 08:36:59 +0100
> > 
> > Martin Graesslin <mgraesslin at kde.org> wrote:  
> > > Hi Wayland-devs,
> > > 
> > > while implementing sub-surface support in KWin I run into a small problem
> > > regarding the get_subsurface request:
> > > 
> > > When should the new subsurface be added to the parent surface?
> > > 
> > > My interpretation was that the subsurface will only be added after the
> > > next
> > > commit on the parent surface. As that would allow setting up the
> > > sub-surface completely and also calls like place_above are
> > > double-buffered. To me this looked like all child surface ordering
> > > changes are double buffered.  
> > Hi Martin,
> > 
> > child surface ordering changes are indeed meant to be applied
> > atomically by a commit on the parent.
> > 
> > And by "commit" on that sentence I mean "when the pending parent
> > surface state is applied". I have struggled a lot with the language in
> > the sub-surface spec.  
> 
> :-)
> 
> >   
> > > But testing with real world applications (systemsettings5 on QtWayland)
> > > showed that the parent surface does not get committed after the
> > > get_subsurface request and my implementation wasn't able to make the
> > > subsurface show at all due to that. On the other hand on Weston it works.
> > > 
> > > So what's the expected way? Is the get_subsurface request double buffered?
> > > If yes, could the documentation be extended about that?  
> > 
> > This is probably a case I have overlooked in both documentation and
> > Weston implementation. I likely have not considered that the to-be
> > sub-surface could be already fully set up, i.e. contents committed
> > and all.
> > 
> > Considering that a sub-surface is specified to start in synchronized
> > mode, I think the following would make sense when the parent surface is
> > already mapped and the to-be sub-surface has already contents
> > committed. The sub-surface gets mapped when:
> > - the client calls commit on the parent surface the next time, or
> > - the client calls set_desync on/after which according to the
> >   sub-surface rules the pending/cached surface state gets applied.  
> 
> that would be when the parents state gets applied?

This is from off the top of my head, but if the parent surface is
already acting non-synchronized itself, then set_desync would map the
sub-surface immediately.

However, please see below. If this gets too tricky, maybe we can just
require parent state to be applied instead, to be more consistent in
behaviour.

> > IOW, I would agree with your interpretation when the sub-surface stays
> > in synchronized mode.
> > 
> > *** (a dead-end pondering)
> > 
> > However, I don't think we can simply say "get_subsurface is
> > double-buffered and gets applied on the next parent surface commit"
> > without some more thought, because there are alternative paths that
> > avoid the need for parent surface commit:
> > 
> > 1. wl_surface A is already mapped, and effectively in desync mode
> > 2. wl_surface B has content applied (proper attach+commit)
> > 3. B is made a sub-surface of A (wl_subcompositor.get_subsurface)
> > 4. B is set to desync mode (wl_subsurface.set_desync)
> > 
> > Since the parent surface is effectively in desync mode, set_desync on B
> > applies cached state immediately. That allows B to be mapped (appear on
> > screen) with two possibilities:
> > - immediately at set_desync, or
> > - on the next commit on B after set_desync.
> > 
> > Which one happens is IMO unspecified. set_desync spec talks about
> > cached state, but as B has not seen commits while being a sub-surface
> > (they happened earlier), it does not have cached state.
> > 
> > Would it make more sense to pick the next commit on B after set_desync?  
> 
> I would say that making surface B a sub-surface of A affects the pending state 
> of surface A. Thus I would say it can only get mapped once surface A's state 
> is applied and the state of surface B just doesn't matter.

That could indeed be the way to make things more clear.

> > 
> > ***
> > 
> > Or, if we actually do specify that get_subsurface will be effective on
> > the next parent surface commit, then there is no way to map the
> > sub-surface without a parent commit. As I see adding sub-surfaces as an
> > action on the parent surface, I'd be fine with this interpretation too.
> > If the client really wants the sub-surface to appear on its own time,
> > it can simply commit the parent surface straight away. Maybe this would
> > be simpler?  
> 
> yep, that's how I would see it.

Cool.

> > 
> > Hmm, and there is the question of positioning the sub-surface.
> > Regardless of modes, getting sub-surface position change applied always
> > requires a commit on the parent surface. Z-ordering is the same too. If
> > the defaults do not happen to be fine, the surfaces would glitch, if
> > the sub-surface was allowed to become mapped without a parent commit.
> > This was probably a major point you wanted to make, right?  
> 
> yes in deed. Actually that was the point which made me aware of that something 
> is amiss. As I implemented the z-ordering in a double buffered way and 
> considered adding a sub-surface as changing the z-order of the sub-surfaces.
> 
> Especially it's not possible to add a new sub-surface as the bottom most 
> surface in a glitch free way if the adding is applied immediately.

Right.

> > 
> > I am happy to admit that the Weston implementation can be wrong here. I
> > have probably subconsciously assumed that one always sets up the
> > surface role first, and content second. Some roles imply that already,
> > though for sub-surfaces you can do either order.
> > 
> > After this pondering, I'm starting to think that always requiring a
> > commit on the parent surface to have get_subsurface applied and appear
> > on screen is the right thing to do.  
> 
> sounds good to me :-)

Very good.


Thanks,
pq

> > 
> > Does anyone else have an opinion for or against making the parent
> > commit always required?
> > 
> > I have filed https://phabricator.freedesktop.org/T7358 to keep track of
> > the needed actions, and a Weston bug depending on the solution.  
> 
> Cheers
> Martin

-------------- 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/20160331/5ace24f9/attachment.sig>