GNOME Bugzilla – Bug 790999
only vaapisink should query for app context
Last modified: 2017-12-06 21:13:07 UTC
Right now there are two contexts that are queried by GstVaapi: the old gst.vaapi.Display which contains a GstVaapiDisplay object and the new gst.vaapi.app.Display which contains a VADisplay opaque structure and a native display object (right now X Display, but soon wl display). The purpose of the last one is to applications to set their own VADisplay when using vaapisink in overlay mode, since gstreamer-vaapi doesn't expose a library for GstVaapiDisplay. Currenty all gstreamer-vaapi elements query for both context types, though the first one is desgined to be shared among the pipeline, and the second only make sense when vaapisink is used, and creates it's own GstVaapiDisplay object (which most be shared among the pipeline afterwards). I think there is a bug: only vaapisink should query for gst.vaapi.app.Display, and the rest of vaapi elements for gst.vaapi.Display. Even more, when vaapisink creates its own GstVaapiDisplay from gst.vaapi.app.Display, it should be propagated (the GstVaapiDisplay context not the other one).
Created attachment 364635 [details] [review] videocontext: only vaapisink queries for app context App context is made for applications that will provide, via GstContext, the VA display and the native display to use by the pipeline, when are using vaapisink as overlay. There ir no use case for encoders, decoders, neither for the postprocessor.
Just a question, if the decoder get created and activated before the display sink, will it manage to use the right dsiplay ?
(In reply to Nicolas Dufresne (stormer) from comment #2) > Just a question, if the decoder get created and activated before the display > sink, will it manage to use the right dsiplay ? They will create the GstVaapiDisplay as usual, by creating a new VADisplay and trying to detect the native window and platform, and creating a new native display. Still, this is a good question, because with this patch, the vaapisink doesn't query upstream for an already created GstVaapiDisplay. Thanks!
Created attachment 364675 [details] [review] videocontext: only vaapisink queries for app context App context is made for applications that will provide, via GstContext, the VA display and the native display to use by the pipeline, when are using vaapisink as overlay. There ir no use case for encoders, decoders, neither for the postprocessor. In the case of the vaapisink, it will query for gst.vaapi.Display to upstream first, and if there is no reply, a gst.vaapi.app.Display context will be posted as a bus message to the application. If replied, a GstVaapiDisplay object is instantiated given the context info, otherwise a GstVaapiDisplay is created with the normal algorithm to guess the graphics platform. Either way, the instantiated GstVaapiDisplay is propagated among the pipeline.
Review of attachment 364675 [details] [review]: ::: gst/vaapi/gstvaapipluginutil.c @@ +283,3 @@ + if (GST_IS_VIDEO_SINK (element)) { + /* Propagate if display was created from application */ + gst_vaapi_video_context_propagate (element, plugin->display); This is wrong. If the sink find a GstVaapiDisplay in upstream, it will propagate it again, which is not intended.
Created attachment 364791 [details] [review] vaapivideocontext: possible memleak when no bus attached
Created attachment 364792 [details] [review] vaapivideocontext: log the name of GstVaapiDisplay
Created attachment 364793 [details] [review] vaapivideocontext: only vaapisink process app context gst.vaapi.app.Display context is made for applications that will provide the VA display and the native display to used by the pipeline, when are using vaapisink as overlay. There are no use case for encoders, decoders, neither for the postprocessor. In the case of the vaapisink, it shall query for gst.vaapi.Display upstream first, and then, if there is no reply, gst.vaapi.app.Display context will be posted in the bus for the application. If the application replies, a GstVaapiDisplay object is instantiated given the context info, otherwise a GstVaapiDisplay is created with the normal algorithm to guess the graphics platform. Either way, the instantiated GstVaapiDisplay is propagated among the pipeline and the have-message bus message. Also only vaapisink will process the gst.vaapi.app.Display, if and only if, it doesn't have a display already set. This is caused because if vaapisink is in a bin (playsink, for example) the need-context is posted twice, leading to an error state.
Review of attachment 364791 [details] [review]: Lgtm.
Review of attachment 364792 [details] [review]: Lgtm.
Review of attachment 364793 [details] [review]: Looks good, no more comments.
Attachment 364791 [details] pushed as 0438a3e - vaapivideocontext: possible memleak when no bus attached Attachment 364792 [details] pushed as 466c839 - vaapivideocontext: log the name of GstVaapiDisplay Attachment 364793 [details] pushed as 8f5933c - vaapivideocontext: only vaapisink process app context
Thanks for the review!
Comment on attachment 364791 [details] [review] vaapivideocontext: possible memleak when no bus attached >+ if (!gst_element_post_message (element, msg)) { > GST_CAT_INFO_OBJECT (GST_CAT_CONTEXT, element, "No bus attached"); >+ gst_message_unref (msg); >+ } This doesn't look right to me. gst_element_post_message() takes ownership of the message. It should take ownership in all cases.
Thanks for spotting, fixed: commit b0d41c5db8c29b3df2a5b23d3da55141d701b992 (HEAD -> master) Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Wed Dec 6 16:11:46 2017 -0500 videoconvert: gst_element_post_message() is transfer full on msg For this reson we need not to unref the message, even if it failed. commit f9a57ccece4552a609b4660d2d3152611f52e0de Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Wed Dec 6 16:11:25 2017 -0500 Revert "vaapivideocontext: possible memleak when no bus attached" This reverts commit 0438a3e62660e64ed390b6bb83bfb560b91664aa.