[RFC wayland] protocol: Add wl_surface.buffer_damage

Pekka Paalanen ppaalanen at gmail.com
Fri Nov 13 00:03:33 PST 2015


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?

> >>>> +      <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? :-)

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.

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

> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 811 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20151113/318582c4/attachment.sig>


More information about the wayland-devel mailing list