[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 <[email protected]>
> > 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 <[email protected]>
> > 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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
