[PATCH weston 1/4] Move animation_list to weston_output().
Scott Moreau
oreaus at gmail.com
Thu Jun 7 15:01:35 PDT 2012
On Thu, Jun 7, 2012 at 3:13 PM, Kristian Høgsberg <hoegsberg at gmail.com>wrote:
> 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
>
Hi Kristian, thanks for reviewing this set.
>
> > ---
> > 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);
>
Yep, that makes sense.
>
> > + 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.
>
I didn't know about this before, will do.
>
> > + &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.
>
ack.
>
> > + }
> > }
> >
> > 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.
>
Sure, no problem.
>
> > 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.
>
Yes, completely overlooked this.
>
> Also, you should remove the timestamp = weston_compositor_get_time()
> initializations (here and other animations) now that we solved that
> problem.
>
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?
>
> >
> > 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.
>
Indeed.
>
> > return fade;
> > }
> > --
> > 1.7.7.6
> >
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20120607/ce749ea5/attachment-0001.html>
More information about the wayland-devel
mailing list