From 74ae0ba5dfec3e873ec6467e67b267b5a9f2de61 Mon Sep 17 00:00:00 2001 From: Henry Wilkes Date: Mon, 2 Mar 2020 12:23:07 +0000 Subject: [PATCH] container: freeze notifies during add and remove Hold the notify signals for the container and the children until after the child has been fully added or removed. After the previous commit, this was used to ensure that the notify::priority signal was sent for children of a clip *after* the child-removed signal. This stopped being the case when the code in ->child_removed was moved to ->remove_child (the latter is called before the child-removed signal is emitted, whilst the former is called afterwards). Rather than undo this move of code, which was necessary to ensure that ->add_child was always reversed, the notify::priority signal is now simply delayed until after removing the child has completed. This was done for all notify signals, as well as in the add method, to ensure consistency. This allows the test_clips.py test_signal_order_when_removing_effect to pass. Also make subclasses take a copy of the list of the children before setting the start and duration, since this can potentially re-order the children (if they have the SET_SIMPLE flag set). --- ges/ges-clip.c | 26 +++++++++++-------- ges/ges-container.c | 68 +++++++++++++++++++++++++++++++++++++++++--------- ges/ges-group.c | 23 +++++++++-------- tests/check/ges/clip.c | 4 +-- 4 files changed, 86 insertions(+), 35 deletions(-) diff --git a/ges/ges-clip.c b/ges/ges-clip.c index 2b4dff7..d57f3f5 100644 --- a/ges/ges-clip.c +++ b/ges/ges-clip.c @@ -187,20 +187,23 @@ _child_priority_changed_cb (GESTimelineElement * child, static gboolean _set_start (GESTimelineElement * element, GstClockTime start) { - GList *tmp; + GList *tmp, *children; GESContainer *container = GES_CONTAINER (element); GST_DEBUG_OBJECT (element, "Setting children start, (initiated_move: %" GST_PTR_FORMAT ")", container->initiated_move); + /* get copy of children, since GESContainer may resort the clip */ + children = ges_container_get_children (container, FALSE); container->children_control_mode = GES_CHILDREN_IGNORE_NOTIFIES; - for (tmp = container->children; tmp; tmp = g_list_next (tmp)) { + for (tmp = children; tmp; tmp = g_list_next (tmp)) { GESTimelineElement *child = (GESTimelineElement *) tmp->data; if (child != container->initiated_move) _set_start0 (GES_TIMELINE_ELEMENT (child), start); } container->children_control_mode = GES_CHILDREN_UPDATE; + g_list_free_full (children, gst_object_unref); return TRUE; } @@ -208,11 +211,12 @@ _set_start (GESTimelineElement * element, GstClockTime start) static gboolean _set_inpoint (GESTimelineElement * element, GstClockTime inpoint) { - GList *tmp; + GList *tmp, *children; GESContainer *container = GES_CONTAINER (element); + children = ges_container_get_children (container, FALSE); container->children_control_mode = GES_CHILDREN_IGNORE_NOTIFIES; - for (tmp = container->children; tmp; tmp = g_list_next (tmp)) { + for (tmp = children; tmp; tmp = g_list_next (tmp)) { GESTimelineElement *child = (GESTimelineElement *) tmp->data; /* FIXME: we should allow the inpoint to be different for children @@ -222,6 +226,7 @@ _set_inpoint (GESTimelineElement * element, GstClockTime inpoint) _set_inpoint0 (child, inpoint); } container->children_control_mode = GES_CHILDREN_UPDATE; + g_list_free_full (children, gst_object_unref); return TRUE; } @@ -229,10 +234,12 @@ _set_inpoint (GESTimelineElement * element, GstClockTime inpoint) static gboolean _set_duration (GESTimelineElement * element, GstClockTime duration) { - GList *tmp; + GList *tmp, *children; GESContainer *container = GES_CONTAINER (element); + /* get copy of children, since GESContainer may resort the clip */ + children = ges_container_get_children (container, FALSE); container->children_control_mode = GES_CHILDREN_IGNORE_NOTIFIES; for (tmp = container->children; tmp; tmp = g_list_next (tmp)) { GESTimelineElement *child = (GESTimelineElement *) tmp->data; @@ -244,6 +251,7 @@ _set_duration (GESTimelineElement * element, GstClockTime duration) } } container->children_control_mode = GES_CHILDREN_UPDATE; + g_list_free_full (children, gst_object_unref); return TRUE; } @@ -358,11 +366,11 @@ _add_child (GESContainer * container, GESTimelineElement * element) { GESClipClass *klass = GES_CLIP_GET_CLASS (GES_CLIP (container)); guint max_prio, min_prio; - GESChildrenControlMode mode = container->children_control_mode; GESClipPrivate *priv = GES_CLIP (container)->priv; g_return_val_if_fail (GES_IS_TRACK_ELEMENT (element), FALSE); + /* NOTE: notifies are currently frozen by ges_container_add */ _get_priority_range (container, &min_prio, &max_prio); if (ELEMENT_FLAG_IS_SET (element, GES_TRACK_ELEMENT_IS_CORE)) { /* NOTE: we are assuming that the core element is **our** core element @@ -376,7 +384,6 @@ _add_child (GESContainer * container, GESTimelineElement * element) /* Set the core element to have the same in-point, which we don't * apply to effects */ - container->children_control_mode = GES_CHILDREN_IGNORE_NOTIFIES; _set_inpoint0 (element, GES_TIMELINE_ELEMENT_INPOINT (container)); } else if (GES_CLIP_CLASS_CAN_ADD_EFFECTS (klass) && GES_IS_BASE_EFFECT (element)) { @@ -420,12 +427,8 @@ _add_child (GESContainer * container, GESTimelineElement * element) return FALSE; } - /* We set the timing value of the child to ours, we avoid infinite loop - * making sure the container ignore notifies from the child */ - container->children_control_mode = GES_CHILDREN_IGNORE_NOTIFIES; _set_start0 (element, GES_TIMELINE_ELEMENT_START (container)); _set_duration0 (element, GES_TIMELINE_ELEMENT_DURATION (container)); - container->children_control_mode = mode; return TRUE; } @@ -435,6 +438,7 @@ _remove_child (GESContainer * container, GESTimelineElement * element) { GESClipPrivate *priv = GES_CLIP (container)->priv; + /* NOTE: notifies are currently frozen by ges_container_add */ if (!ELEMENT_FLAG_IS_SET (element, GES_TRACK_ELEMENT_IS_CORE) && GES_IS_BASE_EFFECT (element)) { GList *tmp; diff --git a/ges/ges-container.c b/ges/ges-container.c index 41c548d..67235b7 100644 --- a/ges/ges-container.c +++ b/ges/ges-container.c @@ -737,8 +737,9 @@ gboolean ges_container_add (GESContainer * container, GESTimelineElement * child) { ChildMapping *mapping; - gboolean notify_start = FALSE; + gboolean ret = FALSE; GESContainerClass *class; + GList *current_children, *tmp; GESContainerPrivate *priv; g_return_val_if_fail (GES_IS_CONTAINER (container), FALSE); @@ -751,15 +752,22 @@ ges_container_add (GESContainer * container, GESTimelineElement * child) GST_DEBUG_OBJECT (container, "adding timeline element %" GST_PTR_FORMAT, child); - container->children_control_mode = GES_CHILDREN_IGNORE_NOTIFIES; + /* freeze all notifies */ + g_object_freeze_notify (G_OBJECT (container)); + /* copy to use at end, since container->children may have child + * added to it */ + current_children = g_list_copy_deep (container->children, + (GCopyFunc) gst_object_ref, NULL); + for (tmp = current_children; tmp; tmp = tmp->next) + g_object_freeze_notify (G_OBJECT (tmp->data)); + g_object_freeze_notify (G_OBJECT (child)); + if (class->add_child) { if (class->add_child (container, child) == FALSE) { - container->children_control_mode = GES_CHILDREN_UPDATE; GST_WARNING_OBJECT (container, "Error adding child %p", child); - return FALSE; + goto done; } } - container->children_control_mode = GES_CHILDREN_UPDATE; /* FIXME: The following code should probably be in * GESGroupClass->add_child, rather than here! A GESClip will avoid this @@ -772,7 +780,7 @@ ges_container_add (GESContainer * container, GESTimelineElement * child) g_hash_table_foreach (priv->mappings, (GHFunc) _resync_start_offsets, container); - notify_start = TRUE; + g_object_notify (G_OBJECT (container), "start"); } mapping = g_slice_new0 (ChildMapping); @@ -807,7 +815,7 @@ ges_container_add (GESContainer * container, GESTimelineElement * child) container->children = g_list_remove (container->children, child); _ges_container_sort_children (container); - return FALSE; + goto done; } _ges_container_add_child_properties (container, child); @@ -817,10 +825,20 @@ ges_container_add (GESContainer * container, GESTimelineElement * child) child); priv->adding_children = g_list_remove (priv->adding_children, child); - if (notify_start) - g_object_notify (G_OBJECT (container), "start"); + ret = TRUE; - return TRUE; +done: + /* thaw all notifies */ + /* Ignore notifies for the start and duration since the child should + * already be correctly set up */ + container->children_control_mode = GES_CHILDREN_IGNORE_NOTIFIES; + g_object_thaw_notify (G_OBJECT (container)); + for (tmp = current_children; tmp; tmp = tmp->next) + g_object_thaw_notify (G_OBJECT (tmp->data)); + g_object_thaw_notify (G_OBJECT (child)); + g_list_free_full (current_children, gst_object_unref); + container->children_control_mode = GES_CHILDREN_UPDATE; + return ret; } /** @@ -838,6 +856,8 @@ ges_container_remove (GESContainer * container, GESTimelineElement * child) { GESContainerClass *klass; GESContainerPrivate *priv; + GList *current_children, *tmp; + gboolean ret = FALSE; g_return_val_if_fail (GES_IS_CONTAINER (container), FALSE); g_return_val_if_fail (GES_IS_TIMELINE_ELEMENT (child), FALSE); @@ -853,9 +873,22 @@ ges_container_remove (GESContainer * container, GESTimelineElement * child) return FALSE; } + /* ref the container since it might be destroyed when the child is + * removed! (see GESGroup ->child_removed) */ + gst_object_ref (container); + /* freeze all notifies */ + g_object_freeze_notify (G_OBJECT (container)); + /* copy to use at end, since container->children may have child + * removed from it */ + current_children = g_list_copy_deep (container->children, + (GCopyFunc) gst_object_ref, NULL); + for (tmp = current_children; tmp; tmp = tmp->next) + g_object_freeze_notify (G_OBJECT (tmp->data)); + + if (klass->remove_child) { if (klass->remove_child (container, child) == FALSE) - return FALSE; + goto done; } container->children = g_list_remove (container->children, child); @@ -873,7 +906,18 @@ ges_container_remove (GESContainer * container, GESTimelineElement * child) } gst_object_unref (child); - return TRUE; + ret = TRUE; + +done: + /* thaw all notifies */ + g_object_thaw_notify (G_OBJECT (container)); + for (tmp = current_children; tmp; tmp = tmp->next) + g_object_thaw_notify (G_OBJECT (tmp->data)); + g_list_free_full (current_children, gst_object_unref); + + gst_object_unref (container); + + return ret; } static void diff --git a/ges/ges-group.c b/ges/ges-group.c index c96f36b..b2cf37e 100644 --- a/ges/ges-group.c +++ b/ges/ges-group.c @@ -366,12 +366,12 @@ _set_priority (GESTimelineElement * element, guint32 priority) static gboolean _set_start (GESTimelineElement * element, GstClockTime start) { - GList *tmp; + GList *tmp, *children; gint64 diff = start - _START (element); GESTimeline *timeline; GESContainer *container = GES_CONTAINER (element); GESTimelineElement *toplevel = - ges_timeline_element_get_toplevel_parent (element);; + ges_timeline_element_get_toplevel_parent (element); gst_object_unref (toplevel); if (GES_GROUP (element)->priv->setting_value == TRUE) @@ -381,11 +381,13 @@ _set_start (GESTimelineElement * element, GstClockTime start) if (ELEMENT_FLAG_IS_SET (element, GES_TIMELINE_ELEMENT_SET_SIMPLE) || ELEMENT_FLAG_IS_SET (toplevel, GES_TIMELINE_ELEMENT_SET_SIMPLE)) { + /* get copy of children, since GESContainer may resort the group */ + children = ges_container_get_children (container, FALSE); container->children_control_mode = GES_CHILDREN_IGNORE_NOTIFIES; - for (tmp = GES_CONTAINER_CHILDREN (element); tmp; tmp = tmp->next) + for (tmp = children; tmp; tmp = tmp->next) _set_start0 (tmp->data, _START (tmp->data) + diff); container->children_control_mode = GES_CHILDREN_UPDATE; - + g_list_free_full (children, gst_object_unref); return TRUE; } @@ -406,7 +408,7 @@ _set_inpoint (GESTimelineElement * element, GstClockTime inpoint) static gboolean _set_duration (GESTimelineElement * element, GstClockTime duration) { - GList *tmp; + GList *tmp, *children; GstClockTime last_child_end = 0, new_end; GESContainer *container = GES_CONTAINER (element); GESGroupPrivate *priv = GES_GROUP (element)->priv; @@ -426,8 +428,10 @@ _set_duration (GESTimelineElement * element, GstClockTime duration) gboolean expending = (_DURATION (element) < duration); new_end = _START (element) + duration; + /* get copy of children, since GESContainer may resort the group */ + children = ges_container_get_children (container, FALSE); container->children_control_mode = GES_CHILDREN_IGNORE_NOTIFIES; - for (tmp = GES_CONTAINER_CHILDREN (element); tmp; tmp = tmp->next) { + for (tmp = children; tmp; tmp = tmp->next) { GESTimelineElement *child = tmp->data; GstClockTime n_dur; @@ -438,6 +442,7 @@ _set_duration (GESTimelineElement * element, GstClockTime duration) } } container->children_control_mode = GES_CHILDREN_UPDATE; + g_list_free_full (children, gst_object_unref); } for (tmp = GES_CONTAINER_CHILDREN (element); tmp; tmp = tmp->next) { @@ -477,6 +482,7 @@ _child_added (GESContainer * group, GESTimelineElement * child) GESGroupPrivate *priv = GES_GROUP (group)->priv; GstClockTime last_child_end = 0, first_child_start = G_MAXUINT64; + /* NOTE: notifies are currently frozen by ges_container_add */ if (!GES_TIMELINE_ELEMENT_TIMELINE (group) && GES_TIMELINE_ELEMENT_TIMELINE (child)) { timeline_add_group (GES_TIMELINE_ELEMENT_TIMELINE (child), @@ -498,7 +504,6 @@ _child_added (GESContainer * group, GESTimelineElement * child) priv->setting_value = TRUE; ELEMENT_SET_FLAG (group, GES_TIMELINE_ELEMENT_SET_SIMPLE); if (first_child_start != GES_TIMELINE_ELEMENT_START (group)) { - group->children_control_mode = GES_CHILDREN_IGNORE_NOTIFIES; _set_start0 (GES_TIMELINE_ELEMENT (group), first_child_start); } @@ -509,7 +514,6 @@ _child_added (GESContainer * group, GESTimelineElement * child) ELEMENT_UNSET_FLAG (group, GES_TIMELINE_ELEMENT_SET_SIMPLE); priv->setting_value = FALSE; - group->children_control_mode = GES_CHILDREN_UPDATE; _update_our_values (GES_GROUP (group)); signals_ids_key = @@ -572,6 +576,7 @@ _child_removed (GESContainer * group, GESTimelineElement * child) guint32 new_priority = G_MAXUINT32; GESGroupPrivate *priv = GES_GROUP (group)->priv; + /* NOTE: notifies are currently frozen by ges_container_add */ _ges_container_sort_children (group); children = GES_CONTAINER_CHILDREN (group); @@ -600,9 +605,7 @@ _child_removed (GESContainer * group, GESTimelineElement * child) new_priority); first_child_start = GES_TIMELINE_ELEMENT_START (children->data); if (first_child_start > GES_TIMELINE_ELEMENT_START (group)) { - group->children_control_mode = GES_CHILDREN_IGNORE_NOTIFIES; _set_start0 (GES_TIMELINE_ELEMENT (group), first_child_start); - group->children_control_mode = GES_CHILDREN_UPDATE; } ELEMENT_UNSET_FLAG (group, GES_TIMELINE_ELEMENT_SET_SIMPLE); priv->setting_value = FALSE; diff --git a/tests/check/ges/clip.c b/tests/check/ges/clip.c index d84ba63..3f8c908 100644 --- a/tests/check/ges/clip.c +++ b/tests/check/ges/clip.c @@ -528,7 +528,7 @@ static void child_removed_cb (GESClip * clip, GESTimelineElement * effect, gboolean * called) { - ASSERT_OBJECT_REFCOUNT (effect, "Keeping alive ref + emission ref", 2); + ASSERT_OBJECT_REFCOUNT (effect, "2 keeping alive ref + emission ref", 3); *called = TRUE; } @@ -551,7 +551,7 @@ GST_START_TEST (test_clip_refcount_remove_child) ASSERT_OBJECT_REFCOUNT (effect, "1 for the container + 1 for the track", 2); fail_unless (ges_track_remove_element (track, effect)); - ASSERT_OBJECT_REFCOUNT (effect, "1 for the container + 1 for the track", 1); + ASSERT_OBJECT_REFCOUNT (effect, "1 for the container", 1); g_signal_connect (clip, "child-removed", G_CALLBACK (child_removed_cb), &called); -- 2.7.4