ges-source-clip: fixed return of duration setter

In general, brought the behaviour of the `start`, `duration` and
`inpoint` setters in line with each other. In particular:
1. fixed return value the GESSourceClip `duration` setter
2. changed the GESClip `start` setter
3. fixed the inpoint callback for GESContainer
4. changed the type of `res` in GESTimelineElement to be gint to
   emphasise that the GES library is using the hack that a return of -1
   from klass->set_duration means no notify signal should be sent out.

Also added a new test for clips to ensure that the setters work for
clips within and outside of timelines, and that the `start`, `inpoint`
and `duration` of a clip will match its children.

diff --git a/ges/ges-clip.c b/ges/ges-clip.c
index edbf05986aa34963612448c24da06a7125f4faa4..49a11da9caf6dd1ab486a2956c05208505566ba6 100644 (file)
--- a/ges/ges-clip.c
+++ b/ges/ges-clip.c
@@ -139,17 +139,16 @@ _set_start (GESTimelineElement * element, GstClockTime start)
   GST_DEBUG_OBJECT (element, "Setting children start, (initiated_move: %"
       GST_PTR_FORMAT ")", container->initiated_move);
 
-  element->start = start;
-  g_object_notify (G_OBJECT (element), "start");
   container->children_control_mode = GES_CHILDREN_IGNORE_NOTIFIES;
   for (tmp = container->children; tmp; tmp = g_list_next (tmp)) {
     GESTimelineElement *child = (GESTimelineElement *) tmp->data;
 
-    _set_start0 (GES_TIMELINE_ELEMENT (child), start);
+    if (child != container->initiated_move)
+      _set_start0 (GES_TIMELINE_ELEMENT (child), start);
   }
   container->children_control_mode = GES_CHILDREN_UPDATE;
 
-  return -1;
+  return TRUE;
 }
 
 static gboolean
@@ -162,9 +161,8 @@ _set_inpoint (GESTimelineElement * element, GstClockTime inpoint)
   for (tmp = container->children; tmp; tmp = g_list_next (tmp)) {
     GESTimelineElement *child = (GESTimelineElement *) tmp->data;
 
-    if (child != container->initiated_move) {
+    if (child != container->initiated_move)
       _set_inpoint0 (child, inpoint);
-    }
   }
   container->children_control_mode = GES_CHILDREN_UPDATE;
 

diff --git a/ges/ges-container.c b/ges/ges-container.c
index d3bc624bf1d99c644ad3f7dee0e9ad490701d1e7..bf7e8c7cfb21a79e351b80115c97f3ce4625f62a 100644 (file)
--- a/ges/ges-container.c
+++ b/ges/ges-container.c
@@ -592,7 +592,7 @@ _child_inpoint_changed_cb (GESTimelineElement * child,
 
   if (container->children_control_mode == GES_CHILDREN_UPDATE_OFFSETS
       || ELEMENT_FLAG_IS_SET (child, GES_TIMELINE_ELEMENT_SET_SIMPLE)) {
-    map->inpoint_offset = _START (container) - _START (child);
+    map->inpoint_offset = _INPOINT (container) - _INPOINT (child);
 
     return;
   }

diff --git a/ges/ges-source-clip.c b/ges/ges-source-clip.c
index 46de6c5c7412387c9e4e337402f8e84d30a7b871..7f5903c14dcaf4bd77bdda8d98276d84dcd8bb97 100644 (file)
--- a/ges/ges-source-clip.c
+++ b/ges/ges-source-clip.c
@@ -77,9 +77,11 @@ _set_duration (GESTimelineElement * element, GstClockTime duration)
   if (element->timeline
       && !ELEMENT_FLAG_IS_SET (element, GES_TIMELINE_ELEMENT_SET_SIMPLE)
       && !ELEMENT_FLAG_IS_SET (toplevel, GES_TIMELINE_ELEMENT_SET_SIMPLE)) {
-    return !timeline_trim_object (element->timeline, element,
-        GES_TIMELINE_ELEMENT_LAYER_PRIORITY (element), NULL, GES_EDGE_END,
-        element->start + duration);
+    if (!timeline_trim_object (element->timeline, element,
+            GES_TIMELINE_ELEMENT_LAYER_PRIORITY (element), NULL, GES_EDGE_END,
+            element->start + duration))
+      return FALSE;
+    return -1;
   }
 
   return

diff --git a/ges/ges-timeline-element.c b/ges/ges-timeline-element.c
index 2a6cbb201a1a4fb28d8b76e07823fcc013f21439..26c087399d94d7e77e35170550d340035ac988d8 100644 (file)
--- a/ges/ges-timeline-element.c
+++ b/ges/ges-timeline-element.c
@@ -835,7 +835,7 @@ ges_timeline_element_set_inpoint (GESTimelineElement * self,
   klass = GES_TIMELINE_ELEMENT_GET_CLASS (self);
 
   if (klass->set_inpoint) {
-    gboolean res = klass->set_inpoint (self, inpoint);
+    gint res = klass->set_inpoint (self, inpoint);
     if (res == TRUE) {
       self->inpoint = inpoint;
       g_object_notify_by_pspec (G_OBJECT (self), properties[PROP_INPOINT]);
@@ -913,7 +913,7 @@ ges_timeline_element_set_duration (GESTimelineElement * self,
       GST_TIME_ARGS (duration));
 
   if (klass->set_duration) {
-    gboolean res = klass->set_duration (self, duration);
+    gint res = klass->set_duration (self, duration);
     if (res == TRUE) {
       self->duration = duration;
       g_object_notify_by_pspec (G_OBJECT (self), properties[PROP_DURATION]);

diff --git a/tests/check/ges/clip.c b/tests/check/ges/clip.c
index 072084093ac86f7377d26c85c897f8ccd6d5b738..238671ddb2b290438fd06cc6b93a1a1ebcfc8166 100644 (file)
--- a/tests/check/ges/clip.c
+++ b/tests/check/ges/clip.c
@@ -22,6 +22,15 @@
 #include <ges/ges.h>
 #include <gst/check/gstcheck.h>
 
+void count_cb (GObject * obj, GParamSpec * pspec, gint * count);
+void test_children_time_val (GESClip * clip, const gchar * prop,
+    GstClockTime val);
+void test_children_time_setter (GESClip * clip, GESTimelineElement * child,
+    const gchar * prop, gboolean (*setter) (GESTimelineElement *, GstClockTime),
+    GstClockTime val1, GstClockTime val2);
+void test_children_time_setting_on_clip (GESClip * clip,
+    GESTimelineElement * child);
+
 GST_START_TEST (test_object_properties)
 {
   GESClip *clip;
@@ -729,6 +738,120 @@ GST_START_TEST (test_effects_priorities)
 
 GST_END_TEST;
 
+void
+count_cb (GObject * obj, GParamSpec * pspec, gint * count)
+{
+  *count = *count + 1;
+}
+
+void
+test_children_time_val (GESClip * clip, const gchar * prop, GstClockTime val)
+{
+  GList *tmp;
+  GstClockTime read_val;
+
+  g_object_get (clip, prop, &read_val, NULL);
+  assert_equals_uint64 (read_val, val);
+  for (tmp = GES_CONTAINER_CHILDREN (clip); tmp != NULL; tmp = tmp->next) {
+    g_object_get (tmp->data, prop, &read_val, NULL);
+    assert_equals_uint64 (read_val, val);
+  }
+}
+
+void
+test_children_time_setter (GESClip * clip, GESTimelineElement * child,
+    const gchar * prop, gboolean (*setter) (GESTimelineElement *, GstClockTime),
+    GstClockTime val1, GstClockTime val2)
+{
+  gint clip_count = 0;
+  gint child_count = 0;
+  gchar *notify_name = g_strconcat ("notify::", prop, NULL);
+
+  g_signal_connect (clip, notify_name, G_CALLBACK (count_cb), &clip_count);
+  g_signal_connect (child, notify_name, G_CALLBACK (count_cb), &child_count);
+  fail_unless (setter (GES_TIMELINE_ELEMENT (clip), val1));
+  test_children_time_val (clip, prop, val1);
+  assert_equals_int (clip_count, 1);
+  assert_equals_int (child_count, 1);
+  clip_count = 0;
+  child_count = 0;
+  fail_unless (setter (child, val2));
+  test_children_time_val (clip, prop, val2);
+  assert_equals_int (clip_count, 1);
+  assert_equals_int (child_count, 1);
+  assert_equals_int (g_signal_handlers_disconnect_by_func (clip,
+          G_CALLBACK (count_cb), &clip_count), 1);
+  assert_equals_int (g_signal_handlers_disconnect_by_func (child,
+          G_CALLBACK (count_cb), &child_count), 1);
+  g_free (notify_name);
+}
+
+void
+test_children_time_setting_on_clip (GESClip * clip, GESTimelineElement * child)
+{
+  /* FIXME: Don't necessarily want to change the inpoint of all the
+   * children if the clip inpoint changes. Really, we would only expect
+   * the inpoint to change for the source elements within a clip.
+   * Setting the inpoint of an operation may be irrelevant, and for
+   * operations where it *is* relevant, we would ideally want it to be
+   * independent from the source element's inpoint (unlike the start and
+   * duration values).
+   * However, this is the current behaviour, but if this is changed this
+   * test should be changed to only check that source elements have
+   * their inpoint changed, and operation elements have their inpoint
+   * unchanged */
+  test_children_time_setter (clip, child, "in-point",
+      ges_timeline_element_set_inpoint, 23, 52);
+  test_children_time_setter (clip, child, "start",
+      ges_timeline_element_set_start, 43, 72);
+  test_children_time_setter (clip, child, "duration",
+      ges_timeline_element_set_duration, 53, 12);
+}
+
+GST_START_TEST (test_children_time_setters)
+{
+  GESTimeline *timeline;
+  GESTrack *audio_track, *video_track;
+  GESLayer *layer;
+
+  GESTimelineElement *effect;
+  GESClip *clips[] = {
+    GES_CLIP (ges_transition_clip_new (GES_VIDEO_STANDARD_TRANSITION_TYPE_CROSSFADE)),  /* operation clip */
+    GES_CLIP (ges_test_clip_new ()),    /* source clip */
+  };
+  gint i;
+
+  ges_init ();
+
+  audio_track = GES_TRACK (ges_audio_track_new ());
+  video_track = GES_TRACK (ges_video_track_new ());
+
+  timeline = ges_timeline_new ();
+  fail_unless (ges_timeline_add_track (timeline, audio_track));
+  fail_unless (ges_timeline_add_track (timeline, video_track));
+
+  layer = ges_timeline_append_layer (timeline);
+
+  effect = GES_TIMELINE_ELEMENT (ges_effect_new ("agingtv"));
+
+  for (i = 0; i < G_N_ELEMENTS (clips); i++) {
+    GESClip *clip = clips[i];
+    fail_unless (ges_container_add (GES_CONTAINER (clip), effect));
+    test_children_time_setting_on_clip (clip, effect);
+    fail_unless (ges_layer_add_clip (layer, clip));
+    test_children_time_setting_on_clip (clip, effect);
+    fail_unless (ges_container_remove (GES_CONTAINER (clip), effect));
+    fail_unless (ges_layer_remove_clip (layer, clip));
+  }
+  gst_object_unref (effect);
+  gst_object_unref (timeline);
+
+  ges_deinit ();
+}
+
+GST_END_TEST;
+
+
 static Suite *
 ges_suite (void)
 {
@@ -745,6 +868,7 @@ ges_suite (void)
   tcase_add_test (tc_chain, test_clip_refcount_remove_child);
   tcase_add_test (tc_chain, test_clip_find_track_element);
   tcase_add_test (tc_chain, test_effects_priorities);
+  tcase_add_test (tc_chain, test_children_time_setters);
 
   return s;
 }