identity: refactor and add tests using GstHarness

Writing a test for unscheduling the gst_clock_id_wait inside the
identity element, found an invalid read, caused by removing the clock-id
when calling _unschedule instead of letting the code calling _wait remove
the clock-id after being unscheduled.

https://bugzilla.gnome.org/show_bug.cgi?id=752055

plugins/elements/gstidentity.c

@@ -401,8 +401,6 @@ gst_identity_sink_event (GstBaseTransform * trans, GstEvent * event)
       if (identity->clock_id) {
         GST_DEBUG_OBJECT (identity, "unlock clock wait");
         gst_clock_id_unschedule (identity->clock_id);
-        gst_clock_id_unref (identity->clock_id);
-        identity->clock_id = NULL;
       }
       GST_OBJECT_UNLOCK (identity);
     }
@@ -886,8 +884,6 @@ gst_identity_change_state (GstElement * element, GstStateChange transition)
       if (identity->clock_id) {
         GST_DEBUG_OBJECT (identity, "unlock clock wait");
         gst_clock_id_unschedule (identity->clock_id);
-        gst_clock_id_unref (identity->clock_id);
-        identity->clock_id = NULL;
       }
       identity->blocked = FALSE;
       g_cond_broadcast (&identity->blocked_cond);

tests/check/elements/identity.c

@@ -3,6 +3,7 @@
  * unit test for identity
  *
  * Copyright (C) <2005> Thomas Vander Stichele <thomas at apestaart dot org>
+ * Copyright (C) <2015> Havard Graff           <havard@pexip.com>
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -20,100 +21,159 @@
  * Boston, MA 02110-1301, USA.
  */
 
-#include <unistd.h>
-
 #include <gst/check/gstcheck.h>
-
+#include <gst/check/gstharness.h>
-static gboolean have_eos = FALSE;
-
-/* For ease of programming we use globals to keep refs for our floating
- * src and sink pads we create; otherwise we always have to do get_pad,
- * get_peer, and then remove references in every test function */
-static GstPad *mysrcpad, *mysinkpad;
-
-
-static GstStaticPadTemplate sinktemplate = GST_STATIC_PAD_TEMPLATE ("sink",
-    GST_PAD_SINK,
-    GST_PAD_ALWAYS,
-    GST_STATIC_CAPS_ANY);
-static GstStaticPadTemplate srctemplate = GST_STATIC_PAD_TEMPLATE ("src",
-    GST_PAD_SRC,
-    GST_PAD_ALWAYS,
-    GST_STATIC_CAPS_ANY);
-
-static gboolean
-event_func (GstPad * pad, GstObject * parent, GstEvent * event)
-{
-  if (GST_EVENT_TYPE (event) == GST_EVENT_EOS) {
-    have_eos = TRUE;
-  }
-
-  gst_event_unref (event);
-  return TRUE;
-}
-
-static GstElement *
-setup_identity (void)
-{
-  GstElement *identity;
-
-  GST_DEBUG ("setup_identity");
-
-  identity = gst_check_setup_element ("identity");
-  mysrcpad = gst_check_setup_src_pad (identity, &srctemplate);
-  mysinkpad = gst_check_setup_sink_pad (identity, &sinktemplate);
-  gst_pad_set_event_function (mysinkpad, event_func);
-  gst_pad_set_active (mysrcpad, TRUE);
-  gst_pad_set_active (mysinkpad, TRUE);
-
-  return identity;
-}
-
-static void
-cleanup_identity (GstElement * identity)
-{
-  GST_DEBUG ("cleanup_identity");
-
-  gst_pad_set_active (mysrcpad, FALSE);
-  gst_pad_set_active (mysinkpad, FALSE);
-  gst_check_teardown_src_pad (identity);
-  gst_check_teardown_sink_pad (identity);
-  gst_check_teardown_element (identity);
-}
 
 GST_START_TEST (test_one_buffer)
 {
-  GstElement *identity;
+  GstHarness *h = gst_harness_new ("identity");
-  GstBuffer *buffer;
+  GstBuffer *buffer_in;
-  GstSegment segment;
+  GstBuffer *buffer_out;
 
-  identity = setup_identity ();
+  gst_harness_set_src_caps_str (h, "mycaps");
-  fail_unless (gst_element_set_state (identity,
-          GST_STATE_PLAYING) == GST_STATE_CHANGE_SUCCESS,
-      "could not set to playing");
 
-  gst_segment_init (&segment, GST_FORMAT_BYTES);
+  buffer_in = gst_buffer_new_and_alloc (4);
-  gst_pad_push_event (mysrcpad, gst_event_new_stream_start ("test"));
+  ASSERT_BUFFER_REFCOUNT (buffer_in, "buffer", 1);
-  gst_pad_push_event (mysrcpad, gst_event_new_segment (&segment));
 
-  buffer = gst_buffer_new_and_alloc (4);
+  gst_buffer_fill (buffer_in, 0, "data", 4);
-  ASSERT_BUFFER_REFCOUNT (buffer, "buffer", 1);
-
-  gst_buffer_fill (buffer, 0, "data", 4);
 
   /* pushing gives away my reference ... */
-  fail_unless (gst_pad_push (mysrcpad, buffer) == GST_FLOW_OK,
+  fail_unless_equals_int (GST_FLOW_OK, gst_harness_push (h, buffer_in));
-      "Failed pushing buffer to identity");
 
-  /* ... but it should end up being collected on the global buffer list */
+  /* ... but it should end up being collected on GstHarness queue */
-  fail_unless (g_list_length (buffers) == 1);
+  fail_unless_equals_int (1, gst_harness_buffers_in_queue (h));
-  fail_unless ((GstBuffer *) (g_list_first (buffers)->data) == buffer);
+  buffer_out = gst_harness_pull (h);
-  ASSERT_BUFFER_REFCOUNT (buffer, "buffer", 1);
+
+  fail_unless (buffer_in == buffer_out);
+  ASSERT_BUFFER_REFCOUNT (buffer_out, "buffer", 1);
 
   /* cleanup */
-  cleanup_identity (identity);
+  gst_buffer_unref (buffer_out);
+  gst_harness_teardown (h);
+}
+GST_END_TEST;
+
+static void
+handoff_func (GstElement * identity, GstBuffer * buf,  GstBuffer ** ret)
+{
+  (void)identity;
+  *ret = buf;
 }
 
+GST_START_TEST (test_signal_handoffs)
+{
+  GstHarness *h = gst_harness_new ("identity");
+  GstBuffer *buffer_in;
+  GstBuffer *buffer_signaled = NULL;
+  gst_harness_set_src_caps_str (h, "mycaps");
+
+  /* connect to the handoff signal */
+  g_signal_connect (h->element, "handoff",
+      G_CALLBACK (handoff_func), &buffer_signaled);
+
+  /* first, turn off signal-handoffs */
+  g_object_set (h->element, "signal-handoffs", FALSE, NULL);
+
+  /* then push a buffer */
+  buffer_in = gst_buffer_new_and_alloc (4);
+  fail_unless_equals_int (GST_FLOW_OK, gst_harness_push (h, buffer_in));
+
+  /* verify that we got no buffer signaled */
+  fail_unless (buffer_signaled == NULL);
+
+  /* now turn on signal-handoffs */
+  g_object_set (h->element, "signal-handoffs", TRUE, NULL);
+
+  /* then push another buffer */
+  buffer_in = gst_buffer_new_and_alloc (4);
+  fail_unless_equals_int (GST_FLOW_OK, gst_harness_push (h, buffer_in));
+
+  /* verify the buffer signaled is equal to the one pushed in */
+  fail_unless (buffer_signaled == buffer_in);
+  ASSERT_BUFFER_REFCOUNT (buffer_signaled, "buffer", 1);
+
+  /* cleanup */
+  gst_harness_teardown (h);
+}
+GST_END_TEST;
+
+GST_START_TEST (test_sync_on_timestamp)
+{
+  /* the reason to use the queue in front of the identity element
+     is to effectively make gst_harness_push asynchronous, not locking
+     up the test, waiting for gst_clock_id_wait */
+  GstHarness * h = gst_harness_new_parse ("queue ! identity sync=1");
+  GstBuffer *buf;
+  GstClock *clock;
+  GstClockTime timestamp = 123456789;
+
+  /* use testclock */
+  gst_harness_use_testclock (h);
+  gst_harness_set_src_caps_str (h, "mycaps");
+
+  /* make a buffer and set the timestamp */
+  buf = gst_buffer_new ();
+  GST_BUFFER_PTS (buf) = timestamp;
+
+  /* push the buffer, and verify it does *not* make it through */
+  gst_harness_push (h, buf);
+  fail_unless_equals_int (0, gst_harness_buffers_in_queue (h));
+
+  /* verify the identity element has registered exactly one GstClockID */
+  fail_unless (gst_harness_wait_for_clock_id_waits (h, 1, 42));
+
+  /* crank the clock and pull the buffer */
+  gst_harness_crank_single_clock_wait (h);
+  buf = gst_harness_pull (h);
+
+  /* verify that the buffer has the right timestamp, and that the time on
+     the clock is equal to the timestamp */
+  fail_unless_equals_int64 (timestamp, GST_BUFFER_PTS (buf));
+  clock = gst_element_get_clock (h->element);
+  fail_unless_equals_int64 (timestamp, gst_clock_get_time (clock));
+
+  /* cleanup */
+  gst_object_unref (clock);
+  gst_buffer_unref (buf);
+  gst_harness_teardown (h);
+}
+GST_END_TEST;
+
+GST_START_TEST (test_stopping_element_unschedules_sync)
+{
+  /* the reason to use the queue in front of the identity element
+     is to effectively make gst_harness_push asynchronous, not locking
+     up the test, waiting for gst_clock_id_wait */
+  GstHarness * h = gst_harness_new_parse ("queue ! identity sync=1");
+  GstBuffer *buf;
+  GstClockTime timestamp = 123456789;
+
+  /* use testclock */
+  gst_harness_use_testclock (h);
+  gst_harness_set_src_caps_str (h, "mycaps");
+
+  /* make a buffer and set the timestamp */
+  buf = gst_buffer_new ();
+  GST_BUFFER_PTS (buf) = timestamp;
+
+  /* push the buffer, and verify it does *not* make it through */
+  gst_harness_push (h, buf);
+  fail_unless_equals_int (0, gst_harness_buffers_in_queue (h));
+
+  /* verify the identity element has registered exactly one GstClockID */
+  fail_unless (gst_harness_wait_for_clock_id_waits (h, 1, 42));
+
+  /* setting identity to READY should unschedule the sync */
+  gst_element_set_state (h->element, GST_STATE_READY);
+
+  /* verify the identity element no longer waits on the clock */
+  fail_unless (gst_harness_wait_for_clock_id_waits (h, 0, 42));
+
+  /* and that the waiting buffer was dropped */
+  fail_unless_equals_int (0, gst_harness_buffers_received (h));
+
+  gst_harness_teardown (h);
+}
 GST_END_TEST;
 
 static Suite *
@@ -124,6 +184,10 @@ identity_suite (void)
 
   suite_add_tcase (s, tc_chain);
   tcase_add_test (tc_chain, test_one_buffer);
+  tcase_add_test (tc_chain, test_signal_handoffs);
+  tcase_add_test (tc_chain, test_sync_on_timestamp);
+  tcase_add_test (tc_chain, test_stopping_element_unschedules_sync);
+
 
   return s;
 }