GNOME Bugzilla – Bug 749950
encoder: VP8: Add CBR Encoding mode
Last modified: 2017-02-10 22:05:50 UTC
The va-intel-driver has CBR encoding support. We should enable this in gstreamer-vaapi too.
Created attachment 310524 [details] [review] encoder: VP8: add CBR encoding mode
I'm not sure of the HRD parameter. I ran these tests: 644M /home/vjaquez/1080p_blue_sky.yuv $ gst-launch-1.0 -ve webmmux name=mux ! filesink location=~/1080p_blue_sky_cbr.webm \ filesrc location=~/1080p_blue_sky.yuv ! videoparse format=i420 width=1920 height=1080 ! progressreport ! vaapiencode_vp8 rate-control=cbr ! mux. 203M /home/vjaquez/1080p_blue_sky_cbr.webm (this one shows some artifacts at the end of the stream) $ gst-launch-1.0 -ve webmmux name=mux ! filesink location=~/1080p_blue_sky.webm \ filesrc location=~/1080p_blue_sky.yuv ! videoparse format=i420 width=1920 height=1080 ! progressreport ! vaapiencode_vp8 ! mux. 16M /home/vjaquez/1080p_blue_sky.webm ... $ gst-launch-1.0 -ve v4l2src device=/dev/video0 num-buffers=1800 ! video/x-raw,format=I420,width=640,height=480 ! vaapipostproc ! queue ! vaapiencode_vp8 rate-control=cbr ! webmmux ! filesink location=~/cam_cbr.webm 21M /home/vjaquez/cam_cbr.webm $ gst-launch-1.0 -ve v4l2src device=/dev/video0 num-buffers=1800 ! video/x-raw,format=I420,width=640,height=480 ! vaapipostproc ! queue ! vaapiencode_vp8 ! webmmux ! filesink location=~/cam.webm 15M /home/vjaquez/cam.webm
Did you test it with bitrate="some value in kbps" It is not working for me when enabling "rate-control=cbr bitrate=4096" in vaapiencode_vp8 (tested sample 1080p_blue_sky.yuv).. Please check the bitrate of encoded bitstream... HRD parameters are not needed for vp8 :)
(In reply to sreerenj from comment #3) > Did you test it with bitrate="some value in kbps" > > It is not working for me when enabling "rate-control=cbr bitrate=4096" in > vaapiencode_vp8 (tested sample 1080p_blue_sky.yuv).. Please check the > bitrate of encoded bitstream... Yup. I just tested gst-launch-1.0 -ve filesrc location=~/1080p_blue_sky.yuv ! videoparse format=i420 width=1920 height=1080 ! progressreport ! vaapiencode_vp8 rate-control=cbr bitrate=40000 ! webmmux ! filesink location=~/1080p_blue_sky-40000.webm And the output is the same as with the "default" bitrate: 203M /home/vjaquez/1080p_blue_sky-40000.webm > > HRD parameters are not needed for vp8 :) The libva-intel-driver demands it: http://cgit.freedesktop.org/vaapi/intel-driver/tree/src/gen8_mfc.c#n3287
(In reply to Víctor Manuel Jáquez Leal from comment #4) > (In reply to > > HRD parameters are not needed for vp8 :) > > The libva-intel-driver demands it: > > http://cgit.freedesktop.org/vaapi/intel-driver/tree/src/gen8_mfc.c#n3287 Aha, my mistake, sorry :(
Created attachment 345284 [details] [review] libs: encoder: vp8: fix calculation of bitrate with aligned to Kbps Base encoder's the unit of bitrate is Kbps. We should honor it so that we use the value of bitrate directly.
Created attachment 345285 [details] [review] libs: encoder: vp8: add CBR encoding mode
On CBR mode, it requires clamp_qindex_high/low in VAEncPictureParameterBufferVP8, otherwise quantizer is set to 0 by default, which means that encoder doesn't use it during calculation of rate control algorithm. I set to 0 as min, 127 as max, which is allowed. Regarding calculation of HRD parameter, I refered to libyami's code, but I'm not sure this does make sense.
Review of attachment 345284 [details] [review]: lgtm
Review of attachment 345285 [details] [review]: almost there :) Could you also add in the commit description "Original-patch-by: blah blah"? ;) ::: gst-libs/gst/vaapi/gstvaapiencoder_vp8.c @@ +269,3 @@ + + misc = GST_VAAPI_ENC_MISC_PARAM_NEW (FrameRate, encoder); + g_assert (misc); this assert is not required since next instruction is to bailout if it's NULL @@ +281,3 @@ + + misc = GST_VAAPI_ENC_MISC_PARAM_NEW (HRD, encoder); + g_assert (misc); ditto @@ +302,3 @@ + VAEncMiscParameterRateControl *rate_control; + misc = GST_VAAPI_ENC_MISC_PARAM_NEW (RateControl, encoder); + g_assert (misc); ditto @@ +310,3 @@ + rate_control->bits_per_second = base_encoder->bitrate * 1000; + rate_control->target_percentage = 70; + rate_control->window_size = DEFAULT_CPB_LENGTH; I prefer to avoid macros for these magic numbers. Just paste the number and add a comment if you want to clarify what it means and where it comes. @@ +368,3 @@ pic_param->sharpness_level = encoder->sharpness_level; + /* For CBR */ shouldn't this be assigned only if CBR is requested?
Review of attachment 345285 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiencoder_vp8.c @@ +299,3 @@ + + /* RateControl params */ + if (GST_VAAPI_ENCODER_RATE_CONTROL (encoder) == GST_VAAPI_RATECONTROL_CBR) { this branch is spurious since this check was done at the beginning of this function (line 267)
(In reply to Víctor Manuel Jáquez Leal from comment #10) > Review of attachment 345285 [details] [review] [review]: > > @@ +368,3 @@ > pic_param->sharpness_level = encoder->sharpness_level; > > + /* For CBR */ > > shouldn't this be assigned only if CBR is requested? Those parameters are used only in CBR mode in driver. In case of CQP, they are not used.
Created attachment 345391 [details] [review] libs: encoder: vp8: add CBR encoding mode
Review of attachment 345391 [details] [review]: Excepting what I just commented, it lgtm.... well, another nitpick, I would set a more detailed commit log comment, saying a little bit more detailed what was done. Anyhow, as these are just small things, I'll update the patch and commit it. Thanks. ::: gst-libs/gst/vaapi/gstvaapiencoder_vp8.c @@ +300,3 @@ + misc = GST_VAAPI_ENC_MISC_PARAM_NEW (RateControl, encoder); + if (!misc) + return FALSE; I would keep the same code style as above and move this out from the scope. @@ +303,3 @@ + + rate_control = misc->data; + memset (rate_control, 0, sizeof (VAEncMiscParameterRateControl)); There's no need to set to zero the structure, it's already done in GST_VAAPI_ENC_MISC_PARAM_NEW @@ +312,3 @@ + + gst_vaapi_enc_picture_add_misc_param (picture, misc); + gst_vaapi_codec_object_replace (&misc, NULL); ditto
Review of attachment 345391 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiencoder_vp8.c @@ +307,3 @@ + rate_control->target_percentage = 70; + /* CPB(Coded picture buffer) length, which could proveided as a property */ + rate_control->window_size = 1500; why 1500? libyami uses 500 ms
Attachment 345391 [details] pushed as b6a3e88 - libs: encoder: vp8: add CBR encoding mode I modified it a bit according to comments Attachment 345284 [details] pushed as ffc5b43 - libs: encoder: vp8: fix bitrate calculation