[PATCH] protocol: Change wl_surface.damage to be in buffer coordinates.
Jason Ekstrand
jason at jlekstrand.net
Wed Feb 26 19:13:16 PST 2014
Hi All,
I talked to Kristian in IRC about this today. It turns out that damage
isn't an issue in the case where we have an integer buffer_scale or a
transform that's a multiple of 180 degress. This is because, in these
cases, the buffer size is at least as large as the surface size and
damaging bigger isn't a problem. It's worth noting that using wl_viewport
to scale up a surface causes the same problem. Allow me to recap:
1) The EGL implementation has no way of knowing about buffer
transformations.
2) The EGL implementation is responsible to call wl_surface.damage inside
of eglSwapBuffers. Because it doesn't know about buffer transformations,
it can't damage a rectangle the same size as the surface.
3) Mesa currently damages an area of INT32_MAX x INT32_MAX to deal with
this. However, in the version 1.0 of the specification, the surface size
was always the same as buffer size. Some EGL implementations (including
older versions of mesa) rely on this and only damage the buffer size.
Therefore, damage is wrong if the surface is rotated by 90 or 270 degrees.
4) Clients have no way to determine if the EGL stack they are using does
max damage or simply damages the buffer area. Therefore, they cannot
assume that the EGL implementation will do the right thing. (Especially
since this used to be perfectly Ok.) This also means that they can't
detect whether or not surface transformations are useable.
As far as I see it, this leaves us with four options:
1) Clients call wl_surface.damage manually right before eglSwapBuffers.
Technically, wl_surface.damage is supposed to be called between
wl_surface.attach and wl_surface.commit. However, I think Weston is fairly
forgiving on this point and it might work.
2) We add an EGL extension for every type of surface transformation.
3) We say that any EGL implementation that does not damage an area of
INT32_MAX x INT32_MAX is broken and hope they change.
4) We change the protocol so damage is in buffer coordinates.
Option 1 breaks the whole idea of eglSwapBuffers and hiding stuff inside of
EGLSurface. Option 2 is right out because we don't want to wait on EGL
implementations before clients can use new protocol bits. That leaves us
with 3 and 4.
As I said, I talked to Kristian on IRC today and he was a much bigger fan
of 3 and trying to hack around things in clients. Initially, we thought
that we could just avoid 90 and 270 degree rotations unless we have
EGL_EXT_swap_buffers_with_damage. The idea was that
eglSwapBuffersWithDamageEXT would simply pass the damage rectangles through
to the compositor so it is then up to the client to hand it the right
damage and buffer transforms would then be 100% ok.
As I was trying to implement this in toytoolkit tonight, I realized that
this doesn't work either and that we've painted ourselves into a corner on
this one. When the EGL_EXT_swap_buffers_with_damage extension was written
the authors, trying to stay consistent with the rest of OpenGL, chose to
make the coordinates of the damage rectangles relative to the lower-left
corner of the surface rather than the upper-left. This means that the EGL
implementation has to vertically flip all of the damage rectangles.
Normally this is a trivial operation. However, if the EGL implementation
does not know the proper height of the surface, the computed rectangles
will be wrong. This means that, not only is eglSwapBuffers broken, but
eglSwapBuffersWithDamage is broken even worse.
Unless we start re-writing EGL extensions, I think we're stuck with option
4.
Thanks,
--Jason Ekstrand
On Tue, Feb 25, 2014 at 2:28 PM, Kristian Høgsberg <krh at bitplanet.net>wrote:
> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140226/cf637a47/attachment-0001.html>
More information about the wayland-devel
mailing list