2008-03-07 Robert Bragg <bob@o-hand.com>

* clutter/clutter-timeline.c:
	Timeline changes to fix the issues identified in bugzilla #439
	
	Notably, this includes some tweaks to timeline semantics. So e.g. for a   
	10 frame timeline here are some points about the new timeline code:
	- When you create a timeline it starts with current_frame_num == 0
	- After starting a timeline, the first timeout is for
	  current_frame_num == 1 (Notably it isn't 0 since there is a delay
	  before the first timeout signals so re-asserting the starting point
	  would give a longer than average first frame.)
	- For a non looping timeline the last timeout would be for
	  current_frame_num == 10
	- For a looping timeline the timeout for current_frame_num == 10 would      
	  be followed by a timeout for current_frame_num == 1 and frame 0 is
	  considered == frame 10.
	- Asking for a timeline of N frames might better be described as asking    
	  for a timeline of _length_ N.

	Warning: Although I tried to test things, I guess it's quite likely that
	this breaks somthing depending on a specific quirk of the previous
	timeline code.

ChangeLog

@@ -1,3 +1,27 @@
+2008-03-07  Robert Bragg  <bob@o-hand.com>
+
+	* clutter/clutter-timeline.c:
+	Timeline changes to fix the issues identified in bugzilla #439
+	
+	Notably, this includes some tweaks to timeline semantics. So e.g. for a   
+	10 frame timeline here are some points about the new timeline code:
+	- When you create a timeline it starts with current_frame_num == 0
+	- After starting a timeline, the first timeout is for
+	  current_frame_num == 1 (Notably it isn't 0 since there is a delay
+	  before the first timeout signals so re-asserting the starting point
+	  would give a longer than average first frame.)
+	- For a non looping timeline the last timeout would be for
+	  current_frame_num == 10
+	- For a looping timeline the timeout for current_frame_num == 10 would      
+	  be followed by a timeout for current_frame_num == 1 and frame 0 is
+	  considered == frame 10.
+	- Asking for a timeline of N frames might better be described as asking    
+	  for a timeline of _length_ N.
+
+	Warning: Although I tried to test things, I guess it's quite likely that
+	this breaks somthing depending on a specific quirk of the previous
+	timeline code.
+
 2008-03-06  Emmanuele Bassi  <ebassi@openedhand.com>
 
 	* clutter/clutter-actor.c:

README

@@ -142,6 +142,20 @@ wanting to port to newer releases (See NEWS for general new feature info).
 
 Release Notes for Clutter 0.8
 -------------------------------
+* Some more focused timeline unit tests have been added and some tweaks to
+  timeline semantics were made; E.g. For a 10 frame timeline here are some
+  points about the semantics:
+  - When you create a timeline it starts with current_frame_num == 0
+  - After starting a timeline, the first timeout is for current_frame_num == 1
+    (Notably it isn't 0 since there is a delay before the first timeout signals
+    so re-asserting the starting frame (0) wouldn't make sense.)
+  - For a non looping timeline the last timeout would be for
+    current_frame_num == 10
+  - For a looping timeline the timeout for current_frame_num == 10 would be
+    followed by a timeout for current_frame_num == 1 and frame 0 is considered
+    == frame 10.
+  - Asking for a timeline of N frames might better be described as asking for
+    a timeline of _length_ N.
 
 * The behaviour of clutter_actor_get_opacity() has been slightly changed;
   instead of returning the composited opacity of the entire parents chain

clutter/clutter-timeline.c

@@ -73,8 +73,7 @@ struct _ClutterTimelinePrivate
 
   gint skipped_frames;
 
-  gulong last_frame_msecs;
+  GTimeVal prev_frame_timeval;
-  gulong start_frame_secs;
   guint  msecs_delta;
 
   guint loop : 1;
@@ -457,29 +456,11 @@ timeline_timeout_func (gpointer data)
   ClutterTimeline        *timeline = data;
   ClutterTimelinePrivate *priv;
   GTimeVal                timeval;
-  gint                    n_frames;
+  guint                   n_frames;
   gulong                  msecs;
-  gboolean                retval = TRUE;
 
   priv = timeline->priv;
 
-  if (!timeline)
-    {
-      CLUTTER_NOTE (SCHEDULER,
-                    "The timeline [%p] has been disposed",
-                    timeline);
-      return FALSE;
-    }
-
-  if (!priv->timeout_id)
-    {
-      CLUTTER_NOTE (SCHEDULER,
-                    "The timeline [%p] has been removed",
-                    timeline);
-
-      return FALSE;
-    }
-
   g_object_ref (timeline);
 
   /* Figure out potential frame skips */
@@ -489,57 +470,34 @@ timeline_timeout_func (gpointer data)
                      timeline,
                      priv->current_frame_num);
 
-  /* Fire off signal */
+  if (!priv->prev_frame_timeval.tv_sec)
-  g_signal_emit (timeline, timeline_signals[NEW_FRAME], 0,
-                 priv->current_frame_num);
-
-  /* Signal removes source ? */
-  if (!priv->timeout_id)
     {
-      g_object_unref (timeline);
+      CLUTTER_NOTE (SCHEDULER,
-      return FALSE;
+                    "Timeline [%p] recieved timeout before being initialised!",
+                    timeline);
+      priv->prev_frame_timeval = timeval;
     }
 
-  if (priv->last_frame_msecs)
+  /* Interpolate the current frame based on the timeval of the
+   * previous frame */
+  msecs = (timeval.tv_sec - priv->prev_frame_timeval.tv_sec) * 1000;
+  msecs += (timeval.tv_usec - priv->prev_frame_timeval.tv_usec)/1000;
+  priv->msecs_delta = msecs;
+  n_frames = msecs / (1000 / priv->fps);
+  if (n_frames == 0)
+      n_frames = 1;
+
+  priv->skipped_frames = n_frames - 1;
+
+  if (priv->skipped_frames)
     {
-      /* Check time diff from out last call and adjust number
+      CLUTTER_TIMESTAMP (SCHEDULER,
-       * of frames to advance accordingly.
+                         "Timeline [%p], skipping %d frames\n",
-      */
+                         timeline,
-      msecs = ((timeval.tv_sec - priv->start_frame_secs) * 1000)
+                         priv->skipped_frames);
-              + (timeval.tv_usec / 1000);
-      n_frames = (msecs - priv->last_frame_msecs)
-                 / (1000 / priv->fps);
-      priv->msecs_delta = msecs - priv->last_frame_msecs;
-
-      if (n_frames <= 0)
-        {
-          n_frames = 1;
-          priv->skipped_frames = 0;
-        }
-      else if (n_frames > 1)
-	{
-	  CLUTTER_TIMESTAMP (SCHEDULER,
-			     "Timeline [%p], skipping %d frames\n",
-			     timeline,
-                             n_frames);
-
-          priv->skipped_frames = n_frames - 1;
-	}
-      else
-        priv->skipped_frames = 0;
-    }
-  else
-    {
-      /* First frame, set up timings.*/
-      priv->start_frame_secs = timeval.tv_sec;
-      priv->skipped_frames   = 0;
-      priv->msecs_delta      = 0;
-
-      msecs     = timeval.tv_usec / 1000;
-      n_frames  = 1;
     }
 
-  priv->last_frame_msecs = msecs;
+  priv->prev_frame_timeval = timeval;
 
   /* Advance frames */
   if (priv->direction == CLUTTER_TIMELINE_FORWARD)
@@ -547,12 +505,35 @@ timeline_timeout_func (gpointer data)
   else
     priv->current_frame_num -= n_frames;
 
-  /* Handle loop or stop */
+  /* If we have not reached the end of the timeline: */
-  if (((priv->direction == CLUTTER_TIMELINE_FORWARD) &&
+  if (!(
-       (priv->current_frame_num > priv->n_frames)) ||
+      ((priv->direction == CLUTTER_TIMELINE_FORWARD) &&
+       (priv->current_frame_num >= priv->n_frames)) ||
       ((priv->direction == CLUTTER_TIMELINE_BACKWARD) &&
-       (priv->current_frame_num < 0)))
+       (priv->current_frame_num <= 0))
+       ))
     {
+      /* Fire off signal */
+      g_signal_emit (timeline, timeline_signals[NEW_FRAME], 
+                     0, priv->current_frame_num);
+      
+      /* Signal pauses timeline ? */
+      if (!priv->timeout_id)
+        {
+          g_object_unref (timeline);
+          return FALSE;
+        }
+
+      g_object_unref (timeline);
+      return TRUE;
+    }
+  else /* Handle loop or stop */
+    {
+      ClutterTimelineDirection saved_direction = priv->direction;
+      guint overflow_frame_num = priv->current_frame_num;
+      gint end_frame;
+  
+      /* In case the signal handlers want to take a peek... */
       if (priv->direction == CLUTTER_TIMELINE_FORWARD)
         {
           priv->current_frame_num = priv->n_frames;
@@ -561,6 +542,22 @@ timeline_timeout_func (gpointer data)
         {
           priv->current_frame_num = 0;
         }
+      end_frame = priv->current_frame_num;
+
+      /* Fire off signal */
+      g_signal_emit (timeline, timeline_signals[NEW_FRAME], 
+                     0, priv->current_frame_num);
+
+      /* Did the signal handler modify the current_frame_num */
+      if (priv->current_frame_num != end_frame)
+        {
+          g_object_unref (timeline);
+          return TRUE;
+        }
+
+      /* Note: If the new-frame signal handler paused the timeline
+       * on the last frame we will still go ahead and send the
+       * completed signal */
 
       CLUTTER_NOTE (SCHEDULER,
                     "Timeline [%p] completed (cur: %d, tot: %d, drop: %d)",
@@ -569,40 +566,81 @@ timeline_timeout_func (gpointer data)
                     priv->n_frames,
                     n_frames - 1);
 
-      /* if we skipped some frame to get here let's see whether we still need
+      if (!priv->loop && priv->timeout_id)
-       * to emit the last new-frame signal with the last frame
+        {
-       */
+          /* We remove the timeout now, so that the completed signal handler
-      if (n_frames > 1)
+           * may choose to re-start the timeline 
-	g_signal_emit (timeline, timeline_signals[NEW_FRAME], 0,
+           *
-                       priv->current_frame_num);
+           * ** Perhaps we should remove this earlier, and regardless
+           * of priv->loop. Are we limiting the things that could be done in
+           * the above new-frame signal handler */
+          timeout_remove (priv->timeout_id);
+          priv->timeout_id = 0;
+        }
+      g_signal_emit (timeline, timeline_signals[COMPLETED], 0);
+
+      /* Again check to see if the user has manually played with
+       * current_frame_num, before we finally stop or loop the timeline */
+
+      if (priv->current_frame_num != end_frame && 
+          !(/* Except allow moving from frame 0 -> n_frame (or vica-versa)
+               since these are considered equivalent */
+            (priv->current_frame_num == 0 && end_frame == priv->n_frames) ||
+            (priv->current_frame_num == priv->n_frames && end_frame == 0)
+          ))
+        {
+          g_object_unref (timeline);
+          return TRUE;
+        }
 
       if (priv->loop)
-	{
+        {
-	  g_signal_emit (timeline, timeline_signals[COMPLETED], 0);
+          /* We try and interpolate smoothly around a loop */
-	  clutter_timeline_rewind (timeline);
+          if (saved_direction == CLUTTER_TIMELINE_FORWARD)
+            priv->current_frame_num = overflow_frame_num - priv->n_frames;
+          else
+            priv->current_frame_num = priv->n_frames + overflow_frame_num;
 
-          retval = TRUE;
+          /* Or if the direction changed, we try and bounce */
-	}
+          if (priv->direction != saved_direction)
+            priv->current_frame_num =
+              priv->n_frames - priv->current_frame_num;
+
+          g_object_unref (timeline);
+          return TRUE;
+        }
       else
-	{
+        {
-          if (priv->timeout_id)
-            {
-              timeout_remove (priv->timeout_id);
-              priv->timeout_id = 0;
-            }
-
-          priv->last_frame_msecs = 0;
-
-          g_signal_emit (timeline, timeline_signals[COMPLETED], 0);
           clutter_timeline_rewind (timeline);
-
+          priv->prev_frame_timeval.tv_sec = 0;
-	  retval = FALSE;
+          priv->prev_frame_timeval.tv_usec = 0;
-	}
+          g_object_unref (timeline);
+          return FALSE;
+        }
     }
+}
 
-  g_object_unref (timeline);
+static guint
+timeline_timeout_add (ClutterTimeline *timeline,
+                      guint          interval,
+                      GSourceFunc    func,
+                      gpointer       data,
+                      GDestroyNotify notify)
+{
+  ClutterTimelinePrivate *priv;
+  GTimeVal timeval;
 
-  return retval;
+  priv = timeline->priv;
+
+  if (priv->prev_frame_timeval.tv_sec == 0)
+    {
+      g_get_current_time (&timeval);
+      priv->prev_frame_timeval = timeval;
+    }
+  priv->skipped_frames   = 0;
+  priv->msecs_delta      = 0;
+
+  return timeout_add (interval, func, data, notify);
 }
 
 static gboolean
@@ -613,9 +651,10 @@ delay_timeout_func (gpointer data)
 
   priv->delay_id = 0;
 
-  priv->timeout_id = timeout_add (FPS_TO_INTERVAL (priv->fps),
+  priv->timeout_id = timeline_timeout_add (timeline,
-                                  timeline_timeout_func,
+                                           FPS_TO_INTERVAL (priv->fps),
-                                  timeline, NULL);
+                                           timeline_timeout_func,
+                                           timeline, NULL);
 
   g_signal_emit (timeline, timeline_signals[STARTED], 0);
 
@@ -651,9 +690,10 @@ clutter_timeline_start (ClutterTimeline *timeline)
     }
   else
     {
-      priv->timeout_id = timeout_add (FPS_TO_INTERVAL (priv->fps),
+      priv->timeout_id = timeline_timeout_add (timeline,
-                                      timeline_timeout_func,
+                                               FPS_TO_INTERVAL (priv->fps),
-                                      timeline, NULL);
+                                               timeline_timeout_func,
+                                               timeline, NULL);
 
       g_signal_emit (timeline, timeline_signals[STARTED], 0);
     }
@@ -686,7 +726,8 @@ clutter_timeline_pause (ClutterTimeline *timeline)
       priv->timeout_id = 0;
     }
 
-  priv->last_frame_msecs = 0;
+  priv->prev_frame_timeval.tv_sec = 0;
+  priv->prev_frame_timeval.tv_usec = 0;
 
   g_signal_emit (timeline, timeline_signals[PAUSED], 0);
 }
@@ -904,9 +945,10 @@ clutter_timeline_set_speed (ClutterTimeline *timeline,
         {
           timeout_remove (priv->timeout_id);
 
-          priv->timeout_id = timeout_add (FPS_TO_INTERVAL (priv->fps),
+          priv->timeout_id = timeline_timeout_add (timeline,
-                                          timeline_timeout_func,
+                                                   FPS_TO_INTERVAL (priv->fps),
-                                          timeline, NULL);
+                                                   timeline_timeout_func,
+                                                   timeline, NULL);
         }
 
       g_object_notify (G_OBJECT (timeline), "fps");