Bug 7949 - initial background doesn't follow session resize
Summary: initial background doesn't follow session resize
Status: CLOSED FIXED
Alias: None
Product: ThinLinc
Classification: Unclassified
Component: Misc (show other bugs)
Version: trunk
Hardware: PC Unknown
: P2 Normal
Target Milestone: 4.15.0
Assignee: Samuel Mannehed
URL:
Keywords: frifl_tester, ossman_tester, prosaic
Depends on:
Blocks: 8207
  Show dependency treegraph
 
Reported: 2022-06-20 15:58 CEST by Linn
Modified: 2023-09-07 10:12 CEST (History)
2 users (show)

See Also:
Acceptance Criteria:
MUST: * The background must not repeat after making the session larger. * The background configuration parameters in /sessionstart must still work as described. COULD: * The background could scale up after making the session larger. * The background could scale down after making the session smaller.


Attachments
Screenshot of how the background appears after making the session larger (434.03 KB, image/png)
2023-07-25 10:17 CEST, Samuel Mannehed
Details

Description Linn cendio 2022-06-20 15:58:56 CEST
This is regarding the background shown behind the profile chooser.

Currently, if the size of the client window is changed when choosing a profile, the background image tiles or is cut off, depending on if the window is becoming larger or smaller. Ideally, the background image should redraw itself, so it always fills the window no matter the size.
Comment 1 Samuel Mannehed cendio 2023-07-25 10:17:17 CEST
Created attachment 1142 [details]
Screenshot of how the background appears after making the session larger
Comment 2 Samuel Mannehed cendio 2023-07-25 10:20:23 CEST
We think it is common to resize the client window during startup or when the profile chooser is showing. In the current state, the background gets repeated in a way that does not look very appealing.

For this reason, we have decided to fix this as part of 4.15.0.
Comment 3 Samuel Mannehed cendio 2023-07-25 10:27:31 CEST
There is not as big an issue when resizing the session to become smaller. This results in parts of the graphic, and (most noticeably) the blue line, disappearing. The ThinLinc logo in the top-left corner stays put.
Comment 7 Samuel Mannehed cendio 2023-08-04 13:57:27 CEST
This should be all done now. Tested server build 3343 on Ubuntu 22.04 and Fedora 38:

> MUST:
> * The background must not repeat after making the session larger.
It does not. However, you can briefly see the repeating pattern before the resize kicks in.

> * The background configuration parameters in /sessionstart must still work as described.
They do. I tested both only a color, and an opaque background image. I also tested a partly transparent image together with a color.

> COULD:
> * The background could scale up after making the session larger.
It does. The image is scaled to fit the height as long as the session isn't wider than a 16:9 aspect ratio. For very wide sessions, it instead fits the width of the image.

> * The background could scale down after making the session smaller.
It does.

It should be noted that the tl-startup-bg process is killed after the profile chooser and other startup programs are done. This means that it doesn't update the root window background for resizes that happen while the user is connected to his desktop. The root window background is sometimes briefly visible when the user logs out. Thus, the background might not fit the size of the window when logging out.

Whether the background is visible or not seems to vary depending on the desktop environment and also on the client platform. The Windows client seems to close down the vncviewer window quicker than the Linux client.
Comment 8 Frida Flodin cendio 2023-08-08 14:39:46 CEST
Tested this on Ubuntu 22.04 and everything seems to work fine and as expected. Except for bug 7952, comment 10 that might have belonged better on this bug? Anyway, the session resize works great, and the background follows the resizing. 

> MUST:
> * The background must not repeat after making the session larger.
it doesn't
> * The background configuration parameters in /sessionstart must still work as described.
yes


> COULD:
> * The background could scale up after making the session larger.
yes
> * The background could scale down after making the session smaller.
yes
Comment 9 Samuel Mannehed cendio 2023-08-09 19:36:11 CEST
Unfortunately, PIL isn't available by default on RHEL9. Its available through the package python3-pillow in EPEL though.. 

Either we see if tl-setup can install it for us, or we package it ourselves. It is licensed under HPND, which shouldn't be a problem.
Comment 10 Samuel Mannehed cendio 2023-08-09 19:43:38 CEST
If we want to include it in ThinLinc, the newest Pillow version we can use is 5.4.1. It is the last Pillow version with Python 3.4 support.
Comment 11 Samuel Mannehed cendio 2023-08-09 19:46:33 CEST
Pillow version Python support can be found here:

https://pillow.readthedocs.io/en/latest/installation.html#python-support
Comment 12 Samuel Mannehed cendio 2023-08-10 14:37:35 CEST
We can't ship Pillow as part of ThinLinc. The imaging core part of PIL is a C-library which needs to be compiled targeting a specific python version. Since we use Python3 from the system, we need code that can work regardless of Python3 version.
Comment 13 Samuel Mannehed cendio 2023-08-11 17:58:07 CEST
We could install Pillow from tl-setup, but both RHEL and SLES require extra repos to be enabled to access the package. The name also differs slightly between platforms:

fedora 38	python3-pillow
rhel9		python3-pillow (EPEL)
ub20.04		python3-pil
ub22.04		python3-pil
sles15		python3-Pillow (PackageHub)

The effect of this would likely be that people installing ThinLinc on a fresh RHEL or SLES wouldn't see the background. EPEL and PackageHub are likely installed on production systems, though.

It seems the best approach right now is to revert the move from Cairo to Pillow. Tests indicate that we can still get the same resize and multi-monitor functionality with Cairo surfaces. Things seem a bit slower than Pillow, sadly, but I think that's something we can live with.
Comment 23 Samuel Mannehed cendio 2023-08-13 20:31:03 CEST
This has now been refactored to use Cairo once again. It turns out doing this for multiple monitors is quite a lot more complex when using surfaces than when using pillow images. One big reason for the simplicity of the Pillow solution was that our python-xlib library has a nice convenience function for PIL images (put_pil_image()).

Now, we instead had to extract the pixel data from the surface and then split it into several requests to put it in the pixmap.

The result is a bit slower. The performance issues are mostly noticeable when using full screen with multiple high-resolution monitors. The slowness seems to be caused by our new pixel data handling.

We have full unit test coverage, and I have tested manually with 2 monitors in different configurations on Fedora 38.

Note that it has proven important during testing to switch which monitor is primary.
Comment 29 Linn cendio 2023-08-15 16:49:48 CEST
Tested with server on Ubuntu 22.04, and resizing works well both when the session is made larger and when it is made smaller. Tested pictures with transparent areas, multiple monitors with different resolutions and monitors that were not perfectly aligned with each other. 

When using two aligned 1920 x 1200 monitors the redraw took 1-2 sec, which is noticeable but not way too slow.

The issue with the slowness have been moved to a separate bug, which will be linked to this bug shortly.
Comment 30 Pierre Ossman cendio 2023-08-16 11:40:28 CEST
Tested various screen configurations, and it always¹ correctly configured the background. I tried:

 * Windowed
 * Full screen on a single monitor
 * Full screen on two identical monitors, side by side
 * Full screen with three monitors, two 1080p, one 2160p, horizontal layout
 * Full screen with three monitors, with monitors jumbled in both horizontal and vertical layout
 * Resizing the session faster than the background has time to change

I also tried going from a single monitor 3840×1200 to two 1920×1200 (i.e. no change in the total frame buffer size), and it handled that perfectly with the background switching from a single large image, to two separate images.

¹ except when bug 7014 was triggered
Comment 31 Samuel Mannehed cendio 2023-09-07 10:12:45 CEST
For posterity, here are the reasons for a two previously undocumented decisions:

 * The switch from pyxlib-ctypes to python-xlib was made due to the availability of X extension bindings. The Xrandr extension, more specifically, was only available through python-xlib. It should be noted that a big advantage of pyxlib-ctypes (which we used before this bug), is that it uses the standard libX11. The standard C library is used everywhere in Linux and will inherently be more well tested. We should think twice about which Xlib library we use in future ThinLinc code.

 * The switch from Cairo to Pillow was originally made due to the more convenient method "put_pil_image()" which was available through python-xlib. Non-Pillow images have to use the less friendly method "put_image()". Due to the availability issues of Pillow on some target platforms we were, as mentioned in earlier comments, forced to switch back to Cairo.

Note You need to log in before you can comment on or make changes to this bug.