[CC-ing ffmpeg-devel] On date Saturday 2011-04-09 12:35:58 +0200, Anton Khirnov wrote: > On Sat, Apr 09, 2011 at 02:47:35AM +0200, Stefano Sabatini wrote: > > From b563c12cb285f1e6eb8dc19d1a18323cd9280ea1 Mon Sep 17 00:00:00 2001 > > From: Stefano Sabatini <stefano.sabatini-l...@poste.it> > > Date: Fri, 8 Apr 2011 18:32:25 +0200 > > Subject: [PATCH] avio: add avio_check() > > > > The new function is more flexible than url_exist(), as it allows to > > specify which access flags to check, and does not require an explicit > > open of the checked resource. > > --- > > libavformat/avio.c | 19 +++++++++++++++++++ > > libavformat/avio.h | 15 +++++++++++++++ > > 2 files changed, 34 insertions(+), 0 deletions(-) > > > > diff --git a/libavformat/avio.c b/libavformat/avio.c > > index 7b066e3..cc57529 100644 > > --- a/libavformat/avio.c > > +++ b/libavformat/avio.c > > @@ -362,6 +362,25 @@ int url_exist(const char *filename) > > return 1; > > } > > > > +int avio_check(const char *url, int flags) > > +{ > > + URLContext *h; > > + int ret = ffurl_alloc(&h, url, flags); > > + if (ret) > > + return ret; > > + > > + if (h->prot->url_check) { > > + ret = h->prot->url_check(h, flags); > > + } else { > > + ret = ffurl_connect(h); > > + if (ret >= 0) > > When is ret > 0? A quick glance at ffurl_connect suggests that it > returns 0 on success or an AVERROR < 0 on error.
The usual convenction is: >= 0 in case of success, < 0 in case of failure, even if the positive values are not used most of the times, so the check can't hurt. > > + ret = flags; > > + } > > + > > + ffurl_close(h); > > + return ret; > > +} > > + > > int64_t ffurl_size(URLContext *h) > > { > > int64_t pos, size; > > diff --git a/libavformat/avio.h b/libavformat/avio.h > > index 03b6f6f..051c06e 100644 > > --- a/libavformat/avio.h > > +++ b/libavformat/avio.h > > @@ -127,6 +127,20 @@ attribute_deprecated void > > url_set_interrupt_cb(URLInterruptCB *interrupt_cb); > > int url_exist(const char *url); > > > > /** > > + * Return AVIO_* access flags corresponding to the access permissions > > + * of the resource in url, or a negative value corresponding to an > > + * AVERROR code in case of failure. The returned access flags are > > + * masked by the value in flags. > > + * > > + * @note This function is intrinsecally unsafe, in the sense that the > > nit: intrins_i_cally > > > + * checked resource may change its existence or permission status from > > + * one call to another. Thus you should not trust the returned value, > > + * unless you are sure that no other processes are accessing the > > + * checked resource. > > + */ > > +int avio_check(const char *url, int flags); > > + > > +/** > > * The callback is called in blocking functions to test regulary if > > * asynchronous interruption is needed. AVERROR_EXIT is returned > > * in this case by the interrupted function. 'NULL' means no interrupt > > @@ -157,6 +171,7 @@ typedef struct URLProtocol { > > int priv_data_size; > > const AVClass *priv_data_class; > > int flags; > > + int (*url_check)(URLContext *h, int mask); > > } URLProtocol; > > > > #if FF_API_REGISTER_PROTOCOL > > -- > > 1.7.2.3 > > > > > From 3337ee5fd1ca5772c293f76e7c15267077c8001f Mon Sep 17 00:00:00 2001 > > From: Stefano Sabatini <stefano.sabatini-l...@poste.it> > > Date: Sat, 9 Apr 2011 01:32:37 +0200 > > Subject: [PATCH] prefer avio_check() over url_exist() > > > > The problem with url_exist() is that it tries to open a resource in > > RDONLY mode. If the file is a FIFO and there is already a reading > > client, the open() call will hang. > > > > By using avio_check() with access mode of 0, the second reading > > process will check if the file exists without attempting to open it, > > thus avoiding the lock. > > > > Fix issue #1663. > > --- > > ffmpeg.c | 2 +- > > ffserver.c | 4 ++-- > > libavformat/img2.c | 6 +++--- > > 3 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/ffmpeg.c b/ffmpeg.c > > index 40e5e67..9153a5b 100644 > > --- a/ffmpeg.c > > +++ b/ffmpeg.c > > @@ -3870,7 +3870,7 @@ static void opt_output_file(const char *filename) > > (strchr(filename, ':') == NULL || > > filename[1] == ':' || > > av_strstart(filename, "file:", NULL))) { > > - if (url_exist(filename)) { > > + if (avio_check(filename, 0) != AVERROR(ENOENT)) { > > if (!using_stdin) { > > fprintf(stderr,"File '%s' already exists. Overwrite ? > > [y/N] ", filename); > > fflush(stderr); > > diff --git a/ffserver.c b/ffserver.c > > index 6e42979..5f165dd 100644 > > --- a/ffserver.c > > +++ b/ffserver.c > > @@ -3684,7 +3684,7 @@ static void build_feed_streams(void) > > for(feed = first_feed; feed != NULL; feed = feed->next_feed) { > > int fd; > > > > - if (url_exist(feed->feed_filename)) { > > + if (url_check(feed->feed_filename, URL_RDONLY)) { > > Ehm... Fixed. > Otherwise the patches looks good. I can confirm that it really solves > 1663. Updated and tested here, seems to work. I tested with: # create a fifo $ mkfifo /tmp/fifo # await to read from fifo $ ffmpeg -i /tmp/fifo -f null out-null.avi & # check for existence on /tmp/fifo, write to it $ ffmpeg -i slow.flv -f rawvideo /tmp/fifo Without the patchset the second ffmpeg instance hangs, with the patchset applied it doesn't. make fate succeeds. Other notes: * avio_check() or ffurl_check()? * for checking existence avio_check() is slightly more awkward than url_exist(), possibilities are: keep url_exist() as inline and make it use avio_check(), or simply drop it, as I tend to prefer in this case. * unfortunately there is no way to check for the existence of a resource without to call ffurl_alloc() in avio_check(). A possibility would be to implement a function av_get_protocol_from_url() and invoke a check_url callback defined directly on the protocol, with no need to allocate a protocol context (implies direct access of the protocol structure). * ffurl_alloc() returns AVERROR(ENOENT) in case of an unknown protocol scheme (e.g. for "foo:bar.avi"). This means in particular that avio_check("foo:bar.avi", 0) will return AVERROR(ENOENT), which doesn't sound too correct. Hint: maybe we could make use of AVERROR_PROTOCOL_NOT_FOUND, even if in this case it should be more correct to say "unknown/unsupported protocol". * in img2.c: checks are done on *readability* rather than on existence, as this is equivalent to the previous code (but I can't decide on this one, that's one of the reason this is taking so much time). -- Everyone complains of his memory, no one of his judgement.
>From 93d7fcba1a85fef948b2c318826ef0f9cdfe6607 Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefano.sabatini-l...@poste.it> Date: Fri, 8 Apr 2011 18:32:25 +0200 Subject: [PATCH] avio: add avio_check() The new function is more flexible than url_exist(), as it allows to specify which access flags to check, and does not require an explicit open of the checked resource. --- libavformat/avio.c | 19 +++++++++++++++++++ libavformat/avio.h | 15 +++++++++++++++ 2 files changed, 34 insertions(+), 0 deletions(-) diff --git a/libavformat/avio.c b/libavformat/avio.c index 7b066e3..cc57529 100644 --- a/libavformat/avio.c +++ b/libavformat/avio.c @@ -362,6 +362,25 @@ int url_exist(const char *filename) return 1; } +int avio_check(const char *url, int flags) +{ + URLContext *h; + int ret = ffurl_alloc(&h, url, flags); + if (ret) + return ret; + + if (h->prot->url_check) { + ret = h->prot->url_check(h, flags); + } else { + ret = ffurl_connect(h); + if (ret >= 0) + ret = flags; + } + + ffurl_close(h); + return ret; +} + int64_t ffurl_size(URLContext *h) { int64_t pos, size; diff --git a/libavformat/avio.h b/libavformat/avio.h index 03b6f6f..fba556e 100644 --- a/libavformat/avio.h +++ b/libavformat/avio.h @@ -127,6 +127,20 @@ attribute_deprecated void url_set_interrupt_cb(URLInterruptCB *interrupt_cb); int url_exist(const char *url); /** + * Return AVIO_* access flags corresponding to the access permissions + * of the resource in url, or a negative value corresponding to an + * AVERROR code in case of failure. The returned access flags are + * masked by the value in flags. + * + * @note This function is intrinsically unsafe, in the sense that the + * checked resource may change its existence or permission status from + * one call to another. Thus you should not trust the returned value, + * unless you are sure that no other processes are accessing the + * checked resource. + */ +int avio_check(const char *url, int flags); + +/** * The callback is called in blocking functions to test regulary if * asynchronous interruption is needed. AVERROR_EXIT is returned * in this case by the interrupted function. 'NULL' means no interrupt @@ -157,6 +171,7 @@ typedef struct URLProtocol { int priv_data_size; const AVClass *priv_data_class; int flags; + int (*url_check)(URLContext *h, int mask); } URLProtocol; #if FF_API_REGISTER_PROTOCOL -- 1.7.2.3
>From 82cab45a87fa517d5972c7a3c6b523c981d4c1d1 Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefano.sabatini-l...@poste.it> Date: Thu, 30 Sep 2010 13:21:42 +0200 Subject: [PATCH] file: implement url_check() callback in the file and pipe protocols --- libavformat/file.c | 16 ++++++++++++++++ 1 files changed, 16 insertions(+), 0 deletions(-) diff --git a/libavformat/file.c b/libavformat/file.c index b8c7c4c..f939a8e 100644 --- a/libavformat/file.c +++ b/libavformat/file.c @@ -94,6 +94,20 @@ static int file_close(URLContext *h) return close(fd); } +static int file_check(URLContext *h, int mask) +{ + struct stat st; + int ret = stat(h->filename, &st); + if (ret < 0) + return AVERROR(errno); + + ret |= st.st_mode&S_IRUSR ? mask&AVIO_RDONLY : 0; + ret |= st.st_mode&S_IWUSR ? mask&AVIO_WRONLY : 0; + ret |= st.st_mode&S_IWUSR && st.st_mode&S_IRUSR ? mask&AVIO_RDWR : 0; + + return ret; +} + URLProtocol ff_file_protocol = { "file", file_open, @@ -102,6 +116,7 @@ URLProtocol ff_file_protocol = { file_seek, file_close, .url_get_file_handle = file_get_handle, + .url_check = file_check, }; #endif /* CONFIG_FILE_PROTOCOL */ @@ -136,6 +151,7 @@ URLProtocol ff_pipe_protocol = { file_read, file_write, .url_get_file_handle = file_get_handle, + .url_check = file_check, }; #endif /* CONFIG_PIPE_PROTOCOL */ -- 1.7.2.3
>From fe9bc3620d98225a2d5c988a38118a69ff2ec845 Mon Sep 17 00:00:00 2001 From: Stefano Sabatini <stefano.sabatini-l...@poste.it> Date: Sat, 9 Apr 2011 01:32:37 +0200 Subject: [PATCH] prefer avio_check() over url_exist() The problem with url_exist() is that it tries to open a resource in RDONLY mode. If the file is a FIFO and there is already a reading client, the open() call will hang. By using avio_check() with access mode of 0, the second reading process will check if the file exists without attempting to open it, thus avoiding the lock. Fix issue #1663. --- ffmpeg.c | 2 +- ffserver.c | 4 ++-- libavformat/img2.c | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/ffmpeg.c b/ffmpeg.c index 40e5e67..df09d6a 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -3870,7 +3870,7 @@ static void opt_output_file(const char *filename) (strchr(filename, ':') == NULL || filename[1] == ':' || av_strstart(filename, "file:", NULL))) { - if (url_exist(filename)) { + if (avio_check(filename, 0) == 0) { if (!using_stdin) { fprintf(stderr,"File '%s' already exists. Overwrite ? [y/N] ", filename); fflush(stderr); diff --git a/ffserver.c b/ffserver.c index 6e42979..4c36a87 100644 --- a/ffserver.c +++ b/ffserver.c @@ -3684,7 +3684,7 @@ static void build_feed_streams(void) for(feed = first_feed; feed != NULL; feed = feed->next_feed) { int fd; - if (url_exist(feed->feed_filename)) { + if (avio_check(feed->feed_filename, 0) == 0) { /* See if it matches */ AVFormatContext *s; int matches = 0; @@ -3757,7 +3757,7 @@ static void build_feed_streams(void) unlink(feed->feed_filename); } } - if (!url_exist(feed->feed_filename)) { + if (avio_check(feed->feed_filename, 0) < 0) { AVFormatContext s1 = {0}, *s = &s1; if (feed->readonly) { diff --git a/libavformat/img2.c b/libavformat/img2.c index 4e55c22..eac551b 100644 --- a/libavformat/img2.c +++ b/libavformat/img2.c @@ -132,11 +132,11 @@ static int find_image_range(int *pfirst_index, int *plast_index, if (av_get_frame_filename(buf, sizeof(buf), path, first_index) < 0){ *pfirst_index = *plast_index = 1; - if(url_exist(buf)) + if (avio_check(buf, AVIO_RDONLY|AVIO_RDWR) > 0) return 0; return -1; } - if (url_exist(buf)) + if (avio_check(buf, AVIO_RDONLY|AVIO_RDWR) > 0) break; } if (first_index == 5) @@ -154,7 +154,7 @@ static int find_image_range(int *pfirst_index, int *plast_index, if (av_get_frame_filename(buf, sizeof(buf), path, last_index + range1) < 0) goto fail; - if (!url_exist(buf)) + if (avio_check(buf, AVIO_RDONLY|AVIO_RDWR) <= 0) break; range = range1; /* just in case... */ -- 1.7.2.3
_______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel