[FFmpeg-devel] [libav-devel] [PATCH 1/3] Add avio_check() function.

[Stefano Sabatini]

[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-lala at 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-lala at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-avio-add-avio_check.patch
Type: text/x-diff
Size: 2482 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110410/d8701851/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-file-implement-url_check-callback-in-the-file-and-pi.patch
Type: text/x-diff
Size: 1421 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110410/d8701851/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-prefer-avio_check-over-url_exist.patch
Type: text/x-diff
Size: 3138 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110410/d8701851/attachment-0002.bin>