Review of attachment 262889 [details] [review]:

That's awesome !

I only have 2-3 little nitpicks around but it really looks nice.
A general remark would be that some tests would be very welcome :)

Can't wait to be able to use that in pitivi !

::: ges/ges-multi-file-source.c
@@ +1,2 @@
+/* GStreamer Editing Services
+ * Copyright (C) 2009 Edward Hervey <edward.hervey@collabora.co.uk>

Copyright lubosz sarnecki :)

@@ +133,3 @@
+  if (asset != NULL) {
+    stream_info = ges_uri_source_asset_get_stream_info (asset);
+    disc_caps = gst_discoverer_stream_info_get_caps (stream_info);

You need to unref them after usage.

::: ges/ges-uri-asset.c
@@ -470,3 +471,4 @@
       "extractable-type", GES_TYPE_URI_CLIP, NULL);
   discoverer = GES_URI_CLIP_ASSET_GET_CLASS (asset)->sync_discoverer;
-  info = gst_discoverer_discover_uri (discoverer, uri, &lerror);
+
+  if (g_str_has_prefix (uri, "multifile://")) {

It would be extra nice to document that new protocol somewhere :)

@@ -471,2 +472,4 @@
   discoverer = GES_URI_CLIP_ASSET_GET_CLASS (asset)->sync_discoverer;
-  info = gst_discoverer_discover_uri (discoverer, uri, &lerror);
+
+  if (g_str_has_prefix (uri, "multifile://")) {
+    first_file = g_strdup_printf (&uri[12], 1);

I know the name is just above but it might be better to strlen "multifile://" and have a define somewhere for that string so that we can change it easily, really a minor nitpick though.

@@ -472,1 +473,4 @@
-  info = gst_discoverer_discover_uri (discoverer, uri, &lerror);
+
+  if (g_str_has_prefix (uri, "multifile://")) {
+    first_file = g_strdup_printf (&uri[12], 1);
+    first_file_uri = gst_filename_to_uri (first_file, &lerror);

Shouldn't you free that ?

(In reply to comment #1)
> @@ -472,1 +473,4 @@
> -  info = gst_discoverer_discover_uri (discoverer, uri, &lerror);
> +
> +  if (g_str_has_prefix (uri, "multifile://")) {
> +    first_file = g_strdup_printf (&uri[12], 1);
> +    first_file_uri = gst_filename_to_uri (first_file, &lerror);

my nit pick would be tahat instead of using g_strdup_printf, you can simply set

  first_file = uri + 12

This way you save an allocation, still need to free the URI as Mathieu said ;-P

Review of attachment 262889 [details] [review]:

::: ges/ges-multi-file-source.c
@@ +1,3 @@
+/* GStreamer Editing Services
+ * Copyright (C) 2009 Edward Hervey <edward.hervey@collabora.co.uk>
+ *               2009 Nokia Corporation

2013

::: ges/ges-uri-asset.c
@@ +473,3 @@
+
+  if (g_str_has_prefix (uri, "multifile://")) {
+    first_file = g_strdup_printf (&uri[12], 1);

Not sure we want 1 rather than 0, and I feel we should actually give the possibility to the user to set the first index, one way or another, it might need to be in the ID itself. we might actually be able to extract the pattern from the first ID?

@Mathieu:

Where should I document this uri? In the doc of UriClipAsset?

@Nicolas:

I need g_strdup_printf for making 0001.jpg out of a strings like %04d.jpg.
I guess uri + 12 is the same as uri[12].

@Thibault:

multifilesrc has start-index and stop-index.
Where should the ability to set this be implemented?
I chose 1 because there might be a possibility that there is no frame 0 and I need the frame only for discovery. So it should be there.

(In reply to comment #4)
> @Thibault:
> 
> multifilesrc has start-index and stop-index.
> Where should the ability to set this be implemented?
> I chose 1 because there might be a possibility that there is no frame 0 and I
> need the frame only for discovery. So it should be there.

We might set the ID in the form of:

  multifile://startindex:stopindex@pattern

if no startindex:stopindex is specified then it should 0, -1.

@Mathieu:

Where should I document this uri? In the doc of UriClipAsset?

I would think there yes, seems like the place where I'd like to get this information if I were a user.

Created attachment 263263 [details] [review]
new patch after review

Created attachment 263264 [details] [review]
new patch after review 2

sorry i just saw i forgot to change stuff in uri clip

Review of attachment 263264 [details] [review]:

Apart from my last comment, I think we're good to go :)

::: ges/ges-multi-file-source.c
@@ +181,3 @@
+    stream_info = ges_uri_source_asset_get_stream_info (asset);
+    g_assert (stream_info);
+    disc_caps = gst_discoverer_stream_info_get_caps (stream_info);

You still need to unref these caps, as you copy them after, the doc of stream_info_get_caps says "unref after usage". Alternatively, you might also just do :
caps = gst_discoverer_stream_info_get_caps (stream_info); I think :)

@Mathieu:

disc_caps is gone after I unref stream_info. I get following warning when I gst_object_unref (disc_caps):

(lt-multifilesrc:15203): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed


I can't use:
caps = gst_discoverer_stream_info_get_caps (stream_info);

Because I need to write to the caps. This way they are read only. I need to copy them.

(In reply to comment #10)
> @Mathieu:
> 
> disc_caps is gone after I unref stream_info. I get following warning when I
> gst_object_unref (disc_caps):
> 

gst_caps_unref(), GstCaps are GstMinObject, not GObject ;-P

Created attachment 263320 [details] [review]
new patch with unrefd caps

Review of attachment 263320 [details] [review]:

Everything good for me

commit 46c65aaaafbe7ea96602d3a54327e5dc1ad748e8
Author: Lubosz Sarnecki <lubosz@gmail.com>
Date:   Tue Nov 12 12:13:31 2013 +0100

    ges: multifilesrc support
    
    * GESMultiFileSource class
    * multifilesrc example
    * Support multifile:// urls in uri asset
    * start/stop index modification
    * Doc
    
    https://bugzilla.gnome.org/show_bug.cgi?id=719373