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

[Måns Rullgård]

Rudolf Polzer <divVerent at alientrap.org> writes:

> 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.

I'm not insisting you do the work to support it properly, nor am I
rejecting this patch outright.  I am merely requesting that we pause and
consider the best solution possible today, which may be a step towards
what we want as the ultimate goal.  This could be as simple as adding a
generic flag indicating that time base != frame rate, for instance.

-- 
M?ns Rullg?rd
mans at mansr.com