gst/gstsystemclock.c: Fix anoying regression that survived a few releases. When adding an async entry while blocking ...

Original commit message from CVS:
* gst/gstsystemclock.c: (gst_system_clock_id_wait_jitter_unlocked),
(gst_system_clock_id_wait_jitter),
(gst_system_clock_id_wait_async), (gst_system_clock_id_unschedule):
Fix anoying regression that survived a few releases. When adding an
async entry while blocking on a sync entry, the sync entry will unblock
but still be busy, so it should continue to wait instead of returning
_BUSY to the app.
Add some comments here and there.
* tests/check/gst/gstsystemclock.c: (mixed_thread),
(mixed_async_cb), (GST_START_TEST), (gst_systemclock_suite):
Add testcase for this.

ChangeLog

@@ -1,3 +1,18 @@
+2007-03-22  Wim Taymans  <wim@fluendo.com>
+
+	* gst/gstsystemclock.c: (gst_system_clock_id_wait_jitter_unlocked),
+	(gst_system_clock_id_wait_jitter),
+	(gst_system_clock_id_wait_async), (gst_system_clock_id_unschedule):
+	Fix anoying regression that survived a few releases. When adding an
+	async entry while blocking on a sync entry, the sync entry will unblock
+	but still be busy, so it should continue to wait instead of returning
+	_BUSY to the app.
+	Add some comments here and there.
+
+	* tests/check/gst/gstsystemclock.c: (mixed_thread),
+	(mixed_async_cb), (GST_START_TEST), (gst_systemclock_suite):
+	Add testcase for this.
+
 2007-03-22  Wim Taymans  <wim@fluendo.com>
 
 	* libs/gst/base/gstbasesrc.c: (gst_base_src_get_range):

gst/gstsystemclock.c

@@ -424,6 +424,10 @@ gst_system_clock_id_wait_jitter_unlocked (GstClock * clock,
         /* else restart if we must */
         if (!restart)
           break;
+
+        /* this can happen if the entry got unlocked because of an async entry
+         * was added to the head of the async queue. */
+        GST_CAT_DEBUG (GST_CAT_CLOCK, "continue waiting for entry %p", entry);
       }
     }
   } else if (diff == 0) {
@@ -441,7 +445,7 @@ gst_system_clock_id_wait_jitter (GstClock * clock, GstClockEntry * entry,
   GstClockReturn ret;
 
   GST_OBJECT_LOCK (clock);
-  ret = gst_system_clock_id_wait_jitter_unlocked (clock, entry, jitter, FALSE);
+  ret = gst_system_clock_id_wait_jitter_unlocked (clock, entry, jitter, TRUE);
   GST_OBJECT_UNLOCK (clock);
 
   return ret;
@@ -485,7 +489,7 @@ no_thread:
 static GstClockReturn
 gst_system_clock_id_wait_async (GstClock * clock, GstClockEntry * entry)
 {
-  GST_CAT_DEBUG (GST_CAT_CLOCK, "adding entry %p", entry);
+  GST_CAT_DEBUG (GST_CAT_CLOCK, "adding async entry %p", entry);
 
   GST_OBJECT_LOCK (clock);
 
@@ -503,7 +507,12 @@ gst_system_clock_id_wait_async (GstClock * clock, GstClockEntry * entry)
    * front, else the thread is just waiting for another entry and
    * will get to this entry automatically. */
   if (clock->entries->data == entry) {
-    GST_CAT_DEBUG (GST_CAT_CLOCK, "send signal");
+    GST_CAT_DEBUG (GST_CAT_CLOCK, "async entry added to head, sending signal");
+    /* this will wake up _all_ entries waiting for the clock because we have
+     * only one cond for all entries (makes allocation faster). Entries that
+     * have not timed out will have their status set to BUSY and should continue
+     * to wait. In the case of the async ones, the new head entry should be
+     * taken and waited for. */
     GST_CLOCK_BROADCAST (clock);
   }
   GST_OBJECT_UNLOCK (clock);
@@ -528,8 +537,11 @@ gst_system_clock_id_unschedule (GstClock * clock, GstClockEntry * entry)
   GST_CAT_DEBUG (GST_CAT_CLOCK, "unscheduling entry %p", entry);
 
   GST_OBJECT_LOCK (clock);
+  /* mark entry as unscheduled, then wake up all entries. The entries that did
+   * not timeout will be woken up but immediatly go to sleep again because their
+   * status would still be busy. */
   entry->status = GST_CLOCK_UNSCHEDULED;
-  GST_CAT_DEBUG (GST_CAT_CLOCK, "send signal");
+  GST_CAT_DEBUG (GST_CAT_CLOCK, "sending signal");
   GST_CLOCK_BROADCAST (clock);
   GST_OBJECT_UNLOCK (clock);
 }

tests/check/gst/gstsystemclock.c

@@ -298,8 +298,76 @@ GST_START_TEST (test_diff)
     fail_if (d[i] != GST_CLOCK_DIFF (time1[i], time2[i]));
   }
 }
-GST_END_TEST Suite *
-gst_systemclock_suite (void)
+
+GST_END_TEST
+/* test if a blocking wait, unblocked by an async entry continues to be
+ * scheduled */
+    typedef struct
+{
+  GstClock *clock;
+  GstClockID id;
+  GstClockTimeDiff jitter;
+  GstClockReturn ret;
+} MixedInfo;
+
+static gpointer
+mixed_thread (MixedInfo * info)
+{
+  info->ret = gst_clock_id_wait (info->id, &info->jitter);
+  return NULL;
+}
+
+static gboolean
+mixed_async_cb (GstClock * clock, GstClockTime time,
+    GstClockID id, gpointer user_data)
+{
+  return TRUE;
+}
+
+GST_START_TEST (test_mixed)
+{
+  GThread *thread;
+  GError *error = NULL;
+  MixedInfo info;
+  GstClockTime base;
+  GstClockID id;
+
+  info.clock = gst_system_clock_obtain ();
+  fail_unless (info.clock != NULL,
+      "Could not create instance of GstSystemClock");
+
+  /* get current time of the clock as base time */
+  base = gst_clock_get_time (info.clock);
+
+  /* create entry to wait for 1 second */
+  info.id = gst_clock_new_single_shot_id (info.clock, base + GST_SECOND);
+
+  /* make and start an entry that is scheduled every 10ms */
+  id = gst_clock_new_periodic_id (info.clock, base, 10 * GST_MSECOND);
+
+  /* start waiting for the entry */
+  thread = g_thread_create ((GThreadFunc) mixed_thread, &info, TRUE, &error);
+  fail_unless (error == NULL, "error creating thread");
+  fail_unless (thread != NULL, "Could not create thread");
+
+  /* wait half a second so we are sure to be in the thread */
+  g_usleep (G_USEC_PER_SEC / 2);
+
+  /* start scheduling the entry */
+  gst_clock_id_wait_async (id, mixed_async_cb, NULL);
+
+  /* wait for thread to finish */
+  g_thread_join (thread);
+  /* entry must have timed out correctly */
+  fail_unless (info.ret == GST_CLOCK_OK, "clock return was %d", info.ret);
+
+  gst_clock_id_unschedule (id);
+  gst_clock_id_unref (info.id);
+  gst_object_unref (info.clock);
+
+}
+
+GST_END_TEST Suite * gst_systemclock_suite (void)
 {
   Suite *s = suite_create ("GstSystemClock");
   TCase *tc_chain = tcase_create ("waiting");
@@ -311,6 +379,7 @@ gst_systemclock_suite (void)
   tcase_add_test (tc_chain, test_periodic_multi);
   tcase_add_test (tc_chain, test_async_order);
   tcase_add_test (tc_chain, test_diff);
+  tcase_add_test (tc_chain, test_mixed);
 
   return s;
 }