[Mesa-dev] [PATCH] st/va encode handle ntsc framerate rate control

[Mark Thompson]

On 26/01/17 18:26, Andy Furniss wrote:
> Tested with ffmpeg and gst-vaapi. Without this bits per
> frame is set way too low.
> 
> Signed-off-by: Andy Furniss <adf.lists at gmail.com>
> ---
>  src/gallium/state_trackers/va/picture.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/src/gallium/state_trackers/va/picture.c b/src/gallium/state_trackers/va/picture.c
> index 82584ea..a024437 100644
> --- a/src/gallium/state_trackers/va/picture.c
> +++ b/src/gallium/state_trackers/va/picture.c
> @@ -119,14 +119,30 @@ getEncParamPreset(vlVaContext *context)
>     context->desc.h264enc.rate_ctrl.fill_data_enable = 1;
>     context->desc.h264enc.rate_ctrl.enforce_hrd = 1;
>     context->desc.h264enc.enable_vui = false;
> -   if (context->desc.h264enc.rate_ctrl.frame_rate_num == 0)
> -      context->desc.h264enc.rate_ctrl.frame_rate_num = 30;
> -   context->desc.h264enc.rate_ctrl.target_bits_picture =
> -      context->desc.h264enc.rate_ctrl.target_bitrate / context->desc.h264enc.rate_ctrl.frame_rate_num;
> -   context->desc.h264enc.rate_ctrl.peak_bits_picture_integer =
> -      context->desc.h264enc.rate_ctrl.peak_bitrate / context->desc.h264enc.rate_ctrl.frame_rate_num;
> -   context->desc.h264enc.rate_ctrl.peak_bits_picture_fraction = 0;
> +   if (context->desc.h264enc.rate_ctrl.frame_rate_num == 0 ||
> +       context->desc.h264enc.rate_ctrl.frame_rate_den == 0) {
> +         context->desc.h264enc.rate_ctrl.frame_rate_num = 30;
> +         context->desc.h264enc.rate_ctrl.frame_rate_den = 1;
> +   }
> +   if (context->desc.h264enc.rate_ctrl.frame_rate_den > 1) {
> +      context->desc.h264enc.rate_ctrl.target_bits_picture =
> +         context->desc.h264enc.rate_ctrl.target_bitrate /
> +         (context->desc.h264enc.rate_ctrl.frame_rate_num /
> +         context->desc.h264enc.rate_ctrl.frame_rate_den + 1);

This is still doing more rounding than necessary?  (Consider 0.5fps as 1/2, say.)  Don't you want:

context->desc.h264enc.rate_ctrl.target_bits_picture =
    (context->desc.h264enc.rate_ctrl.target_bitrate *
     context->desc.h264enc.rate_ctrl.frame_rate_den) /
    context->desc.h264enc.rate_ctrl.frame_rate_num;

(And no need for any extra +1 or conditional, because you've already made sure that neither of them are zero.)

> +      context->desc.h264enc.rate_ctrl.peak_bits_picture_integer =
> +         context->desc.h264enc.rate_ctrl.peak_bitrate /
> +         (context->desc.h264enc.rate_ctrl.frame_rate_num /
> +         context->desc.h264enc.rate_ctrl.frame_rate_den + 1);

Similarly this one.

> +   } else {
> +      context->desc.h264enc.rate_ctrl.target_bits_picture =
> +         context->desc.h264enc.rate_ctrl.target_bitrate /
> +         context->desc.h264enc.rate_ctrl.frame_rate_num;
> +      context->desc.h264enc.rate_ctrl.peak_bits_picture_integer =
> +         context->desc.h264enc.rate_ctrl.peak_bitrate /
> +         context->desc.h264enc.rate_ctrl.frame_rate_num;
> +   }

Doing that would also avoid needing the if at all.

>  
> +   context->desc.h264enc.rate_ctrl.peak_bits_picture_fraction = 0;
>     context->desc.h264enc.ref_pic_mode = 0x00000201;
>  }
>  
> @@ -362,7 +378,7 @@ handleVAEncSequenceParameterBufferType(vlVaDriver *drv, vlVaContext *context, vl
>        context->gop_coeff = VL_VA_ENC_GOP_COEFF;
>     context->desc.h264enc.gop_size = h264->intra_idr_period * context->gop_coeff;
>     context->desc.h264enc.rate_ctrl.frame_rate_num = h264->time_scale / 2;
> -   context->desc.h264enc.rate_ctrl.frame_rate_den = 1;
> +   context->desc.h264enc.rate_ctrl.frame_rate_den = h264->num_units_in_tick;
>     return VA_STATUS_SUCCESS;
>  }

Otherwise LGTM :)

Thanks,

- Mark