After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 749950 - encoder: VP8: Add CBR Encoding mode
encoder: VP8: Add CBR Encoding mode
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
git master
Other Linux
: High enhancement
: 1.11.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 758397
 
 
Reported: 2015-05-27 10:13 UTC by sreerenj
Modified: 2017-02-10 22:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
encoder: VP8: add CBR encoding mode (3.01 KB, patch)
2015-09-02 15:48 UTC, Víctor Manuel Jáquez Leal
none Details | Review
libs: encoder: vp8: fix calculation of bitrate with aligned to Kbps (1.56 KB, patch)
2017-02-09 03:47 UTC, Hyunjun Ko
committed Details | Review
libs: encoder: vp8: add CBR encoding mode (4.00 KB, patch)
2017-02-09 03:48 UTC, Hyunjun Ko
none Details | Review
libs: encoder: vp8: add CBR encoding mode (3.64 KB, patch)
2017-02-10 00:56 UTC, Hyunjun Ko
committed Details | Review

Description sreerenj 2015-05-27 10:13:09 UTC
The va-intel-driver has CBR encoding support. We should enable this in gstreamer-vaapi too.
Comment 1 Víctor Manuel Jáquez Leal 2015-09-02 15:48:20 UTC
Created attachment 310524 [details] [review]
encoder: VP8: add CBR encoding mode
Comment 2 Víctor Manuel Jáquez Leal 2015-09-02 15:54:03 UTC
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
Comment 3 sreerenj 2015-09-07 14:14:39 UTC
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 :)
Comment 4 Víctor Manuel Jáquez Leal 2015-09-07 14:38:59 UTC
(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
Comment 5 sreerenj 2015-09-07 14:41:54 UTC
(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 :(
Comment 6 Hyunjun Ko 2017-02-09 03:47:34 UTC
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.
Comment 7 Hyunjun Ko 2017-02-09 03:48:04 UTC
Created attachment 345285 [details] [review]
libs: encoder: vp8: add CBR encoding mode
Comment 8 Hyunjun Ko 2017-02-09 06:12:20 UTC
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.
Comment 9 Víctor Manuel Jáquez Leal 2017-02-09 11:35:54 UTC
Review of attachment 345284 [details] [review]:

lgtm
Comment 10 Víctor Manuel Jáquez Leal 2017-02-09 11:41:55 UTC
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?
Comment 11 Víctor Manuel Jáquez Leal 2017-02-09 12:14:08 UTC
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)
Comment 12 Hyunjun Ko 2017-02-10 00:55:15 UTC
(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.
Comment 13 Hyunjun Ko 2017-02-10 00:56:05 UTC
Created attachment 345391 [details] [review]
libs: encoder: vp8: add CBR encoding mode
Comment 14 Víctor Manuel Jáquez Leal 2017-02-10 09:46:06 UTC
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
Comment 15 Víctor Manuel Jáquez Leal 2017-02-10 10:10:26 UTC
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
Comment 16 Víctor Manuel Jáquez Leal 2017-02-10 22:04:42 UTC
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