[Ffmpeg-devel] [PATCH] Wildcard and catalog image sequences

[Michael Niedermayer]

Hi

On Wed, Aug 30, 2006 at 11:21:53AM +0200, Michel Bardiaux wrote:
> Michael Niedermayer wrote:
> >Hi
> >
> >On Tue, Aug 29, 2006 at 04:18:35PM +0200, Michel Bardiaux wrote:
> >>First attempt, awaiting comments.
> >>
> >>Basic idea: -fimage2 supporting 2 new kinds of (input) sequences:
> >>
> >>-f image2 'dir/x*.jpg' Guess what?
> >>
> >>-f image2 @some_url where some_url designates a text file (or whatever)
> >>containing the url of each image, 1 per line.
> >>
> >>There are a few pending issues:
> >>
> >>1. In img.c (-fimage) I support only numbered. It supports only
> >>   GIF now and will be deprecated. IMHO no debate.
> >>
> >>2. Numbered sequences only for output. IMHO no debate.
> >>
> >>3. Wildcard patterns use only * and ? (no [] or {} as in most
> >>   UNIX shells). Assume /-separated path components.
> >>   Case-sensitive. Sorted case-sensitive. Using opendir.
> >>   The latter is autoconfigured, but I have no idea how to correctly 
> >>support
> >>   case-insensitivity and / in filenames, since cross-compilation has to
> >>   be supported.
> >>
> >>4. In @-sequences, I trim any \r or space at end of line. Debatable.
> >>
> >>5. No doc patch yet.
> >
> >somehow i feel that it would be better to leave the wildcard handling to 
> >the
> >shell, maybe something with a syntax looking like:
> >ffmpeg -i `echo *.jpg | tr ' ' '*'` ...
> >that way, image lists could also trivially be support by
> >ffmpeg -i `cat filelist | tr ' ' '*'` ...
> >and numbered stuff can be supported by
> >ffmpeg -i `seq -f myfile%02g -s '*' 0 12` ...
> 
> If spaces in filenames, you're screwed 

ls -b ... | tr '\n' '*'


> (and please no flames about not 
> using MS-Windows...)

this has nothing to do with windows, you can put spaces, newlines and many
other things in filenames on linux too


> 
> And anyway you still could use the shell wildcards if so desired, I 
> forgot to give real life examples. The following 4 are quivalent in my 
> test tree:

its not about being able to do it its about having code in svn which is
superfluous, one of the most important goals is to keep the code simple
not to include stuff which serves no purpose

[...]

> >> /* return -1 if no image found */
> >>-static int find_image_range(int *pfirst_index, int *plast_index,
> >>-                            const char *path)
> >>+static int find_image_number_range(int *pfirst_index, int *plast_index,
> >>+                                   const char *path)
> >> {
> >>     char buf[1024];
> >>     int range, last_index, range1, first_index;
> >> 
> >>     /* find the first image */
> >>     for(first_index = 0; first_index < 5; first_index++) {
> >>-        if (get_frame_filename(buf, sizeof(buf), path, first_index) < 0){
> >>+        if (get_frame_filename_number(buf, sizeof(buf), path, 
> >>first_index) < 0){
> >>             *pfirst_index =
> >>-            *plast_index = 1;
> >>+                *plast_index = 1;
> >>             return 0;
> >>         }
> >>         if (url_exist(buf))
> >>@@ -124,8 +129,8 @@
> >>                 range1 = 1;
> >>             else
> >>                 range1 = 2 * range;
> >>-            if (get_frame_filename(buf, sizeof(buf), path,
> >>-                                   last_index + range1) < 0)
> >>+            if (get_frame_filename_number(buf, sizeof(buf), path,
> >>+                                          last_index + range1) < 0)
> >>                 goto fail;
> >>             if (!url_exist(buf))
> >>                 break;
> >>@@ -142,14 +147,191 @@
> >>     *pfirst_index = first_index;
> >>     *plast_index = last_index;
> >>     return 0;
> >>- fail:
> >>+  fail:
> >>     return -1;
> >> }
> >
> >cosmetic
> 
> Right, and that means it should go in a different patch, will do so at 
> once. BTW you missed a similar cosmetic at the line *plast_index = 1; above.

its not only cosmetic but unacceptable, get_frame_filename() outputs a filename
no number so renaming it to get_frame_filename_number() is just wrong
and changing the indention of correctly indented lines is also not ok


> 
> >
> >
> >> 
> >>+static void free_catalog(int last_index, char*** pindex_vector)
> >>+{
> >>+    int i;
> >>+    char** index_vector = *pindex_vector;
> >>+    for(i=0;index_vector&&i<=last_index;i++)
> >>+        av_free(index_vector[i]);
> >>+    av_free(index_vector);
> >>+    *pindex_vector = NULL;
> >
> >av_freep()
> >and the index_vector variable seems unneeded
> 
> Wilco, but my code was actually correct. It seems to me you *require* 
> compact code and are ready to reject a patch simply because you find the 
> coding style too verbose for your taste!

yes, code must be simple, patches must be minimal, no superfluous changes

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is