[FFmpeg-devel] [PATCH] change frame_aspect_ratio to AVRational
Baptiste Coudurier
baptiste.coudurier
Fri Jun 12 19:46:12 CEST 2009
Hi Michael,
On 6/12/2009 8:12 AM, Michael Niedermayer wrote:
> On Thu, Jun 11, 2009 at 09:44:23PM -0700, Baptiste Coudurier wrote:
>> On 6/11/2009 3:34 PM, Michael Niedermayer wrote:
>>> On Thu, Jun 11, 2009 at 03:06:54PM -0700, Baptiste Coudurier wrote:
>>>> 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 ?
>>> hmm, iam not entirely happy with that patch
>>> at least av_reduce() has to be added where its needed
>>> and that needs to take care of cases where a non 0 value becomes
>>> 0 through av_reduce()
>>> maybe we could add a max_sar_component to AVCodec and use that to
>>> reduce in ffmpeg.c ?
>>> It feels correcter, i mean
>>> "lets set 1000/513 and see what comes out, never knowing what really
>>> is stored ..."
>>> vs.
>>> "Ahh codec supports up to 255/255, lets reduce and store that"
>>>
>>> PS: We already have seperate sar in AVStream and AVCodecContext so it
>>> would not affect muxers
>> Well, if there is a way to have a max rational value for
>> "sample_aspect_ratio" in AVOption (255/255),
>
> i think AVOption cant do that yet
Well too bad. Want to improve it ? :)
>> I can implement per codec
>> defaults and min/max using AVOptions * in AVCodec, what do you think ?
>
> I thought simply adding the aspect parameter to ffmpeg.c so it isnt parsed
> via AVOptions would be a simple solution ...
Well I don't see any point adding max_sar_value to AVCodec when
AVOptions exists.
--
Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA
Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer http://www.ffmpeg.org
More information about the ffmpeg-devel
mailing list