Created attachment 311998 [details] [review]
[1/4] pitivi-formatter

Created attachment 311999 [details] [review]
[2/4] timeline-element

Created attachment 312000 [details] [review]
[3/4] uri-asset

Created attachment 312001 [details] [review]
[4/4] pipeline

Review of attachment 311998 [details] [review]:

Looks good

::: ges/ges-pitivi-formatter.c
@@ -689,2 +689,2 @@
   if (priv->clips_table != NULL) {
     g_hash_table_foreach (priv->clips_table,

I wonder why priv->clips_table is not instanciate with g_hash_table_new_full and list_table_destroyer as GDestroyFunc. Not a big deal anyway :)

Review of attachment 311999 [details] [review]:

::: ges/ges-timeline-element.c
@@ -263,1 +263,3 @@
   G_OBJECT_CLASS (ges_timeline_element_parent_class)->finalize (self);
+
+  if (object_name_counts)

Looks wrong, that is a static global counter which should be kept alive for the whole duration of the process and be cleared only on ges_deinit (which I do not remember whether it exists or not :))

Review of attachment 312000 [details] [review]:

Why can't we just use priv->uri ?

Review of attachment 312001 [details] [review]:

OK

Review of attachment 312000 [details] [review]:

::: ges/ges-uri-asset.c
@@ +598,2 @@
   if (g_str_has_prefix (priv->uri, GES_MULTI_FILE_URI_PREFIX)) {
+    trackelement = GES_TRACK_ELEMENT (ges_multi_file_source_new (uri));

Up to this point, this is exactly same as left, but the different thing is whether uri is freed at line 612.
I think I cannot free priv->uri directly because it is used by the other functions.

@@ -595,3 @@
   if (g_str_has_prefix (priv->uri, GES_MULTI_FILE_URI_PREFIX)) {
-    trackelement =
-        GES_TRACK_ELEMENT (ges_multi_file_source_new (g_strdup (priv->uri)));

ges_####_file_source_new also copies string by g_strdup inside. Therefore, as a result, one more duplicated string is created and not freed.

Review of attachment 311999 [details] [review]:

::: ges/ges-timeline-element.c
@@ -263,1 +263,3 @@
   G_OBJECT_CLASS (ges_timeline_element_parent_class)->finalize (self);
+
+  if (object_name_counts)

I thought the lifecycle of object_name_counts variable should be same with timeline-lement because it is defined as static and local variable.

If the purpose of this quark container is to trace object number, how about replacing this with atomic operations like 'g_atomic_int_inc' to make code simple?

(In reply to Justin J. Kim from comment #9)
> Review of attachment 312000 [details] [review] [review]:
> 
> ::: ges/ges-uri-asset.c
> @@ +598,2 @@
>    if (g_str_has_prefix (priv->uri, GES_MULTI_FILE_URI_PREFIX)) {
> +    trackelement = GES_TRACK_ELEMENT (ges_multi_file_source_new (uri));
> 
> Up to this point, this is exactly same as left, but the different thing is
> whether uri is freed at line 612.
> I think I cannot free priv->uri directly because it is used by the other
> functions.

My mistake, thanks for the explanation.

(In reply to Justin J. Kim from comment #10)
> Review of attachment 311999 [details] [review] [review]:
> 
> ::: ges/ges-timeline-element.c
> @@ -263,1 +263,3 @@
>    G_OBJECT_CLASS (ges_timeline_element_parent_class)->finalize (self);
> +
> +  if (object_name_counts)
> 
> I thought the lifecycle of object_name_counts variable should be same with
> timeline-lement because it is defined as static and local variable.

https://phabricator.freedesktop.org/diffusion/GES/browse/master/ges/ges-timeline-element.c;0b900bddc9eb534535e57c42afd27b6fc8d97399$39

I am not sure what you mean, did I miss something? :)

> If the purpose of this quark container is to trace object number, how about
> replacing this with atomic operations like 'g_atomic_int_inc' to make code
> simple?

It is a bit more complex than that as we keep a counter for each object type (name).

Review of attachment 312000 [details] [review]:

OK then.

(In reply to Thibault Saunier from comment #11)
> (In reply to Justin J. Kim from comment #9)
> > I thought the lifecycle of object_name_counts variable should be same with
> > timeline-lement because it is defined as static and local variable.
> 
> https://phabricator.freedesktop.org/diffusion/GES/browse/master/ges/ges-
> timeline-element.c;0b900bddc9eb534535e57c42afd27b6fc8d97399$39
> 
> I am not sure what you mean, did I miss something? :)
> 
> It is a bit more complex than that as we keep a counter for each object type
> (name).

Ah. I might choose wrong English word to explain "local". :)
What I want to say is that object_name_count variable is global variable, but it is created only by timeline-element. Therefore, I was thinking creator should call destroyer also.

OR, how about to make/define the APIs of this quark container as library? instead of using it only inside of timeline-element.

I mean, 
rather than calling like this, 
https://phabricator.freedesktop.org/diffusion/GES/browse/master/ges/ges-timeline-element.c;0b900bddc9eb534535e57c42afd27b6fc8d97399$435

using another function like, ges_quark_next_count (q); 


It could be off-topic though, can I use arcanist for my proposed patch?
phabricator looks like more simple than bugzilla to submit patch. :)

(In reply to Justin J. Kim from comment #13)
> (In reply to Thibault Saunier from comment #11)
> > (In reply to Justin J. Kim from comment #9)
> > > I thought the lifecycle of object_name_counts variable should be same with
> > > timeline-lement because it is defined as static and local variable.
> > 
> > https://phabricator.freedesktop.org/diffusion/GES/browse/master/ges/ges-
> > timeline-element.c;0b900bddc9eb534535e57c42afd27b6fc8d97399$39
> > 
> > I am not sure what you mean, did I miss something? :)
> > 
> > It is a bit more complex than that as we keep a counter for each object type
> > (name).
> 
> 
> I mean, 
> rather than calling like this, 
> https://phabricator.freedesktop.org/diffusion/GES/browse/master/ges/ges-
> timeline-element.c;0b900bddc9eb534535e57c42afd27b6fc8d97399$435
> 
> using another function like, ges_quark_next_count (q); 

Sure, would be cleaner (though that naming does not sound right :))

> It could be off-topic though, can I use arcanist for my proposed patch?
> phabricator looks like more simple than bugzilla to submit patch. :)

For GES I have no problem but you should not use it for anything else (the migration is not done yet, there is nothing official yet, and I am more or less the only one using it right now)