[PATCH weston 1/2] compositor: Generalize output previous damage into per buffer damage

[Ander Conselvan de Oliveira]

On 09/14/2012 08:40 PM, Kristian Høgsberg wrote:
> On Fri, Sep 14, 2012 at 04:12:03PM +0300, Ander Conselvan de Oliveira wrote:
>> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
>>
>> This is a more generic fix for the issue solved in 4f521731 where
>> damage obscured by overlays could be lost in one of the output buffers
>> due to rapid move of a surface in an overlay plane.
>>
>> This changes the renderer so it keeps track of the damage in each
>> buffer. Every time a new frame is drawn, the damage of the frame is
>> added to all the buffers and the rendered regions are cleared from
>> the current buffer's damage.
>
> Very nice, it wasn't too much work after all, and I think the logic is
> actually easier to follow now.
>
> Committed as it, but one small comment/thought below.
>
> Kristian
>
>> ---
>>   src/compositor.c     |   30 ++++++++++++++----------------
>>   src/compositor.h     |    3 ++-
>>   src/gles2-renderer.c |   17 ++++++++++++++---
>>   src/screenshooter.c  |   15 ++++++++++++---
>>   4 files changed, 42 insertions(+), 23 deletions(-)
>>
>> diff --git a/src/compositor.c b/src/compositor.c
>> index 8cb8a3f..bda1381 100644
>> --- a/src/compositor.c
>> +++ b/src/compositor.c

[...]

>> @@ -896,21 +896,10 @@ weston_output_repaint(struct weston_output *output, uint32_t msecs)
>>   	pixman_region32_fini(&opaque);
>>
>>   	pixman_region32_init(&output_damage);
>> -
>> -	pixman_region32_init(&new_damage);
>> -	pixman_region32_copy(&new_damage, &ec->primary_plane.damage);
>> -
>> -	pixman_region32_union(&ec->primary_plane.damage,
>> -			      &ec->primary_plane.damage,
>> -			      &output->previous_damage);
>> -
>> -	pixman_region32_intersect(&output->previous_damage,
>> -				  &new_damage, &output->region);
>> -
>>   	pixman_region32_intersect(&output_damage,
>>   				  &ec->primary_plane.damage, &output->region);
>> -
>> -	pixman_region32_fini(&new_damage);
>> +	pixman_region32_subtract(&ec->primary_plane.damage,
>> +				 &ec->primary_plane.damage, &output->region);
>
> This effectively removes the double buffer swap assumption from the
> core repaint loop and damage tracking, which is very nice.  We just
> pass the region to repaint into the renderer.

I figured this made more sense in the renderer. Specially if one day we 
get support for Rob Bragg's EGL_EXT_buffer_age extension.

[...]

>> diff --git a/src/gles2-renderer.c b/src/gles2-renderer.c
>> index 0e8b8ce..761f4fe 100644
>> --- a/src/gles2-renderer.c
>> +++ b/src/gles2-renderer.c

[...]

>> @@ -736,6 +737,14 @@ gles2_renderer_repaint_output(struct weston_output *output,
>>   		pixman_region32_fini(&undamaged);
>>   	}
>>
>> +	for (i = 0; i < 2; i++)
>> +		pixman_region32_union(&output->buffer_damage[i],
>> +				      &output->buffer_damage[i],
>> +				      output_damage);
>> +
>> +	pixman_region32_union(output_damage, output_damage,
>> +			      &output->buffer_damage[output->current_buffer]);
>> +
>
> output->buffer_damage[output->current_buffer] is already the union of
> output_damage and output->buffer_damage[output->current_buffer] here.
> I was going to suggest that we just pass
> output->buffer_damage[output->current_buffer] to repaint_surfaces, but
> that won't work.  We can't change the output_damage region during
> repaint since then some surfaces won't get repainted.  But we could
> just copy output->buffer_damage[output->current_buffer] to output
> damage instead of doing another union.

Yeah, that would be better. I noticed that but didn't fix for some 
reason. I guess I thought it made the code more confusing, although on 
second thought, we should really go for efficiency in this path.

Cheers,
Ander

>>   	repaint_surfaces(output, output_damage);
>>
>>   	wl_signal_emit(&output->frame_signal, output);
>> @@ -747,6 +756,8 @@ gles2_renderer_repaint_output(struct weston_output *output,
>>   		print_egl_error_state();
>>   	}
>>
>> +	output->current_buffer ^= 1;
>> +
>>   }
>>
>>   static void
>> diff --git a/src/screenshooter.c b/src/screenshooter.c
>> index ba80ce5..ffcc970 100644
>> --- a/src/screenshooter.c
>> +++ b/src/screenshooter.c
>> @@ -261,7 +261,7 @@ weston_recorder_frame_notify(struct wl_listener *listener, void *data)
>>   	struct weston_output *output = data;
>>   	uint32_t msecs = output->frame_time;
>>   	pixman_box32_t *r;
>> -	pixman_region32_t damage;
>> +	pixman_region32_t damage, *previous_damage;
>>   	int i, j, k, n, width, height, run, stride;
>>   	uint32_t delta, prev, *d, *s, *p, next;
>>   	struct {
>> @@ -270,9 +270,18 @@ weston_recorder_frame_notify(struct wl_listener *listener, void *data)
>>   	} header;
>>   	struct iovec v[2];
>>
>> +	/* When recording, this will be exactly the region that was repainted
>> +	 * in this frame. Since overlays are disabled, the whole primary plane
>> +	 * damage is rendered. For the first frame, the whole output will be
>> +	 * damaged and that damage will be added to both buffers causing the
>> +	 * non-current buffer damage to be while output. Rendering will clear
>> +	 * all the damage in the current buffer so in the next frame (when
>> +	 * that is non-current) the only damage left will be the one added
>> +	 * from the primary plane. */
>> +	previous_damage = &output->buffer_damage[output->current_buffer ^ 1];
>> +
>>   	pixman_region32_init(&damage);
>> -	pixman_region32_intersect(&damage, &output->region,
>> -				  &output->previous_damage);
>> +	pixman_region32_intersect(&damage, &output->region, previous_damage);
>>
>>   	r = pixman_region32_rectangles(&damage, &n);
>>   	if (n == 0)
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel