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

[Michael Niedermayer]

On Sun, Jan 27, 2008 at 11:19:11PM +0100, Baptiste Coudurier wrote:
> 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.

the API isnt the problem
its that you need all this information during writing the header
you cant just update it randomly with AVPackets


> 
> > it creates race conditions with threads
> 
> No.

If you set a RESET_CODEC flag in an AVPacket and update
codec_id width/height/extradata in AVCodecContext from the demuxer then the
decoder which can run in a seperate thread (see ffplay) will have these
values change below its fingers while still having older AVPackets to
decode

so, there is a race condition


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

its not pointless to remux one of the streams (for example the svq1 one
droping the rpza)
with multiple AVStreams thats just works


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

you want to feed the svq1 in the rpza decoder? if no you either 
A. close it (will break following P frames)
B. keep several codecs open per stream


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

So you are designing APIs based on guessing? Why dont you try it?
Its a matter of hacking an existing mov so that a frame in the
middle of lets say a svq1 stream is a jpeg, if the following P frame
isnt black/green then QT preserves the codec state.


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

The problem is nothing these days is specific to any container
Just think of mpeg4, it can have multiple objects.
Its a more general case then mov switch per frame, mpeg4 allows
several streams and combines them, this is similar to mov and should
use the same API. Sadly your design would not be capable to handle it

and yes multiple object mpeg4 and scalability was what i had in mind
when suggested the multple AVStream solution, if these were implemented
the mov substreams would naturally fall into it

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20080128/d39d07d4/attachment.pgp>