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

[Baptiste Coudurier]

Hi

Michael Niedermayer wrote:
>>>> [...]
>>>>
>>>> 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
> 

But not the cleanest.

>> Why not matching both
>> cases 
> 
> messy
> 

replacing == with ff_match_codec_tag would not be that messy.

>> or use sub_id 
> 
> sub_id isnt a uppercased codec_tag, and theres stream_codec_tag too ...
> 

declaring a sub_id for old divx/whatever mpeg4 flavour is cleaner IMHO,
and avi demuxer could set it.

>> or having (avi ?) demuxer converting it to upper case ?
> 
> the demuxer should export what is in the file not mess with it
> 

Same for codec, I wish it was that simple. mov is a good example.
I feel that workaround was added to recover avi demuxer failure, and
were moved to places where they didnt belong to.

> 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

Well, values should be kept in their context (container) IMHO and trying
to convert them in a "generic" way is sometimes harmfull and/or wrong.
Width/height is an example, but I see pts/dts also (pkt duration in mov
when stream copy, dts always starting from 0)

Do you really think taking avi as reference is a good thing ? IMHO it is
not a good solution.

Also, there is a solution to let decoder override parameters if it can
better detect them, like it's being done with aac/mp3on4 sample rate,
channels in mov.

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

What is the desired behaviour ? Respect quicktime behaviour ? Which is
the reference player for mov files. Also, when stream copy from mov to
mov it is better to keep same width and height. So I don't really agree
to not set them.

Im sometimes lost considering whole ffmpeg project. My question is what
is the intended behaviour of lavc/lavf outside ffmpeg scope.
Particularly the stream_copy case. What would be the reference player
and is behaviour ? ffplay ? mplayer ?
Shouldn't we consider stream copy out of lavf scope ? And let the main
prog to always set correct values (pts/dts/duration/width/height) ?

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