[Bug 749950] encoder: VP8: Add CBR Encoding mode

[GStreamer (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <vjaquez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #345285|none                        |needs-work
             status|                            |

--- Comment #10 from Víctor Manuel Jáquez Leal <vjaquez at igalia.com> ---
Review of attachment 345285:
 --> (https://bugzilla.gnome.org/review?bug=749950&attachment=345285)

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?

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