[FFmpeg-devel] [PATCH] Detect CFR/VFR in libx264.c for proper i_fps_*

[Rudolf Polzer]

On Fri, Feb 11, 2011 at 08:30:21PM +0000, M?ns Rullg?rd wrote:
> Rudolf Polzer <divVerent at alientrap.org> writes:
> 
> > This change adds a detection of constant/variable frame rate by the same
> > heuristics as libavformat/utils.c already uses (in performing or refusing
> > frame duration calculation). The heuristics is basically to check whether
> > the time base is larger than 1ms, and assume CFR then, VFR otherwise.
> >
> > Without it, x264 estimates a huge macroblock rate, and thus forces H.264 level
> > 5.1 even if the input could perfectly fine pass as e.g. level 2.1.
> >
> > Signed-off-by: Rudolf Polzer <divVerent at alientrap.org>
> > ---
> >  libavcodec/libx264.c |   23 +++++++++++++++++++++--
> >  1 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> > index 0dad5cd..02f7aca 100644
> > --- a/libavcodec/libx264.c
> > +++ b/libavcodec/libx264.c
> > @@ -217,8 +217,27 @@ static av_cold int X264_init(AVCodecContext *avctx)
> >      x4->params.i_height             = avctx->height;
> >      x4->params.vui.i_sar_width      = avctx->sample_aspect_ratio.num;
> >      x4->params.vui.i_sar_height     = avctx->sample_aspect_ratio.den;
> > -    x4->params.i_fps_num = x4->params.i_timebase_den = avctx->time_base.den;
> > -    x4->params.i_fps_den = x4->params.i_timebase_num = avctx->time_base.num;
> > +    x4->params.i_timebase_den = avctx->time_base.den;
> > +    x4->params.i_timebase_num = avctx->time_base.num;
> > +
> > +    if (avctx->time_base.num * 1000LL > avctx->time_base.den) { // "if time base > 1ms"
> > +        // this is a heuristic to distinugish variable from constant frame rate
> > +        // encoding
> > +        // NOTE: this is not a new hack; compute_frame_duration() in
> > +        // libavformat/utils.c uses the same heuristics
> > +        x4->params.i_fps_num = avctx->time_base.den;
> > +        x4->params.i_fps_den = avctx->time_base.num;
> > +        // not doing VFR
> > +        x4->params.b_vfr_input = 0;
> > +    } else {
> > +        // set this to a sensible but guessed frame rate (x264 source also
> > +        // often uses this rate if none is available, but not in any occasion,
> > +        // so we cannot set these to 0/0)
> > +        x4->params.i_fps_num = 25;
> > +        x4->params.i_fps_den = 1;
> > +        // IMPORTANT: tell x264 rate control that we are doing VFR
> > +        x4->params.b_vfr_input = 1;
> > +    }
> 
> This kind of thing doesn't really belong at the individual codec level.
> An override if the heuristic makes a wrong guess might be nice too.

True, but this would need a design change to ffmpeg, as it would need adding
frame rate information to the structs (ideally, for best usefulness, three
things: a VFR flag, peak fps, and average fps).

My point is, though: this currently breaks VFR encoding with x264 - the one
most useful codec today. I want this fixed, and don't really care much how it
is fixed. I don't know ffmpeg internals (and policies) well enough to make my
own patch "the way you guys want it", so someone else will have to take this on
then.

Best regards,

Rudolf Polzer