ges: Rework the way we ensure core elements are not wrongly moved between clips

Instead of focusing on the instances of the clips and their children,
we relax the check to allow moving track element clip between clips
that share a common asset. This makes it as correct conceptually but
more flexible, and the code becomes simpler.
This commit is contained in:
Thibault Saunier 2020-04-07 10:47:07 -04:00 committed by Henry Wilkes
parent ef1c0f0faa
commit e41a6b6fac
8 changed files with 64 additions and 97 deletions

View file

@ -77,8 +77,9 @@ GES.TimelineElement.set_child_property = __timeline_element_set_child_property
GES.TrackElement.set_child_property = GES.TimelineElement.set_child_property
GES.Container.edit = GES.TimelineElement.edit
__prev_asset_repr = GES.Asset.__repr__
def __asset__repr__(self):
return "%s(%s)" % (super(type(self)).__repr__(), self.props.id)
return "%s(%s)" % (__prev_asset_repr(self), self.props.id)
GES.Asset.__repr__ = __asset__repr__

View file

@ -653,9 +653,9 @@ _add_child (GESContainer * container, GESTimelineElement * element)
GESClipClass *klass = GES_CLIP_GET_CLASS (GES_CLIP (container));
guint max_prio, min_prio;
GESTrack *track;
GList *creators;
GESTimeline *timeline = GES_TIMELINE_ELEMENT_TIMELINE (container);
GESClipPrivate *priv = GES_CLIP (container)->priv;
GESAsset *asset, *creator_asset;
g_return_val_if_fail (GES_IS_TRACK_ELEMENT (element), FALSE);
@ -667,12 +667,14 @@ _add_child (GESContainer * container, GESTimelineElement * element)
return FALSE;
}
creators = ges_track_element_get_creators (GES_TRACK_ELEMENT (element));
if (creators && !g_list_find (creators, container)) {
asset = ges_extractable_get_asset (GES_EXTRACTABLE (container));
creator_asset =
ges_track_element_get_creator_asset (GES_TRACK_ELEMENT (element));
if (creator_asset && asset != creator_asset) {
GST_WARNING_OBJECT (container,
"Cannot add the track element %" GES_FORMAT " because it was "
"created for a different clip %" GES_FORMAT " as its core child",
GES_ARGS (element), GES_ARGS (creators->data));
"Cannot add the track element %" GES_FORMAT " as a child "
"because it is a core element created by another clip with a "
"different asset to the current clip's asset", GES_ARGS (element));
return FALSE;
}
@ -692,7 +694,7 @@ _add_child (GESContainer * container, GESTimelineElement * element)
/* NOTE: notifies are currently frozen by ges_container_add */
_get_priority_range (container, &min_prio, &max_prio);
if (creators) {
if (creator_asset) {
/* NOTE: Core track elements that are base effects are added like any
* other core elements. In particular, they are *not* added to the
* list of added effects, so we do not increase nb_effects. */
@ -802,8 +804,6 @@ _remove_child (GESContainer * container, GESTimelineElement * element)
/* height may have changed */
_compute_height (container);
}
/* Creator is not reset so that the child can be readded to @container
* but not to any other clip by the end user */
return TRUE;
}
@ -852,6 +852,9 @@ add_clip_to_list (gpointer key, gpointer clip, GList ** list)
* will not break the track rules:
* + no more than one core child per track
* + every non-core child must be in the same track as a core child
* NOTE: Since this does not change the creator asset of the child, this
* should only be called for transferring children between clips with the
* same asset.
*/
static void
_transfer_child (GESClip * from_clip, GESClip * to_clip,
@ -868,11 +871,6 @@ _transfer_child (GESClip * from_clip, GESClip * to_clip,
ges_container_remove (GES_CONTAINER (from_clip),
GES_TIMELINE_ELEMENT (child));
if (_IS_CORE_CHILD (child)) {
ges_track_element_clear_creators (child);
ges_track_element_add_creator (child, to_clip);
}
to_clip->priv->allow_any_track = TRUE;
ges_container_add (GES_CONTAINER (to_clip), GES_TIMELINE_ELEMENT (child));
to_clip->priv->allow_any_track = FALSE;
@ -881,24 +879,6 @@ _transfer_child (GESClip * from_clip, GESClip * to_clip,
gst_object_unref (child);
}
/* make each clip's child share creators to allow their children
* to be moved between the clips */
static void
_share_creators (GList * clips)
{
GList *clip1, *clip2, *child;
for (clip1 = clips; clip1; clip1 = clip1->next) {
for (child = GES_CONTAINER_CHILDREN (clip1->data); child;
child = child->next) {
if (!_IS_CORE_CHILD (child->data))
continue;
for (clip2 = clips; clip2; clip2 = clip2->next)
ges_track_element_add_creator (child->data, clip2->data);
}
}
}
static GList *
_ungroup (GESContainer * container, gboolean recursive)
{
@ -959,7 +939,6 @@ _ungroup (GESContainer * container, gboolean recursive)
g_hash_table_foreach (_tracktype_clip, (GHFunc) add_clip_to_list, &ret);
g_hash_table_unref (_tracktype_clip);
_share_creators (ret);
return ret;
}
@ -1001,7 +980,7 @@ _group (GList * containers)
GESTimeline *timeline;
GESTrackType supported_formats;
GESLayer *layer;
GList *tmp, *tmp2, *tmpclip, *list;
GList *tmp, *tmp2, *tmpclip;
GstClockTime start, inpoint, duration;
GESTimelineElement *element;
@ -1056,9 +1035,6 @@ _group (GList * containers)
}
}
/* keep containers alive for _share_creators */
list = g_list_copy_deep (containers, (GCopyFunc) gst_object_ref, NULL);
/* And now pass all TrackElements to the first clip,
* and remove others from the layer (updating the supported formats) */
ret = containers->data;
@ -1089,9 +1065,6 @@ _group (GList * containers)
ges_clip_set_supported_formats (GES_CLIP (ret), supported_formats);
_share_creators (list);
g_list_free_full (list, gst_object_unref);
return ret;
}
@ -1137,8 +1110,8 @@ _copy_track_element_to (GESTrackElement * orig, GESClip * to_clip,
* handled */
ges_track_element_copy_bindings (orig, copy, position);
if (_IS_CORE_CHILD (orig))
ges_track_element_add_creator (copy, to_clip);
ges_track_element_set_creator_asset (copy,
ges_track_element_get_creator_asset (orig));
return copy;
}
@ -1451,6 +1424,7 @@ ges_clip_create_track_elements (GESClip * clip, GESTrackType type)
GList *tmp, *ret;
GESClipClass *klass;
gboolean already_created = FALSE;
GESAsset *asset;
g_return_val_if_fail (GES_IS_CLIP (clip), NULL);
@ -1481,8 +1455,9 @@ ges_clip_create_track_elements (GESClip * clip, GESTrackType type)
return NULL;
ret = klass->create_track_elements (clip, type);
asset = ges_extractable_get_asset (GES_EXTRACTABLE (clip));
for (tmp = ret; tmp; tmp = tmp->next)
ges_track_element_add_creator (tmp->data, clip);
ges_track_element_set_creator_asset (tmp->data, asset);
return ret;
}

View file

@ -845,8 +845,6 @@ ges_container_add (GESContainer * container, GESTimelineElement * child)
child);
priv->adding_children = g_list_remove (priv->adding_children, child);
ret = TRUE;
done:
/* thaw all notifies */
/* Ignore notifies for the start and duration since the child should
@ -857,6 +855,7 @@ done:
g_object_thaw_notify (G_OBJECT (tmp->data));
g_object_thaw_notify (G_OBJECT (child));
g_list_free_full (current_children, gst_object_unref);
ret = ! !g_list_find (container->children, child);
gst_object_unref (child);
container->children_control_mode = GES_CHILDREN_UPDATE;
return ret;

View file

@ -84,7 +84,7 @@ GstDebugCategory * _ges_debug (void);
#define GES_ARGS GES_TIMELINE_ELEMENT_ARGS
#define GES_TRACK_ELEMENT_IS_CORE(child) \
(ges_track_element_get_creators (GES_TRACK_ELEMENT (child)) != NULL)
(ges_track_element_get_creator_asset (GES_TRACK_ELEMENT (child)) != NULL)
#define SUPRESS_UNUSED_WARNING(a) (void)a
@ -439,11 +439,11 @@ G_GNUC_INTERNAL void ges_track_element_copy_bindings (GESTrackElement *element,
GESTrackElement *new_element,
guint64 position);
G_GNUC_INTERNAL void ges_track_element_add_creator (GESTrackElement * self,
GESClip * creator);
G_GNUC_INTERNAL void ges_track_element_clear_creators (GESTrackElement * self);
/* NOTE: Returned element is only valid for **pointer comparison** */
G_GNUC_INTERNAL GList * ges_track_element_get_creators (GESTrackElement * self);
G_GNUC_INTERNAL void
ges_track_element_set_creator_asset (GESTrackElement * self,
GESAsset *creator_asset);
G_GNUC_INTERNAL GESAsset *
ges_track_element_get_creator_asset (GESTrackElement * self);
G_GNUC_INTERNAL GstElement* ges_source_create_topbin(const gchar* bin_name, GstElement* sub_element, GPtrArray* elements);
G_GNUC_INTERNAL void ges_track_set_caps(GESTrack* track,

View file

@ -67,7 +67,7 @@ struct _GESTrackElementPrivate
GHashTable *bindings_hashtable; /* We need this if we want to be able to serialize
and deserialize keyframes */
GList *creators; /* the GESClips that are considered our creators */
GESAsset *creator_asset;
};
enum
@ -257,16 +257,6 @@ ges_track_element_dispose (GObject * object)
G_OBJECT_CLASS (ges_track_element_parent_class)->dispose (object);
}
static void
ges_track_element_finalize (GObject * object)
{
GESTrackElement *self = GES_TRACK_ELEMENT (object);
ges_track_element_clear_creators (self);
G_OBJECT_CLASS (ges_track_element_parent_class)->finalize (object);
}
static void
ges_track_element_set_asset (GESExtractable * extractable, GESAsset * asset)
{
@ -342,10 +332,8 @@ ges_track_element_class_init (GESTrackElementClass * klass)
object_class->get_property = ges_track_element_get_property;
object_class->set_property = ges_track_element_set_property;
object_class->dispose = ges_track_element_dispose;
object_class->finalize = ges_track_element_finalize;
object_class->constructed = ges_track_element_constructed;
/**
* GESTrackElement:active:
*
@ -1484,37 +1472,17 @@ ges_track_element_copy_properties (GESTimelineElement * element,
g_free (specs);
}
static void
creator_disposed (GESTrackElement * self, GObject * creator)
{
self->priv->creators = g_list_remove (self->priv->creators, creator);
}
void
ges_track_element_add_creator (GESTrackElement * self, GESClip * creator)
ges_track_element_set_creator_asset (GESTrackElement * self,
GESAsset * creator_asset)
{
if (!g_list_find (self->priv->creators, creator)) {
self->priv->creators = g_list_prepend (self->priv->creators, creator);
g_object_weak_ref (G_OBJECT (creator),
(GWeakNotify) creator_disposed, self);
}
self->priv->creator_asset = creator_asset;
}
void
ges_track_element_clear_creators (GESTrackElement * self)
GESAsset *
ges_track_element_get_creator_asset (GESTrackElement * self)
{
GList *tmp;
for (tmp = self->priv->creators; tmp; tmp = tmp->next)
g_object_weak_unref (tmp->data, (GWeakNotify) creator_disposed, self);
g_list_free (self->priv->creators);
self->priv->creators = NULL;
}
GList *
ges_track_element_get_creators (GESTrackElement * self)
{
return self->priv->creators;
return self->priv->creator_asset;
}
static void

View file

@ -165,6 +165,8 @@ extractable_set_asset (GESExtractable * self, GESAsset * asset)
GESVideoStandardTransitionType value;
GESTransitionClip *trans = GES_TRANSITION_CLIP (self);
const gchar *vtype = ges_asset_get_id (asset);
GESAsset *prev_asset = ges_extractable_get_asset (self);
GList *tmp;
if (!(ges_clip_get_supported_formats (GES_CLIP (self)) &
GES_TRACK_TYPE_VIDEO)) {
@ -188,6 +190,14 @@ extractable_set_asset (GESExtractable * self, GESAsset * asset)
ges_transition_clip_update_vtype_internal (GES_CLIP (self), value, FALSE);
}
if (!prev_asset)
return TRUE;
for (tmp = GES_CONTAINER_CHILDREN (self); tmp; tmp = tmp->next) {
if (ges_track_element_get_creator_asset (tmp->data) == prev_asset)
ges_track_element_set_creator_asset (tmp->data, asset);
}
return TRUE;
}

View file

@ -120,7 +120,7 @@ typedef struct
struct _GESUriSourceAssetPrivate
{
GstDiscovererStreamInfo *sinfo;
GESUriClipAsset *parent_asset;
GESUriClipAsset *creator_asset;
const gchar *uri;
};
@ -379,7 +379,7 @@ _create_uri_source_asset (GESUriClipAsset * asset,
src_priv = GES_URI_SOURCE_ASSET (src_asset)->priv;
src_priv->uri = ges_asset_get_id (GES_ASSET (asset));
src_priv->sinfo = gst_object_ref (sinfo);
src_priv->parent_asset = asset;
src_priv->creator_asset = asset;
ges_track_element_asset_set_track_type (GES_TRACK_ELEMENT_ASSET
(src_asset), type);
@ -855,7 +855,7 @@ ges_uri_source_asset_init (GESUriSourceAsset * self)
priv = self->priv = ges_uri_source_asset_get_instance_private (self);
priv->sinfo = NULL;
priv->parent_asset = NULL;
priv->creator_asset = NULL;
priv->uri = NULL;
}
@ -896,7 +896,7 @@ ges_uri_source_asset_get_filesource_asset (GESUriSourceAsset * asset)
{
g_return_val_if_fail (GES_IS_URI_SOURCE_ASSET (asset), NULL);
return asset->priv->parent_asset;
return asset->priv->creator_asset;
}
/**

View file

@ -253,6 +253,7 @@ class TestTrackElements(common.GESSimpleTimelineTest):
def test_moving_core_track_elements(self):
clip = self.append_clip()
clip1 = self.append_clip()
title_clip = self.append_clip(asset_type=GES.TitleClip)
track_element = clip.find_track_element(None, GES.VideoSource)
self.assertTrue(clip.remove(track_element))
@ -260,9 +261,22 @@ class TestTrackElements(common.GESSimpleTimelineTest):
track_element1 = clip1.find_track_element(None, GES.VideoSource)
self.assertTrue(clip1.remove(track_element1))
self.assertFalse(clip1.add(track_element))
self.assertFalse(clip.add(track_element1))
self.assertTrue(clip1.add(track_element))
self.assertIsNotNone(track_element.get_track())
# We can add another TestSource to the clip as it has the same parent
# asset
self.assertTrue(clip1.add(track_element1))
# We already have a core TrackElement for the video track, not adding
# a second one.
self.assertIsNone(track_element1.get_track())
clip1.remove(track_element)
clip1.remove(track_element1)
title = title_clip.find_track_element(None, GES.VideoSource)
self.assertTrue(title_clip.remove(title))
# But we can't add an element that has been created by a TitleClip
self.assertFalse(clip.add(title))
self.assertFalse(title_clip.add(track_element))
self.assertTrue(clip.add(track_element))
self.assertTrue(clip1.add(track_element1))