| Message ID | 2d9a3b36-3afc-883a-a7a5-62c508adc562@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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.
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;