[FFmpeg-devel] [PATCH] Implement in lavc a flag which makes avcodec_open() to choose the best framerate

[Stefano Sabatini]

On date Sunday 2008-08-31 17:09:12 +0200, Michael Niedermayer encoded:
> On Sun, Aug 31, 2008 at 01:19:31PM +0200, Stefano Sabatini wrote:
> > On date Sunday 2008-08-31 00:11:03 +0200, Michael Niedermayer encoded:
> > [...]
> > > I think we should add a function that takes a list of AVRational and a AVRational
> > > and finds the closest match for that. Its return value should indicate if the
> > > match is exact or approximate
> > > Such function might be usefull for other things as well ...
> > 
> > Yes it's the best solution I can see, it's even simpler
> > application-level-wise, I would like to have thought that!
> >  
> > I also thought about to return the comparation code rather than an
> > "exact" flag like in av_reduce(), I slightly prefer the attached
> > solution but let me know.
> 
> what about returning the actually found value, the user could then run
> av_cmp_q() over it himself when he needs.

Mmh, OK it also simplify the interface.

> > As for the name of the flag to implement in ffmpeg, I'm aware that
> > -choose_best_framerate may sound too long, suggestions are welcome.
> 
> Id say always pick the best framerate that the codec supports, and allow
> the user with -force-fps to override it.

OK for it, this has also the advantage that doesn't break backward compatibility.

> [...]
> > Index: libavutil/rational.h
> > ===================================================================
> > --- libavutil/rational.h	(revision 15120)
> > +++ libavutil/rational.h	(working copy)
> > @@ -113,4 +113,13 @@
> >   */
> >  AVRational av_d2q(double d, int max) av_const;
> >  
> > +/**
> > + * Finds in \p q_list the rational nearest to \p q.
> 
> Finds the nearest value in q_list to q.
> 
> 
> > + * @param nearest_q the pointer to the rational where to copy the
> > + * nearest rational to \p q found
> > + * @param q_list NULL-terminated list of rationals

I realized that it's better to have a {0, 0} terminated list, like
those the AVCodec supported_ramerates ones. From the practical point
of view I think they should be exactly the same, that is I think that:

{ {1, 1}, {2, 2}, NULL }

should be equivalent to:

{ {1, 1}, {2, 2}, { 0, 0} }

for most compilers, anyway in the doubt I preferred the {0, 0}
termination in the documentation.

> > + * @return 1 if the found rational is equivalent to \p q, 0 otherwise
> > + */
> > +int av_find_nearest_q(AVRational *nearest_q, AVRational q, AVRational* q_list);
> > +
> >  #endif /* AVUTIL_RATIONAL_H */
> > Index: libavutil/rational.c
> > ===================================================================
> > --- libavutil/rational.c	(revision 15119)
> > +++ libavutil/rational.c	(working copy)
> > @@ -101,3 +101,60 @@
> >  
> >      return a;
> >  }
> > +
> > +int av_find_nearest_q(AVRational *nearest_q, AVRational q, AVRational* q_list)
> > +{
> > +    AVRational *p= q_list;
> 
> useless temporary variable
> 
> 
> > +    AVRational best_error= (AVRational){INT_MAX, 1};
> 
> > +    nearest_q->num = nearest_q->den = 0;
> 
> unneeded
> 
> 
> > +    for(; p->den!=0; p++){
> 
> the != 0 is superflous
> 
> 
> > +        AVRational error= av_sub_q(q, *p);
> > +        if(error.num <0) error.num *= -1;
> > +        if(av_cmp_q(error, best_error) < 0){
> > +            best_error= error;
> 
> > +            nearest_q->num = p->num;
> > +            nearest_q->den = p->den;
> 
> *nearest_q= *p

Fixed all.

> > +        }
> > +    }
> > +    return !av_cmp_q(*nearest_q, q);
> > +}
> > +
> 
> > +#if TEST
> > +int main (void) {
> > +
> > +    AVRational list_q[] = {
> > +        {24000, 1001},
> > +        {   24,    1},
> > +        {   25,    1},
> > +        {30000, 1001},
> > +        {   30,    1},
> > +        {   50,    1},
> > +        {60000, 1001},
> > +        {   60,    1},
> > +        // Xing's 15fps: (9)
> > +        {   15,    1},
> > +        // libmpeg3's "Unofficial economy rates": (10-13)
> > +        {    5,    1},
> > +        {   10,    1},
> > +        {   12,    1},
> > +        {   15,    1},
> > +        {    0,    0},
> > +    };
> > +
> > +    AVRational q = { 5, 1};
> > +    AVRational nearest_q;
> > +    int exact= av_find_nearest_q(&nearest_q, q, list_q);
> > +#undef printf
> > +    printf ("q=%d/%d(%f), nearest_q=%d/%d(%f), exact=%d\n",
> > +            q.num, q.den, (double)q.num/q.den,
> > +            nearest_q.num, nearest_q.den, (double)nearest_q.num/nearest_q.den,
> > +            exact);
> > +
> > +    q.num= 42, q.den = 1;
> > +    exact= av_find_nearest_q(&q, q, list_q);
> > +    printf ("q=%d/%d(%f), nearest_q=%d/%d(%f), exact=%d\n",
> > +            q.num, q.den, (double)q.num/q.den,
> > +            q.num, q.den, (double)q.num/q.den,
> > +            exact);
> > +}
> > +#endif /* TEST */
> 
> hmm, i think if you cant come up with a shorter self test then drop it.

OK, I'm keeping it for practical and pedagogical reasons, I'm going to
remove it before the commit.

Regards.
-- 
FFmpeg = Furious & Fostering Multimedia Portable Exuberant Game
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implement-nearest-q-01.patch
Type: text/x-diff
Size: 2431 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080831/1092fbce/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implement-choose-best-framerate-01.patch
Type: text/x-diff
Size: 2157 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080831/1092fbce/attachment-0001.patch>