[FFmpeg-devel] [PATCH] make stream-switching work with MOV demuxer

[Baptiste Coudurier]

Reimar D?ffinger wrote:
> On Wed, Jun 24, 2009 at 12:45:03AM -0700, Baptiste Coudurier wrote:
>> Reimar D?ffinger wrote:
>>> On Tue, Jun 23, 2009 at 11:38:09PM -0700, Baptiste Coudurier wrote:
>>>> Reimar D?ffinger wrote:
>>>>> Why should currrent sample be incremented on read error:
>>>>>     /* must be done just before reading, to avoid infinite loop on sample */
>>>>>     sc->current_sample++;
>>>>> but ctts not? I find it hard to believe that really makes sense.
>>>>> Also what do you mean by "it's over currently"? An application can
>>>>> continue to demux (i.e. call mov_read_packet) even if it failed once,
>>>>> can't it?
>>>> Well it can try but it won't make it read sucessfully in any case.
>>>> When it can try to read again there is EAGAIN.
>>> I don't understand that. Are you confusing "can" and "should"?
>>> I did in the mov demuxer
>> No, an application can try to read again but it won't make it read
>> sucessfully in any case.
> 
> Uh, repeating the same thing you already said is not going to get
> anywhere.
> Particularly since I already made a test and proved it not to be true.

Well you hacked based on an hypothetical case, I hardly call that a proof.

> E.g. if it is on a CD on a sector that is damaged, my test showed that
> simply continuing to demux will finally start to work again once the
> data is outside the damaged area.
> Of course, there is the question if and how this situation should be
> handled, still I think it is enough to show that
> 1) a user application may have reason to behave like this

If instructed so.

> 2) the documentation does not contradict such use
> and thus the mov demuxer relies on an undocumented and IMO unreasonable
> assumption for correctness.

I think every code using libavformat was and is probably still treating
-1 as error and stop reading.

Workarounding the documentation is not reasonable.

>> To specify that an application can try to read again there is EAGAIN
>> return value.
> 
> IMO EAGAIN means that an application _should_ try again, which is
> something different. Also when a demuxer returns EAGAIN I would not
> expect it to have skipped something, so this would not work in the
> example of the broken CD sector above.

Yes, _should_ try to read again. IMHO it's a way to say try to read
again. If you want to nuance it by mentioning some kind of error
happened, I see no reason why we shouldn't try.

>>>>      /* must be done just before reading, to avoid infinite loop on sample */
>>>>      sc->current_sample++;
>>>> +if (sc->current_sample == 1) return -1;
>>> and in MPlayer
>>>> -    if(av_read_frame(priv->avfc, &pkt) < 0)
>>>> -        return 0;
>>>> +    while (av_read_frame(priv->avfc, &pkt) < 0) ;
>>> and except for the first missing keyframe and an endless loop on EOF it
>>> plays just fine.
>> Well, when there is no reasonable reason to return -1, it must not
>> return -1. When it is reasonable to assume that something can be still
>> read demuxer can still return EAGAIN.
> 
> How should FFmpeg as a library know when it is reasonable to assume
> that? It doesn't even know anything about the underlying reading
> protocol!

Protocol errors should be handled in the demuxer itself.
In the case of broken sector av_get_buffer would need to return
something different than error.

> Also my understanding of EAGAIN is that the user should (or
> even "must") call the function again and will get a "normal" result (in
> particular: there was no error), EAGAIN would not be appropriate as meaning
> "well, if you try again, it possibly might work, we will have skipped
> a broken part of the file though".
> Obviously this is something to be discussed and documented more.

Well, yes, this is something to discuss.
We might need a way to instruct the user that something went wrong but
he can try again.

>>>> And no they don't diverge further, return -1 is explicit, ie stop
>>>> demuxing, it's over.
>>> The av_read_frame documentation in avformat.h does not support that
>>> claim, it does not say that av_read_frame may not be called any more
>>> after it returned -1.
>>> Maybe this is what we want, but at least it is not documented and IMHO
>>> it is not obviously the best choice either.
>> That's true, documentation is not clear on this.
>> Now that we have EAGAIN and AVERROR_EOF, I think documentation must be
>> updated and mention that any other value means stop demuxing.
> 
> Well, that is a solution, though personally I'd prefer it if
> applications were allowed to retry at their discretion since it allows
> for more flexibility and personally I consider it bad style anyway if the
> demuxer ends up in an invalid state (more that necessary) on error.

Well strictly speaking, nothing guarantee that demuxer is in good state
after returning an error, considering EAGAIN is not an error.

Now I agree with you and I see no problem ensuring that, it would make
demuxers even more robust, but this needs some extensive testing.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org