[Bug 789384] New: glimagesink windows on wayland are created with a fixed window size of 320x240

["GStreamer" (GNOME Bugzilla)]

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

Bug ID: 789384
Summary: glimagesink windows on wayland are created with a
fixed window size of 320x240
Classification: Platform
Product: GStreamer
Version: 1.12.3
OS: Linux
Status: NEW
Severity: normal
Priority: Normal
Component: gst-plugins-bad
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@gmail.com
QA Contact: gstreamer-***@lists.freedesktop.org
GNOME version: ---

glimagesink windows on wayland are created with a fixed window size of 320x240

["GStreamer" (GNOME Bugzilla)]

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

memeka <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

--- Comment #1 from memeka <***@gmail.com> ---
Created attachment 362145
--> https://bugzilla.gnome.org/attachment.cgi?id=362145&action=edit
fix for glimagesink wayland window size

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #362145|none |needs-work
status| |

--- Comment #2 from Matthew Waters (ystreet00) <***@gmail.com> ---
Review of attachment 362145:
--> (https://bugzilla.gnome.org/review?bug=789384&attachment=362145)

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.

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #3 from memeka <***@gmail.com> ---
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?

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #4 from memeka <***@gmail.com> ---
Created attachment 362226
--> https://bugzilla.gnome.org/attachment.cgi?id=362226&action=edit
v2-glimagesink-wayland-add-preffered-window-size

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #5 from memeka <***@gmail.com> ---
sorry I uploaded the old patch by mistake, uploading the new one now.

["GStreamer" (GNOME Bugzilla)]

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

memeka <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #362226|none |rejected
status| |

--- Comment #6 from memeka <***@gmail.com> ---
Review of attachment 362226:
--> (https://bugzilla.gnome.org/review?bug=789384&attachment=362226)

old patch uploaded by mistake

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #7 from memeka <***@gmail.com> ---
Created attachment 362227
--> https://bugzilla.gnome.org/attachment.cgi?id=362227&action=edit
v3-glimagesink-wayland-add-preffered-window-size

["GStreamer" (GNOME Bugzilla)]

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

Julien Isorce <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

["GStreamer" (GNOME Bugzilla)]

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

Tim-Philipp Müller <***@zen.co.uk> changed:

What |Removed |Added
----------------------------------------------------------------------------
Component|gst-plugins-bad |gst-plugins-base

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@intel.com

--- Comment #9 from Matthew Waters (ystreet00) <***@gmail.com> ---
*** Bug 796882 has been marked as a duplicate of this bug. ***

["GStreamer" (GNOME Bugzilla)]

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

***@intel.com changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@intel.com

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #10 from Matthew Waters (ystreet00) <***@gmail.com> ---
*** Bug 797026 has been marked as a duplicate of this bug. ***

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
CC| |***@gmail.com
Resolution|--- |FIXED
Target Milestone|git master |1.15.1

--- Comment #11 from Matthew Waters (ystreet00) <***@gmail.com> ---
commit 090bbd07219a62c0146d9654fd6a3159f1049426
Author: Matthew Waters <***@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 <***@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

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #362228|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #362145|0 |1
is obsolete| |

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #362226|0 |1
is obsolete| |

["GStreamer" (GNOME Bugzilla)]

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

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #362227|0 |1
is obsolete| |

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #12 from ***@intel.com ---
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.

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #13 from Matthew Waters (ystreet00) <***@gmail.com> ---
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.

["GStreamer" (GNOME Bugzilla)]

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

--- Comment #14 from ***@intel.com ---
(In reply to Matthew Waters (ystreet00) from comment #13)

Post by "GStreamer" (GNOME Bugzilla)
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?

["GStreamer" (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@coaxion.net

--- Comment #15 from Sebastian Dröge (slomo) <***@coaxion.net> ---
(In reply to long1.wang from comment #14)

Post by "GStreamer" (GNOME Bugzilla)
(In reply to Matthew Waters (ystreet00) from comment #13)

Post by "GStreamer" (GNOME Bugzilla)
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.