[PATCH weston 09/11] compositor: Add buffer_damage

[Derek Foreman]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 26/11/15 08:54 AM, Pekka Paalanen wrote:
> On Wed, 18 Nov 2015 16:32:32 -0600 Derek Foreman
> <derekf at osg.samsung.com> wrote:
> 
>> Add an implementation of wl_surface.buffer_damage, similar to 
>> wl_surface.damage except it uses buffer co-ordinates.
>> 
>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com> --- 
>> src/compositor.c | 90
>> ++++++++++++++++++++++++++++++++++++++++++++++---------- 
>> src/compositor.h |  4 ++- 2 files changed, 78 insertions(+), 16
>> deletions(-)
> 
> Hi Derek,
> 
> a very good idea to rename weston_surface_state::damage to 
> damage_surface, nicely points out all places it was used so 
> damage_buffer follows easily.
> 
>> 
>> diff --git a/src/compositor.c b/src/compositor.c index
>> 0efb325..9a02b34 100644 --- a/src/compositor.c +++
>> b/src/compositor.c @@ -525,7 +525,8 @@
>> weston_surface_state_init(struct weston_surface_state *state) 
>> state->sx = 0; state->sy = 0;
>> 
>> -	pixman_region32_init(&state->damage); +
>> pixman_region32_init(&state->damage_surface); +
>> pixman_region32_init(&state->damage_buffer); 
>> pixman_region32_init(&state->opaque); 
>> region_init_infinite(&state->input);
>> 
>> @@ -552,7 +553,8 @@ weston_surface_state_fini(struct
>> weston_surface_state *state)
>> 
>> pixman_region32_fini(&state->input); 
>> pixman_region32_fini(&state->opaque); -
>> pixman_region32_fini(&state->damage); +
>> pixman_region32_fini(&state->damage_surface); +
>> pixman_region32_fini(&state->damage_buffer);
>> 
>> if (state->buffer) 
>> wl_list_remove(&state->buffer_destroy_listener.link); @@ -2577,8
>> +2579,23 @@ surface_damage(struct wl_client *client, if (width <
>> 0 || height < 0) return;
>> 
>> -	pixman_region32_union_rect(&surface->pending.damage, -
>> &surface->pending.damage, +
>> pixman_region32_union_rect(&surface->pending.damage_surface, +
>> &surface->pending.damage_surface, +				   x, y, width, height); 
>> +} + +static void +surface_damage_buffer(struct wl_client
>> *client, +		      struct wl_resource *resource, +		      int32_t
>> x, int32_t y, int32_t width, int32_t height) +{ +	struct
>> weston_surface *surface = wl_resource_get_user_data(resource); + 
>> +	if (width < 0 || height < 0) +		return;
> 
> I assume this gets changed along with patch 8.

Yeah, my local copy has the change.

>> + +	pixman_region32_union_rect(&surface->pending.damage_buffer, +
>> &surface->pending.damage_buffer, x, y, width, height); }
>> 
>> @@ -2741,6 +2758,40 @@ weston_surface_build_buffer_matrix(struct
>> weston_surface *surface, weston_matrix_scale(matrix,
>> vp->buffer.scale, vp->buffer.scale, 1); }
>> 
>> +/* Translate pending damage in buffer co-ordinates to surface +
>> * co-ordinates and union it with a pixman_region32_t. + * This
>> should only be called after the buffer is attached. + */ +static
>> void +apply_damage_buffer(pixman_region32_t *dest, +		    struct
>> weston_surface *surface, +		    struct weston_surface_state
>> *state) +{ +	struct weston_buffer *buffer =
>> surface->buffer_ref.buffer; + +	/* wl_surface.damage_buffer needs
>> to be clipped to the buffer, +	 * translated into surface
>> co-ordinates and unioned with +	 * any other surface damage. +	 *
>> None of this makes sense if there is no buffer though. +	 */ +	if
>> (buffer && pixman_region32_not_empty(&state->damage_buffer)) { +
>> pixman_region32_t buffer_damage; + +
>> pixman_region32_intersect_rect(&state->damage_buffer, +
>> &state->damage_buffer, +					       0, 0, buffer->width, +
>> buffer->height); +		pixman_region32_init(&buffer_damage); +
>> weston_matrix_transform_region(&buffer_damage, +
>> &surface->buffer_to_surface_matrix, +
>> &state->damage_buffer); +		pixman_region32_union(dest, dest,
>> &buffer_damage); +		pixman_region32_fini(&buffer_damage); +	} +
>> /* We should clear this on commit even if there was no buffer */ 
>> +	pixman_region32_clear(&state->damage_buffer); +}
> 
> This looks correct.
> 
>> + static void weston_surface_commit_state(struct weston_surface
>> *surface, struct weston_surface_state *state) @@ -2774,15
>> +2825,20 @@ weston_surface_commit_state(struct weston_surface
>> *surface, state->newly_attached = 0; 
>> state->buffer_viewport.changed = 0;
>> 
>> -	/* wl_surface.damage */ +	/* wl_surface.damage and
>> wl_surface.damage_buffer */ if (weston_timeline_enabled_ && -
>> pixman_region32_not_empty(&state->damage)) +
>> (pixman_region32_not_empty(&state->damage_surface) || +
>> pixman_region32_not_empty(&state->damage_buffer))) 
>> TL_POINT("core_commit_damage", TLP_SURFACE(surface), TLP_END); + 
>> pixman_region32_union(&surface->damage, &surface->damage, -
>> &state->damage); +			      &state->damage_surface); + +
>> apply_damage_buffer(&surface->damage, surface, state); + 
>> pixman_region32_intersect_rect(&surface->damage,
>> &surface->damage, 0, 0, surface->width, surface->height); -
>> pixman_region32_clear(&state->damage); +
>> pixman_region32_clear(&state->damage_surface);
> 
> Looking good.
> 
>> 
>> /* wl_surface.set_opaque_region */ 
>> pixman_region32_init(&opaque); @@ -2901,7 +2957,8 @@ static const
>> struct wl_surface_interface surface_interface = { 
>> surface_set_input_region, surface_commit, 
>> surface_set_buffer_transform, -	surface_set_buffer_scale +
>> surface_set_buffer_scale, +	surface_damage_buffer };
>> 
>> static void @@ -3030,11 +3087,12 @@
>> weston_subsurface_commit_to_cache(struct weston_subsurface *sub) 
>> * translated to correspond to the new surface coordinate system *
>> original_mode. */ -
>> pixman_region32_translate(&sub->cached.damage, +
>> pixman_region32_translate(&sub->cached.damage_surface, 
>> -surface->pending.sx, -surface->pending.sy); -
>> pixman_region32_union(&sub->cached.damage, &sub->cached.damage, -
>> &surface->pending.damage); -
>> pixman_region32_clear(&surface->pending.damage); +
>> pixman_region32_union(&sub->cached.damage_surface, +
>> &sub->cached.damage_surface, +
>> &surface->pending.damage_surface); +
>> pixman_region32_clear(&surface->pending.damage_surface);
>> 
>> if (surface->pending.newly_attached) { sub->cached.newly_attached
>> = 1; @@ -3048,6 +3106,8 @@
>> weston_subsurface_commit_to_cache(struct weston_subsurface *sub) 
>> sub->cached.sx += surface->pending.sx; sub->cached.sy +=
>> surface->pending.sy;
>> 
>> +	apply_damage_buffer(&sub->cached.damage_surface, surface,
>> &surface->pending); +
> 
> Hmm, yeah, I think that's right.

Took me a while to figure out that bit.  I tested it by making
client/window.c use wl_surface.damage_buffer instead of
wl_surface.damage, and I ran the weston-subsurfaces demo with, iirc,
- --red-mode=1

>> sub->cached.buffer_viewport.changed |= 
>> surface->pending.buffer_viewport.changed; 
>> sub->cached.buffer_viewport.buffer = @@ -4458,7 +4518,7 @@
>> compositor_bind(struct wl_client *client, struct wl_resource
>> *resource;
>> 
>> resource = wl_resource_create(client, &wl_compositor_interface, -
>> MIN(version, 3), id); +				      MIN(version, 4), id);
> 
> Let's remove this mistake while we are here: don't use MIN(), just
> use 'version'. I think libwayland-server already guarantees we
> don't get invalid versions.
> 
> Clients need to use MIN(), not servers.

Yup, looks like registry_bind() posts an error if global->version <
requested version.

> Can be a separate patch.

I'll wipe all the MIN() out shortly.

>> if (resource == NULL) { wl_client_post_no_memory(client); 
>> return; @@ -4544,7 +4604,7 @@ weston_compositor_create(struct
>> wl_display *display, void *user_data) ec->output_id_pool = 0; 
>> ec->repaint_msec = DEFAULT_REPAINT_WINDOW;
>> 
>> -	if (!wl_global_create(ec->wl_display, &wl_compositor_interface,
>> 3, +	if (!wl_global_create(ec->wl_display,
>> &wl_compositor_interface, 4, ec, compositor_bind)) goto fail;
>> 
>> diff --git a/src/compositor.h b/src/compositor.h index
>> 79348a1..686380e 100644 --- a/src/compositor.h +++
>> b/src/compositor.h @@ -907,7 +907,9 @@ struct
>> weston_surface_state { int32_t sy;
>> 
>> /* wl_surface.damage */ -	pixman_region32_t damage; +
>> pixman_region32_t damage_surface; +	/* wl_surface.damage_buffer
>> */ +	pixman_region32_t damage_buffer;
>> 
>> /* wl_surface.set_opaque_region */ pixman_region32_t opaque;
> 
> Yeah!
> 
> This patch implements the damage_buffer spec v3 as written, so: 
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk> with
> the couple of trivial changes pointed out above.

Thanks for the review!
(obviously this can't land until after damage_buffer does, so I'll
make the trivial changes and put it aside for now)

> Thanks, pq
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJWV0R+AAoJEF5USY5pfxHXtQIH/1GWke4ta65rOxBc8I8CNuGs
3qmzIMmDtcn1470H3oZEaLAJW4A+frVbR4g70cOMJqO0cB8QOtZmpUZL3A1B3c1c
foPX7NfELcA8Kgk3RIbuL40peJrU9shYj8m4g48bGnMWpfvaekp+xKeckjNFvoen
tSxvRE6xmfMQvY3ufs4mElpHxH0uiDV6yrEXEKsfPxXF3PDw7rGIH+kj6H8azfjj
W8KMhmAXTFqwhMMGa5o/ngkHbwUIbpLdpgJnavOF9+hMy3d9tFGo2EC7vPFR0l0Y
nWCWEixvD6Znlt0l54ubdcSSOqBT3uVBbL9bktqtYUXOd+3K9rgBup6Iit28oMY=
=w8i4
-----END PGP SIGNATURE-----