GNOME Bugzilla – Bug 791637
msdk: encode: Add more tuning options
Last modified: 2018-06-22 14:34:43 UTC
MediaSDK is providing a number of tuning options. It doesn't make sense to add all of them as properties, but we should still add some of the important ones. I guess we need to change few of the property names in msdkXXXenc elements. Currently we have three properties qp-i, qp-p and qp-b to set quant values for each frame type. I would prefer either: A) qp, qp-ip and qp-ib (somewhat similar to gstremaer-vaapi , where qp-ip and qp-ib indicates the qp value difference (if there any) from i-frame) B) qp, qp-p, qp-b (qp is the general quant value, qp-p and qp-b are for P and B frames) For me, A seems to be clean. The user only need to set one qp value which is for I,P and B frames. Also there is no qp-i in JPEG encoder, so a generic "qp" is more preferred. Intelligent CQP BRC algorithm requires only one quantization value. So considering all these, I would go for option-A :)
It seems qp-i/p/b can be zero in msdk which means "no limitation on QA". If that is the case, it could be better stick with qp-i/qp-p/qp-b than "qp-difference". Also, jpegenc need separate range (1 to 100). Sorry for the noise.
I am adding the missing features and some code rearrange here: https://github.com/sreerenjb/gst-plugins-bad/commits/devel But maybe it is better to wait for patches from #790752 to land first because those are the core part of the plugins which is difficult to rebase.
Created attachment 368453 [details] [review] msdk: encoder: register only the required properties
Created attachment 368454 [details] [review] msdk: encoder: h265: generalize the behavior of "i-frames" property
Created attachment 368455 [details] [review] msdk: encode: Add property to set slice/partitioning
Created attachment 368456 [details] [review] msdk: encode: Add more rate control options
Created attachment 368457 [details] [review] msdk: h264_enc: Add LookaheadDownsampling support
Created attachment 368458 [details] [review] msdk: encoder: h264: Enable trellis quantization tuning
Created attachment 368459 [details] [review] msdk: move enum definitions to separate file
Created attachment 368460 [details] [review] msdk: h264_enc: Add slice size tuning option
Created attachment 368461 [details] [review] msdk: Add more tuning options
Created attachment 368462 [details] [review] msdk: h264_enc: Enable B-pyramid prediction support
Wondering whether it is the right time to push the patch series now since the pre-release is already out. Any thoughts?? I have done some basic testing(which passed of course) and but ideally, it requires extensive testing to make sure everything works as expected(eg: check slice size whether it works as expected based on the value user given).
It's a fairly new plugin and it's -bad, so I'd say get in as much as you can as soon as you can :)
(In reply to Tim-Philipp Müller from comment #14) > It's a fairly new plugin and it's -bad, so I'd say get in as much as you can > as soon as you can :) K, will do... thanks, Tim :)
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. @@ +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;
(In reply to Víctor Manuel Jáquez Leal from comment #16) > Review of attachment 368453 [details] [review] [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.
(In reply to sreerenj from comment #17) > > ::: 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. +1. Align myself with Sree for this.
Review of attachment 368453 [details] [review]: obsolete
Review of attachment 368454 [details] [review]: obsolete
Review of attachment 368462 [details] [review]: Obsolete
Review of attachment 368461 [details] [review]: Obsolete
Review of attachment 368460 [details] [review]: Obsolete
Review of attachment 368459 [details] [review]: Obsolete
Review of attachment 368458 [details] [review]: Obsolete
Review of attachment 368457 [details] [review]: Obsolete
Review of attachment 368456 [details] [review]: Obsolete
Review of attachment 368455 [details] [review]: Obsolete
Pushed the patches. Few of the properties which are currently available (not all of them are introduced by the above patch series) is not making any sense for vp8 encoder (eg: b-frame count).Some of the others have no clarity in msdk spec (eg: slice-count), not sure whether it can be treated as "segment count" for vp8. The vp8 encoder itself is not working for me. So until we have a msdk release (working solution)which can support vp8/vp9 encode, the main focus should be only to mpeg2/h264 and hevc.
Hi, looks like this patch have broken meson builds of MediaSDK plugin: https://github.com/GStreamer/gst-plugins-bad/commit/ddd02be0def33a9be313038bf409cf294c5d2b2d#diff-773108e4589d91efef04623ac5766186R22 Filename should be changed from 'msdk-enum.c' to 'msdk-enums.c' in meson.build script.
Disregard my previous comment, I've found that the fix is on the 1.14 branch already, it is just not have ben merged into master.