[FFmpeg-devel] [RFC][PATCH] avformat/flvdec: avoid reseting eof_reached to 0 silently

[wm4]

On Mon, 13 Apr 2015 12:02:29 +0800
Zhang Rui <bbcallen at gmail.com> wrote:

> 2015-04-12 22:45 GMT+08:00 Michael Niedermayer <michaelni at gmx.at>:
> > On Sun, Apr 12, 2015 at 12:00:18PM +0800, Zhang Rui wrote:
> >> 2015-04-10 22:04 GMT+08:00 wm4 <nfxjfg at googlemail.com>:
> >> > On Fri, 10 Apr 2015 21:17:42 +0800
> >> > Zhang Rui <bbcallen at gmail.com> wrote:
> >> >>
> >> >> This kind of error handling need some more work in aviobuf.c,
> >> >> and more advises from ffmpeg developers.
> >> >> And i prefer this way than the patch I posted.
> >> >
> >> > stdio.h does it this way: FILE has an error flag that is set when
> >> > something goes wrong.
> >>
> >> AVIOContext has an error field, too. But I don't think it's enough
> >> for EAGAIN situation without some convention.
> >> At least, ffplay doesn't show that.
> >>
> >> >> > Also, why doesn't avio_skip() return an error if the skip count is not
> >> >> > 0 and the stream has reached EOF?
> >> >>
> >> >> The eof handling is quite confusing in ffplay for me. AVERROR_EOF is
> >> >> clear enough.
> >> >
> >> > Well, I have no idea what avio_skip() even returns... it just calls
> >> > avio_seek(), which is a goddamn fucked up mess thanks to years of
> >> > people adding hacks.
> >>
> >> Is there any correct direction to fix it?
> >>
> >> > ffplay probably does it wrong. Wouldn't be surprising. It checks
> >> > avio_feof() after a av_read_frame() call, which doesn't look correct.
> >> > File EOF has absolutely nothing to do with whether a demuxer still has
> >> > data.
> >> >
> >> > On a side note, I'm not sure whether av_read_frame() returning
> >> > AVERROR_EOF is an error at all, or just signals that the end of the
> >> > file was reached. The doxygen on this function isn't helpful either.
> >>
> >> Is there any ideas, or any helpful keywords or threads in mail list archive?
> >
> > a simple error_count field could be added that way one could easily
> > check if the count increased over any series of function call(s)

Seems like a good idea.

> Good enough for internal use of avio_r8().
> 
> > it also could be presented at verbose level by the user application,
> > showing how many io errors where encountered which where not fatal
> 
> Two problems for application:
> 
> 1. Which error should be defined as fatal?
> For avio_r8(), even an EAGAIN can be a fatal.
> The error_count has no more information than error field for application.

Well, EAGAIN is fatal isn't it? Virtually nothing checks the avio_r8()
return value to retry (and expecting it that would be totally
unreasonable), so this has to be handled on a deeper level, possibly
before the error is even set. (Or in other words, EAGAIN is not an
error in some contexts. Although it could be - if you setup a signal
handler to interrupt system calls instead of retrying them
transparently, you probably really want to unblock all blocking calls,
instead of having code to block immediately again by retrying.)

> 2. Nested format, e.g. hls, concatdec.
> The error_count field is supposed to be added to AVIOContext.
> But if the internal input failed, it's weired to set error to the
> outer AVIOContext,
> since it has nothing todo with the outer http/file/... protocol.

What do nested protocols have to do anything with this? In cases when a
protocol reads from another protocol, the error would obviously be
naturally passed along.

> 
> In my opinion, we could stop returning avio error code directly from
> av_read_frame(),
> and limit the error code which could return from av_read_frame(), explicitly.
> e.g.
> 
> // Map various error codes to limited error codes.
> int av_read_frame2(AVFormatContext *s, AVPacket *pkt) {
>     int ret = av_read_frame(s, pkt);
>     switch (ret) {
>         case AVERROR_EOS: // end of stream.
>         case AVERROR_AGAIN: // error can be recovered.
>         case AVERROR_EXIT: // interrupted by user.
>         case AVERROR_FAIL: // generic error
>             return ret;
>         case AVERROR(EAGAIN):
>             return AVERROR_AGAIN;
>         case AVERROR(EOF):
>             return AVERROR_EOS;
>         default:
>             return AVERROR_FAIL;
>     }
> }
> 
> Detailed error code could be obtained from new API av_get_error(),
> as errno/WSAGetLastError() does.

This looks terrible, and also AVERROR(EOF) makes no sense (EOF is
returned by fgetc() and the likes, it's not used for errno). I'm not
sure what this error code mapping is supposed to achieve, and it has
absolutely nothing to do with AVIO errors, unless I'm misunderstanding
you. I maintain that av_read_frame() has absolutely nothing to do with
AVIO, other than the fact that sometimes AVIO error codes are returned
by it (which is awful and makes everything confusing and also makes the
error codes completely meaningless, but business as usual).