[RFC wayland] protocol: Add wl_surface.buffer_damage

[Derek Foreman]

On 09/11/15 09:06 AM, Pekka Paalanen wrote:
> On Fri,  6 Nov 2015 12:55:19 -0600
> Derek Foreman <derekf at osg.samsung.com> 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.buffer_damage 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.buffer_damage request
>> was another option.
>>
> 
> Hi Derek,
> 
> please mention https://bugs.freedesktop.org/show_bug.cgi?id=78190 in
> this patch.

Will do.

>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
>> ---
>>
>> Necro-posting on:
>> http://lists.freedesktop.org/archives/wayland-devel/2014-February/013440.html
>> and
>> http://lists.freedesktop.org/archives/wayland-devel/2014-February/013442.html
>>
>> This came up on IRC again the other day, and it's still an unsolved problem...
>> I'm posting this as RFC to see if anyone's interested in it - I'll do an
>> implementation if we can get an agreement on the protocol text.
> 
> Thanks for picking this up!

Since all the conflict seems to be around how aggressively we should
encourage people to use this instead of the existing surface damage
request, I'll write a weston implementation.

>>  protocol/wayland.xml | 44 ++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 42 insertions(+), 2 deletions(-)
>>
>> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
>> index 9c22d45..1cb2f66 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.
>> @@ -1327,6 +1327,46 @@
>>        </description>
>>        <arg name="scale" type="int"/>
>>      </request>
> 
> I know Jasper suggested deprecating wl_surface.damage, but I see no
> reason for that. wl_surface.damage is well-defined - it is only misused.
> 
> We could add some wording to have both refer to each other as an
> alternative way to post damage. Especially to wl_surface.damage should
> mention buffer_damage as doing what you'd usually expect.

Ok, that sounds viable.

>> +
>> +    <!-- Version 4 additions -->
>> +    <request name="buffer_damage" since="4">
> 
> The name "buffer_damage" is slightly unfortunate. See:
> https://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_swap_buffers_with_damage.txt
> 
> What we are doing in Wayland is always "surface damage"[*], while that
> EGL extension reserves "buffer damage" for a different purpose. I feel
> it might be confusing.
> 
> Could we come up with a another name for this request?
> - wl_surface.damage_pixels
> - wl_surface.damage_by_buffer
> 
> eh... buffer_damage is fine if nothing else sticks. The specification
> language below is clear anyway, IMO.

I'll call it damage_pixels in the next revision.

>> +      <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
>> +	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
>> +	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.
> 
> Essentially a copy from wl_surface.damage without changing anything but
> the coordinate system. Ok.
> 
>> +
>> +	This request differs from wl_surface.damage in only one way - it
>> +	takes damage in buffer co-ordinates instead of surface local
>> +	co-ordinates.  This is desirable because EGL implementations
>> +	are unaware of buffer scale and buffer transform and can only
>> +	provide damage in buffer co-ordinates.  Damage in buffer
>> +	co-ordinates is required for EGLSwapBuffersWithDamage to work
>> +	efficiently.
> 
> Not sure if explaining the EGL side is needed here. Besides EGL, it
> could be any drawing library, and with wl_viewport there are much more
> use cases where buffer_damage is preferable.

Ok, I'll try to make the text a little more generic.

>> +	Mixing wl_surface.buffer_damage and wl_surface.damage requests
>> +	on the same surface will result in undefined behaviour.
> 
> Why undefined? The compositor will always transform between surface and
> buffer coordinate systems: surface to buffer for texture updates, and
> buffer to surface for repaint damage.
> 
> I suppose you can avoid accumulating two different regions depending on
> the coordinate space until wl_surface.commit by saying only one
> coordinate space is guaranteed to work at a time. Is that reason
> enough, or the only reason?

Just lazy.  I don't want to care about it or have to test it.  Saying
not to mix them within a commit is just fine too, I think.

Realistically, I can't imagine anyone wanting to do this in the first
place, so I didn't see much point in spending any time verifying the two
requests worked well together.

I suppose it'd be possible to slaughter clients trying to mix them -
would that be preferably to undefined?

>> +      </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">
> 
> 
> Thanks,
> pq
> 
> 
> [*] There is an off-topic rabbit hole to be dug here, if we would allow
> the compositor to cache shm-based textures... ;-)

[*] Move along, nothing to see here. :D

Why would we do that?  So we could release the buffer immediately and
let the app appear to consume less memory at the compositor's expense?