[PATCH wayland v4] protocol: Add wl_surface.damage_buffer

[Pekka Paalanen]

On Fri, 27 Nov 2015 16:47:19 +0800
Jonas Ådahl <jadahl at gmail.com> wrote:

> On Thu, Nov 26, 2015 at 03:44:12PM -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>  
> 
> Reviewed-by: Jonas Ådahl <jadahl at gmail.com>, with a couple of comments
> below.

Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

...

> > ---
> > 
> > Changes from v3:
> >  replaced the last paragraph with Pekka's version
> >   (with one tiny grammar change)
> > 
> >  protocol/wayland.xml | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 47 insertions(+), 2 deletions(-)
> > 
> > diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> > index f9e6d76..4e97bb9 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.
> > @@ -1109,6 +1109,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"/>
> > @@ -1325,6 +1329,47 @@
> >        </description>
> >        <arg name="scale" type="int"/>
> >      </request>
> > +
> > +    <!-- Version 4 additions -->  
> 
> Bikeshed mode enabled: missing empty line between "Version N additions"
> and actual 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 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_buffer adds pending damage: the new pending
> > +	damage 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 wp_viewport
> > +	or when a drawing library (like EGL) is unaware of buffer scale
> > +	and buffer transform.
> > +
> > +	Since it is impossible to convert between buffer and surface
> > +	co-ordinates until all the possible parameters affecting the
> > +	surface size and the buffer-surface mapping are known at
> > +	wl_surface.commit time, damage from wl_surface.damage and
> > +	wl_surface.damage_buffer must be accumulated separately in a
> > +	compositor and combined during wl_surface.commit at the earliest.  
> 
> As Jason already pointed out, this looks out of place. With some
> imagination, it might also not entirely true. A compositor may put
> damage "together" in a single list, with "transform this way" flags next
> to regions in different coordinate spaces, and then later process and
> combine at commit when all information is available. Sure, it makes
> little sense to do things this way, but it wouldn't be wrong while
> still, depending on how interpreted, not really doing what the above
> paragraph said.

Yeah. It's implementation guideline for compositor writers. Maybe it
should have started by stating that, so that client writers don't get
confused.

I suppose we all agree that in lack of better wording for the moment,
this is good to land. If someone comes up with a better wording, we can
patch that later.

I restored a couple of CCs, and since 26th was thanksgiving in the US,
maybe this should land early next week if no-one objects.


Thanks,
pq

> > +      </description>
> > +
> > +      <arg name="x" type="int"/>
> > +      <arg name="y" type="int"/>
> > +      <arg name="width" type="int"/>
> > +      <arg name="height" type="int"/>
> > +    </request>
> >     </interface>
> >  
> >    <interface name="wl_seat" version="5">
> > -- 
> > 2.6.2
> > 
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel  

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20151127/b28bddf1/attachment.sig>