<div class="gmail_quote">On Thu, Jun 7, 2012 at 3:13 PM, Kristian Høgsberg <<a href="mailto:hoegsberg@gmail.com" target="_blank">hoegsberg@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im">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. </div>Looks good, not much code after all. A few comments below. Kristian </blockquote><div> Hi Kristian, thanks for reviewing this set. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > --- > 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; </div></div>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); </blockquote><div> Yep, that makes sense. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im"> > + 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, </div>Use wl_list_insert_list(output->animation_list.prev, &output->pending_animation_list) to move the list elements in constant time. </blockquote><div> I didn't know about this before, will do. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im"> > + &animation->link); > + } > + wl_list_init(&output->animation_list.pending); > + weston_output_damage(output); </div>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. </blockquote><div> ack. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > + } > } > > 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; </div></div>Can we just call them animation_list and pending_animation_list? I don't think these nested struct always makes things clearer. </blockquote><div> Sure, no problem. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > 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); </div></div>Should be running out surface->output, not the default output. </blockquote><div> Yes, completely overlooked this. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Also, you should remove the timestamp = weston_compositor_get_time() initializations (here and other animations) now that we solved that problem. </blockquote><div> I'm not really clear what you want to happen here. The timestamp should now be set in weston_output_repaint() when adding the pending animations to the animations list? or where? </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div class="im"> > > 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); </div>Same here, run on surface->output. </blockquote><div> Indeed. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > return fade; > } > -- > 1.7.7.6 > > _______________________________________________ > wayland-devel mailing list > <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </blockquote></div>