| Message ID | 16133da8-ce0a-10da-c32c-dc43fdc4e10c@jkqxz.net |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Mark Thompson > Sent: Monday, May 6, 2019 23:21 > To: FFmpeg development discussions and patches <ffmpeg- > devel@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.
On 09/05/2019 03:46, Fu, Linjie wrote: >> -----Original Message----- >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf >> Of Mark Thompson >> Sent: Monday, May 6, 2019 23:21 >> To: FFmpeg development discussions and patches <ffmpeg- >> devel@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(-) > > 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) Applied. Thanks! - Mark
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;