Comment on attachment 266474 [details] [review]
waylandsink: fix surface width/height when caps have changed

I might be missing somethnig but where does this recreate the window after caps changes?
What other sinks are doing is to keep the window size btw... and only adapt video rendering (adding block borders if necessary, scaling, etc.). Let's keep the behaviour between the different sinks the same.

Apart from that this code is broken for non-1/1 pixel-aspect-ratio. Already before your changes though :)

Window isn't recreated only dst.w and dst.h change and window dimension change when the surface is commited.

While user can't change the size of the window I done the assumption that video size == window size.

Do you have an example of how use pixel-aspect-ratio ?

Wayland have a client-side decoration model.
My assumption is that waylandsink doesn't have to provide this decoration, the drawback is that it doesn't provide edges where user interact to re-size the window.

If my assumption is correct window size should always be equal to video size.
If not, does waylandsink have to provide the decorations ?

Created attachment 267416 [details] [review]
waylandsink: allow window resizing

Weston allow to resize the window by keeping pressed CTRL + middle button.
waylandsink then receive configure event with new width and height so before display a new buffer send a reconfigure event to try to negotiate new caps with upstream element.

Wayland assume that the buffer size is equal to the surface to fill so we need to renegotiate new caps and we can't do resize inside the server like XVimagesink can do.

I have make shm_pool creation/destruction depend of wayland life instead of mixing it with sink life.

I have also some clean up around in sink width/height field.

gst-play with files with different resolution is also working now.

Review of attachment 267416 [details] [review]:

Generally looks good. But it's not clear to me what happens if upstream just ignores the reconfigure event and the new size will not be configured. What will happen?

::: ext/wayland/gstwaylandsink.c
@@ +308,3 @@
+    caps = gst_caps_make_writable (caps);
+
+    /* There can only be a single structure because the xcontext

xcontext?

@@ +320,3 @@
+     * fixed width/height caps again if not possible
+     * upstream */
+    if (filter) {

Put the filter bit outside the "if (sink->window)" block at the very end and just always do that

::: ext/wayland/gstwaylandsink.h
@@ +79,3 @@
   struct wl_callback *callback;
   guint redraw_pending :1;
+  gboolean reconfig_needed;

If you already use bitfields here, use them in a way that actually makes sense ;)

Created attachment 268064 [details] [review]
waylandsink: allow window resizing

This patch fix the comments.
"xcontext" comes from the copy/paste I have done from ximagesink to manage reconfigure.

Review of attachment 268064 [details] [review]:

Ok, so what happens if the reconfiguration does not happen and the old size is continued to be used? Is the frame just cropped or black borders added?

::: ext/wayland/gstwaylandsink.c
@@ +296,3 @@
     gst_caps_unref (caps);
     caps = intersection;
   }

You filter here too already, remove that part of the code... not useful to filter twice.

Created attachment 268068 [details] [review]
waylandsink: allow window resizing

If reconfigure failed, wayland window size remain the same without black borders or cropping.

Any additional comments on this patch ?

Hi, not blocking but just to know how hard would it be to merge with https://bugzilla.gnome.org/show_bug.cgi?id=726193 ?

Hi,

It seems that George is doing a very large rework of waylandsink, when do you schedule to merge it ? If it is after 1.4 release, I think you can push my patch to improve a little wayland support.

Hi,

I merged my branch today. My code takes into consideration the caps changes and the gst-play example in the description now works, but there are still some problems if the video is embedded with GstVideoOverlay. I am going to open a new bug for that, though, as the problem is different.

However, I'm not sure if this bug should be closed yet, because I have not implemented the same functionality as in attachment 268068 [details] [review] (the part that handles the configure event from the compositor and resizes the video). I will have a look at it as soon as I can.

Created attachment 287932 [details] [review]
waylandsink: allow window resizing

I have rework my patch to match with your latest development.
We have tested it on our board and it works fine now.

Review of attachment 287932 [details] [review]:

Sounds like something we want. Quick question, why do you delay sending the reconfigure event upstream ?

Created attachment 336086 [details] [review]
waylandsink: manage server window resizing

I have attached another patch that probably (since I did not have time to read the history of this bugzilla) deals with the same feature.
In this version no configure event is triggered through the pipeline. We just make use of the wayland viewport protocol

I started to play with that a bit today. Unless I'm doing something wrong, the implementation of wl_shell_surface does not work in Weston. This greatly limits my ability to test. Both weston and gnome-wayland leaves the edges to 0. I ended up with a completly different implementation. I'll try and clarify what behaviour is to be expected.

I've sent a very partial implementation. I think the old method previously proposed was not suitable with the use of viewporter. In that version, I simply update the render rectangle to be the size of the new request window. Which will by side effect resize the window. If you have the viewporter, it will scale the video keeping aspect ratio, if you don't, the video is kept centered unscaled. This of course is only for the special case where GStreamer create the top level window. Note, it's not perfect for the non-viewporter case, as we don't crop the output like ximagesink would do. It's require some work, as you need to destroy and create new wl_buffer for that. Probably not worth it, to be useful, this element really depends on viewporter.

commit f6b270d8eb0583940b59af35ea8552876234d335
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Sep 22 18:55:23 2016 -0400

    waylandsink: Update our window size on configure event
    
    This is specific to when the waylandsink is not being embedded. In
    this patch we pass the render lock to the window so it can safely
    call gst_wl_window_set_render_rectangle() with the new size.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=722343

Next step will be to push a reconfigure event upstream, and to teach get caps to prepend an "ideal" resolution. I haven't read yet, but I suppose this is most likely what Benjamin patch do.

I've just created this bug, to make the top level window create by the element inline with what an application embedding the window would be.

https://bugzilla.gnome.org/show_bug.cgi?id=792874

Overall, the main issue if you compare X11 Window and WL Surface, it's that the WL Surface does not have a width/height you can request. Hacking around assuming windows width/height is the render rectangle width/height is quite a hack. I'm tempted to propose a new method to GstVideoOverlay. Something like set_window_size(). With that information, we could crop the output for the non-viewporter case, hence stop leaking out of the allocation. And then for all cases, when 792874 is fixed, we'll have a more consistent result when comparing with xvimagesink/ximagesink and other embeddable sinks out there.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/125.