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

[Jason Ekstrand]

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.


>
> > >
> > > >        </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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140225/25cfeb9f/attachment-0001.html>