[PATCH wayland v2] protocol: Clarify the behaviour of wl_surface.attach

Thu Feb 16 09:58:42 UTC 2017

On Wed, 15 Feb 2017 11:49:33 -0600
Derek Foreman <derekf at osg.samsung.com> wrote:

> On 15/02/17 06:50 AM, Pekka Paalanen wrote:
> > On Wed, 15 Feb 2017 12:17:10 +0800
> > Jonas Ådahl <jadahl at gmail.com> wrote:
> >  
> >> On Tue, Feb 14, 2017 at 10:20:06AM -0600, Derek Foreman wrote:  
> >>> Attaching a NULL wl_buffer to a surface is not always valid, but
> >>> the previous text indicated it was.
> >>>
> >>> Instead, let's define what NULL attachment does for all the surface
> >>> roles defined in wayland.xml, stop giving a blanket definition of
> >>> its behavior in wl_surface.attach, and warn developers that they
> >>> should refer to other protocol documentation for a full understanding
> >>> of wl_surface.attach.  
> >>
> >> Good to see these things cleared up. Have some comments on the wording
> >> below, and the "cursor" role behaviour seems still undefined when
> >> attaching a NULL buffer.
> >>  
> >>>
> >>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> >>> ---
> >>>
> >>> Changes from v1:
> >>>  pretty much everything - define NULL attach for wl_shell specifically
> >>>  and remove the generic statement from wl_surface.attach
> >>>  refer readers to "other documentation"
> >>>
> >>>  protocol/wayland.xml | 10 +++++++---
> >>>  1 file changed, 7 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> >>> index 29b63be..1a76e60 100644
> >>> --- a/protocol/wayland.xml
> >>> +++ b/protocol/wayland.xml
> >>> @@ -1002,6 +1002,10 @@
> >>>        the related wl_surface is destroyed. On the client side,
> >>>        wl_shell_surface_destroy() must be called before destroying
> >>>        the wl_surface object.
> >>> +
> >>> +      Attaching a NULL wl_buffer to a surface assigned a role by
> >>> +      wl_shell will remove the content from the surface after the
> >>> +      next commit.  
> >>
> >> How is "removes the content" compared to unmaps? Does this mean a shell
> >> implementation need to keep track of the position when the shell surface
> >> is later remapped? That would be a new requirement that was previously
> >> undefined behaviour, and is not what mutter does at the moment.  
> >
> > Maintaining position has been how I have always read it, but then I
> > shouldn't be allowed near desktop shell protocols. I even thought that
> > unmap also maintains the position.
> >  
> >>>      </description>
> >>>
> >>>      <request name="pong">
> >>> @@ -1324,6 +1328,9 @@
> >>>        <description summary="set the surface contents">
> >>>  	Set a buffer as the content of this surface.
> >>>
> >>> +	The role of the surface influences the behaviour of attach,
> >>> +	so this documentation is incomplete without further reading.
> >>> +  
> >>
> >> Possible alternative wording, possibly in the end of the <description/>,
> >> as it talks about things that is specified below:
> >>
> >> 	The effect committing an attached buffer to a surface depends on
> >> 	what role the surface has been or is going to be assigned to.
> >> 	See the corresponding role specification for further details.  
> >
> > Is it missing the case for "is currently"? I forget our rules about
> > changing an assigned role, but I think the wording you want here is for
> > current or the next assigned role, to cover the case where a
> > wl_surface.commit makes the wl_surface illegal for a following role
> > assignment, not just for doing an illegal thing while the role is
> > already assigned. What role it may have had in the past should not
> > matter.  
> 
> We can't re-assign a role - the wl_surface text is clear that a 
> wl_surface, once given a role, retains it for its lifetime... but we 
> could attach to a surface that does not yet have an assigned role.

Ok, right, so that's what the spec needs to cover then.

> The commit can change the role, though, so I guess what we're ultimately 
> interested in is the role the surface will have after the commit completes.

IIRC not all roles require a commit to become set. But that should be
irrelevant for wl_surface spec.

> I guess things get sticky when we attach a buffer to a surface, commit, 
> then attempt to create an xdg v6 shell surface from it.

I would expect the xdg_shell spec to explicitly forbid that.

> Or even attach a buffer to a surface (no commit!), then attempt to 
> create an xdg v6 surface from it.

And this too, since the idea IIRC was to not draw anything until the
client knows what size it should be.

> In both of these cases the error is not generated by the attach, or by 
> the commit, but by the attempt to generate an xdg v6 surface later, 
> which does not actually try set a role at that point at all :).

I'd say those details need to be explained in the xdg_shell spec.

> This leaves me banging my head on my desk because that puts me very 
> close to where I started:
> 
> The text for attach needs to explain what happens when there is no 
> surface role present.  ie)  I can attach, commit, attach NULL, commit. 
> This will remove the surface content, and I should then be able to 
> create an xdg v6 surface from the surface safely, because no buffer is 

Yes.

> attached.  (This seems like it could be interpreted as an error based on 
> one way of reading xdgv6, but weston does not appear to post an error 
> for this)

To me it would be silly to assume or require that compositors need to
track the whole history of a wl_surface to generate errors. What
actually matters, IMO, is the current and pending wl_surface state at
the time of any related request (not just commit).

To me, these are completely the same from the wl_surface state point of
view in the scope of this discussion:
- create wl_surface
- create wl_surface, attach NULL buffer
- create wl_surface, attach and commit NULL buffer
- create wl_surface, (attach a real buffer){0..n}, attach and commit
  NULL buffer
- create wl_surface, (attach and commit a real buffer){0..n}, attach
  and commit NULL buffer

> What I was originally interested in seems to be more along the lines of:
> Some roles do not allow surfaces to have their content removed.

Sounds like you've come a full circle in the above. :-)
I wouldn't write that in the wl_surface spec, though. It's both too
specific and unhelpfully vague.

> xdg v6 for example, expects no content at creation time, but after you 
> commit content in response to the first configure, you may never remove 
> the content again?

I'll let xdg_shell devs answer that. I have happily forgot all about
why they didn't want to allow committing a NULL buffer.

Thanks,
pq

> 
> Thanks,
> Derek
> 
> >>
> >> This makes read more as the behaviour of *attach* does not change, but
> >> the effect of having attached and committed a buffer.  
> >
> > Yes, it is important to make that distiction. Attach alone does
> > nothing, since a following attach may overwrite the pending buffer
> > again before it is committed. Not that clients would be sane to do
> > that, but for completeness.
> >
> > With the fixed wording:
> > Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >
> > Not R-b because I have not read all the affected role specs in
> > wayland.xml.
> >
> >
> > Thanks,
> > pq
> >  
> >>
> >>
> >> Jonas
> >>  
> >>>  	The new size of the surface is calculated based on the buffer
> >>>  	size transformed by the inverse buffer_transform and the
> >>>  	inverse buffer_scale. This means that the supplied buffer
> >>> @@ -1358,9 +1365,6 @@
> >>>  	the surface contents. However, if the client destroys the
> >>>  	wl_buffer before receiving the wl_buffer.release event, the surface
> >>>  	contents become undefined immediately.
> >>> -
> >>> -	If wl_surface.attach is sent with a NULL wl_buffer, the
> >>> -	following wl_surface.commit will remove the surface content.
> >>>        </description>
> >>>        <arg name="buffer" type="object" interface="wl_buffer" allow-null="true"
> >>>  	   summary="buffer of surface contents"/>
> >>> --
> >>> 2.11.0
> >>>  
> >  
> 

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