From 00a08c69acc99566d6ceac0b5a8cc5b532a62dfa Mon Sep 17 00:00:00 2001 From: Jan Schmidt Date: Tue, 24 Mar 2020 00:23:24 +1100 Subject: [PATCH] splitmuxsrc: Fix some deadlock conditions and a crash When switching the splitmuxsrc state back to NULL quickly, it can encounter deadlocks shutting down the part readers that are still starting up, or encounter a crash if the splitmuxsrc cleaned up the parts before the async callback could run. Taking the state lock to post async-start / async-done messages can deadlock if the state change function is trying to shut down the element, so use some finer grained locks for that. --- gst/multifile/gstsplitmuxpartreader.c | 13 +++++++++---- gst/multifile/gstsplitmuxpartreader.h | 1 + gst/multifile/gstsplitmuxsrc.c | 32 ++++++++++++++++++++++++-------- gst/multifile/gstsplitmuxsrc.h | 4 ++++ 4 files changed, 38 insertions(+), 12 deletions(-) diff --git a/gst/multifile/gstsplitmuxpartreader.c b/gst/multifile/gstsplitmuxpartreader.c index 51a8635..77a1745 100644 --- a/gst/multifile/gstsplitmuxpartreader.c +++ b/gst/multifile/gstsplitmuxpartreader.c @@ -37,6 +37,9 @@ GST_DEBUG_CATEGORY_STATIC (splitmux_part_debug); #define SPLITMUX_PART_TYPE_LOCK(p) g_mutex_lock(&(p)->type_lock) #define SPLITMUX_PART_TYPE_UNLOCK(p) g_mutex_unlock(&(p)->type_lock) +#define SPLITMUX_PART_MSG_LOCK(p) g_mutex_lock(&(p)->msg_lock) +#define SPLITMUX_PART_MSG_UNLOCK(p) g_mutex_unlock(&(p)->msg_lock) + typedef struct _GstSplitMuxPartPad { GstPad parent; @@ -636,6 +639,7 @@ gst_splitmux_part_reader_init (GstSplitMuxPartReader * reader) g_cond_init (&reader->inactive_cond); g_mutex_init (&reader->lock); g_mutex_init (&reader->type_lock); + g_mutex_init (&reader->msg_lock); /* FIXME: Create elements on a state change */ reader->src = gst_element_factory_make ("filesrc", NULL); @@ -683,6 +687,7 @@ splitmux_part_reader_finalize (GObject * object) g_cond_clear (&reader->inactive_cond); g_mutex_clear (&reader->lock); g_mutex_clear (&reader->type_lock); + g_mutex_clear (&reader->msg_lock); g_free (reader->path); @@ -694,12 +699,12 @@ do_async_start (GstSplitMuxPartReader * reader) { GstMessage *message; - GST_STATE_LOCK (reader); + SPLITMUX_PART_MSG_LOCK (reader); reader->async_pending = TRUE; message = gst_message_new_async_start (GST_OBJECT_CAST (reader)); GST_BIN_CLASS (parent_class)->handle_message (GST_BIN_CAST (reader), message); - GST_STATE_UNLOCK (reader); + SPLITMUX_PART_MSG_UNLOCK (reader); } static void @@ -707,7 +712,7 @@ do_async_done (GstSplitMuxPartReader * reader) { GstMessage *message; - GST_STATE_LOCK (reader); + SPLITMUX_PART_MSG_LOCK (reader); if (reader->async_pending) { message = gst_message_new_async_done (GST_OBJECT_CAST (reader), @@ -717,7 +722,7 @@ do_async_done (GstSplitMuxPartReader * reader) reader->async_pending = FALSE; } - GST_STATE_UNLOCK (reader); + SPLITMUX_PART_MSG_UNLOCK (reader); } static void diff --git a/gst/multifile/gstsplitmuxpartreader.h b/gst/multifile/gstsplitmuxpartreader.h index 1659b6e..78ecc6e 100644 --- a/gst/multifile/gstsplitmuxpartreader.h +++ b/gst/multifile/gstsplitmuxpartreader.h @@ -80,6 +80,7 @@ struct _GstSplitMuxPartReader GCond inactive_cond; GMutex lock; GMutex type_lock; + GMutex msg_lock; GstSplitMuxPartReaderPadCb get_pad_cb; gpointer cb_data; diff --git a/gst/multifile/gstsplitmuxsrc.c b/gst/multifile/gstsplitmuxsrc.c index 757f3bd..f8c27fb 100644 --- a/gst/multifile/gstsplitmuxsrc.c +++ b/gst/multifile/gstsplitmuxsrc.c @@ -331,13 +331,13 @@ do_async_start (GstSplitMuxSrc * splitmux) { GstMessage *message; - GST_STATE_LOCK (splitmux); + SPLITMUX_SRC_MSG_LOCK (splitmux); splitmux->async_pending = TRUE; message = gst_message_new_async_start (GST_OBJECT_CAST (splitmux)); GST_BIN_CLASS (parent_class)->handle_message (GST_BIN_CAST (splitmux), message); - GST_STATE_UNLOCK (splitmux); + SPLITMUX_SRC_MSG_UNLOCK (splitmux); } static void @@ -345,7 +345,7 @@ do_async_done (GstSplitMuxSrc * splitmux) { GstMessage *message; - GST_STATE_LOCK (splitmux); + SPLITMUX_SRC_MSG_LOCK (splitmux); if (splitmux->async_pending) { message = gst_message_new_async_done (GST_OBJECT_CAST (splitmux), @@ -355,7 +355,7 @@ do_async_done (GstSplitMuxSrc * splitmux) splitmux->async_pending = FALSE; } - GST_STATE_UNLOCK (splitmux); + SPLITMUX_SRC_MSG_UNLOCK (splitmux); } static GstStateChangeReturn @@ -408,10 +408,14 @@ gst_splitmux_src_change_state (GstElement * element, GstStateChange transition) static void gst_splitmux_src_activate_first_part (GstSplitMuxSrc * splitmux) { - if (!gst_splitmux_src_activate_part (splitmux, 0, GST_SEEK_FLAG_NONE)) { - GST_ELEMENT_ERROR (splitmux, RESOURCE, OPEN_READ, (NULL), - ("Failed to activate first part for playback")); + SPLITMUX_SRC_LOCK (splitmux); + if (splitmux->running) { + if (!gst_splitmux_src_activate_part (splitmux, 0, GST_SEEK_FLAG_NONE)) { + GST_ELEMENT_ERROR (splitmux, RESOURCE, OPEN_READ, (NULL), + ("Failed to activate first part for playback")); + } } + SPLITMUX_SRC_UNLOCK (splitmux); } static GstBusSyncReply @@ -887,6 +891,14 @@ gst_splitmux_src_start (GstSplitMuxSrc * splitmux) gchar **files; guint i; + SPLITMUX_SRC_LOCK (splitmux); + if (splitmux->running) { + /* splitmux is still running / stopping. We can't start again yet */ + SPLITMUX_SRC_UNLOCK (splitmux); + return FALSE; + } + SPLITMUX_SRC_UNLOCK (splitmux); + GST_DEBUG_OBJECT (splitmux, "Starting"); g_signal_emit (splitmux, signals[SIGNAL_FORMAT_LOCATION], 0, &files); @@ -980,7 +992,10 @@ gst_splitmux_src_stop (GstSplitMuxSrc * splitmux) GST_DEBUG_OBJECT (splitmux, "Stopping"); - /* Stop and destroy all parts */ + SPLITMUX_SRC_UNLOCK (splitmux); + + /* Stop and destroy all parts. We don't need the lock here, + * because all parts were created in _start() */ for (i = 0; i < splitmux->num_created_parts; i++) { if (splitmux->parts[i] == NULL) continue; @@ -988,6 +1003,7 @@ gst_splitmux_src_stop (GstSplitMuxSrc * splitmux) g_object_unref (splitmux->parts[i]); splitmux->parts[i] = NULL; } + SPLITMUX_SRC_LOCK (splitmux); SPLITMUX_SRC_PADS_WLOCK (splitmux); pads_list = splitmux->pads; diff --git a/gst/multifile/gstsplitmuxsrc.h b/gst/multifile/gstsplitmuxsrc.h index 4c8f95c..8bb84a7 100644 --- a/gst/multifile/gstsplitmuxsrc.h +++ b/gst/multifile/gstsplitmuxsrc.h @@ -44,6 +44,7 @@ struct _GstSplitMuxSrc GstBin parent; GMutex lock; + GMutex msg_lock; gboolean running; gchar *location; /* OBJECT_LOCK */ @@ -108,6 +109,9 @@ gboolean register_splitmuxsrc (GstPlugin * plugin); #define SPLITMUX_SRC_LOCK(s) g_mutex_lock(&(s)->lock) #define SPLITMUX_SRC_UNLOCK(s) g_mutex_unlock(&(s)->lock) +#define SPLITMUX_SRC_MSG_LOCK(s) g_mutex_lock(&(s)->msg_lock) +#define SPLITMUX_SRC_MSG_UNLOCK(s) g_mutex_unlock(&(s)->msg_lock) + #define SPLITMUX_SRC_PADS_WLOCK(s) g_rw_lock_writer_lock(&(s)->pads_rwlock) #define SPLITMUX_SRC_PADS_WUNLOCK(s) g_rw_lock_writer_unlock(&(s)->pads_rwlock) #define SPLITMUX_SRC_PADS_RLOCK(s) g_rw_lock_reader_lock(&(s)->pads_rwlock) -- 2.7.4