clip: remove children if failed to add to layer

If adding to a layer fails during ges_timeline_add_clip, any new children
that were created during this process should be removed from the clip to
put it back into its previous state.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-editing-services/-/merge_requests/169>

ges/ges-layer.c

@@ -686,15 +686,19 @@ ges_layer_is_empty (GESLayer * layer)
 gboolean
 ges_layer_add_clip (GESLayer * layer, GESClip * clip)
 {
-  GList *tmp;
+  GList *tmp, *prev_children;
   GESAsset *asset;
   GESLayerPrivate *priv;
   GESLayer *current_layer;
-  GESTimeline *timeline = GES_TIMELINE_ELEMENT_TIMELINE (clip);
+  GESTimeline *timeline;
+  GESContainer *container;
 
   g_return_val_if_fail (GES_IS_LAYER (layer), FALSE);
   g_return_val_if_fail (GES_IS_CLIP (clip), FALSE);
 
+  timeline = GES_TIMELINE_ELEMENT_TIMELINE (clip);
+  container = GES_CONTAINER (clip);
+
   GST_DEBUG_OBJECT (layer, "adding clip:%p", clip);
   gst_object_ref_sink (clip);
 
@@ -785,7 +789,18 @@ ges_layer_add_clip (GESLayer * layer, GESClip * clip)
    * invoked by ges_timeline_add_clip */
   g_signal_emit (layer, ges_layer_signals[OBJECT_ADDED], 0, clip);
 
+  prev_children = ges_container_get_children (container, FALSE);
+
   if (timeline && !ges_timeline_add_clip (timeline, clip)) {
+    /* remove any track elements that were newly created */
+    GList *new_children = ges_container_get_children (container, FALSE);
+    for (tmp = new_children; tmp; tmp = tmp->next) {
+      if (!g_list_find (prev_children, tmp->data))
+        ges_container_remove (container, tmp->data);
+    }
+    g_list_free_full (prev_children, gst_object_unref);
+    g_list_free_full (new_children, gst_object_unref);
+
     GST_WARNING_OBJECT (layer, "Could not add the clip %" GES_FORMAT
         " to the timeline %" GST_PTR_FORMAT, GES_ARGS (clip), timeline);
     /* FIXME: change emit signal to FALSE once we are able to delay the
@@ -794,7 +809,9 @@ ges_layer_add_clip (GESLayer * layer, GESClip * clip)
     return FALSE;
   }
 
-  for (tmp = GES_CONTAINER_CHILDREN (clip); tmp; tmp = tmp->next) {
+  g_list_free_full (prev_children, gst_object_unref);
+
+  for (tmp = container->children; tmp; tmp = tmp->next) {
     GESTrack *track = ges_track_element_get_track (tmp->data);
 
     if (track)

tests/check/ges/clip.c

@@ -3074,7 +3074,104 @@ GST_START_TEST (test_copy_paste_children_properties)
 
 GST_END_TEST;
 
+GST_START_TEST (test_unchanged_after_layer_add_failure)
+{
+  GList *found;
+  GESTrack *track;
+  GESTimeline *timeline;
+  GESLayer *layer;
+  GESClip *clip0, *clip1;
+  GESTimelineElement *effect, *source;
 
+  ges_init ();
+
+  timeline = ges_timeline_new ();
+  layer = ges_timeline_append_layer (timeline);
+
+  /* two video tracks */
+  track = GES_TRACK (ges_video_track_new ());
+  fail_unless (ges_timeline_add_track (timeline, track));
+
+  track = GES_TRACK (ges_video_track_new ());
+  fail_unless (ges_timeline_add_track (timeline, track));
+
+  clip0 = GES_CLIP (ges_test_clip_new ());
+  clip1 = GES_CLIP (ges_test_clip_new ());
+
+  gst_object_ref (clip0);
+  gst_object_ref (clip1);
+
+  assert_set_start (clip0, 0);
+  assert_set_duration (clip0, 10);
+  assert_set_start (clip1, 0);
+  assert_set_duration (clip1, 10);
+
+  effect = GES_TIMELINE_ELEMENT (ges_effect_new ("agingtv"));
+  _assert_add (clip1, effect);
+
+  assert_num_children (clip0, 0);
+  assert_num_children (clip1, 1);
+
+  fail_unless (ges_layer_add_clip (layer, clip0));
+
+  assert_num_children (clip0, 2);
+  assert_num_children (clip1, 1);
+
+  fail_unless (GES_CONTAINER_CHILDREN (clip1)->data == effect);
+
+  /* addition should fail since sources would fully overlap */
+  fail_if (ges_layer_add_clip (layer, clip1));
+
+  /* children should be the same */
+  assert_num_children (clip0, 2);
+  assert_num_children (clip1, 1);
+
+  fail_unless (GES_CONTAINER_CHILDREN (clip1)->data == effect);
+
+  /* should be able to add again once we have fixed the problem */
+  fail_unless (ges_layer_remove_clip (layer, clip0));
+
+  assert_num_children (clip0, 2);
+  assert_num_children (clip1, 1);
+
+  fail_unless (ges_layer_add_clip (layer, clip1));
+
+  assert_num_children (clip0, 2);
+  /* now has two sources and two effects */
+  assert_num_children (clip1, 4);
+
+  found = ges_clip_find_track_elements (clip1, NULL, GES_TRACK_TYPE_VIDEO,
+      GES_TYPE_VIDEO_SOURCE);
+  fail_unless_equals_int (g_list_length (found), 2);
+  g_list_free_full (found, gst_object_unref);
+
+  found = ges_clip_find_track_elements (clip1, NULL, GES_TRACK_TYPE_VIDEO,
+      GES_TYPE_EFFECT);
+  fail_unless_equals_int (g_list_length (found), 2);
+  g_list_free_full (found, gst_object_unref);
+
+  /* similarly cannot add clip0 back, and children should not change */
+  /* remove the extra source */
+  _assert_remove (clip0, GES_CONTAINER_CHILDREN (clip0)->data);
+  assert_num_children (clip0, 1);
+  source = GES_CONTAINER_CHILDREN (clip0)->data;
+
+  fail_if (ges_layer_add_clip (layer, clip0));
+
+  /* children should be the same */
+  assert_num_children (clip0, 1);
+  assert_num_children (clip1, 4);
+
+  fail_unless (GES_CONTAINER_CHILDREN (clip0)->data == source);
+
+  gst_object_unref (clip0);
+  gst_object_unref (clip1);
+  gst_object_unref (timeline);
+
+  gst_deinit ();
+}
+
+GST_END_TEST;
 
 static Suite *
 ges_suite (void)
@@ -3102,6 +3199,7 @@ ges_suite (void)
   tcase_add_test (tc_chain, test_children_properties_contain);
   tcase_add_test (tc_chain, test_children_properties_change);
   tcase_add_test (tc_chain, test_copy_paste_children_properties);
+  tcase_add_test (tc_chain, test_unchanged_after_layer_add_failure);
 
   return s;
 }