From fbe3a809644eb71c6064a1e9841bfc07756fcfab Mon Sep 17 00:00:00 2001 From: Eloi BAIL Date: Thu, 30 May 2013 11:59:36 -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 | 34 +++++++++++- ges/ges-timeline.c | 127 +++++++++++++++++++++++++++++--------------- 2 files changed, 118 insertions(+), 43 deletions(-) diff --git a/ges/ges-timeline-pipeline.c b/ges/ges-timeline-pipeline.c index 7b17763..9e3d230 100644 --- a/ges/ges-timeline-pipeline.c +++ b/ges/ges-timeline-pipeline.c @@ -35,6 +35,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 @@ -47,6 +65,9 @@ typedef struct GstPad *blocked_pad; } OutputChain; + + + G_DEFINE_TYPE (GESTimelinePipeline, ges_timeline_pipeline, GST_TYPE_PIPELINE); struct _GESTimelinePipelinePrivate @@ -59,6 +80,7 @@ struct _GESTimelinePipelinePrivate GESPipelineFlags mode; + GMutex dyn_mutex; GList *chains; GstEncodingProfile *profile; @@ -301,6 +323,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) { @@ -399,12 +422,14 @@ pad_added_cb (GstElement * timeline, GstPad * pad, GESTimelinePipeline * self) GstPad *sinkpad; gboolean reconfigured = FALSE; + LOCK_DYN (self); GST_DEBUG_OBJECT (self, "new pad %s:%s , caps:%" GST_PTR_FORMAT, GST_DEBUG_PAD_NAME (pad), GST_PAD_CAPS (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; } @@ -526,7 +551,7 @@ pad_added_cb (GstElement * timeline, GstPad * pad, GESTimelinePipeline * self) /* If chain wasn't already present, insert it in list */ 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; @@ -538,6 +563,7 @@ error: if (sinkpad) gst_object_unref (sinkpad); g_free (chain); + UNLOCK_DYN (self); } } @@ -548,16 +574,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; } @@ -595,6 +624,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"); } @@ -605,6 +635,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; @@ -613,6 +644,7 @@ no_more_pads_cb (GstElement * timeline, GESTimelinePipeline * self) gst_pad_set_blocked_async (chain->blocked_pad, FALSE, pad_blocked, NULL); } } + UNLOCK_DYN (self); } /** diff --git a/ges/ges-timeline.c b/ges/ges-timeline.c index 0b92c02..58048ed 100644 --- a/ges/ges-timeline.c +++ b/ges/ges-timeline.c @@ -53,6 +53,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 + #define GES_TIMELINE_PENDINGOBJS_GET_LOCK(timeline) \ (GES_TIMELINE(timeline)->priv->pendingobjects_lock) #define GES_TIMELINE_PENDINGOBJS_LOCK(timeline) \ @@ -129,6 +147,8 @@ struct _GESTimelinePrivate /* We keep 1 reference to our trackobject here */ GSequence *tracksources; /* TrackSource-s sorted by start/priorities */ + GRecMutex dyn_mutex; + MoveContext movecontext; }; @@ -321,8 +341,8 @@ ges_timeline_class_init (GESTimelineClass * klass) */ properties[PROP_SNAPPING_DISTANCE] = g_param_spec_uint64 ("snapping-distance", "Snapping distance", - "Distance from which moving an object will snap with neighboors", 0, - G_MAXUINT64, 0, G_PARAM_READWRITE); + "Distance from which moving an object will snap with neighboors", + 0, G_MAXUINT64, 0, G_PARAM_READWRITE); g_object_class_install_property (object_class, PROP_SNAPPING_DISTANCE, properties[PROP_SNAPPING_DISTANCE]); @@ -342,8 +362,8 @@ ges_timeline_class_init (GESTimelineClass * klass) */ properties[PROP_UPDATE] = g_param_spec_boolean ("update", "Update", - "Update the internal pipeline on every modification", TRUE, - G_PARAM_READWRITE); + "Update the internal pipeline on every modification", + TRUE, G_PARAM_READWRITE); g_object_class_install_property (object_class, PROP_UPDATE, properties[PROP_UPDATE]); @@ -356,7 +376,8 @@ ges_timeline_class_init (GESTimelineClass * klass) */ ges_timeline_signals[TRACK_ADDED] = g_signal_new ("track-added", G_TYPE_FROM_CLASS (klass), - G_SIGNAL_RUN_FIRST, G_STRUCT_OFFSET (GESTimelineClass, track_added), NULL, + G_SIGNAL_RUN_FIRST, G_STRUCT_OFFSET (GESTimelineClass, + track_added), NULL, NULL, ges_marshal_VOID__OBJECT, G_TYPE_NONE, 1, GES_TYPE_TRACK); /** @@ -368,8 +389,9 @@ ges_timeline_class_init (GESTimelineClass * klass) */ ges_timeline_signals[TRACK_REMOVED] = g_signal_new ("track-removed", G_TYPE_FROM_CLASS (klass), - G_SIGNAL_RUN_FIRST, G_STRUCT_OFFSET (GESTimelineClass, track_removed), - NULL, NULL, ges_marshal_VOID__OBJECT, G_TYPE_NONE, 1, GES_TYPE_TRACK); + G_SIGNAL_RUN_FIRST, G_STRUCT_OFFSET (GESTimelineClass, + track_removed), NULL, + NULL, ges_marshal_VOID__OBJECT, G_TYPE_NONE, 1, GES_TYPE_TRACK); /** * GESTimeline::layer-added @@ -380,7 +402,8 @@ ges_timeline_class_init (GESTimelineClass * klass) */ ges_timeline_signals[LAYER_ADDED] = g_signal_new ("layer-added", G_TYPE_FROM_CLASS (klass), - G_SIGNAL_RUN_FIRST, G_STRUCT_OFFSET (GESTimelineClass, layer_added), NULL, + G_SIGNAL_RUN_FIRST, G_STRUCT_OFFSET (GESTimelineClass, + layer_added), NULL, NULL, ges_marshal_VOID__OBJECT, G_TYPE_NONE, 1, GES_TYPE_TIMELINE_LAYER); /** @@ -392,9 +415,9 @@ ges_timeline_class_init (GESTimelineClass * klass) */ ges_timeline_signals[LAYER_REMOVED] = g_signal_new ("layer-removed", G_TYPE_FROM_CLASS (klass), - G_SIGNAL_RUN_FIRST, G_STRUCT_OFFSET (GESTimelineClass, layer_removed), - NULL, NULL, ges_marshal_VOID__OBJECT, G_TYPE_NONE, 1, - GES_TYPE_TIMELINE_LAYER); + G_SIGNAL_RUN_FIRST, G_STRUCT_OFFSET (GESTimelineClass, + layer_removed), NULL, + NULL, ges_marshal_VOID__OBJECT, G_TYPE_NONE, 1, GES_TYPE_TIMELINE_LAYER); /** * GESTimeline::discovery-error: @@ -406,8 +429,9 @@ ges_timeline_class_init (GESTimelineClass * klass) */ ges_timeline_signals[DISCOVERY_ERROR] = g_signal_new ("discovery-error", G_TYPE_FROM_CLASS (klass), - G_SIGNAL_RUN_FIRST, 0, NULL, NULL, gst_marshal_VOID__OBJECT_BOXED, - G_TYPE_NONE, 2, GES_TYPE_TIMELINE_FILE_SOURCE, GST_TYPE_G_ERROR); + G_SIGNAL_RUN_FIRST, 0, NULL, NULL, + gst_marshal_VOID__OBJECT_BOXED, G_TYPE_NONE, 2, + GES_TYPE_TIMELINE_FILE_SOURCE, GST_TYPE_G_ERROR); /** * GESTimeline::track-objects-snapping: @@ -423,8 +447,8 @@ ges_timeline_class_init (GESTimelineClass * klass) ges_timeline_signals[SNAPING_STARTED] = g_signal_new ("snapping-started", G_TYPE_FROM_CLASS (klass), G_SIGNAL_RUN_LAST, 0, NULL, NULL, NULL, - G_TYPE_NONE, 3, GES_TYPE_TRACK_OBJECT, GES_TYPE_TRACK_OBJECT, - G_TYPE_UINT64); + G_TYPE_NONE, 3, GES_TYPE_TRACK_OBJECT, + GES_TYPE_TRACK_OBJECT, G_TYPE_UINT64); /** * GESTimeline::snapping-end: @@ -440,8 +464,8 @@ ges_timeline_class_init (GESTimelineClass * klass) ges_timeline_signals[SNAPING_ENDED] = g_signal_new ("snapping-ended", G_TYPE_FROM_CLASS (klass), G_SIGNAL_RUN_LAST, 0, NULL, NULL, NULL, - G_TYPE_NONE, 3, GES_TYPE_TRACK_OBJECT, GES_TYPE_TRACK_OBJECT, - G_TYPE_UINT64); + G_TYPE_NONE, 3, GES_TYPE_TRACK_OBJECT, + GES_TYPE_TRACK_OBJECT, G_TYPE_UINT64); } static void @@ -767,7 +791,8 @@ ges_timeline_emit_snappig (GESTimeline * timeline, GESTrackObject * obj1, *mv_ctx->last_snap_ts : GST_CLOCK_TIME_NONE; GST_DEBUG_OBJECT (timeline, "Distance: %" GST_TIME_FORMAT " snapping at %" - GST_TIME_FORMAT, GST_TIME_ARGS (timeline->priv->snapping_distance), + GST_TIME_FORMAT, + GST_TIME_ARGS (timeline->priv->snapping_distance), GST_TIME_ARGS (snap_time)); if (timecode == NULL) { @@ -1008,8 +1033,9 @@ ges_timeline_set_moving_context (GESTimeline * timeline, GESTrackObject * obj, GST_DEBUG_OBJECT (tlobj, "Changing context:\nold: obj: %p, mode: %d, edge: %d \n" - "new: obj: %p, mode: %d, edge: %d ! Has changed %i", mv_ctx->obj, - mv_ctx->mode, mv_ctx->edge, tlobj, mode, edge, mv_ctx->needs_move_ctx); + "new: obj: %p, mode: %d, edge: %d ! Has changed %i", + mv_ctx->obj, mv_ctx->mode, mv_ctx->edge, tlobj, mode, + edge, mv_ctx->needs_move_ctx); clean_movecontext (mv_ctx); mv_ctx->edge = edge; @@ -1041,7 +1067,8 @@ ges_timeline_trim_object_simple (GESTimeline * timeline, GESTrackObject * obj, gboolean ret = TRUE; gint64 real_dur; - GST_DEBUG_OBJECT (obj, "Trimming to %" GST_TIME_FORMAT " %s snaping, edge %i", + GST_DEBUG_OBJECT (obj, + "Trimming to %" GST_TIME_FORMAT " %s snaping, edge %i", GST_TIME_ARGS (position), snapping ? "Is" : "Not", edge); start = ges_track_object_get_start (obj); @@ -1293,7 +1320,8 @@ timeline_roll_object (GESTimeline * timeline, GESTrackObject * obj, /* Check that the object should be resized at this position * even if an error accurs, we keep doing our job */ if (tmpend == start) { - ret &= ges_timeline_trim_object_simple (timeline, tmptckobj, NULL, + ret &= + ges_timeline_trim_object_simple (timeline, tmptckobj, NULL, GES_EDGE_END, position, FALSE); break; } @@ -1312,7 +1340,8 @@ timeline_roll_object (GESTimeline * timeline, GESTrackObject * obj, if (snapped) position = *snapped; - ret &= ges_timeline_trim_object_simple (timeline, obj, NULL, GES_EDGE_END, + ret &= + ges_timeline_trim_object_simple (timeline, obj, NULL, GES_EDGE_END, position, FALSE); /* In the case we reached max_duration we just make sure to roll @@ -1330,7 +1359,8 @@ timeline_roll_object (GESTimeline * timeline, GESTrackObject * obj, /* Check that the object should be resized at this position * even if an error accure, we keep doing our job */ if (end == tmpstart) { - ret &= ges_timeline_trim_object_simple (timeline, tmptckobj, NULL, + ret &= + ges_timeline_trim_object_simple (timeline, tmptckobj, NULL, GES_EDGE_START, position, FALSE); } } @@ -1357,8 +1387,8 @@ gboolean timeline_move_object (GESTimeline * timeline, GESTrackObject * object, GList * layers, GESEdge edge, guint64 position) { - if (!ges_timeline_set_moving_context (timeline, object, GES_EDIT_MODE_RIPPLE, - edge, layers)) { + if (!ges_timeline_set_moving_context + (timeline, object, GES_EDIT_MODE_RIPPLE, edge, layers)) { GST_DEBUG_OBJECT (object, "Could not move to %" GST_TIME_FORMAT, GST_TIME_ARGS (position)); @@ -1437,13 +1467,15 @@ timeline_context_to_layer (GESTimeline * timeline, gint offset) prio = ges_timeline_layer_get_priority (layer); /* We know that the layer exists as we created it */ - new_layer = GES_TIMELINE_LAYER (g_list_nth_data (timeline->priv->layers, - prio + offset)); + new_layer = + GES_TIMELINE_LAYER (g_list_nth_data + (timeline->priv->layers, prio + offset)); if (new_layer == NULL) { do { new_layer = ges_timeline_append_layer (timeline); - } while (ges_timeline_layer_get_priority (new_layer) < prio + offset); + } + while (ges_timeline_layer_get_priority (new_layer) < prio + offset); } ret &= ges_timeline_object_move_to_layer (key, new_layer); @@ -1559,7 +1591,8 @@ discoverer_discovered_cb (GstDiscoverer * discoverer, } if (!found) { - GST_WARNING ("Discovered %s, that seems not to be in the list of sources" + GST_WARNING + ("Discovered %s, that seems not to be in the list of sources" "to discover", uri); GES_TIMELINE_PENDINGOBJS_UNLOCK (timeline); return; @@ -1607,8 +1640,7 @@ discoverer_discovered_cb (GstDiscoverer * discoverer, } else if (GST_IS_DISCOVERER_VIDEO_INFO (sinf)) { tfs_supportedformats |= GES_TRACK_TYPE_VIDEO; ges_timeline_filesource_set_supported_formats (tfs, tfs_supportedformats); - if (gst_discoverer_video_info_is_image ((GstDiscovererVideoInfo *) - sinf)) { + if (gst_discoverer_video_info_is_image ((GstDiscovererVideoInfo *) sinf)) { tfs_supportedformats |= GES_TRACK_TYPE_AUDIO; ges_timeline_filesource_set_supported_formats (tfs, tfs_supportedformats); @@ -1652,7 +1684,8 @@ layer_object_added_cb (GESTimelineLayer * layer, GESTimelineObject * object, GESTimeline * timeline) { if (ges_timeline_object_is_moving_from_layer (object)) { - GST_DEBUG ("TimelineObject %p is moving from a layer to another, not doing" + GST_DEBUG + ("TimelineObject %p is moving from a layer to another, not doing" " anything on it", object); timeline->priv->movecontext.needs_move_ctx = TRUE; return; @@ -1707,7 +1740,8 @@ layer_object_removed_cb (GESTimelineLayer * layer, GESTimelineObject * object, GList *trackobjects, *tmp; if (ges_timeline_object_is_moving_from_layer (object)) { - GST_DEBUG ("TimelineObject %p is moving from a layer to another, not doing" + GST_DEBUG + ("TimelineObject %p is moving from a layer to another, not doing" " anything on it", object); return; } @@ -1722,6 +1756,7 @@ layer_object_removed_cb (GESTimelineLayer * layer, GESTimelineObject * object, GESTrackObject *trobj = (GESTrackObject *) tmp->data; GST_DEBUG ("Trying to remove TrackObject %p", trobj); + LOCK_DYN (timeline); if (G_LIKELY (g_list_find_custom (timeline->priv->tracks, ges_track_object_get_track (trobj), (GCompareFunc) custom_find_track))) { @@ -1730,6 +1765,7 @@ layer_object_removed_cb (GESTimelineLayer * layer, GESTimelineObject * object, ges_timeline_object_release_track_object (object, trobj); } + UNLOCK_DYN (timeline); /* removing the reference added by _get_track_objects() */ g_object_unref (trobj); } @@ -1804,8 +1840,8 @@ track_object_removed_cb (GESTrack * track, GESTrackObject * object, if (GES_IS_TRACK_SOURCE (object)) { g_signal_handlers_disconnect_by_func (object, trackobj_start_changed_cb, NULL); - g_signal_handlers_disconnect_by_func (object, trackobj_duration_changed_cb, - NULL); + g_signal_handlers_disconnect_by_func (object, + trackobj_duration_changed_cb, NULL); /* Make sure to reinitialise the moving context next time */ timeline->priv->movecontext.needs_move_ctx = TRUE; @@ -1828,6 +1864,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; @@ -1854,6 +1891,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 @@ -2234,8 +2272,9 @@ 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); priv->tracks = g_list_append (priv->tracks, tr_priv); - + UNLOCK_DYN (timeline); /* Listen to pad-added/-removed */ g_signal_connect (track, "pad-added", (GCallback) pad_added_cb, tr_priv); g_signal_connect (track, "pad-removed", (GCallback) pad_removed_cb, tr_priv); @@ -2297,17 +2336,19 @@ ges_timeline_remove_track (GESTimeline * timeline, GESTrack * track) GESTimelinePrivate *priv = timeline->priv; GST_DEBUG ("timeline:%p, track:%p", timeline, track); - + LOCK_DYN (timeline); if (G_UNLIKELY (!(tmp = g_list_find_custom (priv->tracks, (gconstpointer) track, (GCompareFunc) custom_find_track)))) { GST_WARNING ("Track doesn't belong to this timeline"); + UNLOCK_DYN (timeline); return FALSE; } tr_priv = tmp->data; - priv->tracks = g_list_remove (priv->tracks, tr_priv); + priv->tracks = g_list_remove (priv->tracks, tr_priv); + UNLOCK_DYN (timeline); ges_track_set_timeline (track, NULL); /* Remove ghost pad */ @@ -2362,13 +2403,15 @@ GESTrack * ges_timeline_get_track_for_pad (GESTimeline * timeline, GstPad * pad) { GList *tmp; - + LOCK_DYN (timeline); for (tmp = timeline->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; } -- 1.8.1.2