[FFmpeg-devel] vaapi_encode: Fix the vaapi h264 encoder VBR/CBR metadata setting error.

Submitted by Jun Zhao on April 13, 2017, 12:34 p.m.

Details

Message ID 2d9a3b36-3afc-883a-a7a5-62c508adc562@gmail.com
State New
Headers show

Commit Message

Jun Zhao April 13, 2017, 12:34 p.m.
From 1fa48b45fe962d8c342d158d9c16ce24139ffd84 Mon Sep 17 00:00:00 2001
From: Jun Zhao <jun.zhao@intel.com>
Date: Thu, 13 Apr 2017 20:07:10 +0800
Subject: [PATCH] vaapi_encode: Fix the vaapi h264 encoder VBR/CBR metadata
 setting error.

before this fix, mediainfo check the bit rate control mode metadata info
is wrong.

Reproduce step:
encode with CBR or VBR mode like this:
./ffmpeg -y -hwaccel vaapi -vaapi_device /dev/dri/renderD128 \
-hwaccel_output_format vaapi -i input.mp4 -an -c:v h264_vaapi \
-maxrate 5M -b:v 5M output_cbr.mp4

./ffmpeg -y -hwaccel vaapi -vaapi_device /dev/dri/renderD128 \
-hwaccel_output_format vaapi -i input.mp4 -an -c:v h264_vaapi \
-maxrate 6M -b:v 5M output_cbr.mp4

then use the mediainfo check the bit rate control mode.

Signed-off-by: Jun Zhao <jun.zhao@intel.com>
---
 libavcodec/vaapi_encode_h264.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Mark Thompson April 13, 2017, 2:05 p.m.
On 13/04/17 13:34, Jun Zhao wrote:
> From 1fa48b45fe962d8c342d158d9c16ce24139ffd84 Mon Sep 17 00:00:00 2001
> From: Jun Zhao <jun.zhao@intel.com>
> Date: Thu, 13 Apr 2017 20:07:10 +0800
> Subject: [PATCH] vaapi_encode: Fix the vaapi h264 encoder VBR/CBR metadata
>  setting error.
> 
> before this fix, mediainfo check the bit rate control mode metadata info
> is wrong.
> 
> Reproduce step:
> encode with CBR or VBR mode like this:
> ./ffmpeg -y -hwaccel vaapi -vaapi_device /dev/dri/renderD128 \
> -hwaccel_output_format vaapi -i input.mp4 -an -c:v h264_vaapi \
> -maxrate 5M -b:v 5M output_cbr.mp4
> 
> ./ffmpeg -y -hwaccel vaapi -vaapi_device /dev/dri/renderD128 \
> -hwaccel_output_format vaapi -i input.mp4 -an -c:v h264_vaapi \
> -maxrate 6M -b:v 5M output_cbr.mp4
> 
> then use the mediainfo check the bit rate control mode.
> 
> Signed-off-by: Jun Zhao <jun.zhao@intel.com>

Not yet generating the HRD and timing information in VBR mode was deliberate, because I don't know enough about the conformance properties of the new proprietary rate controller which the Intel driver has switched to.  The old CBR mode was straightforward to verify from the source code, and it seemed reasonable to assume that the new CBR mode would be too given the constraints on it (and also because existing versions of lavc couldn't be modified).

Can you say anything about the HRD conformance of the Intel driver here, or was this patch purely for the metadata output (which may not actually match the stream, hence my concern)?

Only the Intel driver is relevant to this as far as I know - the Mesa driver does not support the packed headers necessary for this information to be included in the stream.


> ---
>  libavcodec/vaapi_encode_h264.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c
> index 92e2955..47d0c94 100644
> --- a/libavcodec/vaapi_encode_h264.c
> +++ b/libavcodec/vaapi_encode_h264.c
> @@ -760,7 +760,7 @@ static int vaapi_encode_h264_write_extra_header(AVCodecContext *avctx,
>      char tmp[256];
>      size_t header_len;
>  
> -    if (index == 0 && ctx->va_rc_mode == VA_RC_CBR) {
> +    if (index == 0 && (ctx->va_rc_mode == VA_RC_CBR || ctx->va_rc_mode == VA_RC_VBR)) {
>          *type = VAEncPackedHeaderH264_SEI;
>  
>          init_put_bits(&pbc, tmp, sizeof(tmp));
> @@ -868,7 +868,7 @@ static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
>              mseq->fixed_frame_rate_flag = 0;
>          }
>  
> -        if (ctx->va_rc_mode == VA_RC_CBR) {
> +        if (ctx->va_rc_mode == VA_RC_CBR || ctx->va_rc_mode == VA_RC_VBR) {
>              priv->send_timing_sei = 1;
>              mseq->nal_hrd_parameters_present_flag = 1;
>  
> @@ -886,8 +886,7 @@ static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
>              mseq->cpb_size_value_minus1[0] =
>                  (ctx->hrd_params.hrd.buffer_size >> mseq->cpb_size_scale + 4) - 1;
>  
> -            // CBR mode isn't actually available here, despite naming.
> -            mseq->cbr_flag[0] = 0;
> +            mseq->cbr_flag[0] = (ctx->va_rc_mode == VA_RC_CBR ? 1 : 0);

As the comment notes, the necessary CBR mode isn't actually available.  Drivers only offer "soft" CBR, which can result in HRD overflow; this specifies "hard" CBR, so if you want to set this flag you would need to insert filler NAL units as well.

Thanks,

- Mark
Jun Zhao April 14, 2017, 1:01 a.m.
On 2017/4/13 22:05, Mark Thompson wrote:
> On 13/04/17 13:34, Jun Zhao wrote:
>> From 1fa48b45fe962d8c342d158d9c16ce24139ffd84 Mon Sep 17 00:00:00 2001
>> From: Jun Zhao <jun.zhao@intel.com>
>> Date: Thu, 13 Apr 2017 20:07:10 +0800
>> Subject: [PATCH] vaapi_encode: Fix the vaapi h264 encoder VBR/CBR metadata
>>  setting error.
>>
>> before this fix, mediainfo check the bit rate control mode metadata info
>> is wrong.
>>
>> Reproduce step:
>> encode with CBR or VBR mode like this:
>> ./ffmpeg -y -hwaccel vaapi -vaapi_device /dev/dri/renderD128 \
>> -hwaccel_output_format vaapi -i input.mp4 -an -c:v h264_vaapi \
>> -maxrate 5M -b:v 5M output_cbr.mp4
>>
>> ./ffmpeg -y -hwaccel vaapi -vaapi_device /dev/dri/renderD128 \
>> -hwaccel_output_format vaapi -i input.mp4 -an -c:v h264_vaapi \
>> -maxrate 6M -b:v 5M output_cbr.mp4
>>
>> then use the mediainfo check the bit rate control mode.
>>
>> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
> 
> Not yet generating the HRD and timing information in VBR mode was deliberate, because I don't know enough about the conformance properties of the new proprietary rate controller which the Intel driver has switched to.  The old CBR mode was straightforward to verify from the source code, and it seemed reasonable to assume that the new CBR mode would be too given the constraints on it (and also because existing versions of lavc couldn't be modified).
> 
> Can you say anything about the HRD conformance of the Intel driver here, or was this patch purely for the metadata output (which may not actually match the stream, hence my concern)?
> 
> Only the Intel driver is relevant to this as far as I know - the Mesa driver does not support the packed headers necessary for this information to be included in the stream.
> 
> 

I don't know the details about Intel driver's HRD conformance model, this patch 
just want to correct the bit rate mode metadata information.

>> ---
>>  libavcodec/vaapi_encode_h264.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c
>> index 92e2955..47d0c94 100644
>> --- a/libavcodec/vaapi_encode_h264.c
>> +++ b/libavcodec/vaapi_encode_h264.c
>> @@ -760,7 +760,7 @@ static int vaapi_encode_h264_write_extra_header(AVCodecContext *avctx,
>>      char tmp[256];
>>      size_t header_len;
>>  
>> -    if (index == 0 && ctx->va_rc_mode == VA_RC_CBR) {
>> +    if (index == 0 && (ctx->va_rc_mode == VA_RC_CBR || ctx->va_rc_mode == VA_RC_VBR)) {
>>          *type = VAEncPackedHeaderH264_SEI;
>>  
>>          init_put_bits(&pbc, tmp, sizeof(tmp));
>> @@ -868,7 +868,7 @@ static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
>>              mseq->fixed_frame_rate_flag = 0;
>>          }
>>  
>> -        if (ctx->va_rc_mode == VA_RC_CBR) {
>> +        if (ctx->va_rc_mode == VA_RC_CBR || ctx->va_rc_mode == VA_RC_VBR) {
>>              priv->send_timing_sei = 1;
>>              mseq->nal_hrd_parameters_present_flag = 1;
>>  
>> @@ -886,8 +886,7 @@ static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
>>              mseq->cpb_size_value_minus1[0] =
>>                  (ctx->hrd_params.hrd.buffer_size >> mseq->cpb_size_scale + 4) - 1;
>>  
>> -            // CBR mode isn't actually available here, despite naming.
>> -            mseq->cbr_flag[0] = 0;
>> +            mseq->cbr_flag[0] = (ctx->va_rc_mode == VA_RC_CBR ? 1 : 0);
> 
> As the comment notes, the necessary CBR mode isn't actually available.  Drivers only offer "soft" CBR, which can result in HRD overflow; this specifies "hard" CBR, so if you want to set this flag you would need to insert filler NAL units as well.
> 

In 1.8.0 release driver (https://lists.01.org/pipermail/intel-vaapi-media/2017-March/000016.html), 
I can't reproduce the overflow issue with CBR mode. So I guess the new driver fix the HRD overflow
issue (I don't dig in the 1.8.0 driver) .

> Thanks,
> 
> - Mark
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Mark Thompson April 14, 2017, 11:22 a.m.
On 14/04/17 02:01, Jun Zhao wrote:
> On 2017/4/13 22:05, Mark Thompson wrote:
>> On 13/04/17 13:34, Jun Zhao wrote:
>>> From 1fa48b45fe962d8c342d158d9c16ce24139ffd84 Mon Sep 17 00:00:00 2001
>>> From: Jun Zhao <jun.zhao@intel.com>
>>> Date: Thu, 13 Apr 2017 20:07:10 +0800
>>> Subject: [PATCH] vaapi_encode: Fix the vaapi h264 encoder VBR/CBR metadata
>>>  setting error.
>>>
>>> before this fix, mediainfo check the bit rate control mode metadata info
>>> is wrong.
>>>
>>> Reproduce step:
>>> encode with CBR or VBR mode like this:
>>> ./ffmpeg -y -hwaccel vaapi -vaapi_device /dev/dri/renderD128 \
>>> -hwaccel_output_format vaapi -i input.mp4 -an -c:v h264_vaapi \
>>> -maxrate 5M -b:v 5M output_cbr.mp4
>>>
>>> ./ffmpeg -y -hwaccel vaapi -vaapi_device /dev/dri/renderD128 \
>>> -hwaccel_output_format vaapi -i input.mp4 -an -c:v h264_vaapi \
>>> -maxrate 6M -b:v 5M output_cbr.mp4
>>>
>>> then use the mediainfo check the bit rate control mode.
>>>
>>> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
>>
>> Not yet generating the HRD and timing information in VBR mode was deliberate, because I don't know enough about the conformance properties of the new proprietary rate controller which the Intel driver has switched to.  The old CBR mode was straightforward to verify from the source code, and it seemed reasonable to assume that the new CBR mode would be too given the constraints on it (and also because existing versions of lavc couldn't be modified).
>>
>> Can you say anything about the HRD conformance of the Intel driver here, or was this patch purely for the metadata output (which may not actually match the stream, hence my concern)?
>>
>> Only the Intel driver is relevant to this as far as I know - the Mesa driver does not support the packed headers necessary for this information to be included in the stream.
>>
>>
> 
> I don't know the details about Intel driver's HRD conformance model, this patch 
> just want to correct the bit rate mode metadata information.
> 
>>> ---
>>>  libavcodec/vaapi_encode_h264.c | 7 +++----
>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c
>>> index 92e2955..47d0c94 100644
>>> --- a/libavcodec/vaapi_encode_h264.c
>>> +++ b/libavcodec/vaapi_encode_h264.c
>>> @@ -760,7 +760,7 @@ static int vaapi_encode_h264_write_extra_header(AVCodecContext *avctx,
>>>      char tmp[256];
>>>      size_t header_len;
>>>  
>>> -    if (index == 0 && ctx->va_rc_mode == VA_RC_CBR) {
>>> +    if (index == 0 && (ctx->va_rc_mode == VA_RC_CBR || ctx->va_rc_mode == VA_RC_VBR)) {
>>>          *type = VAEncPackedHeaderH264_SEI;
>>>  
>>>          init_put_bits(&pbc, tmp, sizeof(tmp));
>>> @@ -868,7 +868,7 @@ static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
>>>              mseq->fixed_frame_rate_flag = 0;
>>>          }
>>>  
>>> -        if (ctx->va_rc_mode == VA_RC_CBR) {
>>> +        if (ctx->va_rc_mode == VA_RC_CBR || ctx->va_rc_mode == VA_RC_VBR) {
>>>              priv->send_timing_sei = 1;
>>>              mseq->nal_hrd_parameters_present_flag = 1;
>>>  
>>> @@ -886,8 +886,7 @@ static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
>>>              mseq->cpb_size_value_minus1[0] =
>>>                  (ctx->hrd_params.hrd.buffer_size >> mseq->cpb_size_scale + 4) - 1;
>>>  
>>> -            // CBR mode isn't actually available here, despite naming.
>>> -            mseq->cbr_flag[0] = 0;
>>> +            mseq->cbr_flag[0] = (ctx->va_rc_mode == VA_RC_CBR ? 1 : 0);
>>
>> As the comment notes, the necessary CBR mode isn't actually available.  Drivers only offer "soft" CBR, which can result in HRD overflow; this specifies "hard" CBR, so if you want to set this flag you would need to insert filler NAL units as well.
>>
> 
> In 1.8.0 release driver (https://lists.01.org/pipermail/intel-vaapi-media/2017-March/000016.html), 
> I can't reproduce the overflow issue with CBR mode. So I guess the new driver fix the HRD overflow
> issue (I don't dig in the 1.8.0 driver) .

Try encoding video of a solid colour:

ffmpeg -y -vaapi_device /dev/dri/renderD128 -f lavfi -i color=s=1280x720:r=30 -vf format=nv12,hwupload -c:v h264_vaapi -b 1M -maxrate 1M out.h264

I get output packets that are about 1600 bits each after the initial few.  With cbr_flag set the HSS must add 1000000 bits to the CPB every second, but we are only removing 30 packets totalling about 30*1600 = 48000 bits in that same time.  Hence it will always overflow after not very long.

Patch hide | download patch | download mbox

diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c
index 92e2955..47d0c94 100644
--- a/libavcodec/vaapi_encode_h264.c
+++ b/libavcodec/vaapi_encode_h264.c
@@ -760,7 +760,7 @@  static int vaapi_encode_h264_write_extra_header(AVCodecContext *avctx,
     char tmp[256];
     size_t header_len;
 
-    if (index == 0 && ctx->va_rc_mode == VA_RC_CBR) {
+    if (index == 0 && (ctx->va_rc_mode == VA_RC_CBR || ctx->va_rc_mode == VA_RC_VBR)) {
         *type = VAEncPackedHeaderH264_SEI;
 
         init_put_bits(&pbc, tmp, sizeof(tmp));
@@ -868,7 +868,7 @@  static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
             mseq->fixed_frame_rate_flag = 0;
         }
 
-        if (ctx->va_rc_mode == VA_RC_CBR) {
+        if (ctx->va_rc_mode == VA_RC_CBR || ctx->va_rc_mode == VA_RC_VBR) {
             priv->send_timing_sei = 1;
             mseq->nal_hrd_parameters_present_flag = 1;
 
@@ -886,8 +886,7 @@  static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
             mseq->cpb_size_value_minus1[0] =
                 (ctx->hrd_params.hrd.buffer_size >> mseq->cpb_size_scale + 4) - 1;
 
-            // CBR mode isn't actually available here, despite naming.
-            mseq->cbr_flag[0] = 0;
+            mseq->cbr_flag[0] = (ctx->va_rc_mode == VA_RC_CBR ? 1 : 0);
 
             mseq->initial_cpb_removal_delay_length_minus1 = 23;
             mseq->cpb_removal_delay_length_minus1         = 23;