genodelabs / genode

Genode OS Framework
https://genode.org/
Other
1.07k stars 253 forks source link

nitpicker: restore usability of capture session for screen-recording tools #5365

Open jschlatow opened 1 week ago

jschlatow commented 1 week ago

As mentioned in #5356, the behaviour of nitpicker's Capture::Connection::screen_size() has changed. Previously, screen_size() has been used to get the bounding box of all capture clients (i.e. the panorama). Now, it appears as if screen_size() is used by capture clients to get their policy-affected individual geometry, which is a reasonable use case. However, the screen_size_sigh is not called on policy changes but only on panorama changes. I may be missing something here, yet I'm wondering whether this is actually the intended behaviour.

The use case where this issue popped up is the use of the Capture session for screen-recording tools like screenshot or VNC server. These tools ideally require access to the panorama geometry in order to decide what area to capture. Thus, I'm wondering whether the Capture session should be extended by a panorama_size() method.

chelmuth commented 1 week ago

Actually, I analyzed this behavior yesterday while testing the intel_fb implementation for corner cases on panorama dimension, capture policy, and connector configuration changes. In the concrete case, I wanted to ensure that intel_fb clears stale pixels if its view port is shrunken due to capture-policy changes in nitpicker. My current work-in-progress adds a call to screen_size_changed() in Nitpickers Capture_session::apply_policy() for this purpose, which is debatable but a discussion starting point.

jschlatow commented 1 week ago

These tools ideally require access to the panorama geometry in order to decide what area to capture. Thus, I'm wondering whether the Capture session should be extended by a panorama_size() method.

On a second thought, an alternative could be to add a helper component that uses the GUI session to determine the panorama size and that provides the capture geometry to the screenshot component. Although this solution is more complex than equipping the Capture session with a way to return the panorama size, it is more flexible. For instance, the helper component could be replaced by an interactive version that allows the user to select what area to capture.

nfeske commented 3 days ago

Commit https://github.com/genodelabs/genode/commit/6ce26bc59b7e099971800d9b3653871e74f5d02f restores the original behavior of the screen_size function.

That said, while reworking the capture session, I was tempted to drop the screen_size query feature but kept it for now to avoid disrupting clients like @jschlatow's screen-capturing apps. Well, I disrupted them anyway. ;-)

add a helper component that uses the GUI session to determine the panorama size and that provides the capture geometry to the screenshot component

I'd even argue that such applications could request a GUI session to obtain all the information of interest (or alternatively rely on configured values). I think there is no harm granting, e.g., a screenshot app, both a capture session and a GUI session routed to nitpicker directly. A dedicated component wouldn't bring much benefit, or would it?

jschlatow commented 3 days ago

Thanks! I haven't tested the fix but it looks good to me.

A dedicated component would bring the benefit of interchangeability, e.g. replacing it with a user-interactive version.

jschlatow commented 2 days ago

@nfeske I've now published the screenshot archives on my depot and index. The archive version is 2024-10-09. I noticed that the tool now causes a short screen flickering, when a screenshot is taken. Do you have an explanation for this? Note, I merely cherry-picked 6ce26bc, so maybe I missed another commit.

nfeske commented 2 days ago

Do you have an explanation for this?

I have no idea. Does the screenshot utility open and close a capture session for each screenshot?

jschlatow commented 1 day ago

Do you have an explanation for this?

I have no idea. Does the screenshot utility open and close a capture session for each screenshot?

Yes, it does. I changed this so that the screenshot utility opens a capture session on startup and a Capture::Connection::Screen when a screenshot is taken. This removes the flickering when a screenshot is taken. However, on startup the image displayed on screen appears clipped on the left side for a one or two seconds. I've published the updated screenshot archive 2024-10-24 on my jschlatow_test depot.