Count timeline frames using the FPS instead of an integer interval

Bug 1495 - Timelines run 4% short

Previously the timelines were timed by calculating the interval
between each frame stored as an integer number of milliseconds so some
precision is lost. For example, requesting 60 frames per second gets
converted to 16 ms per frame which is actually 62.5 frames per
second. This makes the timeline shorter by 4%.

This patch merges the common code for timing from the timeout pools
and frame sources into an internal clutter-timeout-interval file. This
stores the interval directly as the FPS and counts the number of
frames that have been reached instead of the elapsed time.

clutter/Makefile.am

@@ -185,6 +185,7 @@ source_c = \
 	$(srcdir)/clutter-texture.c 		\
 	$(srcdir)/clutter-text.c                \
 	$(srcdir)/clutter-timeline.c 		\
+	$(srcdir)/clutter-timeout-interval.c    \
 	$(srcdir)/clutter-timeout-pool.c	\
 	$(srcdir)/clutter-units.c		\
 	$(srcdir)/clutter-util.c 		\
@@ -199,6 +200,7 @@ source_h_priv = \
 	$(srcdir)/clutter-id-pool.h 		\
 	$(srcdir)/clutter-script-private.h	\
 	$(srcdir)/clutter-stage-window.h	\
+	$(srcdir)/clutter-timeout-interval.h    \
 	$(NULL)
 
 libclutter_@CLUTTER_FLAVOUR@_@CLUTTER_API_VERSION@_la_LIBADD = \

clutter/clutter-frame-source.c

@@ -3,7 +3,7 @@
  *
  * An OpenGL based 'interactive canvas' library.
  *
- * Authored By Matthew Allum  <mallum@openedhand.com>
+ * Authored By Neil Roberts  <neil@linux.intel.com>
  *
  * Copyright (C) 2008 OpenedHand
  *
@@ -28,6 +28,7 @@
 #endif
 
 #include "clutter-frame-source.h"
+#include "clutter-timeout-interval.h"
 
 typedef struct _ClutterFrameSource ClutterFrameSource;
 
@@ -35,8 +36,7 @@ struct _ClutterFrameSource
 {
   GSource source;
 
-  GTimeVal start_time;
+  ClutterTimeoutInterval timeout;
-  guint last_time, frame_time;
 };
 
 static gboolean clutter_frame_source_prepare  (GSource *source, gint *timeout);
@@ -57,7 +57,7 @@ static GSourceFuncs clutter_frame_source_funcs =
  * clutter_frame_source_add_full:
  * @priority: the priority of the frame source. Typically this will be in the
  *            range between #G_PRIORITY_DEFAULT and #G_PRIORITY_HIGH.
- * @interval: the time between calls to the function, in milliseconds
+ * @fps: the number of times per second to call the function
  * @func: function to call
  * @data: data to pass to the function
  * @notify: function to call when the timeout source is removed
@@ -74,7 +74,7 @@ static GSourceFuncs clutter_frame_source_funcs =
  * the interval time to execute then the function will be called again
  * half the interval time after it finished. In contrast
  * g_timeout_add_full() would not fire until a full interval after the
- * function completes so the delay between calls would be @interval *
+ * function completes so the delay between calls would be 1.0 / @fps *
  * 1.5. This function does not however try to invoke the function
  * multiple times to catch up missing frames if @func takes more than
  * @interval ms to execute.
@@ -85,7 +85,7 @@ static GSourceFuncs clutter_frame_source_funcs =
  */
 guint
 clutter_frame_source_add_full (gint           priority,
-			       guint          interval,
+			       guint          fps,
 			       GSourceFunc    func,
 			       gpointer       data,
 			       GDestroyNotify notify)
@@ -95,9 +95,7 @@ clutter_frame_source_add_full (gint           priority,
 				  sizeof (ClutterFrameSource));
   ClutterFrameSource *frame_source = (ClutterFrameSource *) source;
 
-  frame_source->last_time = 0;
+  _clutter_timeout_interval_init (&frame_source->timeout, fps);
-  frame_source->frame_time = interval;
-  g_get_current_time (&frame_source->start_time);
 
   if (priority != G_PRIORITY_DEFAULT)
     g_source_set_priority (source, priority);
@@ -113,7 +111,7 @@ clutter_frame_source_add_full (gint           priority,
 
 /**
  * clutter_frame_source_add:
- * @interval: the time between calls to the function, in milliseconds
+ * @fps: the number of times per second to call the function
  * @func: function to call
  * @data: data to pass to the function
  *
@@ -124,55 +122,25 @@ clutter_frame_source_add_full (gint           priority,
  * Since: 0.8
  */
 guint
-clutter_frame_source_add (guint          interval,
+clutter_frame_source_add (guint          fps,
 			  GSourceFunc    func,
 			  gpointer       data)
 {
   return clutter_frame_source_add_full (G_PRIORITY_DEFAULT,
-					interval, func, data, NULL);
+					fps, func, data, NULL);
-}
-
-static guint
-clutter_frame_source_get_ticks (ClutterFrameSource *frame_source)
-{
-  GTimeVal time_now;
-
-  g_source_get_current_time ((GSource *) frame_source, &time_now);
-  
-  return (time_now.tv_sec - frame_source->start_time.tv_sec) * 1000
-         + (time_now.tv_usec - frame_source->start_time.tv_usec) / 1000;
 }
 
 static gboolean
-clutter_frame_source_prepare (GSource *source, gint *timeout)
+clutter_frame_source_prepare (GSource *source, gint *delay)
 {
   ClutterFrameSource *frame_source = (ClutterFrameSource *) source;
+  GTimeVal current_time;
 
-  guint now = clutter_frame_source_get_ticks (frame_source);
+  g_source_get_current_time (source, &current_time);
 
-  /* If time has gone backwards or the time since the last frame is
+  return _clutter_timeout_interval_prepare (&current_time,
-     greater than the two frames worth then reset the time and do a
+                                            &frame_source->timeout,
-     frame now */
+                                            delay);
-  if (frame_source->last_time > now ||
-      (now - frame_source->last_time) > frame_source->frame_time * 2)
-    {
-      frame_source->last_time = now - frame_source->frame_time;
-      if (timeout)
-	*timeout = 0;
-      return TRUE;
-    }
-  else if (now - frame_source->last_time >= frame_source->frame_time)
-    {
-      if (timeout)
-	*timeout = 0;
-      return TRUE;
-    }
-  else
-    {
-      if (timeout)
-	*timeout = frame_source->frame_time + frame_source->last_time - now;
-      return FALSE;
-    }
 }
 
 static gboolean
@@ -183,16 +151,11 @@ clutter_frame_source_check (GSource *source)
 
 static gboolean
 clutter_frame_source_dispatch (GSource     *source,
-			       GSourceFunc callback,
+			       GSourceFunc  callback,
-			       gpointer    user_data)
+			       gpointer     user_data)
 {
   ClutterFrameSource *frame_source = (ClutterFrameSource *) source;
 
-  if ((* callback) (user_data))
+  return _clutter_timeout_interval_dispatch (&frame_source->timeout,
-    {
+                                             callback, user_data);
-      frame_source->last_time += frame_source->frame_time;
-      return TRUE;
-    }
-  else
-    return FALSE;
 }

clutter/clutter-frame-source.h

@@ -32,12 +32,12 @@
 
 G_BEGIN_DECLS
 
-guint clutter_frame_source_add (guint          interval,
+guint clutter_frame_source_add (guint          fps,
 				GSourceFunc    func,
 				gpointer       data);
 
 guint clutter_frame_source_add_full (gint           priority,
-				     guint          interval,
+				     guint          fps,
 				     GSourceFunc    func,
 				     gpointer       data,
 				     GDestroyNotify notify);

clutter/clutter-main.c

@@ -919,7 +919,7 @@ clutter_threads_add_timeout (guint       interval,
  * clutter_threads_add_frame_source_full:
  * @priority: the priority of the frame source. Typically this will be in the
  *            range between #G_PRIORITY_DEFAULT and #G_PRIORITY_HIGH.
- * @interval: the time between calls to the function, in milliseconds
+ * @fps: the number of times per second to call the function
  * @func: function to call
  * @data: data to pass to the function
  * @notify: function to call when the timeout source is removed
@@ -948,7 +948,7 @@ clutter_threads_add_timeout (guint       interval,
  */
 guint
 clutter_threads_add_frame_source_full (gint           priority,
-				       guint          interval,
+				       guint          fps,
 				       GSourceFunc    func,
 				       gpointer       data,
 				       GDestroyNotify notify)
@@ -963,14 +963,14 @@ clutter_threads_add_frame_source_full (gint           priority,
   dispatch->notify = notify;
 
   return clutter_frame_source_add_full (priority,
-					interval,
+					fps,
 					clutter_threads_dispatch, dispatch,
 					clutter_threads_dispatch_free);
 }
 
 /**
  * clutter_threads_add_frame_source:
- * @interval: the time between calls to the function, in milliseconds
+ * @fps: the number of times per second to call the function
  * @func: function to call
  * @data: data to pass to the function
  *
@@ -981,14 +981,14 @@ clutter_threads_add_frame_source_full (gint           priority,
  * Since: 0.8
  */
 guint
-clutter_threads_add_frame_source (guint       interval,
+clutter_threads_add_frame_source (guint       fps,
 				  GSourceFunc func,
 				  gpointer    data)
 {
   g_return_val_if_fail (func != NULL, 0);
 
   return clutter_threads_add_frame_source_full (G_PRIORITY_DEFAULT,
-						interval,
+						fps,
 						func, data,
 						NULL);
 }

clutter/clutter-main.h

@@ -128,12 +128,12 @@ guint            clutter_threads_add_timeout_full   (gint           priority,
                                                      GSourceFunc    func,
                                                      gpointer       data,
                                                      GDestroyNotify notify);
-guint            clutter_threads_add_frame_source   (guint          interval,
+guint            clutter_threads_add_frame_source   (guint          fps,
 						     GSourceFunc    func,
 						     gpointer       data);
 guint            clutter_threads_add_frame_source_full
                                                     (gint           priority,
-						     guint          interval,
+						     guint          fps,
 						     GSourceFunc    func,
 						     gpointer       data,
 						     GDestroyNotify notify);

clutter/clutter-timeline.c

@@ -93,8 +93,6 @@
 
 G_DEFINE_TYPE (ClutterTimeline, clutter_timeline, G_TYPE_OBJECT);
 
-#define FPS_TO_INTERVAL(f) (1000 / (f))
-
 struct _ClutterTimelinePrivate
 {
   ClutterTimelineDirection direction;
@@ -174,7 +172,7 @@ timeline_pool_init (void)
 }
 
 static guint
-timeout_add (guint          interval,
+timeout_add (guint          fps,
              GSourceFunc    func,
              gpointer       data,
              GDestroyNotify notify)
@@ -185,13 +183,13 @@ timeout_add (guint          interval,
     {
       g_assert (timeline_pool != NULL);
       res = clutter_timeout_pool_add (timeline_pool,
-                                      interval,
+                                      fps,
                                       func, data, notify);
     }
   else
     {
       res = clutter_threads_add_frame_source_full (CLUTTER_PRIORITY_TIMELINE,
-						   interval,
+						   fps,
 						   func, data, notify);
     }
 
@@ -814,7 +812,7 @@ timeline_timeout_func (gpointer data)
 
 static guint
 timeline_timeout_add (ClutterTimeline *timeline,
-                      guint            interval,
+                      guint            fps,
                       GSourceFunc      func,
                       gpointer         data,
                       GDestroyNotify   notify)
@@ -833,7 +831,7 @@ timeline_timeout_add (ClutterTimeline *timeline,
   priv->skipped_frames   = 0;
   priv->msecs_delta      = 0;
 
-  return timeout_add (interval, func, data, notify);
+  return timeout_add (fps, func, data, notify);
 }
 
 static gboolean
@@ -845,7 +843,7 @@ delay_timeout_func (gpointer data)
   priv->delay_id = 0;
 
   priv->timeout_id = timeline_timeout_add (timeline,
-                                           FPS_TO_INTERVAL (priv->fps),
+                                           priv->fps,
                                            timeline_timeout_func,
                                            timeline, NULL);
 
@@ -884,7 +882,7 @@ clutter_timeline_start (ClutterTimeline *timeline)
   else
     {
       priv->timeout_id = timeline_timeout_add (timeline,
-                                               FPS_TO_INTERVAL (priv->fps),
+                                               priv->fps,
                                                timeline_timeout_func,
                                                timeline, NULL);
 
@@ -1150,7 +1148,7 @@ clutter_timeline_set_speed (ClutterTimeline *timeline,
           timeout_remove (priv->timeout_id);
 
           priv->timeout_id = timeline_timeout_add (timeline,
-                                                   FPS_TO_INTERVAL (priv->fps),
+                                                   priv->fps,
                                                    timeline_timeout_func,
                                                    timeline, NULL);
         }

clutter/clutter-timeout-interval.c

@@ -0,0 +1,126 @@
+/*
+ * Clutter.
+ *
+ * An OpenGL based 'interactive canvas' library.
+ *
+ * Authored By Neil Roberts  <neil@linux.intel.com>
+ *
+ * Copyright (C) 2009  Intel Corporation.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+/* This file contains the common code to check whether an interval has
+   expired used in clutter-frame-source and clutter-timeout-pool. */
+
+#include "clutter-timeout-interval.h"
+
+void
+_clutter_timeout_interval_init (ClutterTimeoutInterval *interval,
+                                guint fps)
+{
+  g_get_current_time (&interval->start_time);
+  interval->fps = fps;
+  interval->frame_count = 0;
+}
+
+static guint
+_clutter_timeout_interval_get_ticks (const GTimeVal *current_time,
+                                     ClutterTimeoutInterval *interval)
+{
+  return ((current_time->tv_sec - interval->start_time.tv_sec) * 1000
+          + (current_time->tv_usec - interval->start_time.tv_usec) / 1000);
+}
+
+gboolean
+_clutter_timeout_interval_prepare (const GTimeVal *current_time,
+                                   ClutterTimeoutInterval *interval,
+                                   gint *delay)
+{
+  guint elapsed_time
+    = _clutter_timeout_interval_get_ticks (current_time, interval);
+  guint new_frame_num = elapsed_time * interval->fps / 1000;
+
+  /* If time has gone backwards or the time since the last frame is
+     greater than the two frames worth then reset the time and do a
+     frame now */
+  if (new_frame_num < interval->frame_count ||
+      new_frame_num - interval->frame_count > 2)
+    {
+      /* Get the frame time rounded up to the nearest ms */
+      guint frame_time = (1000 + interval->fps - 1) / interval->fps;
+
+      /* Reset the start time */
+      interval->start_time = *current_time;
+      /* Move the start time as if one whole frame has elapsed */
+      g_time_val_add (&interval->start_time, -(gint) frame_time * 1000);
+
+      interval->frame_count = 0;
+
+      if (delay)
+	*delay = 0;
+      return TRUE;
+    }
+  else if (new_frame_num > interval->frame_count)
+    {
+      if (delay)
+	*delay = 0;
+      return TRUE;
+    }
+  else
+    {
+      if (delay)
+	*delay = ((interval->frame_count + 1) * 1000 / interval->fps
+                  - elapsed_time);
+      return FALSE;
+    }
+}
+
+gboolean
+_clutter_timeout_interval_dispatch (ClutterTimeoutInterval *interval,
+                                    GSourceFunc             callback,
+                                    gpointer                user_data)
+{
+  if ((* callback) (user_data))
+    {
+      interval->frame_count++;
+      return TRUE;
+    }
+  else
+    return FALSE;
+}
+
+gint
+_clutter_timeout_interval_compare_expiration (const ClutterTimeoutInterval *a,
+                                              const ClutterTimeoutInterval *b)
+{
+  guint a_delay = 1000 / a->fps;
+  guint b_delay = 1000 / b->fps;
+  glong b_difference;
+  gint comparison;
+
+  b_difference = ((a->start_time.tv_sec - b->start_time.tv_sec) * 1000
+                  + (a->start_time.tv_usec - b->start_time.tv_usec) / 1000);
+
+  comparison = ((gint) ((a->frame_count + 1) * a_delay)
+                - (gint) ((b->frame_count + 1) * b_delay + b_difference));
+
+  return (comparison < 0 ? -1
+          : comparison > 0 ? 1
+          : 0);
+}

clutter/clutter-timeout-interval.h

@@ -0,0 +1,56 @@
+/*
+ * Clutter.
+ *
+ * An OpenGL based 'interactive canvas' library.
+ *
+ * Authored By Neil Roberts  <neil@linux.intel.com>
+ *
+ * Copyright (C) 2009  Intel Corporation.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __CLUTTER_TIMEOUT_INTERVAL_H__
+#define __CLUTTER_TIMEOUT_INTERVAL_H__
+
+#include <glib.h>
+
+G_BEGIN_DECLS
+
+typedef struct _ClutterTimeoutInterval ClutterTimeoutInterval;
+
+struct _ClutterTimeoutInterval
+{
+  GTimeVal start_time;
+  guint frame_count, fps;
+};
+
+void _clutter_timeout_interval_init (ClutterTimeoutInterval *interval,
+                                     guint fps);
+
+gboolean _clutter_timeout_interval_prepare (const GTimeVal *current_time,
+                                            ClutterTimeoutInterval *interval,
+                                            gint *delay);
+
+gboolean _clutter_timeout_interval_dispatch (ClutterTimeoutInterval *interval,
+                                             GSourceFunc     callback,
+                                             gpointer        user_data);
+
+gint _clutter_timeout_interval_compare_expiration
+                                              (const ClutterTimeoutInterval *a,
+                                               const ClutterTimeoutInterval *b);
+
+G_END_DECLS
+
+#endif /* __CLUTTER_TIMEOUT_INTERVAL_H__ */

clutter/clutter-timeout-pool.c

@@ -36,6 +36,7 @@
 
 #include "clutter-debug.h"
 #include "clutter-timeout-pool.h"
+#include "clutter-timeout-interval.h"
 
 typedef struct _ClutterTimeout  ClutterTimeout;
 typedef enum {
@@ -49,8 +50,7 @@ struct _ClutterTimeout
   ClutterTimeoutFlags flags;
   gint refcount;
 
-  guint interval;
+  ClutterTimeoutInterval interval;
-  guint last_time;
 
   GSourceFunc func;
   gpointer data;
@@ -94,7 +94,6 @@ clutter_timeout_sort (gconstpointer a,
 {
   const ClutterTimeout *t_a = a;
   const ClutterTimeout *t_b = b;
-  gint comparison;
 
   /* Keep 'ready' timeouts at the front */
   if (TIMEOUT_READY (t_a))
@@ -103,16 +102,8 @@ clutter_timeout_sort (gconstpointer a,
   if (TIMEOUT_READY (t_b))
     return 1;
 
-  /* Otherwise sort by expiration time */
+  return _clutter_timeout_interval_compare_expiration (&t_a->interval,
-  comparison = (t_a->last_time + t_a->interval)
+                                                       &t_b->interval);
-    - (t_b->last_time + t_b->interval);
-  if (comparison < 0)
-    return -1;
-
-  if (comparison > 0)
-    return 1;
-
-  return 0;
 }
 
 static gint
@@ -124,15 +115,17 @@ clutter_timeout_find_by_id (gconstpointer a,
   return t_a->id == GPOINTER_TO_UINT (b) ? 0 : 1;
 }
 
-static guint
+static ClutterTimeout *
-clutter_timeout_pool_get_ticks (ClutterTimeoutPool *pool)
+clutter_timeout_new (guint fps)
 {
-  GTimeVal time_now;
+  ClutterTimeout *timeout;
 
-  g_source_get_current_time ((GSource *) pool, &time_now);
+  timeout = g_slice_new0 (ClutterTimeout);
+  _clutter_timeout_interval_init (&timeout->interval, fps);
+  timeout->flags = CLUTTER_TIMEOUT_NONE;
+  timeout->refcount = 1;
 
-  return (time_now.tv_sec - pool->start_time.tv_sec) * 1000
+  return timeout;
-    + (time_now.tv_usec - pool->start_time.tv_usec) / 1000;
 }
 
 static gboolean
@@ -140,66 +133,12 @@ clutter_timeout_prepare (ClutterTimeoutPool *pool,
                          ClutterTimeout     *timeout,
                          gint               *next_timeout)
 {
-  guint now = clutter_timeout_pool_get_ticks (pool);
+  GTimeVal now;
 
-  /* If time has gone backwards or the time since the last frame is
+  g_source_get_current_time (&pool->source, &now);
-     greater than the two frames worth then reset the time and do a
-     frame now */
-  if (timeout->last_time > now || now - timeout->last_time
-      > timeout->interval * 2)
-    {
-      timeout->last_time = now - timeout->interval;
-      if (next_timeout)
-	*next_timeout = 0;
-      return TRUE;
-    }
-  else if (now - timeout->last_time >= timeout->interval)
-    {
-      if (next_timeout)
-	*next_timeout = 0;
-      return TRUE;
-    }
-  else
-    {
-      if (next_timeout)
-	*next_timeout = timeout->interval + timeout->last_time - now;
-      return FALSE;
-    }
-}
 
-static gboolean
+  return _clutter_timeout_interval_prepare (&now, &timeout->interval,
-clutter_timeout_dispatch (GSource        *source,
+                                            next_timeout);
-                          ClutterTimeout *timeout)
-{
-  gboolean retval = FALSE;
-
-  if (G_UNLIKELY (!timeout->func))
-    {
-      g_warning ("Timeout dispatched without a callback.");
-      return FALSE;
-    }
-
-  if (timeout->func (timeout->data))
-    {
-      timeout->last_time += timeout->interval;
-
-      retval = TRUE;
-    }
-
-  return retval;
-}
-
-static ClutterTimeout *
-clutter_timeout_new (guint interval)
-{
-  ClutterTimeout *timeout;
-
-  timeout = g_slice_new0 (ClutterTimeout);
-  timeout->interval = interval;
-  timeout->flags = CLUTTER_TIMEOUT_NONE;
-  timeout->refcount = 1;
-
-  return timeout;
 }
 
 /* ref and unref are always called under the main Clutter lock, so there
@@ -350,7 +289,8 @@ clutter_timeout_pool_dispatch (GSource     *source,
       l->next = pool->dispatched_timeouts;
       pool->dispatched_timeouts = l;
 
-      if (!clutter_timeout_dispatch (source, timeout))
+      if (!_clutter_timeout_interval_dispatch (&timeout->interval,
+                                               timeout->func, timeout->data))
 	{
 	  /* The timeout may have already been removed, but nothing
            * can be added to the dispatched_timeout list except in this
@@ -500,7 +440,6 @@ clutter_timeout_pool_add (ClutterTimeoutPool *pool,
 
   retval = timeout->id = pool->next_id++;
 
-  timeout->last_time = clutter_timeout_pool_get_ticks (pool);
   timeout->func = func;
   timeout->data = data;
   timeout->notify = notify;

clutter/clutter-timeout-pool.h

@@ -43,7 +43,7 @@ typedef struct _ClutterTimeoutPool    ClutterTimeoutPool;
 
 ClutterTimeoutPool *clutter_timeout_pool_new    (gint                priority);
 guint               clutter_timeout_pool_add    (ClutterTimeoutPool *pool,
-                                                 guint               interval,
+                                                 guint               fps,
                                                  GSourceFunc         func,
                                                  gpointer            data,
                                                  GDestroyNotify      notify);