[FFmpeg-devel] [PATCH] change frame_aspect_ratio to AVRational

[Baptiste Coudurier]

On 6/11/2009 1:14 PM, Michael Niedermayer wrote:
> On Thu, Jun 11, 2009 at 11:36:31AM -0700, Baptiste Coudurier wrote:
>> On 6/11/2009 4:27 AM, Michael Niedermayer wrote:
>>> On Wed, Jun 10, 2009 at 09:23:40PM -0700, Baptiste Coudurier wrote:
>>>> Hi
>>>>
>>>> $subject, mainly to avoid using double which behaves not correctly
>>>> when using 16:9 commandline.
>>> [...]
>>>> @@ -3012,7 +3012,8 @@
>>>>          st->stream_copy = 1;
>>>>          video_enc->codec_type = CODEC_TYPE_VIDEO;
>>>>          video_enc->sample_aspect_ratio =
>>>> -        st->sample_aspect_ratio = av_d2q(frame_aspect_ratio*frame_height/frame_width, 255);
>>>> +        st->sample_aspect_ratio =
>>>> +            av_mul_q(frame_aspect_ratio, (AVRational){ frame_height, frame_width });
>>>>      } else {
>>>>          const char *p;
>>>>          int i;
>>>> @@ -3039,7 +3040,8 @@
>>>>  
>>>>          video_enc->width = frame_width + frame_padright + frame_padleft;
>>>>          video_enc->height = frame_height + frame_padtop + frame_padbottom;
>>>> -        video_enc->sample_aspect_ratio = av_d2q(frame_aspect_ratio*video_enc->height/video_enc->width, 255);
>>>> +        video_enc->sample_aspect_ratio =
>>>> +            av_mul_q(frame_aspect_ratio, (AVRational){ frame_height, frame_width });
>>>>          video_enc->pix_fmt = frame_pix_fmt;
>>>>          st->sample_aspect_ratio = video_enc->sample_aspect_ratio;
>>>>  
>>> previously these where limited to 255/255 now they arent anymore, i think
>>> some codecs dont support arbitrary fractions
>> Ok, quick survey:
>> theora encoder supports rational.
>> h264 supports rational on 16bits.
>> libxvid indeed checks for 255/255.
>> mpeg12 converts back to float and match supported ones.
>> mjpeg is rational on 16bits.
>> h263/mpeg4 sar is rational on 8bits.
>>
>> Ok to add av_reduce to codecs needing it ?
> 
> encoders should not second guess the user
> i mean if they accept a 300/301 then they should either store that or
> if they cannot then fail
> 
> it seems to me the current code is fine, is your patch fixing something
> besides changing float to AVRational ?
> if not we maybe could just keep it as it is

Yes it keeps current par when converting when par > 255.
resolution 720x512 DAR 16:9 PAR 512:405.

mov can store this perfectly, codec par must be equal to stream par
and current code alters both.

Stream #0.0: Video: mpeg2video, yuv422p, 720x512 [PAR 67:53 DAR
3015:1696], q=2-31, 30000 kb/s, 90k tbn, 29.97 tbc

Even with stream copy, mov can store rational on 32bits.
and H.264 supports this exact PAR corresponding to DAR.

Can we please apply the patch ?

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