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

[Baptiste Coudurier]

Reimar D?ffinger wrote:
> 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

No, an application can try to read again but it won't make it read
sucessfully in any case.
To specify that an application can try to read again there is EAGAIN
return value.

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

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

AVERROR_EOF should be used correctly, at least in mov demuxer.
It will be returned only if no sample are left. For any other reason
IMHO I see currently it must return -1.

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

Yes, if there is any valid reason to not return -1 in the future, it
will be changed.

In the future, AVERROR_EOS could be introduced to mention that one
specific stream is over but others are still available, example when mov
is not interleaved and you have one packet at the end of the file
(timecode track packet). In this case, it must not return -1, but we are
still far from that, and that would not change the ctts case, ie this is
still end of stream.

In our case, I believe a simple patch should fix the problem.

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

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

Well both a bit but I liked the patch using st->discard as it allowed an
extra simplification and I could remove sc->ffindex which has bothered
me since quite some time.

But you are right, a way to reorder the code so current_sample is
incremented at the end would be great. I'll think about it.

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