| Message ID | d93961f6-5a88-3dda-b3f3-bde82e948ffa@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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.zhao@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.zhao@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
On 2017/5/11 19:39, Mark Thompson wrote: > 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? Now libva don't have a query interface for mb_rate_control, and I think mb_rate_control supported by SKL+ (SKL/APL/KBL/GML) > > >> From b32e4c9c1de47b3bf76327b0ecd11ccf9e3c693f Mon Sep 17 00:00:00 2001 >> From: Jun Zhao <jun.zhao@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.zhao@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). > Now Libva don't have a query interface, I will open a request for this. >> + 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.) > Now i965 driver use default (0) == disable (2), (1) is enable, I think libva/va.h comment for mb_rate_control is confused. >> + >> 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. > Will change to used AV_OPT_TYPE_BOOL in V2 patch. >> { NULL }, >> }; >> >> -- >> 2.11.0 >> > > Thanks, > > - Mark > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On 12/05/17 05:00, Jun Zhao wrote: > On 2017/5/11 19:39, Mark Thompson wrote: >> On 11/05/17 01:29, Jun Zhao wrote: >>> From b32e4c9c1de47b3bf76327b0ecd11ccf9e3c693f Mon Sep 17 00:00:00 2001 >>> From: Jun Zhao <jun.zhao@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.zhao@intel.com> >>> --- >>> libavcodec/vaapi_encode_h264.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> ... >>> + 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.) >> > > Now i965 driver use default (0) == disable (2), (1) is enable, I think libva/va.h comment for mb_rate_control is confused. The header matches the implementation in the H.265 encoder as well - it looks like the H.264 encoder is wrong to me. I made <https://github.com/01org/intel-vaapi-driver/issues/166> to clarify. Thanks, - Mark
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; } 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"); + else + ctx->rc_params.rc.rc_flags.bits.mb_rate_control = opt->mbbrc; + 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", + 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"}, { NULL }, };