GNOME Bugzilla – Bug 777409
pluginbase: Query for OpenGL app_context
Last modified: 2017-02-01 16:25:11 UTC
Created attachment 343677 [details] [review] pluginbase: Query for OpenGL app_context Currently the VAAPI plugins perform a query for gst.gl.local_context in order to find an OpenGL context. I've run into some situations where this doesn't work, because the element is being used in a setting where the full pipeline hasn't yet been hooked up (this seems to happen in some decodebin configurations). In these cases, the query doesn't work because it can't make it all the way to the element which has the context. Normally, the last resort one can use to solve this problem is to set up a sync bus handler and provide a gst.gl.app_context, which the plugin will then use to construct a local context. This is the pattern used by most of the OpenGL plugins. However, the VAAPI plugins don't do this, so providing a context through a sync bus handler is not possible. I've put together a patch which adds this behavior, bringing the VAAPI plugins in line with how the rest of the OpenGL plugins work. The existing behavior of querying for a local_context is still preferred, however in cases where one can't be found, it now falls back to the app_context search. This allowed me to get around the situation I was facing, so I wanted to submit the patch upstream and hopefully have it accepted here so that others will not experience the same issue.
Created attachment 343678 [details] [review] pluginbase: Query for OpenGL app_context
Review of attachment 343678 [details] [review]: A couple of things need work on. ::: gst/vaapi/gstvaapipluginbase.c @@ +77,3 @@ plugin_set_display (plugin, display); + + gst_gl_handle_set_context (element, context, (GstGLDisplay **)&plugin->gl_display, (GstGLContext **)&plugin->gl_other_context); Doesn't this need #if USE_GST_GL_HELPERS around it? @@ +229,3 @@ + if (plugin->gl_display) { + GError *error = NULL; + gst_gl_display_create_context ((GstGLDisplay *)plugin->gl_display, (GstGLContext *)plugin->gl_other_context, (GstGLContext **)&gl_context, &error); This will race with any other element attempting to create an OpenGL context. You should follow the same pattern as the other OpenGL elements here: https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/gstglbasefilter.c#n330
Thanks Matt. I would recommend to update your patch with Matthew's comments. But, I will not merge it, at least for the moment, because I'm not sure if we want to create GstGLDisplay inside gstreamer-vaapi. But still, it is an interesting idea. Perhaps you can convince us :)
Created attachment 343734 [details] [review] pluginbase: Query for OpenGL app_context Thanks for your feedback. I've updated the patch to do the display thread loop as you mentioned, and it seems to work. Victor, all right, I'll see if I can convince you. :) This patch actually doesn't create a GstGLDisplay by itself, it can only query for one which has been set by the rest of the pipeline. The only thing that this change would allow is for the plugin to construct its own GstGLContext based on the display and possibly other_context that it found by querying the pipeline. So yes, we may construct a new context object, whereas before we could only query for one from somebody else. But we would always do it based on a display and other_context that was provided from outside. Does that sound permissible, or would you rather not create any OpenGL objects at all?
Created attachment 343736 [details] [review] pluginbase: Query for OpenGL app_context Fixed some rebase garbage.
Review of attachment 343736 [details] [review]: This looks good from the GL side :)
Review of attachment 343736 [details] [review]: ::: gst/vaapi/gstvaapipluginbase.c @@ +253,3 @@ + } while (!gst_gl_display_add_context (gl_display, (GstGLContext *)gl_context)); + GST_OBJECT_UNLOCK (gl_display); + } I would move all this block to a new function. ::: gst/vaapi/gstvaapivideocontext.c @@ +82,3 @@ + structure = gst_context_get_structure (context); + ret = gst_structure_get (structure, GST_VAAPI_DISPLAY_CONTEXT_TYPE_NAME, + GST_VAAPI_TYPE_DISPLAY, display_ptr, NULL); I guess you are working in branch 1.10 because this doesn't compile in master, since GST_VAAPI_TYPE_DISPLAY doesn't exist, it is GST_TYPE_VAAPI_DISPLAY.
Review of attachment 343736 [details] [review]: ::: gst/vaapi/gstvaapipluginbase.c @@ +81,3 @@ + +#if USE_GST_GL_HELPERS + gst_gl_handle_set_context (element, context, (GstGLDisplay **)&plugin->gl_display, (GstGLContext **)&plugin->gl_other_context); element doesn't exist either
Review of attachment 343736 [details] [review]: Also, please check you patch against gst-indent, to verify the project's indentation.
Created attachment 343911 [details] [review] pluginbase: Query for OpenGL app_context Sorry, my project is using an old version of GStreamer, so I don't have an easy way to develop against master. I've updated the patch to move the new code off to a function, and hopefully fix all of the build and indent errors. Let me know if I missed anything else.
Hi Matt, How's it going? How are you testing and with what intel-driver version? Sean
I'm testing using VA-API 0.39.3 on the i965 driver. However, the change being discussed here lies entirely within the GStreamer plugin itself, so it should be independent of the version of the underlying driver.
Review of attachment 343911 [details] [review]: It looks good in general. Just a couple nitpicks. What I'm worried now, is what will happen in non OpenGL environment? for example headless environment, or embedded (using kmssink, for example), or in Wayland (we have currently a couple problems there) ::: gst/vaapi/gstvaapipluginbase.c @@ +235,3 @@ + + GstGLDisplay *gl_display = (GstGLDisplay *) plugin->gl_display; + GstGLContext *gl_other_context = (GstGLContext *) plugin->gl_other_context; these two lines will raise a warning in ISO C90 compilers. @@ +236,3 @@ + GstGLDisplay *gl_display = (GstGLDisplay *) plugin->gl_display; + GstGLContext *gl_other_context = (GstGLContext *) plugin->gl_other_context; + if (gl_display) { the idiomatic style in gstreamer-vaapi has been -normally- early returns. I guess here applies.
Review of attachment 343911 [details] [review]: ::: gst/vaapi/gstvaapipluginbase.c @@ +232,3 @@ +#if USE_GST_GL_HELPERS + gst_gl_ensure_element_data (plugin, (GstGLDisplay **) & plugin->gl_display, + (GstGLContext **) & plugin->gl_other_context); another issue: here's a memory leak, since these two new member variables are never unrefed Do we really need to keep those objects around?
Created attachment 344069 [details] [review] pluginbase: Query for OpenGL app_context Here's an updated patch that fixes the ISO C90 error, uses an early return for the display check, and correctly unrefs the new variables. As far as what happens in non-OpenGL situations, basically all that will happen is that the app_context and display queries will return NULL, so we won't make a context and things will proceed the way they had in the past. This just provides a little bit of an extra opportunity for things to succeed that might not have been present before, but it doesn't change its ability to handle the case where it doesn't succeed. I also wish that we didn't need the gl_display and gl_other_context variables, but unfortunately the weird way that gst_gl_handle_set_context() works requires them. Basically we call gst_gl_ensure_element_data(), which will send out a query, and then the application will call set_context() on the element. So the set_context() function needs a place to save those items to so that they can be referenced once the gst_gl_ensure_element_data() function returns. I don't see a way to do that that doesn't require adding new variables into the element itself. I've added code to free them in the close() function, though, so hopefully they won't cause any problems.
(In reply to Matt Fischer from comment #15) > Created attachment 344069 [details] [review] [review] > pluginbase: Query for OpenGL app_context Matt, just note that it still leads to compilation failure because of this line :) ( there is no "element" ) +#if USE_GST_GL_HELPERS + gst_gl_handle_set_context (element, context, (GstGLDisplay **)&plugin->gl_display, (GstGLContext **)&plugin->gl_other_context); Anyway, I like the purpose of this patch. In addition, it fixes very old issue for vaapi/egl. Thanks Matt! But we should think about cases which don't need opengl stuff. For example, with this patch, it's not running with vaapisink.(on X/Wayland) Probably it's related to GLDisplay(X/Wayland), which is created already. With kmssink(on non X/Wayland), it fails to create gl context, and fails to play. With x(v)imagesink/waylandsink, it looks playing fine, but it also creates GLDisplay/GLContext which is not necessary.
Probably, there's something that I can try based on this patch. Victor, do you think it's worth trying?
(In reply to Hyunjun Ko from comment #17) > Probably, there's something that I can try based on this patch. > Victor, do you think it's worth trying? I don't understand what do you mean, but I guess it is worth. I also like the approach, but it needs work to handle all the use cases of gstreamer-vaapi
Created attachment 344161 [details] [review] pluginbase: Query for OpenGL app_context Ack, so sorry. This old branch that I'm stuck developing on is killing me. :) I can believe that there are still issues on some platforms, but I'm having trouble understanding what more you're suggesting we ought to do in this specific patch. Can you be more specific about the ways that it's failing in some of those other environments? I wonder whether the fixes for those issues might need to take the form of a patch separate from this one.
Review of attachment 344161 [details] [review]: gstreamer-vaapi supports many drawing "backends": X11 (GLX and EGL), Wayland and DRM As far as I understand, you had have in mind the first one. We need to assure the next ones. I haven't checked Wayland, but I have DRM. DRM is used in two use cases: 1) headless, that means that you want decoding but without a X11 display, for example streaming or storing in a file. 2) for kmssink, for embedded, where the frames are painted in the screen using the drm/kms system calls in the kernel. [gst-master] [vjaquez@ip147 gstreamer-vaapi]$ GST_DEBUG=gl*:5 gst-play-1.0 ~/patterns/300\ -\ Rise\ of\ an\ Empire\ -\ Trailer\ 2.mp4 --videosink="kmssink" Press 'k' to see a list of keyboard shortcuts. Now playing /home/vjaquez/patterns/300 - Rise of an Empire - Trailer 2.mp4 0:00:01.402086168 6209 0x7ff5bc16eb20 INFO gldisplay gstgldisplay.c:282:gst_gl_display_new: creating a display, user choice:(NULL) (platform: (NULL)) 0:00:01.403068790 6209 0x7ff5bc16eb20 ERROR gldisplay gstgldisplay_x11.c:95:gst_gl_display_x11_new: Failed to open X11 display connection with name, '(null)' 0:00:01.403791116 6209 0x7ff5bc16eb20 ERROR gldisplay gstgldisplay_wayland.c:124:gst_gl_display_wayland_new: Failed to open Wayland display connection. 0:00:01.404259972 6209 0x7ff5bc16eb20 DEBUG gldisplay gstgldisplay_egl.c:116:gst_gl_display_egl_get_from_native: egl no display extensions: EGL_EXT_client_extensions EGL_EXT_platform_base EGL_EXT_p latform_wayland EGL_EXT_platform_x11 EGL_MESA_platform_gbm EGL_MESA_platform_surfaceless EGL_KHR_client_get_all_proc_addresses EGL_KHR_debug 0:00:01.404673110 6209 0x7ff5bc16eb20 DEBUG gldisplay gstgldisplay.c:650:_get_gl_context_for_thread_unlocked:<gldisplayegl0> No GL context for thread (nil) 0:00:01.404725220 6209 0x7ff5bc16eb20 DEBUG gldisplay gstgldisplay.c:675:gst_gl_display_get_gl_context_for_thread:<gldisplayegl0> returning context (NULL) for thread (nil) 0:00:01.405019866 6209 0x7ff5bc16eb20 INFO glcontext gstglcontext.c:333:gst_gl_context_new: creating a context for display <gldisplayegl0>, user choice:(null) 0:00:01.405199071 6209 0x7ff5bc16eb20 DEBUG glcontext gstglcontext.c:367:gst_gl_context_new:<glcontextegl0> Done creating context for display <gldisplayegl0> (user_choice:(null)) 0:00:01.405237426 6209 0x7ff5bc16eb20 DEBUG gldisplay gstgldisplay.c:508:gst_gl_display_create_context:<gldisplayegl0> creating context <glcontextegl0> from other context (NULL) 0:00:01.405279682 6209 0x7ff5bc16eb20 DEBUG glcontext gstglcontext.c:990:gst_gl_context_create:<glcontextegl0> other_context:(NULL) 0:00:01.405348329 6209 0x7ff5bc16eb20 INFO glwindow gstglwindow.c:244:gst_gl_window_new: creating a window, user choice:(null) 0:00:01.405390974 6209 0x7ff5bc16eb20 INFO glwindow gstglwindow_x11.c:129:gst_gl_window_x11_new: Wrong display type 32 for this window type 1 0:00:01.405441524 6209 0x7ff5bc16eb20 WARN glwindow gstglwindow.c:277:gst_gl_window_new: Could not create window. user specified (null), creating dummy window 0:00:01.405553687 6209 0x7ff5bc16eb20 DEBUG glcontext gstglcontext.c:899:gst_gl_context_set_window:<glcontextegl0> window:<gldummywindow0> 0:00:01.405813413 6209 0x1bdbf70 DEBUG glcontext gstglcontext.c:1148:gst_gl_context_create_thread:<glcontextegl0> Creating thread 0:00:01.405918450 6209 0x1bdbf70 INFO glcontext gstglcontext.c:1205:gst_gl_context_create_thread:<glcontextegl0> Attempting to create opengl context. user chosen api(s) (any), compiled api su pport (opengl opengl3) display api (any) 0:00:01.405970103 6209 0x1bdbf70 DEBUG glcontext gstglcontext_egl.c:288:gst_gl_context_egl_create_context:<glcontextegl0> Creating EGL context libEGL warning: DRI3: xcb_connect failed libEGL warning: DRI2: xcb_connect failed libEGL warning: DRI2: xcb_connect failed It doesn't fail, at this moment, but it would fail, besides the wasted bits and cpu cycles. A workaround in this case would be, if the GstGLDisplay created is the dummy one, skip the GstGLContext creation.
I guess there must be something that I'm missing about the way the OpenGL support in GStreamer works. I would have expected that in a scenario like that, there wouldn't be a GstGLDisplay created at all, and then my patch would see that and just bypass creating an OpenGL context completely. I don't see any code in kmssink which does anything with OpenGL at all...where does the GstGLDisplay get created in this pipeline?
Oh, wait, never mind, I see it now. I didn't realize gst_gl_ensure_element_data() actually created a display if it couldn't find one. That does complicate things, because I agree that randomly creating a display is likely to lead to problems. Is there some easy way to tell if the display we got back was a default one? Another alternative would be to avoid using gst_gl_ensure_element_data() and instead perform the same function ourselves (perhaps using the query code that exists in gstvaapivideocontext.c). Does that sound like a better idea?
Created attachment 344306 [details] [review] vaapivideocontext: context type can be rejected Instead of calling g_return_val_if_fail() to check the context type, we should use a normal conditional, since it is possible that other context types can arrive and try to be assigned. Otherwise a critical log message is printed. This happens when we use playbin3 with vaapipostproc as video-filter.
Created attachment 344307 [details] [review] plugins: create a GL context on certain conditions If a GstVaapiDisplay is not found in the GStreamer context sharing, then VAAPI elements look for a local GstGLContext in gst context sharing mechanism ('gst.gl.local.context'). If this GstGLContext not found either then, only the VAAPI decoders and the VAAPI post-processor, will try to instantiate a new GstGLContext. If a valid GstGLContext is received, then a new GstVaapiDisplay will be instantiated with the platform, API and windowing specified by the instantiated GstGLContext. Original-Patch-By: Matt Fischer <matt.fischer@garmin.com>
Matt, Do these patches work for you? What do you think about this approach?
It works for my use cases. However there are a couple of things that seem strange about this version: * gst_vaapi_plugin_base_find_gl_context() now lives in gstvaapipluginutil.c. Should it be renamed? * Also gst_vaapi_plugin_base_find_gl_context() calls gst_vaapi_plugin_base_create_gl_context(), it seems odd for a utility function to assume that it's a PLUGIN_BASE. Maybe it would be better to just move the contents of gst_vaapi_ensure_display() into gst_vaapi_plugin_base_ensure_display()? That's the only place that calls it anyway. Also, the IS_VIDEO_SINK || IS_VIDEO_ENCODER check causes a change in the previous behavior. Before, it would always attempt to at least query for a local context, now it will not perform that query if it's a sink or encoder. If we really need to avoid doing the context construction for sinks and encoders, would it be better to put that check around the gst_vaapi_plugin_base_create_gl_context() call instead, so that the previous behavior is retained? I'm not sure I quite understand why we need to exclude sinks and encoders in the first place, though. Now that you have the check to reject default-created GL display, shouldn't it be safe to use this code for all VAAPI plugins?
(In reply to Matt Fischer from comment #26) > It works for my use cases. However there are a couple of things that seem > strange about this version: > > * gst_vaapi_plugin_base_find_gl_context() now lives in gstvaapipluginutil.c. > Should it be renamed? > * Also gst_vaapi_plugin_base_find_gl_context() calls > gst_vaapi_plugin_base_create_gl_context(), it seems odd for a utility > function to assume that it's a PLUGIN_BASE. Nice catch! > Maybe it would be better to just move the contents of > gst_vaapi_ensure_display() into gst_vaapi_plugin_base_ensure_display()? > That's the only place that calls it anyway. That makes sense. > Also, the IS_VIDEO_SINK || IS_VIDEO_ENCODER check causes a change in the > previous behavior. Before, it would always attempt to at least query for a > local context, now it will not perform that query if it's a sink or encoder. > If we really need to avoid doing the context construction for sinks and > encoders, would it be better to put that check around the > gst_vaapi_plugin_base_create_gl_context() call instead, so that the previous > behavior is retained? Good insight. We would need to split those patches in order to merge it to branch 1.10 without modifying the behavior in the stable branch. > I'm not sure I quite understand why we need to exclude sinks and encoders in > the first place, though. Now that you have the check to reject > default-created GL display, shouldn't it be safe to use this code for all > VAAPI plugins? If vaapisink is used, the va display created for it shall be used in the pipeline. In the case of the encoders now I have doubts.
Created attachment 344410 [details] [review] vaapivideocontext: context type can be rejected Instead of calling g_return_val_if_fail() to check the context type, we should use a normal conditional, since it is possible that other context types can arrive and try to be assigned. Otherwise a critical log message is printed. This happens when we use playbin3 with vaapipostproc as video-filter.
Created attachment 344411 [details] [review] plugins: create a GL context on certain conditions If a GstVaapiDisplay is not found in the GStreamer context sharing, then VAAPI elements look for a local GstGLContext in gst context sharing mechanism ('gst.gl.local.context'). If this GstGLContext not found either then, only the VAAPI decoders and the VAAPI post-processor, will try to instantiate a new GstGLContext. If a valid GstGLContext is received, then a new GstVaapiDisplay will be instantiated with the platform, API and windowing specified by the instantiated GstGLContext. Original-Patch-By: Matt Fischer <matt.fischer@garmin.com>
Created attachment 344412 [details] [review] plugins: handle GL params through context query If the element instantiated the GL display and context, they should handle them too through the context query.
(In reply to Víctor Manuel Jáquez Leal from comment #27) > (In reply to Matt Fischer from comment #26) > > It works for my use cases. However there are a couple of things that seem > > strange about this version: > > > > * gst_vaapi_plugin_base_find_gl_context() now lives in gstvaapipluginutil.c. > > Should it be renamed? > > * Also gst_vaapi_plugin_base_find_gl_context() calls > > gst_vaapi_plugin_base_create_gl_context(), it seems odd for a utility > > function to assume that it's a PLUGIN_BASE. > > Nice catch! Done. > > > Maybe it would be better to just move the contents of > > gst_vaapi_ensure_display() into gst_vaapi_plugin_base_ensure_display()? > > That's the only place that calls it anyway. > > That makes sense. Giving a second thought, I prefer to keeping it, just for the semantic distinction (base plugin vs any derived future plugin). > > Also, the IS_VIDEO_SINK || IS_VIDEO_ENCODER check causes a change in the > > previous behavior. Before, it would always attempt to at least query for a > > local context, now it will not perform that query if it's a sink or encoder. > > If we really need to avoid doing the context construction for sinks and > > encoders, would it be better to put that check around the > > gst_vaapi_plugin_base_create_gl_context() call instead, so that the previous > > behavior is retained? > > Good insight. We would need to split those patches in order to merge it to > branch 1.10 without modifying the behavior in the stable branch. I'm having doubts about merging this in 1.10, since it is a big change in functionality. > > > I'm not sure I quite understand why we need to exclude sinks and encoders in > > the first place, though. Now that you have the check to reject > > default-created GL display, shouldn't it be safe to use this code for all > > VAAPI plugins? > > If vaapisink is used, the va display created for it shall be used in the > pipeline. In the case of the encoders now I have doubts. I have added a comment on that to explain it.
Review of attachment 344411 [details] [review]: ::: gst/vaapi/gstvaapipluginutil.c @@ +252,3 @@ + * GstVaapiDisplay. Let's them to choose their own + * GstVaapiDisplay */ + GstVaapiPluginBase *const plugin = GST_VAAPI_PLUGIN_BASE (element); Maybe it would be a bit cleaner to handle this by making a vfunc that the postproc and decoder classes implement, rather than downcasting the element in here?
Review of attachment 344411 [details] [review]: I'm going to push to master this patches now. ::: gst/vaapi/gstvaapipluginutil.c @@ +252,3 @@ + * GstVaapiDisplay. Let's them to choose their own + * GstVaapiDisplay */ + if (GST_IS_VIDEO_SINK (element) || GST_IS_VIDEO_ENCODER (element)) It is not a downcast, it is like is_a operator to check the class of the instance. I think this is simpler than adding another virtual method.
Attachment 344410 [details] pushed as 43d4f0b - vaapivideocontext: context type can be rejected Attachment 344411 [details] pushed as 0968ce4 - plugins: create a GL context on certain conditions Attachment 344412 [details] pushed as c9bd45f - plugins: handle GL params through context query
Great! Thank you so much for your help with this.