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

@@ -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;

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)
+  ret = TRUE;
-    g_object_notify (G_OBJECT (container), "start");
 
-  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

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;

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