GNOME Bugzilla – Bug 703235
[gstreamer-1.2-port] Add support for GstContext
Last modified: 2013-09-28 07:34:38 UTC
This bug is to track the patches required to support GstContext (available in GStreamer 1.2) and deprecate GstVideoContext in gstreamer-vaapi
Created attachment 248020 [details] [review] initial port to GStreamer 1.2 https://bugzilla.gnome.org/show_bug.cgi?id=698054
Created attachment 248021 [details] [review] pluginutil: add gst_vaapi_create_display() https://bugzilla.gnome.org/show_bug.cgi?id=698054
Created attachment 248022 [details] [review] pluginutil: add locks This seems to be healthy. https://bugzilla.gnome.org/show_bug.cgi?id=698054
Created attachment 248023 [details] [review] display: add gstcontext utilities declared an use a boxed type for display https://bugzilla.gnome.org/show_bug.cgi?id=698054
Created attachment 248024 [details] [review] libs: add gstvaapivideocontext This is thin compat layer for the deprecated GstVideoContext. https://bugzilla.gnome.org/show_bug.cgi?id=698054
Created attachment 248025 [details] [review] postproc: set context https://bugzilla.gnome.org/show_bug.cgi?id=698054
Created attachment 248026 [details] [review] decode: set context https://bugzilla.gnome.org/show_bug.cgi?id=698054
Created attachment 248027 [details] [review] decode: query context https://bugzilla.gnome.org/show_bug.cgi?id=698054
Created attachment 248028 [details] [review] sink: set context https://bugzilla.gnome.org/show_bug.cgi?id=698054
Created attachment 248029 [details] [review] sink: handle events https://bugzilla.gnome.org/show_bug.cgi?id=698054
Created attachment 248030 [details] [review] sink: handle context query https://bugzilla.gnome.org/show_bug.cgi?id=698054
Review of attachment 248020 [details] [review]: This patch breaks vaapipostproc: somehow prevents the caps negotiation in the vaapipostproc sink pad.
Review of attachment 248021 [details] [review]: Safe enough to be moved to master too. Thanks.
Review of attachment 248020 [details] [review]: I changed this one to disable vaapipostproc altogether.
Review of attachment 248022 [details] [review]: This looks correct, but is it actually needed? i.e. do you have cases/ways to exhaust the need of this patch, like crashes without? Thanks.
Review of attachment 248023 [details] [review]: I would like gst-libs/ (libgstvaapi) to remain independent from core GStreamer features, unless they are self-contained like simple data structures. Maybe move that to gstpluginutil too?
Review of attachment 248024 [details] [review]: I would like gst-libs/ (libgstvaapi) to remain independent from core GStreamer features, unless they are self-contained like simple data structures. Maybe move that to gstpluginutil too? [Same comment as for the previous patch] :)
Review of attachment 248026 [details] [review]: Testing the hunks-commenting feature for finer grained comments. :) ::: gst/vaapi/gstvaapidecode.c @@ +526,3 @@ + gst_vaapi_display_unref(decode->set_display); + GST_INFO_OBJECT(element, "set display = %p", display); + decode->set_display = display; I think it's better to use gst_vaapi_display_replace() + gst_vaapi_display_unref() of the display we got from the GstStructure. That way, you could even get rid of the heavy GstObject locks. @@ +549,3 @@ + GstContext *context; + + decode->display = gst_vaapi_display_ref(decode->set_display); Are we guaranteed that display is NULL if set_display is set? If so, this means they are mutually exclusive and we can keep a single display var + possible use a signalling var instead (flag). If not, this means it's probably easier/safer to just use gst_vaapi_display_replace() here? ::: gst/vaapi/gstvaapidecode.h @@ +71,3 @@ GstVaapiDisplay *display; +#if GST_CHECK_VERSION(1,1,0) + GstVaapiDisplay *set_display; I am still looking for the exact meaning of "set_display". Is this a "foreign" display as in "some display the user asks the vaapidecode element to use"? If so, "foreign_display" is probably a more appropriate name? wrt. the other comment about whether set_display/display are mutually exclusive. If this is the case, we could keep display + add a is_foreign_display flag?
(In reply to comment #13) > Review of attachment 248021 [details] [review]: > > Safe enough to be moved to master too. Thanks. Did you push this? I don't see it in the gitorious repo :(
(In reply to comment #15) > Review of attachment 248022 [details] [review]: > > This looks correct, but is it actually needed? i.e. do you have cases/ways to > exhaust the need of this patch, like crashes without? Thanks. No, I didn't see any crash because of this, but I thought that it would be sane. Anyway I guess we could ditch this patch for the moment.
(In reply to comment #16) > Review of attachment 248023 [details] [review]: > > I would like gst-libs/ (libgstvaapi) to remain independent from core GStreamer > features, unless they are self-contained like simple data structures. Maybe > move that to gstpluginutil too? I've found a problem with my approach: the bins that would like to set context, would need to create the GstVaapiDisplay. That's the reason why egl offers an external library to be linked: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/egl/egl.h We would need to have a thin library that offer the management of this boxed structure by the user.
(In reply to comment #17) > Review of attachment 248024 [details] [review]: > > I would like gst-libs/ (libgstvaapi) to remain independent from core GStreamer > features, unless they are self-contained like simple data structures. Maybe > move that to gstpluginutil too? [Same comment as for the previous patch] :) Agree!
(In reply to comment #18) > Review of attachment 248026 [details] [review]: > > Testing the hunks-commenting feature for finer grained comments. :) > > ::: gst/vaapi/gstvaapidecode.c > @@ +526,3 @@ > + gst_vaapi_display_unref(decode->set_display); > + GST_INFO_OBJECT(element, "set display = %p", display); > + decode->set_display = display; > > I think it's better to use gst_vaapi_display_replace() + > gst_vaapi_display_unref() of the display we got from the GstStructure. That > way, you could even get rid of the heavy GstObject locks. I can do that. > > @@ +549,3 @@ > + GstContext *context; > + > + decode->display = gst_vaapi_display_ref(decode->set_display); > > Are we guaranteed that display is NULL if set_display is set? If so, this means > they are mutually exclusive and we can keep a single display var + possible use > a signalling var instead (flag). If not, this means it's probably easier/safer > to just use gst_vaapi_display_replace() here? > > ::: gst/vaapi/gstvaapidecode.h > @@ +71,3 @@ > GstVaapiDisplay *display; > +#if GST_CHECK_VERSION(1,1,0) > + GstVaapiDisplay *set_display; > > I am still looking for the exact meaning of "set_display". Is this a "foreign" > display as in "some display the user asks the vaapidecode element to use"? If > so, "foreign_display" is probably a more appropriate name? Yes, is the "foreign" display that comes from other element or from the bin, and it is going to be use as the element's display. It should be mutually exclusive, but I'm not sure that it will happen for sure. > > wrt. the other comment about whether set_display/display are mutually > exclusive. If this is the case, we could keep display + add a > is_foreign_display flag?
Created attachment 249036 [details] [review] enable compilation with GStreamer 1.2 This patch adds the GStreamer API version 1.2 in the configure option. Within this API version it is mandatory: * To avoid the use of GstSurfaceConverter API * To avoid the use of GstVideoContext API Also, as we find some issues with the caps negotiation, this patch also disables the compilation vaapipostproc element.
Created attachment 249037 [details] [review] pluginutil: add gst_vaapi_create_display() Move out the display creation to another function: gst_vaapi_create_display() It makes the code more readable.
Created attachment 249038 [details] [review] display: utilities for GstContext Declare an use a boxed type for display sharing through GstContext.
Created attachment 249039 [details] [review] pluginutil: compat layer for GstVideoContext Thin compat layer for the deprecated GstVideoContext. Add two functions only when compiled with Gstreamer v1.2: * gst_vaapi_video_context_prepare(): queries if a context is already set in the pipeline * gst_vaapi_video_context_propagate(): propagates the created context
Created attachment 249040 [details] [review] pluginutil: add gst_vaapi_display_found() This utility function checks if there's already an assigned display, either in the element's structure or in its context.
Created attachment 249041 [details] [review] decode: add context handling Added this new function for GStreamer v1.2: gst_vaapidecode_set_context(): handle the display context And handle the context query from downstream.
Created attachment 249042 [details] [review] sink: add context handling gst_vaapisink_set_context(): set the display context gst_vaapisink_event(): downstream event requesting a context And also handle the context query.
Created attachment 249213 [details] [review] enable compilation with GStreamer 1.2 This patch adds the GStreamer API version 1.2 in the configure option. Within this API version it is mandatory: * To avoid the use of GstSurfaceConverter API * To avoid the use of GstVideoContext API Also, as we find some issues with the caps negotiation, this patch also disables the compilation vaapipostproc element.
Created attachment 249214 [details] [review] display: utilities for GstContext Declare an use a boxed type for display sharing through GstContext.
Created attachment 249215 [details] [review] pluginutil: compat layer for GstVideoContext Thin compat layer for the deprecated GstVideoContext. Add two functions only when compiled with Gstreamer v1.2: * gst_vaapi_video_context_prepare(): queries if a context is already set in the pipeline * gst_vaapi_video_context_propagate(): propagates the created context
Created attachment 249216 [details] [review] pluginutil: add gst_vaapi_display_found() This utility function checks if there's already an assigned display, either in the element's structure or in its context.
Created attachment 249218 [details] [review] decode: add context handling Added this new function for GStreamer v1.2: gst_vaapidecode_set_context(): handle the display context And handle the context query from downstream.
Created attachment 249219 [details] [review] sink: add context handling gst_vaapisink_set_context(): set the display context gst_vaapisink_event(): downstream event requesting a context And also handle the context query.
Hi, I plan to merge "display: utilities for GstContext" and "pluguntil: compat layer for GstVideoContext" patches into a single one, while also providing a single file for the implementation: gstvaapivideocontext.[ch]. That should be enough then.
Ported to the latest GStreamer 1.2.0 APIs. It works for me, though fill free to re-open with new patches. In particular, gst_element_get_context() is gone so we cannot perform step (1) any longer. Tested with an explicit pipeline and I could indeed see that the GstVaapiDisplay gets propagated as described. However, through a playbin pipeline, the run_context_query() function fails to find any display from its downstream peers. Most likely normal since vaapidecode & vaapisink are not linked at that time. Though, there is room for improvement, especially for checking for display_types[].
commit 9e3d24c6696256afca1c3f382125636e1c536253 Author: Víctor Manuel Jáquez Leal <vjaquez@igalia.com> Date: Fri Jul 12 12:58:57 2013 -0400 plugins: add support for GstContext API. Add support for the new GstContext API from GStreamer 1.2.x. - implement the GstElement::set_context() hook ; - reply to the `context' query from downstream elements. https://bugzilla.gnome.org/show_bug.cgi?id=703235 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit c67b783275a3b9bad1a26d6895ae2a51dd218456 Author: Víctor Manuel Jáquez Leal <vjaquez@igalia.com> Date: Wed May 22 12:07:52 2013 -0400 plugins: add compat layer for GstVideoContext. Add thin compatibility layer for the deprecated GstVideoContext API. For GStreamer API >= 1.2, this involves the following two functions: - gst_vaapi_video_context_prepare(): queries if a context is already set in the pipeline ; - gst_vaapi_video_context_propagate(): propagates the newly-created context to the rest of the pipeline. https://bugzilla.gnome.org/show_bug.cgi?id=703235 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit f75762d910097f0fe4f730fad3dc9ff1d4985438 Author: Víctor Manuel Jáquez Leal <vjaquez@igalia.com> Date: Tue May 21 12:42:39 2013 -0400 plugins: initial port to GStreamer 1.2. Port vaapidecode and vaapisink plugins to GStreamer API >= 1.2. This is rather minimalistic so that to test the basic functionality. Disable vaapipostproc plugin for now as further polishing is needed. Also disable GstVideoContext interface support since this API is now gone in 1.2.x. This is preparatory work for GstContext support. https://bugzilla.gnome.org/show_bug.cgi?id=703235 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>
\o/
(In reply to comment #38) > Ported to the latest GStreamer 1.2.0 APIs. It works for me, though fill free to > re-open with new patches. In particular, gst_element_get_context() is gone so > we cannot perform step (1) any longer. Instead of that you implement GstElement::set_context in your element now and remember if any context of your interest is set. Also the query should be sent downstream, then upstream, then the need-context message. Just took a short look at these patches but this part seems to be missing.