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

[Pekka Paalanen]

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?

> 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?

> ---
> 
> 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.

> 
>  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?

>        </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.

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.

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