After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 766206 - plugins: negotiate GstGLContext during the GstContext sharing
plugins: negotiate GstGLContext during the GstContext sharing
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
git master
Other Linux
: Normal normal
: 1.9.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 724352 734093
Blocks: 767203
 
 
Reported: 2016-05-10 08:02 UTC by Víctor Manuel Jáquez Leal
Modified: 2016-07-27 13:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
plugins: add gst_vaapi_plugin_base_find_gl_context() (5.50 KB, patch)
2016-07-12 17:22 UTC, Víctor Manuel Jáquez Leal
none Details | Review
pluginutil: set GLX display type (1.83 KB, patch)
2016-07-12 17:22 UTC, Víctor Manuel Jáquez Leal
none Details | Review
vaapidecode: delay the GstVaapiDisplay instantiating (3.77 KB, patch)
2016-07-12 17:22 UTC, Víctor Manuel Jáquez Leal
none Details | Review
plugins: add gst_vaapi_plugin_base_find_gl_context() (5.49 KB, patch)
2016-07-13 15:11 UTC, Víctor Manuel Jáquez Leal
none Details | Review
plugins: add gst_vaapi_plugin_base_find_gl_context() (5.49 KB, patch)
2016-07-14 10:48 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
pluginutil: set GLX display type (1.83 KB, patch)
2016-07-14 10:48 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
vaapidecode: delay the GstVaapiDisplay instantiating (2.63 KB, patch)
2016-07-14 10:48 UTC, Víctor Manuel Jáquez Leal
none Details | Review
vaapidecode: delay the GstVaapiDisplay instantiating (3.21 KB, patch)
2016-07-14 11:43 UTC, Víctor Manuel Jáquez Leal
committed Details | Review

Description Víctor Manuel Jáquez Leal 2016-05-10 08:02:00 UTC
From the mailing list:

https://lists.freedesktop.org/archives/gstreamer-devel/2016-February/056469.html

---8<------

> - in gst_vaapi_texture_glx_new_wrapped(), a default gl_api seems to be                                                                                                                                           
> searched once by calling the method gl_get_current_api() before reception of                                                                                                                                     
> the query allocation containing my "gst.gl.GstGLContext"                                                                                                                                                         
> in my case, the GL version is "4.5.0 NVIDIA 358.16", GL_CONTEXT_PROFILE_MASK                                                                                                                                     
> = 0 (seems to be a bug on nvidia card) : the method gl_get_current_api()                                                                                                                                         
> can't decode the version and return GST_VAAPI_GL_API_NONE !!                                                                                                                                                     
> my gl_api (GL_API_OPENGL) contained in the GLContext (in the query                                                                                                                                               
> allocation) is never read, nor the environment variable GST_GL_API,                                                                                                                                              
> ........................................                                                                                                                                                                         
>                                                                                                                                                                                                                  
>                                                                                                                                                                                                                  
> to continue, I modify gl_get_current_api() to return GST_VAAPI_GL_API_OPENGL                                                                                                                                     

Perhaps vaapi element should query for the GstGLContext using the context
mechanism, rather than rely on the allocation query.

-----8<------
Comment 1 Víctor Manuel Jáquez Leal 2016-07-12 17:22:30 UTC
Created attachment 331372 [details] [review]
plugins: add gst_vaapi_plugin_base_find_gl_context()

Using the GstContext mechanism, it is possible to find if the pipeline
shares a GstGLContext, even if we are not to negotiating GLTextureUpload
meta. This is interesting because we could negotiate system memory caps
feature, but enable DMABuf if the GstGLContext is EGL with some extensions.

This patch exposes a new method gst_vaapi_plugin_base_find_gl_context().
Comment 2 Víctor Manuel Jáquez Leal 2016-07-12 17:22:35 UTC
Created attachment 331373 [details] [review]
pluginutil: set GLX display type

The function gst_vaapi_create_display_from_gl_context() cretes a
GstVaapiDisplay given a GstGLContext. But it didn't created a GLX VA display
when the GL platform was GLX, but a plain X11 VA display.

This patch fixes that, by querying the GL platform earlier.
Comment 3 Víctor Manuel Jáquez Leal 2016-07-12 17:22:41 UTC
Created attachment 331374 [details] [review]
vaapidecode: delay the GstVaapiDisplay instantiating

Delay the GstVaapiDisplay instantiating until the VA decoder has to be
initialized. In this way the element has more chances to find an already
created GstVaapiDisplay, or a GL context, in the pipeline.

In order to delay the VA display instantiating, it is no possible to force the
driver white list check when the element is changing state from NULL to READY
anymore, because there is not a VA display to check. Hence, the vmethod
change_state() is not handled anymore.

Nonetheless, this brings a regression: the driver white list check up is not
functional anymore. This regression will be addressed again in other task.
Comment 4 Víctor Manuel Jáquez Leal 2016-07-12 17:25:00 UTC
These patches work but with a regression: the driver white listing is ignored, since now the VA display is created when the decoder is in playing state.

Because of the regression, this fix depends on bug 734093 and bug 724352.
Comment 5 Matthew Waters (ystreet00) 2016-07-13 14:28:54 UTC
Review of attachment 331372 [details] [review]:

Looks mostly ok from the libgstgl side.

::: gst/vaapi/gstvaapivideocontext.c
@@ +27,3 @@
 #include "gstvaapivideocontext.h"
+#if USE_GST_GL_HELPERS
+# include <gst/gl/gstglcontext.h>

Any of the object specific headers aren't guarenteed to be straight includable. Use #include <gst/gl/gl.h>

@@ +266,3 @@
+  if (gl_context && gl_context_ptr) {
+    *gl_context_ptr = gl_context;
+    return TRUE;

This may not do what the function name implies.

One should probably assert that gl_context_ptr is non-NULL on function entry (otherwise there's no point calling the function) and always return TRUE when a gl_context is found.

::: gst/vaapi/gstvaapivideocontext.h
@@ +59,3 @@
+G_GNUC_INTERNAL
+gboolean
+gst_gl_find_local_context (GstElement * element, GstObject ** gl_context_ptr);

Creating a function in the gst_gl namespace isn't a good idea at all.
Comment 6 Víctor Manuel Jáquez Leal 2016-07-13 14:59:47 UTC
Thanks, Matthew, for your kind review!! :)

(In reply to Matthew Waters (ystreet00) from comment #5)
> Review of attachment 331372 [details] [review] [review]:
> 
> Looks mostly ok from the libgstgl side.
> 
> ::: gst/vaapi/gstvaapivideocontext.c
> @@ +27,3 @@
>  #include "gstvaapivideocontext.h"
> +#if USE_GST_GL_HELPERS
> +# include <gst/gl/gstglcontext.h>
> 
> Any of the object specific headers aren't guarenteed to be straight
> includable. Use #include <gst/gl/gl.h>
> 
> @@ +266,3 @@
> +  if (gl_context && gl_context_ptr) {
> +    *gl_context_ptr = gl_context;
> +    return TRUE;
> 
> This may not do what the function name implies.
> 
> One should probably assert that gl_context_ptr is non-NULL on function entry
> (otherwise there's no point calling the function) and always return TRUE
> when a gl_context is found.
> 
> ::: gst/vaapi/gstvaapivideocontext.h
> @@ +59,3 @@
> +G_GNUC_INTERNAL
> +gboolean
> +gst_gl_find_local_context (GstElement * element, GstObject **
> gl_context_ptr);
> 
> Creating a function in the gst_gl namespace isn't a good idea at all.

D'oh!
Comment 7 Víctor Manuel Jáquez Leal 2016-07-13 15:11:57 UTC
Created attachment 331420 [details] [review]
plugins: add gst_vaapi_plugin_base_find_gl_context()

Using the GstContext mechanism, it is possible to find if the pipeline
shares a GstGLContext, even if we are not to negotiating GLTextureUpload
meta. This is interesting because we could negotiate system memory caps
feature, but enable DMABuf if the GstGLContext is EGL with some extensions.
Comment 8 Víctor Manuel Jáquez Leal 2016-07-14 10:48:34 UTC
Created attachment 331479 [details] [review]
plugins: add gst_vaapi_plugin_base_find_gl_context()

Using the GstContext mechanism, it is possible to find if the pipeline
shares a GstGLContext, even if we are not to negotiating GLTextureUpload
meta. This is interesting because we could negotiate system memory caps
feature, but enable DMABuf if the GstGLContext is EGL with some extensions.
Comment 9 Víctor Manuel Jáquez Leal 2016-07-14 10:48:39 UTC
Created attachment 331480 [details] [review]
pluginutil: set GLX display type

The function gst_vaapi_create_display_from_gl_context() cretes a
GstVaapiDisplay given a GstGLContext. But it didn't created a GLX VA display
when the GL platform was GLX, but a plain X11 VA display.

This patch fixes that, by querying the GL platform earlier.
Comment 10 Víctor Manuel Jáquez Leal 2016-07-14 10:48:46 UTC
Created attachment 331481 [details] [review]
vaapidecode: delay the GstVaapiDisplay instantiating

Delay the GstVaapiDisplay instantiating until the VA decoder has to be
initialized. In this way the element has more chances to find an already
created GstVaapiDisplay, or a GL context, in the pipeline.
Comment 11 Víctor Manuel Jáquez Leal 2016-07-14 11:43:20 UTC
Created attachment 331488 [details] [review]
vaapidecode: delay the GstVaapiDisplay instantiating

Delay the GstVaapiDisplay instantiating until when changing the state from
READY to PAUSE. In this way the element has more chances to find an already
created GstVaapiDisplay, or a GL context, in the pipeline.
Comment 12 Víctor Manuel Jáquez Leal 2016-07-27 13:27:20 UTC
Attachment 331479 [details] pushed as 6049867 - plugins: add gst_vaapi_plugin_base_find_gl_context()
Attachment 331480 [details] pushed as 0faff37 - pluginutil: set GLX display type
Attachment 331488 [details] pushed as cc6c2d2 - vaapidecode: delay the GstVaapiDisplay instantiating