[FFmpeg-devel] [PATCH] lavc/vaapi_encode_h264: Enable MB rate control

[Jun Zhao]

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 at 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 at 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>