From 346f8875537ff2e95657ffacc79db211de76332c Mon Sep 17 00:00:00 2001 From: Thibault Saunier Date: Sat, 18 Jun 2016 16:16:00 -0400 Subject: [PATCH] ges: Don't remove track elements from clips when removing from layer And reuse the same previously created element when adding the clip back to a layer, avoiding losing all setting done on clip children in that situation This is a behaviour change but previous behaviour was actually totally unexpected and people working around that weird behaviour will moste probably not care about that change Differential Revision: https://phabricator.freedesktop.org/D1094 --- ges/ges-clip.c | 20 ++++++++++++++++++-- ges/ges-container.c | 8 +++++++- ges/ges-timeline.c | 6 ++++-- ges/ges-uri-clip.c | 11 ++++++++--- tests/check/ges/basic.c | 6 ++---- tests/check/python/test_clip.py | 28 ++++++++++++++++++++++++++++ 6 files changed, 67 insertions(+), 12 deletions(-) diff --git a/ges/ges-clip.c b/ges/ges-clip.c index a7bcc5d2ed..74438488bd 100644 --- a/ges/ges-clip.c +++ b/ges/ges-clip.c @@ -869,7 +869,7 @@ ges_clip_create_track_element (GESClip * clip, GESTrackType type) GList * ges_clip_create_track_elements (GESClip * clip, GESTrackType type) { - GList *result, *tmp; + GList *result = NULL, *tmp, *children; GESClipClass *klass; guint max_prio, min_prio; @@ -884,7 +884,23 @@ ges_clip_create_track_elements (GESClip * clip, GESTrackType type) GST_DEBUG_OBJECT (clip, "Creating TrackElements for type: %s", ges_track_type_name (type)); - result = klass->create_track_elements (clip, type); + children = ges_container_get_children (GES_CONTAINER (clip), TRUE); + for (tmp = children; tmp; tmp = tmp->next) { + GESTrackElement *child = GES_TRACK_ELEMENT (tmp->data); + + if (!GES_IS_BASE_EFFECT (child) && !ges_track_element_get_track (child) && + ges_track_element_get_track_type (child) & type) { + + GST_DEBUG_OBJECT (clip, "Removing for reusage: %" GST_PTR_FORMAT, child); + result = g_list_prepend (result, g_object_ref (child)); + ges_container_remove (GES_CONTAINER (clip), tmp->data); + } + } + g_list_free_full (children, gst_object_unref); + + if (!result) { + result = klass->create_track_elements (clip, type); + } _get_priority_range (GES_CONTAINER (clip), &min_prio, &max_prio); for (tmp = result; tmp; tmp = tmp->next) { diff --git a/ges/ges-container.c b/ges/ges-container.c index 219abf672d..6a564579d6 100644 --- a/ges/ges-container.c +++ b/ges/ges-container.c @@ -364,9 +364,15 @@ _paste (GESTimelineElement * element, GESTimelineElement * ref, static void _dispose (GObject * object) { + GList *tmp; GESContainer *self = GES_CONTAINER (object); + GList *children = ges_container_get_children (self, FALSE); - g_hash_table_unref (self->priv->mappings); + for (tmp = children; tmp; tmp = tmp->next) + ges_container_remove (self, tmp->data); + + g_list_free_full (children, gst_object_unref); + self->children = NULL; G_OBJECT_CLASS (ges_container_parent_class)->dispose (object); } diff --git a/ges/ges-timeline.c b/ges/ges-timeline.c index e93718ac79..236cc4a1be 100644 --- a/ges/ges-timeline.c +++ b/ges/ges-timeline.c @@ -2511,6 +2511,9 @@ layer_object_removed_cb (GESLayer * layer, GESClip * clip, GESTrackElement *track_element = (GESTrackElement *) tmp->data; GESTrack *track = ges_track_element_get_track (track_element); + if (!track) + continue; + GST_DEBUG_OBJECT (timeline, "Trying to remove TrackElement %p", track_element); @@ -2521,8 +2524,7 @@ layer_object_removed_cb (GESLayer * layer, GESClip * clip, (GCompareFunc) custom_find_track) || track == NULL)) { GST_DEBUG ("Belongs to one of the tracks we control"); - ges_container_remove (GES_CONTAINER (clip), - GES_TIMELINE_ELEMENT (track_element)); + ges_track_remove_element (track, track_element); } UNLOCK_DYN (timeline); } diff --git a/ges/ges-uri-clip.c b/ges/ges-uri-clip.c index 0252d3d6e2..133692d3ee 100644 --- a/ges/ges-uri-clip.c +++ b/ges/ges-uri-clip.c @@ -293,7 +293,9 @@ extractable_set_asset (GESExtractable * self, GESAsset * asset) GES_TIMELINE_ELEMENT (uriclip)->asset = asset; if (layer) { - for (tmp = GES_CONTAINER_CHILDREN (self); tmp; tmp = tmp->next) { + GList *children = ges_container_get_children (GES_CONTAINER (self), TRUE); + + for (tmp = children; tmp; tmp = tmp->next) { if (GES_IS_SOURCE (tmp->data)) { GESTrack *track = ges_track_element_get_track (tmp->data); @@ -303,10 +305,13 @@ extractable_set_asset (GESExtractable * self, GESAsset * asset) video_source = gst_object_ref (tmp->data); ges_track_remove_element (track, tmp->data); + ges_container_remove (GES_CONTAINER (self), tmp->data); } } + g_list_free_full (children, g_object_unref); gst_object_ref (clip); + ges_layer_remove_clip (layer, clip); res = ges_layer_add_clip (layer, clip); @@ -314,9 +319,9 @@ extractable_set_asset (GESExtractable * self, GESAsset * asset) if (GES_IS_SOURCE (tmp->data)) { GESTrack *track = ges_track_element_get_track (tmp->data); - if (track->type == GES_TRACK_TYPE_AUDIO) + if (track->type == GES_TRACK_TYPE_AUDIO && audio_source) ges_track_element_copy_properties (audio_source, tmp->data); - else if (track->type == GES_TRACK_TYPE_VIDEO) + else if (track->type == GES_TRACK_TYPE_VIDEO && video_source) ges_track_element_copy_properties (video_source, tmp->data); } } diff --git a/tests/check/ges/basic.c b/tests/check/ges/basic.c index 6da18bb376..039190dbc0 100644 --- a/tests/check/ges/basic.c +++ b/tests/check/ges/basic.c @@ -119,8 +119,6 @@ GST_START_TEST (test_ges_scenario) ASSERT_OBJECT_REFCOUNT (layer, "layer", 1); tmp_layer = ges_clip_get_layer (GES_CLIP (source)); fail_unless (tmp_layer == NULL); - trackelements = GES_CONTAINER_CHILDREN (source); - fail_unless (trackelements == NULL); /* No unreffing then */ gst_object_unref (source); GST_DEBUG ("Removing track from the timeline"); @@ -499,9 +497,9 @@ GST_START_TEST (test_ges_timeline_remove_track) tmp = ges_layer_get_clips (layer); assert_equals_int (g_list_length (tmp), 3); g_list_foreach (tmp, (GFunc) gst_object_unref, NULL); - g_list_free (tmp); - check_destroyed (G_OBJECT (timeline), G_OBJECT (layer), t1, t2, t3, NULL); + gst_check_objects_destroyed_on_unref (G_OBJECT (timeline), + G_OBJECT (layer), t1, t2, t3, NULL); } GST_END_TEST; diff --git a/tests/check/python/test_clip.py b/tests/check/python/test_clip.py index 9bbce02d4d..9c91a142d2 100644 --- a/tests/check/python/test_clip.py +++ b/tests/check/python/test_clip.py @@ -98,3 +98,31 @@ class TestTitleClip(unittest.TestCase): children2 = clip2.get_children(True) self.assertNotEqual(children2[0].props.priority, children2[1].props.priority) +class TestTrackElements(unittest.TestCase): + def test_add_to_layer_with_effect_remove_add(self): + timeline = GES.Timeline.new_audio_video() + self.assertEqual(len(timeline.get_tracks()), 2) + layer = timeline.append_layer() + + test_clip = GES.TestClip() + self.assertEqual(test_clip.get_children(True), []) + self.assertTrue(layer.add_clip(test_clip)) + audio_source = test_clip.find_track_element(None, GES.AudioSource) + self.assertIsNotNone(audio_source) + + self.assertTrue(test_clip.set_child_property("volume", 0.0)) + self.assertEqual(audio_source.get_child_property("volume")[1], 0.0) + + effect = GES.Effect.new("agingtv") + test_clip.add(effect) + + children = test_clip.get_children(True) + layer.remove_clip(test_clip) + self.assertEqual(test_clip.get_children(True), children) + + self.assertTrue(layer.add_clip(test_clip)) + self.assertEqual(test_clip.get_children(True), children) + + audio_source = test_clip.find_track_element(None, GES.AudioSource) + self.assertFalse(audio_source is None) + self.assertEqual(audio_source.get_child_property("volume")[1], 0.0)