> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf > Of Mark Thompson > Sent: Monday, May 6, 2019 23:21 > To: FFmpeg development discussions and patches <ffmpeg- > de...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".