From 7e4b164c125926784805446482151e4e7bfffe96 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Tim-Philipp=20M=C3=BCller?= Date: Sat, 30 May 2009 20:36:25 +0100 Subject: [PATCH] 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. --- plugins/elements/gstfakesink.c | 33 +++++++++++-- plugins/elements/gstfakesink.h | 1 + tests/check/Makefile.am | 4 ++ tests/check/elements/fakesink.c | 103 ++++++++++++++++++++++++++++++++++++++++ 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 * <2007> Wim Taymans + * <2009> Tim-Philipp Müller * * 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 +#include #include 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; } -- 2.7.4