Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Repaint scheduling #4588

Merged
merged 7 commits into from Nov 17, 2019
Merged

Repaint scheduling #4588

merged 7 commits into from Nov 17, 2019

Conversation

@YaLTeR
Copy link
Contributor

YaLTeR commented Sep 25, 2019

Requires swaywm/wlroots#1832.

This PR establishes the necessary infrastructure to delay output renders and frame callbacks to reduce the visual latency between a surface committing new contents and the contents being shown on-screen. Additionally, it adds configurable constant delays to individual output renders, and to individual view frame callbacks. The way the code is structured makes it possible to subsequently add some kind of an adaptive mode which figures out the delays automatically for lowest possible latency with minimal dropped frames.

For more details on how this works see how it's done in Weston: this PR does pretty much the exact same thing, except it also adds frame callback delaying. Also see swaywm/wlroots#655 (comment) for my comments after implementing a proof-of-concept of this in rootston.

Currently undergoing dogfooding, thus far seems to work well. I have a test client which renders on frame callbacks and prints the delay from receiving a frame callback and the subsequent presentation time. Using this client, I was able to monitor a drop in latency from a little less than 2 frames on the default configuration to about half a frame on two different setups.

How to set up the constants:

  1. output max_render_time. Run something animating and full-screen, like glxgears (as I noted in swaywm/wlroots#655 (comment), the damaged region affects the commit time). Start with 1, then go up if you see frame drops. On my laptop with an integrated Intel graphics card this works out to be about 56 ms (Weston went with 7 by default). My desktop with an AMD GPU can easily do 1 (the lowest value).
  2. View max_render_time. This one will vary per application (since every application obviously takes a different time to render). Run the application full-screen, start with 1, increase until it stops dropping frames. My test client runs well at 2, and a game I'm playing runs well at 3. I haven't tried adjusting this for anything else because I don't really need those extra couple of milliseconds of reduced latency for anything else.
@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Sep 25, 2019

How to set up the constants

This should probably make it into the man pages

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Sep 26, 2019

Needs updating per #4584

Did some testing, seems to work pretty well. Nice work!

@YaLTeR YaLTeR force-pushed the YaLTeR:repaint-delay-2 branch from 3faf5b1 to d58f92c Sep 26, 2019
@YaLTeR

This comment has been minimized.

Copy link
Contributor Author

YaLTeR commented Sep 26, 2019

Done, although I can't get anything to scan-out directly for some reason (as indicated by absence of Scanning out fullscreen view in sway.log).

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Sep 26, 2019

I can't get anything to scan-out directly for some reason

This highly depends on the planes IN_FORMATS. You can check the format used by a client with WAYLAND_DEBUG=1 and check which formats are supported by your planes with drm_info.

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Sep 26, 2019

Needs rebasing once more.

@YaLTeR YaLTeR force-pushed the YaLTeR:repaint-delay-2 branch from d58f92c to ddbe470 Sep 26, 2019
@ddevault ddevault mentioned this pull request Oct 7, 2019
3 of 5 tasks complete
@YaLTeR YaLTeR force-pushed the YaLTeR:repaint-delay-2 branch from ddbe470 to 7716349 Oct 9, 2019
@YaLTeR

This comment has been minimized.

Copy link
Contributor Author

YaLTeR commented Oct 9, 2019

Rebased and introduced sway_surface to store frame_done_timer, which was moved from the wlroots PR.

@YaLTeR YaLTeR force-pushed the YaLTeR:repaint-delay-2 branch 2 times, most recently from 261fdad to 9924acc Oct 9, 2019
@YaLTeR

This comment has been minimized.

Copy link
Contributor Author

YaLTeR commented Oct 16, 2019

Just found a crash that can happen on output disconnect. In the beginning of output_repaint_timer_handler there's this line:

output->wlr_output->block_idle_frame = false;

and wlr_output was be NULL. Should output_repaint_timer_handler just return on output->wlr_output == NULL? Is it possible for the output itself to get de-allocated while output_repaint_timer_handler is scheduled to run?

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Oct 19, 2019

Should output_repaint_timer_handler just return on output->wlr_output == NULL

Probably, yes.

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Oct 19, 2019

Future direction: is it possible to detect dropped frames and tune these numbers automatically?

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Oct 19, 2019

Been testing this branch, mpv basically hoses the whole thing. Video playback will drop frames at just about any value of max_render_time under 15, and those values basically eliminate the value-add of this feature for all other windows.

@YaLTeR

This comment has been minimized.

Copy link
Contributor Author

YaLTeR commented Oct 19, 2019

Future direction: is it possible to detect dropped frames and tune these numbers automatically?

That's what the idea was for the next step; the properties even allow for a new value like "adaptive" for this potential future behavior.

Video playback will drop frames at just about any value of max_render_time under 15, and those values basically eliminate the value-add of this feature for all other windows.

I'm assuming you're talking about the output property here? I wonder why mpv behaves weirdly with this, maybe they do their own frame delaying which is interfering or something?

I've been noticing mpv freezing video output on me lately (until I move the mouse for example), but I haven't investigated whether it was related to max_render_time. In any case it seems like a bug in mpv if it stops rendering like that. My test client that renders only by subscribing to a frame callback from within the previous frame callback is able to continue rendering fine, even if max_render_time is set to a value that's too low.

@tokyovigilante

This comment has been minimized.

Copy link
Contributor

tokyovigilante commented Oct 19, 2019

I took this for a test drive on my X1Y4 (Intel UHD 620) and only dropped frames in mpv at 7ms and above (ie 7ms output max_render_time). I’m using the native Wayland mpv video output, HW accel using vaapi and Vulkan.

@YaLTeR YaLTeR force-pushed the YaLTeR:repaint-delay-2 branch from f371da7 to f0cc215 Oct 19, 2019
@Emantor

This comment has been minimized.

Copy link
Contributor

Emantor commented Oct 19, 2019

Looking at mpv-player/mpv@ea4685b there have been some changes within the mpv wayland backend rather recently, maybe its a good idea to test with a current build from master.

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Oct 19, 2019

mpv from master is definitely an improvement, I can get it down to 8ms without noticable frame dropping. It's still the worst client I've tested yet, though.

@emersion emersion self-requested a review Oct 19, 2019
@YaLTeR

This comment has been minimized.

Copy link
Contributor Author

YaLTeR commented Oct 21, 2019

I've been noticing mpv freezing video output on me lately (until I move the mouse for example), but I haven't investigated whether it was related to max_render_time.

After watching a couple hours of videos with max_render_time switched off without issues, I can say that it's most likely related. I'll note that I'm using mpv 0.29.1 with wayland, gpu-hq and display-resample. I still believe it's not a sway issue as everything else works fine.

Copy link
Member

emersion left a comment

I like how simple this PR is. Some comments, mainly about how delays are computed.

sway/desktop/output.c Show resolved Hide resolved
sway/desktop/output.c Show resolved Hide resolved
sway/desktop/output.c Outdated Show resolved Hide resolved
@YaLTeR YaLTeR force-pushed the YaLTeR:repaint-delay-2 branch from f0cc215 to 32a14e7 Nov 16, 2019
@YaLTeR YaLTeR requested a review from emersion Nov 16, 2019
@YaLTeR

This comment has been minimized.

Copy link
Contributor Author

YaLTeR commented Nov 16, 2019

Rebased and updated as per comments. Thus far testing showed it working as intended both on the DRM backend and on the Wayland backend (although that one needs swaywm/wlroots#1916 for 100% proper testing).

@YaLTeR

This comment has been minimized.

Copy link
Contributor Author

YaLTeR commented Nov 17, 2019

(although that one needs swaywm/wlroots#1916 for 100% proper testing)

Tested using emersion's patches, everything seems to work as intended.

include/sway/config.h Outdated Show resolved Hide resolved
include/sway/tree/view.h Outdated Show resolved Hide resolved
sway/desktop/output.c Outdated Show resolved Hide resolved
include/sway/output.h Outdated Show resolved Hide resolved
@YaLTeR YaLTeR force-pushed the YaLTeR:repaint-delay-2 branch from f525263 to 36c62da Nov 17, 2019
@YaLTeR YaLTeR requested a review from emersion Nov 17, 2019
if (delay < 1) {
output_repaint_timer_handler(output);
} else {
output->wlr_output->block_idle_frame = true;

This comment has been minimized.

Copy link
@emersion

emersion Nov 17, 2019

Member

Instead of having this workaround in wlroots, I'd be more comfortable having it in Sway only, until we find a better solution. Do you think adding a sway_output.block_frame_events field and a if (output->block_frame_events) { return; } to the frame handler would be equivalent?

This comment has been minimized.

Copy link
@YaLTeR

YaLTeR Nov 17, 2019

Author Contributor

I can't say if it would be equivalent from just looking at the code, since block_idle_frame actually blocks the call to backend's schedule_frame, which contains some non-trivial code in case of DRM and commits the surface right away in case of Wayland.

This comment has been minimized.

Copy link
@emersion

emersion Nov 17, 2019

Member

I think this sequence of events happen:

  • We get a frame event. We setup our repaint timer.
  • A surface commits, wlr_output_damage schedules a frame. This sets wlr_output.frame_pending to true.
  • Our frame timer fires, but a frame is already pending.

I think moving block_idle_frame into wlr_output_damage would be better, since it's the helper scheduling frame events behind our backs. This will help reducing the complexity added to the already-complex wlr_output code.

This comment has been minimized.

Copy link
@YaLTeR

YaLTeR Nov 17, 2019

Author Contributor

I think moving block_idle_frame into wlr_output_damage would be better, since it's the helper scheduling frame events behind our backs. This will help reducing the complexity added to the already-complex wlr_output code.

I'm worried about the following sequence of events:

  1. A surface commits, a frame is scheduled using the idle timer.
  2. Immediately after we get a frame event which sets up the repaint timer.
  3. The idle frame renders and breaks it.

Or is the 1-2 transition like this impossible? As in, can there be a situation where a surface commit is directly followed by a frame event, without idle timers getting the chance to run in the meantime?

This comment has been minimized.

Copy link
@emersion

emersion Nov 17, 2019

Member

I think moving block_idle_frame into wlr_output_damage would be better, since it's the helper scheduling frame events behind our backs.

Actually, this isn't quite right. One other wlroots module calls wlr_output_schedule_frame: the screencopy implementation. wlr_output.block_idle_frame is a way for Sway to say to wlroots "don't worry, I'll send a frame shortly".

Well, let's just merge this and I'll open an issue about all of these problems. The wlr_output interface hasn't been designed with repaint scheduling in mind.

I'm worried about the following sequence of events

The 1-2 transition is impossible because if a frame event is pending (e.g. because we've swapped the output buffers) then wlr_output_schedule_frame is a no-op.

Copy link
Member

emersion left a comment

Apart from this, looks pretty good!

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Nov 17, 2019

Build failure is unrelated. Opened a PR to fix it: #4733

YaLTeR added 7 commits Sep 25, 2019
For extending wlr_surface with additional things.
It's possible for the output to be disconnected in just the right moment
for wlr_output to be NULL in the repaint handler, causing a crash. This
check fixes that crash.
@YaLTeR YaLTeR force-pushed the YaLTeR:repaint-delay-2 branch from 36c62da to daa39ff Nov 17, 2019
Copy link
Member

emersion left a comment

LGTM

@emersion emersion merged commit ba8586e into swaywm:master Nov 17, 2019
3 checks passed
3 checks passed
builds.sr.ht: alpine.yml builds.sr.ht job completed successfully
Details
builds.sr.ht: archlinux.yml builds.sr.ht job completed successfully
Details
builds.sr.ht: freebsd.yml builds.sr.ht job completed successfully
Details
@emersion

This comment has been minimized.

Copy link
Member

emersion commented Nov 17, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.