[RFC wayland] protocol: Add wl_surface.buffer_damage
Derek Foreman
derekf at osg.samsung.com
Thu Nov 12 11:48:10 PST 2015
On 12/11/15 01:27 PM, Jason Ekstrand wrote:
> Thanks for picking this up! Also, thanks for posting this on the bug,
> I would have missed it otherwise!
Thanks to Pekka for prodding me to do so. :)
> On Thu, Nov 12, 2015 at 11:16 AM, Derek Foreman <derekf at osg.samsung.com> wrote:
>> 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.
>
> Either buffer_damage or damage_pixels is fine by me. I think I would
> have a slight preference towards buffer_damage but meh.
Me too, honestly. :)
>>>> + <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?
>
> We could say that it's the union of the two but I kind of like "don't
> do this" better.
>
>>>> + </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?
>
> Actually, we do... We upload them using glTextureSubData and only
> update a portion of it. Fun fact: at one point, we were actually
> getting that wrong in the face of buffer transformations. Might still
> be, I'm not sure.
There's also the pixman renderer, which I think just keeps a buffer
reference...
> It's worth noting that this patch is completely useless without
> something like this:
>
> http://lists.freedesktop.org/archives/wayland-devel/2014-April/014004.html
>
> Without libwayland tracking object versions, the EGL implementation
> will have no idea whether it has a version 4 surface or not.
Argh, thanks for bringing that up. It's not in patchwork, it never
happened. ;)
I'll rebase your patch and re-post it later today. Initial comments
back in 2014 looked promising.
Thanks!
> --Jason
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
More information about the wayland-devel
mailing list