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

[Zhang Rui]

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.

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.

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.