[FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up rate control configuration

[Mark Thompson]

On 14/06/18 08:22, Li, Zhong wrote:
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
>> Of Xiang, Haihao
>> Sent: Thursday, June 14, 2018 2:08 PM
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v2 16/36] vaapi_encode: Clean up rate
>> control configuration
>>
>> On Wed, 2018-06-13 at 23:42 +0100, Mark Thompson wrote:
>>> On 13/06/18 08:03, Xiang, Haihao wrote:
>>>> On Fri, 2018-06-08 at 00:43 +0100, Mark Thompson wrote:
>>>>> Query which modes are supported and select between VBR and CBR
>>>>> based on that - this removes all of the codec-specific rate
>>>>> control mode selection code.
>>>>> ---
>>>>>  doc/encoders.texi               |   2 -
>>>>>  libavcodec/vaapi_encode.c       | 173
>> ++++++++++++++++++++++++++++-------
>>>>> ----
>>>>> -
>>>>>  libavcodec/vaapi_encode.h       |   6 +-
>>>>>  libavcodec/vaapi_encode_h264.c  |  18 +----
>>>>> libavcodec/vaapi_encode_h265.c  |  14 +---
>>>>>  libavcodec/vaapi_encode_mjpeg.c |   3 +-
>>>>>  libavcodec/vaapi_encode_mpeg2.c |   9 +--
>>>>>  libavcodec/vaapi_encode_vp8.c   |  13 +--
>>>>>  libavcodec/vaapi_encode_vp9.c   |  13 +--
>>>>>  9 files changed, 137 insertions(+), 114 deletions(-)
>>>>>
>>>>> ...
>>>>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
>>>>> index f4c063734c..5de5483454 100644
>>>>> --- a/libavcodec/vaapi_encode.c
>>>>> +++ b/libavcodec/vaapi_encode.c
>>>>> ...
>>>>> +    if (avctx->flags & AV_CODEC_FLAG_QSCALE ||
>>>>> +        avctx->bit_rate <= 0) {

This condition ^

>>>>> +        if (rc_attr.value & VA_RC_CQP) {
>>>>> +            av_log(avctx, AV_LOG_VERBOSE, "Using
>> constant-quality
>>>>> mode.\n");
>>>>> +            ctx->va_rc_mode = VA_RC_CQP;
>>>>> +            return 0;
>>>>> +        } else {
>>>>> +            av_log(avctx, AV_LOG_ERROR, "Driver does not
>> support "
>>>>> +                   "constant-quality mode (%#x).\n",
>> rc_attr.value);
>>>>> +            return AVERROR(EINVAL);
>>>>> +        }
>>>>> +    }
>>>>> ...
>>>>> +    } else if (avctx->rc_max_rate == avctx->bit_rate) {
>>>>> +        if (!(rc_attr.value & VA_RC_CBR)) {
>>>>> +            av_log(avctx, AV_LOG_WARNING, "Driver does not
>> support "
>>>>> +                   "CBR mode (%#x), using VBR mode
>> instead.\n",
>>>>> +                   rc_attr.value);
>>>>> +            ctx->va_rc_mode = VA_RC_VBR;
>>>>> +        } else {
>>>>> +            ctx->va_rc_mode = VA_RC_CBR;
>>>>> +        }
>>>>>
>>>>> -    if (ctx->va_rc_mode == VA_RC_CBR) {
>>>>>          rc_bits_per_second   = avctx->bit_rate;
>>>>>          rc_target_percentage = 100;
>>>>> -        rc_window_size       = 1000;
>>>>> +
>>>>>      } else {
>>>>> -        if (avctx->rc_max_rate < avctx->bit_rate) {
>>>>> -            // Max rate is unset or invalid, just use the normal
>> bitrate.
>>>>> +        if (rc_attr.value & VA_RC_VBR) {
>>>>> +            ctx->va_rc_mode = VA_RC_VBR;
>>>>
>>>> Is it better to take it as CBR when avctx->rc_max_rate is 0 and CBR
>>>> is supported by driver?
>>>
>>> I don't think so?  VBR with the specified target is probably what you
>>> want in most cases, and I think anyone with specific constraints that
>>> want constant bitrate should expect to set maxrate to achieve that.
>>>
>>
>> I agree VBR is probably what an user wants in most case, however target
>> percent set to 50% is not suitable for most case. To get a specific target
>> percent, user should set both target bitrate and max bitrate, so it is
>> reasonable to ask user must set both target bitrate and max bitrate for VBR
>> cases, and for CBR user may set target bitrate only.
> 
> How about set the max_rate to be a very larger number such as INT_MAX if user hasn't set it? 
> User may don't set max_rate on purpose, expecting better quality with unlimited bitrate fluctuation (common requirement for local video files).
> Double of target_bit_rate is too strict IMHO. And I haven't such a limitation in x264 ABR mode.

This unconstrained setup you describe was my intent (as you say, it's usually what you want for local files), but unfortunately the API doesn't really let us do it - the target bitrate is specified as an integer percentage of the max bitrate.  50% was an arbitrary value picked to not have a strong constraint but also not be small enough that we need to think about rounding/overflow problems.

We could try to pick a large value with the right properties (for example: if target < 2^32 / 100 then choose maxrate = target * 100, percentage = 1; otherwise choose percentage = 2^32 * 100 / bitrate, maxrate = bitrate * 100 / percentage), but that would be complex to test and I don't think the drivers handle this sort of setup very well.

>> I just saw it is also VBR in QSV when max bitrate is not set so I'm fine to keep
>> consistency with QSV for this case.
> 
> What will happen if user set a max_rate but without a setting for target_bitrate? 
> The mode will be VBR (if driver support) with target_bitrate=0. 
> Tried this on qsv, MSDK returns an error of invalid video parameters. 
> Is it ok for vaapi? And also with iHD driver?

If AVCodecContext.bit_rate isn't set then we use constant-quality mode instead - see the block I've pointed out above.

There isn't currently any constant-quality with max-rate constraint mode in VAAPI.

>>>>> +
>>>>> +            // We only have a target bitrate, but VAAPI requires
>> that a
>>>>> +            // maximum rate be supplied as well.  Since the user
>> has
>>>>> +            // offered no particular constraint, arbitrarily pick a
>>>>> +            // maximum rate of double the target rate.
>>>>> +            rc_bits_per_second   = 2 * avctx->bit_rate;
>>>>> +            rc_target_percentage = 50;
>>>>> +        } else {
>>>>> +            ctx->va_rc_mode = VA_RC_CBR;
>>>>> +
>>>>>              rc_bits_per_second   = avctx->bit_rate;
>>>>>              rc_target_percentage = 100;
>>>>> -        } else {
>>>>> -            rc_bits_per_second   = avctx->rc_max_rate;
>>>>> -            rc_target_percentage = (avctx->bit_rate * 100) /
>>>>> rc_bits_per_second;
>>>>>          }
>>>>> -        rc_window_size = (hrd_buffer_size * 1000) / avctx->bit_rate;
>>>>>      }
>>>>> ...

Thanks,

- Mark