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

[Michael Niedermayer]

On Fri, Jun 12, 2009 at 10:46:12AM -0700, Baptiste Coudurier wrote:
> 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 ? :)

my todo list is too long thus not this year ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- 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-devel/attachments/20090612/45e66474/attachment.pgp>