GNOME Bugzilla – Bug 750310
GL: allow an application to provide an external backend
Last modified: 2015-08-16 13:41:12 UTC
Recently I introduced GstGLContextGPUProcess http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=b377112ee38912d316e77b4e2102041389dc0051 This backend is useful for any application that only provides its own proc_addr to forward gl calls to a dedicated GPU process. I agree with Matthew that it should probably stay in the application. Though we need to figure out how to handle the case in glimagesink (see link above). I could check if other_context has a window but it is hard to maintain. Matthew you suggested to handle this through some new signals in GstGLDisplay. As discussed already that would be great if you can put your detailed ideas here. What I basically need is 3 things: - 1: Tell that other_context the app provides (the GstGLGPUContext) through GST_MESSAGE_NEEDS_CONTEXT is not a context to share resources with but it is a replacement. - 2: The app is connected to glimagesink's "client-draw" signal. gst_gl_window_draw should take the glimagesink's _draw_cb in parameter (instead of installing a cb). (- 3: The gstgldisplay is unique in my case and do not rely on any platform since I do not go through all gst_gl_context creations. ) 1 and 2 would avoid the special case in glimagesink I mentioned above.
1) is solved by adding a "create-context" signal on GstGLDisplay that is called in lieu of the current gst_gl_context_new(); gst_gl_context_create(); There you set your custom window on your custom context. And viola, you have gstgl using your custom GstGLContext. Would also need a helper for elements to call the signal without worrying about GstGLDisplay locking. 2) You should really create your own element and stop abusing glimagesink for its client-draw signal. The original intent for client-draw was to allow the application to draw the input texture however in the output window provided by glimagesink. Passing the output to another process/library is better suited to an appsink/glappsink like element if needed. See gtkgst for some inspiration. 3) Already handled by GstContext.
So if I understand correctly this bug is about whether the GPU backend should stay or be removed again?
I am going to remove it but I need to implement 1) and 2). I will have a look this week.
Created attachment 305314 [details] [review] Revert "gl: add GstGLContextGPUProcess backend"
Created attachment 305315 [details] [review] Revert "glcontext: add gst_gl_context_set_display helper"
Created attachment 305316 [details] [review] glcontext: move display from priv
Created attachment 305317 [details] [review] gldisplay: add create-context signal Matthew I could not move context_new/create dance to gst_gl_display_create_context because of window id. Indeed it is not easy so I suggest to do it separately later. Also since I create the gstgldisplay from the app, I wonder if I could just pass the gpucontext at this time (with gst_gl_display_make_one_to_one (display, context) or something) and also add a helper/property to check whether the pair display/context is one-to-one. But I am fine with the signal.
Review of attachment 305317 [details] [review]: (In reply to Julien Isorce from comment #7) > Created attachment 305317 [details] [review] [review] > gldisplay: add create-context signal > > Matthew I could not move context_new/create dance to > gst_gl_display_create_context because of window id. Indeed it is not easy so > I suggest to do it separately later. > > Also since I create the gstgldisplay from the app, I wonder if I could just > pass the gpucontext at this time (with gst_gl_display_make_one_to_one > (display, context) or something) and also add a helper/property to check > whether the pair display/context is one-to-one. But I am fine with the > signal. The set_window_handle is already done after context creation anyway so yes, you can move the _new/_create() to a new helper and have glimagesink call it and set the window id later. You don't need any extra functions to pass the context from the app to the pipeline, as long as you add the context to the the display with gst_gl_display_add_context, the elements will find it already. However, that is slated to change for the one context per streaming thread bug to be dependent on calling threads. Which is why the new signal is needed to do this. ::: ext/gl/gstglimagesink.c @@ +612,3 @@ if (!gl_sink->context) { + if (gst_gl_display_create_context (gl_sink->display, &gl_sink->context)) { + GstGLWindow *window = gst_gl_context_get_window (gl_sink->context); This whole entire if block needs to disappear and the gst_gl_display_create_context needs to replace the later gst_gl_context_new/gst_gl_context_create.
Review of attachment 305314 [details] [review]: .
Review of attachment 305315 [details] [review]: .
Review of attachment 305316 [details] [review]: .
(In reply to Matthew Waters from comment #8) > ::: ext/gl/gstglimagesink.c > @@ +612,3 @@ > if (!gl_sink->context) { > + if (gst_gl_display_create_context (gl_sink->display, > &gl_sink->context)) { > + GstGLWindow *window = gst_gl_context_get_window (gl_sink->context); > > This whole entire if block needs to disappear Ok I'll have a look tomorrow but I think it will require some refactoring first.
Created attachment 305550 [details] [review] Revert "gl: add GstGLContextGPUProcess backend" Previous revert patch was not reverting everything.
Created attachment 305552 [details] [review] gldisplay: add create-context signal Matthew, I just realized I forgot to update the doc in this patch but let me know if this patch matches your thoughts.
Created attachment 305565 [details] [review] gldisplay: add gst_gl_display_create_context Updated doc and I also realized that other_context is an input param. So signature is *other_context before **p_context.
Review of attachment 305565 [details] [review]: Some comments. ::: gst-libs/gst/gl/gstgldisplay.c @@ +113,3 @@ + * @object: the #GstGLDisplay + * + * Allow to provide a @GstGLContext one-to-one with the display. It's not one-to-one (whatever that means) it's overriding the context creation mechanism. Should also mention this could be called from any thread. Also that it's emitted with display's object lock held. @@ +120,3 @@ + g_signal_new ("create-context", G_TYPE_FROM_CLASS (klass), + G_SIGNAL_RUN_LAST, 0, NULL, NULL, g_cclosure_marshal_generic, + GST_GL_TYPE_CONTEXT, 0); Should also pass the other context to the signal. @@ +371,3 @@ + * @error: resulting #GError + * + * Returns: Whether @pcontext contains the new created context. Perhaps "whether a new context could be created" Also mention that it requires the display's object lock to be held. @@ +384,3 @@ + g_return_val_if_fail (display != NULL, FALSE); + g_return_val_if_fail (p_context != NULL, FALSE); + g_return_val_if_fail (*p_context == NULL, FALSE); Not sure you want to assert that what the p_context points to is NULL @@ +388,3 @@ + g_return_val_if_fail (*error == NULL, FALSE); + + g_signal_emit (display, gst_gl_display_signals[CREATE_CONTEXT], 0, &context); Should also pass other_context to the signal. @@ +405,3 @@ + gst_object_ref (other_context); + else + other_context = gst_gl_display_get_gl_context_for_thread (display, NULL); This is not going to work for non-glimagesink elements. Better to just use the other_context directly and make glimagesink do this instead.
Created attachment 305599 [details] [review] gldisplay: add gst_gl_display_create_context Addressed remarks.
Created attachment 305646 [details] [review] gldisplay: add gst_gl_display_create_context Re-based to current master and executed some tests.
Review of attachment 305646 [details] [review]: Looks good. Now there's the need to replace the other invocations of gst_gl_context_new(); gst_gl_context_create(); with gst_gl_display_create_context(); to be consistent.
Created attachment 305683 [details] [review] gl: use gst_gl_display_create_context in more elements. It remains gstglstereosplit and gtkgstglwidget.
commit 66d833fcf27aa5eefae2065c76b24163a3b708b2 Author: Julien Isorce <j.isorce@samsung.com> Date: Mon Jun 15 16:09:54 2015 +0100 gldisplay: add gst_gl_display_create_context It also emits a create-context signal so that an application can provide an external GstGLContext backend. https://bugzilla.gnome.org/show_bug.cgi?id=750310 commit b0995fcca01e58ce6b585d0f00152763fc89036f Author: Julien Isorce <j.isorce@samsung.com> Date: Mon Jun 15 16:36:26 2015 +0100 glcontext: move display from priv https://bugzilla.gnome.org/show_bug.cgi?id=750310 commit 5c23b98e27d87c0a133f2aa4e045f78da2ed1e3e Author: Julien Isorce <j.isorce@samsung.com> Date: Thu Jun 18 10:55:28 2015 +0100 Revert "glcontext: add gst_gl_context_set_display helper" This reverts commit 71b8103cbd16fff9cf5a65cf517083cb794aa3b5. commit 5b003b68cae024a1daefe0650b791f95336aeaef Author: Julien Isorce <j.isorce@samsung.com> Date: Thu Jun 18 10:52:18 2015 +0100 Revert "gl: add GstGLContextGPUProcess backend" This reverts commit b377112ee38912d316e77b4e2102041389dc0051.
What's left here? I wanted to get 1.5.2 out this week, and this looks like something that should be done before.
I need to push last attachment and also make a patch to use gst_gl_display_create_context in gstglstereosplit and gstglwidget .
Moving target milestone from 1.5.2 to 1.5.3 since it is usable already, just need to spread api usage everywhere. (see comment #23)
Review of attachment 305683 [details] [review]: Looks good. There's also caopengllayersink, glstereosplit and gtkglsink that need this as well.
I'll try to continue this tomorrow.
Created attachment 307816 [details] [review] glstereosplit: use gst_gl_display_create_context
Created attachment 307817 [details] [review] gstglwidget: use gst_gl_display_create_context
Created attachment 307818 [details] [review] caopengllayersink: use gst_gl_display_create_context
I have not tested gtkglwidget since I have gtk3 < 3.15. Also I cannot test caopengllayersink here.
Comment on attachment 305683 [details] [review] gl: use gst_gl_display_create_context in more elements. commit 57f389d9ce2224a01511ce09f8c0957b25d4382a Author: Julien Isorce <j.isorce@samsung.com> Date: Fri Jun 19 11:57:06 2015 +0100 gl: use gst_gl_display_create_context in more elements. glbasefilter, glbasemixer and gltestsrc. https://bugzilla.gnome.org/show_bug.cgi?id=750310
Review of attachment 307816 [details] [review]: Looks good. Good catch on the lock :)
Review of attachment 307817 [details] [review]: Feel free to merge after fixing the comment. ::: ext/gtk/gtkgstglwidget.c @@ +553,3 @@ + gst_gl_display_create_context (priv->display, priv->other_context, + &priv->context, NULL); Also need to catch the failure and unlock/return false as in the failure case of the code it's replacing.
Review of attachment 307818 [details] [review]: Looks good.
Created attachment 307830 [details] [review] gstglwidget: use gst_gl_display_create_context Matthew would you mind to test it ? I do not have gtk3 > 3.15 so no GtkGLAreaClass. Thx!
Review of attachment 307830 [details] [review]: Compiles and works fine.
Comment on attachment 307816 [details] [review] glstereosplit: use gst_gl_display_create_context commit 6fc5b181386c206e3cf0663cfa1087486dff1144 Author: Julien Isorce <j.isorce@samsung.com> Date: Tue Jul 21 11:21:27 2015 +0100 glstereosplit: use gst_gl_display_create_context Also unlock the lock on error. https://bugzilla.gnome.org/show_bug.cgi?id=750310
Comment on attachment 307818 [details] [review] caopengllayersink: use gst_gl_display_create_context commit 5457e55f255518d679b59a170951e299ecd8c5f6 Author: Julien Isorce <j.isorce@samsung.com> Date: Tue Jul 21 11:28:08 2015 +0100 caopengllayersink: use gst_gl_display_create_context https://bugzilla.gnome.org/show_bug.cgi?id=750310
Comment on attachment 307830 [details] [review] gstglwidget: use gst_gl_display_create_context commit 6125ea7e9778f52cf36f5a797bf6429c7a61a4da Author: Julien Isorce <j.isorce@samsung.com> Date: Tue Jul 21 11:23:21 2015 +0100 gstglwidget: use gst_gl_display_create_context Also handle the failure case. https://bugzilla.gnome.org/show_bug.cgi?id=750310