For what it's worth, I've reviewed this from the Wayland side of things, and it looks fine to me.

Hi George, well done!

Just trying to catch-up, do you have the log of the discussion somewhere that leads to this GstWaylandVideo interface ? :)

As I have not followed those discussions I wonder if gst_video_overlay_set_render_rectangle could have been used instead of a new gst_wayland_video_set_surface_size ?

Also why is it not possible to manage the _pause_rendering / _resume_rendering in the waylandsink directly ?
I'm not against, I just wonder :). Also this GstWaylandVideo could be useful in glimagesink for our wayland backend.

I think one commit for the lib, and 1 (or 2 if you have initial cleanup) commit for ext/wayland would be fine to merge it.

Finally I asked gtk maintainer if it could be possible to handle most of the subsurface code of gtk itself when using "gdk_window_ensure_native (gdkwindow)".
Could save some lines here http://cgit.collabora.com/git/user/gkiagia/gst-wayland-gtk-demo.git/tree/main.c and avoid to duplicate same code over and over on each application etc...
They said it sounds useful. So at this point we should just open a bug and ask them to implement the prototypes we need. We can discuss that on IRC.

Having this in gtk, would it allow to remove those 2 _pause_rendering / _resume_rendering functions ?

Cheers!

(In reply to comment #2)
> Hi George, well done!
> 
> Just trying to catch-up, do you have the log of the discussion somewhere that
> leads to this GstWaylandVideo interface ? :)

I don't think there was a discussion about that. There was only an irc discussion with Daniel about the requirements of the sink for resizing and after that I came up with this interface.

> As I have not followed those discussions I wonder if
> gst_video_overlay_set_render_rectangle could have been used instead of a new
> gst_wayland_video_set_surface_size ?
> 
> Also why is it not possible to manage the _pause_rendering / _resume_rendering
> in the waylandsink directly ?
> I'm not against, I just wonder :). Also this GstWaylandVideo could be useful in
> glimagesink for our wayland backend.

I'm glad that you raised this topic.

gst_video_overlay_set_render_rectangle can indeed be used instead of gst_wayland_video_set_surface_size. The reason I didn't do that is because the semantics are slightly different I think. set_render_rectangle is to specify a sub-region inside the given surface, while in our case we want to draw in the whole given surface, the only problem being that surfaces in wayland have no specific size. So we need a method to set that size and complement the lack of such an api in wayland directly. This is no big deal, though, especially if we document it. But for the moment we need GstWaylandVideo anyway (see below), so that's why I have kept it like this.

_pause_rendering and _resume_rendering now are another problem. These functions intend to solve the problem that we need to synchronize the main drawing thread with waylandsink's drawing thread when a resizing happens.

In general, wayland subsurfaces have an API for synchronization, wl_subsurface_set_(de)sync (afair). When a subsurface is set in sync mode, all its redrawing operations are queued to happen when the parent (sub)surface is redrawn. This is handy here and of course we expect toolkits to use it, but it's not enough. When the parent (sub)surface is redrawn, we cannot guarantee that the last drawn frame of the gstreamer subsurface has the correct geometry. This is why _resume_rendering exists and as you can see, it waits for a new frame to be drawn (which will be drawn with the new geometry). If we don't do that, resizing has quite visible artifacts.

Now before I added the wl_scaler support, _pause_rendering was also quite useful, but I've started to think that it perhaps can go away now. The reason it was added in the first place was to avoid rendering anything while the internal state of the sink was being changed, as a resize would trigger a caps & pool change, which would have to be negotiated before continuing. It also still serves as an optimization in order to avoid sending buffers to wayland that will never be shown, as the subsurface is in sync mode and it will be redrawn in _resume_rendering before the end of the resize operation.

So, perhaps if we drop _pause_rendering and its performance optimization and replace _set_surface_size with _set_render_rectangle, then maybe we can also replace _resume_rendering with gst_video_overlay_expose, but again the semantics are different imho. It's doable if we document it, though.

> Finally I asked gtk maintainer if it could be possible to handle most of the
> subsurface code of gtk itself when using "gdk_window_ensure_native
> (gdkwindow)".
> Could save some lines here
> http://cgit.collabora.com/git/user/gkiagia/gst-wayland-gtk-demo.git/tree/main.c
> and avoid to duplicate same code over and over on each application etc...
> They said it sounds useful. So at this point we should just open a bug and ask
> them to implement the prototypes we need. We can discuss that on IRC.

Yes, gtk definitely needs to implement that stuff. And also fix a lot of bugs...

Also about GstWaylandWindowHandle http://cgit.collabora.com/git/user/gkiagia/gst-plugins-bad.git/tree/gst-libs/gst/wayland/wayland.h?h=waylandsink#n37 there is a solution to remove it.


struct wl_display *display = gdk_wayland_display_get_wl_display (gdk_window_get_display (gtk_widget_get_window (widget)));
GstContext *context = gst_context_new_display_handle ((guintptr) display, FALSE);

In gstgl we use gst_element_set_context (GST_ELEMENT (GST_MESSAGE_SRC (message)), context); from the application to pass the wl_display

The waylandsink should call msg = gst_message_new_need_context (GST_OBJECT_CAST (waylandsink), GST_DISPLAY_HANDLE_CONTEXT_TYPE); 

See https://bug709747.bugzilla-attachments.gnome.org/attachment.cgi?id=266903 the whole commit is wrong but just about how to use GstContext. There are other examples in gstg (http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/gstglfilter.c#n221)l and eglglessink.

That would be great to pass the wl_display this way because we already do this way in gstgl. In order to uniform.

Also this way you could just pass directly the wl_surface handle to set_window_handle, and set the siwe using the function you introduced gst_wayland_sink_set_surface_size.

The it will remain gst_wayland_video_set_surface_size, pause / resume in libgstwayland.

As we discuss on IRC it's ok to add this in the mean time gtk has no subsurface support and also to have more control (to optimize also). Also we should not rely on if an UI tool kit has the wl_subsurface support or not. So we will still need those 3 functions.

Later we could move those 3 functions to GstVideoOverlay.

Before I forget there were also 4 small things I noticed:
- G_BEGIN_DECLS/ END are missing in ext/wayland/wldisplay.h and wlvideoformat.h
- if wrong location in filesrc, "warning : gst_wl_window_is_top_level : assertion window != NULL failed", gkiagia: "missing check, in case the window does not exist when the sink stops"
- scaler-client-protocol.h is not generated though present in Makefile.am
- crash when its bufferpool get negotiated with upstream, it seems the pool get stopped twice

I have fixed most of the mentioned issues:
* Use GstContext for the display handle
* G_BEGIN/END_DECLS
* assertion failure if the window does not exist when the sink stops
* scaler-client-protocol.h not being generated

Plus I have rebased on top of master, fixed a g-i generation issue and updated wl_scaler to version 2.

I cannot reproduce the mentioned crash in the buffer pool, but afair from my discussion with Julien on irc at the time of writing of his comments, the backtrace was showing a possible bug in GStreamer core (the buffer pool was being stopped twice, which should not be possible to happen normally)

And with some more improvements last week, I fixed some locking issues and also simplified the GstWaylandVideo interface as it has been discussed in the past:

* set_surface_size is gone & replaced by gst_video_overlay_set_render_rectangle

The semantics also changed a bit there. The subsurface is now being created inside waylandsink, so the parent application only needs to set its top-level surface as the window handle and use set_render_rectangle to specify the sub-area of the window where the video should appear.

* pause_rendering is now begin_geometry_change
* resume_rendering is now end_geometry_change

The implementation of these methods has also changed to just do wl_subsurface_set_sync/desync and resizing is implemented by directly committing a wl_viewport.set_destination change inside set_render_rectangle (instead of waiting for the next redraw in the streaming thread). This way, resizing is independent of rendering and the API is faster and simpler. This is only possible because of wl_scaler version 2, though, and tbh, there is a bug in weston that prevents it from fully working as expected (so resizing is not really smooth unless you also call gst_video_overlay_expose() after set_render_rectangle() and there are some complications there, but this needs fixing in weston)

The nice part about these changes is that waylandsink can now be used properly with gtk without ugly hacks in the client and it works pretty well. The sample code has also been updated:

http://cgit.collabora.com/git/user/gkiagia/gst-wayland-gtk-demo.git/tree/main.c

Great!

Are the (4 or 5) gstwaylandsink patches submitted recently obsoletes ?

I have no other remarks but maybe the contributors mentioned in the copyright have ? Otherwise please push.

(In reply to comment #8)
> Great!
> 
> Are the (4 or 5) gstwaylandsink patches submitted recently obsoletes ?

If you mean the patches on bugzilla, yes, they are obsolete.

> I have no other remarks but maybe the contributors mentioned in the copyright
> have ? Otherwise please push.

Thanks! :)

Merged in master. eb8ab3732e0abb0e364b99037819b8ce1b337b88 up to 0badc1f5fb3d5669d3ef9bcc5a586f80572360f5

Arnaud Vrac (1):
      wayland: install .pc file

George Kiagiadakis (66):
      waylandsink: tidy up the header files
      waylandsink: process display events in a separate thread
      waylandsink: remove unused variables
      waylandsink: Use XDG_RUNTIME_DIR instead of /tmp for the shm file
      waylandsink: move struct shm_pool and its related functions to waylandpool.c
      waylandsink/waylandpool: move code around for better readability
      waylandsink: split video format related functions out to a separate file
      waylandsink/waylandpool: refactor code
      waylandsink/waylandpool: find the video format from the GstVideoInfo instead of accessing the sink
      waylandsink/waylandpool: improve debug message
      waylandsink: remove callback and redraw_pending variables from the window structure
      waylandsink: split window-related code out to a new GstWlWindow class
      waylandsink: cleanup header includes
      waylandsink: apply the same debug category to all the subobjects
      waylandsink: remove the useless wayland_lock
      waylandsink: access sink->pool in a more atomic fashion
      waylandsink: make the display property useful
      waylandsink/waylandpool: ref the display instead of the sink to avoid cyclic references
      waylandsink: unref the buffer pool
      waylandsink/wlvideoformat: add mappings for many common formats
      waylandsink: handle the list of supported formats properly
      wayland: Add new gst-wayland library containing a new GstWaylandVideo interface
      waylandsink: implement with stubs the GstWaylandVideo & GstVideoOverlay interfaces
      waylandsink: implement the GstVideoOverlay & GstWaylandVideo interfaces
      waylandsink/waylandpool: unlink mmaped shm files so that they don't remain on the file system
      waylandsink/waylandpool: call the start/stop methods of the parent class
      waylandsink/waylandpool: remove useless munmap call
      waylandsink: Handle wl_buffer::release and don't reuse buffers that are not released
      waylandsink: Wait for the frame_cb to redraw and drop frames meanwhile
      waylandsink: Set external surfaces and their child objects to use our own event queue
      waylandsink: Build bindings for the unstable wl_scaler spec
      waylandsink: Use wl_scaler/wl_viewport to scale the surface in the compositor/hardware
      waylandsink: Implement expose() and handle resizing properly in non-PLAYING states
      waylandsink: Use a boolean in combination with render_cond to comply with GCond's usage documentation
      waylandsink: increase debug messages
      waylandsink/wlwindow: reuse code between the two constructors
      waylandsink: set an empty input region on the video surface
      waylandsink: fix crash in case there is no pool because of a caps negotiation error
      waylandsink: Support all video formats supported by the display
      waylandsink: create/destroy the display when entering/leaving the READY state instead of PAUSED
      waylandsink/wldisplay: bind to the latest available wl_compositor version
      waylandsink: Add myself to the authors list
      waylandsink: remove unused functions
      waylandsink/Makefile.am: Fix scaler-client-protocol.h generation
      wayland/Makefile.am: link with gstvideo to avoid introspection errors
      waylandsink: Update wl_scaler to version 2
      waylandsink: fix assertion failure when stopping immediately after starting, without displaying anything
      waylandsink: add G_BEGIN/END_DECLS on all headers for consistency
      waylandsink: drop width/height arguments from gst_wl_window_new_from_surface()
      waylandsink: get the external display handle using GstContext
      wayland: add public API for creating & using the display handle GstContext
      waylandsink: create and maintain the subsurface inside the sink
      wayland: remove gst_wayland_video_set_surface_size()
      waylandsink: cleanup GstWlWindow a bit after the overlaying semantics change
      waylandsink: move surface resizing logic to the GstWlWindow and make it be called from the main thread
      waylandsink: Replace the OBJECT_LOCK with a private render_lock to lock render operations
      waylandsink: remove the OBJECT_LOCK from set_caps()
      waylandsink: protect access to the display with a new display_lock
      waylandsink: protect access to properties with the OBJECT_LOCK
      waylandsink: remove the manual synchronization from pause/resume_rendering and use subsurface sync/desync
      waylandsink: rename pause/resume_rendering to begin/end_geometry_change and update their documentation
      waylandsink: improve the way the video size is passed to wlwindow and also improve the code for window creation
      waylandsink/wlwindow: take into account the video aspect ratio when determining the size of the surface
      waylandsink/wlwindow: do not commit a resize when it happens due to a video info change
      waylandsink: remove the buffer from the surface when going PAUSED -> READY
      waylandsink/wldisplay: verify that all the required interfaces have been found on the compositor

I'm a bit unhappy how this set of commits just snuck in a new library with new application API here.

Even if it's gst-plugins-bad and not necessarily API stable, that's not how we do things.

New API and new libraries should be explicitly posted in bugzilla for review, and I understand in this case there have even been conversations that indicated that this might not be the best or final solution yet.

It's one thing if this is a helper lib for plugins, but this is application API so it's doubly unfortunate that we're now adding new API for applications that's possibly dead on arrival and deprecated before even having been released.

Can we disable the new lib for the time being and aim to sort this out in the next cycle?

Also see bug #732207 and bug #732280

IMHO we should not install the headers of the lib at all as it's not even clear that this is how toolkits are supposed to work with Wayland. This is all very experimental and not a finished design yet.

See discussion in bug #732280.

(In reply to comment #11) 
> New API and new libraries should be explicitly posted in bugzilla for review,
> and I understand in this case there have even been conversations that indicated
> that this might not be the best or final solution yet.

Well, the purpose of this bug report was exactly that, although I admit I probably did not make it very clear. Sorry for the confusion.
 
> Can we disable the new lib for the time being and aim to sort this out in the
> next cycle?

We can, but then waylandsink can't be embedded anywhere. Unless we can disable it by default and have people build it on demand if needed.


(In reply to comment #13)
> IMHO we should not install the headers of the lib at all as it's not even clear
> that this is how toolkits are supposed to work with Wayland. This is all very
> experimental and not a finished design yet.

About the GstWaylandVideo interface, OK. Maybe we can follow Julien's advice (bug 732280 comment 3) and transform those two functions into action signals and kill the interface then.

But what about the GstContext methods that this library contains?

(from the header)
-----------------
/* The type of GstContext used to pass the wl_display pointer
 * from the application to the sink */
#define GST_WAYLAND_DISPLAY_HANDLE_CONTEXT_TYPE "GstWaylandDisplayHandleContextType"

gboolean gst_is_wayland_display_handle_need_context_message (GstMessage * msg);
GstContext *
gst_wayland_display_handle_context_new (struct wl_display * display);
struct wl_display *
gst_wayland_display_handle_context_get_handle (GstContext * context);
------------------

This should for sure be useful as well to any element that connects to wayland (ex. glimagesink) and I don't think it should go away.

(In reply to comment #14)
> We can, but then waylandsink can't be embedded anywhere. Unless we can disable
> it by default and have people build it on demand if needed.

Sorry, just to clarify: The GstWaylandVideo interface is optional to embed waylandsink (although recommended), but the wayland display GstContext is required.

Maybe worth some clarity, but I think the idea is to drop the lib from the public API (that might mean dropping the feature involved) for 1.4, so nobody builds on top of it, then it can be re-introduce for 1.5, and hopefully it will be right and ready for 1.6.