Move elapsed-time calculations into ClutterTimeline

Instead of calculating a delta in the master clock, and passing that
into each timeline, make each timeline individually responsible for
remembering the last time and computing the delta.

This:

 - Fixes a problem where we could spin infinitely processing
   timeline-only frames with < 1msec differences.
 - Makes timelines consistently start timing on the first frame;
   instead of doing different things for the first started timeline
   and other timelines.
 - Improves accuracy of elapsed time computations by avoiding
   accumulating microsecond => millisecond truncation errors.

http://bugzilla.openedhand.com/show_bug.cgi?id=1637

Signed-off-by: Emmanuele Bassi <ebassi@linux.intel.com>

6 files changed, 48 insertions, 95 deletions

diff --git a/clutter/clutter-master-clock.c b/clutter/clutter-master-clock.c
index 8e6d569b1..08a5c744f 100644
--- a/clutter/clutter-master-clock.c
+++ b/clutter/clutter-master-clock.c
@@ -66,8 +66,6 @@ struct _ClutterMasterClock
    * a redraw on the stage and drive the animations
    */
   GSource *source;
-
-  guint timelines_running : 1;
 };
 
 struct _ClutterMasterClockClass
@@ -364,8 +362,6 @@ _clutter_master_clock_remove_timeline (ClutterMasterClock *master_clock,
 {
   master_clock->timelines = g_slist_remove (master_clock->timelines,
                                             timeline);
-  if (master_clock->timelines == NULL)
-    master_clock->timelines_running = FALSE;
 }
 
 /*
@@ -395,39 +391,14 @@ _clutter_master_clock_start_running (ClutterMasterClock *master_clock)
 void
 _clutter_master_clock_advance (ClutterMasterClock *master_clock)
 {
-  gulong msecs;
-  GSList *l;
+  GSList *l, *next;
 
   g_return_if_fail (CLUTTER_IS_MASTER_CLOCK (master_clock));
 
-  if (master_clock->timelines == NULL)
-    return;
-
-  if (!master_clock->timelines_running)
-    {
-      /* When we start running, we count the first frame as time 0,
-       * so we don't need an advance. (And master_clock->prev_tick
-       * doesn't meaningfully relate to these timelines.)
-       */
-      master_clock->timelines_running = TRUE;
-      return;
-    }
-
-  msecs = (master_clock->cur_tick.tv_sec - master_clock->prev_tick.tv_sec) * 1000
-        + (master_clock->cur_tick.tv_usec - master_clock->prev_tick.tv_usec) / 1000;
-
-  if (msecs == 0)
-    return;
-
-  CLUTTER_NOTE (SCHEDULER, "Advancing %d timelines by %lu milliseconds",
-                g_slist_length (master_clock->timelines),
-                msecs);
-
-  for (l = master_clock->timelines; l != NULL; l = l->next)
+  for (l = master_clock->timelines; l != NULL; l = next)
     {
-      ClutterTimeline *timeline = l->data;
+      next = l->next;
 
-      if (clutter_timeline_is_playing (timeline))
-        clutter_timeline_advance_delta (timeline, msecs);
+      clutter_timeline_do_tick (l->data, &master_clock->cur_tick);
     }
 }
diff --git a/clutter/clutter-timeline.c b/clutter/clutter-timeline.c
index 45093d404..2d68bc5d1 100644
--- a/clutter/clutter-timeline.c
+++ b/clutter/clutter-timeline.c
@@ -62,8 +62,13 @@ struct _ClutterTimelinePrivate
 
   GHashTable *markers_by_name;
 
+  /* Time we last advanced the elapsed time and showed a frame */
+  GTimeVal last_frame_time;
+
   guint loop       : 1;
   guint is_playing : 1;
+  /* If we've just started playing and haven't yet gotten a tick from the master clock */
+  guint waiting_first_tick : 1;
 };
 
 typedef struct {
@@ -489,13 +494,18 @@ set_is_playing (ClutterTimeline *timeline,
   priv->is_playing = is_playing;
   master_clock = _clutter_master_clock_get_default ();
   if (priv->is_playing)
-    _clutter_master_clock_add_timeline (master_clock, timeline);
+    {
+      _clutter_master_clock_add_timeline (master_clock, timeline);
+      priv->waiting_first_tick = TRUE;
+    }
   else
-    _clutter_master_clock_remove_timeline (master_clock, timeline);
+    {
+      _clutter_master_clock_remove_timeline (master_clock, timeline);
+    }
 }
 
 static gboolean
-clutter_timeline_advance_internal (ClutterTimeline *timeline)
+clutter_timeline_do_frame (ClutterTimeline *timeline)
 {
   ClutterTimelinePrivate *priv;
 
@@ -1135,19 +1145,18 @@ clutter_timeline_get_delta (ClutterTimeline *timeline)
 }
 
 /*
- * clutter_timeline_advance_delta:
+ * clutter_timeline_do_tick
  * @timeline: a #ClutterTimeline
- * @msecs: advance in milliseconds
+ * @tick_time: time of advance
  *
- * Advances @timeline by @msecs. This function is called by the master
- * clock and it is used to advance a timeline by the amount of milliseconds
- * elapsed since the last redraw operation. The @timeline will use this
- * interval to emit the #ClutterTimeline::new-frame signal and eventually
- * skip frames.
+ * Advances @timeline based on the time passed in @msecs. This
+ * function is called by the master clock. The @timeline will use this
+ * interval to emit the #ClutterTimeline::new-frame signal and
+ * eventually skip frames.
  */
 void
-clutter_timeline_advance_delta (ClutterTimeline *timeline,
-                                guint            msecs)
+clutter_timeline_do_tick (ClutterTimeline *timeline,
+			  GTimeVal        *tick_time)
 {
   ClutterTimelinePrivate *priv;
 
@@ -1155,9 +1164,25 @@ clutter_timeline_advance_delta (ClutterTimeline *timeline,
 
   priv = timeline->priv;
 
-  priv->msecs_delta = msecs;
+  if (priv->waiting_first_tick)
+    {
+      priv->last_frame_time = *tick_time;
+      priv->waiting_first_tick = FALSE;
+    }
+  else
+    {
+      gint msecs =
+	(tick_time->tv_sec - priv->last_frame_time.tv_sec) * 1000
+        + (tick_time->tv_usec - priv->last_frame_time.tv_usec) / 1000;
 
-  clutter_timeline_advance_internal (timeline);
+      if (msecs != 0)
+	{
+	  /* Avoid accumulating error */
+	  g_time_val_add (&priv->last_frame_time, msecs * 1000L);
+	  priv->msecs_delta = msecs;
+	  clutter_timeline_do_frame (timeline);
+	}
+    }
 }
 
 static inline void
diff --git a/clutter/clutter-timeline.h b/clutter/clutter-timeline.h
index 3b372408f..2cb83e7d5 100644
--- a/clutter/clutter-timeline.h
+++ b/clutter/clutter-timeline.h
@@ -155,8 +155,8 @@ void             clutter_timeline_advance_to_marker     (ClutterTimeline *timeli
                                                          const gchar     *marker_name);
 
 /*< private >*/
-void             clutter_timeline_advance_delta         (ClutterTimeline *timeline,
-                                                         guint            msecs);
+void             clutter_timeline_do_tick               (ClutterTimeline *timeline,
+							 GTimeVal        *tick_time);
 
 G_END_DECLS
 
diff --git a/tests/conform/test-timeline-interpolate.c b/tests/conform/test-timeline-interpolate.c
index c4837a5ec..29fe9263f 100644
--- a/tests/conform/test-timeline-interpolate.c
+++ b/tests/conform/test-timeline-interpolate.c
@@ -24,8 +24,6 @@ typedef struct _TestState
   gint completion_count;
   gboolean passed;
   guint source_id;
-  GTimeVal prev_tick;
-  gulong msecs_delta;
 } TestState;
 
 
@@ -139,21 +137,11 @@ frame_tick (gpointer data)
 {
   TestState *state = data;
   GTimeVal cur_tick = { 0, };
-  gulong msecs;
 
   g_get_current_time (&cur_tick);
 
-  if (state->prev_tick.tv_sec == 0)
-    state->prev_tick = cur_tick;
-
-  msecs = (cur_tick.tv_sec - state->prev_tick.tv_sec) * 1000
-        + (cur_tick.tv_usec - state->prev_tick.tv_usec) / 1000;
-
   if (clutter_timeline_is_playing (state->timeline))
-   clutter_timeline_advance_delta (state->timeline, msecs);
-
-  state->msecs_delta = msecs;
-  state->prev_tick = cur_tick;
+   clutter_timeline_do_tick (state->timeline, &cur_tick);
 
   return TRUE;
 }
@@ -180,9 +168,6 @@ test_timeline_interpolate (TestConformSimpleFixture *fixture,
   state.new_frame_counter = 0;
   state.passed = TRUE;
   state.expected_frame = 0;
-  state.prev_tick.tv_sec = 0;
-  state.prev_tick.tv_usec = 0;
-  state.msecs_delta = 0;
 
   state.source_id =
     clutter_threads_add_frame_source (60, frame_tick, &state);
diff --git a/tests/conform/test-timeline-rewind.c b/tests/conform/test-timeline-rewind.c
index b1cfa13c0..67668c55f 100644
--- a/tests/conform/test-timeline-rewind.c
+++ b/tests/conform/test-timeline-rewind.c
@@ -12,8 +12,6 @@ typedef struct _TestState
   ClutterTimeline *timeline;
   gint rewind_count;
   guint source_id;
-  GTimeVal prev_tick;
-  gulong msecs_delta;
 } TestState;
 
 static gboolean
@@ -74,21 +72,11 @@ frame_tick (gpointer data)
 {
   TestState *state = data;
   GTimeVal cur_tick = { 0, };
-  gulong msecs;
 
   g_get_current_time (&cur_tick);
 
-  if (state->prev_tick.tv_sec == 0)
-    state->prev_tick = cur_tick;
-
-  msecs = (cur_tick.tv_sec - state->prev_tick.tv_sec) * 1000
-        + (cur_tick.tv_usec - state->prev_tick.tv_usec) / 1000;
-
   if (clutter_timeline_is_playing (state->timeline))
-   clutter_timeline_advance_delta (state->timeline, msecs);
-
-  state->msecs_delta = msecs;
-  state->prev_tick = cur_tick;
+   clutter_timeline_do_tick (state->timeline, &cur_tick);
 
   return TRUE;
 }
@@ -111,9 +99,6 @@ test_timeline_rewind (TestConformSimpleFixture *fixture,
 		 (GSourceFunc)watchdog_timeout,
                  &state);
   state.rewind_count = 0;
-  state.prev_tick.tv_sec = 0;
-  state.prev_tick.tv_usec = 0;
-  state.msecs_delta = 0;
 
   state.source_id =
     clutter_threads_add_frame_source (60, frame_tick, &state);
diff --git a/tests/conform/test-timeline.c b/tests/conform/test-timeline.c
index 94568095e..0a1038e54 100644
--- a/tests/conform/test-timeline.c
+++ b/tests/conform/test-timeline.c
@@ -185,9 +185,6 @@ typedef struct _FrameCounter    FrameCounter;
 
 struct _FrameCounter
 {
-  GTimeVal prev_tick;
-  gulong msecs_delta;
-
   GSList *timelines;
 };
 
@@ -197,27 +194,17 @@ frame_tick (gpointer data)
   FrameCounter *counter = data;
   GTimeVal cur_tick = { 0, };
   GSList *l;
-  gulong msecs;
 
   g_get_current_time (&cur_tick);
 
-  if (counter->prev_tick.tv_sec == 0)
-    counter->prev_tick = cur_tick;
-
-  msecs = (cur_tick.tv_sec - counter->prev_tick.tv_sec) * 1000
-        + (cur_tick.tv_usec - counter->prev_tick.tv_usec) / 1000;
-
   for (l = counter->timelines; l != NULL; l = l->next)
     {
       ClutterTimeline *timeline = l->data;
 
       if (clutter_timeline_is_playing (timeline))
-        clutter_timeline_advance_delta (timeline, msecs);
+        clutter_timeline_do_tick (timeline, &cur_tick);
     }
 
-  counter->msecs_delta = msecs;
-  counter->prev_tick = cur_tick;
-
   return TRUE;
 }