GNOME Bugzilla – Bug 757598
improve GstContext sharing
Last modified: 2015-11-10 14:35:50 UTC
Since Gstreamer 1.7, set_context() vmethod needs to be chained up with the parent class in order to broadcast all its contexts when the element is added into a bin: http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=d5ded1588920c4471eefe055d09095d9e5e989b5 There is no need to guard the call, because before GStreamer 1.7, the set_context() vmethod was NULL in the element class, hence the conditional call make it safe. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314843 [details] [review] plugin: chain up set_context() vmethod
Created attachment 314877 [details] [review] plugin: chain up set_context() vmethod Since Gstreamer 1.7, set_context() vmethod needs to be chained up with the parent class in order to broadcast all its contexts when the element is added into a bin: http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=d5ded1588920c4471eefe055d09095d9e5e989b5 There is no need to guard the call, because before GStreamer 1.7, the set_context() vmethod was NULL in the element class, hence the conditional call make it safe. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314878 [details] [review] gstvaapivideocontext: fix indentation gst-indent does not handle correctly some expression like function declaration with attributes, breaking the following expressions. This patch makes gst-indent to ignore the attributed function declartion so the followed function definition is not mangled, such as happened in commit b4154a Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314879 [details] [review] vaapivideocontext: refactor context category debug Refactor the extraction GST_CAT_CONTEXT logging using a only once initializator, so we could get the debug category from different code paths, safely. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314880 [details] [review] vaapivideocontext: refactor gst_vaapi_video_context_prepare() First, refactorized run_context_query() into _gst_context_run_query(), adding a new parameter: the pad direction, in order to simplify the code. Second, added a new helper function: _gst_context_query(), which is a generic context query function. It isolates the operation of running the query and sets the context if found, also it enhances the logs. _gst_context_query() is similar to the one used in GstGL. Perhaps, in the future this helper function will be merged into the core libraries of GStreamer. Finally, gst_vaapi_video_context_prepare() was rewritten to use _gst_context_query(). Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314881 [details] [review] vaapivideocontext: rename context structure The context structure is named "display" which is too generic. The contrary happens, for example, with GstGL, what uses the same name as the context, and its logs make more sense. This patch renames the context structure with the same name as the context, thus GST_PTR_FORMAT can pretty print it.
Created attachment 314882 [details] [review] plugins: set display through context Instead of setting the display to the plugin directly after its creation, do it through the gstreamer's context mechanism, avoiding double assignations. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314883 [details] [review] plugins: check if display is set in sync Since the context messages are sync'ed, the display assignation happens in the same thread, hence we can know if the display was found or not as soon we call for it. In order to take advantage of it, gst_vaapi_video_context_prepare() receives, as a new parameter, the address of the plugin's display, and reports back if the display was found and set. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314884 [details] [review] vaapivideocontext: add gst_vaapi_video_context_set_display() This function set the display to an already created context. This function is going to be used later. Also, gst_vaapi_video_context_new_with_display() now uses this function. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314885 [details] [review] plugin: don't loose previous context at query When processing the GST_CONTEXT_QUERY we should not loose the previous context in the query, we should only add our display structure. This patch copies the old context, if it is there, and stamp our display on it. Otherwise, a new context is created. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314886 [details] [review] plugins: fix context query handling The current context query handling design is flawed: the function gst_vaapi_reply_to_query() returns FALSE either if the query is not a GST_CONTEXT_QUERY of if the query could not be handled correctly. But the pad query function should handle differently each case. This patch changes the gst_vaapi_reply_to_query() for gst_vaapi_handle_context_query() and changes it usage in all the vaapi plugins to match the correct context query handling. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314887 [details] [review] plugins: don't create display at caps query Caps query can happen before the element has a bus. The display creation should be should occur on the context negotiation, when the bus is already configured. Then at caps query no display should be created. Instead of force the display creation, we graciously fail the allowed_caps() creation. This change only applies for vaapidecode and vaapisink. The vaapipostroc, as a basetransform descendant, seems to be not affected by this, nor the encoders. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314888 [details] [review] vaapidecode: return pad's template caps if no display A caps query can occur before the element has a display. In that case, the element can return its pad's template. But when the element already has a display, and the caps probe fails, the element shall return an empty caps, so the auto-plug could try with another decoder. If the element has a display and the caps probe works, then the computed caps should be returned. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314889 [details] [review] plugin: chain up set_context() vmethod Since Gstreamer 1.7, set_context() vmethod needs to be chained up with the parent class in order to broadcast all its contexts when the element is added into a bin: http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=d5ded1588920c4471eefe055d09095d9e5e989b5 There is no need to guard the call, because before GStreamer 1.7, the set_context() vmethod was NULL in the element class, hence the conditional call make it safe. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314890 [details] [review] gstvaapivideocontext: fix indentation gst-indent does not handle correctly some expression like function declaration with attributes, breaking the following expressions. This patch makes gst-indent to ignore the attributed function declartion so the followed function definition is not mangled, such as happened in commit b4154a Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314891 [details] [review] vaapivideocontext: refactor context category debug Refactor the extraction GST_CAT_CONTEXT logging using a only once initializator, so we could get the debug category from different code paths, safely. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314892 [details] [review] vaapivideocontext: refactor gst_vaapi_video_context_prepare() First, refactorized run_context_query() into _gst_context_run_query(), adding a new parameter: the pad direction, in order to simplify the code. Second, added a new helper function: _gst_context_query(), which is a generic context query function. It isolates the operation of running the query and sets the context if found, also it enhances the logs. _gst_context_query() is similar to the one used in GstGL. Perhaps, in the future this helper function will be merged into the core libraries of GStreamer. Finally, gst_vaapi_video_context_prepare() was rewritten to use _gst_context_query(). Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314893 [details] [review] vaapivideocontext: rename context structure The context structure is named "display" which is too generic. The contrary happens, for example, with GstGL, what uses the same name as the context, and its logs make more sense. This patch renames the context structure with the same name as the context, thus GST_PTR_FORMAT can pretty print it.
Created attachment 314894 [details] [review] plugins: set display through context Instead of setting the display to the plugin directly after its creation, do it through the gstreamer's context mechanism, avoiding double assignations. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314895 [details] [review] plugins: check if display is set in sync Since the context messages are sync'ed, the display assignation happens in the same thread, hence we can know if the display was found or not as soon we call for it. In order to take advantage of it, gst_vaapi_video_context_prepare() receives, as a new parameter, the address of the plugin's display, and reports back if the display was found and set. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314896 [details] [review] vaapivideocontext: add gst_vaapi_video_context_set_display() This function set the display to an already created context. This function is going to be used later. Also, gst_vaapi_video_context_new_with_display() now uses this function. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314897 [details] [review] plugin: don't lose previous context at query When processing the GST_CONTEXT_QUERY we should not lose the previous context in the query, we should only add our display structure. This patch copies the old context, if it is there, and stamp our display on it. Otherwise, a new context is created. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314898 [details] [review] plugins: fix context query handling The current context query handling design is flawed: the function gst_vaapi_reply_to_query() returns FALSE either if the query is not a GST_CONTEXT_QUERY of if the query could not be handled correctly. But the pad query function should handle differently each case. This patch changes the gst_vaapi_reply_to_query() for gst_vaapi_handle_context_query() and changes it usage in all the vaapi plugins to match the correct context query handling. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314899 [details] [review] plugins: don't create display at caps query Caps query can happen before the element has a bus. The display creation should be should occur on the context negotiation, when the bus is already configured. Then at caps query no display should be created. Instead of force the display creation, we graciously fail the allowed_caps() creation. This change only applies for vaapidecode and vaapisink. The vaapipostroc, as a basetransform descendant, seems to be not affected by this, nor the encoders. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Created attachment 314900 [details] [review] vaapidecode: return pad's template caps if no display A caps query can occur before the element has a display. In that case, the element can return its pad's template. But when the element already has a display, and the caps probe fails, the element shall return an empty caps, so the auto-plug could try with another decoder. If the element has a display and the caps probe works, then the computed caps should be returned. Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>
Though these patches don't solve a specific problem, they're a step forward. @sree, could you give a quick review? to see if everything makes sense to you.
Attachment 314889 [details] pushed as 1e96fae - plugin: chain up set_context() vmethod Attachment 314890 [details] pushed as c20318d - gstvaapivideocontext: fix indentation Attachment 314891 [details] pushed as 7e9ee7f - vaapivideocontext: refactor context category debug Attachment 314892 [details] pushed as 5d0ab36 - vaapivideocontext: refactor gst_vaapi_video_context_prepare() Attachment 314893 [details] pushed as 959c139 - vaapivideocontext: rename context structure Attachment 314894 [details] pushed as b45089f - plugins: set display through context Attachment 314895 [details] pushed as ef3fb4a - plugins: check if display is set in sync Attachment 314896 [details] pushed as ed280f5 - vaapivideocontext: add gst_vaapi_video_context_set_display() Attachment 314897 [details] pushed as 28c366a - plugin: don't lose previous context at query Attachment 314898 [details] pushed as b2707c8 - plugins: fix context query handling Attachment 314899 [details] pushed as d69f747 - plugins: don't create display at caps query Attachment 314900 [details] pushed as 75e7a0a - vaapidecode: return pad's template caps if no display
(In reply to Víctor Manuel Jáquez Leal from comment #26) > Though these patches don't solve a specific problem, they're a step forward. > > @sree, could you give a quick review? to see if everything makes sense to > you. @Victor, Sorry for the delay :(.. lgtm,...but honestly haven't done any depth code review...Hope you done basic tests...autoplugging, clutter-gst etc... Good work :)
(In reply to sreerenj from comment #28) > @Victor, Sorry for the delay :(.. > lgtm,...but honestly haven't done any depth code review...Hope you done > basic tests...autoplugging, clutter-gst etc... No problem. Yes, I tested it a bit before pushing. At least, in Gst 1.4, 1.6 and master, I haven't broke anything that weren't broke before :)