2 frames 2 pacing #1156

yshui · 2023-12-18T22:14:07Z

Another batch of frame pacing improvements. Hopefully it's acceptable for general use now.

codecov · 2023-12-19T09:56:41Z

Codecov Report

Attention: 402 lines in your changes are missing coverage. Please review.

Comparison is base (e76cf43) 37.41% compared to head (d8f3037) 36.68%.
Report is 4 commits behind head on next.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #1156      +/-   ##
==========================================
- Coverage   37.41%   36.68%   -0.73%     
==========================================
  Files          49       50       +1     
  Lines       11197    11489     +292     
==========================================
+ Hits         4189     4215      +26     
- Misses       7008     7274     +266

Files	Coverage Δ
src/backend/driver.h	`60.00% <ø> (ø)`
src/common.h	`75.00% <ø> (ø)`
src/config.h	`23.52% <ø> (ø)`
src/event.c	`69.72% <ø> (ø)`
src/x.h	`87.50% <ø> (ø)`
src/statistics.c	`32.43% <0.00%> (+4.52%)`	⬆️
src/backend/backend.c	`62.12% <66.66%> (+1.20%)`	⬆️
src/utils.h	`29.41% <50.00%> (ø)`
src/backend/driver.c	`31.11% <0.00%> (-3.04%)`	⬇️
src/x.c	`44.54% <0.00%> (-1.62%)`	⬇️
... and 3 more

... and 1 file with indirect coverage changes

FT-Labs · 2023-12-19T10:28:28Z

@yshui

Hi,

I'm not much aware of the frame pacing topic but when enabled with animations (glx, xrender) everything seems to be messed up. After opening several windows any window movement/resize becomes too laggy and crashes everything eventually (all windows disappear or start making awkward movements).

Can you guide me a little bit about frame pacing? I'm not sure if it should be included with animations though. Also my fork is not like other forks, it is 100% original picom (this one) with animations included in it. I also maintain all of your commits, so that it is not behind of the original.

	if ((ps->o.legacy_backends || ps->o.animations || ps->o.benchmark || !ps->backend_data->ops->last_render_time) &&
	    ps->frame_pacing) {
		// Disable frame pacing if we are using a legacy backend or if we are in
		// benchmark mode, or if the backend doesn't report render time

Also if you have a little bit of free time I have one more question, it is not related to this issue.
Since animations (resize/window movements) trigger hundreds of events (clear window flags/set window flags/add damage etc) in a second I think this increases CPU usage and decreases performance a bit; since if animation is not used it will trigger only once per one event. Or for example, picom spams logs because it tries to clear flags of a destroyed window (it fades out and triggers a custom animation) but window is unmapped/destroyed from X. Previously I just blocked sending logs if the window animation is still in process but it is not an ideal solution, since it only blocks sending log messages, I wonder if there is any way to do it without those functions being called?

yshui · 2023-12-19T11:15:19Z

@FT-Labs can you try merging this branch with your animation branch?

yshui · 2023-12-19T11:16:39Z

@FT-Labs btw, what's the current status on animation? judging from the PRs there are multiple efforts on animation, and i don't know how they are related to each other.

FT-Labs · 2023-12-19T17:56:27Z

@yshui

I've merged the code with this branch and it fixed all performance issues. There is no performance issue with frame pacing enabled now.

https://github.com/FT-Labs/picom/tree/pacing-fixes

It is close to done actually, I do not have that much idea about other forks. As a start, I have used @dccsillag's fork 1 1-5 years ago, but a lot has changed since it did not get updated. I just try to keep in line with original picom without breaking the animations.

To sum up, here is what works now:
Shadows without glitches
Smooth animations without using too much cpu/memory
Fade out destroyed/unmapped windows
~10 different animations
Open/close window animations
Workspace switch animations
Works with custom shaders
No flickering on any transparent window (focused/not focused) when resizing/moving

Remaining problems:
Make workspace switch animation behave correctly for any WM/DE

This is troublesome because workspace/tag/desktops are dependent on developers, and there is no strict rule. However, the preferred way is set all of NET_WM_DESKTOP of all windows then change NET_CURRENT_DESKTOP atom of root window. The issue is while awesomewm or xmonad follows this rule but other wm like dwm have different implementations
Sometimes workspace switch animation can go towards center of root viewport instead of going to the center of current monitor. This issue only occurs on multiple monitor setups. Like minimize animation shrinks all of the visible windows to the center of monitor.
Some people reported glitches with some layouts in bspwm, most of them are solved with disabling pacing before this branch.

Aside from these it is done and works good. Of course, there could be more improvements but I think they can be done later since they would classify as features

I mistakenly assumed that PresentCompleteNotify event signifies the end of a vblank (or the start of scanout). But actually this event can in theory in sent at any point during a vblank, with its timestamp pointing to when the end of vblank is. (that's why we often find the timestamp to be in the future). Add a delay so schedule_render is actually called at the end of vblank, so it doesn't mistakenly think the render is too slow to complete. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

Make it simpler to stop requesting PresentCompleteNotify when there is nothing to render. Related: #1079 Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

Previously everytime we receive a vblank event, we always request a new one. This made the logic somewhat simpler. But this generated many useless vblank events, and wasted power. We only need vblank events for two things: 1. after we rendered a frame, we need to know when it has been displayed on the screen. 2. estimating the refresh rate. This commit makes sure we only request vblank events when it's actually needed. Fixes #1079 Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

We unredirect because we receive bad vblank events, and also vblank events at a different interval compared to when the screen is on. But it is enough to just not record the vblank interval statistics when the screen is off. Although, unredirecting when display is off can also fix the problem where use-damage causes the screen to flicker when the display is turned off then back on. So we need something else for that. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

This is where we keep temporary, short living, private debug options. Adding and removing command line and config file options are troublesome, and we don't want people adding these to their config files. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

Factored out vblank event generation, add abstraction over how vblank events are generated. The goal is so we can request vblank events in different ways based on the driver we are running on. Tried to simplify the frame scheduling logic, we will see if I succeeded or not. Also, the logic to exclude vblank events for vblank interval estimation when the screen off is dropped. It's too hard to get right, we need to find something robust. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

Disable timing estimation based pacing by default, as it might not work well across drivers, and might have subtle bugs. You can try setting `PICOM_DEBUG=smart_frame_pacing` if you want to try it out. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

Also, vblank event callback should call schedule_render to queue renders instead of starting the draw timer directly, so that the CPU time calculation will be correct. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

present_notify_msc with divisor == 0 has undocumented special meaning, it means async present notify, which means we could receive MSC notifications from the past. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

I noticed sometimes full frame rate video is rendered at half frame rate sometimes. That is because the damage notify is sent very close to vblank, and since we request vblank events when we get the damage, we miss the vblank event immediately after, despite the damage happening before the vblank. request next ...... next next damage vblank vblank vblank | | | ...... | v v v v ---------------------->>>>>>--------- `request vblank` is triggered by `damage`, but because it's too close to `next vblank`, that vblank is missed despite we requested before it happening, and we only get `next next vblank`. The result is we will drop every other frame. The solution in this commit is that we will keep requesting vblank events, right after we received a vblank event, even when nobody is asking for them. We would do that for a set number of vblanks before stopping (currently 4). Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

Right now we rely on `reschedule_render_at_vblank` being scheduled for the render finish check. Which means we will only know if a frame has finished rendering the next time we queue a render. And we only check at vblank boundary, which means when a render is queued we have to wait for the next vblank, when had we checked earlier, we will be able to start rendering immediately Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

It's kind of dumb anyway. If we get damage event right after a vblank event, we would waste the whole vblank. Instead improve the frame scheduling logic to target the right vblank interval. This only affects smart_frame_pacing anyway. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

Present extension based scheduler doesn't work well on NVIDIA drivers. GLX_SGI_video_sync is less accurate, but is rumoured to work. See [kwin's usage](https://invent.kde.org/plasma/kwin/-/blob/master/src/ backends/x11/standalone/x11_standalone_sgivideosyncvsyncmonitor.cpp) Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

Useful for debugging. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

We are not using it for anything at the moment, and it is breaking CI. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

yshui · 2023-12-19T23:52:19Z

@FT-Labs cool. after 11 is out of the door, the focus for 12 would be getting animation over the finishing line

Stebalien · 2023-12-19T23:59:25Z

I'm no longer noticing any power draw differences introduced by frame pacing when running this branch, but I've also switched from Intel to AMD so I don't get quite the same level of power state information 🤷.

FT-Labs · 2023-12-20T19:34:37Z

@yshui sorry if it is a stupid question, what is 11 and 12 :D

absolutelynothelix · 2023-12-20T19:36:42Z

@FT-Labs, picom v11 and v12 respectively.

FT-Labs · 2023-12-22T23:06:13Z

Ok, got it

morgoth6 · 2023-12-23T10:48:49Z

It seems if smart_frame_pacing is enabled, picom log-spam huge amount of warnings:

PresentCompleteNotify msc is going backwards, last_msc: 767522245 , current msc: 0

As soon as for display turns off (DPMS) In my case my picom.log grow to 2GB+ (and eats quite significant portion of my battera at the same time)

Maybe it would be good idea to utilize DPMSInfoNotify ? It should be working now (#134) and for older systems it would be no-op (?)

src/picom.c

yshui · 2023-12-25T15:08:37Z

Wow, DPMSInfoNotify is merged?! Never thought I'd see this day 😆

Otherwise we might be repeatedly hitting this condition and spam the warning. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

yshui · 2024-01-14T15:46:20Z

Things seem to be going well, so I am merging this.

yshui force-pushed the pacing-fixes branch 2 times, most recently from 292c116 to 6603c1b Compare December 18, 2023 22:34

yshui force-pushed the pacing-fixes branch 2 times, most recently from ff691e6 to ecedd55 Compare December 19, 2023 10:04

yshui force-pushed the pacing-fixes branch 3 times, most recently from b1f1c5f to c9840c1 Compare December 19, 2023 10:58

yshui added 17 commits December 19, 2023 22:36

core: simplify the pacing logic a little bit

827380e

Make it simpler to stop requesting PresentCompleteNotify when there is nothing to render. Related: #1079 Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

core: simplify pacing logic a bit more

3838b45

Also, vblank event callback should call schedule_render to queue renders instead of starting the draw timer directly, so that the CPU time calculation will be correct. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

x: fix x_request_vblank_event

25d16b0

present_notify_msc with divisor == 0 has undocumented special meaning, it means async present notify, which means we could receive MSC notifications from the past. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

core: add debug options to override the vblank scheduler

b582d29

Useful for debugging. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

driver: choose sgi_video_sync scheduler for NVIDIA

a28e221

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

core: make missing dpms extension non-fatal

6e0bad0

We are not using it for anything at the moment, and it is breaking CI. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

core: disable frame pacing when vsync is disabled

359d004

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

yshui force-pushed the pacing-fixes branch from a5b2ceb to 359d004 Compare December 19, 2023 23:27

morgoth6 reviewed Dec 23, 2023

View reviewed changes

src/picom.c Show resolved Hide resolved

core: reset msc counter if it went backwards

d8f3037

Otherwise we might be repeatedly hitting this condition and spam the warning. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

yshui merged commit 148e61a into next Jan 14, 2024
16 checks passed

yshui deleted the pacing-fixes branch January 14, 2024 15:46

yshui mentioned this pull request Feb 7, 2024

Improve frame timing (a.k.a. reduce input lag) #147

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2 frames 2 pacing #1156

2 frames 2 pacing #1156

yshui commented Dec 18, 2023 •

edited

Loading

codecov bot commented Dec 19, 2023 •

edited

Loading

FT-Labs commented Dec 19, 2023

yshui commented Dec 19, 2023

yshui commented Dec 19, 2023

FT-Labs commented Dec 19, 2023

yshui commented Dec 19, 2023 •

edited

Loading

Stebalien commented Dec 19, 2023

FT-Labs commented Dec 20, 2023

absolutelynothelix commented Dec 20, 2023

FT-Labs commented Dec 22, 2023

morgoth6 commented Dec 23, 2023

yshui commented Dec 25, 2023

yshui commented Jan 14, 2024

2 frames 2 pacing #1156

2 frames 2 pacing #1156

Conversation

yshui commented Dec 18, 2023 • edited Loading

codecov bot commented Dec 19, 2023 • edited Loading

Codecov Report

FT-Labs commented Dec 19, 2023

yshui commented Dec 19, 2023

yshui commented Dec 19, 2023

FT-Labs commented Dec 19, 2023

yshui commented Dec 19, 2023 • edited Loading

Stebalien commented Dec 19, 2023

FT-Labs commented Dec 20, 2023

absolutelynothelix commented Dec 20, 2023

FT-Labs commented Dec 22, 2023

morgoth6 commented Dec 23, 2023

yshui commented Dec 25, 2023

yshui commented Jan 14, 2024

yshui commented Dec 18, 2023 •

edited

Loading

codecov bot commented Dec 19, 2023 •

edited

Loading

yshui commented Dec 19, 2023 •

edited

Loading