[FFmpeg-cvslog] r11621 - trunk/libavformat/mov.c

[Baptiste Coudurier]

Michael Niedermayer wrote:
> On Sun, Jan 27, 2008 at 07:29:35PM +0100, Baptiste Coudurier wrote:
>> Michael Niedermayer wrote:
>>> On Sun, Jan 27, 2008 at 03:38:24PM +0100, Baptiste Coudurier wrote:
>>>> Hi,
>>>>
>>>> michael wrote:
>>>>> Author: michael
>>>>> Date: Sat Jan 26 20:50:04 2008
>>>>> New Revision: 11621
>>>>>
>>>>> Log:
>>>>> Select non jpeg if there are multiple substreams.
>>>>>
>>>>>
>>>>> Modified:
>>>>>    trunk/libavformat/mov.c
>>>>>
>>>>> Modified: trunk/libavformat/mov.c
>>>>> ==============================================================================
>>>>> --- trunk/libavformat/mov.c	(original)
>>>>> +++ trunk/libavformat/mov.c	Sat Jan 26 20:50:04 2008
>>>>> @@ -600,8 +600,10 @@ static int mov_read_stsd(MOVContext *c, 
>>>>>          get_be16(pb); /* reserved */
>>>>>          get_be16(pb); /* index */
>>>>>  
>>>>> -        if (st->codec->codec_tag) {
>>>>> -            /* multiple fourcc, just skip for now */
>>>>> +        if (st->codec->codec_tag && st->codec->codec_tag != MKTAG('j', 'p', 'e', 'g')) {
>>>>> +            /* multiple fourcc, we skip jpeg, this isnt correct, we should export it as
>>>>> +               seperate AVStream but this needs a few changes in the mov demuxer, patch
>>>>> +               welcome */
>>>>>              url_fskip(pb, size - (url_ftell(pb) - start_pos));
>>>>>              continue;
>>>>>          }
>>>> This is hackish, 
>>> yes i know
>>>
>>>
>>>> and Im against it. 
>>>> You will have to add all image types
>>>> (png and so on). 
>>> yes i will when i stumble across samples
>>>
>>>
>>>> The proper solution must be implemented, 
>>> well a lot of things must be implemented, now if we had the manpower
>>> time, motivation, ...
>>>
>>>
>>>> and adding
>>>> hacks won't help.
>>> that is the part i strongly disagree with
>>> it does help alot until (if ever) the correct solution is implemented
>> Here you have double standards, please remember the pcm 20 and 24 bits
>> in dvds patch a long time ago. Those files are still unplaybable, still
>> you didn't accept the patch.
> 
> iam not mans, you are mixing people up here
> 
> iam not mpeg-ps maintainer and wouldnt strongly mind a hack in mpeg-ps
> to work around lpcm problems, though the correct place is a bitstream
> filter
> and doing that in a bitstream filter would be trivial its just noone
> copy and pasted the code in one
> you hardly can compare that to designing a new API, rewriting 1/3 of the
> mov demuxer, ffmpeg.c and getting libavfilter into shape
> 
> 
>>> and its a very small isolated change, the correct solution requires large
>>> changes in the mov demuxer and in ffmpeg
>>> correct is to export the individual streams as seperate AVStreams, export
>>> their relation in some not yet existing API and then use the not yet in
>>> svn avfilter to merge them after deocoding,
>> Another way is to make possible the change of codec and notify it
>> through an AVPacket flag. I had a patch handling this correctly.
> 
> sorry, you cannot change the codec in an AVStream this violates the API

API can be extended, like it was many times already.

> it creates race conditions with threads

No.

> breaks ALL remuxing (even to mov)

No it does not break anything, if it's done well.
Now don't even try to remux this stream into any other container not
supporting this feature, this would be pointless, and real stream copy
in mov case is to remux the whole stream in one track, not splitting them.

> and its not the codec its everything that can change including resolution
> 
> also, lets for the moment ignore the issues above
> how is the user app supposed to handle it?

We can discuss about that.

> should it free the codec and open another if the change happens?

Why freeing codec ? You free the codec at the end of the demuxing,
like it's being done atm.

> what happens with seeking? what is in extradata?

Mov seeking must be done at container level, you cannot seek in bytes,
so this does not matter at all.

> 
> also i fear that freeing the codec is wrong, the codec state must be
> preserved from the previous frames

This is just pure guess, and I do guess it's not the case.

> also i say it again, this is absolutely not limited to the first frame
> any random frame in the middle can be a different codec, closing and opening
> codecs will break their state. And expecting the user app to keep several
> codecs per stream open and resolve all that with seeking and different
> resolutions and colorspaces is hardly a solution ...
> 
> also theres a file (resurrection.mov) with RPZA and SVQ1 mixed and i emphasize
> mixed
> heres the qtdump of it to proof this:
>        chunk 1 samples 1 id 1
>        chunk 6 samples 1 id 2
>        chunk 7 samples 3 id 2
>        chunk 325 samples 1 id 2
>        chunk 326 samples 1 id 1
> 

I know that, I have already anwered and patched demuxer to handle those
case, I even patched demuxer to correctly decode these streams, though
only with synchronous demux/decode. Asynchronous decoding needs
notification of when codec change and substream id.

The whole concept in mov is specific to mov and you keep wanting formats
to adapt to your view of libavformat API. I don't.

Maybe having AVCodecContext * in AVStream is just bad, and application
using libavformat don't need it. AVStream exports what's in the
container (width/height/channels/samplerate), and application choose to
set it to AVCodecContext or not. Libavformat just inform which packet
belongs to which track and which substream id.

>> No need for avfilter.
> 
> ohh well, why am i awnsering this

I'll just ignore your attitude.

> look if you have 2 codecs outputing different colorspaces and resolutions
> you need to convert one otherwise you will have some problems with displaying
> them i the same window with most video output drivers
> this is also totally indepandant of AVPackets with flags or several AVStreams
> 
> also above file:
> svq1 (PIX_FMT_YUV410P)
> rpza (PIX_FMT_RGB555)
> 

This is user application problem, not demuxing problem. Besides
Quicktime Player handles that greatly.

> [...]
> 
>>> i dont think its justified to have many mov files unplayable until then
>> It is justified for sake of avoiding ugly hacks like this.
>> Since the beginning those files aren't playbable with lavf,
>> and with any other opensource demuxers I know about.
> 
> mplayers demux_mov.c does play them AFAIK

It does not.

> you could say its luck, but its not

It's not luck, it's just the fact that they consider last entry in stsd,
like some other demuxers do. This is plain broken.

> the first frame is often a jpeg, its less likely in the middle, so the first
> stsd is jpeg, mplayer always uses the last, that simply us more reliable
> than choosing the first blindly.

How often ? I have one where it is a gif and Reimar says some are png.
And some files have a last picture as a jpeg, so choosing blindly the
last is not reliable either. It's just plain broken.

>> One must jump in and implement it correctly, with this ugly hacks nobody
>> will jump in since 'it just works', and this rule applies to every hack
>> you discard for the sake of good implementation IMHO.
> 
> no, if it doesnt "just work" people use another player,

Quicktime Player plays them greatly !

> or they will patch
> the code with 1 or 2 lines to skip the jpeg like in the hack
> noone will volunteerly implement it "cleanly" unless the simlpe hack doesnt
> work but if it doesnt work it also doesnt hurt such implementation attempts
> 

No, this will just make them think that hacks is good, and they will
forget about the issue because 'it somewhat works'.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A.                                    http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312