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

[Reimar Döffinger]

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?

> Seek will update everything correctly in any case, afterwards.

Which seek?

> This patch break duration computation, and this time IMHO it is ugly and
> messy.

Well, depends on how ugly you consider it that in the current code
sc->current_sample is not the current sample but the next sample for
most of the code, while sc->ctts_index is the current one and not the
next one, and with each read/seek/mov demux error they diverge further.

> The other patch looks ok, and it is not uglier IMHO, it will allow sc =
> msc removal since sc == st->priv_data, and sc->ffindex will no longer be
> needed.

Which is actually a completely independent change and can be applied to
both patches (and should be applied independently anyway), so I can't see
it as an argument for either one.
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.