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

[Reimar Döffinger]

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.
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
2) the documentation does not contradict such use
and thus the mov demuxer relies on an undocumented and IMO unreasonable
assumption for correctness.

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

> >>      /* 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! 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.

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