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

[Reimar Döffinger]

On Tue, Jun 23, 2009 at 11:38:09PM -0700, Baptiste Coudurier wrote:
> Reimar D?ffinger wrote:
> > On Tue, Jun 23, 2009 at 01:06:58PM -0700, Baptiste Coudurier wrote:
> >> Reimar D?ffinger wrote:
> >>> On Tue, Jun 23, 2009 at 09:19:38PM +0200, Reimar D?ffinger wrote:
> >>>> On Tue, Jun 23, 2009 at 11:59:21AM -0700, Baptiste Coudurier wrote:
> >>>>> I think discard variable can be avoided by checking
> >>>>> s->streams[sc->ffindex]->discard.
> >>>>>
> >>>>> We could even declare AVStream *st = s->streams[sc->ffindex] when sample
> >>>>> is found and factore because it is already done when computing duration.
> >>>>>
> >>>>> We could even get rid of sc->ffindex by choosing AVStream in the loop
> >>>>> and use ->priv_data, but well that can be done separately.
> >>>> I think it is uglier, but here it is.
> >>> Ok, I think the real issue is that things that belong together
> >>> (incrementing current_sample and incrementing ctts_sample) are done in
> >>> different places.
> >>> I also think it is wrong that on read error only current_sample is
> >>> incremented.
> >> I'm not sure, but if read error happens, it's over currently (return
> >> -1), so the ctts incrementation should not be needed anyway.
> > 
> > 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
>      /* 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.
I agree that with the current API this is a really stupid thing in
almost all cases, but if AVERROR_EOF was used correctly, it would
be sensible that an application reading from unreliable media would
try to continue demuxing on AVERROR_EIO.
And above test shows that it would indeed work with the mov demuxer -
except that with each read error ctts_index would be further off.

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

> > The idea of this patch is to have a function that increments the
> > position in the given stream and make sure that it is called always.
> 
> I find it ugly and messy. Thanks for your understanding.

Any part specifically btw.? The extra function, the goto, ...?