[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