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

[Michel Bardiaux]

Michael Niedermayer wrote:
> 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' '*'

Well, yes, you can fix anything with a long enough pipe. That does not 
necessarily mean its not wothwhile implementing the functionality in a 
more compat way.

> 
> 
>> (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

True, but its uncommon. The most famous example in MSW is "c:\Program 
Files"...

> 
> 
>> 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

Numbered sequences are in the library, so they can be used from client 
applications, not necessarily from the command line in the shell. IMO if 
the wildcard sequences are unacceptable, then the numbered ones should 
disappear too (for input at least). Is that what you're proposing, to 
replace numbered by *-delimited?

And what if the command line becomes overly long for shells not as 
generouslt parametrized as in Linux?

> 
> [...]
> 
>>>> /* 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

get_frame_filename_numbered OK?

> and changing the indention of correctly indented lines is also not ok

This is now in another thread.

> 
> 
>>>
>>>> +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
> 
> [...]


-- 
Michel Bardiaux
R&D Director
T +32 [0] 2 790 29 41
F +32 [0] 2 790 29 02
E mailto:mbardiaux at mediaxim.be

Mediaxim NV/SA
Vorstlaan 191 Boulevard du Souverain
Brussel 1160 Bruxelles
http://www.mediaxim.com/