[RFC wayland] protocol: Add wl_surface.buffer_damage
Derek Foreman
derekf at osg.samsung.com
Fri Nov 13 09:56:16 PST 2015
On 13/11/15 02:03 AM, Pekka Paalanen wrote:
> On Thu, 12 Nov 2015 13:48:10 -0600
> Derek Foreman <derekf at osg.samsung.com> wrote:
>
>> 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. :)
>
> Haha, sorry for the bad suggestion. :-D
>
> My opposition to damage_pixels is: what if the buffer does not contain
> pixels?
That had occurred to me too, but wasn't quite sure how damage would work
on such buffers anyway.
Back to buffer_damage then?
>>>>>> + <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.
>
> Don't do this indeed, but should it be a fatal error, or just
> undefined? Or defined as whole-surface damage? :-)
whole-surface damage is pretty friendly, I guess. However it might not
be immediately obvious why performance is poor.
> I have hard time making my mind. If it's not a fatal error, I am sure
> someone will do it even if by accident. When someone does it, I'd
> expect the undefined behaviour to be practically damaging the whole
> surface, so you wouldn't easily notice it.
>
> So... for the sake of forcing programs to be more correct, make it a
> fatal error? In which case we need a new error code in wl_surface.
I wouldn't mind doing it this way. I'm not really a fan of subtlety. :)
>>>>>> + </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...
>
> Oh, this not what I meant. Right now Weston uses one GL-texture per
> wl_surface. Whenever content changes, it causes a glTexSubImage2D
> upload. If a client is using static pre-rendered buffers in a cyclic
> animation, the compositor will be doing uploads all the time.
>
> If the compositor had a GL-texture per buffer... but as said, let's not
> go into that rabbit hole. ;-)
*YIKES*
Yeah, let's stay away from that.
Thanks,
Derek
>>> 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.
>
> Right. It would have come up sooner or later, when someone tried to fix
> Mesa.
>
>
> Thanks,
> pq
>
>
>
> _______________________________________________
> 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