[FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up rate control configuration
Li, Zhong
zhong.li at intel.com
Thu Jun 14 10:22:57 EEST 2018
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Xiang, Haihao
> Sent: Thursday, June 14, 2018 2:08 PM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up rate
> control configuration
>
> 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.
How about set the max_rate to be a very larger number such as INT_MAX if user hasn't set it?
User may don't set max_rate on purpose, expecting better quality with unlimited bitrate fluctuation (common requirement for local video files).
Double of target_bit_rate is too strict IMHO. And I haven't such a limitation in x264 ABR mode.
>
> 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.
What will happen if user set a max_rate but without a setting for target_bitrate?
The mode will be VBR (if driver support) with target_bitrate=0.
Tried this on qsv, MSDK returns an error of invalid video parameters.
Is it ok for vaapi? And also with iHD driver?
>
> 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
More information about the ffmpeg-devel
mailing list