On Sun, 2024-02-25 at 19:15 +0000, Mark Thompson wrote:
> On 23/02/2024 07:14, fei.w.wang-at-intel....@ffmpeg.org wrote:
> > From: Fei Wang <fei.w.w...@intel.com>
> >
> > Signed-off-by: Fei Wang <fei.w.w...@intel.com>
> > ---
> > update:
> > Print mbbrc status with "ON/OFF" instead of "0/1".
> >
> > doc/encoders.texi | 3 +++
> > libavcodec/vaapi_encode.c | 11 ++++++++++-
> > libavcodec/vaapi_encode.h | 9 ++++++++-
> > 3 files changed, 21 insertions(+), 2 deletions(-)
>
> Can you explain what the option actually does? The current
> documentation doesn't really give any hint of whether a user would
> want it on or off.
>
> It also seems like this is a legacy name and maybe should have a more
> generic name to make sense - the iHD driver at least accepts the
> option for H.265 and VP9, neither of which have macroblocks.
How about BLBRC(Block level bitrate control)?
>
> > diff --git a/doc/encoders.texi b/doc/encoders.texi
> > index 9f477d7c53..3f11a80039 100644
> > --- a/doc/encoders.texi
> > +++ b/doc/encoders.texi
> > @@ -4086,6 +4086,9 @@ Quality-defined variable-bitrate.
> > Average variable bitrate.
> > @end table
> >
> > +@item mbbrc
> > +Enable Macroblock based rate control. Invalid for CQP mode.
> > +
> > @end table
> >
> > Each encoder also has its own specific options:
> > diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> > index 86f4110cd2..e4bf15fea0 100644
> > --- a/libavcodec/vaapi_encode.c
> > +++ b/libavcodec/vaapi_encode.c
> > @@ -1806,6 +1806,9 @@ static av_cold int
> > vaapi_encode_init_rate_control(AVCodecContext *avctx)
> > int i, first = 1, res;
> >
> > supported_va_rc_modes = rc_attr.value;
> > + if (ctx->mbbrc && !(supported_va_rc_modes & VA_RC_MB))
> > + ctx->mbbrc = 0;
>
> Probably not a good idea to silently disable what the user explicitly
> requested.
>
> > +
> > for (i = 0; i < FF_ARRAY_ELEMS(vaapi_encode_rc_modes);
> > i++) {
> > rc_mode = &vaapi_encode_rc_modes[i];
> > if (supported_va_rc_modes & rc_mode->va_mode) {
> > @@ -2017,13 +2020,18 @@ rc_mode_found:
> > ctx->va_bit_rate = rc_bits_per_second;
> >
> > av_log(avctx, AV_LOG_VERBOSE, "RC mode: %s.\n", rc_mode-
> > >name);
> > +
> > + if (ctx->mbbrc && ctx->va_rc_mode == VA_RC_CQP)
> > + ctx->mbbrc = 0;
> > + av_log(avctx, AV_LOG_VERBOSE, "MB bitrate control: %s.\n",
> > ctx->mbbrc ? "ON" : "OFF");
> > +
> > if (rc_attr.value == VA_ATTRIB_NOT_SUPPORTED) {
> > // This driver does not want the RC mode attribute to be
> > set.
> > } else {
> > ctx->config_attributes[ctx->nb_config_attributes++] =
> > (VAConfigAttrib) {
> > .type = VAConfigAttribRateControl,
> > - .value = ctx->va_rc_mode,
> > + .value = ctx->mbbrc ? ctx->va_rc_mode | VA_RC_MB :
> > ctx->va_rc_mode,
> > };
> > }
> >
> > @@ -2052,6 +2060,7 @@ rc_mode_found:
> > #if VA_CHECK_VERSION(1, 1, 0)
> > .ICQ_quality_factor = av_clip(rc_quality, 1, 51),
> > .max_qp = (avctx->qmax > 0 ? avctx->qmax
> > : 0),
> > + .rc_flags.bits.mb_rate_control = ctx->mbbrc ? 1 : 2,
>
> What did the previous 0 "use default" do here?
0 means auto decide by driver. This feature may has impact on frame
psnr, so set it as off by default and expect user enable it explicitly.
Thanks
Fei
>
> > #endif
> > #if VA_CHECK_VERSION(1, 3, 0)
> > .quality_factor = rc_quality,
> > diff --git a/libavcodec/vaapi_encode.h b/libavcodec/vaapi_encode.h
> > index 6964055b93..9774cc86b1 100644
> > --- a/libavcodec/vaapi_encode.h
> > +++ b/libavcodec/vaapi_encode.h
> > @@ -216,6 +216,9 @@ typedef struct VAAPIEncodeContext {
> > // available modes).
> > int explicit_rc_mode;
> >
> > + // Macroblock based rate control.
> > + int mbbrc;
> > +
> > // Explicitly-set QP, for use with the "qp" options.
> > // (Forces CQP mode when set, overriding everything else.)
> > int explicit_qp;
> > @@ -538,7 +541,11 @@ int ff_vaapi_encode_close(AVCodecContext
> > *avctx);
> > VAAPI_ENCODE_RC_MODE(VBR, "Variable-bitrate"), \
> > VAAPI_ENCODE_RC_MODE(ICQ, "Intelligent constant-quality"), \
> > VAAPI_ENCODE_RC_MODE(QVBR, "Quality-defined variable-
> > bitrate"), \
> > - VAAPI_ENCODE_RC_MODE(AVBR, "Average variable-bitrate")
> > + VAAPI_ENCODE_RC_MODE(AVBR, "Average variable-bitrate"), \
> > + { "mbbrc", \
> > + "Macroblock based bitrate control",\
> > + OFFSET(common.mbbrc), AV_OPT_TYPE_BOOL, \
> > + { .i64 = 0 }, 0, 1, FLAGS }
> >
> >
> > #endif /* AVCODEC_VAAPI_ENCODE_H */
>
> Thanks,
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
