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

[wm4]

On Tue, 14 Apr 2015 11:24:23 +0800
Zhang Rui <bbcallen at gmail.com> wrote:

> 2015-04-14 1:09 GMT+08:00 wm4 <nfxjfg at googlemail.com>:
> > 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.)
> 
> "verbose level by the user application" is the only concern here.
> It has nothing to do with avio itself.

What does "verbose level by the user application" mean? av_log messages?

> I agree with you on "error_count is a good idea".
> 
> >> 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.
> 
> Concern only about "verbose level by the user application", too.
> 
> Actually, It is a format (AVFormatContext), but not an avio (AVIOContext)
> which reads from another format (AVFormatContext), for hls, concatdec situation.
> The error/error_count field of the internal AVIOContext
> is simply ignored without being passed along.
> 
> Whatever, it's not a serious problem, but only some opinion about
> "verbose" idea.
> 
> It does have nothing to do with avio. (Maybe kind of off topic).
> 
> >>
> >> 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
> 
> Concern about "verbose level by the user application", again.
> 
> I want to let av_read_frame() tell us (user application) what to do next,
> by returning limited code, at least limited on special situation
> which need more handling, including
> AV_ERROR_EAGIN;
> AV_ERROR_END_OF_STREAM_YOU_NAME_IT;
> AV_ERROR_EXIT; // interrupted with interrupt call back.

Yes, this sounds like a good idea. It's really how it should work.

> as most api does, e.g., socket, posix.
> 
> After that we will not bother with unmentioned return code,
> which is "awful", "confusing" and "meaningless" as you said.
> 
> The error code mapping is trying to make demuxer's "business as usual".
> However, all demuxers should take the responsibility
> of returning correct code, sooner or later.