[FFmpeg-devel] [PATCH] vaapi_encode: Fix the vaapi h264 encoder VBR/CBR metadata setting error.

[Mark Thompson]

On 13/04/17 13:34, Jun Zhao wrote:
> From 1fa48b45fe962d8c342d158d9c16ce24139ffd84 Mon Sep 17 00:00:00 2001
> From: Jun Zhao <jun.zhao at intel.com>
> Date: Thu, 13 Apr 2017 20:07:10 +0800
> Subject: [PATCH] vaapi_encode: Fix the vaapi h264 encoder VBR/CBR metadata
>  setting error.
> 
> before this fix, mediainfo check the bit rate control mode metadata info
> is wrong.
> 
> Reproduce step:
> encode with CBR or VBR mode like this:
> ./ffmpeg -y -hwaccel vaapi -vaapi_device /dev/dri/renderD128 \
> -hwaccel_output_format vaapi -i input.mp4 -an -c:v h264_vaapi \
> -maxrate 5M -b:v 5M output_cbr.mp4
> 
> ./ffmpeg -y -hwaccel vaapi -vaapi_device /dev/dri/renderD128 \
> -hwaccel_output_format vaapi -i input.mp4 -an -c:v h264_vaapi \
> -maxrate 6M -b:v 5M output_cbr.mp4
> 
> then use the mediainfo check the bit rate control mode.
> 
> Signed-off-by: Jun Zhao <jun.zhao at intel.com>

Not yet generating the HRD and timing information in VBR mode was deliberate, because I don't know enough about the conformance properties of the new proprietary rate controller which the Intel driver has switched to.  The old CBR mode was straightforward to verify from the source code, and it seemed reasonable to assume that the new CBR mode would be too given the constraints on it (and also because existing versions of lavc couldn't be modified).

Can you say anything about the HRD conformance of the Intel driver here, or was this patch purely for the metadata output (which may not actually match the stream, hence my concern)?

Only the Intel driver is relevant to this as far as I know - the Mesa driver does not support the packed headers necessary for this information to be included in the stream.


> ---
>  libavcodec/vaapi_encode_h264.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c
> index 92e2955..47d0c94 100644
> --- a/libavcodec/vaapi_encode_h264.c
> +++ b/libavcodec/vaapi_encode_h264.c
> @@ -760,7 +760,7 @@ static int vaapi_encode_h264_write_extra_header(AVCodecContext *avctx,
>      char tmp[256];
>      size_t header_len;
>  
> -    if (index == 0 && ctx->va_rc_mode == VA_RC_CBR) {
> +    if (index == 0 && (ctx->va_rc_mode == VA_RC_CBR || ctx->va_rc_mode == VA_RC_VBR)) {
>          *type = VAEncPackedHeaderH264_SEI;
>  
>          init_put_bits(&pbc, tmp, sizeof(tmp));
> @@ -868,7 +868,7 @@ static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
>              mseq->fixed_frame_rate_flag = 0;
>          }
>  
> -        if (ctx->va_rc_mode == VA_RC_CBR) {
> +        if (ctx->va_rc_mode == VA_RC_CBR || ctx->va_rc_mode == VA_RC_VBR) {
>              priv->send_timing_sei = 1;
>              mseq->nal_hrd_parameters_present_flag = 1;
>  
> @@ -886,8 +886,7 @@ static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
>              mseq->cpb_size_value_minus1[0] =
>                  (ctx->hrd_params.hrd.buffer_size >> mseq->cpb_size_scale + 4) - 1;
>  
> -            // CBR mode isn't actually available here, despite naming.
> -            mseq->cbr_flag[0] = 0;
> +            mseq->cbr_flag[0] = (ctx->va_rc_mode == VA_RC_CBR ? 1 : 0);

As the comment notes, the necessary CBR mode isn't actually available.  Drivers only offer "soft" CBR, which can result in HRD overflow; this specifies "hard" CBR, so if you want to set this flag you would need to insert filler NAL units as well.

Thanks,

- Mark