Created attachment 362145 [details] [review]
fix for glimagesink wayland window size

Review of attachment 362145 [details] [review]:

This would break the set_render_rectangle() use case.

The order for chosen sizes is:
1. set_render_rectangle()
2. preferred size
3. some default fallback

This patch essentially removes 1) from ever happening.

::: gst-libs/gst/gl/wayland/gstglwindow_wayland_egl.c
@@ +306,3 @@
 
+  if (window_egl->window.preffered_width > 0)
+    width = window_egl->window.preffered_width;

s/preffered/preferred/

@@ -304,3 @@
 
-  if (window_egl->window.window_width > 0)
-    width = window_egl->window.window_width;

This cannot just be removed as it's also used for the set_render_rectangle() case.

@@ +312,3 @@
 
+  if (window_egl->window.preffered_height > 0)
+    height = window_egl->window.preffered_height;

s/preffered/preferred/

@@ +582,3 @@
+  window_egl->window.preffered_height = height;
+  if (window_egl->window.window_height != height || window_egl->window.window_width != width) {
+    window_resize (window_egl, width, height);

Is this call really necessary?  This would also prefer the preferred size rather than a possible set_render_rectangle() size which is not correct.

I'm not sure what is the intended order of events, but this is what happens in reality:

* create_subsurfaces is called before window_width and window_height are set
* create_subsurfaces is called before preferred_height and preferred_width are set from my patch
* window of size 320x240 (defaults) is created
* caps are set, video continues to play in 320x240 window

This is what happens with my patch:

* like above, create_subsurfaces is created with 320x240 default window
* create_subsurfaces sets window_height and window_width to default values
* set_preferred_size is called (from caps?), preferred values are set
* window_height and window_width are checked against preferred vales, then window is resized

> +  if (window_egl->window.window_height != height || window_egl->window.window_width != width) {
> +    window_resize (window_egl, width, height);
> Is this call really necessary?  This would also prefer the preferred size rather than a possible set_render_rectangle() size which is not correct.

so yes, window_resize is really necessary, or else window is again stuck at 320x240 for all videos.

> -  if (window_egl->window.window_width > 0)
> -    width = window_egl->window.window_width;
> This cannot just be removed as it's also used for the set_render_rectangle() case.

Not sure when set_render_rectangle is or should be called, but like i said, window_width and window_height are set by create_subsurfaces to the default values. afterwards, I could detect no resize event, and values remain set the these defaults.
I could check first if window_width && window_height are > 0, if so set width/height to these values; if not then check for preferred values to set, and lastly set default values. Would that be more convenient?

Created attachment 362226 [details] [review]
v2-glimagesink-wayland-add-preffered-window-size

sorry I uploaded the old patch by mistake, uploading the new one now.

Review of attachment 362226 [details] [review]:

old patch uploaded by mistake

Created attachment 362227 [details] [review]
v3-glimagesink-wayland-add-preffered-window-size

Created attachment 362228 [details] [review]
v3-glimagesink-wayland-add-preferred-window-size

*** Bug 796882 has been marked as a duplicate of this bug. ***

*** Bug 797026 has been marked as a duplicate of this bug. ***

commit 090bbd07219a62c0146d9654fd6a3159f1049426
Author: Matthew Waters <matthew@centricular.com>
Date:   Tue Aug 28 14:31:43 2018 +1000

    gl/wayland: correctly use the set_render_rectangle size first
    
    https://bugzilla.gnome.org/show_bug.cgi?id=789384

commit 08ebb8264debb3b03d31a3a66ad61c0ac3e23812
Author: memeka <mihailescu2m@gmail.com>
Date:   Tue Oct 24 17:39:50 2017 +1030

    gl/wayland: add preferred window size, and set it according to video size
    
    The glimagesink wayland backend lacks the implementation of
    gst_gl_window_wayland_egl_set_preferred_size. Because of this, glimagesink windows on
    wayland are created with a fixed window size of 320x240.
    
    [Matthew Waters]: gst-indent sources
    
    https://bugzilla.gnome.org/show_bug.cgi?id=789384

Hi, Maintainer,

Could you push this patch into one tag? It should be something like 1.14.3 etc. Because from Clear Linux integration point of view, need merge it from a fixed version by tag.

This will become a part of the future 1.16 release.  You are welcome to backport the patch in your distribution if you need it.

(In reply to Matthew Waters (ystreet00) from comment #13)
> This will become a part of the future 1.16 release.  You are welcome to
> backport the patch in your distribution if you need it.

In Clear Linux way, it's heavily depends on upstream's tag release. Because just by a tag release, there will be an URL of tarball. Then it can be fetched into Clear Linux spec file to integrate.

So question is when this future 1.16 is released?

(In reply to long1.wang from comment #14)
> (In reply to Matthew Waters (ystreet00) from comment #13)
> > This will become a part of the future 1.16 release.  You are welcome to
> > backport the patch in your distribution if you need it.
> 
> In Clear Linux way, it's heavily depends on upstream's tag release. Because
> just by a tag release, there will be an URL of tarball. Then it can be
> fetched into Clear Linux spec file to integrate.

The tarball is not a proper release tarball though, it is going to be missing files and compiling from that is not supported.

I'd recommend backporting this patch for the time being if you don't want to wait for 1.16.