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).

diff --git a/ges/ges-clip.c b/ges/ges-clip.c
index 2b4dff72126f73c66364284d360d7fed978f5881..d57f3f5b1186e1cff49516265baaa03b313d0c79 100644 (file)
--- 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 41c548db3af92302e6824293bb7c34f092de1014..67235b71832d66b4e2edc2a8ccb0ab8198483720 100644 (file)
--- 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 c96f36b3d840b01b035f7bc7ee2433f6b041e79a..b2cf37e6acbe885f4d7d3cede3a4b7732524a181 100644 (file)
--- 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 d84ba63a2037e20d73332201cec61e006e242133..3f8c908b7c554c89187eed6e7c04bfef72f9fc74 100644 (file)
--- 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);