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

Michel Bardiaux mbardiaux
Wed Aug 30 11:21:53 CEST 2006


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 (and please no flames about not 
using MS-Windows...)

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:

ffmpeg_g -f image2 -i tmp/x%06d.jpg -y tmp/y.mpg

ffmpeg_g -f image2 -i tmp/'x*.jpg' -y tmp/y2.mpg

ffmpeg_g -f image2 -i @cata.txt -y tmp/y3.mpg

ffmpeg_g -f image2 -i @- -y tmp/y3.mpg


> 
> anyway, review of your patch is below, iam unsure if i will or will not apply
> it if you fix all below but dont leave the wildcard handling to the shell
> to which it IMO belongs ... comments are of course welcome
> 
> [...]
> 
> [configure part left to maintainer]
> 
> [...]
> 
>>  /* 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.

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

> 
> 
>> +}
>>  
>> +static int amatch(const char* s, const char* p, char matchZeroOrMore, char matchOne,
>> +                  int case_sensitive)
>> +{
>> +    int c, scc;
>> +
>> +    for (;;) {
>> +        scc = *s++;
>> +        c = *p++;
>> +        if(c==matchZeroOrMore) {
>> +            if (!*p)
>> +                return (1);
> 
> the () is unneeded

<SIGH>

> 
> 
>> +            for (s--; *s; s++)
>> +                if (amatch(s, p, matchZeroOrMore, matchOne, case_sensitive))
>> +                    return (1);
>> +            return (0);
>> +        } else if(c==0) {
>> +            return (scc == 0);
>> +        } else if(c==matchOne) {
>> +            if (scc == 0)
>> +                return (0);
>> +            continue;
>> +        } else if(case_sensitive) {
>> +            if (c != scc)
>> +                return (0);
>> +            continue;
>> +        } else {
>> +            if (toupper(c) != toupper(scc))
>> +                return (0);
>> +            continue;
> 
> the 3 continues do nothing

and thus degrade readability. OK.

> 
> 
> [...]
>> +/* return -1 if no image found */
>> +static int find_image_catalog_range(int *pfirst_index, int *plast_index, char*** pindex_vector,
>> +                                    const char *path)
> 
> the name sucks, this function does more then just finding the range, it loads
> the whole thing 

Easily changed. I had used that name to have maximal symmetry between 
all 3 sequence types, as a guid for implementation.

> 
> 
>> +{
>> +    char buf[1024];
>> +    int last_index = (-1), n_total, fileopen = 0;
>> +    char** catalog = NULL;
>> +    ByteIOContext bic;
>> +
>> +    if(strcmp(path, "@-")==0) {
>> +            if(url_fopen(&bic, "pipe:", URL_RDONLY)<0)
>> +                    goto fail;
>> +    } else {
>> +            if(url_fopen(&bic, path+1, URL_RDONLY)<0)
>> +                    goto fail;
>> +    }
> 
> char *filename= strcmp(path, "@-") ? path+1 : "pipe:";
> if(url_fopen(&bic, filename, URL_RDONLY)<0)
>     goto fail;

OK but under protest.

> 
> 
>> +    fileopen = 1;
>> +
>> +    catalog = av_mallocz(sizeof(char*)*(n_total=128));
>> +
>> +    while (url_fgets(&bic, buf, sizeof(buf))) {
>> +        while (isspace(buf[strlen(buf)-1]))
>> +            buf[strlen(buf)-1]=0;
> 
> wont that fail if the string is made of only spaces?

I had to assume that the file would contain exactly one valid filename 
on every line. The loop is not to get rid of spaces, but of possible \r. 
Will change the code to do *just* that.

BTW I realize that a stricter syntax would be better, something like 
xml. But that would open 2 cans of worms: a consensus on the spec, and 
the need for an LGPL XML library.

> 
> 
> [...]
>> Index: libavformat/utils.c
>> ===================================================================
>> --- libavformat/utils.c	(revision 6123)
>> +++ libavformat/utils.c	(working copy)
> [...]
>> +int filename_wildcard_test(const char *filename)
>> +{
>> +    if(!filename)
>> +        return (-1);
>> +    else if(strchr(filename, '*') || strchr(filename, '?'))
>> +        return 0;
>> +    else
>> +        return (-1);
> 
> return filename && (strchr(filename, '*') || strchr(filename, '?'));

Definitely *NOT*. The existing code for numbered sequence, which I 
decided not to touch, used -1 for failure and 0 for OK, in both . If you 
insist on a one-liner, it should be

return (filename && (strchr(filename, '*') || strchr(filename, '?')))?0:-1;

IMNSHO the readability is not great, and the optimality unchanged.

> 
> 
>> +}
>> +
>> +int filename_catalog_test(const char *filename)
>> +{
>> +    if(!filename)
>> +        return (-1);
>> +    else if(filename[0]=='@')
>> +        return 0;
>> +    else
>> +        return (-1);
> 
> return filename && filename[0]=='@';

See above.

> 
> 
>> +}
>> +
>> +int filename_multiple_test(const char *filename)
>> +{
>> +    if (filename_number_test(filename)==0 ||
>> +        filename_wildcard_test(filename)==0 ||
>> +        filename_catalog_test(filename)==0)
>> +        return 0;
>> +    else
>> +        return (-1);
>> +}
> 
> return   filename_number_test  (filename)
>       || filename_wildcard_test(filename)
>       || filename_catalog_test (filename);

See above

> 
> [...]
>> @@ -3012,6 +3043,38 @@
>>  }
>>  
>>  /**
>> + * Returns in 'buf' the nth path in the catalog
>> + */
>> +int get_frame_filename_catalog(char *buf, int buf_size,
>> +                               char** catalog, int catsize, int number)
>> +{
>> +    size_t len;
>> +    char* p = catalog[number];
>> +    if(number >= catsize)
>> +        goto fail;
>> +    len = strlen(p)+1;
>> +    if(len>buf_size)
>> +        goto fail;
>> +    memcpy(buf, p, len);
>> +    return 0;
>> +  fail:
>> +    *buf = '\0';
>> +    return -1;
>> +}
>> +
>> +int get_frame_filename_multiple(char *buf, int buf_size,
>> +                                const char* path, char** catalog, int catsize, int number)
> 
> non static functions should have some prefix to avoid conflicts with other libs

Of course I know that! But avformat.h is lousy with functions without 
av_ prefix. Among others, get_frame_filename and filename_number_test. I 
do intend to change these, but that would be cosmetic, no?

I havent attached a modified patch since I havent done any substantial 
change yet.

Greetings,
-- 
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/




More information about the ffmpeg-devel mailing list