[FFmpeg-devel,1/2] lavc/vaapi_encode: correct the HRD buffer size calculate.

From 7eef9be1c8a92bf625d62a0f97f762f1342c6d78 Mon Sep 17 00:00:00 2001
From: Jun Zhao <jun.zhao@intel.com>
Date: Tue, 31 Oct 2017 10:13:42 +0800
Subject: [PATCH 1/2] lavc/vaapi_encode: correct the HRD buffer size calculate.

when rc_buffer_size didn't setting, always use the max bit rate
per second as HRD buffer size.

Signed-off-by: Jun Zhao <jun.zhao@intel.com>
Signed-off-by: Wang, Yi A <yi.a.wang@intel.com>
---
 libavcodec/vaapi_encode.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

On 31/10/17 02:37, Jun Zhao wrote:
> From 7eef9be1c8a92bf625d62a0f97f762f1342c6d78 Mon Sep 17 00:00:00 2001
> From: Jun Zhao <jun.zhao@intel.com>
> Date: Tue, 31 Oct 2017 10:13:42 +0800
> Subject: [PATCH 1/2] lavc/vaapi_encode: correct the HRD buffer size calculate.
> 
> when rc_buffer_size didn't setting, always use the max bit rate
> per second as HRD buffer size.
> 
> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
> Signed-off-by: Wang, Yi A <yi.a.wang@intel.com>
> ---
>  libavcodec/vaapi_encode.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> index 590f4be4ed..d5f89ef346 100644
> --- a/libavcodec/vaapi_encode.c
> +++ b/libavcodec/vaapi_encode.c
> @@ -1144,19 +1144,9 @@ static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx)
>          return AVERROR(EINVAL);
>      }
>  
> -    if (avctx->rc_buffer_size)
> -        hrd_buffer_size = avctx->rc_buffer_size;
> -    else
> -        hrd_buffer_size = avctx->bit_rate;
> -    if (avctx->rc_initial_buffer_occupancy)
> -        hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy;
> -    else
> -        hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4;
> -
>      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.
> @@ -1166,8 +1156,17 @@ static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx)
>              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;
>      }
> +    rc_window_size = (rc_bits_per_second * 1000) / avctx->bit_rate;
> +
> +    if (avctx->rc_buffer_size)
> +        hrd_buffer_size = avctx->rc_buffer_size;
> +    else
> +        hrd_buffer_size = rc_bits_per_second;
> +    if (avctx->rc_initial_buffer_occupancy)
> +        hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy;
> +    else
> +        hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4;
>  
>      ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl;
>      ctx->rc_params.rc = (VAEncMiscParameterRateControl) {
> -- 
> 2.14.1
> 

Why?  If the RC buffer size is unset it currently guesses one second of the target bitrate - in what way is the peak bitrate any more appropriate as a guess?

- Mark

On 2017/10/31 18:19, Mark Thompson wrote:
> On 31/10/17 02:37, Jun Zhao wrote:
>> From 7eef9be1c8a92bf625d62a0f97f762f1342c6d78 Mon Sep 17 00:00:00 2001
>> From: Jun Zhao <jun.zhao@intel.com>
>> Date: Tue, 31 Oct 2017 10:13:42 +0800
>> Subject: [PATCH 1/2] lavc/vaapi_encode: correct the HRD buffer size calculate.
>>
>> when rc_buffer_size didn't setting, always use the max bit rate
>> per second as HRD buffer size.
>>
>> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
>> Signed-off-by: Wang, Yi A <yi.a.wang@intel.com>
>> ---
>>  libavcodec/vaapi_encode.c | 21 ++++++++++-----------
>>  1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
>> index 590f4be4ed..d5f89ef346 100644
>> --- a/libavcodec/vaapi_encode.c
>> +++ b/libavcodec/vaapi_encode.c
>> @@ -1144,19 +1144,9 @@ static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx)
>>          return AVERROR(EINVAL);
>>      }
>>  
>> -    if (avctx->rc_buffer_size)
>> -        hrd_buffer_size = avctx->rc_buffer_size;
>> -    else
>> -        hrd_buffer_size = avctx->bit_rate;
>> -    if (avctx->rc_initial_buffer_occupancy)
>> -        hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy;
>> -    else
>> -        hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4;
>> -
>>      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.
>> @@ -1166,8 +1156,17 @@ static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx)
>>              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;
>>      }
>> +    rc_window_size = (rc_bits_per_second * 1000) / avctx->bit_rate;
>> +
>> +    if (avctx->rc_buffer_size)
>> +        hrd_buffer_size = avctx->rc_buffer_size;
>> +    else
>> +        hrd_buffer_size = rc_bits_per_second;
>> +    if (avctx->rc_initial_buffer_occupancy)
>> +        hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy;
>> +    else
>> +        hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4;
>>  
>>      ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl;
>>      ctx->rc_params.rc = (VAEncMiscParameterRateControl) {
>> -- 
>> 2.14.1
>>
> Why?  If the RC buffer size is unset it currently guesses one second of the target bitrate - in what way is the peak bitrate any more appropriate as a guess?
In VBR mode, when rc_max_rate more than bit_rate, I think HDR size need
to use rc_max_rate, not the bit_rate.
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

On 31/10/17 15:11, Jun Zhao wrote:> On 2017/10/31 18:19, Mark Thompson wrote:
>> On 31/10/17 02:37, Jun Zhao wrote:
>>> From 7eef9be1c8a92bf625d62a0f97f762f1342c6d78 Mon Sep 17 00:00:00 2001
>>> From: Jun Zhao <jun.zhao@intel.com>
>>> Date: Tue, 31 Oct 2017 10:13:42 +0800
>>> Subject: [PATCH 1/2] lavc/vaapi_encode: correct the HRD buffer size calculate.
>>>
>>> when rc_buffer_size didn't setting, always use the max bit rate
>>> per second as HRD buffer size.
>>>
>>> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
>>> Signed-off-by: Wang, Yi A <yi.a.wang@intel.com>
>>> ---
>>>  libavcodec/vaapi_encode.c | 21 ++++++++++-----------
>>>  1 file changed, 10 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
>>> index 590f4be4ed..d5f89ef346 100644
>>> --- a/libavcodec/vaapi_encode.c
>>> +++ b/libavcodec/vaapi_encode.c
>>> @@ -1144,19 +1144,9 @@ static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx)
>>>          return AVERROR(EINVAL);
>>>      }
>>>  
>>> -    if (avctx->rc_buffer_size)
>>> -        hrd_buffer_size = avctx->rc_buffer_size;
>>> -    else
>>> -        hrd_buffer_size = avctx->bit_rate;
>>> -    if (avctx->rc_initial_buffer_occupancy)
>>> -        hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy;
>>> -    else
>>> -        hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4;
>>> -
>>>      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.
>>> @@ -1166,8 +1156,17 @@ static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx)
>>>              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;
>>>      }
>>> +    rc_window_size = (rc_bits_per_second * 1000) / avctx->bit_rate;
>>> +
>>> +    if (avctx->rc_buffer_size)
>>> +        hrd_buffer_size = avctx->rc_buffer_size;
>>> +    else
>>> +        hrd_buffer_size = rc_bits_per_second;
>>> +    if (avctx->rc_initial_buffer_occupancy)
>>> +        hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy;
>>> +    else
>>> +        hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4;
>>>  
>>>      ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl;
>>>      ctx->rc_params.rc = (VAEncMiscParameterRateControl) {
>>> -- 
>>> 2.14.1
>>>
>> Why?  If the RC buffer size is unset it currently guesses one second of the target bitrate - in what way is the peak bitrate any more appropriate as a guess?
> In VBR mode, when rc_max_rate more than bit_rate, I think HDR size need
> to use rc_max_rate, not the bit_rate.

Ok, why do you think that?  I'm not necessarily against this change (in cases where it matters the user will provide rc_buffer_size explicitly), but please provide some reasoning for it.

- Mark

On 2017/10/31 23:45, Mark Thompson wrote:
> On 31/10/17 15:11, Jun Zhao wrote:> On 2017/10/31 18:19, Mark Thompson wrote:
>>> On 31/10/17 02:37, Jun Zhao wrote:
>>>> From 7eef9be1c8a92bf625d62a0f97f762f1342c6d78 Mon Sep 17 00:00:00 2001
>>>> From: Jun Zhao <jun.zhao@intel.com>
>>>> Date: Tue, 31 Oct 2017 10:13:42 +0800
>>>> Subject: [PATCH 1/2] lavc/vaapi_encode: correct the HRD buffer size calculate.
>>>>
>>>> when rc_buffer_size didn't setting, always use the max bit rate
>>>> per second as HRD buffer size.
>>>>
>>>> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
>>>> Signed-off-by: Wang, Yi A <yi.a.wang@intel.com>
>>>> ---
>>>>  libavcodec/vaapi_encode.c | 21 ++++++++++-----------
>>>>  1 file changed, 10 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
>>>> index 590f4be4ed..d5f89ef346 100644
>>>> --- a/libavcodec/vaapi_encode.c
>>>> +++ b/libavcodec/vaapi_encode.c
>>>> @@ -1144,19 +1144,9 @@ static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx)
>>>>          return AVERROR(EINVAL);
>>>>      }
>>>>  
>>>> -    if (avctx->rc_buffer_size)
>>>> -        hrd_buffer_size = avctx->rc_buffer_size;
>>>> -    else
>>>> -        hrd_buffer_size = avctx->bit_rate;
>>>> -    if (avctx->rc_initial_buffer_occupancy)
>>>> -        hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy;
>>>> -    else
>>>> -        hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4;
>>>> -
>>>>      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.
>>>> @@ -1166,8 +1156,17 @@ static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx)
>>>>              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;
>>>>      }
>>>> +    rc_window_size = (rc_bits_per_second * 1000) / avctx->bit_rate;
>>>> +
>>>> +    if (avctx->rc_buffer_size)
>>>> +        hrd_buffer_size = avctx->rc_buffer_size;
>>>> +    else
>>>> +        hrd_buffer_size = rc_bits_per_second;
>>>> +    if (avctx->rc_initial_buffer_occupancy)
>>>> +        hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy;
>>>> +    else
>>>> +        hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4;
>>>>  
>>>>      ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl;
>>>>      ctx->rc_params.rc = (VAEncMiscParameterRateControl) {
>>>> -- 
>>>> 2.14.1
>>>>
>>> Why?  If the RC buffer size is unset it currently guesses one second of the target bitrate - in what way is the peak bitrate any more appropriate as a guess?
>> In VBR mode, when rc_max_rate more than bit_rate, I think HDR size need
>> to use rc_max_rate, not the bit_rate.
> Ok, why do you think that?  I'm not necessarily against this change (in cases where it matters the user will provide rc_buffer_size explicitly), but please provide some reasoning for it.
I think the code logic have considered the explicit rc_buffer_size
setting, and this patch
correct the HRD buffer size with MAX(rc_max_rate, bit_rate) when no
setting rc_buffer_size in
VBR mode. In the old logic, in VBR mode, if user don't setting
rc_buffer_size
explicitly, the HRD buffer size always use avctx->bit_rate, it does not
make sense.
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

On 31/10/17 16:00, Jun Zhao wrote:
> On 2017/10/31 23:45, Mark Thompson wrote:
>> On 31/10/17 15:11, Jun Zhao wrote:> On 2017/10/31 18:19, Mark Thompson wrote:
>>>> On 31/10/17 02:37, Jun Zhao wrote:
>>>>> From 7eef9be1c8a92bf625d62a0f97f762f1342c6d78 Mon Sep 17 00:00:00 2001
>>>>> From: Jun Zhao <jun.zhao@intel.com>
>>>>> Date: Tue, 31 Oct 2017 10:13:42 +0800
>>>>> Subject: [PATCH 1/2] lavc/vaapi_encode: correct the HRD buffer size calculate.
>>>>>
>>>>> when rc_buffer_size didn't setting, always use the max bit rate
>>>>> per second as HRD buffer size.
>>>>>
>>>>> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
>>>>> Signed-off-by: Wang, Yi A <yi.a.wang@intel.com>
>>>>> ---
>>>>>  libavcodec/vaapi_encode.c | 21 ++++++++++-----------
>>>>>  1 file changed, 10 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
>>>>> index 590f4be4ed..d5f89ef346 100644
>>>>> --- a/libavcodec/vaapi_encode.c
>>>>> +++ b/libavcodec/vaapi_encode.c
>>>>> @@ -1144,19 +1144,9 @@ static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx)
>>>>>          return AVERROR(EINVAL);
>>>>>      }
>>>>>  
>>>>> -    if (avctx->rc_buffer_size)
>>>>> -        hrd_buffer_size = avctx->rc_buffer_size;
>>>>> -    else
>>>>> -        hrd_buffer_size = avctx->bit_rate;
>>>>> -    if (avctx->rc_initial_buffer_occupancy)
>>>>> -        hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy;
>>>>> -    else
>>>>> -        hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4;
>>>>> -
>>>>>      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.
>>>>> @@ -1166,8 +1156,17 @@ static av_cold int vaapi_encode_init_rate_control(AVCodecContext *avctx)
>>>>>              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;
>>>>>      }
>>>>> +    rc_window_size = (rc_bits_per_second * 1000) / avctx->bit_rate;
>>>>> +
>>>>> +    if (avctx->rc_buffer_size)
>>>>> +        hrd_buffer_size = avctx->rc_buffer_size;
>>>>> +    else
>>>>> +        hrd_buffer_size = rc_bits_per_second;
>>>>> +    if (avctx->rc_initial_buffer_occupancy)
>>>>> +        hrd_initial_buffer_fullness = avctx->rc_initial_buffer_occupancy;
>>>>> +    else
>>>>> +        hrd_initial_buffer_fullness = hrd_buffer_size * 3 / 4;
>>>>>  
>>>>>      ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl;
>>>>>      ctx->rc_params.rc = (VAEncMiscParameterRateControl) {
>>>>> -- 
>>>>> 2.14.1
>>>>>
>>>> Why?  If the RC buffer size is unset it currently guesses one second of the target bitrate - in what way is the peak bitrate any more appropriate as a guess?
>>> In VBR mode, when rc_max_rate more than bit_rate, I think HDR size need
>>> to use rc_max_rate, not the bit_rate.
>> Ok, why do you think that?  I'm not necessarily against this change (in cases where it matters the user will provide rc_buffer_size explicitly), but please provide some reasoning for it.
> I think the code logic have considered the explicit rc_buffer_size
> setting, and this patch
> correct the HRD buffer size with MAX(rc_max_rate, bit_rate) when no
> setting rc_buffer_size in
> VBR mode.

I agree that this is what your patch does.

>           In the old logic, in VBR mode, if user don't setting
> rc_buffer_size
> explicitly, the HRD buffer size always use avctx->bit_rate, it does not
> make sense.
Why not?

- Mark