[FFmpeg-devel] [PATCH] make stream-switching work with MOV demuxer
Baptiste Coudurier
baptiste.coudurier
Wed Jun 24 08:38:09 CEST 2009
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
More information about the ffmpeg-devel
mailing list