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

Zhang Rui bbcallen at gmail.com
Thu Apr 16 09:25:06 CEST 2015


2015-04-15 18:38 GMT+08:00 wm4 <nfxjfg at googlemail.com>:
> 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?

Never mind, it is about whether we make the error_count field public
to user application.

>> 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.

The difficult part is keeping "business as usual".
Since AVERROR(EOF) is already a "De facto standard",
AV_ERROR_END_OF_STREAM_YOU_NAME_IT
would break many user applications.

I will submit a new RFC patch for the new topic.

>> 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.


More information about the ffmpeg-devel mailing list