[PATCH weston 1/4] Move animation_list to weston_output().

Kristian Høgsberg hoegsberg at gmail.com
Thu Jun 7 14:13:44 PDT 2012


On Thu, Jun 07, 2012 at 09:12:29AM -0600, Scott Moreau wrote:
> Here we introduce a pending animation_list to which animations are
> added by weston_animation_run(). This fixes a timing issue when
> an animation is started from a keybinding and makes it so frame
> handlers are called only once per a single output's refresh.

Looks good, not much code after all.  A few comments below.

Kristian

> ---
>  src/compositor.c |   47 +++++++++++++++++++++++++++++++++++++++++------
>  src/compositor.h |   11 ++++++++++-
>  src/util.c       |   10 ++++------
>  3 files changed, 55 insertions(+), 13 deletions(-)
> 
> diff --git a/src/compositor.c b/src/compositor.c
> index 3039c3d..9110d4c 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -968,8 +968,7 @@ fade_frame(struct weston_animation *animation,
>  	if (weston_spring_done(&compositor->fade.spring)) {
>  		compositor->fade.spring.current =
>  			compositor->fade.spring.target;
> -		wl_list_remove(&animation->link);
> -		wl_list_init(&animation->link);
> +		weston_animation_stop(animation);
>  
>  		if (compositor->fade.spring.current < 0.001) {
>  			destroy_surface(&surface->surface.resource);
> @@ -981,6 +980,30 @@ fade_frame(struct weston_animation *animation,
>  	}
>  }
>  
> +WL_EXPORT void
> +weston_animation_run(struct weston_compositor *compositor,
> +			struct weston_output *output,
> +			struct weston_animation *animation)
> +{
> +	struct weston_output *o;
> +
> +	if (output == NULL) {
> +		/* Select first output in list */
> +		wl_list_for_each(o, &compositor->output_list, link)
> +			break;
> +	} else
> +		o = output;

Hmm, if it's a weston_animation_* function it should take a
weston_animation as its first argument.  And the compositor argument
is only there so we can get the first output from it if output is
NULL...  I think just 

	weston_output_run_animation(output, animation) and

and the couple of cases where we don't have a surface to provide the
output we'll just have to use

	output = container_of(compositor->output_list.next,
			      struct weston_output, link);

> +	wl_list_insert(o->animation_list.pending.prev, &animation->link);
> +}
> +
> +WL_EXPORT void
> +weston_animation_stop(struct weston_animation *animation)
> +{
> +	wl_list_remove(&animation->link);
> +	wl_list_init(&animation->link);
> +}
> +
>  struct weston_frame_callback {
>  	struct wl_resource resource;
>  	struct wl_list link;
> @@ -1064,8 +1087,19 @@ weston_output_repaint(struct weston_output *output, int msecs)
>  		wl_resource_destroy(&cb->resource);
>  	}
>  
> -	wl_list_for_each_safe(animation, next, &ec->animation_list, link)
> +	wl_list_for_each_safe(animation, next,
> +				&output->animation_list.current, link)
>  		animation->frame(animation, output, msecs);
> +
> +	if (!wl_list_empty(&output->animation_list.pending)) {
> +		wl_list_for_each_safe(animation, next,
> +					&output->animation_list.pending, link) {
> +			wl_list_insert(output->animation_list.current.prev,

Use

	wl_list_insert_list(output->animation_list.prev,
			    &output->pending_animation_list)

to move the list elements in constant time.

> +							&animation->link);
> +		}
> +		wl_list_init(&output->animation_list.pending);
> +		weston_output_damage(output);

Use weston_compositor_schedule_repaint() here instead.
weston_output_damage() makes us repaint the entire output.  We don't
want to actually have to repaint anything.

> +	}
>  }
>  
>  static int
> @@ -1171,8 +1205,8 @@ weston_compositor_fade(struct weston_compositor *compositor, float tint)
>  
>  	weston_surface_damage(compositor->fade.surface);
>  	if (wl_list_empty(&compositor->fade.animation.link))
> -		wl_list_insert(compositor->animation_list.prev,
> -			       &compositor->fade.animation.link);
> +		weston_animation_run(compositor, NULL,
> +					&compositor->fade.animation);
>  }
>  
>  static void
> @@ -2843,6 +2877,8 @@ weston_output_init(struct weston_output *output, struct weston_compositor *c,
>  
>  	wl_signal_init(&output->frame_signal);
>  	wl_list_init(&output->frame_callback_list);
> +	wl_list_init(&output->animation_list.current);
> +	wl_list_init(&output->animation_list.pending);
>  	wl_list_init(&output->resource_list);
>  
>  	output->id = ffs(~output->compositor->output_id_pool) - 1;
> @@ -2967,7 +3003,6 @@ weston_compositor_init(struct weston_compositor *ec,
>  	wl_list_init(&ec->key_binding_list);
>  	wl_list_init(&ec->button_binding_list);
>  	wl_list_init(&ec->axis_binding_list);
> -	wl_list_init(&ec->animation_list);
>  	weston_spring_init(&ec->fade.spring, 30.0, 1.0, 1.0);
>  	ec->fade.animation.frame = fade_frame;
>  	wl_list_init(&ec->fade.animation.link);
> diff --git a/src/compositor.h b/src/compositor.h
> index 2d52048..30cb7bb 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -131,6 +131,10 @@ struct weston_output {
>  	struct weston_compositor *compositor;
>  	struct weston_matrix matrix;
>  	struct wl_list frame_callback_list;
> +	struct {
> +		struct wl_list current;
> +		struct wl_list pending;
> +	} animation_list;

Can we just call them animation_list and pending_animation_list?  I
don't think these nested struct always makes things clearer.

>  	int32_t x, y, mm_width, mm_height;
>  	struct weston_border border;
>  	pixman_region32_t region;
> @@ -287,7 +291,6 @@ struct weston_compositor {
>  	struct wl_list key_binding_list;
>  	struct wl_list button_binding_list;
>  	struct wl_list axis_binding_list;
> -	struct wl_list animation_list;
>  	struct {
>  		struct weston_spring spring;
>  		struct weston_animation animation;
> @@ -531,6 +534,12 @@ weston_compositor_activity(struct weston_compositor *compositor);
>  void
>  weston_compositor_update_drag_surfaces(struct weston_compositor *compositor);
>  
> +void
> +weston_animation_run(struct weston_compositor *compositor,
> +				struct weston_output *output,
> +				struct weston_animation *animation);
> +void
> +weston_animation_stop(struct weston_animation *animation);
>  
>  struct weston_binding;
>  typedef void (*weston_key_binding_handler_t)(struct wl_seat *seat,
> diff --git a/src/util.c b/src/util.c
> index 3d28f8f..23cf222 100644
> --- a/src/util.c
> +++ b/src/util.c
> @@ -108,7 +108,7 @@ struct weston_fade {
>  static void
>  weston_zoom_destroy(struct weston_zoom *zoom)
>  {
> -	wl_list_remove(&zoom->animation.link);
> +	weston_animation_stop(&zoom->animation);
>  	wl_list_remove(&zoom->listener.link);
>  	wl_list_remove(&zoom->transform.link);
>  	zoom->surface->geometry.dirty = 1;
> @@ -188,8 +188,7 @@ weston_zoom_run(struct weston_surface *surface, GLfloat start, GLfloat stop,
>  	wl_signal_add(&surface->surface.resource.destroy_signal,
>  		      &zoom->listener);
>  
> -	wl_list_insert(&surface->compositor->animation_list,
> -		       &zoom->animation.link);
> +	weston_animation_run(surface->compositor, NULL, &zoom->animation);

Should be running out surface->output, not the default output.

Also, you should remove the timestamp = weston_compositor_get_time()
initializations (here and other animations) now that we solved that
problem.

>  
>  	return zoom;
>  }
> @@ -443,7 +442,7 @@ weston_environment_get_fd(const char *env)
>  static void
>  weston_fade_destroy(struct weston_fade *fade)
>  {
> -	wl_list_remove(&fade->animation.link);
> +	weston_animation_stop(&fade->animation);
>  	wl_list_remove(&fade->listener.link);
>  	fade->surface->geometry.dirty = 1;
>  	if (fade->done)
> @@ -510,8 +509,7 @@ weston_fade_run(struct weston_surface *surface,
>  	wl_signal_add(&surface->surface.resource.destroy_signal,
>  		      &fade->listener);
>  
> -	wl_list_insert(&surface->compositor->animation_list,
> -		       &fade->animation.link);
> +	weston_animation_run(surface->compositor, NULL, &fade->animation);

Same here, run on surface->output.

>  	return fade;
>  }
> -- 
> 1.7.7.6
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel


More information about the wayland-devel mailing list