On 13/11/15 02:03 AM, Pekka Paalanen wrote: > On Thu, 12 Nov 2015 13:48:10 -0600 > Derek Foreman <der...@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 <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. :) > > 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@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
