[FFmpeg-devel] [PATCH] vaapi_encode: Refactor encode misc parameter buffer creation

Fu, Linjie linjie.fu at intel.com
Thu May 9 05:46:04 EEST 2019


> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Mark Thompson
> Sent: Monday, May 6, 2019 23:21
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: [FFmpeg-devel] [PATCH] vaapi_encode: Refactor encode misc
> parameter buffer creation
> 
> This removes the use of the nonstandard combined structures, which
> generated some warnings with clang and will cause alignment problems
> with some parameter buffer types.
> ---
> On 27/03/2019 14:18, Carl Eugen Hoyos wrote:
> > Attached patch fixes many warnings when compiling vaapi with clang.
> > Also tested with clang-3.4.
> > ...
> 
> How about this approach instead?  I think something like it is going to be
> needed anyway because of alignment problems with parameter structures
> which aren't yet used.
> 
> - Mark
> 
> 
>  libavcodec/vaapi_encode.c      | 71 ++++++++++++++++++++++++----------
>  libavcodec/vaapi_encode.h      | 23 +++--------
>  libavcodec/vaapi_encode_h264.c |  8 ++--
>  3 files changed, 60 insertions(+), 42 deletions(-)
> 
> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> index 2dda451882..95031187df 100644
> --- a/libavcodec/vaapi_encode.c
> +++ b/libavcodec/vaapi_encode.c
> @@ -103,6 +103,29 @@ static int
> vaapi_encode_make_param_buffer(AVCodecContext *avctx,
>      return 0;
>  }
> 
> +static int vaapi_encode_make_misc_param_buffer(AVCodecContext
> *avctx,
> +                                               VAAPIEncodePicture *pic,
> +                                               int type,
> +                                               const void *data, size_t len)
> +{
> +    // Construct the buffer on the stack - 1KB is much larger than any
> +    // current misc parameter buffer type (the largest is EncQuality at
> +    // 224 bytes).
> +    uint8_t buffer[1024];
> +    VAEncMiscParameterBuffer header = {
> +        .type = type,
> +    };
> +    size_t buffer_size = sizeof(header) + len;
> +    av_assert0(buffer_size <= sizeof(buffer));
> +
> +    memcpy(buffer, &header, sizeof(header));
> +    memcpy(buffer + sizeof(header), data, len);
> +
> +    return vaapi_encode_make_param_buffer(avctx, pic,
> +                                          VAEncMiscParameterBufferType,
> +                                          buffer, buffer_size);
> +}
> +
>  static int vaapi_encode_wait(AVCodecContext *avctx,
>                               VAAPIEncodePicture *pic)
>  {
> @@ -212,10 +235,10 @@ static int vaapi_encode_issue(AVCodecContext
> *avctx,
> 
>      if (pic->type == PICTURE_TYPE_IDR) {
>          for (i = 0; i < ctx->nb_global_params; i++) {
> -            err = vaapi_encode_make_param_buffer(avctx, pic,
> -                                                 VAEncMiscParameterBufferType,
> -                                                 (char*)ctx->global_params[i],
> -                                                 ctx->global_params_size[i]);
> +            err = vaapi_encode_make_misc_param_buffer(avctx, pic,
> +                                                      ctx->global_params_type[i],
> +                                                      ctx->global_params[i],
> +                                                      ctx->global_params_size[i]);
>              if (err < 0)
>                  goto fail;
>          }
> @@ -1034,14 +1057,14 @@ int ff_vaapi_encode2(AVCodecContext *avctx,
> AVPacket *pkt,
>      return AVERROR(ENOSYS);
>  }
> 
> -static av_cold void vaapi_encode_add_global_param(AVCodecContext
> *avctx,
> -                                                  VAEncMiscParameterBuffer *buffer,
> -                                                  size_t size)
> +static av_cold void vaapi_encode_add_global_param(AVCodecContext
> *avctx, int type,
> +                                                  void *buffer, size_t size)
>  {
>      VAAPIEncodeContext *ctx = avctx->priv_data;
> 
>      av_assert0(ctx->nb_global_params < MAX_GLOBAL_PARAMS);
> 
> +    ctx->global_params_type[ctx->nb_global_params] = type;
>      ctx->global_params     [ctx->nb_global_params] = buffer;
>      ctx->global_params_size[ctx->nb_global_params] = size;
> 
> @@ -1575,8 +1598,7 @@ rc_mode_found:
>                     rc_bits_per_second, rc_window_size);
>          }
> 
> -        ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl;
> -        ctx->rc_params.rc = (VAEncMiscParameterRateControl) {
> +        ctx->rc_params = (VAEncMiscParameterRateControl) {
>              .bits_per_second    = rc_bits_per_second,
>              .target_percentage  = rc_target_percentage,
>              .window_size        = rc_window_size,
> @@ -1591,7 +1613,9 @@ rc_mode_found:
>              .quality_factor     = rc_quality,
>  #endif
>          };
> -        vaapi_encode_add_global_param(avctx, &ctx->rc_params.misc,
> +        vaapi_encode_add_global_param(avctx,
> +                                      VAEncMiscParameterTypeRateControl,
> +                                      &ctx->rc_params,
>                                        sizeof(ctx->rc_params));
>      }
> 
> @@ -1600,12 +1624,13 @@ rc_mode_found:
>                 "initial fullness %"PRId64" bits.\n",
>                 hrd_buffer_size, hrd_initial_buffer_fullness);
> 
> -        ctx->hrd_params.misc.type = VAEncMiscParameterTypeHRD;
> -        ctx->hrd_params.hrd = (VAEncMiscParameterHRD) {
> +        ctx->hrd_params = (VAEncMiscParameterHRD) {
>              .initial_buffer_fullness = hrd_initial_buffer_fullness,
>              .buffer_size             = hrd_buffer_size,
>          };
> -        vaapi_encode_add_global_param(avctx, &ctx->hrd_params.misc,
> +        vaapi_encode_add_global_param(avctx,
> +                                      VAEncMiscParameterTypeHRD,
> +                                      &ctx->hrd_params,
>                                        sizeof(ctx->hrd_params));
>      }
> 
> @@ -1619,11 +1644,13 @@ rc_mode_found:
>      av_log(avctx, AV_LOG_VERBOSE, "RC framerate: %d/%d (%.2f fps).\n",
>             fr_num, fr_den, (double)fr_num / fr_den);
> 
> -    ctx->fr_params.misc.type = VAEncMiscParameterTypeFrameRate;
> -    ctx->fr_params.fr.framerate = (unsigned int)fr_den << 16 | fr_num;
> -
> +    ctx->fr_params = (VAEncMiscParameterFrameRate) {
> +        .framerate = (unsigned int)fr_den << 16 | fr_num,
> +    };
>  #if VA_CHECK_VERSION(0, 40, 0)
> -    vaapi_encode_add_global_param(avctx, &ctx->fr_params.misc,
> +    vaapi_encode_add_global_param(avctx,
> +                                  VAEncMiscParameterTypeFrameRate,
> +                                  &ctx->fr_params,
>                                    sizeof(ctx->fr_params));
>  #endif
> 
> @@ -1885,10 +1912,12 @@ static av_cold int
> vaapi_encode_init_quality(AVCodecContext *avctx)
>              quality = attr.value;
>          }
> 
> -        ctx->quality_params.misc.type = VAEncMiscParameterTypeQualityLevel;
> -        ctx->quality_params.quality.quality_level = quality;
> -
> -        vaapi_encode_add_global_param(avctx, &ctx->quality_params.misc,
> +        ctx->quality_params = (VAEncMiscParameterBufferQualityLevel) {
> +            .quality_level = quality,
> +        };
> +        vaapi_encode_add_global_param(avctx,
> +                                      VAEncMiscParameterTypeQualityLevel,
> +                                      &ctx->quality_params,
>                                        sizeof(ctx->quality_params));
>      }
>  #else
> diff --git a/libavcodec/vaapi_encode.h b/libavcodec/vaapi_encode.h
> index 44a8db566e..80f6c680b4 100644
> --- a/libavcodec/vaapi_encode.h
> +++ b/libavcodec/vaapi_encode.h
> @@ -244,28 +244,17 @@ typedef struct VAAPIEncodeContext {
> 
>      // Global parameters which will be applied at the start of the
>      // sequence (includes rate control parameters below).
> -    VAEncMiscParameterBuffer *global_params[MAX_GLOBAL_PARAMS];
> +    int             global_params_type[MAX_GLOBAL_PARAMS];
> +    const void     *global_params     [MAX_GLOBAL_PARAMS];
>      size_t          global_params_size[MAX_GLOBAL_PARAMS];
>      int          nb_global_params;
> 
>      // Rate control parameters.
> -    struct {
> -        VAEncMiscParameterBuffer misc;
> -        VAEncMiscParameterRateControl rc;
> -    } rc_params;
> -    struct {
> -        VAEncMiscParameterBuffer misc;
> -        VAEncMiscParameterHRD hrd;
> -    } hrd_params;
> -    struct {
> -        VAEncMiscParameterBuffer misc;
> -        VAEncMiscParameterFrameRate fr;
> -    } fr_params;
> +    VAEncMiscParameterRateControl rc_params;
> +    VAEncMiscParameterHRD        hrd_params;
> +    VAEncMiscParameterFrameRate   fr_params;
>  #if VA_CHECK_VERSION(0, 36, 0)
> -    struct {
> -        VAEncMiscParameterBuffer misc;
> -        VAEncMiscParameterBufferQualityLevel quality;
> -    } quality_params;
> +    VAEncMiscParameterBufferQualityLevel quality_params;
>  #endif
> 
>      // Per-sequence parameter structure (VAEncSequenceParameterBuffer*).
> diff --git a/libavcodec/vaapi_encode_h264.c
> b/libavcodec/vaapi_encode_h264.c
> index 4cf99d7c78..d1427112ea 100644
> --- a/libavcodec/vaapi_encode_h264.c
> +++ b/libavcodec/vaapi_encode_h264.c
> @@ -471,9 +471,9 @@ static int
> vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
>              (ctx->va_bit_rate >> hrd->bit_rate_scale + 6) - 1;
> 
>          hrd->cpb_size_scale =
> -            av_clip_uintp2(av_log2(ctx->hrd_params.hrd.buffer_size) - 15 - 4, 4);
> +            av_clip_uintp2(av_log2(ctx->hrd_params.buffer_size) - 15 - 4, 4);
>          hrd->cpb_size_value_minus1[0] =
> -            (ctx->hrd_params.hrd.buffer_size >> hrd->cpb_size_scale + 4) - 1;
> +            (ctx->hrd_params.buffer_size >> hrd->cpb_size_scale + 4) - 1;
> 
>          // CBR mode as defined for the HRD cannot be achieved without filler
>          // data, so this flag cannot be set even with VAAPI CBR modes.
> @@ -488,8 +488,8 @@ static int
> vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
> 
>          // This calculation can easily overflow 32 bits.
>          bp->nal.initial_cpb_removal_delay[0] = 90000 *
> -            (uint64_t)ctx->hrd_params.hrd.initial_buffer_fullness /
> -            ctx->hrd_params.hrd.buffer_size;
> +            (uint64_t)ctx->hrd_params.initial_buffer_fullness /
> +            ctx->hrd_params.buffer_size;
>          bp->nal.initial_cpb_removal_delay_offset[0] = 0;
>      } else {
>          sps->vui.nal_hrd_parameters_present_flag = 0;
> --
> 2.20.1

LGTM, for separating Misc parameter structure(they are separated in MSDK too)
This is more robust for the features with alignment issues.(ROI, trellis, max frame size as far as I know)

Thanks.


More information about the ffmpeg-devel mailing list