Created attachment 253481 [details] [review]
reimplement context sharing

Created attachment 253482 [details] [review]
test context sharing

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

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?

(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.

Created attachment 253511 [details] [review]
rename external-gl-context to other-context

Created attachment 253512 [details] [review]
egl doesn't require the EGLConfig from other_context

(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?

(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

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?

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

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

(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 :).

(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

(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.

Created attachment 253963 [details] [review]
rename external-gl-context to other-context

Created attachment 253964 [details] [review]
egl doesn't require the EGLConfig from other_context

Created attachment 253965 [details] [review]
move window error enums into context

some code cleanups

Created attachment 253966 [details] [review]
x11" remove dead code

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.

Created attachment 255009 [details] [review]
tests: update for GstGLContext

Created attachment 255012 [details] [review]
move gl vtable from GstGLDisplay to GstGLContext

Created attachment 255013 [details] [review]
unref the buffer pool on shutdown

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?

(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 :).

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

Can someone provide a summary of the status of this now? What's missing?

(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