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 704808 - Split GstGLDisplay into that and GstGLContext
Split GstGLDisplay into that and GstGLContext
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-gl
git master
Other Linux
: Normal normal
: 1.1.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 704321 704809
 
 
Reported: 2013-07-24 12:47 UTC by Sebastian Dröge (slomo)
Modified: 2013-08-22 08:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
new GstGLContext object (59.77 KB, patch)
2013-08-21 04:17 UTC, Matthew Waters (ystreet00)
committed Details | Review
port subclasses to new GstGLContext object (150.94 KB, patch)
2013-08-21 04:18 UTC, Matthew Waters (ystreet00)
committed Details | Review
android: tentative port to GstGLContext (7.18 KB, patch)
2013-08-21 04:18 UTC, Matthew Waters (ystreet00)
committed Details | Review
move egl into its own directory (33.01 KB, patch)
2013-08-21 04:20 UTC, Matthew Waters (ystreet00)
committed Details | Review

Description Sebastian Dröge (slomo) 2013-07-24 12:47:03 UTC
The former should just be the display connection, the latter should be the GL thread and everything else. GstGLDisplay would be shared via GstContext in the pipeline, GstGLContext would be shared between elements via the ALLOCATION query (as part of the GstGLBufferPool).
Comment 1 Matthew Waters (ystreet00) 2013-08-15 06:17:29 UTC
I have started on this.

I have made the following split between GstGLDisplay/GstGLContext/GstGLWindow

GstGLDisplay - information relating to all elements/application (gl_api, gl vtable? (have to check whether vfuncs are per context or not), possibly a application defined GstGLContext to share with)
GstGLContext - represents a GL context.  As such is platform specific (GLX, EGL, WGL, etc).  Holds a reference to a single GstGLWindow.
GstGLWindow - represents a window/surface that we are rendering to/with.  Eventually will be split into 2 categories, offscreen and onscreen.

GstGLContext + GstGLWindow together define a platform. e.g EGL+x11, WGL+win32, Cocoa, etc.  Some combinations are not possible e.g WGL+x11.

The next step after this is done would be sorting out the propogation of GstGLContext's through the pipeline.
Comment 2 Sebastian Dröge (slomo) 2013-08-15 08:34:34 UTC
(In reply to comment #1)
> have to check whether vfuncs are per context or not

They are not, but they depend on the GL version used. GLES vs. desktop GL at least. However we can't reliable load different ones in the same process anyway.

> GstGLContext - represents a GL context.  As such is platform specific (GLX,
> EGL, WGL, etc).  Holds a reference to a single GstGLWindow.
> GstGLWindow - represents a window/surface that we are rendering to/with. 
> Eventually will be split into 2 categories, offscreen and onscreen.

Can't we have a GL context without a window? E.g. the filters don't really require a window (offscreen or onscreen)


Otherwise sounds like a good plan :)
Comment 3 Matthew Waters (ystreet00) 2013-08-15 12:23:23 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > have to check whether vfuncs are per context or not
> 
> They are not, but they depend on the GL version used. GLES vs. desktop GL at
> least. However we can't reliable load different ones in the same process
> anyway.

Technically calling wglGetProcAddress with different contexts can return different values however I don't think that it occurs in practice.  Cogl does them per context.  Actually with mesa, you get the same function regardless of GL vs GLES however I don't think that mesa allows both GL and GLES to be used at the same time (not tested).

> > GstGLContext - represents a GL context.  As such is platform specific (GLX,
> > EGL, WGL, etc).  Holds a reference to a single GstGLWindow.
> > GstGLWindow - represents a window/surface that we are rendering to/with. 
> > Eventually will be split into 2 categories, offscreen and onscreen.
> 
> Can't we have a GL context without a window? E.g. the filters don't really
> require a window (offscreen or onscreen)

Some platforms allow it (depending on GL Version or extension), some don't.  On Win32 the only way you can get a HDC is from a window.  GLX only allows it with GL 3.0+.  EGL has EGL_KHR_surfaceless_context.  Dunno about Cocoa.  Besides, I put the display connection stuff into GstGLWindow :) it seems useless to create a seperate object for one variable :).

> Otherwise sounds like a good plan :)
Comment 4 Sebastian Dröge (slomo) 2013-08-16 08:10:10 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > have to check whether vfuncs are per context or not
> > 
> > They are not, but they depend on the GL version used. GLES vs. desktop GL at
> > least. However we can't reliable load different ones in the same process
> > anyway.
> 
> Technically calling wglGetProcAddress with different contexts can return
> different values however I don't think that it occurs in practice.  Cogl does
> them per context.  Actually with mesa, you get the same function regardless of
> GL vs GLES however I don't think that mesa allows both GL and GLES to be used
> at the same time (not tested).

Then let's better play safe, it doesn't make much of a difference anyway :)

> > > GstGLContext - represents a GL context.  As such is platform specific (GLX,
> > > EGL, WGL, etc).  Holds a reference to a single GstGLWindow.
> > > GstGLWindow - represents a window/surface that we are rendering to/with. 
> > > Eventually will be split into 2 categories, offscreen and onscreen.
> > 
> > Can't we have a GL context without a window? E.g. the filters don't really
> > require a window (offscreen or onscreen)
> 
> Some platforms allow it (depending on GL Version or extension), some don't.  On
> Win32 the only way you can get a HDC is from a window.  GLX only allows it with
> GL 3.0+.  EGL has EGL_KHR_surfaceless_context.  Dunno about Cocoa.  Besides, I
> put the display connection stuff into GstGLWindow :) it seems useless to create
> a seperate object for one variable :).

Ok :) Making a offscreen surface is not that bad anyway, could probably be just a hint when creating a context that we don't need a surface... but if we get one it's not a problem either.
Comment 5 Matthew Waters (ystreet00) 2013-08-21 04:16:03 UTC
(In reply to comment #4)
> (In reply to comment #3)
> Ok :) Making a offscreen surface is not that bad anyway, could probably be just
> a hint when creating a context that we don't need a surface... but if we get
> one it's not a problem either.

Sure
Comment 6 Matthew Waters (ystreet00) 2013-08-21 04:17:46 UTC
Created attachment 252500 [details] [review]
new GstGLContext object
Comment 7 Matthew Waters (ystreet00) 2013-08-21 04:18:20 UTC
Created attachment 252501 [details] [review]
port subclasses to new GstGLContext object
Comment 8 Matthew Waters (ystreet00) 2013-08-21 04:18:55 UTC
Created attachment 252502 [details] [review]
android: tentative port to GstGLContext
Comment 9 Matthew Waters (ystreet00) 2013-08-21 04:20:53 UTC
Created attachment 252503 [details] [review]
move egl into its own directory

alternatively available at https://github.com/ystreet/gst-plugins-gl/tree/context
Comment 10 Sebastian Dröge (slomo) 2013-08-21 06:44:48 UTC
Review of attachment 252500 [details] [review]:

::: gst-libs/gst/gl/gstglcontext.h
@@ +1,3 @@
+/*
+ * GStreamer
+ * Copyright (C) 2012 Matthew Waters <ystreet00@gmail.com>

We have 2013 ;)

@@ +54,3 @@
+
+  /*< public >*/
+//  GMutex        lock;

Why the commented out mutex?

@@ +90,3 @@
+GstGLAPI      gst_gl_context_get_gl_api       (GstGLContext *context);
+
+gboolean      gst_gl_context_create           (GstGLContext *context, guintptr external_gl_context, GError ** error);

This is what creates the window? Better name it create_window() then. Or is it initialization the context? Then better call it initialize() or something like that

::: gst-libs/gst/gl/gstgldisplay.h
@@ +61,3 @@
   GstGLAPI              gl_api;
 
   GstGLFuncs           *gl_vtable;

Didn't we say that the vtable, etc should be per context? And I assume there could be multiple contexts per display connection, so we should allow that. E.g. you could want to share the display with the complete pipeline, but have multiple contexts running in differents threads
Comment 11 Sebastian Dröge (slomo) 2013-08-21 06:46:53 UTC
Otherwise looks generally like a good step in the right direction :) Thanks for working on this
Comment 12 Matthew Waters (ystreet00) 2013-08-21 07:49:03 UTC
(In reply to comment #10)
> Review of attachment 252500 [details] [review]:
> 
> ::: gst-libs/gst/gl/gstglcontext.h
> @@ +1,3 @@
> +/*
> + * GStreamer
> + * Copyright (C) 2012 Matthew Waters <ystreet00@gmail.com>
> 
> We have 2013 ;)

Doh.

> @@ +54,3 @@
> +
> +  /*< public >*/
> +//  GMutex        lock;
> 
> Why the commented out mutex?

Taken from GstGLWindow, thought I might need it.  It was removed in the next patch.

> @@ +90,3 @@
> +GstGLAPI      gst_gl_context_get_gl_api       (GstGLContext *context);
> +
> +gboolean      gst_gl_context_create           (GstGLContext *context, guintptr
> external_gl_context, GError ** error);
> 
> This is what creates the window? Better name it create_window() then. Or is it
> initialization the context? Then better call it initialize() or something like
> that

In the end, creates the context.  In this patch, it just calls the window vfunc.  The next patch moves code so that GstGLContext takes control of that (and other) aspects of the GL context.  Creating the actual window is done within GstGLWindow (as a result of calling _create). TBH I don't think it really matters whether it's called create or initialize.

> ::: gst-libs/gst/gl/gstgldisplay.h
> @@ +61,3 @@
>    GstGLAPI              gl_api;
> 
>    GstGLFuncs           *gl_vtable;
> 
> Didn't we say that the vtable, etc should be per context? And I assume there
> could be multiple contexts per display connection, so we should allow that.
> E.g. you could want to share the display with the complete pipeline, but have
> multiple contexts running in differents threads

Yes and yes.  It's up next ;) (probably as part of #704809).
Comment 13 Sebastian Dröge (slomo) 2013-08-21 14:12:01 UTC
Alright, let's get this shipped then :) Would you like me to merge that now from your branch or wait until the next, quite related part is done?
Comment 14 Matthew Waters (ystreet00) 2013-08-22 06:10:48 UTC
(In reply to comment #13)
> Alright, let's get this shipped then :) Would you like me to merge that now
> from your branch or wait until the next, quite related part is done?

It should be good to go now.  I fixed up the compilation of tests and refcounting between window and context so we can swap out window's from contexts (before creation).  That should help with offscreen vs onscreen windows later on down the track.  It's all very related to the concept of 'give me a GL context';).

In the end, the only thing that has changed from an element's POV is that it now has to create a GstGLContext and set that on GstGLDisplay.  All of the previous restrictions are still there.

P.S. There's a race in context creation but that was there before this patch anyway.
Comment 15 Sebastian Dröge (slomo) 2013-08-22 08:51:04 UTC
All merged and pushed, thanks! :)