Control how many frames are captured per second

[zsolt-donca]

Addresses #66 by introducing an optional command-line argument to limit the number of frames captured per second, as the mouse lag issue becomes less noticeable if the frame rate is smaller. The argument represents the targeted frames-per-second value (e.g. pass -f 10 for 10 FPS).

The implementation took inspiration from https://github.com/any1/wayvnc, but no code was copied other than the file include/time-util.h.

The actual FPS value can be lower than the targeted one because xdg-desktop-portal-wlr uses the zwlr_screencopy_frame_v1_copy_with_damage function (emphasis on with_damage), which waits until the screen changes.

The implementation differs from that of wayvnc that it does not try to "smooth" the capture rate over time, and it always uses the capture time of the last frame as its best estimate for how much time to sleep for current frame, and in order to reach the targeted FPS value. Let me know if there is interest in implementing this - for one, I am not sure if its worth it, and especially on possible implications with regards to xdp only capturing the frame on damage, which can make the measured "time to capture" value vary significantly, and could impact the smoothing negatively (which expects the values to be predictable).

Another difference when compared to wayvnc's implementation is that this code simply suspends the current thread with usleep, while wayvnc uses a more sophisticated timer-based solution based on the author's own library https://github.com/any1/aml.

With regards to testing, it seems to (approximately) produce the given targeted FPS value for me, at least as long as I keep changing the screen contents (by moving the mouse, for example). For example, when running with xdg-desktop-portal-wlr -l DEBUG -f 10, it produced these logs for me:

2020/12/25 18:55:24 [DEBUG] - fps_limit: average FPS in the last 5 seconds: 10.04
2020/12/25 18:55:29 [DEBUG] - fps_limit: average FPS in the last 5 seconds: 9.97
2020/12/25 18:55:34 [DEBUG] - fps_limit: average FPS in the last 5 seconds: 10.07
2020/12/25 18:55:39 [DEBUG] - fps_limit: average FPS in the last 5 seconds: 9.96
2020/12/25 18:55:45 [DEBUG] - fps_limit: average FPS in the last 5 seconds: 10.02

Another example with -f 20:

2020/12/25 19:01:30 [DEBUG] - fps_limit: average FPS in the last 5 seconds: 19.57
2020/12/25 19:01:35 [DEBUG] - fps_limit: average FPS in the last 5 seconds: 19.94
2020/12/25 19:01:40 [DEBUG] - fps_limit: average FPS in the last 5 seconds: 19.93
2020/12/25 19:01:45 [DEBUG] - fps_limit: average FPS in the last 5 seconds: 19.92

[maximbaz]

I can confirm that this PR helps a lot on 4k screen, nice job 👍

For reference, if I run with -l DEBUG -f 60 or -l DEBUG -f 10000, I can see that my frame rate does not exceed 30, so I assume this is the natural rate, the maximum that I can achieve.

I can only feel that the mouse lag is almost gone at the value -f 15, anything higher already feels bad.

Just curious, @zsolt-donca what value did you end up using?

[zsolt-donca]

@maximbaz I'm glad it works for you as well!

I ended up using -f 8, as that's about the same frame rate that my (company's) favorite video-conferencing application limits to as well (http://8x8.vc/). With this frame rate, I don't feel any mouse lag at all. This is definitely not the highest value I can go without a noticable mouse lag, it's just that anything higher than this is a waste of resources in my case.

EDIT: I just tested with -f 15 as well, and it works fine as well. Maybe also with -f 20, but I am not sure.

[danshick]

This looks good to me for the most part. I'll test this PR soon.

[zsolt-donca]

I just realized that in certain places I am not respecting the contribution guide on line length:

Try to keep your lines under 80 columns, but you can go up to 100 if it improves readability. Don't break lines indiscriminately, try to find nice breaking points so your code is easy to read.

I will soon update my PR to fix this.

[danshick]

BTW, your FreeBSD builds are failing, which we need to resolve before we can merge this.

Mako fixes your issues with libepoll-shim on FreeBSD.

You need to do this in the meson.build:

https://github.com/emersion/mako/blob/dbd9c2bc98ef0cec7801a049f4bf8d61f7be36d0/meson.build#L32-L36

And you need to edit the .builds/freebsd.yml to include this:

https://github.com/emersion/mako/blob/dbd9c2bc98ef0cec7801a049f4bf8d61f7be36d0/.builds/freebsd.yml#L5

[zsolt-donca]

@danshick sorry about that, I wasn't paying attention to the automated builds linked in here. Thanks for info on how to fix it as well.

[emersion]

Overall the logic looks good to me. Here are some comments to improve readability and style.

[emersion]

Also needs a rebase on top of master.

[columbarius]

When you rebase it, could you please add fps_limit to screencast_conf and the config file? I think that could be useful.
@emersion @danshick Any objections?

[emersion]

Yup looks like a good idea to me

[zsolt-donca]

@emersion I think I addressed all your comments, thank you for the review. Please note that some questions are still open.

@emersion @danshick Will come back soon with a rebase on top of the current master, and with support for the fps limit in the newly introduced config file.

[danshick]

Thanks for all of your work on this and your patience with us through these reviews and redesigns.

[zsolt-donca]

I rebased the entire branch on the current master. Introduced the key screencast:max_fps in the config file; for example:

[screencast]
output_name=DP-6
max_fps=8

Note that the sample config had an error and was using ouput instead of output_name. Fixed it in this commit: 9acf4eb

Also note that I could not test the log statements and, in particular, the fps measurement after doing this rebase because of #91 . Will retest everything once that's fixed.

[danshick]

@zsolt-donca You can comment out

xdg-desktop-portal-wlr/src/core/config.c

Line 62 in 07154bb

logprint(INFO, "config: using config file %s", *configfile);

for testing, but we do need to resolve that issue regardless.

[zsolt-donca]

I rebased this branch once again on the latest master. I also changed a defensive check with an actual assertion as suggested by @emersion .

I also found a minor issue with how the fps measurements were logged, as it was printing the elapsed time threshold instead of the actual elapsed time (f6c44ca). I think the timing varies also because we are capturing a frame only if there is damage. Sample log with the new code:

2021/03/05 09:42:32 [DEBUG] - fps_limit: average FPS in the last 5.44 seconds: 2.02
2021/03/05 09:42:37 [DEBUG] - fps_limit: average FPS in the last 5.01 seconds: 2.00
2021/03/05 09:42:43 [DEBUG] - fps_limit: average FPS in the last 5.50 seconds: 2.00
2021/03/05 09:42:48 [DEBUG] - fps_limit: average FPS in the last 5.02 seconds: 1.99
2021/03/05 09:42:53 [DEBUG] - fps_limit: average FPS in the last 5.51 seconds: 2.00
2021/03/05 09:42:59 [DEBUG] - fps_limit: average FPS in the last 5.49 seconds: 2.00

I also retested all functionality that I could think of, including the new ones with the config file (e.g. the CLI argument takes precedence over the config file, if both are missing then there is no limit).

This should now addres all feedback that I received so far. What should be the next step? Should I squash all my commits into a single one so that it can be merged?

[danshick]

I think the timing varies also because we are capturing a frame only if there is damage.

Yup, exactly what I'd expect.

I also retested all functionality...

Thanks!

This should now addres all feedback that I received so far. What should be the next step? Should I squash all my commits into a single one so that it can be merged?

Yeah, normally at this point, I'd do an interactive rebase to try to reduce the overall number of commits. We tend towards a rebase and merge strategy, and for a reasonably big PR, it is okay if it comes over as more than one commit.

I'd want @emersion to take a final look and I'll start running your PR as a daily driver for a couple of days to make sure I don't bump into any major bugs, but I think this should be approved and merged shortly.

Edit: I've installed your branch as my system version of xdpw. So far so good.

[emersion]

Code LGTM, thanks!

@zsolt-donca, can you rebase and split your work into logical commits?

@danshick, if testing goes well, feel free to merge!

[zsolt-donca]

@emersion thanks again for the review!

Are you guys okay with me squashing the whole thing into a single commit for this pull request? I can't reasonably think of a better way, especially now that the master branch has support for config files.

Just a small question: what should I set in config.sample to max_fps? Especially now that we are considering installing it as per #92, we should probably put the default values, which for max_fps would be 0, meaning no limit.

[emersion]

Are you guys okay with me squashing the whole thing into a single commit for this pull request?

OK. There are a few unrelated style changes, but it's no big deal.

Just a small question: what should I set in config.sample to max_fps?

We can always adjust it if/when we decide to ship it.

[danshick]

@zsolt-donca Lemme know when you've done any last minute cleanup/squashing and I'll merge this. I haven't seen any crashing or issues over the past couple of days.

[zsolt-donca]

@danshick

Lemme know when you've done any last minute cleanup/squashing...

I just squashed the whole thing into a single commit. This should be it from me. I'd appreciate if you could give feedback on the commit message, in particular if it is okay with regards to the contributing guide.

[emersion]

Ah, thanks for taking the time to write a good commit message, greatly appreciated! Yes, it looks good to me.

Since @danshick gave their greenlight, let's just merge this :)

Thanks a lot!

[zsolt-donca]

@emersion @danshick I would also like to thank you for your guidance and meticulous reviews! It was fun working on this, I enjoyed it. 😄

[danshick]

It was fun working on this, I enjoyed it. 😄

We're happy to have your contributions!