<div dir="ltr">And I forgot reply-all again. This is why I shouldn't respond from my phone.<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Feb 25, 2014 at 8:07 AM, Pekka Paalanen <span dir="ltr"><<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">On Tue, 25 Feb 2014 07:44:24 -0600<br>
<div><div class="h5">Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br>
<br>
> I'll do a full reply later, just a quick one now.<br>
> On Feb 25, 2014 2:31 AM, "Pekka Paalanen" <<a href="mailto:ppaalanen@gmail.com">ppaalanen@gmail.com</a>> wrote:<br>
> ><br>
> > On Mon, 24 Feb 2014 22:32:05 -0600<br>
> > Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br>
> ><br>
> > > When buffer_transform and buffer_scale were first introduced, added, we<br>
> > > specified surface damage to be in surface coordinates. However, this<br>
> does<br>
> ><br>
> > Don't forget wl_viewport.<br>
> ><br>
> > > not and will never work properly with EGL. Because the buffer transform<br>
> > > and scale are handled directly by the client and not passed through EGL,<br>
> > > the EGL implementation cannot damage the buffer in surface coordinates.<br>
> > > This is a problem both for regular eglSwapBuffers which damages the<br>
> entire<br>
> > > surface and for eglSwapBuffersWithDamage which only partially damages<br>
> the<br>
> > > surface.<br>
> ><br>
> > With this change, you will allow SwapBuffersWithDamage to report proper<br>
> > damage, but the plain SwapBuffers is still forced to damage the whole. I<br>
> > guess that's the intention anyway, right?<br></div></div></blockquote><div><br></div><div>eglSwapBuffersWithDamage should be blindly passing damage rectangles on to the compositor. It's then up to the client to use it correctly. See below.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="h5">
> ><br>
> > > As far as I can see, there are three options for fixing this:<br>
> > > 1) Add EGL extensions (most likely eglSurfaceAttrib values) to set the<br>
> > > buffer scale and transform so that EGL is aware of them.<br>
> > > 2) Change the protocol now before the problem spreads.<br>
> > > 3) Compositors forever ignore damage on non-shm surfaces.<br>
> > ><br>
> > > I don't like 1) because we've already worked fairly hard to separate EGL<br>
> > > from the rest of the protocol, and I don't think we want to even more<br>
> EGL<br>
> > > extensions. I don't like 3) because it makes damage tracking completely<br>
> > > useless. Therefore, I propose 2).<br>
> ><br>
> > It's a nice idea...<br>
> ><br>
> > > Lest people think that we actually have to break protocol here, we<br>
> don't.<br>
> > > Compositors will have to detect surface version 4 and treat damage<br>
> > > accordingly. From a client perspective, it simply means that you can't<br>
> use<br>
> > > buffer_transform or buffer_scale with EGL unless you have a version 4<br>
> > > wl_surface.<br>
> ><br>
> > ...but this execution causes a subtle change in behaviour based on the<br>
> > interface version in both compositors and clients. Especially clients<br>
> > cannot simply just stay using damage in surface coordinates, unless<br>
> > they also stick to version <4, but OTOH, gently forcing everyone to<br>
> > migrate might be desireable?<br></div></div></blockquote><div><br></div><div>Yes it does. And it means that clients will have to be careful. Or they can make their lives simpler by simply not using buffer_transform or buffer_scale unless they have at least version 4. IMHO, I always like damage in buffer coordinates better anyway.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="h5">
> ><br>
> > > ---<br>
> > ><br>
> > > If this gets generally approved, I'll hack up some weston patches to<br>
> > > implement it. It won't take too much code. Just need something to do<br>
> > > inverse transforms of regions.<br>
> ><br>
> > Not only that, but saying damage is in buffer coordinates requires that<br>
> > damage will be queueable state in the Presentation extension. It's not<br>
> > a show-stopper, but now we would need to queue or not queue damage<br>
> > based on the bound interface version. It sounds pretty complicated to<br>
> > me.<br>
> ><br>
> > OTOH, queueable damage is something that was raised in the Presentation<br>
> > discussions, but punted as "can add later if needed". It could be<br>
> > useful.<br></div></div></blockquote><div><br></div><div>Honestly, I don't see any problem with queuing automatically implying max damage for now. It makes the surface/buffer state split not as nice, but I don't know that it's a problem. If we do decide to queue it, clients won't partially damage out-of-order surfaces anyway because there's a huge race there.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="h5">
> ><br>
> > ><br>
> > > protocol/wayland.xml | 16 +++++++++++++---<br>
> > > 1 file changed, 13 insertions(+), 3 deletions(-)<br>
> > ><br>
> > > diff --git a/protocol/wayland.xml b/protocol/wayland.xml<br>
> > > index bf6acd1..873d0f9 100644<br>
> > > --- a/protocol/wayland.xml<br>
> > > +++ b/protocol/wayland.xml<br>
> > > @@ -176,7 +176,7 @@<br>
> > > </event><br>
> > > </interface><br>
> > ><br>
> > > - <interface name="wl_compositor" version="3"><br>
> > > + <interface name="wl_compositor" version="4"><br>
> > > <description summary="the compositor singleton"><br>
> > > A compositor. This object is a singleton global. The<br>
> > > compositor is in charge of combining the contents of multiple<br>
> > > @@ -962,7 +962,7 @@<br>
> > > </event><br>
> > > </interface><br>
> > ><br>
> > > - <interface name="wl_surface" version="3"><br>
> > > + <interface name="wl_surface" version="4"><br>
> > > <description summary="an onscreen surface"><br>
> > > A surface is a rectangular area that is displayed on the screen.<br>
> > > It has a location, size and pixel contents.<br>
> > > @@ -1041,7 +1041,9 @@<br>
> > ><br>
> > > Damage is double-buffered state, see wl_surface.commit.<br>
> > ><br>
> > > - The damage rectangle is specified in surface local coordinates.<br>
> > > + In versions 3 and previous, the damage rectangle was specified in<br>
> > > + surface local coordinates. In versions 4 and above, the damage<br>
> > > + rectangle is specified in buffer coordinates.<br>
> > ><br>
> > > The initial value for pending damage is empty: no damage.<br>
> > > wl_surface.damage adds pending damage: the new pending damage<br>
> > > @@ -1212,6 +1214,10 @@<br>
> > > Note that if the transform value includes 90 or 270 degree<br>
> rotation,<br>
> > > the width of the buffer will become the surface height and the<br>
> height<br>
> > > of the buffer will become the surface width.<br>
> > > +<br>
> > > + Note: It is recommended that you do not use buffer_transform with<br>
> > > + EGL unless your wl_surface has version at least 4. Otherwise, EGL<br>
> > > + will not report damage correctly.<br>
> ><br>
> > Need more wording here I think: full-surface damage as implemented by<br>
> > Mesa will still work. Just add the word "partial" to damage?<br>
><br>
> That's just the problem, if doesn't. I found this thanks to mesa. It<br>
> reports full damage but reports it sideways. Open up Weston on KMS with<br>
> either a 90 or 270 rotation, turn on fan debugging, and fire up<br>
> weston-terminal.<br>
<br>
</div></div>Are you sure? See:<br>
<a href="http://cgit.freedesktop.org/mesa/mesa/tree/src/egl/drivers/dri2/platform_wayland.c?h=10.0#n602" target="_blank">http://cgit.freedesktop.org/mesa/mesa/tree/src/egl/drivers/dri2/platform_wayland.c?h=10.0#n602</a></blockquote>
<div><br></div><div>Absolutely positive. With weston-terminal running on SHM, there's no problem because it submits damage in surface coordinates. With it running on my old EGL, it doesn't damage correctly because damage is submitted in buffer coordinates. I can provide a debug log if you'd like.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
<br>
I damages an area of (0,0)-(INT32_MAX, INT32_MAX). That's slightly<br>
ugly, but perfectly valid.<br>
<br>
<a href="http://cgit.freedesktop.org/mesa/mesa/commit/?id=bce64c6c83122b1f4a684cc7890c7a61d2f9ffd7" target="_blank">http://cgit.freedesktop.org/mesa/mesa/commit/?id=bce64c6c83122b1f4a684cc7890c7a61d2f9ffd7</a><br>
<br>
Can we rely on this?<br></blockquote><div><br></div><div>I'm running off whatever version of mesa is packaged for Fedora 20. It must not have that patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><br>
> ><br>
> > > </description><br>
> > > <arg name="transform" type="int"/><br>
> > > </request><br>
> > > @@ -1236,6 +1242,10 @@<br>
> > > Note that if the scale is larger than 1, then you have to attach<br>
> > > a buffer that is larger (by a factor of scale in each dimension)<br>
> > > than the desired surface size.<br>
> > > +<br>
> > > + Note: It is recommended that you do not use buffer_scale with<br>
> > > + EGL unless your wl_surface has version at least 4. Otherwise, EGL<br>
> > > + will not report damage correctly.<br>
> ><br>
> > Ditto.<br>
> ><br>
> > > </description><br>
> > > <arg name="scale" type="int"/><br>
> > > </request><br>
> ><br>
> > And then you need to do the same with wl_viewport.<br>
> ><br>
> > An alternative would be adding a new request "buffer_damage" instead of<br>
> > modifying the behaviour of "damage". It would be much easier to<br>
> > document that: "damage is not queued, while buffer_damage is." It<br>
> > raises the question of what happens if both are set; we can just take<br>
> > the union in the compositor.<br>
><br>
> Nope. We could add a surface_damage for things that really want it bit I<br>
> don't see the point. We need to fix damage for the EGL implementations we<br>
> can't change.<br>
<br>
</div>Umm, precisely the EGL implementations we cannot change will continue<br>
using the surface coordinates semantics, and using wl_surface<br>
version < 4. If you keep backwards compatibility in the protocol, these<br>
will stay exactly as broken as they are now.<br></blockquote><div><br></div><div>No EGL implementations are using surface coordinates now and they never will because they don't know what surface coordinates are. Since the EGL implementation is not aware of the buffer transform or buffer scale, they are going to do one of two things: Either they will do what Mesa apparently now does now and damage the universe, or they will damage what they know which is the buffer size positioned at 0, 0. If they damage the universe then changing coordinate systems won't affect anything. However, if they were written against 1.0 where you could assume buffer size == surface size and they damage the buffer size then they're already broken and this fixes them.</div>
<div><br></div><div>The problem is that, while we can patch Mesa, we can't patch everything. There will be EGL implementations that continue to damage just the buffer they know. Also, we have no good way client-side or server-side to detect if the EGL implementation damages just the buffer or, like mesa, damages the universe. As such, we can't trust their damage unless we specify that it's in buffer coordinates.</div>
<div><br></div><div>Are we breaking eglSwapBuffersWithDamage? I don't think so. Implementations should be blindly passing damage rectangles at which point it's up to the client to ensure that they're passed in the right coordinates. Unless the EGL implementation is going through the extra work of trying to clip the damage to the buffer before it passes it to the compositor, we should be fine fine. If the EGL implementation is clipping them, then it's already terribly broken because, again, it can't possibly clip them to surface coordinates correctly anyway.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
<br>
Thanks,<br>
pq<br>
<div class=""><div class="h5"><br>
><br>
> ><br>
> > Using a new request would be slightly more clear to me, and it would<br>
> > allow clients to continue using surface damage if it is more<br>
> > convenient, now that surface damage has been the only way so far.<br></div></div></blockquote><div><br></div><div>Unfortunately, that won't work. Again, the problem is that EGL only knows buffer coordinates, so that's how it submits it's damage.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">
> ><br>
> > Technically, I don't see any reason why your proposal would not work. I<br>
> > do see opportunities for confusion. My alternative is not much better<br>
> > than your proposal, either.<br>
> ><br>
> > I agree that the proposal does have tangible benefits: EGL with<br>
> > damage, and queued damage. It's probably fine, just takes a bit of time<br>
> > to digest.<br>
> ><br>
> ><br>
> > Thanks,<br>
> > pq<br>
<br>
</div></div></blockquote></div><br></div></div>