[FFmpeg-devel] [PATCH] Make Make url_seek() return ESPIPE if the seek operation is not defined

[Stefano Sabatini]

On date Monday 2010-03-22 10:27:03 +0100, Michael Niedermayer encoded:
> On Sun, Mar 21, 2010 at 10:51:12PM +0100, Stefano Sabatini wrote:
> > On date Sunday 2010-03-21 22:38:28 +0100, Michael Niedermayer encoded:
> > > On Sun, Mar 21, 2010 at 07:38:05PM +0100, Stefano Sabatini wrote:
> > > > On date Friday 2010-03-19 00:50:55 +0100, Stefano Sabatini encoded:
> > > > > On date Tuesday 2010-03-16 23:55:21 +0100, Stefano Sabatini encoded:
> > > > > > Hi, $subj.
> > > > > > -- 
> > > > > > FFmpeg = Fascinating Frenzy Mystic Purposeless Ecumenical Gargoyle
> > > > > 
> > > > > > From 29721c9bf73702a2e62697368f8dca8194508326 Mon Sep 17 00:00:00 2001
> > > > > > From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> > > > > > Date: Tue, 16 Mar 2010 22:48:37 +0100
> > > > > > Subject: [PATCH 01/12] Make url_seek() return ESPIPE if the seek operation is not defined in
> > > > > >  the protocol, rather than EPIPE.
> > > > > > 
> > > > > > ---
> > > > > >  libavformat/avio.c |    2 +-
> > > > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > > > 
> > > > > > diff --git a/libavformat/avio.c b/libavformat/avio.c
> > > > > > index af9e049..1d12436 100644
> > > > > > --- a/libavformat/avio.c
> > > > > > +++ b/libavformat/avio.c
> > > > > > @@ -201,7 +201,7 @@ int64_t url_seek(URLContext *h, int64_t pos, int whence)
> > > > > >      int64_t ret;
> > > > > >  
> > > > > >      if (!h->prot->url_seek)
> > > > > > -        return AVERROR(EPIPE);
> > > > > > +        return AVERROR(ESPIPE);
> > > > > >      ret = h->prot->url_seek(h, pos, whence & ~AVSEEK_FORCE);
> > > > > >      return ret;
> > > > > >  }
> > > > 
> > > > Well I wasn't lucid when I wrote that, ESPIPE is meaningful only with
> > > > PIPES, maybe AVERROR_NOTSUPP is more meaningful here.
> > > 
> > > why do you want to change every error code in lav* ?
> > 
> > I was trying to review all the AVERROR(EPIPE) uses in libav*, I don't
> > mind too much about this one, I just want to dump the current
> > definition of AVERROR_EOF and replace it with an FFmpeg specific error
> > code, that's why I'm trying to double check how AVERROR_EOF =
> > AVERROR(EPIPE) is currently used.
> 
> AVERROR_NOTSUPP feels more toward a feature that can be supported but is not
> we also might want to have a 
> AVERROR_DISABLED for a feature suported but not compiled in
> but then there is AVERROR_PATCHWELCOME and iam not sure what AVERROR_NOTSUPP is
> supposed to mean then
> anyway, error docs are poor and you are changing error code without their
> meaning being clearly documented.

Description for AVERROR_NOTSUPP:
#define AVERROR_NOTSUPP     AVERROR(ENOSYS)  /**< Operation not supported. */

Description for AVERROR_PATCHWELCOME:
#define AVERROR_PATCHWELCOME    (-MKTAG('P','A','W','E')) /**< Not yet implemented in FFmpeg. Patches welcome. */

My interpretation of AVERROR_NOTSUPP:
The *operation* requested is not legal/valid, as it is not supported
by the element being requested.

So AVERROR_NOTSUPP should be returned just in case a certain operation
is requested to an object for which that operation doesn't make sense
/ cannot be implemented, and in this sense is semantically near to the
meaning of ENOSYS.

Think for example of a seek operation in a pipe, for which
AVERROR_PATCHWELCOME wouldn't make sense.

I recognize though that the term "supported" in the "not supported"
expression may lead to confusion, as it suggests what you said, so I'd
suggest to use instead AVERROR_ILLEGALOPERATION or
AVERROR_INVALIDOPERATION.

As for the distinction between AVERROR_DISABLED and
AVERROR_PATCHWELCOME, I don't think that would be a good idea
implementation-wise, as most of the time a program can't tell the
distinction amongst the two.

For example av_error_input_format() doesn't know if it can't recognize
a format because it has not been compiled in, because the format is
not implemented, or because we inflicted to him bogus random data.

If you agree with the semantics I proposed, I'll post a patch for
clarifying the description of AVERROR_NOTSUPP, also tell me what do
you think about the AVERROR_INVALIDOPERATION rename.

Regards.
-- 
FFmpeg = Fiendish & Formidable MultiPurpose Erratic Guru