summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorTim-Philipp Müller <tim.muller@collabora.co.uk>2009-05-30 19:36:25 (GMT)
committerTim-Philipp Müller <tim.muller@collabora.co.uk>2009-05-31 18:17:33 (GMT)
commit7e4b164c125926784805446482151e4e7bfffe96 (patch)
tree7b1425949320e7a3e59499975dc98f505180c64d
parent7c4e618471eb4218785d5891da46b67de952397e (diff)
fakesink: hack around crasher bug in g_object_notify() for out-of-band events
GObject may crash if two threads do concurrent g_object_notify() on the same object. This may happen if fakesink receives an out-of-band event such as FLUSH_START while processing a buffer or serialised event in the streaming thread. Since this may happen with the default settings during a common operation like a seek, and there seems to be little chance of a timely fix in GObject (see #166020), we should hack around this issue by protecting all of fakesink's direct g_object_notify() calls with a lock. Also add unit test for the above. Fixes #554460.
-rw-r--r--plugins/elements/gstfakesink.c33
-rw-r--r--plugins/elements/gstfakesink.h1
-rw-r--r--tests/check/Makefile.am4
-rw-r--r--tests/check/elements/fakesink.c103
4 files changed, 138 insertions, 3 deletions
diff --git a/plugins/elements/gstfakesink.c b/plugins/elements/gstfakesink.c
index 0ba5383..b31801e 100644
--- a/plugins/elements/gstfakesink.c
+++ b/plugins/elements/gstfakesink.c
@@ -113,6 +113,7 @@ static void gst_fake_sink_set_property (GObject * object, guint prop_id,
const GValue * value, GParamSpec * pspec);
static void gst_fake_sink_get_property (GObject * object, guint prop_id,
GValue * value, GParamSpec * pspec);
+static void gst_fake_sink_finalize (GObject * obj);
static GstStateChangeReturn gst_fake_sink_change_state (GstElement * element,
GstStateChange transition);
@@ -182,6 +183,7 @@ gst_fake_sink_class_init (GstFakeSinkClass * klass)
gobject_class->set_property = GST_DEBUG_FUNCPTR (gst_fake_sink_set_property);
gobject_class->get_property = GST_DEBUG_FUNCPTR (gst_fake_sink_get_property);
+ gobject_class->finalize = gst_fake_sink_finalize;
g_object_class_install_property (gobject_class, PROP_STATE_ERROR,
g_param_spec_enum ("state-error", "State Error",
@@ -265,6 +267,17 @@ gst_fake_sink_init (GstFakeSink * fakesink, GstFakeSinkClass * g_class)
fakesink->state_error = DEFAULT_STATE_ERROR;
fakesink->signal_handoffs = DEFAULT_SIGNAL_HANDOFFS;
fakesink->num_buffers = DEFAULT_NUM_BUFFERS;
+ g_static_rec_mutex_init (&fakesink->notify_lock);
+}
+
+static void
+gst_fake_sink_finalize (GObject * obj)
+{
+ GstFakeSink *sink = GST_FAKE_SINK (obj);
+
+ g_static_rec_mutex_free (&sink->notify_lock);
+
+ G_OBJECT_CLASS (parent_class)->finalize (obj);
}
static void
@@ -344,6 +357,20 @@ gst_fake_sink_get_property (GObject * object, guint prop_id, GValue * value,
}
}
+static void
+gst_fake_sink_notify_last_message (GstFakeSink * sink)
+{
+ /* FIXME: this hacks around a bug in GLib/GObject: doing concurrent
+ * g_object_notify() on the same object might lead to crashes, see
+ * http://bugzilla.gnome.org/show_bug.cgi?id=166020#c60 and follow-ups.
+ * So we really don't want to do a g_object_notify() here for out-of-band
+ * events with the streaming thread possibly also doing a g_object_notify()
+ * for an in-band buffer or event. */
+ g_static_rec_mutex_lock (&sink->notify_lock);
+ g_object_notify ((GObject *) sink, "last_message");
+ g_static_rec_mutex_unlock (&sink->notify_lock);
+}
+
static gboolean
gst_fake_sink_event (GstBaseSink * bsink, GstEvent * event)
{
@@ -367,7 +394,7 @@ gst_fake_sink_event (GstBaseSink * bsink, GstEvent * event)
g_free (sstr);
GST_OBJECT_UNLOCK (sink);
- g_object_notify (G_OBJECT (sink), "last_message");
+ gst_fake_sink_notify_last_message (sink);
}
if (GST_BASE_SINK_CLASS (parent_class)->event) {
@@ -392,7 +419,7 @@ gst_fake_sink_preroll (GstBaseSink * bsink, GstBuffer * buffer)
sink->last_message = g_strdup_printf ("preroll ******* ");
GST_OBJECT_UNLOCK (sink);
- g_object_notify (G_OBJECT (sink), "last_message");
+ gst_fake_sink_notify_last_message (sink);
}
if (sink->signal_handoffs) {
g_signal_emit (sink,
@@ -448,7 +475,7 @@ gst_fake_sink_render (GstBaseSink * bsink, GstBuffer * buf)
GST_MINI_OBJECT_CAST (buf)->flags, buf);
GST_OBJECT_UNLOCK (sink);
- g_object_notify (G_OBJECT (sink), "last_message");
+ gst_fake_sink_notify_last_message (sink);
}
if (sink->signal_handoffs)
g_signal_emit (G_OBJECT (sink), gst_fake_sink_signals[SIGNAL_HANDOFF], 0,
diff --git a/plugins/elements/gstfakesink.h b/plugins/elements/gstfakesink.h
index 2db98b9..9e382dd 100644
--- a/plugins/elements/gstfakesink.h
+++ b/plugins/elements/gstfakesink.h
@@ -82,6 +82,7 @@ struct _GstFakeSink {
gchar *last_message;
gint num_buffers;
gint num_buffers_left;
+ GStaticRecMutex notify_lock;
};
struct _GstFakeSinkClass {
diff --git a/tests/check/Makefile.am b/tests/check/Makefile.am
index 31644e1..ef25ab0 100644
--- a/tests/check/Makefile.am
+++ b/tests/check/Makefile.am
@@ -152,6 +152,10 @@ libs_gdp_LDADD = \
elements_fdsrc_CFLAGS=$(GST_OBJ_CFLAGS) $(CHECK_CFLAGS) -DTESTFILE=\"$(top_srcdir)/configure.ac\"
elements_filesrc_CFLAGS=$(GST_OBJ_CFLAGS) $(CHECK_CFLAGS) -DTESTFILE=\"$(top_srcdir)/configure.ac\"
+elements_fakesink_LDADD = \
+ $(top_builddir)/libs/gst/base/libgstbase-@GST_MAJORMINOR@.la \
+ $(LDADD)
+
libs_basesrc_LDADD = \
$(top_builddir)/libs/gst/base/libgstbase-@GST_MAJORMINOR@.la \
$(LDADD)
diff --git a/tests/check/elements/fakesink.c b/tests/check/elements/fakesink.c
index c05cc08..60cde64 100644
--- a/tests/check/elements/fakesink.c
+++ b/tests/check/elements/fakesink.c
@@ -4,6 +4,7 @@
*
* Copyright (C) <2005> Thomas Vander Stichele <thomas at apestaart dot org>
* <2007> Wim Taymans <wim@fluendo.com>
+ * <2009> Tim-Philipp Müller <tim centricular net>
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Library General Public
@@ -23,6 +24,7 @@
#include <unistd.h>
+#include <gst/base/gstpushsrc.h>
#include <gst/check/gstcheck.h>
typedef struct
@@ -858,6 +860,106 @@ GST_START_TEST (test_position)
GST_END_TEST;
+/* like fakesrc, but also pushes an OOB event after each buffer */
+typedef GstPushSrc OOBSource;
+typedef GstPushSrcClass OOBSourceClass;
+
+GST_BOILERPLATE (OOBSource, oob_source, GstPushSrc, GST_TYPE_PUSH_SRC);
+
+static void
+oob_source_base_init (gpointer g_class)
+{
+ static GstStaticPadTemplate sinktemplate = GST_STATIC_PAD_TEMPLATE ("src",
+ GST_PAD_SRC, GST_PAD_ALWAYS, GST_STATIC_CAPS_ANY);
+
+ gst_element_class_add_pad_template (GST_ELEMENT_CLASS (g_class),
+ gst_static_pad_template_get (&sinktemplate));
+}
+
+static GstFlowReturn
+oob_source_create (GstPushSrc * src, GstBuffer ** p_buf)
+{
+ *p_buf = gst_buffer_new ();
+
+ gst_pad_push_event (GST_BASE_SRC_PAD (src),
+ gst_event_new_custom (GST_EVENT_CUSTOM_DOWNSTREAM_OOB, NULL));
+
+ return GST_FLOW_OK;
+}
+
+static void
+oob_source_class_init (OOBSourceClass * klass)
+{
+ GstPushSrcClass *pushsrc_class = GST_PUSH_SRC_CLASS (klass);
+
+ pushsrc_class->create = GST_DEBUG_FUNCPTR (oob_source_create);
+}
+
+static void
+oob_source_init (OOBSource * src, OOBSourceClass * g_class)
+{
+ /* nothing to do */
+}
+
+#define NOTIFY_RACE_NUM_PIPELINES 20
+
+typedef struct
+{
+ GstElement *src;
+ GstElement *queue;
+ GstElement *sink;
+ GstElement *pipe;
+} NotifyRacePipeline;
+
+static void
+test_notify_race_setup_pipeline (NotifyRacePipeline * p)
+{
+ p->pipe = gst_pipeline_new ("pipeline");
+ p->src = g_object_new (oob_source_get_type (), NULL);
+
+ p->queue = gst_element_factory_make ("queue", NULL);
+ g_object_set (p->queue, "max-size-buffers", 2, NULL);
+
+ p->sink = gst_element_factory_make ("fakesink", NULL);
+ gst_bin_add (GST_BIN (p->pipe), p->src);
+ gst_bin_add (GST_BIN (p->pipe), p->queue);
+ gst_bin_add (GST_BIN (p->pipe), p->sink);
+ gst_element_link_many (p->src, p->queue, p->sink, NULL);
+
+ fail_unless_equals_int (gst_element_set_state (p->pipe, GST_STATE_PLAYING),
+ GST_STATE_CHANGE_ASYNC);
+ fail_unless_equals_int (gst_element_get_state (p->pipe, NULL, NULL, -1),
+ GST_STATE_CHANGE_SUCCESS);
+}
+
+static void
+test_notify_race_cleanup_pipeline (NotifyRacePipeline * p)
+{
+ gst_element_set_state (p->pipe, GST_STATE_NULL);
+ gst_object_unref (p->pipe);
+ memset (p, 0, sizeof (NotifyRacePipeline));
+}
+
+/* we create N pipelines to make sure the notify race isn't per-class, but
+ * only per instance */
+GST_START_TEST (test_notify_race)
+{
+ NotifyRacePipeline pipelines[NOTIFY_RACE_NUM_PIPELINES];
+ int i;
+
+ for (i = 0; i < G_N_ELEMENTS (pipelines); ++i) {
+ test_notify_race_setup_pipeline (&pipelines[i]);
+ }
+
+ g_usleep (2 * G_USEC_PER_SEC);
+
+ for (i = 0; i < G_N_ELEMENTS (pipelines); ++i) {
+ test_notify_race_cleanup_pipeline (&pipelines[i]);
+ }
+}
+
+GST_END_TEST;
+
static Suite *
fakesink_suite (void)
{
@@ -872,6 +974,7 @@ fakesink_suite (void)
tcase_add_test (tc_chain, test_eos);
tcase_add_test (tc_chain, test_eos2);
tcase_add_test (tc_chain, test_position);
+ tcase_add_test (tc_chain, test_notify_race);
return s;
}