On 20/11/15 06:22 AM, Pekka Paalanen wrote: > On Fri, 20 Nov 2015 16:01:48 +0800 > Jonas Ådahl <jad...@gmail.com> wrote: > >> On Wed, Nov 18, 2015 at 04:17:58PM -0600, Derek Foreman 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.damage_buffer 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.damage_buffer request >>> was another option. >>> >>> This will eventually resolve: >>> https://bugs.freedesktop.org/show_bug.cgi?id=78190 >>> by making the problem irrelevant. >>> >>> Signed-off-by: Derek Foreman <der...@osg.samsung.com> >>> --- >>> >>> Differnces from v1: >>> - Don't make such a big deal about GL in the request documentation >>> - rename the request about 3 times, eventually settle on damage_buffer >>> - don't say anything about mixing damage and damage_buffer >>> >>> protocol/wayland.xml | 44 ++++++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 42 insertions(+), 2 deletions(-) >>> >>> diff --git a/protocol/wayland.xml b/protocol/wayland.xml >>> index 9c22d45..7665307 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. >>> @@ -1111,6 +1111,10 @@ >>> 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. >>> + >>> + Alternatively, damage can be posted with wl_surface.damage_buffer >>> + which uses buffer co-ordinates instead of surface co-ordinates, >>> + and is probably the preferred and intuitive way of doing this. >>> </description> >>> >>> <arg name="x" type="int"/> >>> @@ -1327,6 +1331,42 @@ >>> </description> >>> <arg name="scale" type="int"/> >>> </request> >>> + >>> + <!-- Version 4 additions --> >>> + <request name="damage_buffer" since="4"> >>> + <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 >> >> I guess the order of attach and damage_buffer here should be dropped, as >> we are dropping it from wl_surface.damage already.
Yup, was waiting to land the other patch before doing that here too. >>> + 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 > > damage_buffer Yup. >>> + 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. >>> + >>> + This request differs from wl_surface.damage in only one way - it >>> + takes damage in buffer co-ordinates instead of surface local >>> + co-ordinates. While this generally is more intuitive than surface >>> + co-ordinates, it is especially desirable when using wl_viewport >> >> s/wl_viewport/wp_viewport/. Got it, thanks. >> With those two fixed: Reviewed-by: Jonas Ådahl <jad...@gmail.com> >> >> >> Jonas >> >> >>> + or when a drawing library (like EGL) is unaware of buffer scale >>> + and buffer transform. >>> + </description> >>> + >>> + <arg name="x" type="int"/> >>> + <arg name="y" type="int"/> >>> + <arg name="width" type="int"/> >>> + <arg name="height" type="int"/> >>> + </request> >>> </interface> > > Right, agreed with Jonas. > > Furthermore, now both damage and damage_buffer are documented to affect > "pending damage". I'm sure this raises questions on what happens when > the same commit is using both kinds of damage and changing one of > buffer_scale, buffer_transform, or wp_viewport parameters. > > I suspect we should say something, because otherwise implementors might > use just one region to accumulate damage, in either surface or buffer > coordinates, and converting damage from the other coordinate space > immediately, before commit. This leads to two different possible > behaviours. Converting from buffer to surface without knowing surface bounds (which you can't know until commit...) is impossible - if you don't clip a "0, 0 - MAX, MAX" submission before feeding it to matrix multiplication functions really cool stuff happens. I think everyone should have the joy of coming to the discovery on their own. I found it very enlightening. But if you insist, I can add documentation to ruin this surprise for future implementors. > Even if a commit is not using both kinds of damage at the same time, it > is possible that the submitted damage is in the other coordinate system > than what the compositor is collecting, which leads to the same > problem: should the coordinate space conversion happen with the > current, pending, or new paramters. I think it has to be the pending parameters, nothing else makes sense? > I have always though it like you implemented it: the compositor collect > two regions, one for each damage coordinate space, and merges them only > at commit when all paramaters are locked down. My only concern with the current implementation is that the gl texture upload will be based on co-ordinates that have been transformed from buffer to surface, then back to buffer. It should be a cleanly reversible transformation, and keeping things separate forever seemed like a lot of headache (especially if people tried to mix the two requests). > The spec for wl_surface.commit is actually lacking since the > introduction of buffer_scale and buffer_transform (and wp_viewport). It > says that pending wl_buffer is applied first and all the rest second. > This wording was to ensure that the new surface size takes effect > before e.g. any regions are clipped to it. But wl_buffer is not the > only parameter affecting the surface size, buffer_scale, transform and > viewport affect it too. So scale, transform, and viewport must be > considered together the wl_buffer. > > I think the above rationale also answers the question about when you > can convert damage from one coordinate space to another: it must happen > with the final new buffer_scale, buffer_transform, and wp_viewport > parameters. Therefore the conversion cannot happen before > wl_surface.commit, and one must collect two regions until commit. This makes sense to me. > This is too much to infer from the spec, so we should explain it > somehow. How about mandating the collection of two regions and saying > conversion can happen only with the final parameters? > > It could be as simple as changing damage_buffer spec to talk about > pending damage_buffer region. The notes about conversion should > probably be added to wl_surface.commit, as that attempts (but already > fails) to document how this all is supposed to work together. Sounds reasonable, I'll try to write this up. > Thanks, > pq > _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
