The Wayland display sounds like a good candidate for GstContext usage instead of a property.

Review of attachment 256826 [details] [review]:

Does all this still work without setting a window handle/display, and then creates a new window?

::: configure.ac
@@ +878,3 @@
+  else
+    PKG_CHECK_MODULES(GTK2, gtk+-2.0, HAVE_GTK_20=yes, HAVE_GTK_20=no)
+    if test "x$HAVE_GTK_20" = "xyes"; then

Just remove GTK2 support, nobody needs that :)

::: gst-libs/gst/gl/gstgldisplay.h
@@ +68,2 @@
 GstGLAPI       gst_gl_display_get_gl_api             (GstGLDisplay * display);
+gpointer       gst_gl_display_get_platform_display   (GstGLDisplay * display);

Would this be an EGLDisplay with EGL, a X11 display connection with X11, etc.? Make it's usage consistent :) I think that would make sense in general, and then also connected with GstContext usage

::: gst-libs/gst/gl/wayland/wayland_event_source.c
@@ +113,2 @@
   if (source->pfd.revents) {
+    gst_gl_wl_display_roundtrip_queue (source->display, source->queue);

Why not the Wayland function?

::: tests/examples/gtk/gstgtk.c
@@ +62,3 @@
+#elif defined(GDK_WINDOWING_X11) && defined(GDK_WINDOW_XID)
+  gst_video_overlay_set_window_handle (videooverlay,
+      (guintptr) GDK_WINDOW_XID (gtk_widget_get_window (window)));

This code probably also needs some work to be ported to GTK3 for the other backends. Take a look at gst-plugins-base/tests/examples/overlay. There's a GTK example.

(In reply to comment #2)
> Review of attachment 256826 [details] [review]:
> 
> Does all this still work without setting a window handle/display, and then
> creates a new window?

For wayland_egl, and about the display: yes, if you go back to ready state first. No if you do that in paused or playing.
For wayland_egl surface, yes in paused or playing state too.

> 
> ::: configure.ac
> @@ +878,3 @@
> +  else
> +    PKG_CHECK_MODULES(GTK2, gtk+-2.0, HAVE_GTK_20=yes, HAVE_GTK_20=no)
> +    if test "x$HAVE_GTK_20" = "xyes"; then
> 
> Just remove GTK2 support, nobody needs that :)

WHy not keep it as it's done in my patch, then in a near future remove GTK2 support. No ?
Also it could be interesting for comparisons.

> 
> ::: gst-libs/gst/gl/gstgldisplay.h
> @@ +68,2 @@
>  GstGLAPI       gst_gl_display_get_gl_api             (GstGLDisplay * display);
> +gpointer       gst_gl_display_get_platform_display   (GstGLDisplay * display);
> 
> Would this be an EGLDisplay with EGL, a X11 display connection with X11, etc.?

With wayland_egl, it's a wl_display, not a EGLDisplay. EGLDisplay is created using gst_gl_window_get_display which return this wl_display too.
Later one is available when the window is already "open".

Note sure about glx/X11 and egl/X11 display, because in gst-gl 0.10 it was not necessary  that glimagesink and the application share the same instance of X11 Display. (same with win32, wgl, egl)

Maybe it's only related to wayland.

> Make it's usage consistent :) I think that would make sense in general, and
> then also connected with GstContext usage
> 
> ::: gst-libs/gst/gl/wayland/wayland_event_source.c
> @@ +113,2 @@
>    if (source->pfd.revents) {
> +    gst_gl_wl_display_roundtrip_queue (source->display, source->queue);
> 
> Why not the Wayland function?

Interesting point, initially I replaced the wl_display_dispatch (which is there in master) by wl_display_dispatch_queue. But that's not enough when a surface and its child does not share the same queue.
I found this morning this discussion https://bugs.launchpad.net/glmark2/+bug/1211076 . The comments have also an interest. It's not the exact same problem but it seems that the common point is that there is a difference between events and requests.

So I needed something like wl_display_roundtrip but for just a queue, and wl_display_roundtrip_queue does not exist. So I created gst_gl_wl_display_roundtrip_queue. See wl_display_roundtrip in
http://cgit.freedesktop.org/wayland/wayland/tree/src/wayland-client.c

> 
> ::: tests/examples/gtk/gstgtk.c
> @@ +62,3 @@
> +#elif defined(GDK_WINDOWING_X11) && defined(GDK_WINDOW_XID)
> +  gst_video_overlay_set_window_handle (videooverlay,
> +      (guintptr) GDK_WINDOW_XID (gtk_widget_get_window (window)));
> 
> This code probably also needs some work to be ported to GTK3 for the other
> backends. Take a look at gst-plugins-base/tests/examples/overlay. There's a GTK
> example.

Ok thx.

(In reply to comment #3)
> (In reply to comment #2)
> > Review of attachment 256826 [details] [review] [details]:
> > 
> > Does all this still work without setting a window handle/display, and then
> > creates a new window?
> 
> For wayland_egl, and about the display: yes, if you go back to ready state
> first. No if you do that in paused or playing.
> For wayland_egl surface, yes in paused or playing state too.

Sounds good :)

> > 
> > ::: configure.ac
> > @@ +878,3 @@
> > +  else
> > +    PKG_CHECK_MODULES(GTK2, gtk+-2.0, HAVE_GTK_20=yes, HAVE_GTK_20=no)
> > +    if test "x$HAVE_GTK_20" = "xyes"; then
> > 
> > Just remove GTK2 support, nobody needs that :)
> 
> WHy not keep it as it's done in my patch, then in a near future remove GTK2
> support. No ?
> Also it could be interesting for comparisons.

Or remove it later, just makes the configure check a bit more complicated for no good reason ;)

> > ::: gst-libs/gst/gl/gstgldisplay.h
> > @@ +68,2 @@
> >  GstGLAPI       gst_gl_display_get_gl_api             (GstGLDisplay * display);
> > +gpointer       gst_gl_display_get_platform_display   (GstGLDisplay * display);
> > 
> > Would this be an EGLDisplay with EGL, a X11 display connection with X11, etc.?
> 
> With wayland_egl, it's a wl_display, not a EGLDisplay. EGLDisplay is created
> using gst_gl_window_get_display which return this wl_display too.
> Later one is available when the window is already "open".
> 
> Note sure about glx/X11 and egl/X11 display, because in gst-gl 0.10 it was not
> necessary  that glimagesink and the application share the same instance of X11
> Display. (same with win32, wgl, egl)
> 
> Maybe it's only related to wayland.

So for Wayland there is a EGLDisplay and a separate wl_display? Ugly :/ I think this should somehow be made more clear then in the API, and more strict distinction between the EGLDisplay-like things and the wl_display.

> > Make it's usage consistent :) I think that would make sense in general, and
> > then also connected with GstContext usage
> > 
> > ::: gst-libs/gst/gl/wayland/wayland_event_source.c
> > @@ +113,2 @@
> >    if (source->pfd.revents) {
> > +    gst_gl_wl_display_roundtrip_queue (source->display, source->queue);
> > 
> > Why not the Wayland function?
> 
> Interesting point, initially I replaced the wl_display_dispatch (which is there
> in master) by wl_display_dispatch_queue. But that's not enough when a surface
> and its child does not share the same queue.
> I found this morning this discussion
> https://bugs.launchpad.net/glmark2/+bug/1211076 . The comments have also an
> interest. It's not the exact same problem but it seems that the common point is
> that there is a difference between events and requests.
> 
> So I needed something like wl_display_roundtrip but for just a queue, and
> wl_display_roundtrip_queue does not exist. So I created
> gst_gl_wl_display_roundtrip_queue. See wl_display_roundtrip in
> http://cgit.freedesktop.org/wayland/wayland/tree/src/wayland-client.c

Makes sense, maybe add a comment with all this explanation to the code :)

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> So for Wayland there is a EGLDisplay and a separate wl_display? Ugly :/ I think
> this should somehow be made more clear then in the API, and more strict
> distinction between the EGLDisplay-like things and the wl_display.

Not sure to follow you because a EGLDisplay is made from NativeDisplayType, see eglGetDisplay (http://www.khronos.org/registry/egl/sdk/docs/man/xhtml/eglGetDisplay.html)
so when using egl there is always a EGLDisplay and a separate native display.


To resume the handle returned by "gst_gl_window_get_display" is always the native display, either on egl, glx, wgl etc. (note that there is no glwayland, only egl)

Currently I can see that this is GstGLContext that own the EGLDisplay. (Note that the only way to retrieve the EGLDisplay from GstGLDisplay is to do GST_GL_CONTEXT_EGL (gst_gl_context_get_gl_context (context))->egl_display. 
Also eglGetDisplay is called in "gst_gl_context_egl_create_context".

So actually for egl we need a way to let the application know "its native display instance" to our gstegl engine. I choosed to use a a property on glimagesink but maybe a better solution would be to add GstVideoOverlay::gst_video_overlay_set_display_handle  ?

Well then the thing is how to transmit this native display handle from glimagesink to the GstGLWindow, before the context calls GstGLWindow::open.

Actually I did not realize that this is the same question for the window handle :) (gst_gl_window_set_window_handle) 
So I'm going to create a gst_gl_window_set_display_handle (as there is a gst_gl_window_get_display which one should be renamed to gst_gl_window_get_display_handle)

When not using egl, gst_gl_window_set_display_handle would just be empty.

That will allow to remove the confusing "gst_gl_display_new_with_platform".

Sounds like a good plan, I think I agree with you :) Sorry for the confusion.

But I still think that the wl_display should be propagated in the pipeline via GstContext, as would ideally the X11 display (note: not the connection. Just ":0" or ":1" for example, you could have multiple connections to that).

(In reply to comment #6)
> But I still think that the wl_display should be propagated in the pipeline via
> GstContext

Could you briefly describe how to do that from the application ? 

(just as a note the example app uses gtk3 and does not used any gl stuff except those from gst-plugins-gl) (Should I use gst_element_set_context ?)

Thx

The sink will:
1) query upstream for a wl_display context, if no success
2) post a need-context message on the bus

The message will be answered by a bin if a bin knows a context already, otherwise the application can answer it. If the application answer it, it works the same way as the GstVideoOverlay interface basically.

Check eglglessink for the GstContext handling.

Created attachment 260056 [details] [review]
wayland_egl: use subsurface to implement GstVideoOverlay interface

Now use GstContext instead of a property to pass the wl_display instance.
Also now the engine handles position and resizing from the parent surface.

(In reply to comment #9)
> Created an attachment (id=260056) [details] [review]
> wayland_egl: use subsurface to implement GstVideoOverlay interface
> 
> Now use GstContext instead of a property to pass the wl_display instance.
> Also now the engine handles position and resizing from the parent surface.

I think that the display handle should be stored inside subclasses of GstGLDisplay.  Ideally the idea would be to (all using GstContext) first ask for a GstGLDisplay and if that fails, ask for winsys specific display handles and if that also fails then ask for winsys specific display names.  Only after all that fails do we create our own GstGLDisplay (and hence winsys display).  Of course this would require extending our code to allow external display handle sources.

Also, some defined types for display handles should ideally end up in -base as well, much like the GstEGLDisplay in -bad/gst-libs/gst/egl.

Comment on attachment 260056 [details] [review]
wayland_egl: use subsurface to implement GstVideoOverlay interface

Sounds like a good plan, yes

Created attachment 266903 [details] [review]
wayland_egl: use subsurface to implement GstVideoOverlay interface

Just spend some few minutes to make it build with current master. (note that now subsurface has been moved from weston to wayland so it will not requires to manually add thoses subsurfaces protocol files with recent master)

Note that it seems that the subsurface work should be done in the app and so just pass to the sink as a normal wl_surface.

Well as subsurface is not yet integrated in gtk, we may wait what gtk decides to do and just keep this patch around just in case. And we may just use some part of it.

result: https://drive.google.com/file/d/0Bw6t368wtIMYbU16Tm5acGg0Y2M

I think this patch should be split up in multiple patches, there seem to be a few independent changes on top of each other :)

But I agree that we should wait for GTK to see how they're going to implement that. And also maybe talk with them to understand their plans.

What's the status of this?

I just grep subsurface in git gtk an there is still 0 occurrence.

In the end we should do like I suggested in my attached patch and like latest gstwaylandsink: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/wayland/wlwindow.c#n198  do.
I.e., creates the subsurface in the sink and not from the application, use gst_video_overlay_set_window_handle to pass the (parent) wlsurface you get from the application.

We should match this example http://cgit.collabora.com/git/user/gkiagia/gst-wayland-gtk-demo.git/tree/main.c.

In the end I think most of the attached patch can be use without any change. I'll happy to do it when I get back a proper env. Otherwise if anyone wants to go ahead I am happy too.

Oups I forgot to pull, there is actually a new window type: GDK_WINDOW_SUBSURFACE which is created with an internal function gdk_wayland_window_create_subsurface. Not sure if it get called by gdk_window_ensure_native or not, we should clarify the situation then.

Some progress!

commit 2704fdaa590b510316a0be1a4a41a7d77cada1ef
Author: Matthew Waters <matthew@centricular.com>
Date:   Thu May 21 17:48:31 2015 +1000

    gl/wayland: add GstGLDisplayWayland
    
    Simple implementation split from GstGLWindowWayland
    
    Can now have multiple glimagesink elements all displaying output
    linked via GL or otherwise (barring GL platform limitations).
    
    The intel driver is racy and can crash setting up the two glimagesink contexts.
    
    e.g.
    videotestsrc ! tee name=t ! queue ! glupload ! glimagesinkelement
      t. ! queue ! gleffects_blur ! glimagesinkelement
    
    videotestsrc ! glupload ! glfiltercube ! tee name=t ! queue ! glimagesinkelement
      t. ! queue ! gleffects_blur ! glimagesinkelement

Current status is that wayland subsurfaces are still poorly supported between all the combinations of weston, gnome-shell and gtk that I've tried.

Ok, so I have subsurfaces somewhat working with Gtk+.  The snag is that it seems to require some kind of intervention from the application to either tell us where to place the subsurface or manually create a GtkDrawingArea backed by a subsurface.  The former is implemented in the patch by Julien above, the latter would look something like https://git.gnome.org/browse/gtk+/tree/tests/subsurface.c

(In reply to Matthew Waters (ystreet00) from comment #18)
> Some progress!
>     gl/wayland: add GstGLDisplayWayland
>     Can now have multiple glimagesink elements all displaying output
>     linked via GL or otherwise (barring GL platform limitations).

Great !

(In reply to Matthew Waters (ystreet00) from comment #20)
> Ok, so I have subsurfaces somewhat working with Gtk+.  The snag is that it
> seems to require some kind of intervention from the application to either
> tell us where to place the subsurface or manually create a GtkDrawingArea
> backed by a subsurface.  The former is implemented in the patch by Julien
> above, the latter would look something like
> https://git.gnome.org/browse/gtk+/tree/tests/subsurface.c

Any update since last May ?

(In reply to Julien Isorce from comment #21)
> (In reply to Matthew Waters (ystreet00) from comment #20)
> Any update since last May ?

It's there, it mostly works however it still requires app intervention for resizes et al.

The general trend has been to use specific elements that integrate better with the toolkit/UI like gtk(gl)sink, qmlglsink, caopengllayersink and avoid GstVideoOverlay.

commit de389e07dc553110bb68cc5399bcfc3802df067f
Author: Matthew Waters <matthew@centricular.com>
Date:   Wed May 27 16:39:06 2015 +1000

    gl/wayland: implement basic video overlay support via subsurfaces
    
    Currently does not position the subsurface relative to the parent surface at all

commit e385adc241da19242574214d4129233098d277f8
Author: Matthew Waters <matthew@centricular.com>
Date:   Fri May 29 18:06:27 2015 +1000

    gl/wayland: implement setting the render rectangle
    
    Places our subsurface at the rectangle provided position