<div dir="ltr"><div><div>You are correct that it is equally valid to convert buffer_damage coordinates to surface coordinates and store the union of those. </div><div>More importantly, I think you are correct that the "new" transform should be used. Ideally the last transform the client sent (even though it is not committed yet). I made a mistake of thinking you had to use the old transform to go from surface to buffer coordinates, that was wrong. This also means that the compositor only has to track the current transformation and only has to keep one copy of it that is replaced when it gets the request, so it is probably the simplest. </div><div>I figure clients will send damage rectangles produced by their drawing toolkit as damage requests, which are necessarily in buffer coordinates. If the client is correctly implemented and using wl_surface.damage, it will transform this from buffer coordinates to surface coordinates. My belief is that most clients are written on the assumption that the compositor immediately transforms these back, and therefore have to use any transform they already have and cannot use a transform that was not sent yet. So it probably uses whatever transform it just sent. Although allowing the final composite to be performed on only a region is really nice, I suspect the main advantage of buffer damage is to reduce the size of the rectangle copied from the buffer to gpu texture memory, which is why I always figured buffer coordinates are more useful for compositors and why clients would assume compositors would immediately translate back. </div></div></div><div class="gmail_extra"> <div class="gmail_quote">On Fri, Nov 20, 2015 at 6:37 PM, Derek Foreman <<a href="mailto:derekf@osg.samsung.com" target="_blank">derekf@osg.samsung.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 20/11/15 07:39 PM, Bill Spitzak wrote: > I am pretty certain you can just convert wl_surface.damage coordinates > *to* buffer coordinates, using the transform the old buffer was attached > with, and thus merge all the wl_surface.damage and > wl_surface.damage_buffer into a single region in buffer space. Yep, that should be fine. We have all the code to do that already since we previously stored damage in surface co-ords exclusively and gl texture uploads needed buffer co-ordinates. We need the co-ordinates in both spaces regardless - surface as a step to screen space co-ordinates for that damage, and buffer for texture damage. I chose to convert buffer space co-ordinates to screen space since all the existing code used screen space. I just dumped it all into the accumulated surface space region after the buffer to surface matrix was set up. > If the transformation does not change, you can back-transform this > region into the changed region in screen space. > > You seem to be worried about what happens if the transformation is > different between the old and new buffers. But in that case the > on-screen changed area is the union of the entire old buffer with it's > old transform and the new buffer with it's new transform. The damage > region is ignored when figuring out the screen-space damage so it does > not matter what it was set to. Well, I'm somewhat concerned about loss of precision due to the double transform - but I think the matrices we're working with should be ok for that. I'm also annoyed that wasted math gets done (buffer to surface and back again), but I think avoiding that wasted transform is probably far more code complexity than is worthwhile to eliminate what's not very much math. I'm actually not bothered by the transform being changed - I think we just have to specify that all damage is posted in the "new co-ordinates" that the rest of the commit sets up, then we don't have to worry. We do have an existing bug - either in simple-damage or weston - where changing transformations results in bad damage. weston-simple-damage --rotating-transform shows this. This has been around before damage_buffer existed, so I haven't bothered to dig into it yet. Now that I think of it, I think the client's posting bad damage - it's posting the remove for the old bouncing-green-ball and the arrival of the new frame of bouncing-green-ball as if they were in the same co-ordinate space. > If a client actually sends the "correct" wl_surface.damage for a > transform change, it will be a region that translates to cover the > entire old buffer. This may be excessive but is harmless. Nod. I guess some of our trivial test apps (simple-damage has a fixed white border) could preserve some buffer content over certain transform changes, but in the real world I can't imagine anyone trying to do this. > I suppose rotating a perfectly square buffer which is filled with a > solid color could, under the old system, turn into no damage, while the > new one will not know (unless it compares the buffers) and damage an > unchanged area. However this seems really trivial. Any client that > actually tries to do this correctly with wl_surface.damage will send a > region that necessarily translates to something that includes all the > damage to the buffer. You just broke my head. :) I think we agree that on a transform change any app (that isn't impossibly clever) doing the right thing will damage its entire buffer or its entire surface, depending on which request it chooses. And that the end result should be the same regardless which they chose. I may be missing some trick in that last paragraph, but I don't think it's one that could be usefully leveraged by a real world client? Maybe this takes us back to the simple-damage bug. Even if it translated the damage from the previous frame to the new frame space and posted damage "properly", it would still be broken with the GL renderer because the buffer damage wouldn't be properly accounted for. The pixman renderer doesn't need the buffer damage (no texture upload), so it might actually draw the right thing if the surface damage were correct. It's far too late on a Friday to verify any of this. I'll report back sometime next week. ;) Thanks, Derek > On Fri, Nov 20, 2015 at 1:49 PM, Derek Foreman <<a href="mailto:derekf@osg.samsung.com">derekf@osg.samsung.com</a> <div><div class="h5">> <mailto:<a href="mailto:derekf@osg.samsung.com">derekf@osg.samsung.com</a>>> 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: > <a href="https://bugs.freedesktop.org/show_bug.cgi?id=78190" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=78190</a> > by making the problem irrelevant. > > Signed-off-by: Derek Foreman <<a href="mailto:derekf@osg.samsung.com">derekf@osg.samsung.com</a> </div></div>> <mailto:<a href="mailto:derekf@osg.samsung.com">derekf@osg.samsung.com</a>>> <div><div class="h5">> --- > > New change: Explain that damages can't be combined until commit > > Also, copy the new wl_surface.damage text that doesn't require attach > to be before damage > > (enough text is new that I haven't carried over any RBs) > > protocol/wayland.xml | 48 > ++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 46 insertions(+), 2 deletions(-) > > diff --git a/protocol/wayland.xml b/protocol/wayland.xml > index 525e092..4d75f39 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. > @@ -1109,6 +1109,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"/> > @@ -1325,6 +1329,46 @@ > </description> > <arg name="scale" type="int"/> > </request> > + > +  > + <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 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_buffer 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. > + > + 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 wp_viewport > + or when a drawing library (like EGL) is unaware of buffer scale > + and buffer transform. > + > + Since it is impossible to convert between buffer and surface > + co-ordinates until the late stages of wl_surface.commit, > + damage from wl_surface.damage and wl_surace.damage_buffer must > + be accumulated separately and only combined during > + wl_surface.commit. > + </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"> > -- > 2.6.2 > > _______________________________________________ > wayland-devel mailing list > <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> </div></div>> <mailto:<a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a>> > <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> > > </blockquote></div> </div>