[FFmpeg-devel] [PATCH] Redefine AVERROR_EOF as a specific FFmpeg error code at the next lavu major bump

[Stefano Sabatini]

On date Sunday 2010-03-14 20:01:16 +0100, Michael Niedermayer encoded:
> On Sat, Mar 13, 2010 at 09:20:21PM +0100, Stefano Sabatini wrote:
> > Hi, $subj.
> > -- 
> > FFmpeg = Fast & Fierce Mastodontic Plastic Easy Guide
> 
> >  error.h |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 0e189262c6eff5c66cb595e68e06d46cccedd7e4  0005-Change-the-definition-of-AVERROR_EOF-at-the-next-lib.patch
> > >From aff03d4184558d02b6f20605eb1adc32a9eade39 Mon Sep 17 00:00:00 2001
> > From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> > Date: Sat, 13 Mar 2010 21:17:06 +0100
> > Subject: [PATCH 5/5] Change the definition of AVERROR_EOF at the next libavutil major bump,
> >  using an FFmpeg specific error code rather than EPIPE, which has a
> >  quite different semantics.
> 
> there are matches for EPIPE in the source, i am thus not in favor of this
> patch because you wont debug and fix the resulting bugs rather i would
> have too.

For some generic explanation of the error codes:

[1] http://www.opengroup.org/onlinepubs/009695399/basedefs/errno.h.html
[2] http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_03.html

* EPIPE = error 32 = "Broken pipe. A write was attempted on a socket,
  pipe, or FIFO for which there is no process to read the data.

  Now since the description is not particularly helpful I did some
  research using the powerful Internet:
  http://www.google.it/search?q=epipe+%22broken+pipe%22+meaning

  http://epipe.com/epipe 
  |EPIPE error usually occurs in situations where there is a
  |communications circuit or a pipe which has broken down for some reason
  |(for example if the other end of the pipe has stopped listening or
  |somebody has unplugged the network cable), and therefore the desired
  |I/O operation would not complete and instead produces an error value
  |EPIPE. The EPIPE error macro is defined in errno.h standard include
  |file.

  From this description results quite clear that the EPIPE meaning is
  quite different from that of "End of file" (which in turns in our case
  may also mean "End of stream").

* ESPIPE = error 29 = "Illegal seek" in [1], while in [2]:
  "Invalid seek. An attempt was made to access the file offset
  associated with a pipe or FIFO."

  The "S" in the error symbol seems related to the Seek operation,
  main use seems related to the operation of lseek executed on a pipe
  for which that operation does not make sense.

  The meaning to me looks like "The seek operation invoked on this
  object doesn't make sense / is illegal / is invalid".

  From the GNU libc manual (lseek() description):

  |  `ESPIPE'
  |        The FILEDES corresponds to an object that cannot be
  |        positioned, such as a pipe, FIFO or terminal device.
  |        (POSIX.1 specifies this error only for pipes and FIFOs, but
  |        in the GNU system, you always get `ESPIPE' if the object is
  |        not seekable.)

Result of grep:

*** ./libavdevice/alsa-audio-common.c:173:    if (err == -EPIPE) {

This use depends on the ALSA interface (no blame intended):
http://www.alsa-project.org/alsa-doc/alsa-lib/group___p_c_m___direct.html

*** ./libavformat/avio.c:204:        return AVERROR(EPIPE);

int64_t url_seek(URLContext *h, int64_t pos, int whence)
{
    int64_t ret;

    if (!h->prot->url_seek)
        return AVERROR(EPIPE);
    ret = h->prot->url_seek(h, pos, whence & ~AVSEEK_FORCE);
    return ret;
}

here looks like we should use instead:
    if ((ret = h->prot->url_seek) < 0)
        return ret;

*** ./libavformat/rtspenc.c:96:                return AVERROR(EPIPE);
*** ./libavformat/rtspenc.c:101:                return AVERROR(EPIPE);

            ret = ff_rtsp_read_reply(s, &reply, NULL, 1);
            if (ret < 0)
                return AVERROR(EPIPE);
            if (ret == 1)
                ff_rtsp_skip_packet(s);
            /* XXX: parse message */
            if (rt->state != RTSP_STATE_STREAMING)
                return AVERROR(EPIPE);

I'm not qualified enough to say if this use is correct, but the use
seems consistent with the meaning of EPIPE which is "Stream in the
channel interrupted for some error condition"). Maybe it would be
better even in the first case to just return ret.

*** ./libavformat/aviobuf.c:159:            return AVERROR(EPIPE);

Context:
int64_t url_fseek(ByteIOContext *s, int64_t offset, int whence)
...
        if (s->eof_reached)
            return AVERROR(EPIPE);

seems a perfect place where to return AVERROR_EOF.

*** ./libavformat/aviobuf.c:162:        int64_t res = AVERROR(EPIPE);

Context:
        int64_t res = AVERROR(EPIPE);
        ...

        if (!s->seek || (res = s->seek(s->opaque, offset, SEEK_SET)) < 0)
            return res;

Here EPIPE is returned if s->seek is not defined, maybe ESPIPE would
be more appropriate.

*** ./libavformat/aviobuf.c:199:        return AVERROR(EPIPE);

Context:
int64_t url_fsize(ByteIOContext *s)
{
    int64_t size;

    if(!s)
        return AVERROR(EINVAL);

    if (!s->seek)
        return AVERROR(EPIPE);

Again I believe that ESPIPE is more meaningful here.

*** ./libavutil/error.h:41:#define AVERROR_EOF         AVERROR(EPIPE)   /**< End of file. */

As you see I'm willing to fix all the possible incongruent uses of
EPIPE in the code. I'll post a patch on a case by case basis, then
we should be able to safely apply this patch.

Regards.

PS this error handling fix job is boring to death but someone has to
do it, and I'm happy it's finally going on as it was on my todo list
since when I started hacking on the FFmpeg code.
-- 
FFmpeg = Fantastic & Faithless Minimal Philosophical Exxagerate Gadget