On 11/05/17 01:29, Jun Zhao wrote: > Enable the MB level rate control, verified in i965 driver master branch with > Skylake.
I think it makes sense to expose this, but can you explain a bit more what this actually does? All I can see in the i965 driver is that it allocates an extra buffer (as scratch data somehow?) and then passes the option to the proprietary driver blob. (VAAPI encoder documentation is incoming from <https://git.libav.org/?p=libav.git;a=commit;h=41dda860870fb1566b17f6b0b61922f0ef89be47>, if you want to write a bit more for that.) Also, what set of platforms is it usable on? Can we detect whether it will do anything or not? > From b32e4c9c1de47b3bf76327b0ecd11ccf9e3c693f Mon Sep 17 00:00:00 2001 > From: Jun Zhao <jun.z...@intel.com> > Date: Tue, 9 May 2017 08:19:16 +0800 > Subject: [PATCH] lavc/vaapi_encode_h264: Enable MB rate control. > > Enables macroblock-level bitrate control that generally improves > subjective visual quality. It may have a negative impact on > performance and objective visual quality metrics. Default is off > and can't compatible with Constant QP. > > Signed-off-by: Jun Zhao <jun.z...@intel.com> > --- > libavcodec/vaapi_encode_h264.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c > index 92e29554ed..130a9302eb 100644 > --- a/libavcodec/vaapi_encode_h264.c > +++ b/libavcodec/vaapi_encode_h264.c > @@ -168,6 +168,7 @@ typedef struct VAAPIEncodeH264Options { > int qp; > int quality; > int low_power; > + int mbbrc; Maybe call it "mb_rate_control" to match the actual option? > } VAAPIEncodeH264Options; > > > @@ -1157,6 +1158,12 @@ static av_cold int > vaapi_encode_h264_configure(AVCodecContext *avctx) > #endif > } > > + if (ctx->va_rc_mode == VA_RC_CQP && opt->mbbrc != 0) > + av_log(avctx, AV_LOG_WARNING, "The MB level bitrate control is not " > + "compatible with Constant QP, it's will ignored it.\n"); I wouldn't include this warning - I think it's implicit that any rate control parameters will by unused in non-RC modes (compare bit_rate, rc_max_rate, rc_buffer_size, etc.). On the other hand, it might make sense to warn if we know the option isn't implemented by the driver (if we can even detect that). > + else > + ctx->rc_params.rc.rc_flags.bits.mb_rate_control = opt->mbbrc; The documentation says that 0 is default, 1 is enable and 2 is disable. Since this is becoming an active choice, pass 1 or 2 here depending on the option setting? (Given the explanation, avoiding "default" seems reasonable to me.) > + > return 0; > } > > @@ -1283,6 +1290,10 @@ static const AVOption vaapi_encode_h264_options[] = { > { "low_power", "Use low-power encoding mode (experimental: only > supported " > "on some platforms, does not support all features)", > OFFSET(low_power), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, FLAGS }, > + { "mbbrc", "MB level bitrate control", Again, I think "mb_rate_control" would be clearer. > + OFFSET(mbbrc), AV_OPT_TYPE_FLAGS, { .i64 = 0 }, 0, 1, FLAGS, "mbbrc" }, > + { "off", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 0 }, 0, 0, FLAGS, > "mbbrc"}, > + { "on", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 1 }, 0, 0, FLAGS, > "mbbrc"}, FLAGS will do something very strange here (like allowing you to specify "on+off", which would end up meaning 0|1 == 1 == "on"). You want AV_OPT_TYPE_BOOL. > { NULL }, > }; > > -- > 2.11.0 > Thanks, - Mark _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
