GNOME Bugzilla – Bug 793705
msdk: Implement Video Post Processing element
Last modified: 2018-04-25 19:34:39 UTC
Add a VPP element in msdk plugin.
Created attachment 370068 [details] [review] add vpp element
Created attachment 370069 [details] [review] add denoise and rotation
Created attachment 370070 [details] [review] add deinterlace
Created attachment 370071 [details] [review] add procamp/colorbalance
Created attachment 370072 [details] [review] add edge enhancement
Created attachment 370073 [details] [review] add mirroring
Created attachment 370074 [details] [review] add scaling filter algorithms
Created attachment 370075 [details] [review] add force-aspect-ratio
Created attachment 370076 [details] [review] add frc
Review of attachment 370068 [details] [review]: ::: sys/msdk/gstmsdkvpp.c @@ +339,3 @@ + if (caps) + gst_caps_unref (caps); + Doesn't need to unref the caps. Otherwise it might cause an error like this. (gst-launch-1.0:23029): GStreamer-CRITICAL **: gst_mini_object_unref: assertion 'GST_MINI_OBJECT_REFCOUNT_VALUE (mini_object) > 0' failed @@ +795,3 @@ +static GstCaps * +gst_msdkvpp_fixate_caps (GstBaseTransform * trans, + GstPadDirection direction, GstCaps * caps, GstCaps * othercaps) This vmethod takes ownership of othercaps. You need to unref this caps.
(In reply to Hyunjun Ko from comment #10) > Review of attachment 370068 [details] [review] [review]: > > ::: sys/msdk/gstmsdkvpp.c > @@ +339,3 @@ > + if (caps) > + gst_caps_unref (caps); > + > > Doesn't need to unref the caps. Otherwise it might cause an error like this. > (gst-launch-1.0:23029): GStreamer-CRITICAL **: gst_mini_object_unref: > assertion 'GST_MINI_OBJECT_REFCOUNT_VALUE (mini_object) > 0' failed > > @@ +795,3 @@ > +static GstCaps * > +gst_msdkvpp_fixate_caps (GstBaseTransform * trans, > + GstPadDirection direction, GstCaps * caps, GstCaps * othercaps) > > This vmethod takes ownership of othercaps. > You need to unref this caps. Thank you for catching those! I will fix both of them in next iteration.
Review of attachment 370068 [details] [review]: ::: sys/msdk/gstmsdkvpp.c @@ +239,3 @@ + goto error_pool_config; + + *aligned_info = info; I can't find where aligned_info is used :)
Review of attachment 370068 [details] [review]: ::: sys/msdk/gstmsdkvpp.c @@ +239,3 @@ + goto error_pool_config; + + *aligned_info = info; aligned_info = &thiz->sinkpad_buffer_pool_info; Or aligned_info = &thiz->srcpad_buffer_pool_info; sinkpad_buffer_pool_info has been used in multiple places. srcpad_buffer_pool_info is required in future (copy the output buffer if downstream doesn't have videometa support)
(In reply to sreerenj from comment #13) > Review of attachment 370068 [details] [review] [review]: > > ::: sys/msdk/gstmsdkvpp.c > @@ +239,3 @@ > + goto error_pool_config; > + > + *aligned_info = info; > > aligned_info = &thiz->sinkpad_buffer_pool_info; Or aligned_info = > &thiz->srcpad_buffer_pool_info; > > sinkpad_buffer_pool_info has been used in multiple places. > srcpad_buffer_pool_info is required in future (copy the output buffer if > downstream doesn't have videometa support) Ah right. Sorry for noize.
Review of attachment 370068 [details] [review]: ::: sys/msdk/Makefile.am @@ +23,3 @@ gstmsdk.c \ + msdk-enums.c \ + gstmsdkvpputil.c nitpick: spaces vs tabs :) @@ +51,3 @@ + gstmsdkenc.h \ + gstmsdkvpp.h \ + gstmsdkvpputil.h ditto ::: sys/msdk/gstmsdkvpp.c @@ +58,3 @@ + "width = (int) [ 1, MAX ], height = (int) [ 1, MAX ]," + "interlace-mode = (string) progressive") + ); Isn't better use GST_VIDEO_CAPS_MAKE ("{ NV12, I420, YUY2, UYVY, BGRA }") ", interlace-mode = (string) progressive" ?? @@ +69,3 @@ + "width = (int) [ 1, MAX ], height = (int) [ 1, MAX ]," + "interlace-mode = (string) progressive") + ); Ditto @@ +83,3 @@ + +#define gst_msdkvpp_parent_class parent_class +G_DEFINE_TYPE (GstMsdkVPP, gst_msdkvpp, GST_TYPE_BASE_TRANSFORM); If msdkvpp is not going to double the frame-rate (two output frames per one input) when deinterlacing, perhaps it would make sense to inherit from GstVideoFilter @@ +239,3 @@ + goto error_pool_config; + + *aligned_info = info; I would rename "aligned_info" with "pool_info" and add a comment before updating, something like: /* updating pool info with allocator's info */ @@ +315,3 @@ + pool = + gst_msdkvpp_create_buffer_pool (thiz, GST_PAD_SRC, caps, min_buffers); + thiz->srcpad_buffer_pool = pool; AFAIU, this assignation should be moved out of this block. What would happen if we reuse downstream pool? @@ +339,3 @@ + if (caps) + gst_caps_unref (caps); + Yup: caps in gst_query_parse_allocation() are not transfered: https://developer.gnome.org/gstreamer/stable/gstreamer-GstQuery.html#gst-query-parse-allocation @@ +381,3 @@ + } + + pool = thiz->sinkpad_buffer_pool; AFAIU, this is not OK. Every time upstream ask for a pool, we should provide a new instantiated pool. Consider tees or similars. @@ +437,3 @@ + +static MsdkSurface * +get_msdk_surface_from_input_buffer (GstMsdkVPP * thiz, GstBuffer * inbuf) This function really needs to be refactored and moved in to a common utility source file. @@ +515,3 @@ + if (status != MFX_WRN_DEVICE_BUSY) + break; + /* If device is busy, wait 1ms and retry, as per MSDK's recomendation */ Captain grammar here: "recommendation" :D @@ +533,3 @@ + status = MFX_ERR_NONE; + + gst_buffer_copy_into (outbuf, inbuf, GST_BUFFER_COPY_TIMESTAMPS, 0, -1); Keep in mind if it is required to copy, not only timestamps, but flags (sync) and non-memory metadata (such as ROI metadata)
Review of attachment 370069 [details] [review]: ::: sys/msdk/gstmsdkvpp.c @@ +736,3 @@ thiz->out_num_surfaces = request[1].NumFrameSuggested; + extra line?
Review of attachment 370068 [details] [review]: ::: sys/msdk/gstmsdkvpp.c @@ +701,3 @@ + thiz->param.NumExtParam = thiz->num_extra_params; + thiz->param.ExtParam = thiz->extra_params; + } The compilation will break at this point and commit, since these two members are not yet defined. Also, I wonder if GArray wouldn't make sense here.
Review of attachment 370070 [details] [review]: ::: sys/msdk/gstmsdkvpp.c @@ -60,1 @@ ); "interlace-mode = (string){ progressive, interleaved, mixed }" ???
Review of attachment 370076 [details] [review]: this is like the videorate element!! nice! ::: sys/msdk/gstmsdkvpp.c @@ +569,3 @@ + MFXVideoCORE_SyncOperation (session, sync_point, 10000); + + /* More than one output buffers are generated */ then GstVideoFilter class cannot be used...
(In reply to Víctor Manuel Jáquez Leal from comment #19) > Review of attachment 370076 [details] [review] [review]: > > this is like the videorate element!! nice! > > ::: sys/msdk/gstmsdkvpp.c > @@ +569,3 @@ > + MFXVideoCORE_SyncOperation (session, sync_point, 10000); > + > + /* More than one output buffers are generated */ > > then GstVideoFilter class cannot be used... Why? basetransform allows dropping frame. Even in gst-vaapi we use double-framerate in deinterlace usecases...
Review of attachment 370068 [details] [review]: ::: sys/msdk/gstmsdkvpp.c @@ +58,3 @@ + "width = (int) [ 1, MAX ], height = (int) [ 1, MAX ]," + "interlace-mode = (string) progressive") + ); Done. @@ +69,3 @@ + "width = (int) [ 1, MAX ], height = (int) [ 1, MAX ]," + "interlace-mode = (string) progressive") + ); Done! @@ +83,3 @@ + +#define gst_msdkvpp_parent_class parent_class +G_DEFINE_TYPE (GstMsdkVPP, gst_msdkvpp, GST_TYPE_BASE_TRANSFORM); MSDK has specific modes to double the frame rate in deinterlacing. There are some misunderstandings in the way we interpret fps in deinterlacing. MSDK requires the field-rate as input and frame-rate as output. Suppose h264parse negotiate "framerate=25": here msdk expect input rate 50 and output rate 25 (weave) or 50(bob). I will add some comments and fixes to the deinterlacing later on (not included in this patch series). I have to double check the parser code, if parser negotiate filed-rate as framerate, then we are not supposed to double it again :) @@ +239,3 @@ + goto error_pool_config; + + *aligned_info = info; Done. @@ +315,3 @@ + pool = + gst_msdkvpp_create_buffer_pool (thiz, GST_PAD_SRC, caps, min_buffers); + thiz->srcpad_buffer_pool = pool; I don't understand, reuse downstream pool should work as it is (update_pool = true) Do you mean to update the size/min+max_buffersize? If so, yes that part can be added. If there is a downstream msdk element (then only the pool will be reused) we should already have the correct size but min_max_count could be updated. @@ +339,3 @@ + if (caps) + gst_caps_unref (caps); + Done @@ +795,3 @@ +static GstCaps * +gst_msdkvpp_fixate_caps (GstBaseTransform * trans, + GstPadDirection direction, GstCaps * caps, GstCaps * othercaps) Done
Review of attachment 370068 [details] [review]: ::: sys/msdk/gstmsdkvpp.c @@ +437,3 @@ + +static MsdkSurface * +get_msdk_surface_from_input_buffer (GstMsdkVPP * thiz, GstBuffer * inbuf) Well, MsdkSurface itself is not needed unless we wanna support Async-depth > 1. Not sure whether it makes really sense to add Async-depth to vpp. @@ +515,3 @@ + if (status != MFX_WRN_DEVICE_BUSY) + break; + /* If device is busy, wait 1ms and retry, as per MSDK's recomendation */ Will fix it :) @@ +533,3 @@ + status = MFX_ERR_NONE; + + gst_buffer_copy_into (outbuf, inbuf, GST_BUFFER_COPY_TIMESTAMPS, 0, -1); Yup, we need to take few more stuff from gst-vaapi. Will do it in another patch. thanks. @@ +701,3 @@ + thiz->param.NumExtParam = thiz->num_extra_params; + thiz->param.ExtParam = thiz->extra_params; + } Aha, messed up the rebase I think ! Will fix it. GArray, well, maybe in the next iteration :), same could be needed for encoder too.
Review of attachment 370068 [details] [review]: ::: sys/msdk/gstmsdkvpp.c @@ +381,3 @@ + } + + pool = thiz->sinkpad_buffer_pool; Is it really neede? one vpp element supposed to accept only one type(resolution) of frames and why upstream need two pools? Maybe increase the max-buffers? This is a sample pipeline which works fine: DISPLAY=:0 GST_GL_PLATFORM=egl gst-launch-1.0 -q filesrc location=sample.mp4 ! qtdemux ! h264parse ! msdkh264dec ! tee name=t ! queue ! msdkvpp ! video/x-raw, width=640, height=480 ! glimagesink t. ! queue ! videoscale ! video/x-raw, width=320, height=240 ! videoconvert ! xvimagesink
I will push the updated patches and file new bugs for missing features and improvements.
Review of attachment 370068 [details] [review]: rejected
Review of attachment 370069 [details] [review]: obsolete
Review of attachment 370070 [details] [review]: obsolete
Review of attachment 370071 [details] [review]: obsolete
Review of attachment 370072 [details] [review]: obsolete
Review of attachment 370073 [details] [review]: obsolete
Review of attachment 370074 [details] [review]: obsolete
Review of attachment 370075 [details] [review]: obsolete
Review of attachment 370076 [details] [review]: obsolete
Commits: d3d89f02b34a71f0c62324d4223fd412a393466e 36e81744d1a3be9ecb1b55d5ce8f5ba7c5315368 f5a3d3d7995d537b282204f0f089c9a5c1d0aa56 93c5dd2478c6e92928be9e6a7c8ee90e7c7dee65 108c8fde7fc52c656d954288c71234c6f8ccd369 51b6345dc4666097b10daeb191efb963b682acdb fb8c536393294455636468e6c08169e2456f6eab c0ea4bdafb388af4a70fe14e62d2cd82873daa3f e4b4f0949613f53f3cd84e9ca6d5c7dfbec8f188
Review of attachment 370068 [details] [review]: ::: sys/msdk/gstmsdkvpp.c @@ +315,3 @@ + pool = + gst_msdkvpp_create_buffer_pool (thiz, GST_PAD_SRC, caps, min_buffers); + thiz->srcpad_buffer_pool = pool; I meant the assignation of thiz->srcpad_buffer_pool What if downstream offers a proper bufferpool, thus it is not required to be created, and thiz->srcpad_buffer_pool is not set @@ +381,3 @@ + } + + pool = thiz->sinkpad_buffer_pool; here's a long explanation: https://bugzilla.gnome.org/show_bug.cgi?id=748344 long story short: Caching pools makes certain renegotiation scenario fails. Create a new pool for each propose_allocation() call here. Allocator can be reused without problems.
(In reply to Víctor Manuel Jáquez Leal from comment #36) > Review of attachment 370068 [details] [review] [review]: > > ::: sys/msdk/gstmsdkvpp.c > @@ +315,3 @@ > + pool = > + gst_msdkvpp_create_buffer_pool (thiz, GST_PAD_SRC, caps, > min_buffers); > + thiz->srcpad_buffer_pool = pool; > > I meant the assignation of thiz->srcpad_buffer_pool > > What if downstream offers a proper bufferpool, thus it is not required to be > created, and thiz->srcpad_buffer_pool is not set Yup right! BTW, I think each msdk element should always create its own bufferpool in decide_allocation. Each of the MSDK components has to allocate resources (mfxSurfaces)separately even though we share the context. Which means each msdk element create a new pool in decide_allocation irrespective of what downstream provides. The unnecessary copy can be avoided in each element by checking whether the buffer is originated from a msdk pool.
Created attachment 370877 [details] [review] msdkvpp: Allocation query fixes
Review of attachment 370877 [details] [review]: ::: sys/msdk/gstmsdkvpp.c @@ +408,3 @@ gst_object_unref (thiz->sinkpad_buffer_pool); + thiz->sinkpad_buffer_pool = gst_msdkvpp_create_buffer_pool (thiz, + GST_PAD_SINK, caps, min_buffers); This code is not required. I mean, if downstream reject the pool, I imagine you could delay the instantiation of sinkpad_buffer_pool until you really need, avoid to have two pool dancing around.
(In reply to Víctor Manuel Jáquez Leal from comment #39) > Review of attachment 370877 [details] [review] [review]: > > ::: sys/msdk/gstmsdkvpp.c > @@ +408,3 @@ > gst_object_unref (thiz->sinkpad_buffer_pool); > + thiz->sinkpad_buffer_pool = gst_msdkvpp_create_buffer_pool (thiz, > + GST_PAD_SINK, caps, min_buffers); > > This code is not required. I mean, if downstream reject the pool, I imagine > you could delay the instantiation of sinkpad_buffer_pool until you really > need, avoid to have two pool dancing around. Then we need to cache the allocation query caps and later, just before performing the input buffer transformation, it has to create pool based this caps. I would rather prefer to keep the current code which maintains an internal pool which seems simpler to me.
Review of attachment 370877 [details] [review]: Pushed as 5184f85d77b9b09c8fec14cdd965143db5cfe710