[patch] glob matching for image series
ffmpeg can take a sequentially numbered series of image files and make them into a video. I like to datestamp my images "20120106_134500.jpg" or similar. I used the "glob" library to allow things like: ffmpeg -i \*.jpg ... It picks things up in ascii-betical order and is much simpler than writing scripts to link or rename all my images. I think it's a good idea and people will find this to be natural and useful behavior. I posted copy of my repo to: http://bolson.org/~bolson/ffmpeg.git/ My changes are in a branch "bolson-wordexp" (slightly misnamed...) I can make a patch file if you have a preferred way of dealing with git. I tried to do this a year ago but used the "wordexp" library, but was blocked because wordexp can run shell commands passed in as an argument, creating a potential security hole if someone managed to make a web page pass arguments all the way in to "-i ...*.jpg" of ffmpeg. I backed off to the lesser "glob" library that just does pattern matching on file names. My change includes an update to the configure script, a documentation change, and the functional change to the code that reads image sequences. Brian Olson http://bolson.org/
L'octidi 18 nivôse, an CCXX, Brian Olson a écrit :
I posted copy of my repo to: http://bolson.org/~bolson/ffmpeg.git/
My changes are in a branch "bolson-wordexp" (slightly misnamed...)
I can make a patch file if you have a preferred way of dealing with git.
The preferred way for external contributors (and most internal ones too, in fact) is to use git send-email, or the equivalent using git format-patch. You will probably need to squash your patches together into a single one. The patch then gets reviewed inline.
I tried to do this a year ago but used the "wordexp" library, but was blocked because wordexp can run shell commands passed in as an argument, creating a potential security hole if someone managed to make a web page pass arguments all the way in to "-i ...*.jpg" of ffmpeg. I backed off to the lesser "glob" library that just does pattern matching on file names.
My change includes an update to the configure script, a documentation change, and the functional change to the code that reads image sequences.
That sounds quite thorough, I hope we can make it work. Regards, -- Nicolas George
To make a singular net diff, here's the output of `git diff master mybranch` (also attached if that's easier to parse): diff --git a/configure b/configure index 43b63bb..47146d8 100755 --- a/configure +++ b/configure @@ -1179,6 +1179,7 @@ HAVE_LIST=" GetProcessMemoryInfo GetProcessTimes getrusage + glob gnu_as ibm_asm inet_aton @@ -3021,6 +3022,7 @@ check_func_headers windows.h GetProcessAffinityMask check_func_headers windows.h GetProcessTimes check_func_headers windows.h MapViewOfFile check_func_headers windows.h VirtualAlloc +check_func_headers glob.h glob check_header dlfcn.h check_header dxva2api.h -D_WIN32_WINNT=0x0600 diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi index 83a45a8..ded3098 100644 --- a/doc/ffmpeg.texi +++ b/doc/ffmpeg.texi @@ -1226,7 +1226,11 @@ ffmpeg -f image2 -i foo-%03d.jpeg -r 12 -s WxH foo.avi The syntax @code{foo-%03d.jpeg} specifies to use a decimal number composed of three digits padded with zeroes to express the sequence number. It is the same syntax supported by the C printf function, but -only formats accepting a normal integer are suitable. +only formats accepting a normal integer are suitable. When importing +an image sequence, -i also accepts shell-like wildcard patterns such as +@code{foo-*.jpeg}, @code{foo-???.jpeg} or @code{foo-00[234]*.jpeg}. +It will probably be necessary to escape these patterns so they do not +get interpreted by your shell. @item You can put many streams of the same type in the output: diff --git a/libavformat/img2.c b/libavformat/img2.c index bc35591..7f94a1f 100644 --- a/libavformat/img2.c +++ b/libavformat/img2.c @@ -29,6 +29,9 @@ #include "avformat.h" #include "avio_internal.h" #include "internal.h" +#if HAVE_GLOB +#include <glob.h> +#endif typedef struct { const AVClass *class; /**< Class for private options. */ @@ -44,6 +47,10 @@ typedef struct { char *framerate; /**< Set by a private option. */ int loop; int updatefirst; +#if HAVE_GLOB + int use_glob; + glob_t globstate; +#endif } VideoData; typedef struct { @@ -133,6 +140,38 @@ static enum CodecID av_str2id(const IdStrMap *tags, const char *str) return CODEC_ID_NONE; } +#if HAVE_GLOB +static int glob_errfunc(const char* epath, int glob_error) { + return 0; +} +static int is_glob(const char* path) { + while (*path != '\0') { + switch (*path) { + case '*': + case '?': + case '[': + case ']': + case '{': + case '}': + case '\\': + return 1; + } + path++; + } + return 0; +} +static int use_glob(VideoData *s) { + return s->use_glob; +} +#else +static int is_glob(const char* path) { + return 0; +} +static int use_glob(VideoData *s) { + return 0; +} +#endif + /* return -1 if no image found */ static int find_image_range(int *pfirst_index, int *plast_index, const char *path) @@ -192,6 +231,8 @@ static int read_probe(AVProbeData *p) if (p->filename && av_str2id(img_tags, p->filename)) { if (av_filename_number_test(p->filename)) return AVPROBE_SCORE_MAX; + else if (is_glob(p->filename)) + return AVPROBE_SCORE_MAX; else return AVPROBE_SCORE_MAX/2; } @@ -263,10 +304,42 @@ static int read_header(AVFormatContext *s1, AVFormatParameters *ap) } if (!s->is_pipe) { - if (find_image_range(&first_index, &last_index, s->path) < 0) - return AVERROR(ENOENT); - s->img_first = first_index; - s->img_last = last_index; + if (is_glob(s->path)) { +#if HAVE_GLOB + int gerr; + s->use_glob = 1; +/* Locally define as 0 (bitwise-OR no-op) any missing glob options that + are non-posix glibc/bsd extensions. */ +#ifndef GLOB_NOMAGIC +#define GLOB_NOMAGIC 0 +#endif +#ifndef GLOB_TILDE +#define GLOB_TILDE 0 +#endif +#ifndef GLOB_TILDE_CHECK +#define GLOB_TILDE_CHECK GLOB_TILDE +#endif +#ifndef GLOB_BRACE +#define GLOB_BRACE 0 +#endif + gerr = glob( + s->path, + GLOB_NOCHECK|GLOB_BRACE|GLOB_NOMAGIC|GLOB_TILDE_CHECK, + glob_errfunc, + &s->globstate); + if (gerr != 0) { + return AVERROR(ENOENT); + } + s->img_first = 0; + s->img_last = s->globstate.gl_pathc - 1; +#endif + } else { + if (find_image_range(&first_index, &last_index, s->path) < 0) { + return AVERROR(ENOENT); + } + s->img_first = first_index; + s->img_last = last_index; + } s->img_number = first_index; /* compute duration */ st->start_time = 0; @@ -296,7 +369,8 @@ static int read_header(AVFormatContext *s1, AVFormatParameters *ap) static int read_packet(AVFormatContext *s1, AVPacket *pkt) { VideoData *s = s1->priv_data; - char filename[1024]; + char filename_bytes[1024]; + char* filename = filename_bytes; int i; int size[3]={0}, ret[3]={0}; AVIOContext *f[3]; @@ -309,9 +383,17 @@ static int read_packet(AVFormatContext *s1, AVPacket *pkt) } if (s->img_number > s->img_last) return AVERROR_EOF; - if (av_get_frame_filename(filename, sizeof(filename), - s->path, s->img_number)<0 && s->img_number > 1) - return AVERROR(EIO); + if (use_glob(s)) { +#if HAVE_GLOB + filename = s->globstate.gl_pathv[s->img_number]; +#endif + } else { + if (av_get_frame_filename( + filename_bytes, sizeof(filename_bytes), + s->path, s->img_number)<0 && + s->img_number > 1) + return AVERROR(EIO); + } for(i=0; i<3; i++){ if (avio_open2(&f[i], filename, AVIO_FLAG_READ, &s1->interrupt_callback, NULL) < 0) { @@ -361,6 +443,16 @@ static int read_packet(AVFormatContext *s1, AVPacket *pkt) } } +static int read_close(struct AVFormatContext* s1) { + VideoData *s = s1->priv_data; +#if HAVE_GLOB + if (s->use_glob) { + globfree(&s->globstate); + } +#endif + return 0; +} + #if CONFIG_IMAGE2_MUXER || CONFIG_IMAGE2PIPE_MUXER /******************************************************/ /* image output */ @@ -494,6 +586,7 @@ AVInputFormat ff_image2_demuxer = { .read_probe = read_probe, .read_header = read_header, .read_packet = read_packet, + .read_close = read_close, .flags = AVFMT_NOFILE, .priv_class = &img2_class, }; On Jan 8, 2012, at 12:50 PM, Nicolas George wrote:
The preferred way for external contributors (and most internal ones too, in fact) is to use git send-email, or the equivalent using git format-patch. You will probably need to squash your patches together into a single one. The patch then gets reviewed inline.
On Sun, Jan 08, 2012 at 08:17:06PM -0500, Brian Olson wrote:
To make a singular net diff, here's the output of `git diff master mybranch` (also attached if that's easier to parse):
Please do not gzip it; "git format-patch master" from mybranch should generate a proper patch (assuming master is up-to-date and mybranch correctly rebased with "git rebase master").
diff --git a/configure b/configure index 43b63bb..47146d8 100755 --- a/configure +++ b/configure @@ -1179,6 +1179,7 @@ HAVE_LIST=" GetProcessMemoryInfo GetProcessTimes getrusage + glob
Isn't your mailer breaking the indent?
gnu_as ibm_asm inet_aton @@ -3021,6 +3022,7 @@ check_func_headers windows.h GetProcessAffinityMask check_func_headers windows.h GetProcessTimes check_func_headers windows.h MapViewOfFile check_func_headers windows.h VirtualAlloc +check_func_headers glob.h glob
check_header dlfcn.h check_header dxva2api.h -D_WIN32_WINNT=0x0600 diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi index 83a45a8..ded3098 100644 --- a/doc/ffmpeg.texi +++ b/doc/ffmpeg.texi @@ -1226,7 +1226,11 @@ ffmpeg -f image2 -i foo-%03d.jpeg -r 12 -s WxH foo.avi The syntax @code{foo-%03d.jpeg} specifies to use a decimal number composed of three digits padded with zeroes to express the sequence number. It is the same syntax supported by the C printf function, but -only formats accepting a normal integer are suitable. +only formats accepting a normal integer are suitable. When importing +an image sequence, -i also accepts shell-like wildcard patterns such as +@code{foo-*.jpeg}, @code{foo-???.jpeg} or @code{foo-00[234]*.jpeg}. +It will probably be necessary to escape these patterns so they do not +get interpreted by your shell.
@item You can put many streams of the same type in the output: diff --git a/libavformat/img2.c b/libavformat/img2.c index bc35591..7f94a1f 100644 --- a/libavformat/img2.c +++ b/libavformat/img2.c @@ -29,6 +29,9 @@ #include "avformat.h" #include "avio_internal.h" #include "internal.h" +#if HAVE_GLOB +#include <glob.h> +#endif
typedef struct { const AVClass *class; /**< Class for private options. */ @@ -44,6 +47,10 @@ typedef struct { char *framerate; /**< Set by a private option. */ int loop; int updatefirst; +#if HAVE_GLOB + int use_glob; + glob_t globstate; +#endif } VideoData;
typedef struct { @@ -133,6 +140,38 @@ static enum CodecID av_str2id(const IdStrMap *tags, const char *str) return CODEC_ID_NONE; }
+#if HAVE_GLOB +static int glob_errfunc(const char* epath, int glob_error) { + return 0; +}
tab are not allowed in FFmpeg; a hook will reject the patch. Also a few hint about the coding style: - "\n" before the '{' for a function - stick the '*' to the variable name More insight in doc/developer.texi, Coding Rules section.
+static int is_glob(const char* path) { + while (*path != '\0') { + switch (*path) { + case '*': + case '?': + case '[': + case ']': + case '{': + case '}': + case '\\': + return 1; + } + path++; + } + return 0; +}
strchr?
+static int use_glob(VideoData *s) { + return s->use_glob; +} +#else +static int is_glob(const char* path) { + return 0; +} +static int use_glob(VideoData *s) { + return 0; +} +#endif +
Please add some spacing between the functions.
/* return -1 if no image found */ static int find_image_range(int *pfirst_index, int *plast_index, const char *path) @@ -192,6 +231,8 @@ static int read_probe(AVProbeData *p) if (p->filename && av_str2id(img_tags, p->filename)) { if (av_filename_number_test(p->filename)) return AVPROBE_SCORE_MAX; + else if (is_glob(p->filename)) + return AVPROBE_SCORE_MAX; else return AVPROBE_SCORE_MAX/2; } @@ -263,10 +304,42 @@ static int read_header(AVFormatContext *s1, AVFormatParameters *ap) }
if (!s->is_pipe) { - if (find_image_range(&first_index, &last_index, s->path) < 0) - return AVERROR(ENOENT); - s->img_first = first_index; - s->img_last = last_index; + if (is_glob(s->path)) { +#if HAVE_GLOB + int gerr; + s->use_glob = 1; +/* Locally define as 0 (bitwise-OR no-op) any missing glob options that + are non-posix glibc/bsd extensions. */ +#ifndef GLOB_NOMAGIC +#define GLOB_NOMAGIC 0 +#endif +#ifndef GLOB_TILDE +#define GLOB_TILDE 0 +#endif +#ifndef GLOB_TILDE_CHECK +#define GLOB_TILDE_CHECK GLOB_TILDE +#endif +#ifndef GLOB_BRACE +#define GLOB_BRACE 0 +#endif + gerr = glob( + s->path, + GLOB_NOCHECK|GLOB_BRACE|GLOB_NOMAGIC|GLOB_TILDE_CHECK, + glob_errfunc, + &s->globstate);
Please do not break the line like this; also that part has some tabs too.
+ if (gerr != 0) { + return AVERROR(ENOENT); + } + s->img_first = 0; + s->img_last = s->globstate.gl_pathc - 1; +#endif + } else { + if (find_image_range(&first_index, &last_index, s->path) < 0) { + return AVERROR(ENOENT); + } + s->img_first = first_index; + s->img_last = last_index; + } s->img_number = first_index; /* compute duration */ st->start_time = 0; @@ -296,7 +369,8 @@ static int read_header(AVFormatContext *s1, AVFormatParameters *ap) static int read_packet(AVFormatContext *s1, AVPacket *pkt) { VideoData *s = s1->priv_data; - char filename[1024]; + char filename_bytes[1024]; + char* filename = filename_bytes;
char *filename
int i; int size[3]={0}, ret[3]={0}; AVIOContext *f[3]; @@ -309,9 +383,17 @@ static int read_packet(AVFormatContext *s1, AVPacket *pkt) } if (s->img_number > s->img_last) return AVERROR_EOF; - if (av_get_frame_filename(filename, sizeof(filename), - s->path, s->img_number)<0 && s->img_number > 1) - return AVERROR(EIO); + if (use_glob(s)) { +#if HAVE_GLOB + filename = s->globstate.gl_pathv[s->img_number]; +#endif + } else { + if (av_get_frame_filename( + filename_bytes, sizeof(filename_bytes), + s->path, s->img_number)<0 && + s->img_number > 1) + return AVERROR(EIO); + } for(i=0; i<3; i++){ if (avio_open2(&f[i], filename, AVIO_FLAG_READ, &s1->interrupt_callback, NULL) < 0) { @@ -361,6 +443,16 @@ static int read_packet(AVFormatContext *s1, AVPacket *pkt) } }
+static int read_close(struct AVFormatContext* s1) { + VideoData *s = s1->priv_data; +#if HAVE_GLOB + if (s->use_glob) { + globfree(&s->globstate); + } +#endif + return 0; +} + #if CONFIG_IMAGE2_MUXER || CONFIG_IMAGE2PIPE_MUXER /******************************************************/ /* image output */ @@ -494,6 +586,7 @@ AVInputFormat ff_image2_demuxer = { .read_probe = read_probe, .read_header = read_header, .read_packet = read_packet, + .read_close = read_close, .flags = AVFMT_NOFILE, .priv_class = &img2_class, };
On Jan 8, 2012, at 12:50 PM, Nicolas George wrote:
The preferred way for external contributors (and most internal ones too, in fact) is to use git send-email, or the equivalent using git format-patch. You will probably need to squash your patches together into a single one. The patch then gets reviewed inline.
Also, please do not top post. -- Clément B.
Le nonidi 19 nivôse, an CCXX, Brian Olson a écrit :
To make a singular net diff, here's the output of `git diff master mybranch` (also attached if that's easier to parse):
Your mailer mangled the diff. Maybe you should find a more tech-friendly one. The attachment was fine, but do not gzip it, it makes it harder to quote it, and the compression is completely lost by the base64 encoding.
On Jan 8, 2012, at 12:50 PM, Nicolas George wrote:
Please do not top-post.
diff --git a/configure b/configure index 43b63bb..47146d8 100755 --- a/configure +++ b/configure @@ -1179,6 +1179,7 @@ HAVE_LIST=" GetProcessMemoryInfo GetProcessTimes getrusage + glob gnu_as ibm_asm inet_aton @@ -3021,6 +3022,7 @@ check_func_headers windows.h GetProcessAffinityMask check_func_headers windows.h GetProcessTimes check_func_headers windows.h MapViewOfFile check_func_headers windows.h VirtualAlloc +check_func_headers glob.h glob
check_header dlfcn.h check_header dxva2api.h -D_WIN32_WINNT=0x0600 diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi index 83a45a8..ded3098 100644 --- a/doc/ffmpeg.texi +++ b/doc/ffmpeg.texi @@ -1226,7 +1226,11 @@ ffmpeg -f image2 -i foo-%03d.jpeg -r 12 -s WxH foo.avi The syntax @code{foo-%03d.jpeg} specifies to use a decimal number composed of three digits padded with zeroes to express the sequence number. It is the same syntax supported by the C printf function, but -only formats accepting a normal integer are suitable. +only formats accepting a normal integer are suitable. When importing +an image sequence, -i also accepts shell-like wildcard patterns such as +@code{foo-*.jpeg}, @code{foo-???.jpeg} or @code{foo-00[234]*.jpeg}. +It will probably be necessary to escape these patterns so they do not +get interpreted by your shell.
@item You can put many streams of the same type in the output: diff --git a/libavformat/img2.c b/libavformat/img2.c index bc35591..7f94a1f 100644 --- a/libavformat/img2.c +++ b/libavformat/img2.c @@ -29,6 +29,9 @@ #include "avformat.h" #include "avio_internal.h" #include "internal.h" +#if HAVE_GLOB +#include <glob.h> +#endif
typedef struct { const AVClass *class; /**< Class for private options. */ @@ -44,6 +47,10 @@ typedef struct { char *framerate; /**< Set by a private option. */ int loop; int updatefirst; +#if HAVE_GLOB + int use_glob; + glob_t globstate; +#endif } VideoData;
typedef struct { @@ -133,6 +140,38 @@ static enum CodecID av_str2id(const IdStrMap *tags, const char *str) return CODEC_ID_NONE; }
+#if HAVE_GLOB +static int glob_errfunc(const char* epath, int glob_error) {
Please use "char *name", not "char* name". And the brace on the next line.
+ return 0; +} +static int is_glob(const char* path) { + while (*path != '\0') { + switch (*path) { + case '*': + case '?': + case '[': + case ']': + case '{': + case '}': + case '\\': + return 1; + } + path++; + }
I believe strcspn could trivialize that.
+ return 0; +} +static int use_glob(VideoData *s) { + return s->use_glob; +} +#else +static int is_glob(const char* path) { + return 0; +} +static int use_glob(VideoData *s) { + return 0; +} +#endif
I believe it would be slightly simpler if the use_glob field were unconditional and accessed directly.
+ /* return -1 if no image found */ static int find_image_range(int *pfirst_index, int *plast_index, const char *path) @@ -192,6 +231,8 @@ static int read_probe(AVProbeData *p) if (p->filename && av_str2id(img_tags, p->filename)) { if (av_filename_number_test(p->filename)) return AVPROBE_SCORE_MAX; + else if (is_glob(p->filename)) + return AVPROBE_SCORE_MAX; else return AVPROBE_SCORE_MAX/2; } @@ -263,10 +304,42 @@ static int read_header(AVFormatContext *s1, AVFormatParameters *ap) }
if (!s->is_pipe) { - if (find_image_range(&first_index, &last_index, s->path) < 0) - return AVERROR(ENOENT); - s->img_first = first_index; - s->img_last = last_index; + if (is_glob(s->path)) { +#if HAVE_GLOB + int gerr; + s->use_glob = 1; +/* Locally define as 0 (bitwise-OR no-op) any missing glob options that + are non-posix glibc/bsd extensions. */ +#ifndef GLOB_NOMAGIC +#define GLOB_NOMAGIC 0 +#endif +#ifndef GLOB_TILDE +#define GLOB_TILDE 0 +#endif +#ifndef GLOB_TILDE_CHECK +#define GLOB_TILDE_CHECK GLOB_TILDE +#endif +#ifndef GLOB_BRACE +#define GLOB_BRACE 0 +#endif
This bunch of defines breaks the reading order: maybe put them somewhere else, at the top with the rest of the ifdefry.
+ gerr = glob( + s->path, + GLOB_NOCHECK|GLOB_BRACE|GLOB_NOMAGIC|GLOB_TILDE_CHECK, + glob_errfunc, + &s->globstate);
The indentation is strange, and there are tabs.
+ if (gerr != 0) {
if (gerr)
+ return AVERROR(ENOENT);
Please match gerr against the GLOB_* error codes.
+ } + s->img_first = 0; + s->img_last = s->globstate.gl_pathc - 1; +#endif + } else { + if (find_image_range(&first_index, &last_index, s->path) < 0) { + return AVERROR(ENOENT); + } + s->img_first = first_index; + s->img_last = last_index;
Do not reindent this part, it will make it easier to see what you actually change.
+ } s->img_number = first_index;
Unless I am mistake, first_index can be unitialized.
/* compute duration */ st->start_time = 0; @@ -296,7 +369,8 @@ static int read_header(AVFormatContext *s1, AVFormatParameters *ap) static int read_packet(AVFormatContext *s1, AVPacket *pkt) { VideoData *s = s1->priv_data; - char filename[1024]; + char filename_bytes[1024];
filename_buf seems more natural.
+ char* filename = filename_bytes; int i; int size[3]={0}, ret[3]={0}; AVIOContext *f[3]; @@ -309,9 +383,17 @@ static int read_packet(AVFormatContext *s1, AVPacket *pkt) } if (s->img_number > s->img_last) return AVERROR_EOF; - if (av_get_frame_filename(filename, sizeof(filename), - s->path, s->img_number)<0 && s->img_number > 1) - return AVERROR(EIO); + if (use_glob(s)) { +#if HAVE_GLOB + filename = s->globstate.gl_pathv[s->img_number]; +#endif + } else { + if (av_get_frame_filename( + filename_bytes, sizeof(filename_bytes), + s->path, s->img_number)<0 && + s->img_number > 1) + return AVERROR(EIO); + }
Again, do not reindent existing code: leave it for a later patch.
for(i=0; i<3; i++){ if (avio_open2(&f[i], filename, AVIO_FLAG_READ, &s1->interrupt_callback, NULL) < 0) { @@ -361,6 +443,16 @@ static int read_packet(AVFormatContext *s1, AVPacket *pkt) } }
+static int read_close(struct AVFormatContext* s1) { + VideoData *s = s1->priv_data; +#if HAVE_GLOB + if (s->use_glob) { + globfree(&s->globstate); + }
You can omit the braces in such simple cases.
+#endif + return 0; +} + #if CONFIG_IMAGE2_MUXER || CONFIG_IMAGE2PIPE_MUXER /******************************************************/ /* image output */ @@ -494,6 +586,7 @@ AVInputFormat ff_image2_demuxer = { .read_probe = read_probe, .read_header = read_header, .read_packet = read_packet, + .read_close = read_close, .flags = AVFMT_NOFILE, .priv_class = &img2_class, };
On the whole, it looks promising. Regards, -- Nicolas George
@@ -296,7 +369,8 @@ static int read_header(AVFormatContext *s1, AVFormatParameters *ap) static int read_packet(AVFormatContext *s1, AVPacket *pkt) { VideoData *s = s1->priv_data; - char filename[1024]; + char filename_bytes[1024];
filename_buf seems more natural.
+ char* filename = filename_bytes;
Renaming the array seems unrelated, why not simply call the new pointer "filename_buf"? -Thilo
On Jan 9, 2012, at 7:00 AM, Nicolas George wrote:
On the whole, it looks promising.
Thanks. Sorry for the delay, got busy with other things. Style cleaned up based on feedback. Is there a copy of the presubmit script that I can run locally to check modified code? Simplified is_glob() using strcspn() (cool, hadn't seen that part of the C strings standard library before, thanks Nicolas) Fixed usage of first_index and last_index so that they shouldn't be used uninitted. Hopefully the patch attachment will be cleaner this time...
On Jan 20, 2012, at 4:35 PM, Brian Olson wrote:
Thanks. Sorry for the delay, got busy with other things.
Style cleaned up based on feedback. Is there a copy of the presubmit script that I can run locally to check modified code?
Simplified is_glob() using strcspn() (cool, hadn't seen that part of the C strings standard library before, thanks Nicolas)
Fixed usage of first_index and last_index so that they shouldn't be used uninitted.
Hopefully the patch attachment will be cleaner this time...
Trying again to attach
On Fri, 20 Jan 2012 17:04:49 -0500, Brian Olson wrote:
On Jan 20, 2012, at 4:35 PM, Brian Olson wrote:
Thanks. Sorry for the delay, got busy with other things.
Style cleaned up based on feedback. Is there a copy of the presubmit script that I can run locally to check modified code?
Simplified is_glob() using strcspn() (cool, hadn't seen that part of the C strings standard library before, thanks Nicolas)
Fixed usage of first_index and last_index so that they shouldn't be used uninitted.
Hopefully the patch attachment will be cleaner this time...
Trying again to attach
nothing was attached. if you are using git, you can use git-send-email to send the patch to the list. -compn
On Jan 20, 2012, at 4:35 PM, Brian Olson wrote:
Thanks. Sorry for the delay, got busy with other things.
Style cleaned up based on feedback. Is there a copy of the presubmit script that I can run locally to check modified code?
Simplified is_glob() using strcspn() (cool, hadn't seen that part of the C strings standard library before, thanks Nicolas)
Fixed usage of first_index and last_index so that they shouldn't be used uninitted.
Hopefully the patch attachment will be cleaner this time...
This is weird. I didn't have this trouble last time, and I don't have this trouble sending to people, so I think the list is filtering me out. Last time I pasted the diff in, but it didn't display monospace and so was ugly. And I attached a gzip of the diff, and that got through, but people didn't like that for other reasons. (Sending with MacOS X mail) Brian Olson http://bolson.org/
On 1/20/12, Brian Olson <icic@bolson.org> wrote:
On Jan 20, 2012, at 4:35 PM, Brian Olson wrote:
Thanks. Sorry for the delay, got busy with other things.
Style cleaned up based on feedback. Is there a copy of the presubmit script that I can run locally to check modified code?
Simplified is_glob() using strcspn() (cool, hadn't seen that part of the C strings standard library before, thanks Nicolas)
Fixed usage of first_index and last_index so that they shouldn't be used uninitted.
Hopefully the patch attachment will be cleaner this time...
This is weird. I didn't have this trouble last time, and I don't have this trouble sending to people, so I think the list is filtering me out. Last time I pasted the diff in, but it didn't display monospace and so was ugly. And I attached a gzip of the diff, and that got through, but people didn't like that for other reasons. (Sending with MacOS X mail)
You can still post patch in plain txt. Hope that your mail client will not corrupt it somehow.
On Jan 20, 2012, at 4:35 PM, Brian Olson wrote:
Style cleaned up based on feedback. Is there a copy of the presubmit script that I can run locally to check modified code?
Simplified is_glob() using strcspn() (cool, hadn't seen that part of the C strings standard library before, thanks Nicolas)
Fixed usage of first_index and last_index so that they shouldn't be used uninitted.
diff --git a/configure b/configure index 3bace2b..474ef93 100755 --- a/configure +++ b/configure @@ -1179,6 +1179,7 @@ HAVE_LIST=" GetProcessMemoryInfo GetProcessTimes getrusage + glob gnu_as ibm_asm inet_aton @@ -3021,6 +3022,7 @@ check_func_headers windows.h GetProcessAffinityMask check_func_headers windows.h GetProcessTimes check_func_headers windows.h MapViewOfFile check_func_headers windows.h VirtualAlloc +check_func_headers glob.h glob check_header dlfcn.h check_header dxva2api.h -D_WIN32_WINNT=0x0600 diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi index e2f6add..690bd83 100644 --- a/doc/ffmpeg.texi +++ b/doc/ffmpeg.texi @@ -1226,7 +1226,11 @@ ffmpeg -f image2 -i foo-%03d.jpeg -r 12 -s WxH foo.avi The syntax @code{foo-%03d.jpeg} specifies to use a decimal number composed of three digits padded with zeroes to express the sequence number. It is the same syntax supported by the C printf function, but -only formats accepting a normal integer are suitable. +only formats accepting a normal integer are suitable. When importing +an image sequence, -i also accepts shell-like wildcard patterns such as +@code{foo-*.jpeg}, @code{foo-???.jpeg} or @code{foo-00[234]*.jpeg}. +It will probably be necessary to escape these patterns so they do not +get interpreted by your shell. @item You can put many streams of the same type in the output: diff --git a/libavformat/img2.c b/libavformat/img2.c index bc35591..17eb899 100644 --- a/libavformat/img2.c +++ b/libavformat/img2.c @@ -29,6 +29,25 @@ #include "avformat.h" #include "avio_internal.h" #include "internal.h" +#if HAVE_GLOB +#include <glob.h> + +/* Locally define as 0 (bitwise-OR no-op) any missing glob options that + are non-posix glibc/bsd extensions. */ +#ifndef GLOB_NOMAGIC +#define GLOB_NOMAGIC 0 +#endif +#ifndef GLOB_TILDE +#define GLOB_TILDE 0 +#endif +#ifndef GLOB_TILDE_CHECK +#define GLOB_TILDE_CHECK GLOB_TILDE +#endif +#ifndef GLOB_BRACE +#define GLOB_BRACE 0 +#endif + +#endif /* HAVE_GLOB */ typedef struct { const AVClass *class; /**< Class for private options. */ @@ -44,6 +63,10 @@ typedef struct { char *framerate; /**< Set by a private option. */ int loop; int updatefirst; + int use_glob; +#if HAVE_GLOB + glob_t globstate; +#endif } VideoData; typedef struct { @@ -133,6 +156,25 @@ static enum CodecID av_str2id(const IdStrMap *tags, const char *str) return CODEC_ID_NONE; } +#if HAVE_GLOB +static int glob_errfunc(const char *epath, int glob_error) +{ + return 0; +} + +static int is_glob(const char *path) +{ + size_t span = strcspn(path, "*?[]{}\\"); + /* Did we hit a glob char or get to the end? */ + return path[span] != '\0'; +} +#else +static int is_glob(const char *path) +{ + return 0; +} +#endif + /* return -1 if no image found */ static int find_image_range(int *pfirst_index, int *plast_index, const char *path) @@ -192,6 +234,8 @@ static int read_probe(AVProbeData *p) if (p->filename && av_str2id(img_tags, p->filename)) { if (av_filename_number_test(p->filename)) return AVPROBE_SCORE_MAX; + else if (is_glob(p->filename)) + return AVPROBE_SCORE_MAX; else return AVPROBE_SCORE_MAX/2; } @@ -263,8 +307,23 @@ static int read_header(AVFormatContext *s1, AVFormatParameters *ap) } if (!s->is_pipe) { - if (find_image_range(&first_index, &last_index, s->path) < 0) - return AVERROR(ENOENT); + if (is_glob(s->path)) { +#if HAVE_GLOB + int gerr; + s->use_glob = 1; + gerr = glob(s->path, GLOB_NOCHECK|GLOB_BRACE|GLOB_NOMAGIC|GLOB_TILDE_CHECK, glob_errfunc, &s->globstate); + if (gerr != 0) { + return AVERROR(ENOENT); + } + first_index = 0; + last_index = s->globstate.gl_pathc - 1; +#endif + } else { + s->use_glob = 0; + if (find_image_range(&first_index, &last_index, s->path) < 0) { + return AVERROR(ENOENT); + } + } s->img_first = first_index; s->img_last = last_index; s->img_number = first_index; @@ -296,7 +355,8 @@ static int read_header(AVFormatContext *s1, AVFormatParameters *ap) static int read_packet(AVFormatContext *s1, AVPacket *pkt) { VideoData *s = s1->priv_data; - char filename[1024]; + char filename_bytes[1024]; + char *filename = filename_bytes; int i; int size[3]={0}, ret[3]={0}; AVIOContext *f[3]; @@ -309,9 +369,17 @@ static int read_packet(AVFormatContext *s1, AVPacket *pkt) } if (s->img_number > s->img_last) return AVERROR_EOF; - if (av_get_frame_filename(filename, sizeof(filename), - s->path, s->img_number)<0 && s->img_number > 1) - return AVERROR(EIO); + if (s->use_glob) { +#if HAVE_GLOB + filename = s->globstate.gl_pathv[s->img_number]; +#endif + } else { + if (av_get_frame_filename( + filename_bytes, sizeof(filename_bytes), + s->path, s->img_number)<0 && + s->img_number > 1) + return AVERROR(EIO); + } for(i=0; i<3; i++){ if (avio_open2(&f[i], filename, AVIO_FLAG_READ, &s1->interrupt_callback, NULL) < 0) { @@ -361,6 +429,16 @@ static int read_packet(AVFormatContext *s1, AVPacket *pkt) } } +static int read_close(struct AVFormatContext* s1) { + VideoData *s = s1->priv_data; +#if HAVE_GLOB + if (s->use_glob) { + globfree(&s->globstate); + } +#endif + return 0; +} + #if CONFIG_IMAGE2_MUXER || CONFIG_IMAGE2PIPE_MUXER /******************************************************/ /* image output */ @@ -494,6 +572,7 @@ AVInputFormat ff_image2_demuxer = { .read_probe = read_probe, .read_header = read_header, .read_packet = read_packet, + .read_close = read_close, .flags = AVFMT_NOFILE, .priv_class = &img2_class, };
Le duodi 2 pluviôse, an CCXX, Brian Olson a écrit :
Simplified is_glob() using strcspn() (cool, hadn't seen that part of the C strings standard library before, thanks Nicolas)
Fixed usage of first_index and last_index so that they shouldn't be used uninitted.
Sorry to have kept you waiting, I was otherwise busy.
diff --git a/configure b/configure index 3bace2b..474ef93 100755 --- a/configure +++ b/configure @@ -1179,6 +1179,7 @@ HAVE_LIST=" GetProcessMemoryInfo GetProcessTimes getrusage + glob gnu_as ibm_asm inet_aton @@ -3021,6 +3022,7 @@ check_func_headers windows.h GetProcessAffinityMask check_func_headers windows.h GetProcessTimes check_func_headers windows.h MapViewOfFile check_func_headers windows.h VirtualAlloc +check_func_headers glob.h glob
check_header dlfcn.h check_header dxva2api.h -D_WIN32_WINNT=0x0600 diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi index e2f6add..690bd83 100644 --- a/doc/ffmpeg.texi +++ b/doc/ffmpeg.texi @@ -1226,7 +1226,11 @@ ffmpeg -f image2 -i foo-%03d.jpeg -r 12 -s WxH foo.avi The syntax @code{foo-%03d.jpeg} specifies to use a decimal number composed of three digits padded with zeroes to express the sequence number. It is the same syntax supported by the C printf function, but -only formats accepting a normal integer are suitable. +only formats accepting a normal integer are suitable. When importing +an image sequence, -i also accepts shell-like wildcard patterns such as +@code{foo-*.jpeg}, @code{foo-???.jpeg} or @code{foo-00[234]*.jpeg}. +It will probably be necessary to escape these patterns so they do not +get interpreted by your shell.
@item You can put many streams of the same type in the output: diff --git a/libavformat/img2.c b/libavformat/img2.c index bc35591..17eb899 100644 --- a/libavformat/img2.c +++ b/libavformat/img2.c @@ -29,6 +29,25 @@ #include "avformat.h" #include "avio_internal.h" #include "internal.h" +#if HAVE_GLOB +#include <glob.h> + +/* Locally define as 0 (bitwise-OR no-op) any missing glob options that + are non-posix glibc/bsd extensions. */ +#ifndef GLOB_NOMAGIC +#define GLOB_NOMAGIC 0 +#endif +#ifndef GLOB_TILDE +#define GLOB_TILDE 0 +#endif +#ifndef GLOB_TILDE_CHECK +#define GLOB_TILDE_CHECK GLOB_TILDE +#endif +#ifndef GLOB_BRACE +#define GLOB_BRACE 0 +#endif + +#endif /* HAVE_GLOB */
typedef struct { const AVClass *class; /**< Class for private options. */ @@ -44,6 +63,10 @@ typedef struct { char *framerate; /**< Set by a private option. */ int loop; int updatefirst; + int use_glob; +#if HAVE_GLOB + glob_t globstate; +#endif } VideoData;
typedef struct { @@ -133,6 +156,25 @@ static enum CodecID av_str2id(const IdStrMap *tags, const char *str) return CODEC_ID_NONE; }
+#if HAVE_GLOB +static int glob_errfunc(const char *epath, int glob_error) +{ + return 0; +}
According to Single Unix, passing NULL as errfunc would have the same effect. Do you have any reason not to use that feature?
+ +static int is_glob(const char *path) +{ + size_t span = strcspn(path, "*?[]{}\\"); + /* Did we hit a glob char or get to the end? */ + return path[span] != '\0'; +} +#else +static int is_glob(const char *path) +{ + return 0; +} +#endif + /* return -1 if no image found */ static int find_image_range(int *pfirst_index, int *plast_index, const char *path) @@ -192,6 +234,8 @@ static int read_probe(AVProbeData *p) if (p->filename && av_str2id(img_tags, p->filename)) { if (av_filename_number_test(p->filename)) return AVPROBE_SCORE_MAX; + else if (is_glob(p->filename)) + return AVPROBE_SCORE_MAX; else return AVPROBE_SCORE_MAX/2; } @@ -263,8 +307,23 @@ static int read_header(AVFormatContext *s1, AVFormatParameters *ap) }
if (!s->is_pipe) { - if (find_image_range(&first_index, &last_index, s->path) < 0) - return AVERROR(ENOENT); + if (is_glob(s->path)) { +#if HAVE_GLOB + int gerr; + s->use_glob = 1; + gerr = glob(s->path, GLOB_NOCHECK|GLOB_BRACE|GLOB_NOMAGIC|GLOB_TILDE_CHECK, glob_errfunc, &s->globstate); + if (gerr != 0) { + return AVERROR(ENOENT); + } + first_index = 0; + last_index = s->globstate.gl_pathc - 1; +#endif + } else { + s->use_glob = 0; + if (find_image_range(&first_index, &last_index, s->path) < 0) { + return AVERROR(ENOENT); + }
Unless I am mistaken, these lines are only reindented and get a useless pair of braces: could you restore them to their original state, to make the diff easier to follow? If I end up applying the patch, I would commit the reindent afterwards.
+ } s->img_first = first_index; s->img_last = last_index; s->img_number = first_index; @@ -296,7 +355,8 @@ static int read_header(AVFormatContext *s1, AVFormatParameters *ap) static int read_packet(AVFormatContext *s1, AVPacket *pkt) { VideoData *s = s1->priv_data; - char filename[1024]; + char filename_bytes[1024]; + char *filename = filename_bytes; int i; int size[3]={0}, ret[3]={0}; AVIOContext *f[3]; @@ -309,9 +369,17 @@ static int read_packet(AVFormatContext *s1, AVPacket *pkt) } if (s->img_number > s->img_last) return AVERROR_EOF; - if (av_get_frame_filename(filename, sizeof(filename), - s->path, s->img_number)<0 && s->img_number > 1) - return AVERROR(EIO); + if (s->use_glob) { +#if HAVE_GLOB + filename = s->globstate.gl_pathv[s->img_number]; +#endif + } else { + if (av_get_frame_filename( + filename_bytes, sizeof(filename_bytes), + s->path, s->img_number)<0 && + s->img_number > 1) + return AVERROR(EIO);
Same thing here: I think this line is unchanged, and I would like to be sure.
+ } for(i=0; i<3; i++){ if (avio_open2(&f[i], filename, AVIO_FLAG_READ, &s1->interrupt_callback, NULL) < 0) { @@ -361,6 +429,16 @@ static int read_packet(AVFormatContext *s1, AVPacket *pkt) } }
+static int read_close(struct AVFormatContext* s1) { + VideoData *s = s1->priv_data; +#if HAVE_GLOB + if (s->use_glob) { + globfree(&s->globstate); + } +#endif + return 0; +} + #if CONFIG_IMAGE2_MUXER || CONFIG_IMAGE2PIPE_MUXER /******************************************************/ /* image output */ @@ -494,6 +572,7 @@ AVInputFormat ff_image2_demuxer = { .read_probe = read_probe, .read_header = read_header, .read_packet = read_packet, + .read_close = read_close, .flags = AVFMT_NOFILE, .priv_class = &img2_class, };
On the whole, it looks quite good. Does anyone object to this? Regards, -- Nicolas George
Hi Nicolas, Nicolas George wrote:
Le duodi 2 pluviôse, an CCXX, Brian Olson a écrit :
Simplified is_glob() using strcspn() (cool, hadn't seen that part of the C strings standard library before, thanks Nicolas)
Fixed usage of first_index and last_index so that they shouldn't be used uninitted.
Sorry to have kept you waiting, I was otherwise busy. [...]
On the whole, it looks quite good. Does anyone object to this?
I had the idea that another solution would be making -i accept multiple arguments and leave the globbing work to the user's shell. But that would have a load of problems of its own I think. Anyway currently I tend to think that the glob() solution is also quite OK. Alexander
On Jan 29, 2012, at 4:35 PM, Alexander Strasser wrote:
On the whole, it looks quite good. Does anyone object to this?
I had the idea that another solution would be making -i accept multiple arguments and leave the globbing work to the user's shell. But that would have a load of problems of its own I think.
Anyway currently I tend to think that the glob() solution is also quite OK.
glob-ing in shell could pretty easily hit OS size limits on what can be passed through argv. A few thousand frames could do it. I think the %d approach and the glob-in-ffmpeg approach put the work in the most efficient context. Nicolas, I'll work up a less-change diff without whitespace changes. In the places where I've worked if the functional change didn't have the final formatting there would never be a change to go back and clean it up. Brian Olson http://bolson.org/
On Jan 29, 2012, at 1:37 PM, Nicolas George wrote:
+static int glob_errfunc(const char *epath, int glob_error) +{ + return 0; +}
According to Single Unix, passing NULL as errfunc would have the same effect. Do you have any reason not to use that feature?
I guess I wrote this as a place holder in case I needed to handle glob errors and turn them into specific libavformat errors. I could delete this, or leave it as an explicit placeholder and disambiguator of the NULL parameter to glob(). I think the rest of the style things are cleaned up, making a more minimal patch without whitespace changes on a few lines where I had reindented things. diff --git a/configure b/configure index 348342d..4f6788a 100755 --- a/configure +++ b/configure @@ -1184,6 +1184,7 @@ HAVE_LIST=" GetProcessMemoryInfo GetProcessTimes getrusage + glob gnu_as ibm_asm inet_aton @@ -3048,6 +3049,7 @@ check_func_headers windows.h GetProcessAffinityMask check_func_headers windows.h GetProcessTimes check_func_headers windows.h MapViewOfFile check_func_headers windows.h VirtualAlloc +check_func_headers glob.h glob check_header dlfcn.h check_header dxva2api.h -D_WIN32_WINNT=0x0600 diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi index 55748f1..2b710d2 100644 --- a/doc/ffmpeg.texi +++ b/doc/ffmpeg.texi @@ -1260,7 +1260,11 @@ ffmpeg -f image2 -i foo-%03d.jpeg -r 12 -s WxH foo.avi The syntax @code{foo-%03d.jpeg} specifies to use a decimal number composed of three digits padded with zeroes to express the sequence number. It is the same syntax supported by the C printf function, but -only formats accepting a normal integer are suitable. +only formats accepting a normal integer are suitable. When importing +an image sequence, -i also accepts shell-like wildcard patterns such as +@code{foo-*.jpeg}, @code{foo-???.jpeg} or @code{foo-00[234]*.jpeg}. +It will probably be necessary to escape these patterns so they do not +get interpreted by your shell. @item You can put many streams of the same type in the output: diff --git a/libavformat/img2.c b/libavformat/img2.c index 622d759..d0e101f 100644 --- a/libavformat/img2.c +++ b/libavformat/img2.c @@ -29,6 +29,25 @@ #include "avformat.h" #include "avio_internal.h" #include "internal.h" +#if HAVE_GLOB +#include <glob.h> + +/* Locally define as 0 (bitwise-OR no-op) any missing glob options that + are non-posix glibc/bsd extensions. */ +#ifndef GLOB_NOMAGIC +#define GLOB_NOMAGIC 0 +#endif +#ifndef GLOB_TILDE +#define GLOB_TILDE 0 +#endif +#ifndef GLOB_TILDE_CHECK +#define GLOB_TILDE_CHECK GLOB_TILDE +#endif +#ifndef GLOB_BRACE +#define GLOB_BRACE 0 +#endif + +#endif /* HAVE_GLOB */ typedef struct { const AVClass *class; /**< Class for private options. */ @@ -44,6 +63,10 @@ typedef struct { char *framerate; /**< Set by a private option. */ int loop; int updatefirst; + int use_glob; +#if HAVE_GLOB + glob_t globstate; +#endif } VideoData; typedef struct { @@ -137,6 +160,25 @@ static enum CodecID av_str2id(const IdStrMap *tags, const char *str) return CODEC_ID_NONE; } +#if HAVE_GLOB +static int glob_errfunc(const char *epath, int glob_error) +{ + return 0; +} + +static int is_glob(const char *path) +{ + size_t span = strcspn(path, "*?[]{}\\"); + /* Did we hit a glob char or get to the end? */ + return path[span] != '\0'; +} +#else +static int is_glob(const char *path) +{ + return 0; +} +#endif + /* return -1 if no image found */ static int find_image_range(int *pfirst_index, int *plast_index, const char *path) @@ -196,6 +238,8 @@ static int read_probe(AVProbeData *p) if (p->filename && av_str2id(img_tags, p->filename)) { if (av_filename_number_test(p->filename)) return AVPROBE_SCORE_MAX; + else if (is_glob(p->filename)) + return AVPROBE_SCORE_MAX; else return AVPROBE_SCORE_MAX/2; } @@ -256,8 +300,21 @@ static int read_header(AVFormatContext *s1) } if (!s->is_pipe) { + s->use_glob = is_glob(s->path); + if (s->use_glob) { +#if HAVE_GLOB + int gerr; + gerr = glob(s->path, GLOB_NOCHECK|GLOB_BRACE|GLOB_NOMAGIC|GLOB_TILDE_CHECK, glob_errfunc, &s->globstate); + if (gerr != 0) { + return AVERROR(ENOENT); + } + first_index = 0; + last_index = s->globstate.gl_pathc - 1; +#endif + } else { if (find_image_range(&first_index, &last_index, s->path) < 0) return AVERROR(ENOENT); + } s->img_first = first_index; s->img_last = last_index; s->img_number = first_index; @@ -289,7 +346,8 @@ static int read_header(AVFormatContext *s1) static int read_packet(AVFormatContext *s1, AVPacket *pkt) { VideoData *s = s1->priv_data; - char filename[1024]; + char filename_bytes[1024]; + char *filename = filename_bytes; int i; int size[3]={0}, ret[3]={0}; AVIOContext *f[3]; @@ -302,9 +360,15 @@ static int read_packet(AVFormatContext *s1, AVPacket *pkt) } if (s->img_number > s->img_last) return AVERROR_EOF; - if (av_get_frame_filename(filename, sizeof(filename), + if (s->use_glob) { +#if HAVE_GLOB + filename = s->globstate.gl_pathv[s->img_number]; +#endif + } else { + if (av_get_frame_filename(filename_bytes, sizeof(filename_bytes), s->path, s->img_number)<0 && s->img_number > 1) return AVERROR(EIO); + } for(i=0; i<3; i++){ if (avio_open2(&f[i], filename, AVIO_FLAG_READ, &s1->interrupt_callback, NULL) < 0) { @@ -354,6 +418,16 @@ static int read_packet(AVFormatContext *s1, AVPacket *pkt) } } +static int read_close(struct AVFormatContext* s1) { + VideoData *s = s1->priv_data; +#if HAVE_GLOB + if (s->use_glob) { + globfree(&s->globstate); + } +#endif + return 0; +} + #if CONFIG_IMAGE2_MUXER || CONFIG_IMAGE2PIPE_MUXER /******************************************************/ /* image output */ @@ -487,6 +561,7 @@ AVInputFormat ff_image2_demuxer = { .read_probe = read_probe, .read_header = read_header, .read_packet = read_packet, + .read_close = read_close, .flags = AVFMT_NOFILE, .priv_class = &img2_class, };
On Jan 31, 2012, at 1:52 AM, Brian Olson wrote:
On Jan 29, 2012, at 1:37 PM, Nicolas George wrote:
+static int glob_errfunc(const char *epath, int glob_error) +{ + return 0; +}
According to Single Unix, passing NULL as errfunc would have the same effect. Do you have any reason not to use that feature?
I guess I wrote this as a place holder in case I needed to handle glob errors and turn them into specific libavformat errors. I could delete this, or leave it as an explicit placeholder and disambiguator of the NULL parameter to glob().
I think the rest of the style things are cleaned up, making a more minimal patch without whitespace changes on a few lines where I had reindented things.
Cleaning up that one-last-thing, and rewarming the thread, here's a patch (against master branch from earlier today) that removes the excess glob error handler function and cleans up the #if statements around it. diff --git a/configure b/configure index 70fc016..1ead0d1 100755 --- a/configure +++ b/configure @@ -1186,6 +1186,7 @@ HAVE_LIST=" GetProcessMemoryInfo GetProcessTimes getrusage + glob gnu_as ibm_asm inet_aton @@ -3059,6 +3060,7 @@ check_func_headers windows.h GetProcessAffinityMask check_func_headers windows.h GetProcessTimes check_func_headers windows.h MapViewOfFile check_func_headers windows.h VirtualAlloc +check_func_headers glob.h glob check_header dlfcn.h check_header dxva2api.h -D_WIN32_WINNT=0x0600 diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi index 51a282e..32b8cf9 100644 --- a/doc/ffmpeg.texi +++ b/doc/ffmpeg.texi @@ -1282,7 +1282,11 @@ ffmpeg -f image2 -i foo-%03d.jpeg -r 12 -s WxH foo.avi The syntax @code{foo-%03d.jpeg} specifies to use a decimal number composed of three digits padded with zeroes to express the sequence number. It is the same syntax supported by the C printf function, but -only formats accepting a normal integer are suitable. +only formats accepting a normal integer are suitable. When importing +an image sequence, -i also accepts shell-like wildcard patterns such as +@code{foo-*.jpeg}, @code{foo-???.jpeg} or @code{foo-00[234]*.jpeg}. +It will probably be necessary to escape these patterns so they do not +get interpreted by your shell. @item You can put many streams of the same type in the output: diff --git a/libavformat/img2.c b/libavformat/img2.c index 5776c0d..740bf82 100644 --- a/libavformat/img2.c +++ b/libavformat/img2.c @@ -29,6 +29,25 @@ #include "avformat.h" #include "avio_internal.h" #include "internal.h" +#if HAVE_GLOB +#include <glob.h> + +/* Locally define as 0 (bitwise-OR no-op) any missing glob options that + are non-posix glibc/bsd extensions. */ +#ifndef GLOB_NOMAGIC +#define GLOB_NOMAGIC 0 +#endif +#ifndef GLOB_TILDE +#define GLOB_TILDE 0 +#endif +#ifndef GLOB_TILDE_CHECK +#define GLOB_TILDE_CHECK GLOB_TILDE +#endif +#ifndef GLOB_BRACE +#define GLOB_BRACE 0 +#endif + +#endif /* HAVE_GLOB */ typedef struct { const AVClass *class; /**< Class for private options. */ @@ -44,6 +63,10 @@ typedef struct { char *framerate; /**< Set by a private option. */ int loop; int updatefirst; + int use_glob; +#if HAVE_GLOB + glob_t globstate; +#endif } VideoData; typedef struct { @@ -138,6 +161,17 @@ static enum CodecID av_str2id(const IdStrMap *tags, const char *str) return CODEC_ID_NONE; } +static int is_glob(const char *path) +{ +#if HAVE_GLOB + size_t span = strcspn(path, "*?[]{}\\"); + /* Did we hit a glob char or get to the end? */ + return path[span] != '\0'; +#else + return 0; +#endif +} + /* return -1 if no image found */ static int find_image_range(int *pfirst_index, int *plast_index, const char *path) @@ -197,6 +231,8 @@ static int read_probe(AVProbeData *p) if (p->filename && av_str2id(img_tags, p->filename)) { if (av_filename_number_test(p->filename)) return AVPROBE_SCORE_MAX; + else if (is_glob(p->filename)) + return AVPROBE_SCORE_MAX; else return AVPROBE_SCORE_MAX/2; } @@ -257,8 +293,21 @@ static int read_header(AVFormatContext *s1) } if (!s->is_pipe) { + s->use_glob = is_glob(s->path); + if (s->use_glob) { +#if HAVE_GLOB + int gerr; + gerr = glob(s->path, GLOB_NOCHECK|GLOB_BRACE|GLOB_NOMAGIC|GLOB_TILDE_CHECK, NULL, &s->globstate); + if (gerr != 0) { + return AVERROR(ENOENT); + } + first_index = 0; + last_index = s->globstate.gl_pathc - 1; +#endif + } else { if (find_image_range(&first_index, &last_index, s->path) < 0) return AVERROR(ENOENT); + } s->img_first = first_index; s->img_last = last_index; s->img_number = first_index; @@ -290,7 +339,8 @@ static int read_header(AVFormatContext *s1) static int read_packet(AVFormatContext *s1, AVPacket *pkt) { VideoData *s = s1->priv_data; - char filename[1024]; + char filename_bytes[1024]; + char *filename = filename_bytes; int i; int size[3]={0}, ret[3]={0}; AVIOContext *f[3]; @@ -303,9 +353,15 @@ static int read_packet(AVFormatContext *s1, AVPacket *pkt) } if (s->img_number > s->img_last) return AVERROR_EOF; - if (av_get_frame_filename(filename, sizeof(filename), + if (s->use_glob) { +#if HAVE_GLOB + filename = s->globstate.gl_pathv[s->img_number]; +#endif + } else { + if (av_get_frame_filename(filename_bytes, sizeof(filename_bytes), s->path, s->img_number)<0 && s->img_number > 1) return AVERROR(EIO); + } for(i=0; i<3; i++){ if (avio_open2(&f[i], filename, AVIO_FLAG_READ, &s1->interrupt_callback, NULL) < 0) { @@ -355,6 +411,16 @@ static int read_packet(AVFormatContext *s1, AVPacket *pkt) } } +static int read_close(struct AVFormatContext* s1) { + VideoData *s = s1->priv_data; +#if HAVE_GLOB + if (s->use_glob) { + globfree(&s->globstate); + } +#endif + return 0; +} + #if CONFIG_IMAGE2_MUXER || CONFIG_IMAGE2PIPE_MUXER /******************************************************/ /* image output */ @@ -488,6 +554,7 @@ AVInputFormat ff_image2_demuxer = { .read_probe = read_probe, .read_header = read_header, .read_packet = read_packet, + .read_close = read_close, .flags = AVFMT_NOFILE, .priv_class = &img2_class, };
On Sun, Feb 19, 2012 at 06:06:35PM -0500, Brian Olson wrote:
On Jan 31, 2012, at 1:52 AM, Brian Olson wrote:
On Jan 29, 2012, at 1:37 PM, Nicolas George wrote:
+static int glob_errfunc(const char *epath, int glob_error) +{ + return 0; +}
According to Single Unix, passing NULL as errfunc would have the same effect. Do you have any reason not to use that feature?
I guess I wrote this as a place holder in case I needed to handle glob errors and turn them into specific libavformat errors. I could delete this, or leave it as an explicit placeholder and disambiguator of the NULL parameter to glob().
I think the rest of the style things are cleaned up, making a more minimal patch without whitespace changes on a few lines where I had reindented things.
Cleaning up that one-last-thing, and rewarming the thread, here's a patch (against master branch from earlier today) that removes the excess glob error handler function and cleans up the #if statements around it.
if there are no further comments / reviews then ill apply this in a few days thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 3 "Rare item" - "Common item with rare defect or maybe just a lie" "Professional" - "'Toy' made in china, not functional except as doorstop" "Experts will know" - "The seller hopes you are not an expert"
L'octidi 8 ventôse, an CCXX, Michael Niedermayer a écrit :
if there are no further comments / reviews then ill apply this in a few days
I re-read it, it looks ok. Unfortunately, it no longer apply cleanly since the merge of the img2dec/img2enc split. Here is a fixed version; it is in my repository, I can push it easily. Regards, -- Nicolas George
Hi, Nicolas George wrote:
L'octidi 8 ventôse, an CCXX, Michael Niedermayer a écrit :
if there are no further comments / reviews then ill apply this in a few days
I re-read it, it looks ok.
I have nothing against it. After reconsidering multiple times I withdraw my "objection" (well it was never meant as one in the first place).
Unfortunately, it no longer apply cleanly since the merge of the img2dec/img2enc split. Here is a fixed version; it is in my repository, I can push it easily.
Fine for me. I have some more thoughts on this, but as the author of the patch has gone through a rather longish review process (escpecially considering the wordexp history) I will not veto if you just go ahead with this version. After having that said, I will still express my opinions on this feature in the remaining part of this mail in the hope that it will be of use/interest for some and if it is for entertainment only ;) First of all I kind of like the idea of having this functionality in! I do *not* like how this changes the default behaviour of libavformat and even introduces a platform-specific behaviour. Please read my comment below. [...]
diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c index e8b695f..d10a7a4 100644 --- a/libavformat/img2dec.c +++ b/libavformat/img2dec.c [...] @@ -27,6 +27,25 @@ #include "libavutil/parseutils.h" #include "avformat.h" #include "internal.h" +#if HAVE_GLOB +#include <glob.h> + +/* Locally define as 0 (bitwise-OR no-op) any missing glob options that + are non-posix glibc/bsd extensions. */ +#ifndef GLOB_NOMAGIC +#define GLOB_NOMAGIC 0 +#endif +#ifndef GLOB_TILDE +#define GLOB_TILDE 0 +#endif +#ifndef GLOB_TILDE_CHECK +#define GLOB_TILDE_CHECK GLOB_TILDE +#endif +#ifndef GLOB_BRACE +#define GLOB_BRACE 0 +#endif + +#endif /* HAVE_GLOB */
Keep those in mind. [...]
if (!s->is_pipe) { + s->use_glob = is_glob(s->path); + if (s->use_glob) { +#if HAVE_GLOB + int gerr; + gerr = glob(s->path, GLOB_NOCHECK|GLOB_BRACE|GLOB_NOMAGIC|GLOB_TILDE_CHECK, NULL, &s->globstate);
If I understand this correctly we get at least braces depending on platform and also probably varying tilde handling if any at all.
+ if (gerr != 0) { + return AVERROR(ENOENT); + } + first_index = 0; + last_index = s->globstate.gl_pathc - 1; +#endif + } else { if (find_image_range(&first_index, &last_index, s->path) < 0) return AVERROR(ENOENT); + }
Might be worth to mention that globs take precedence over image ranges in the docs part. [remaining patch snipped] Instead I would propose following ideal behaviour: 1. Support globbing not only for picture input. 2. Do it only if it was explicitly enabled 3. Agree on a set of features like ?*[]{} and fail if is not available (maybe with the additional possibility to force it on with what is available) 4. Tell the user if he requested to enable globbing but it wasn't available at all in his lavf binary. I know this is controversial, especially point 3. Also I do think point 1 could left out at first and left as possible future enhancement. The described behaviour would allow for implementation of applications that behave consistently across platforms. That is a property I appreciate very much and that I would like to see from FFmpeg programs and libraries. Alexander
On Feb 27, 2012, at 2:11 PM, Alexander Strasser wrote:
Instead I would propose following ideal behaviour: 1. Support globbing not only for picture input.
So, I think you mean that ideally everything that can accept multiple inputs via -i a -i b -i c would accept -i '[abc]' (OR --input-glob '[abc]' if I understand your point 2)
2. Do it only if it was explicitly enabled
Explicitly enabled - at compile time or option time? (I'm guessing you mean something like the second) ./configure --with-glob OR ffmpeg --input-glob \*.jpg The current image sequence input has extra machinery to do expansion of %d (or now glob) style image names, and seems to do that pretty well. I feel like something would be less efficient doing -i 1.jpg -i 2.jpg -i 3.jpg or an equivalent --image-glob that uses common code to expand the glob and pass thousands of frame arguments to the img import code. Or maybe it would be fine.
3. Agree on a set of features like ?*[]{} and fail if is not available (maybe with the additional possibility to force it on with what is available) 4. Tell the user if he requested to enable globbing but it wasn't available at all in his lavf binary.
My thinking behind the set of flags for glob is that I first read the documentation for an expanded non-posix glob() and thought "yes, all of these are good features, I want them" .. and then noticed that many are non-posix and shouldn't be relied on. In some more-is-better sense, having them is better than not having them. It does lead to possible inconsistency between ffmpeg deployments, but the ones that have it are better, right?
I know this is controversial, especially point 3. Also I do think point 1 could left out at first and left as possible future enhancement.
The described behaviour would allow for implementation of applications that behave consistently across platforms. That is a property I appreciate very much and that I would like to see from FFmpeg programs and libraries.
More consistency would be available if someone reimplemented an expanded glob() and bundled that source into ffmpeg, but of course that's maybe not the best thing to do either. 4 seems pretty uncontroversial. It could be pretty straightforward now to detect glob-chars in the filename (using the is_glob() function being added in this patch) and at least emit a warning if glob() was not compiled in.
Hi Brian, very nice to hear you commenting on my proposal. Brian Olson wrote:
On Feb 27, 2012, at 2:11 PM, Alexander Strasser wrote:
Instead I would propose following ideal behaviour: 1. Support globbing not only for picture input.
So, I think you mean that ideally everything that can accept multiple inputs via -i a -i b -i c would accept -i '[abc]' (OR --input-glob '[abc]' if I understand your point 2)
Yes, you got both of them right. Though for point 2 the name should be more in the style of the existing ffmpeg options.
2. Do it only if it was explicitly enabled
Explicitly enabled - at compile time or option time? (I'm guessing you mean something like the second) ./configure --with-glob OR ffmpeg --input-glob \*.jpg
Yes, this one. (Also see comment above.) Another advantage of doing it that way is the average user doesn't need to double escape characters that she didn't want to be expanded by neither her shell nor ffmpeg.
The current image sequence input has extra machinery to do expansion of %d (or now glob) style image names, and seems to do that pretty well. I feel like something would be less efficient doing -i 1.jpg -i 2.jpg -i 3.jpg or an equivalent --image-glob that uses common code to expand the glob and pass thousands of frame arguments to the img import code. Or maybe it would be fine.
You are right, it might not be trivial and that is why I said ideally in the first sentence and advised to leave it out for now. If it can be done cleanly, it can be done as incremental work. Also if no one needs/wants it, we can very well live without it. I just thought it might be an interesting goal and very convenient from time to time.
3. Agree on a set of features like ?*[]{} and fail if is not available (maybe with the additional possibility to force it on with what is available) 4. Tell the user if he requested to enable globbing but it wasn't available at all in his lavf binary.
My thinking behind the set of flags for glob is that I first read the documentation for an expanded non-posix glob() and thought "yes, all of these are good features, I want them" .. and then noticed that many are non-posix and shouldn't be relied on. In some more-is-better sense, having them is better than not having them. It does lead to possible inconsistency between ffmpeg deployments, but the ones that have it are better, right?
OK, what do you want to have? I think we can go with it easily: I will follow you on your next comments and it should be no problem. Maybe it is better to only go for one way of tilde expansion though. The difference might be subtle when it is working one way on one machine and the other way on another which I would like to avoid. It is IMHO a pain to explain to users or to document in manuals.
I know this is controversial, especially point 3. Also I do think point 1 could left out at first and left as possible future enhancement.
The described behaviour would allow for implementation of applications that behave consistently across platforms. That is a property I appreciate very much and that I would like to see from FFmpeg programs and libraries.
More consistency would be available if someone reimplemented an expanded glob() and bundled that source into ffmpeg, but of course that's maybe not the best thing to do either.
Not really better; though it might not be that unreasonable after all. But let's try without it first!
4 seems pretty uncontroversial. It could be pretty straightforward now to detect glob-chars in the filename (using the is_glob() function being added in this patch) and at least emit a warning if glob() was not compiled in.
How about handling point 4 the same as point 3? Make it possible to tell if anything or all (no glob() available) is missing in the binary and emit a warning via av_log. The latter information that all is missing should be available via the configure #define already. Alexander
Le nonidi 9 ventôse, an CCXX, Alexander Strasser a écrit :
1. Support globbing not only for picture input.
How do you imagine it should be handled, from the API point of view? We are nowhere near supporting demuxer-level concatenation, IMHO, so the syntax for various parts of the command line will have to change four or five times before we are able of implementing anything like that.
2. Do it only if it was explicitly enabled
I find that rather user-unfriendly. OTOH, having options to ensure the mode of expansion would be useful for robust automated scripts: ffmpeg -f img2 -expand printf photo%05d.jpg ffmpeg -f img2 -expand glob photo\*.jpg
3. Agree on a set of features like ?*[]{} and fail if is not available (maybe with the additional possibility to force it on with what is available)
I do not like it at all in principle: if we were to cripple features just to be compatible with the lowest common denominator, we could just all install windows and be done with it. On the other hand, for this particular case: - GLOB_BRACE, could be marginally useful, but also cause trouble (the files that randomly appears on people's hard drives tend to have braces or parentheses to indicate the checksum, name of the team or whatever, or so I have been told). - GLOB_TILDE(_CHECK): this is not related to globbing at all, this is some kind of special syntax for a bizarre environment variable expansion (~ expands like $HOME, but only at the start; ~foo would expand like $HOME_FOO, assuming the contents of passwd is part of the environment). Also, while it is perfectly normal for 'foo*.jpg' to be supported only by the img2 demuxer, there is no logic of being able to write ~/image.jpg and not ~/video.avi. Also, for the command-line tool, the user can just not escape the ~ and let their shell expand it. - GLOB_NOMAGIC should be useless, since glob is called only if is_glob has already detected metacharacters. So in this particular case, I do not mind sticking to the standard features at all.
4. Tell the user if he requested to enable globbing but it wasn't available at all in his lavf binary.
I second that.
The described behaviour would allow for implementation of applications that behave consistently across platforms. That is a property I appreciate very much and that I would like to see from FFmpeg programs and libraries.
Unfortunately, consistency is often at odds with feature-fullness, and someone who decided to use one particular platform because it has some interesting extensions should be able to benefit from these extensions. What can be done, OTOH, is to make sure that someone who wants to stick to the common denominator can avoid triggering unexpected extensions. Regards, -- Nicolas George
Hi Nicolas, I assume you read my reply to Brian in the meanwhile, therefore some relevant parts of this mail will be left uncommented. Nicolas George wrote:
Le nonidi 9 ventôse, an CCXX, Alexander Strasser a écrit :
1. Support globbing not only for picture input.
How do you imagine it should be handled, from the API point of view? We are nowhere near supporting demuxer-level concatenation, IMHO, so the syntax for various parts of the command line will have to change four or five times before we are able of implementing anything like that.
2. Do it only if it was explicitly enabled
I find that rather user-unfriendly. OTOH, having options to ensure the mode of expansion would be useful for robust automated scripts:
I see. What do you think about the attached patch? It builds directly on Brian's work and tries to be user-friendly and consistent while not making currently working use cases more hard/impossible when globbing is compiled in. The few examples are given in the patch commit message. I also attached my complete test log so you can dig in deeper. Beware the examples found in the test log are rather extreme to test more corner cases.
ffmpeg -f img2 -expand printf photo%05d.jpg ffmpeg -f img2 -expand glob photo\*.jpg
Looks like a nice idea. I am OK with adding such features later on.
3. Agree on a set of features like ?*[]{} and fail if is not available (maybe with the additional possibility to force it on with what is available)
I do not like it at all in principle: if we were to cripple features just to be compatible with the lowest common denominator, we could just all install windows and be done with it.
On the other hand, for this particular case:
- GLOB_BRACE, could be marginally useful, but also cause trouble (the files that randomly appears on people's hard drives tend to have braces or parentheses to indicate the checksum, name of the team or whatever, or so I have been told).
This tends to be no problem with my proposed patch. Very useful glob chars, [], are also used frequently for the same purposes you quote above. This works out too with my patch.
- GLOB_TILDE(_CHECK): this is not related to globbing at all, this is some kind of special syntax for a bizarre environment variable expansion (~ expands like $HOME, but only at the start; ~foo would expand like $HOME_FOO, assuming the contents of passwd is part of the environment). Also, while it is perfectly normal for 'foo*.jpg' to be supported only by the img2 demuxer, there is no logic of being able to write ~/image.jpg and not ~/video.avi.
Also, for the command-line tool, the user can just not escape the ~ and let their shell expand it.
So you think we can just drop it?
- GLOB_NOMAGIC should be useless, since glob is called only if is_glob has already detected metacharacters.
Agreed. Though didn't change it in this patch though.
So in this particular case, I do not mind sticking to the standard features at all.
Ok, tell me if I should prepare a patch removing the GLOB_BRACE, GLOB_TILDE(_CHECK), GLOB_NOMAGIC.
4. Tell the user if he requested to enable globbing but it wasn't available at all in his lavf binary.
I second that.
I wanted to do this as an incremental improvement to not further delay Brian's work getting into ffmpeg. I thought about printing a warning if the user used glob chars in image2 input path but glob support was not compiled in. Did you have something else in mind? If we decide to keep GLOB_BRACE, that should be taken into account too. Also I think these warnings will have much lower chance of unexpected occurrence after my patch, because only enabled glob chars would count (e.g. %[). BTW if my patch passes review I can of course write an update for the documentation and resubmit. While writing this a further question pops up, should ] and } be removed in is_glob() detection code? If don't think it will make much sense to try to glob it if we only found closing brackets/braces.
The described behaviour would allow for implementation of applications that behave consistently across platforms. That is a property I appreciate very much and that I would like to see from FFmpeg programs and libraries.
Unfortunately, consistency is often at odds with feature-fullness, and someone who decided to use one particular platform because it has some interesting extensions should be able to benefit from these extensions.
What can be done, OTOH, is to make sure that someone who wants to stick to the common denominator can avoid triggering unexpected extensions.
Not sure I can fully agree. It is very broad statement and therefore not easy to comment on specifically. But in any case I think we should get along with each other :) Alexander
On Sun, Mar 11, 2012 at 05:46:08PM +0100, Alexander Strasser wrote:
Hi Nicolas,
I assume you read my reply to Brian in the meanwhile, therefore some relevant parts of this mail will be left uncommented.
Nicolas George wrote:
Le nonidi 9 ventôse, an CCXX, Alexander Strasser a écrit :
1. Support globbing not only for picture input.
How do you imagine it should be handled, from the API point of view? We are nowhere near supporting demuxer-level concatenation, IMHO, so the syntax for various parts of the command line will have to change four or five times before we are able of implementing anything like that.
2. Do it only if it was explicitly enabled
I find that rather user-unfriendly. OTOH, having options to ensure the mode of expansion would be useful for robust automated scripts:
I see. What do you think about the attached patch? It builds directly on Brian's work and tries to be user-friendly and consistent while not making currently working use cases more hard/impossible when globbing is compiled in.
The few examples are given in the patch commit message. I also attached my complete test log so you can dig in deeper. Beware the examples found in the test log are rather extreme to test more corner cases.
ffmpeg -f img2 -expand printf photo%05d.jpg ffmpeg -f img2 -expand glob photo\*.jpg
Looks like a nice idea. I am OK with adding such features later on.
3. Agree on a set of features like ?*[]{} and fail if is not available (maybe with the additional possibility to force it on with what is available)
I do not like it at all in principle: if we were to cripple features just to be compatible with the lowest common denominator, we could just all install windows and be done with it.
On the other hand, for this particular case:
- GLOB_BRACE, could be marginally useful, but also cause trouble (the files that randomly appears on people's hard drives tend to have braces or parentheses to indicate the checksum, name of the team or whatever, or so I have been told).
This tends to be no problem with my proposed patch. Very useful glob chars, [], are also used frequently for the same purposes you quote above. This works out too with my patch.
- GLOB_TILDE(_CHECK): this is not related to globbing at all, this is some kind of special syntax for a bizarre environment variable expansion (~ expands like $HOME, but only at the start; ~foo would expand like $HOME_FOO, assuming the contents of passwd is part of the environment). Also, while it is perfectly normal for 'foo*.jpg' to be supported only by the img2 demuxer, there is no logic of being able to write ~/image.jpg and not ~/video.avi.
Also, for the command-line tool, the user can just not escape the ~ and let their shell expand it.
So you think we can just drop it?
- GLOB_NOMAGIC should be useless, since glob is called only if is_glob has already detected metacharacters.
Agreed. Though didn't change it in this patch though.
So in this particular case, I do not mind sticking to the standard features at all.
Ok, tell me if I should prepare a patch removing the GLOB_BRACE, GLOB_TILDE(_CHECK), GLOB_NOMAGIC.
4. Tell the user if he requested to enable globbing but it wasn't available at all in his lavf binary.
I second that.
I wanted to do this as an incremental improvement to not further delay Brian's work getting into ffmpeg.
I thought about printing a warning if the user used glob chars in image2 input path but glob support was not compiled in. Did you have something else in mind?
If we decide to keep GLOB_BRACE, that should be taken into account too.
Also I think these warnings will have much lower chance of unexpected occurrence after my patch, because only enabled glob chars would count (e.g. %[).
BTW if my patch passes review I can of course write an update for the documentation and resubmit.
While writing this a further question pops up, should ] and } be removed in is_glob() detection code? If don't think it will make much sense to try to glob it if we only found closing brackets/braces.
The described behaviour would allow for implementation of applications that behave consistently across platforms. That is a property I appreciate very much and that I would like to see from FFmpeg programs and libraries.
Unfortunately, consistency is often at odds with feature-fullness, and someone who decided to use one particular platform because it has some interesting extensions should be able to benefit from these extensions.
What can be done, OTOH, is to make sure that someone who wants to stick to the common denominator can avoid triggering unexpected extensions.
Not sure I can fully agree. It is very broad statement and therefore not easy to comment on specifically. But in any case I think we should get along with each other :)
Alexander
img2dec.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) 0b95708302972c93e8055e4b040b2a3451d2bd50 0001-img2dec-Recognize-glob-meta-chars-only-if-prefixed-b.patch From e2e044ff23cb28b6c885d041cdd3a59f47c3ec4a Mon Sep 17 00:00:00 2001 From: Alexander Strasser <eclipse7@gmx.net> Date: Sun, 11 Mar 2012 15:24:51 +0100 Subject: [PATCH] img2dec: Recognize glob meta chars only if prefixed by %
This changes globbing support to only be used if the character contains at least one glob meta character that is preceded by an unescaped %. To escape a literal % one would use %% which is identical to the way to match a % with image2 sequence generation feature.
* Makes it possible to have patterns like %04d-[720p].jpg work again with sequence number generation. Previously this would always be detected as a glob pattern and was interpreted by the image2 glob code instead.
* Makes it possible to use %*-[720p].jpg to match above pattern without having to double escape it to be not interpreted by most shells and not by the image2 glob code (previously one would need to use \*-\\\[720p\\\].jpg to achieve the same)
applied thanks! [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Its not that you shouldnt use gotos but rather that you should write readable code and code with gotos often but not always is less readable
Michael Niedermayer wrote:
On Sun, Mar 11, 2012 at 05:46:08PM +0100, Alexander Strasser wrote: [...]
img2dec.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) 0b95708302972c93e8055e4b040b2a3451d2bd50 0001-img2dec-Recognize-glob-meta-chars-only-if-prefixed-b.patch From e2e044ff23cb28b6c885d041cdd3a59f47c3ec4a Mon Sep 17 00:00:00 2001 From: Alexander Strasser <eclipse7@gmx.net> Date: Sun, 11 Mar 2012 15:24:51 +0100 Subject: [PATCH] img2dec: Recognize glob meta chars only if prefixed by %
This changes globbing support to only be used if the character contains at least one glob meta character that is preceded by an unescaped %. To escape a literal % one would use %% which is identical to the way to match a % with image2 sequence generation feature.
* Makes it possible to have patterns like %04d-[720p].jpg work again with sequence number generation. Previously this would always be detected as a glob pattern and was interpreted by the image2 glob code instead.
* Makes it possible to use %*-[720p].jpg to match above pattern without having to double escape it to be not interpreted by most shells and not by the image2 glob code (previously one would need to use \*-\\\[720p\\\].jpg to achieve the same)
applied
thanks!
I just remembered that this deserves a documentation update. Will send one this weekend. Alexander
On Mon, Feb 27, 2012 at 10:33:22AM +0100, Nicolas George wrote:
L'octidi 8 ventôse, an CCXX, Michael Niedermayer a écrit :
if there are no further comments / reviews then ill apply this in a few days
I re-read it, it looks ok.
Unfortunately, it no longer apply cleanly since the merge of the img2dec/img2enc split. Here is a fixed version; it is in my repository, I can push it easily.
Regards,
-- Nicolas George
configure | 2 + doc/ffmpeg.texi | 6 +++- libavformat/img2dec.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 77 insertions(+), 3 deletions(-) a41125a961fee3a8b4da6bba076a60604f84b3e4 0001-img2-glob-matching-for-image-series.patch From 7b82b00b80b51472a643ca98334e377d881b8962 Mon Sep 17 00:00:00 2001 From: Brian Olson <icic@bolson.org> Date: Mon, 27 Feb 2012 10:27:17 +0100 Subject: [PATCH] img2: glob matching for image series
patch applied thanks to everyone who contributed! [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB DNS cache poisoning attacks, popular search engine, Google internet authority dont be evil, please
On Sat, Jan 07, 2012 at 03:13:57PM -0500, Brian Olson wrote:
ffmpeg can take a sequentially numbered series of image files and make them into a video. I like to datestamp my images "20120106_134500.jpg" or similar. I used the "glob" library to allow things like:
ffmpeg -i \*.jpg ...
It picks things up in ascii-betical order and is much simpler than writing scripts to link or rename all my images. I think it's a good idea and people will find this to be natural and useful behavior.
I posted copy of my repo to: http://bolson.org/~bolson/ffmpeg.git/
My changes are in a branch "bolson-wordexp" (slightly misnamed...)
I can make a patch file if you have a preferred way of dealing with git.
iam happy with any method but 2 changes where the 1st adds wordexp support and the 2nd replaces wordexp by glob are a bit ehm messy ... that should be just one clean change that adds glob support. and for a single patch simply posting the patch is probably the easiest way. If you maintain some code for longer and there are regular changes then i prefer if i can just pull them in from a git repo instead of having to apply x patches by hand possibly forgeting one in the process. But the method of submitting really doesnt matter much, we are flexible and can take the code by any means if some contributor has a strong preferance [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Democracy is the form of government in which you can choose your dictator
participants (8)
-
Alexander Strasser -
Brian Olson -
Clément Bœsch -
compn -
Michael Niedermayer -
Nicolas George -
Paul B Mahol -
Thilo Borgmann