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

[GStreamer (GNOME Bugzilla)]

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

--- Comment #17 from sreerenj <bsreerenj at gmail.com> ---
(In reply to Víctor Manuel Jáquez Leal from comment #16)
> Review of attachment 368453 [details] [review]:
> 
> 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.

Yup, much better. 
I kept whatever code existed before as it is and added news stuff on top :)
Will change it. Thanks!


> 
> @@ +143,3 @@
> +gboolean
> +gst_msdkenc_get_common_property (GObject * object, guint prop_id,
> +                              GValue * value, GParamSpec * pspec);
> 
> Indentation doesn't look right.
> 

Yup, fix on the way.

> ::: 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;

There are other routines like this we can move too. But I think it is better to
keep everything simple like other gstreamer elements than providing helper
function like we did in gstreamer-vaapi.

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