From 7e121ff167c900faae46154f076ff5a35d49f938 Mon Sep 17 00:00:00 2001 From: Thibault Saunier Date: Wed, 29 May 2013 14:05:52 -0400 Subject: [PATCH] ges: Protect from Gst dynamic callbacks The pad-added and no-more-pad signal can be emited from any thread so we have to protect our code from that --- ges/ges-timeline-pipeline.c | 30 ++++++++++++++++++++++++++++++ ges/ges-timeline.c | 36 ++++++++++++++++++++++++++++++++++-- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/ges/ges-timeline-pipeline.c b/ges/ges-timeline-pipeline.c index c544d42db3..cfe06499a4 100644 --- a/ges/ges-timeline-pipeline.c +++ b/ges/ges-timeline-pipeline.c @@ -37,6 +37,24 @@ #define DEFAULT_TIMELINE_MODE TIMELINE_MODE_PREVIEW +/* lock to protect dynamic callbacks, like pad-added or nmp */ +#define DYN_LOCK(pipeline) (&GES_TIMELINE_PIPELINE (pipeline)->priv->dyn_mutex) +#define LOCK_DYN(pipeline) G_STMT_START { \ + GST_INFO_OBJECT (pipeline, "Getting dynamic lock from %p", \ + g_thread_self()); \ + g_mutex_lock (DYN_LOCK (pipeline)); \ + GST_INFO_OBJECT (pipeline, "Got Dynamic lock from %p", \ + g_thread_self()); \ + } G_STMT_END + +#define UNLOCK_DYN(pipeline) G_STMT_START { \ + GST_INFO_OBJECT (pipeline, "Unlocking dynamic lock from %p", \ + g_thread_self()); \ + g_mutex_unlock (DYN_LOCK (pipeline)); \ + GST_INFO_OBJECT (pipeline, "Unlocked Dynamic lock from %p", \ + g_thread_self()); \ + } G_STMT_END + /* Structure corresponding to a timeline - sink link */ typedef struct @@ -61,6 +79,7 @@ struct _GESTimelinePipelinePrivate GESPipelineFlags mode; + GMutex dyn_mutex; GList *chains; GstEncodingProfile *profile; @@ -462,6 +481,7 @@ new_output_chain_for_track (GESTimelinePipeline * self, GESTrack * track) return chain; } +/* Should be called with LOCK_DYN */ static OutputChain * get_output_chain_for_track (GESTimelinePipeline * self, GESTrack * track) { @@ -560,6 +580,7 @@ pad_added_cb (GstElement * timeline, GstPad * pad, GESTimelinePipeline * self) GstCaps *caps; gboolean reconfigured = FALSE; + LOCK_DYN (self); caps = gst_pad_query_caps (pad, NULL); GST_DEBUG_OBJECT (self, "new pad %s:%s , caps:%" GST_PTR_FORMAT, @@ -571,6 +592,7 @@ pad_added_cb (GstElement * timeline, GstPad * pad, GESTimelinePipeline * self) if (G_UNLIKELY (!track)) { GST_WARNING_OBJECT (self, "Couldn't find coresponding track !"); + UNLOCK_DYN (self); return; } @@ -696,6 +718,7 @@ pad_added_cb (GstElement * timeline, GstPad * pad, GESTimelinePipeline * self) if (!get_output_chain_for_track (self, track)) self->priv->chains = g_list_append (self->priv->chains, chain); + UNLOCK_DYN (self); GST_DEBUG ("done"); return; @@ -707,6 +730,7 @@ error: if (sinkpad) gst_object_unref (sinkpad); g_free (chain); + UNLOCK_DYN (self); } } @@ -717,16 +741,19 @@ pad_removed_cb (GstElement * timeline, GstPad * pad, GESTimelinePipeline * self) GESTrack *track; GstPad *peer; + LOCK_DYN (self); GST_DEBUG_OBJECT (self, "pad removed %s:%s", GST_DEBUG_PAD_NAME (pad)); if (G_UNLIKELY (!(track = ges_timeline_get_track_for_pad (self->priv->timeline, pad)))) { GST_WARNING_OBJECT (self, "Couldn't find coresponding track !"); + UNLOCK_DYN (self); return; } if (G_UNLIKELY (!(chain = get_output_chain_for_track (self, track)))) { GST_DEBUG_OBJECT (self, "Pad wasn't used"); + UNLOCK_DYN (self); return; } @@ -765,6 +792,7 @@ pad_removed_cb (GstElement * timeline, GstPad * pad, GESTimelinePipeline * self) self->priv->chains = g_list_remove (self->priv->chains, chain); g_free (chain); + UNLOCK_DYN (self); GST_DEBUG ("done"); } @@ -775,6 +803,7 @@ no_more_pads_cb (GstElement * timeline, GESTimelinePipeline * self) GList *tmp; GST_DEBUG ("received no-more-pads"); + LOCK_DYN (self); for (tmp = self->priv->chains; tmp; tmp = g_list_next (tmp)) { OutputChain *chain = (OutputChain *) tmp->data; @@ -786,6 +815,7 @@ no_more_pads_cb (GstElement * timeline, GESTimelinePipeline * self) chain->probe_id = 0; } } + UNLOCK_DYN (self); } /** diff --git a/ges/ges-timeline.c b/ges/ges-timeline.c index 9fe5d1e241..d15fb88216 100644 --- a/ges/ges-timeline.c +++ b/ges/ges-timeline.c @@ -65,6 +65,24 @@ GST_DEBUG_CATEGORY_STATIC (ges_timeline_debug); #undef GST_CAT_DEFAULT #define GST_CAT_DEFAULT ges_timeline_debug +/* lock to protect dynamic callbacks, like pad-added */ +#define DYN_LOCK(timeline) (&GES_TIMELINE (timeline)->priv->dyn_mutex) +#define LOCK_DYN(timeline) G_STMT_START { \ + GST_INFO_OBJECT (timeline, "Getting dynamic lock from %p", \ + g_thread_self()); \ + g_rec_mutex_lock (DYN_LOCK (timeline)); \ + GST_INFO_OBJECT (timeline, "Got Dynamic lock from %p", \ + g_thread_self()); \ + } G_STMT_END + +#define UNLOCK_DYN(timeline) G_STMT_START { \ + GST_INFO_OBJECT (timeline, "Unlocking dynamic lock from %p", \ + g_thread_self()); \ + g_rec_mutex_unlock (DYN_LOCK (timeline)); \ + GST_INFO_OBJECT (timeline, "Unlocked Dynamic lock from %p", \ + g_thread_self()); \ + } G_STMT_END + typedef struct TrackObjIters { GSequenceIter *iter_start; @@ -143,6 +161,7 @@ struct _GESTimelinePrivate /* We keep 1 reference to our trackelement here */ GSequence *tracksources; /* Source-s sorted by start/priorities */ + GRecMutex dyn_mutex; GList *priv_tracks; /* FIXME: We should definitly offer an API over this, * probably through a ges_layer_get_track_elements () method */ @@ -387,7 +406,7 @@ ges_timeline_class_init (GESTimelineClass * klass) g_param_spec_boolean ("auto-transition", "Auto-Transition", "whether the transitions are added", FALSE, G_PARAM_READWRITE)); - /** + /** * GESTimeline:snapping-distance: * * Distance (in nanoseconds) from which a moving object will snap @@ -1966,6 +1985,7 @@ layer_object_removed_cb (GESLayer * layer, GESClip * clip, /* FIXME Check if we should actually check that we control the * track in the new management of TrackElement context */ + LOCK_DYN (timeline); if (G_LIKELY (g_list_find_custom (timeline->priv->priv_tracks, track, (GCompareFunc) custom_find_track) || track == NULL)) { GST_DEBUG ("Belongs to one of the tracks we control"); @@ -1973,6 +1993,7 @@ layer_object_removed_cb (GESLayer * layer, GESClip * clip, ges_container_remove (GES_CONTAINER (clip), GES_TIMELINE_ELEMENT (track_element)); } + UNLOCK_DYN (timeline); } g_signal_handlers_disconnect_by_func (clip, clip_track_element_added_cb, timeline); @@ -2127,6 +2148,7 @@ pad_added_cb (GESTrack * track, GstPad * pad, TrackPrivate * tr_priv) } /* Remember the pad */ + LOCK_DYN (tr_priv->timeline); GST_OBJECT_LOCK (track); tr_priv->pad = pad; @@ -2153,6 +2175,7 @@ pad_added_cb (GESTrack * track, GstPad * pad, TrackPrivate * tr_priv) GST_DEBUG ("Signaling no-more-pads"); gst_element_no_more_pads (GST_ELEMENT (tr_priv->timeline)); } + UNLOCK_DYN (tr_priv->timeline); } static void @@ -2477,8 +2500,10 @@ ges_timeline_add_track (GESTimeline * timeline, GESTrack * track) tr_priv->track = track; /* Add the track to the list of tracks we track */ + LOCK_DYN (timeline); timeline->priv->priv_tracks = g_list_append (timeline->priv->priv_tracks, tr_priv); + UNLOCK_DYN (timeline); timeline->tracks = g_list_append (timeline->tracks, track); /* Listen to pad-added/-removed */ @@ -2551,14 +2576,17 @@ ges_timeline_remove_track (GESTimeline * timeline, GESTrack * track) GST_DEBUG ("timeline:%p, track:%p", timeline, track); priv = timeline->priv; + LOCK_DYN (timeline); if (G_UNLIKELY (!(tmp = g_list_find_custom (priv->priv_tracks, track, (GCompareFunc) custom_find_track)))) { GST_WARNING ("Track doesn't belong to this timeline"); + UNLOCK_DYN (timeline); return FALSE; } tr_priv = tmp->data; priv->priv_tracks = g_list_remove (priv->priv_tracks, tr_priv); + UNLOCK_DYN (timeline); timeline->tracks = g_list_remove (timeline->tracks, track); ges_track_set_timeline (track, NULL); @@ -2616,11 +2644,15 @@ ges_timeline_get_track_for_pad (GESTimeline * timeline, GstPad * pad) { GList *tmp; + LOCK_DYN (timeline); for (tmp = timeline->priv->priv_tracks; tmp; tmp = g_list_next (tmp)) { TrackPrivate *tr_priv = (TrackPrivate *) tmp->data; - if (pad == tr_priv->ghostpad) + if (pad == tr_priv->ghostpad) { + UNLOCK_DYN (timeline); return tr_priv->track; + } } + UNLOCK_DYN (timeline); return NULL; }