[Ffmpeg-devel] Re: [PATCH] mpegvideo remove fourcc upper case conversion

[Michael Niedermayer]

Hi

On Tue, Aug 22, 2006 at 09:47:19PM +0200, Baptiste Coudurier wrote:
> Hi
> 
> Michael Niedermayer wrote:
> > Hi
> > 
> > On Tue, Aug 22, 2006 at 06:52:19PM +0200, Baptiste Coudurier wrote:
> >> Hi
> >>
> >> Here is a patch to remove uppercase conversion of fourcc. That cause
> >> conversion if mpeg essence is stored in container which already have a
> >> fourcc (mov), and IMHO a codec should not alter fourcc.
> > 
> > rejected, this will break decoding of many old divx and xvid files
> > grep for codec_tag ...
> 
> IMHO, like I said codec should not alter fourcc. 

yes, but its the simplest solution


> Why not matching both
> cases 

messy


> or use sub_id 

sub_id isnt a uppercased codec_tag, and theres stream_codec_tag too ...


> or having (avi ?) demuxer converting it to upper case ?

the demuxer should export what is in the file not mess with it

if you really care about this, then add a (stream_)codec_tag to MpegEncContext
and uppercase that and change all codecs to use it instead of AVCodecContext


> 
> > btw, speaking of breaking stuff, did you test that the unconditional
> > setting of AVCodecContext.width/height in the mov/mp4 demuxer has no 
> > negative effects if they are wrong? (they are often wrong for mpeg4)
> > 
> > [...]
> 
> I thought lavc would always override width and height for mpeg4, and
> having tested on all files I got, I saw no problems. Do you know a file
> which cause problem ?

just set them incorrectly by hand like:
           st->codec->width = 987;get_be16(pb); /* width */
           st->codec->height = 123; get_be16(pb); /* height */

then try ffmpeg -i with a mp4/mov of your choice:
it shows:
Stream #0.0(eng): Video: mpeg4, yuv420p, 987x123, 30.00 fps(r)

or try ffmpeg -i x.mp4 -vcodec copy x.avi
the avi file will have the incorrect values stored in its header
the validity of such values in mov is debateable (croping ...) but
in other formats its illegal and will break playback on many players


> 
> Also why not setting them ? What would be the policy ? Not setting
> fields which can cause problem with some maybe broken files ? I don't
> know... but that is not only related to width and height IMHO. I admit I
> don't like workarounds much :/

the problem is that in mov setting nonsense width/height to force croping
or scaling might not be illegal, i dont know ... but ive seen plenty of 
files which used that (the recent h261 file is one), in all other formats
like avi you cannot just set the width/height to random values
some formats like mpeg1/2 / h264 even have their own seperate fields to specify
some rectangle within the image which the user is supposed to see (AVPanScan)
but no container i know of except mov uses the one and only fields for 
width and height for such things

so simply ignoring the width/height in mov completely if the codec can set its
own width/height is the simplest fix, alternatively you could set the
width/height from mov in AVCodecContext.pan_scan and then change
av_find_stream_info() so it puts the width/height from AVCodecContext.pan_scan
in AVCodecContext if the codec fails with a width=height=0

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

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is