GNOME Bugzilla – Bug 778732
Encoders: add VBR feature for AVC/VP8/HEVC
Last modified: 2017-06-07 15:42:18 UTC
The request is about VBR.As the encoder in VAAPI driver is improved and new feature VBR is added for AVC/VP8/HEVC. Now upstreaming is on-going. here VP9 is ready in driver. The middleware should support this.
Created attachment 352506 [details] [review] libs: encoder: h265: Adds VBR Encoding support Enables Variable BitRate mode, which does set FrameRate and RateControl parameters.
Created attachment 352507 [details] [review] libs: encoder: vp8: Adds VBR Encoding support
Note that, regarding vp9, there was already the issue. See bug #766832
Review of attachment 352506 [details] [review]: a couple comments ::: gst-libs/gst/vaapi/gstvaapiencoder_h265.c @@ +1579,2 @@ pic_param->pic_fields.bits.cu_qp_delta_enabled_flag = TRUE; + since coding unit quantization parameter changes are required for non-constant-QP modes, I would rewrite this as pic_param->pic_fields.bits.cu_qp_delta_enabled_flag = GST_VAAPI_ENCODER_RATE_CONTROL (encoder) != GST_VAAPI_RATECONTROL_CQP; @@ +1805,3 @@ + memset (rate_control, 0, sizeof (VAEncMiscParameterRateControl)); + rate_control->bits_per_second = encoder->bitrate_bits; + rate_control->target_percentage = 70; I'm a bit worried about this target_percentage, the hard-coded value (70) it is copied again and again like a some kind of cargo cult. /* this is the bit-rate the rate control is targeting, as a percentage of the maximum * bit-rate for example if target_percentage is 95 then the rate control will target * a bit-rate that is 95% of the maximum bit-rate */ looking at the ffmpeg implementation of the vaapi encoder, the target_percentage is 100% if the rate-control is CBR. Otherwise a ad-hoc formula is used.
Review of attachment 352507 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiencoder_vp8.c @@ +224,3 @@ + if (GST_VAAPI_ENCODER_RATE_CONTROL (encoder) & GST_VAAPI_RATECONTROL_CBR || + GST_VAAPI_ENCODER_RATE_CONTROL (encoder) & GST_VAAPI_RATECONTROL_VBR) This is wrong: GstVaapiRateControl is not a bitwise enum, is a normal enum. So no bitwise operations are allowed @@ +271,3 @@ + if (GST_VAAPI_ENCODER_RATE_CONTROL (encoder) != GST_VAAPI_RATECONTROL_CBR && + GST_VAAPI_ENCODER_RATE_CONTROL (encoder) != GST_VAAPI_RATECONTROL_VBR) why not just if (GST_VAAPI_ENCODER_RATE_CONTROL (encoder) == GST_VAAPI_RATECONTROL_CQP) return; ??
(In reply to Víctor Manuel Jáquez Leal from comment #5) > Review of attachment 352506 [details] [review] [review]: > > a couple comments > > ::: gst-libs/gst/vaapi/gstvaapiencoder_h265.c > @@ +1579,2 @@ > pic_param->pic_fields.bits.cu_qp_delta_enabled_flag = TRUE; > + > > since coding unit quantization parameter changes are required for > non-constant-QP modes, I would rewrite this as > > pic_param->pic_fields.bits.cu_qp_delta_enabled_flag = > GST_VAAPI_ENCODER_RATE_CONTROL (encoder) != GST_VAAPI_RATECONTROL_CQP; I like this better. Thanks! > > @@ +1805,3 @@ > + memset (rate_control, 0, sizeof (VAEncMiscParameterRateControl)); > + rate_control->bits_per_second = encoder->bitrate_bits; > + rate_control->target_percentage = 70; > > I'm a bit worried about this target_percentage, the hard-coded value (70) it > is copied again and again like a some kind of cargo cult. > > /* this is the bit-rate the rate control is targeting, as a percentage > of the maximum > * bit-rate for example if target_percentage is 95 then the rate control > will target > * a bit-rate that is 95% of the maximum bit-rate > */ > > looking at the ffmpeg implementation of the vaapi encoder, the > target_percentage is 100% if the rate-control is CBR. Otherwise a ad-hoc > formula is used. I agree with your concern. Probably we can think about this more and provide another patch only for this, including h264/h265/vp8/vp9.
Created attachment 353042 [details] [review] libs: encoder: h265: Adds VBR Encoding support Enables Variable BitRate mode, which does set FrameRate and RateControl parameters.
Created attachment 353043 [details] [review] libs: encoder: vp8: Adds VBR Encoding support
Created attachment 353044 [details] [review] libs: encoder: Describes more detail about the bitrate property
Created attachment 353056 [details] [review] libs: encoder: h265: Adds VBR Encoding support
Created attachment 353057 [details] [review] libs: encoder: vp8: Adds VBR Encoding support
Created attachment 353295 [details] [review] libs: encoder: h265: Adds VBR Encoding support Enables Variable BitRate mode, which does set FrameRate and RateControl parameters.
Created attachment 353296 [details] [review] libs: encoder: vp8: Adds VBR Encoding support
y # Bug 778732 - Encoders: add VBR feature for AVC/VP8/HEVC - NEW Attachment 353044 [details] pushed as cbd912b - libs: encoder: Describes more detail about the bitrate property Attachment 353295 [details] pushed as f68d045 - libs: encoder: h265: Adds VBR Encoding support Attachment 353296 [details] pushed as 5d345b0 - libs: encoder: vp8: Adds VBR Encoding support