[Bug 778732] Encoders: add VBR feature for AVC/VP8/HEVC/VP9

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=778732

--- Comment #7 from Hyunjun Ko <zzoon at igalia.com> ---
(In reply to Víctor Manuel Jáquez Leal from comment #5)
> 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;

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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.