On Thu, 2018-06-14 at 07:22 +0000, Li, Zhong wrote:
> > -----Original Message-----
> > From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
> > Of Xiang, Haihao
> > Sent: Thursday, June 14, 2018 2:08 PM
> > To: ffmpeg-devel@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.
>
The max bitrate is used to limit the peak bitrate, so setting the max bitrate to
INT_MAX doesn't make sense to me.
> >
> > 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?
>
It is taken as CQP in ffmpeg VAAPI when target bitrate is 0 after applying this
patch, so it should work 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
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
