[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