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 <der...@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 <der...@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 <der...@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
