[PATCH wayland v2] protocol: Add wl_surface.damage_buffer

[Derek Foreman]

On 20/11/15 06:22 AM, Pekka Paalanen wrote:
> 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.

Yup, was waiting to land the other patch before doing that here too.

>>> +	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

Yup.

>>> +	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/.

Got it, thanks.

>> 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.

Converting from buffer to surface without knowing surface bounds (which
you can't know until commit...) is impossible - if you don't clip a "0,
0 - MAX, MAX" submission before feeding it to matrix multiplication
functions really cool stuff happens.

I think everyone should have the joy of coming to the discovery on their
own.  I found it very enlightening.

But if you insist, I can add documentation to ruin this surprise for
future implementors.

> 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 think it has to be the pending parameters, nothing else makes sense?

> 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.

My only concern with the current implementation is that the gl texture
upload will be based on co-ordinates that have been transformed from
buffer to surface, then back to buffer.  It should be a cleanly
reversible transformation, and keeping things separate forever seemed
like a lot of headache (especially if people tried to mix the two requests).

> 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 makes sense to me.

> 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.

Sounds reasonable, I'll try to write this up.

> Thanks,
> pq
>