[Bug 791637] msdk: encode: Add more tuning options

[GStreamer (GNOME Bugzilla)]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #368453|none                        |reviewed
             status|                            |

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

Overall, the approach looks OK.

Just a couple nitpicks

::: sys/msdk/gstmsdkenc.c
@@ +1262,3 @@
 }

+

This patch looks overly complex because of the code swapping. Could be possible
to split it two patches, one for the code move and other for the new functions,
if the move of code is truly required.

::: sys/msdk/gstmsdkenc.h
@@ +55,2 @@
 #define MAX_EXTRA_PARAMS 8
+#define GST_MSDKENC_COMMON_PROPERTIES 13

I don't see the need to define this macro only. I would expose in the header
the whole enum, with a proper name space:

enum
{
  GST_MSDKENC_PROP_HARDWARE = 1,
  GST_MSDKENC_PROP_ASYNC_DEPTH,
  GST_MSDKENC_PROP_TARGET_USAGE,
  GST_MSDKENC_PROP_RATE_CONTROL,
  GST_MSDKENC_PROP_BITRATE,
  GST_MSDKENC_PROP_MAX_FRAME_SIZE,
  GST_MSDKENC_PROP_MAX_VBV_BITRATE,
  GST_MSDKENC_PROP_AVBR_ACCURACY,
  GST_MSDKENC_PROP_AVBR_CONVERGENCE,
  GST_MSDKENC_PROP_RC_LOOKAHEAD_DEPTH,
  GST_MSDKENC_PROP_QPI,
  GST_MSDKENC_PROP_QPP,
  GST_MSDKENC_PROP_QPB,
  GST_MSDKENC_PROP_GOP_SIZE,
  GST_MSDKENC_PROP_REF_FRAMES,
  GST_MSDKENC_PROP_I_FRAMES,
  GST_MSDKENC_PROP_B_FRAMES,
  GST_MSDKENC_PROP_NUM_SLICES,
  GST_MSDKENC_PROP_MBBRC,
  GST_MSDKENC_PROP_ADAPTIVE_I,
  GST_MSDKENC_PROP_ADAPTIVE_B,
  GST_MSDKENC_PROP_MAX
};


Later, others inherited classes could look for those properties by its ID, and
also could use GST_MSDKENC_PROP_MAX for the next ID value for their own
properties.

@@ +143,3 @@
+gboolean
+gst_msdkenc_get_common_property (GObject * object, guint prop_id,
+                              GValue * value, GParamSpec * pspec);

Indentation doesn't look right.

::: sys/msdk/gstmsdkh264enc.c
@@ +44,3 @@
 enum
 {
+  PROP_CABAC = GST_MSDKENC_COMMON_PROPERTIES + 1,

For example this would be:

PROP_CABAC = GST_MSDKENC_MAX,

::: sys/msdk/gstmsdkh265enc.c
@@ +188,3 @@

+  gobject_class->set_property = gst_msdkh265enc_set_property;
+  gobject_class->get_property = gst_msdkh265enc_get_property;

As this is a pattern that repeats in several encodesr (h265, mpeg2 and vp8) I
would add helper functions in the base case for set and get properties:

g_object_class->set_property = gst_msdkenc_set_property;
g_object_class->get_property = gst_msdkenc_get_property;

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