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.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).
>
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
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
