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 750310 - GL: allow an application to provide an external backend
GL: allow an application to provide an external backend
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal blocker
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-06-02 23:18 UTC by Julien Isorce
Modified: 2015-08-16 13:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Revert "gl: add GstGLContextGPUProcess backend" (17.63 KB, patch)
2015-06-15 15:48 UTC, Julien Isorce
none Details | Review
Revert "glcontext: add gst_gl_context_set_display helper" (2.00 KB, patch)
2015-06-15 15:48 UTC, Julien Isorce
committed Details | Review
glcontext: move display from priv (3.05 KB, patch)
2015-06-15 15:49 UTC, Julien Isorce
committed Details | Review
gldisplay: add create-context signal (4.21 KB, patch)
2015-06-15 15:57 UTC, Julien Isorce
none Details | Review
Revert "gl: add GstGLContextGPUProcess backend" (18.23 KB, patch)
2015-06-18 12:01 UTC, Julien Isorce
committed Details | Review
gldisplay: add create-context signal (7.71 KB, patch)
2015-06-18 12:04 UTC, Julien Isorce
none Details | Review
gldisplay: add gst_gl_display_create_context (7.68 KB, patch)
2015-06-18 13:01 UTC, Julien Isorce
none Details | Review
gldisplay: add gst_gl_display_create_context (7.64 KB, patch)
2015-06-18 14:52 UTC, Julien Isorce
none Details | Review
gldisplay: add gst_gl_display_create_context (7.64 KB, patch)
2015-06-19 08:36 UTC, Julien Isorce
committed Details | Review
gl: use gst_gl_display_create_context in more elements. (3.66 KB, patch)
2015-06-19 11:04 UTC, Julien Isorce
committed Details | Review
glstereosplit: use gst_gl_display_create_context (1.21 KB, patch)
2015-07-21 10:31 UTC, Julien Isorce
committed Details | Review
gstglwidget: use gst_gl_display_create_context (1.00 KB, patch)
2015-07-21 10:32 UTC, Julien Isorce
none Details | Review
caopengllayersink: use gst_gl_display_create_context (1.09 KB, patch)
2015-07-21 10:33 UTC, Julien Isorce
committed Details | Review
gstglwidget: use gst_gl_display_create_context (1.31 KB, patch)
2015-07-21 14:15 UTC, Julien Isorce
committed Details | Review

Description Julien Isorce 2015-06-02 23:18:29 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.
Comment 1 Matthew Waters (ystreet00) 2015-06-03 01:58:57 UTC
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.
Comment 2 Tim-Philipp Müller 2015-06-13 20:12:12 UTC
So if I understand correctly this bug is about whether the GPU backend should stay or be removed again?
Comment 3 Julien Isorce 2015-06-14 10:27:21 UTC
I am going to remove it but I need to implement 1) and 2). I will have a look this week.
Comment 4 Julien Isorce 2015-06-15 15:48:19 UTC
Created attachment 305314 [details] [review]
Revert "gl: add GstGLContextGPUProcess backend"
Comment 5 Julien Isorce 2015-06-15 15:48:45 UTC
Created attachment 305315 [details] [review]
Revert "glcontext: add gst_gl_context_set_display helper"
Comment 6 Julien Isorce 2015-06-15 15:49:04 UTC
Created attachment 305316 [details] [review]
glcontext: move display from priv
Comment 7 Julien Isorce 2015-06-15 15:57:45 UTC
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.
Comment 8 Matthew Waters (ystreet00) 2015-06-16 09:12:51 UTC
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.
Comment 9 Matthew Waters (ystreet00) 2015-06-16 13:13:53 UTC
Review of attachment 305314 [details] [review]:

.
Comment 10 Matthew Waters (ystreet00) 2015-06-16 13:14:10 UTC
Review of attachment 305315 [details] [review]:

.
Comment 11 Matthew Waters (ystreet00) 2015-06-16 13:14:24 UTC
Review of attachment 305316 [details] [review]:

.
Comment 12 Julien Isorce 2015-06-16 15:11:10 UTC
(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.
Comment 13 Julien Isorce 2015-06-18 12:01:10 UTC
Created attachment 305550 [details] [review]
Revert "gl: add GstGLContextGPUProcess backend"

Previous revert patch was not reverting everything.
Comment 14 Julien Isorce 2015-06-18 12:04:17 UTC
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.
Comment 15 Julien Isorce 2015-06-18 13:01:11 UTC
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.
Comment 16 Matthew Waters (ystreet00) 2015-06-18 13:32:35 UTC
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.
Comment 17 Julien Isorce 2015-06-18 14:52:40 UTC
Created attachment 305599 [details] [review]
gldisplay: add gst_gl_display_create_context

Addressed remarks.
Comment 18 Julien Isorce 2015-06-19 08:36:25 UTC
Created attachment 305646 [details] [review]
gldisplay: add gst_gl_display_create_context

Re-based to current master and executed some tests.
Comment 19 Matthew Waters (ystreet00) 2015-06-19 08:59:16 UTC
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.
Comment 20 Julien Isorce 2015-06-19 11:04:22 UTC
Created attachment 305683 [details] [review]
gl: use gst_gl_display_create_context in more elements.

It remains gstglstereosplit and gtkgstglwidget.
Comment 21 Julien Isorce 2015-06-19 13:17:54 UTC
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.
Comment 22 Sebastian Dröge (slomo) 2015-06-22 10:40:07 UTC
What's left here? I wanted to get 1.5.2 out this week, and this looks like something that should be done before.
Comment 23 Julien Isorce 2015-06-22 12:11:56 UTC
I need to push last attachment and also make a patch to use gst_gl_display_create_context in gstglstereosplit and gstglwidget .
Comment 24 Julien Isorce 2015-06-22 14:10:03 UTC
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)
Comment 25 Matthew Waters (ystreet00) 2015-07-07 08:58:58 UTC
Review of attachment 305683 [details] [review]:

Looks good.  There's also caopengllayersink, glstereosplit and gtkglsink that need this as well.
Comment 26 Julien Isorce 2015-07-20 16:50:41 UTC
I'll try to continue this tomorrow.
Comment 27 Julien Isorce 2015-07-21 10:31:37 UTC
Created attachment 307816 [details] [review]
glstereosplit: use gst_gl_display_create_context
Comment 28 Julien Isorce 2015-07-21 10:32:00 UTC
Created attachment 307817 [details] [review]
gstglwidget: use gst_gl_display_create_context
Comment 29 Julien Isorce 2015-07-21 10:33:23 UTC
Created attachment 307818 [details] [review]
caopengllayersink: use gst_gl_display_create_context
Comment 30 Julien Isorce 2015-07-21 10:37:28 UTC
I have not tested gtkglwidget since I have gtk3 < 3.15. Also I cannot test caopengllayersink here.
Comment 31 Julien Isorce 2015-07-21 11:01:40 UTC
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
Comment 32 Matthew Waters (ystreet00) 2015-07-21 12:37:24 UTC
Review of attachment 307816 [details] [review]:

Looks good.  Good catch on the lock :)
Comment 33 Matthew Waters (ystreet00) 2015-07-21 12:38:52 UTC
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.
Comment 34 Matthew Waters (ystreet00) 2015-07-21 12:40:44 UTC
Review of attachment 307818 [details] [review]:

Looks good.
Comment 35 Julien Isorce 2015-07-21 14:15:24 UTC
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!
Comment 36 Matthew Waters (ystreet00) 2015-07-21 14:20:04 UTC
Review of attachment 307830 [details] [review]:

Compiles and works fine.
Comment 37 Julien Isorce 2015-07-21 14:23:29 UTC
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 38 Julien Isorce 2015-07-21 14:23:55 UTC
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 39 Julien Isorce 2015-07-21 14:24:19 UTC
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