[PATCH wayland v2] protocol: Add wl_surface.damage_buffer

[Pekka Paalanen]

On Fri, 20 Nov 2015 16:01:48 +0800
Jonas Ådahl <jadahl at gmail.com> wrote:

> On Wed, Nov 18, 2015 at 04:17:58PM -0600, Derek Foreman wrote:
> > wl_surface.damage uses surface local co-ordinates.
> > 
> > Buffer scale and buffer transforms came along, and EGL surfaces
> > have no understanding of them.
> > 
> > Theoretically, clients pass damage rectangles - in Y-inverted surface
> > co-ordinates) to EGLSwapBuffersWithDamage, and the EGL implementation
> > passed them on to wayland.  However, for this to work the EGL
> > implementation must be able to flip those rectangles into the space
> > the compositor is expecting, but it's unable to do so because it
> > doesn't know the height of the transformed buffer.
> > 
> > So, currently, EGLSwapBuffersWithDamage is unusable and EGLSwapBuffers
> > has to pass (0,0) - (INT32_MAX, INT32_MAX) damage to function.
> > 
> > wl_surface.damage_buffer allows damage to be registered on a surface
> > in buffer co-ordinates, avoiding this problem.
> > 
> > Credit where it's due, these ideas are not entirely my own:
> > Over a year ago the idea of changing damage co-ordinates to buffer
> > co-ordinates was suggested (by Jason Ekstrand), and it was at least
> > partially rejected and abandoned.  At the time it was also suggested
> > (by Pekka Paalanen) that adding a new wl_surface.damage_buffer request
> > was another option.
> > 
> > This will eventually resolve:
> > https://bugs.freedesktop.org/show_bug.cgi?id=78190
> > by making the problem irrelevant.
> > 
> > Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> > ---
> > 
> > Differnces from v1:
> >  - Don't make such a big deal about GL in the request documentation
> >  - rename the request about 3 times, eventually settle on damage_buffer
> >  - don't say anything about mixing damage and damage_buffer
> > 
> >  protocol/wayland.xml | 44 ++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 42 insertions(+), 2 deletions(-)
> > 
> > diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> > index 9c22d45..7665307 100644
> > --- a/protocol/wayland.xml
> > +++ b/protocol/wayland.xml
> > @@ -176,7 +176,7 @@
> >      </event>
> >    </interface>
> >  
> > -  <interface name="wl_compositor" version="3">
> > +  <interface name="wl_compositor" version="4">
> >      <description summary="the compositor singleton">
> >        A compositor.  This object is a singleton global.  The
> >        compositor is in charge of combining the contents of multiple
> > @@ -986,7 +986,7 @@
> >      </event>
> >    </interface>
> >  
> > -  <interface name="wl_surface" version="3">
> > +  <interface name="wl_surface" version="4">
> >      <description summary="an onscreen surface">
> >        A surface is a rectangular area that is displayed on the screen.
> >        It has a location, size and pixel contents.
> > @@ -1111,6 +1111,10 @@
> >  	wl_surface.commit assigns pending damage as the current damage,
> >  	and clears pending damage. The server will clear the current
> >  	damage as it repaints the surface.
> > +
> > +	Alternatively, damage can be posted with wl_surface.damage_buffer
> > +	which uses buffer co-ordinates instead of surface co-ordinates,
> > +	and is probably the preferred and intuitive way of doing this.
> >        </description>
> >  
> >        <arg name="x" type="int"/>
> > @@ -1327,6 +1331,42 @@
> >        </description>
> >        <arg name="scale" type="int"/>
> >      </request>
> > +
> > +    <!-- Version 4 additions -->
> > +    <request name="damage_buffer" since="4">
> > +      <description summary="mark part of the surface damaged using buffer co-ordinates">
> > +	This request is used to describe the regions where the pending
> > +	buffer is different from the current surface contents, and where
> > +	the surface therefore needs to be repainted. The pending buffer
> > +	must be set by wl_surface.attach before sending damage. The
> 
> I guess the order of attach and damage_buffer here should be dropped, as
> we are dropping it from wl_surface.damage already.
> 
> > +	compositor ignores the parts of the damage that fall outside of
> > +	the surface.
> > +
> > +	Damage is double-buffered state, see wl_surface.commit.
> > +
> > +	The damage rectangle is specified in buffer coordinates.
> > +
> > +	The initial value for pending damage is empty: no damage.
> > +	wl_surface.damage adds pending damage: the new pending damage

damage_buffer

> > +	is the union of old pending damage and the given rectangle.
> > +
> > +	wl_surface.commit assigns pending damage as the current damage,
> > +	and clears pending damage. The server will clear the current
> > +	damage as it repaints the surface.
> > +
> > +	This request differs from wl_surface.damage in only one way - it
> > +	takes damage in buffer co-ordinates instead of surface local
> > +	co-ordinates.  While this generally is more intuitive than surface
> > +	co-ordinates, it is especially desirable when using wl_viewport
> 
> s/wl_viewport/wp_viewport/.
> 
> With those two fixed: Reviewed-by: Jonas Ådahl <jadahl at gmail.com>
> 
> 
> Jonas
> 
> 
> > +	or when a drawing library (like EGL) is unaware of buffer scale
> > +	and buffer transform.
> > +      </description>
> > +
> > +      <arg name="x" type="int"/>
> > +      <arg name="y" type="int"/>
> > +      <arg name="width" type="int"/>
> > +      <arg name="height" type="int"/>
> > +    </request>
> >     </interface>

Right, agreed with Jonas.

Furthermore, now both damage and damage_buffer are documented to affect
"pending damage". I'm sure this raises questions on what happens when
the same commit is using both kinds of damage and changing one of
buffer_scale, buffer_transform, or wp_viewport parameters.

I suspect we should say something, because otherwise implementors might
use just one region to accumulate damage, in either surface or buffer
coordinates, and converting damage from the other coordinate space
immediately, before commit. This leads to two different possible
behaviours.

Even if a commit is not using both kinds of damage at the same time, it
is possible that the submitted damage is in the other coordinate system
than what the compositor is collecting, which leads to the same
problem: should the coordinate space conversion happen with the
current, pending, or new paramters.

I have always though it like you implemented it: the compositor collect
two regions, one for each damage coordinate space, and merges them only
at commit when all paramaters are locked down.

The spec for wl_surface.commit is actually lacking since the
introduction of buffer_scale and buffer_transform (and wp_viewport). It
says that pending wl_buffer is applied first and all the rest second.
This wording was to ensure that the new surface size takes effect
before e.g. any regions are clipped to it. But wl_buffer is not the
only parameter affecting the surface size, buffer_scale, transform and
viewport affect it too. So scale, transform, and viewport must be
considered together the wl_buffer.

I think the above rationale also answers the question about when you
can convert damage from one coordinate space to another: it must happen
with the final new buffer_scale, buffer_transform, and wp_viewport
parameters. Therefore the conversion cannot happen before
wl_surface.commit, and one must collect two regions until commit.

This is too much to infer from the spec, so we should explain it
somehow. How about mandating the collection of two regions and saying
conversion can happen only with the final parameters?

It could be as simple as changing damage_buffer spec to talk about
pending damage_buffer region. The notes about conversion should
probably be added to wl_surface.commit, as that attempts (but already
fails) to document how this all is supposed to work together.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 811 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20151120/7580cabb/attachment.sig>