From 1a6deea7a7eb9e954d692cb948f81d76dd2e8483 Mon Sep 17 00:00:00 2001 From: Emmanuele Bassi Date: Sun, 19 Aug 2007 17:06:22 +0000 Subject: [PATCH] Use g_list_insert_sorted() instead of our custom function The clutter_timeout_pool_insert_sorted() function caused an inversion of the timeout sources in the pool; this led to a wrong behaviour in the execution of the timeout functions. See bug 471. This patch drops clutter_timeout_pool_insert_sorted() in favour of the standard g_list_insert_sorted(), which produces identical behaviours with and without the pool. A new test, written by Rob Bradford, has been added to the regression test suite in order to identify sorting issues with the timeout pools. --- clutter/clutter-timeout-pool.c | 85 ++++++++++++---------------------- tests/Makefile.am | 3 +- tests/test-timeline.c | 55 ++++++++++++++++++++++ 3 files changed, 87 insertions(+), 56 deletions(-) create mode 100644 tests/test-timeline.c diff --git a/clutter/clutter-timeout-pool.c b/clutter/clutter-timeout-pool.c index 1ef1853e1..8e2b5902f 100644 --- a/clutter/clutter-timeout-pool.c +++ b/clutter/clutter-timeout-pool.c @@ -93,14 +93,23 @@ clutter_timeout_sort (gconstpointer a, const ClutterTimeout *t_a = a; const ClutterTimeout *t_b = b; gint comparison; - + /* Keep 'ready' timeouts at the front */ - return (comparison = TIMEOUT_READY (t_a) - TIMEOUT_READY (t_b)) - ? -comparison - /* Otherwise sort by expiration time */ - : ((comparison = t_a->expiration.tv_sec - t_b->expiration.tv_sec) - ? comparison - : t_a->expiration.tv_usec - t_b->expiration.tv_usec); + if (TIMEOUT_READY (t_a)) + return -1; + + if (TIMEOUT_READY (t_b)) + return 1; + + /* Otherwise sort by expiration time */ + comparison = t_a->expiration.tv_sec - t_b->expiration.tv_sec; + if (comparison < 0) + return -1; + + if (comparison > 0) + return 1; + + return (t_a->expiration.tv_usec - t_b->expiration.tv_usec); } static gint @@ -320,54 +329,13 @@ clutter_timeout_pool_check (GSource *source) return (pool->ready > 0); } -static GList * -clutter_timeout_pool_insert_sorted (GList *node, GList *list) -{ - GList *l, *last = NULL, *next; - - /* Find the first timeout which expires after this one */ - for (l = list; l; l = l->next) - { - ClutterTimeout *t_a = node->data; - ClutterTimeout *t_b = l->data; - - last = l; - next = l->next; - - if (clutter_timeout_sort (t_a, t_b) < 0) - break; - } - - /* If we didn't find an entry then add it at the end */ - if (!l) - { - node->prev = last; - node->next = NULL; - - if (last) - last->next = node; - } - else - { - /* Otherwise insert it before the found entry */ - node->next = l; - node->prev = l->prev; - l->prev = node; - - if (node->prev) - node->prev->next = node; - } - - return node->prev ? list : node; -} - static gboolean clutter_timeout_pool_dispatch (GSource *source, GSourceFunc func, gpointer data) { ClutterTimeoutPool *pool = (ClutterTimeoutPool *) source; - GList *l; + GList *dispatched_timeouts; /* the main loop might have predicted this, so we repeat the * check for ready timeouts. @@ -383,6 +351,7 @@ clutter_timeout_pool_dispatch (GSource *source, while (pool->timeouts && pool->timeouts->data && pool->ready-- > 0) { ClutterTimeout *timeout = pool->timeouts->data; + GList *l; /* One of the ready timeouts may have been removed during dispatch, * in which case pool->ready will be wrong, but the ready timeouts @@ -436,16 +405,22 @@ clutter_timeout_pool_dispatch (GSource *source, } /* Re-insert the dispatched timeouts in sorted order */ - while (pool->dispatched_timeouts) + dispatched_timeouts = pool->dispatched_timeouts; + while (dispatched_timeouts) { - GList *next = pool->dispatched_timeouts->next; - - pool->timeouts = clutter_timeout_pool_insert_sorted (pool->dispatched_timeouts, - pool->timeouts); + ClutterTimeout *timeout = dispatched_timeouts->data; + GList *next = dispatched_timeouts->next; - pool->dispatched_timeouts = next; + if (timeout) + pool->timeouts = g_list_insert_sorted (pool->timeouts, timeout, + clutter_timeout_sort); + + dispatched_timeouts = next; } + g_list_free (pool->dispatched_timeouts); + pool->dispatched_timeouts = NULL; + pool->ready = 0; clutter_threads_leave (); diff --git a/tests/Makefile.am b/tests/Makefile.am index 3855f4dc8..8f82da967 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1,7 +1,7 @@ noinst_PROGRAMS = test-textures test-events test-offscreen test-scale \ test-actors test-behave test-text test-entry test-project \ test-boxes test-perspective test-rotate test-depth \ - test-threads + test-threads test-timeline INCLUDES = -I$(top_srcdir)/ LDADD = $(top_builddir)/clutter/libclutter-@CLUTTER_FLAVOUR@-@CLUTTER_MAJORMINOR@.la @@ -22,5 +22,6 @@ test_perspective_SOURCES = test-perspective.c test_rotate_SOURCES = test-rotate.c test_depth_SOURCES = test-depth.c test_threads_SOURCES = test-threads.c +test_timeline_SOURCES = test-timeline.c EXTRA_DIST = redhand.png diff --git a/tests/test-timeline.c b/tests/test-timeline.c new file mode 100644 index 000000000..9831b0983 --- /dev/null +++ b/tests/test-timeline.c @@ -0,0 +1,55 @@ +#include + +static void +timeline_1_new_frame_cb (ClutterTimeline *timeline, gint frame_no) +{ + g_debug ("1: Doing frame %d.", frame_no); +} + +static void +timeline_2_new_frame_cb (ClutterTimeline *timeline, gint frame_no) +{ + g_debug ("2: Doing frame %d.", frame_no); +} + +static void +timeline_3_new_frame_cb (ClutterTimeline *timeline, gint frame_no) +{ + g_debug ("3: Doing frame %d.", frame_no); +} + +int +main (int argc, char **argv) +{ + ClutterTimeline *timeline_1; + ClutterTimeline *timeline_2; + ClutterTimeline *timeline_3; + + clutter_init (&argc, &argv); + + timeline_1 = clutter_timeline_new (100, 50); + timeline_2 = clutter_timeline_clone (timeline_1); + timeline_3 = clutter_timeline_clone (timeline_1); + + g_signal_connect (timeline_1, + "new-frame", G_CALLBACK (timeline_1_new_frame_cb), + NULL); + g_signal_connect (timeline_2, + "new-frame", G_CALLBACK (timeline_2_new_frame_cb), + NULL); + g_signal_connect (timeline_3, + "new-frame", G_CALLBACK (timeline_3_new_frame_cb), + NULL); + + clutter_timeline_start (timeline_1); + clutter_timeline_start (timeline_2); + clutter_timeline_start (timeline_3); + + clutter_main (); + + g_object_unref (timeline_1); + g_object_unref (timeline_2); + g_object_unref (timeline_3); + + return; +}