uri-clip: don't assume duration needs to stay the same

ges_uri_clip_asset_get_duration does not tell us what the duration in
the timeline needs to be. Especially when we have time effects, or
effects with finite max-durations. So we should no longer expect the
duration to stay the same when replacing assets. Instead, we just check
that the new max-duration would be compatible with the current in-point
(which was not checked before), and the clip would not be totally
overlapped if its duration-limit changes.

This is based on the assumption that each source is replaced one-to-one
in its track. If a source is replaced with nothing in the same track,
this check may be a little too strong (but still mostly weaker than
before). However, problems could occur if track selection does
something unexpected, such as placing the new source in a track not
previously occupied.

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

ges/ges-clip.c

@@ -863,6 +863,55 @@ ges_clip_can_set_max_duration_of_child (GESClip * clip, GESTrackElement * child,
   return TRUE;
 }
 
+gboolean
+ges_clip_can_set_max_duration_of_all_core (GESClip * clip,
+    GstClockTime max_duration, GError ** error)
+{
+  GList *tmp;
+  GList *child_data = NULL;
+
+  for (tmp = GES_CONTAINER_CHILDREN (clip); tmp; tmp = tmp->next) {
+    GESTimelineElement *child = tmp->data;
+    DurationLimitData *data =
+        _duration_limit_data_new (GES_TRACK_ELEMENT (child));
+
+    if (_IS_CORE_CHILD (child)) {
+      /* don't check that it has an internal-source, since we are assuming
+       * we will have one if the max-duration is valid */
+      if (GES_CLOCK_TIME_IS_LESS (max_duration, child->inpoint)) {
+        GST_INFO_OBJECT (clip, "Cannot set the max-duration from %"
+            GST_TIME_FORMAT " to %" GST_TIME_FORMAT " because it would "
+            "cause the in-point of its core child %" GES_FORMAT
+            " to exceed its max-duration",
+            GST_TIME_ARGS (child->maxduration),
+            GST_TIME_ARGS (max_duration), GES_ARGS (child));
+        g_set_error (error, GES_ERROR, GES_ERROR_NOT_ENOUGH_INTERNAL_CONTENT,
+            "Cannot set the max-duration of the child \"%s\" under the "
+            "clip \"%s\" to %" GST_TIME_FORMAT " because it would be "
+            "below the in-point of %" GST_TIME_FORMAT " of the child",
+            child->name, GES_TIMELINE_ELEMENT_NAME (clip),
+            GST_TIME_ARGS (max_duration), GST_TIME_ARGS (child->inpoint));
+
+        _duration_limit_data_free (data);
+        g_list_free_full (child_data, _duration_limit_data_free);
+        return FALSE;
+      }
+      data->max_duration = max_duration;
+    }
+
+    child_data = g_list_prepend (child_data, data);
+  }
+
+  if (!_can_update_duration_limit (clip, child_data, error)) {
+    GST_INFO_OBJECT (clip, "Cannot set the max-duration of the core "
+        "children to %" GST_TIME_FORMAT " because the duration-limit "
+        "cannot be adjusted", GST_TIME_ARGS (max_duration));
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
 static void
 _child_max_duration_changed (GESContainer * container,
     GESTimelineElement * child)

ges/ges-internal.h

@@ -423,6 +423,7 @@ G_GNUC_INTERNAL void              ges_clip_set_moving_from_layer  (GESClip *clip
 G_GNUC_INTERNAL GESTrackElement*  ges_clip_create_track_element   (GESClip *clip, GESTrackType type);
 G_GNUC_INTERNAL GList*            ges_clip_create_track_elements  (GESClip *clip, GESTrackType type);
 G_GNUC_INTERNAL gboolean          ges_clip_can_set_inpoint_of_child (GESClip * clip, GESTrackElement * child, GstClockTime inpoint, GError ** error);
+G_GNUC_INTERNAL gboolean          ges_clip_can_set_max_duration_of_all_core (GESClip * clip, GstClockTime max_duration, GError ** error);
 G_GNUC_INTERNAL gboolean          ges_clip_can_set_max_duration_of_child (GESClip * clip, GESTrackElement * child, GstClockTime max_duration, GError ** error);
 G_GNUC_INTERNAL gboolean          ges_clip_can_set_active_of_child (GESClip * clip, GESTrackElement * child, gboolean active, GError ** error);
 G_GNUC_INTERNAL gboolean          ges_clip_can_set_priority_of_child (GESClip * clip, GESTrackElement * child, guint32 priority, GError ** error);

ges/ges-uri-clip.c

@@ -231,34 +231,38 @@ extractable_set_asset (GESExtractable * self, GESAsset * asset)
   GESUriClip *uriclip = GES_URI_CLIP (self);
   GESUriClipAsset *uri_clip_asset;
   GESClip *clip = GES_CLIP (self);
+  GESContainer *container = GES_CONTAINER (clip);
+  GESTimelineElement *element = GES_TIMELINE_ELEMENT (self);
   GESLayer *layer = ges_clip_get_layer (clip);
   GList *tmp, *children;
   GHashTable *source_by_track;
+  GstClockTime max_duration;
+  GESAsset *prev_asset;
 
   g_return_val_if_fail (GES_IS_URI_CLIP_ASSET (asset), FALSE);
 
   uri_clip_asset = GES_URI_CLIP_ASSET (asset);
-  if (GST_CLOCK_TIME_IS_VALID (GES_TIMELINE_ELEMENT_DURATION (self)) &&
-      ges_uri_clip_asset_get_duration (uri_clip_asset) <
-      GES_TIMELINE_ELEMENT_INPOINT (self) +
-      GES_TIMELINE_ELEMENT_DURATION (self)) {
-    GST_INFO_OBJECT (self,
-        "Can not set asset to %p as its duration is %" GST_TIME_FORMAT
-        " < to inpoint %" GST_TIME_FORMAT " + %" GST_TIME_FORMAT " = %"
-        GST_TIME_FORMAT, asset,
-        GST_TIME_ARGS (ges_uri_clip_asset_get_duration (uri_clip_asset)),
-        GST_TIME_ARGS (GES_TIMELINE_ELEMENT_INPOINT (self)),
-        GST_TIME_ARGS (GES_TIMELINE_ELEMENT_DURATION (self)),
-        GST_TIME_ARGS (GES_TIMELINE_ELEMENT_INPOINT (self) +
-            GES_TIMELINE_ELEMENT_DURATION (self)));
 
+  /* new sources elements will have their max-duration set to
+   * max_duration. Check that this is possible with the new uri
+   * NOTE: we are assuming that all the new core children will end up
+   * in the same tracks as the previous core children */
+  max_duration = ges_uri_clip_asset_get_max_duration (uri_clip_asset);
+  if (!ges_clip_can_set_max_duration_of_all_core (clip, max_duration, NULL)) {
+    GST_INFO_OBJECT (self, "Can not set asset to %p as its max-duration %"
+        GST_TIME_FORMAT " is too low", asset, GST_TIME_ARGS (max_duration));
 
     return FALSE;
   }
 
-  if (GST_CLOCK_TIME_IS_VALID (GES_TIMELINE_ELEMENT_DURATION (clip)) == FALSE)
+  if (!container->children && !GST_CLOCK_TIME_IS_VALID (element->duration)) {
-    _set_duration0 (GES_TIMELINE_ELEMENT (uriclip),
+    if (!ges_timeline_element_set_duration (element,
-        ges_uri_clip_asset_get_duration (uri_clip_asset));
+            ges_uri_clip_asset_get_duration (uri_clip_asset))) {
+      GST_ERROR_OBJECT (self, "Failed to set the duration using a new "
+          "uri asset when we have no children. This should not happen");
+      return FALSE;
+    }
+  }
 
   ges_uri_clip_set_is_image (uriclip,
       ges_uri_clip_asset_is_image (uri_clip_asset));
@@ -268,11 +272,35 @@ extractable_set_asset (GESExtractable * self, GESAsset * asset)
         ges_clip_asset_get_supported_formats (GES_CLIP_ASSET (uri_clip_asset)));
   }
 
-  GES_TIMELINE_ELEMENT (uriclip)->asset = asset;
+  prev_asset = element->asset;
+  element->asset = asset;
+
+  /* FIXME: it would be much better if we could have a way to replace
+   * each source one-to-one with a new source in the same track, e.g.
+   * a user supplied
+   * GESSource * ges_uri_clip_swap_source (
+   *   GESClip * clip, GESSource * replace, GList * new_sources,
+   *   gpointer user_data)
+   *
+   * and they select a new source from new_sources to replace @replace, or
+   * %NULL to remove it without a replacement. The default would swap
+   * one video for another video, etc.
+   *
+   * Then we could use this information with
+   * ges_clip_can_update_duration_limit, using the new max-duration and
+   * replacing each source in the same track, to test that the operation
+   * can succeed (basically extending
+   * ges_clip_can_set_max_duration_of_all_core, but with the added
+   * information that sources without a replacement will not contribute
+   * to the duration-limit, and all of the siblings in the same track will
+   * also be removed from the track).
+   *
+   * Then we can perform the replacement, whilst avoiding track-selection
+   * (similar to GESClip's _transfer_child). */
 
   source_by_track = g_hash_table_new_full (NULL, NULL,
       gst_object_unref, gst_object_unref);
-  children = ges_container_get_children (GES_CONTAINER (self), TRUE);
+  children = ges_container_get_children (container, FALSE);
 
   for (tmp = children; tmp; tmp = tmp->next) {
     GESTrackElement *child = tmp->data;
@@ -287,39 +315,51 @@ extractable_set_asset (GESExtractable * self, GESAsset * asset)
        * timeline will remove it from its track */
       /* removing the core element will also empty its non-core siblings
        * from the same track */
-      ges_container_remove (GES_CONTAINER (self), GES_TIMELINE_ELEMENT (child));
+      ges_container_remove (container, GES_TIMELINE_ELEMENT (child));
     }
   }
   g_list_free_full (children, g_object_unref);
 
   contains_core = FALSE;
 
+  /* keep alive */
+  gst_object_ref (self);
   if (layer) {
-    gst_object_ref (clip);
 
-    ges_layer_remove_clip (layer, clip);
+    res = ges_layer_remove_clip (layer, clip);
-    /* adding back to the layer will trigger the re-creation of the core
-     * children */
-    res = ges_layer_add_clip (layer, clip);
 
-    /* NOTE: assume that core children in the same tracks correspond to
+    if (res) {
-     * the same source! */
+      /* adding back to the layer will trigger the re-creation of the core
-    for (tmp = GES_CONTAINER_CHILDREN (clip); tmp; tmp = tmp->next) {
+       * children */
-      GESTrackElement *child = tmp->data;
+      res = ges_layer_add_clip (layer, clip);
-      if (ges_track_element_is_core (child)) {
-        GESTrackElement *orig_source = g_hash_table_lookup (source_by_track,
-            ges_track_element_get_track (child));
-        contains_core = TRUE;
 
-        if (orig_source) {
+      if (!res)
-          ges_track_element_copy_properties (GES_TIMELINE_ELEMENT (orig_source),
+        GST_ERROR_OBJECT (self, "Failed to add the uri clip %s back into "
-              GES_TIMELINE_ELEMENT (child));
+            "its layer. This is likely caused by track-selection for the "
-          ges_track_element_copy_bindings (orig_source, child,
+            "core sources and effects failing because the core sources "
-              GST_CLOCK_TIME_NONE);
+            "were not replaced in the same tracks", element->name);
+
+      /* NOTE: assume that core children in the same tracks correspond to
+       * the same source! */
+      for (tmp = container->children; tmp; tmp = tmp->next) {
+        GESTrackElement *child = tmp->data;
+        if (ges_track_element_is_core (child)) {
+          GESTrackElement *orig_source = g_hash_table_lookup (source_by_track,
+              ges_track_element_get_track (child));
+          contains_core = TRUE;
+
+          if (orig_source) {
+            ges_track_element_copy_properties (GES_TIMELINE_ELEMENT
+                (orig_source), GES_TIMELINE_ELEMENT (child));
+            ges_track_element_copy_bindings (orig_source, child,
+                GST_CLOCK_TIME_NONE);
+          }
         }
       }
+    } else {
+      GST_ERROR_OBJECT (self, "Failed to remove from the layer. This "
+          "should not happen");
     }
-    gst_object_unref (clip);
     gst_object_unref (layer);
   }
   g_hash_table_unref (source_by_track);
@@ -327,11 +367,18 @@ extractable_set_asset (GESExtractable * self, GESAsset * asset)
   if (res) {
     g_free (uriclip->priv->uri);
     uriclip->priv->uri = g_strdup (ges_asset_get_id (asset));
+
+    if (!contains_core) {
+      if (!ges_timeline_element_set_max_duration (element, max_duration))
+        GST_ERROR_OBJECT (self, "Failed to set the max-duration on the uri "
+            "clip when it has no children. This should not happen");
+    }
+  } else {
+    element->asset = prev_asset;
   }
 
-  if (!contains_core)
+  /* if re-adding failed, clip may be destroyed */
-    ges_timeline_element_set_max_duration (GES_TIMELINE_ELEMENT (uriclip),
+  gst_object_unref (self);
-        ges_uri_clip_asset_get_max_duration (uri_clip_asset));
 
   return res;
 }

tests/check/ges/asset.c

@@ -228,9 +228,6 @@ GST_START_TEST (test_uri_clip_change_asset)
   asset1 = GES_ASSET (ges_uri_clip_asset_request_sync (uri1, NULL));
   fail_unless_equals_int (g_list_length (GES_CONTAINER_CHILDREN (extractable)),
       2);
-  fail_if (ges_extractable_set_asset (extractable, asset1));
-  ges_timeline_element_set_duration (GES_TIMELINE_ELEMENT (extractable),
-      ges_uri_clip_asset_get_duration (GES_URI_CLIP_ASSET (asset1)));
   fail_unless (ges_extractable_set_asset (extractable, asset1));
   fail_unless_equals_int (g_list_length (GES_CONTAINER_CHILDREN (extractable)),
       1);