[PATCH] protocol: Change wl_surface.damage to be in buffer coordinates.

Kristian Høgsberg krh at bitplanet.net
Tue Feb 25 12:28:09 PST 2014


On Tue, Feb 25, 2014 at 8:13 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> And I forgot reply-all again.  This is why I shouldn't respond from my
> phone.
>
>
> On Tue, Feb 25, 2014 at 8:07 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>>
>> On Tue, 25 Feb 2014 07:44:24 -0600
>> Jason Ekstrand <jason at jlekstrand.net> wrote:
>>
>> > I'll do a full reply later, just a quick one now.
>>
>> > On Feb 25, 2014 2:31 AM, "Pekka Paalanen" <ppaalanen at gmail.com> wrote:
>> > >
>> > > On Mon, 24 Feb 2014 22:32:05 -0600
>> > > Jason Ekstrand <jason at jlekstrand.net> wrote:
>> > >
>> > > > When buffer_transform and buffer_scale were first introduced, added,
>> > > > we
>> > > > specified surface damage to be in surface coordinates.  However,
>> > > > this
>> > does
>> > >
>> > > Don't forget wl_viewport.
>> > >
>> > > > not and will never work properly with EGL.  Because the buffer
>> > > > transform
>> > > > and scale are handled directly by the client and not passed through
>> > > > EGL,
>> > > > the EGL implementation cannot damage the buffer in surface
>> > > > coordinates.
>> > > > This is a problem both for regular eglSwapBuffers which damages the
>> > entire
>> > > > surface and for eglSwapBuffersWithDamage which only partially
>> > > > damages
>> > the
>> > > > surface.
>> > >
>> > > With this change, you will allow SwapBuffersWithDamage to report
>> > > proper
>> > > damage, but the plain SwapBuffers is still forced to damage the whole.
>> > > I
>> > > guess that's the intention anyway, right?
>
>
> eglSwapBuffersWithDamage should be blindly passing damage rectangles on to
> the compositor.  It's then up to the client to use it correctly.  See below.
>
>>
>> > >
>> > > > As far as I can see, there are three options for fixing this:
>> > > >  1) Add EGL extensions (most likely eglSurfaceAttrib values) to set
>> > > > the
>> > > >     buffer scale and transform so that EGL is aware of them.
>> > > >  2) Change the protocol now before the problem spreads.
>> > > >  3) Compositors forever ignore damage on non-shm surfaces.
>> > > >
>> > > > I don't like 1) because we've already worked fairly hard to separate
>> > > > EGL
>> > > > from the rest of the protocol, and I don't think we want to even
>> > > > more
>> > EGL
>> > > > extensions.  I don't like 3) because it makes damage tracking
>> > > > completely
>> > > > useless.  Therefore, I propose 2).
>> > >
>> > > It's a nice idea...
>> > >
>> > > > Lest people think that we actually have to break protocol here, we
>> > don't.
>> > > > Compositors will have to detect surface version 4 and treat damage
>> > > > accordingly.  From a client perspective, it simply means that you
>> > > > can't
>> > use
>> > > > buffer_transform or buffer_scale with EGL unless you have a version
>> > > > 4
>> > > > wl_surface.
>> > >
>> > > ...but this execution causes a subtle change in behaviour based on the
>> > > interface version in both compositors and clients. Especially clients
>> > > cannot simply just stay using damage in surface coordinates, unless
>> > > they also stick to version <4, but OTOH, gently forcing everyone to
>> > > migrate might be desireable?
>
>
> 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.
>
>>
>> > >
>> > > > ---
>> > > >
>> > > > If this gets generally approved, I'll hack up some weston patches to
>> > > > implement it.  It won't take too much code.  Just need something to
>> > > > do
>> > > > inverse transforms of regions.
>> > >
>> > > Not only that, but saying damage is in buffer coordinates requires
>> > > that
>> > > damage will be queueable state in the Presentation extension. It's not
>> > > a show-stopper, but now we would need to queue or not queue damage
>> > > based on the bound interface version. It sounds pretty complicated to
>> > > me.
>> > >
>> > > OTOH, queueable damage is something that was raised in the
>> > > Presentation
>> > > discussions, but punted as "can add later if needed". It could be
>> > > useful.
>
>
> 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.
>
>>
>> > >
>> > > >
>> > > >  protocol/wayland.xml | 16 +++++++++++++---
>> > > >  1 file changed, 13 insertions(+), 3 deletions(-)
>> > > >
>> > > > diff --git a/protocol/wayland.xml b/protocol/wayland.xml
>> > > > index bf6acd1..873d0f9 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
>> > > > @@ -962,7 +962,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.
>> > > > @@ -1041,7 +1041,9 @@
>> > > >
>> > > >       Damage is double-buffered state, see wl_surface.commit.
>> > > >
>> > > > -     The damage rectangle is specified in surface local
>> > > > coordinates.
>> > > > +     In versions 3 and previous, the damage rectangle was specified
>> > > > in
>> > > > +     surface local coordinates.  In versions 4 and above, 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
>> > > > @@ -1212,6 +1214,10 @@
>> > > >       Note that if the transform value includes 90 or 270 degree
>> > rotation,
>> > > >       the width of the buffer will become the surface height and the
>> > height
>> > > >       of the buffer will become the surface width.
>> > > > +
>> > > > +     Note: It is recommended that you do not use buffer_transform
>> > > > with
>> > > > +     EGL unless your wl_surface has version at least 4.  Otherwise,
>> > > > EGL
>> > > > +     will not report damage correctly.
>> > >
>> > > Need more wording here I think: full-surface damage as implemented by
>> > > Mesa will still work. Just add the word "partial" to damage?
>> >
>> > That's just the problem, if doesn't.  I found this thanks to mesa.  It
>> > reports full damage but reports it sideways.  Open up Weston on KMS with
>> > either a 90 or 270 rotation, turn on fan debugging, and fire up
>> > weston-terminal.
>>
>> Are you sure? See:
>>
>> http://cgit.freedesktop.org/mesa/mesa/tree/src/egl/drivers/dri2/platform_wayland.c?h=10.0#n602
>
>
> 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.
>
>>
>>
>>
>> I damages an area of (0,0)-(INT32_MAX, INT32_MAX). That's slightly
>> ugly, but perfectly valid.
>>
>>
>> http://cgit.freedesktop.org/mesa/mesa/commit/?id=bce64c6c83122b1f4a684cc7890c7a61d2f9ffd7
>>
>> Can we rely on this?
>
>
> I'm running off whatever version of mesa is packaged for Fedora 20.  It must
> not have that patch.

We can't change existing  behavior here.  Mesa was change to damage
the max rectangle to fix the rotated terminal issue.  See mesa commit
bce64c6c83122b1f4a684cc7890c7a61d2f9ffd7 and bug
https://bugs.freedesktop.org/show_bug.cgi?id=70250.  surface.damage
takes surface coordinates and for plain eglSwapBuffer that means we
have to damage INT32_MAX x INT32_MAX.  For eglSwapBufferWithDamage,
the application is responsible for passing in damage rects in surface
coordinates.

Kristian


>>
>>
>> > >
>> > > >        </description>
>> > > >        <arg name="transform" type="int"/>
>> > > >      </request>
>> > > > @@ -1236,6 +1242,10 @@
>> > > >       Note that if the scale is larger than 1, then you have to
>> > > > attach
>> > > >       a buffer that is larger (by a factor of scale in each
>> > > > dimension)
>> > > >       than the desired surface size.
>> > > > +
>> > > > +     Note: It is recommended that you do not use buffer_scale with
>> > > > +     EGL unless your wl_surface has version at least 4.  Otherwise,
>> > > > EGL
>> > > > +     will not report damage correctly.
>> > >
>> > > Ditto.
>> > >
>> > > >        </description>
>> > > >        <arg name="scale" type="int"/>
>> > > >      </request>
>> > >
>> > > And then you need to do the same with wl_viewport.
>> > >
>> > > An alternative would be adding a new request "buffer_damage" instead
>> > > of
>> > > modifying the behaviour of "damage". It would be much easier to
>> > > document that: "damage is not queued, while buffer_damage is." It
>> > > raises the question of what happens if both are set; we can just take
>> > > the union in the compositor.
>> >
>> > Nope.  We could add a surface_damage for things that really want it bit
>> > I
>> > don't see the point.  We need to fix damage for the EGL implementations
>> > we
>> > can't change.
>>
>> Umm, precisely the EGL implementations we cannot change will continue
>> using the surface coordinates semantics, and using wl_surface
>> version < 4. If you keep backwards compatibility in the protocol, these
>> will stay exactly as broken as they are now.
>
>
> 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.
>
> 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.
>
> 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.
>
>>
>>
>> Thanks,
>> pq
>>
>>
>> >
>> > >
>> > > Using a new request would be slightly more clear to me, and it would
>> > > allow clients to continue using surface damage if it is more
>> > > convenient, now that surface damage has been the only way so far.
>
>
> Unfortunately, that won't work.  Again, the problem is that EGL only knows
> buffer coordinates, so that's how it submits it's damage.
>
>>
>> > >
>> > > Technically, I don't see any reason why your proposal would not work.
>> > > I
>> > > do see opportunities for confusion. My alternative is not much better
>> > > than your proposal, either.
>> > >
>> > > I agree that the proposal does have tangible benefits: EGL with
>> > > damage, and queued damage. It's probably fine, just takes a bit of
>> > > time
>> > > to digest.
>> > >
>> > >
>> > > Thanks,
>> > > pq
>>
>
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>


More information about the wayland-devel mailing list