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

[Baptiste Coudurier]

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.

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

If the user is trying to rewind and play again.

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

Your path is ugly and messy IMHO.
And no they don't diverge further, return -1 is explicit, ie stop
demuxing, it's over.

Reordering the code so you avoid incrementing is ok, nonetheless.
If you want to try.

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

It is an argument for both IMHO.

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

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