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 704806 - Implement GL context sharing
Implement GL context sharing
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-gl
git master
Other Linux
: Normal major
: 1.2.0
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-07-24 12:43 UTC by Sebastian Dröge (slomo)
Modified: 2013-10-02 13:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
reimplement context sharing (18.05 KB, patch)
2013-08-29 10:14 UTC, Matthew Waters (ystreet00)
committed Details | Review
test context sharing (6.57 KB, patch)
2013-08-29 10:14 UTC, Matthew Waters (ystreet00)
committed Details | Review
rename external-gl-context to other-context (3.36 KB, patch)
2013-08-29 14:30 UTC, Matthew Waters (ystreet00)
needs-work Details | Review
egl doesn't require the EGLConfig from other_context (2.20 KB, patch)
2013-08-29 14:30 UTC, Matthew Waters (ystreet00)
needs-work Details | Review
rename external-gl-context to other-context (3.53 KB, patch)
2013-09-03 12:30 UTC, Matthew Waters (ystreet00)
committed Details | Review
egl doesn't require the EGLConfig from other_context (2.20 KB, patch)
2013-09-03 12:30 UTC, Matthew Waters (ystreet00)
committed Details | Review
move window error enums into context (6.93 KB, patch)
2013-09-03 12:31 UTC, Matthew Waters (ystreet00)
committed Details | Review
x11" remove dead code (1.61 KB, patch)
2013-09-03 12:32 UTC, Matthew Waters (ystreet00)
committed Details | Review
tests: update for GstGLContext (1.46 KB, patch)
2013-09-16 09:13 UTC, Matthew Waters (ystreet00)
committed Details | Review
move gl vtable from GstGLDisplay to GstGLContext (210.15 KB, patch)
2013-09-16 10:18 UTC, Matthew Waters (ystreet00)
committed Details | Review
unref the buffer pool on shutdown (2.90 KB, patch)
2013-09-16 10:18 UTC, Matthew Waters (ystreet00)
committed Details | Review

Description Sebastian Dröge (slomo) 2013-07-24 12:43:52 UTC
Needed for example if filter and sink use a different GstGLDisplay but want to share memory. Currently this just causes the memory (i.e. texture) of the filter GstGLDisplay to be unusable in the sink.
Comment 1 Matthew Waters (ystreet00) 2013-08-29 10:14:18 UTC
Created attachment 253481 [details] [review]
reimplement context sharing
Comment 2 Matthew Waters (ystreet00) 2013-08-29 10:14:45 UTC
Created attachment 253482 [details] [review]
test context sharing
Comment 3 Sebastian Dröge (slomo) 2013-08-29 12:31:59 UTC
Review of attachment 253481 [details] [review]:

::: gst-libs/gst/gl/egl/gstglcontext_egl.c
@@ +24,3 @@
 
+/* FIXME: Sharing contexts requires the EGLDisplay & EGLConfig to be the same
+ * may need to box it.

The config should always be the same if the display is the same. A display can only be initialized to one config iirc
Comment 4 Sebastian Dröge (slomo) 2013-08-29 12:35:56 UTC
Additional to that it would now make sense to change all these external-context GObject properties to "other-context" for consistency and make them of type GstGLContext instead of guintptr/guint64. Can you do that? :)


Also, does this really already fix the case where we have two contexts. One producing a texture, one wanting to render it?
Comment 5 Matthew Waters (ystreet00) 2013-08-29 13:16:29 UTC
(In reply to comment #3)
> Review of attachment 253481 [details] [review]:
> 
> ::: gst-libs/gst/gl/egl/gstglcontext_egl.c
> @@ +24,3 @@
> 
> +/* FIXME: Sharing contexts requires the EGLDisplay & EGLConfig to be the same
> + * may need to box it.
> 
> The config should always be the same if the display is the same. A display can
> only be initialized to one config iirc

So it does :).  I'll fix that.

(In reply to comment #4)
> Additional to that it would now make sense to change all these external-context
> GObject properties to "other-context" for consistency and make them of type
> GstGLContext instead of guintptr/guint64. Can you do that? :)

Can do.

> Also, does this really already fix the case where we have two contexts. One
> producing a texture, one wanting to render it?

With WGL, GLX and EGL, yes.  I can't test this on a real Cocoa environment and GNUstep apparently doesn't like shared contexts :(.  Also, that's exactly what the test case does and what I used to test this.  One thread draws to the fbo (with a texture as an attachment) as fast it can, the other renders the texture to the window).  Try it :).

The only thing that needs to change later is for the moving of the vtable from GstGLDisplay to GstGLContext.
Comment 6 Matthew Waters (ystreet00) 2013-08-29 14:30:23 UTC
Created attachment 253511 [details] [review]
rename external-gl-context to other-context
Comment 7 Matthew Waters (ystreet00) 2013-08-29 14:30:59 UTC
Created attachment 253512 [details] [review]
egl doesn't require the EGLConfig from other_context
Comment 8 Sebastian Dröge (slomo) 2013-08-29 14:32:11 UTC
(In reply to comment #5)

> > Also, does this really already fix the case where we have two contexts. One
> > producing a texture, one wanting to render it?
> 
> With WGL, GLX and EGL, yes.  I can't test this on a real Cocoa environment and
> GNUstep apparently doesn't like shared contexts :(.  Also, that's exactly what
> the test case does and what I used to test this.  One thread draws to the fbo
> (with a texture as an attachment) as fast it can, the other renders the texture
> to the window).  Try it :).

But this requires the second context to be initialized from the first one, right? If both contexts are created independently (but happen to share the same display?) this won't work?
Comment 9 Matthew Waters (ystreet00) 2013-08-29 14:34:14 UTC
(In reply to comment #8)
> (In reply to comment #5)
> 
> > > Also, does this really already fix the case where we have two contexts. One
> > > producing a texture, one wanting to render it?
> > 
> > With WGL, GLX and EGL, yes.  I can't test this on a real Cocoa environment and
> > GNUstep apparently doesn't like shared contexts :(.  Also, that's exactly what
> > the test case does and what I used to test this.  One thread draws to the fbo
> > (with a texture as an attachment) as fast it can, the other renders the texture
> > to the window).  Try it :).
> 
> But this requires the second context to be initialized from the first one,
> right? If both contexts are created independently (but happen to share the same
> display?) this won't work?

Correct
Comment 10 Sebastian Dröge (slomo) 2013-08-30 09:02:18 UTC
That's only half the solution here then, also it will only work for two contexts and not more :)

What options do you see to share contexts independently? Other than downloading and uploading again?
Comment 11 Sebastian Dröge (slomo) 2013-08-30 09:07:59 UTC
Review of attachment 253511 [details] [review]:

I think some of the elements in gst/gl use this pattern too

::: gst-libs/gst/gl/gstglfilter.c
@@ +151,2 @@
     {
+      filter->other_context = g_value_get_object (value);

g_value_dup_object() here, and then make sure to unref it again when it's not used anymore
Comment 12 Sebastian Dröge (slomo) 2013-08-30 09:09:35 UTC
Review of attachment 253512 [details] [review]:

::: gst-libs/gst/gl/egl/gstglcontext_egl.c
@@ -171,3 @@
-    config_attrib[i++] = EGL_CONFIG_ID;
-    config_attrib[i++] = config_id;
-    config_attrib[i++] = EGL_NONE;

I think we should still do that as maybe the display was initialized from the outside with a different config already. Just to be on the save side :)

Just change the FIXME comment, and do a display equality check here and fail if it's not equal
Comment 13 Matthew Waters (ystreet00) 2013-09-01 04:47:28 UTC
(In reply to comment #10)
> That's only half the solution here then, also it will only work for two
> contexts and not more :)

You can use either context then to share with another context that you create, there's nothing limiting you to 2 contexts (barring implementation-specific limitations).

> What options do you see to share contexts independently? Other than downloading
> and uploading again?

You can't share contexts independently, they must be initialized with another context so that they can share the same object space.

(In reply to comment #11)
> Review of attachment 253511 [details] [review]:
> 
> I think some of the elements in gst/gl use this pattern too
> 
> ::: gst-libs/gst/gl/gstglfilter.c
> @@ +151,2 @@
>      {
> +      filter->other_context = g_value_get_object (value);
> 
> g_value_dup_object() here, and then make sure to unref it again when it's not
> used anymore

The only elements that don't inherit from glfilter are glmosaic, gltestsrc and glimagesink and I couldn't find it there.

(In reply to comment #12)
> Review of attachment 253512 [details] [review]:
> 
> ::: gst-libs/gst/gl/egl/gstglcontext_egl.c
> @@ -171,3 @@
> -    config_attrib[i++] = EGL_CONFIG_ID;
> -    config_attrib[i++] = config_id;
> -    config_attrib[i++] = EGL_NONE;
> 
> I think we should still do that as maybe the display was initialized from the
> outside with a different config already. Just to be on the save side :)

How would we get the config from outside?  I thought that EGLConfig's were associated with EGLSurfaces.

> Just change the FIXME comment, and do a display equality check here and fail if
> it's not equal

However we explicitly set them equal at the start of _create() if we're sharing so that seems pointless :).
Comment 14 Sebastian Dröge (slomo) 2013-09-02 09:26:44 UTC
(In reply to comment #13)
> (In reply to comment #10)

> > What options do you see to share contexts independently? Other than downloading
> > and uploading again?
> 
> You can't share contexts independently, they must be initialized with another
> context so that they can share the same object space.

I see, for that we should implement a fallback copying path in GstGLMemory then :( Maybe for some implementations this could be made less worse by having a custom implementation around EGLImage or something else.

> (In reply to comment #11)
> > Review of attachment 253511 [details] [review] [details]:
> > 
> > I think some of the elements in gst/gl use this pattern too
> > 
> > ::: gst-libs/gst/gl/gstglfilter.c
> > @@ +151,2 @@
> >      {
> > +      filter->other_context = g_value_get_object (value);
> > 
> > g_value_dup_object() here, and then make sure to unref it again when it's not
> > used anymore
> 
> The only elements that don't inherit from glfilter are glmosaic, gltestsrc and
> glimagesink and I couldn't find it there.

Indeed, should've looked before. Sorry

> (In reply to comment #12)
> > Review of attachment 253512 [details] [review] [details]:
> > 
> > ::: gst-libs/gst/gl/egl/gstglcontext_egl.c
> > @@ -171,3 @@
> > -    config_attrib[i++] = EGL_CONFIG_ID;
> > -    config_attrib[i++] = config_id;
> > -    config_attrib[i++] = EGL_NONE;
> > 
> > I think we should still do that as maybe the display was initialized from the
> > outside with a different config already. Just to be on the save side :)
> 
> How would we get the config from outside?  I thought that EGLConfig's were
> associated with EGLSurfaces.

I guess in the future we could allow outside code to initialize the EGLDisplay themselves somehow... not that important I guess
Comment 15 Matthew Waters (ystreet00) 2013-09-02 12:18:07 UTC
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #10)
> 
> > > What options do you see to share contexts independently? Other than downloading
> > > and uploading again?
> > 
> > You can't share contexts independently, they must be initialized with another
> > context so that they can share the same object space.
> 
> I see, for that we should implement a fallback copying path in GstGLMemory then
> :( Maybe for some implementations this could be made less worse by having a
> custom implementation around EGLImage or something else.

There are essentially two cases we have to deal with:
1. gstglfilter ! queue ! another_gl_filter (not based on libgstgl)
2. gstglfilter ! queue ! non-GL element

Solutions :):
1. Unconditionally share with a global GstGLContext (all GL shareable objects would be shared even those not related at all)
2. Attempt to figure out some kind of context chain that can be used to create a context that can download/upload the data from another thread.  It would probably look like some kind of tree possibly even inside GstGLContext.
3. Attempt to download before crossing a potential thread boundary.  We currently already do this based on GstGLMemory existence in a buffer.
4. Allow external contexts to be wrapped up in GstGLContext.

Use case 1 requires some kind of communication of the GL contexts that each filter on either side of the queue can use (or EGLImage/equivalent).  Would require solution 4 and (2 or 1)
Use case 2 is already partially solved by solution 3.  The problem with that solution is that it is probably too coarse a distinction to be useful for integration with other GL elements that don't use libgstgl.  Alternatively one could require libgstgl for OpenGL with GStreamer and implement solution 2 :) or define the interaction of GL contexts between elements in a pipeline and use solution 2 + 4.
Comment 16 Matthew Waters (ystreet00) 2013-09-03 12:30:03 UTC
Created attachment 253963 [details] [review]
rename external-gl-context to other-context
Comment 17 Matthew Waters (ystreet00) 2013-09-03 12:30:38 UTC
Created attachment 253964 [details] [review]
egl doesn't require the EGLConfig from other_context
Comment 18 Matthew Waters (ystreet00) 2013-09-03 12:31:36 UTC
Created attachment 253965 [details] [review]
move window error enums into context

some code cleanups
Comment 19 Matthew Waters (ystreet00) 2013-09-03 12:32:04 UTC
Created attachment 253966 [details] [review]
x11" remove dead code
Comment 20 Sebastian Dröge (slomo) 2013-09-03 16:02:23 UTC
1. Maybe a globally shared context is not the worst idea at all, what doesn't seem right there is that it involves creating a surface/window though.

2+4 sounds ok too, 3 should be implemented as a fallback path anyway I guess.
Comment 21 Matthew Waters (ystreet00) 2013-09-16 09:13:13 UTC
Created attachment 255009 [details] [review]
tests: update for GstGLContext
Comment 22 Matthew Waters (ystreet00) 2013-09-16 10:18:14 UTC
Created attachment 255012 [details] [review]
move gl vtable from GstGLDisplay to GstGLContext
Comment 23 Matthew Waters (ystreet00) 2013-09-16 10:18:52 UTC
Created attachment 255013 [details] [review]
unref the buffer pool on shutdown
Comment 24 Sebastian Dröge (slomo) 2013-09-16 10:22:03 UTC
Review of attachment 255009 [details] [review]:

::: tests/check/libs/gstglmemory.c
@@ +39,3 @@
   display = gst_gl_display_new ();
+  context = gst_gl_context_new (display);
+  gst_gl_display_set_context (display, context);

Can't a display have multiple contexts?
Comment 25 Matthew Waters (ystreet00) 2013-09-16 10:35:54 UTC
(In reply to comment #24)
> Review of attachment 255009 [details] [review]:
> 
> ::: tests/check/libs/gstglmemory.c
> @@ +39,3 @@
>    display = gst_gl_display_new ();
> +  context = gst_gl_context_new (display);
> +  gst_gl_display_set_context (display, context);
> 
> Can't a display have multiple contexts?

Yes that's the plan however that was written when the vtable was still in GstGLDisplay.  The API still exists because we pass the GstGLContext through the GstGLDisplay peer query which is going to be changed in the very near future :).
Comment 26 Julien Isorce 2013-09-27 10:16:39 UTC
I also pushed the next 6 patchs from https://github.com/ystreet/gst-plugins-gl/commits/context :

commit 586382aca85b9b1a3c23d4ca172ef69e24ba787d
Author: Matthew Waters <ystreet00@gmail.com>
Date:   Fri Sep 27 01:15:25 2013 +1000

    make gen_texture/del_texture threadsafe
    
    Use stack allocated instead of static variables
    
    Conflicts:
        gst-libs/gst/gl/gstglutils.c

commit 064b11db319b3cbd2a0c63937d609963524e12c4
Author: Matthew Waters <ystreet00@gmail.com>
Date:   Wed Sep 25 12:26:57 2013 +1000

    window: add send_message_async vmethod
    
    - provide a default synchronous send_message
    - make context creation threadsafe again

commit 21747712a70b039e3aa88508754361714bd0132a
Author: Matthew Waters <ystreet00@gmail.com>
Date:   Tue Sep 24 16:37:11 2013 +1000

    make the state change test pass

commit 7c3a5cca4669c1a8b35c1e8fd804472323436c65
Author: Matthew Waters <ystreet00@gmail.com>
Date:   Tue Sep 24 14:07:58 2013 +1000

    x11: fix make check segfault

commit f18207b390d5e4804bb76f980bcfabaa6a3ad1e4
Author: Matthew Waters <ystreet00@gmail.com>
Date:   Fri Sep 20 11:55:36 2013 +1000

    fix some refcount errors

commit 8ffda5c6b214fe9e851e5f4b7570b6eeecdc4d7c
Author: Matthew Waters <ystreet00@gmail.com>
Date:   Fri Sep 20 11:54:42 2013 +1000

    mixer: timestamp buffers
Comment 27 Sebastian Dröge (slomo) 2013-09-27 13:06:35 UTC
Can someone provide a summary of the status of this now? What's missing?
Comment 28 Matthew Waters (ystreet00) 2013-09-30 10:40:12 UTC
(In reply to comment #27)
> Can someone provide a summary of the status of this now? What's missing?

I believe this has been completed.  The rest of the context stuff is integration with GstContext and properly separating GstGLContext's based on the streaming thread which we already have bugs for ([1] and [2]).  I vote to close :)

[1] https://bugzilla.gnome.org/show_bug.cgi?id=704809
[2] https://bugzilla.gnome.org/show_bug.cgi?id=704321