[FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up rate control configuration

Mark Thompson sw at jkqxz.net
Thu Jun 14 01:42:36 EEST 2018


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?

(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.

>> +
>> +            // 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


More information about the ffmpeg-devel mailing list