[FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up rate control configuration
Xiang, Haihao
haihao.xiang at intel.com
Thu Jun 14 09:07:42 EEST 2018
On Wed, 2018-06-13 at 23:42 +0100, Mark Thompson wrote:
> On 13/06/18 08:03, Xiang, Haihao wrote:
> > On Fri, 2018-06-08 at 00:43 +0100, Mark Thompson wrote:
> > > Query which modes are supported and select between VBR and CBR based
> > > on that - this removes all of the codec-specific rate control mode
> > > selection code.
> > > ---
> > > doc/encoders.texi | 2 -
> > > libavcodec/vaapi_encode.c | 173 ++++++++++++++++++++++++++++-------
> > > ----
> > > -
> > > libavcodec/vaapi_encode.h | 6 +-
> > > libavcodec/vaapi_encode_h264.c | 18 +----
> > > libavcodec/vaapi_encode_h265.c | 14 +---
> > > libavcodec/vaapi_encode_mjpeg.c | 3 +-
> > > libavcodec/vaapi_encode_mpeg2.c | 9 +--
> > > libavcodec/vaapi_encode_vp8.c | 13 +--
> > > libavcodec/vaapi_encode_vp9.c | 13 +--
> > > 9 files changed, 137 insertions(+), 114 deletions(-)
> > >
> > > ...
> > > diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> > > index f4c063734c..5de5483454 100644
> > > --- a/libavcodec/vaapi_encode.c
> > > +++ b/libavcodec/vaapi_encode.c
> > > ...
> > > @@ -1313,44 +1286,144 @@ static av_cold int
> > > vaapi_encode_config_attributes(AVCodecContext *avctx)
> > > static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx)
> > > {
> > > VAAPIEncodeContext *ctx = avctx->priv_data;
> > > - int rc_bits_per_second;
> > > - int rc_target_percentage;
> > > - int rc_window_size;
> > > - int hrd_buffer_size;
> > > - int hrd_initial_buffer_fullness;
> > > + int64_t rc_bits_per_second;
> > > + int rc_target_percentage;
> > > + int rc_window_size;
> > > + int64_t hrd_buffer_size;
> > > + int64_t hrd_initial_buffer_fullness;
> > > int fr_num, fr_den;
> > > + VAConfigAttrib rc_attr = { VAConfigAttribRateControl };
> > > + VAStatus vas;
> > > +
> > > + vas = vaGetConfigAttributes(ctx->hwctx->display,
> > > + ctx->va_profile, ctx->va_entrypoint,
> > > + &rc_attr, 1);
> > > + if (vas != VA_STATUS_SUCCESS) {
> > > + av_log(avctx, AV_LOG_ERROR, "Failed to query rate control "
> > > + "config attribute: %d (%s).\n", vas, vaErrorStr(vas));
> > > + return AVERROR_EXTERNAL;
> > > + }
> > >
> > > - if (avctx->bit_rate > INT32_MAX) {
> > > - av_log(avctx, AV_LOG_ERROR, "Target bitrate of 2^31 bps or "
> > > - "higher is not supported.\n");
> > > + if (rc_attr.value == VA_ATTRIB_NOT_SUPPORTED) {
> > > + av_log(avctx, AV_LOG_VERBOSE, "Driver does not report any "
> > > + "supported rate control modes: assuming constant-
> > > quality.\n");
> >
> > How about logging it as warning message?
>
> What would a user do about it?
>
User may know what is not supported in the driver and he/she might get wrong
result, he/she may file an issue to the driver.
> (Note that it gets logged for JPEG encoding on all versions of the i965 driver
> except the most recent, so if it were a warning it would be seen by everyone
> using those versions.)
>
> > > + ctx->va_rc_mode = VA_RC_CQP;
> > > + return 0;
> > > + }
> > > + if (avctx->flags & AV_CODEC_FLAG_QSCALE ||
> > > + avctx->bit_rate <= 0) {
> > > + if (rc_attr.value & VA_RC_CQP) {
> > > + av_log(avctx, AV_LOG_VERBOSE, "Using constant-quality
> > > mode.\n");
> > > + ctx->va_rc_mode = VA_RC_CQP;
> > > + return 0;
> > > + } else {
> > > + av_log(avctx, AV_LOG_ERROR, "Driver does not support "
> > > + "constant-quality mode (%#x).\n", rc_attr.value);
> > > + return AVERROR(EINVAL);
> > > + }
> > > + }
> > > +
> > > + if (!(rc_attr.value & (VA_RC_CBR | VA_RC_VBR))) {
> > > + av_log(avctx, AV_LOG_ERROR, "Driver does not support any "
> > > + "bitrate-targetted rate control modes.\n");
> > > return AVERROR(EINVAL);
> > > }
> > >
> > > if (avctx->rc_buffer_size)
> > > hrd_buffer_size = avctx->rc_buffer_size;
> > > + else if (avctx->rc_max_rate > 0)
> > > + hrd_buffer_size = avctx->rc_max_rate;
> > > else
> > > hrd_buffer_size = avctx->bit_rate;
> > > - if (avctx->rc_initial_buffer_occupancy)
> > > + if (avctx->rc_initial_buffer_occupancy) {
> > > + if (avctx->rc_initial_buffer_occupancy > hrd_buffer_size) {
> > > + av_log(avctx, AV_LOG_ERROR, "Invalid RC buffer settings: "
> > > + "must have initial buffer size (%d) < "
> > > + "buffer size (%"PRId64").\n",
> > > + avctx->rc_initial_buffer_occupancy, hrd_buffer_size);
> > > + return AVERROR(EINVAL);
> > > + }
> > > hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy;
> > > - else
> > > + } else {
> > > hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4;
> > > + }
> > > +
> > > + if (avctx->rc_max_rate && avctx->rc_max_rate < avctx->bit_rate) {
> > > + av_log(avctx, AV_LOG_ERROR, "Invalid bitrate settings: must have
> > > "
> > > + "bitrate (%"PRId64") <= maxrate (%"PRId64").\n",
> > > + avctx->bit_rate, avctx->rc_max_rate);
> > > + return AVERROR(EINVAL);
> > > + }
> > > +
> > > + if (avctx->rc_max_rate > avctx->bit_rate) {
> > > + if (!(rc_attr.value & VA_RC_VBR)) {
> > > + av_log(avctx, AV_LOG_WARNING, "Driver does not support "
> > > + "VBR mode (%#x), using CBR mode instead.\n",
> > > + rc_attr.value);
> > > + ctx->va_rc_mode = VA_RC_CBR;
> > > + } else {
> > > + ctx->va_rc_mode = VA_RC_VBR;
> > > + }
> > > +
> > > + rc_bits_per_second = avctx->rc_max_rate;
> > > + rc_target_percentage = (avctx->bit_rate * 100) / avctx-
> > > >rc_max_rate;
> >
> > I think rc_target_percentage should be 100 for CBR case.
>
> Yes; fixed.
>
> The rc_bits_per_second value is also wrong in that case (shouldn't be
> rc_max_rate if we can't use it), fixed similarly.
>
> > > +
> > > + } else if (avctx->rc_max_rate == avctx->bit_rate) {
> > > + if (!(rc_attr.value & VA_RC_CBR)) {
> > > + av_log(avctx, AV_LOG_WARNING, "Driver does not support "
> > > + "CBR mode (%#x), using VBR mode instead.\n",
> > > + rc_attr.value);
> > > + ctx->va_rc_mode = VA_RC_VBR;
> > > + } else {
> > > + ctx->va_rc_mode = VA_RC_CBR;
> > > + }
> > >
> > > - if (ctx->va_rc_mode == VA_RC_CBR) {
> > > rc_bits_per_second = avctx->bit_rate;
> > > rc_target_percentage = 100;
> > > - rc_window_size = 1000;
> > > +
> > > } else {
> > > - if (avctx->rc_max_rate < avctx->bit_rate) {
> > > - // Max rate is unset or invalid, just use the normal bitrate.
> > > + if (rc_attr.value & VA_RC_VBR) {
> > > + ctx->va_rc_mode = VA_RC_VBR;
> >
> > Is it better to take it as CBR when avctx->rc_max_rate is 0 and CBR is
> > supported
> > by driver?
>
> I don't think so? VBR with the specified target is probably what you want in
> most cases, and I think anyone with specific constraints that want constant
> bitrate should expect to set maxrate to achieve that.
>
I agree VBR is probably what an user wants in most case, however target percent
set to 50% is not suitable for most case. To get a specific target percent, user
should set both target bitrate and max bitrate, so it is reasonable to ask user
must set both target bitrate and max bitrate for VBR cases, and for CBR user may
set target bitrate only.
I just saw it is also VBR in QSV when max bitrate is not set so I'm fine to keep
consistency with QSV for this case.
Thanks
Haihao
> > > +
> > > + // We only have a target bitrate, but VAAPI requires that a
> > > + // maximum rate be supplied as well. Since the user has
> > > + // offered no particular constraint, arbitrarily pick a
> > > + // maximum rate of double the target rate.
> > > + rc_bits_per_second = 2 * avctx->bit_rate;
> > > + rc_target_percentage = 50;
> > > + } else {
> > > + ctx->va_rc_mode = VA_RC_CBR;
> > > +
> > > rc_bits_per_second = avctx->bit_rate;
> > > rc_target_percentage = 100;
> > > - } else {
> > > - rc_bits_per_second = avctx->rc_max_rate;
> > > - rc_target_percentage = (avctx->bit_rate * 100) /
> > > rc_bits_per_second;
> > > }
> > > - rc_window_size = (hrd_buffer_size * 1000) / avctx->bit_rate;
> > > }
> > >
> > > + rc_window_size = (hrd_buffer_size * 1000) / rc_bits_per_second;
> > > +
> > > + av_log(avctx, AV_LOG_VERBOSE, "RC mode: %s, %d%% of %"PRId64" bps "
> > > + "over %d ms.\n", ctx->va_rc_mode == VA_RC_VBR ? "VBR" : "CBR",
> > > + rc_target_percentage, rc_bits_per_second, rc_window_size);
> > > + av_log(avctx, AV_LOG_VERBOSE, "RC buffer: %"PRId64" bits, "
> > > + "initial fullness %"PRId64" bits.\n",
> > > + hrd_buffer_size, hrd_initial_buffer_fullness);
> > > +
> > > + if (rc_bits_per_second > UINT32_MAX ||
> > > + hrd_buffer_size > UINT32_MAX ||
> > > + hrd_initial_buffer_fullness > UINT32_MAX) {
> > > + av_log(avctx, AV_LOG_ERROR, "RC parameters of 2^32 or "
> > > + "greater are not supported by VAAPI.\n");
> > > + return AVERROR(EINVAL);
> > > + }
> > > +
> > > + ctx->va_bit_rate = rc_bits_per_second;
> > > +
> > > + ctx->config_attributes[ctx->nb_config_attributes++] =
> > > + (VAConfigAttrib) {
> > > + .type = VAConfigAttribRateControl,
> > > + .value = ctx->va_rc_mode,
> > > + };
> > > +
> > > ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl;
> > > ctx->rc_params.rc = (VAEncMiscParameterRateControl) {
> > > .bits_per_second = rc_bits_per_second,
> > > @@ -1611,6 +1684,10 @@ av_cold int ff_vaapi_encode_init(AVCodecContext
> > > *avctx)
> > > if (err < 0)
> > > goto fail;
> > >
> > > + err = vaapi_encode_init_rate_control(avctx);
> > > + if (err < 0)
> > > + goto fail;
> > > +
> > > err = vaapi_encode_config_attributes(avctx);
> > > if (err < 0)
> > > goto fail;
> > > @@ -1658,12 +1735,6 @@ av_cold int ff_vaapi_encode_init(AVCodecContext
> > > *avctx)
> > > goto fail;
> > > }
> > >
> > > - if (ctx->va_rc_mode & ~VA_RC_CQP) {
> > > - err = vaapi_encode_init_rate_control(avctx);
> > > - if (err < 0)
> > > - goto fail;
> > > - }
> > > -
> > > if (ctx->codec->configure) {
> > > err = ctx->codec->configure(avctx);
> > > if (err < 0)
> > > ...
> >
> > The logic for rate control is more clear to me in this patch.
>
> Thanks,
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list