Created attachment 275680 [details] [review]
ges-timeline: Fix ges_timeline_get_project macro.

This macro was a little confused about its own meaning.

Created attachment 275681 [details] [review]
ges-launch: When a scenario is set, don't request triplets

Created attachment 275682 [details] [review]
ges-validate: add an add-asset action

Conflicts:
	tools/ges-validate.c

Created attachment 275683 [details] [review]
scenarios: Add a remove-asset action, and make add-asset sync.

Created attachment 275684 [details] [review]
validate: Add add-layer and remove-layer

Created attachment 275685 [details] [review]
ges-launch: Only create a layer if needed.

That way scenarios can start with an empty timeline

Created attachment 275686 [details] [review]
timeline-element: return TRUE in _set_name when both names match.

Created attachment 275687 [details] [review]
validate: add remove / add clip actions

And a helper to get a layer by priority

Review of attachment 275680 [details] [review]:

Not sure, I liked it ;P

Review of attachment 275681 [details] [review]:

OK, we should had some more testing about what the scenario is about in the future though

Review of attachment 275681 [details] [review]:

OK, we should had some more testing about what the scenario is about in the future though

Review of attachment 275682 [details] [review]:

OK

Review of attachment 275683 [details] [review]:

::: tools/ges-validate.c
@@ +148,3 @@
   }
 
+  if (type == GES_TYPE_URI_CLIP)

That should go into the previous commit

Review of attachment 275684 [details] [review]:

::: tools/ges-validate.c
@@ +173,3 @@
+  }
+
+  layer = ges_layer_new();

Please verify that that layer does not exist first

Review of attachment 275685 [details] [review]:

OK

Review of attachment 275686 [details] [review]:

OK

Review of attachment 275687 [details] [review]:

That commit name is weird

::: tools/ges-validate.c
@@ +203,3 @@
+_remove_layer (GstValidateScenario *scenario, GstValidateAction *action)
+{
+  GESTimeline *timeline = get_timeline(scenario);

Have you double checked we do not get a ref? (I have a doubt)

@@ +235,3 @@
+  gboolean res = FALSE;
+
+  gst_structure_get_int(action->structure, "layer-priority", &layer_priority);

Hrm, why don't you get the layer from the clip directly? I think it would make more sense

@@ +281,3 @@
+
+  if (!asset || error) {
+    GST_ERROR("There was an error requesting the asset with id %s and type %s", asset_id, type_string);

You could print error->message, it is often useful :)

@@ +298,3 @@
+  }
+
+  clip = ges_layer_add_asset(layer, asset, -1, 0, duration, GES_TRACK_TYPE_UNKNOWN);

duration = GST_CLOCK_TIME_NONE will do what you want.

(In reply to comment #17)
> Review of attachment 275687 [details] [review]:
> 
> That commit name is weird
> 
> ::: tools/ges-validate.c
> @@ +203,3 @@
> +_remove_layer (GstValidateScenario *scenario, GstValidateAction *action)
> +{
> +  GESTimeline *timeline = get_timeline(scenario);
> 
> Have you double checked we do not get a ref? (I have a doubt)

We do get one, as the get_property for pipeline.timeline uses set_object.
Fixing that in a subsequent commit for every action.

> 
> @@ +235,3 @@
> +  gboolean res = FALSE;
> +
> +  gst_structure_get_int(action->structure, "layer-priority", &layer_priority);
> 
> Hrm, why don't you get the layer from the clip directly? I think it would make
> more sense
> 

Yep, done

> @@ +281,3 @@
> +
> +  if (!asset || error) {
> +    GST_ERROR("There was an error requesting the asset with id %s and type
> %s", asset_id, type_string);
> 
> You could print error->message, it is often useful :)

True, done

> 
> @@ +298,3 @@
> +  }
> +
> +  clip = ges_layer_add_asset(layer, asset, -1, 0, duration,
> GES_TRACK_TYPE_UNKNOWN);
> 
> duration = GST_CLOCK_TIME_NONE will do what you want.

OK, done

(In reply to comment #14)
> Review of attachment 275684 [details] [review]:
> 
> ::: tools/ges-validate.c
> @@ +173,3 @@
> +  }
> +
> +  layer = ges_layer_new();
> 
> Please verify that that layer does not exist first

Done

(In reply to comment #13)
> Review of attachment 275683 [details] [review]:
> 
> ::: tools/ges-validate.c
> @@ +148,3 @@
>    }
> 
> +  if (type == GES_TYPE_URI_CLIP)
> 
> That should go into the previous commit

Done

Pushed there : https://github.com/MathieuDuponchelle/PitiviGes/commits/timeline_element_name

If you still see things to fix, I'll close that bug and open a new one for a new review.

Beware, in that branch, there also is a new commit, https://github.com/MathieuDuponchelle/PitiviGes/commits/timeline_element_name which depends on the "stateless" patch to be merged in validate.

Other than that I think we're good to go:D

(In reply to comment #21)
> Pushed there :
> https://github.com/MathieuDuponchelle/PitiviGes/commits/timeline_element_name
> 
> If you still see things to fix, I'll close that bug and open a new one for a
> new review.
> 
> Beware, in that branch, there also is a new commit,
> https://github.com/MathieuDuponchelle/PitiviGes/commits/timeline_element_name
> which depends on the "stateless" patch to be merged in validate.

It has been merged already.

> Other than that I think we're good to go:D

Looks like it is, go ahea :)

Comment on attachment 275681 [details] [review]
ges-launch: When a scenario is set, don't request triplets

commit 49a21044cab43043598b972b9dee200a2b51c60b

Comment on attachment 275680 [details] [review]
ges-timeline: Fix ges_timeline_get_project macro.

commit 71b9d421b8968ccab245314587aa15bffd78f208

Comment on attachment 275682 [details] [review]
ges-validate: add an add-asset action

commit 3c7b9d0ea466f6745bf501c6a55d95ae27f44cd9

Comment on attachment 275682 [details] [review]
ges-validate: add an add-asset action

ooops

Comment on attachment 275685 [details] [review]
ges-launch: Only create a layer if needed.

commit f16ad15ef359ebcec4312ef58dfad3f87502bc19

Comment on attachment 275686 [details] [review]
timeline-element: return TRUE in _set_name when both names match.

commit 12a2d96a342efa58d1ce8e943d01c51bdd6bc935

Just doing some bugzilla cleanup.

Awesome work Mathieu

All those patch are present on: http://cgit.collabora.com/git/user/tsaunier/gst-editing-service/log/?h=post_1.4 -- We should push that branch!

Luis if you wanna give more eyes to it, you are very welcomed ;)

Very busy this week but I will look at it as soon as I can :)

Review of attachment 275683 [details] [review]:

This was merged quite some time ago it seems

Review of attachment 275684 [details] [review]:

This was merged quite some time ago it seems

Review of attachment 275684 [details] [review]:

This was merged quite some time ago it seems

Review of attachment 275687 [details] [review]:

This was merged quite some time ago it seems