-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
vsyncthread mode for qopenglwindow frameswapped driven phase locked loop #12469
Conversation
Where does that 99.90 come from? And how does it look? If you uncomment the logging of the delta in the phase locked loop, don't you see it settle on a value. BTW, on Windows there was no problem with the animation to start with, was there? IIRC we use setSwapInterval(0) on windows. Can you try with setSwapInterval(1) ? (The dropped frames was not correct btw, fixing now) |
works on linux -- hard to tell if it's better because waveforms are generally smooth already. |
On Windows there were always stucking Waveforms, but for some seconds they can run smooth with 60fps in VSync. My guess is that the Qt event queue is blocked and the signal/slot connection for the VSync isn't delivered in time. I can measure this for scrolling the library (using #12094) and I found #12445 as root cause for some other stucking using the profiler - but there are also cases where I couldn't identify the cause yet - this might be a slowly increasing phase shift. But with this PR it's a different behavior. Here the vsyncthread seems to be free wheeling - I guess the 100fps are real, but out of VSync. If I set setSwapInterval to 1 the PLL seems to lock somehow, but not to 60fps: With
With
|
src/waveform/vsyncthread.cpp
Outdated
int signalled = 0; | ||
// Signal to render and swap the gl widgets (waveforms, spinnies, vumeters) | ||
// This works best when offset with a delay wrt the vsync time. | ||
const int offset = 8000; // Found by trial and error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I set this to 12000 and it looks much better on Windows (with setSwapInterval=1). I would say better than before, but not perfect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, so with setSwapInterval(0) you get 160 fps from the phase locked loop.
When you say better than before, do you mean better than with this PR with setSwapInterval(0), or better than without this PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant, this PR + setSwapInterval(1) + offset=12000 was slightly better than 2.4. All other combinations were worse.
I haven't tried your last commit.
I tested on my 2015 MacBook. It runs smooth there too, though not as smooth as on my M1: Disruptions (like scrolling the library, resizing, hide/show) have a more noticable effect. (That is not surprising as the M1 is much faster). Now, compared to ST_TIMER, after a disruption it seems to take a bit longer to stabilize, but eventually it does. Not sure if that's because the PLL gets out of sync or because the event queue is chocking. When I have time, I'll do a bit of logging and tweaking. |
For me the Without it and ALSA @ 23 ms and 60 Hz, I have a smooth waveform with blocks of ~5 jitters every 60-90 s. The jitter effects sometimes both decks in the same way or shifts the waveform against each other. With |
Try to set setSwapInterval to 1 as on macOS. Without I see exactly the same on Windows, as you describe. |
Same for me on Linux. |
9ee83d6
to
a6e3880
Compare
The latest version works out of the box on my Windows11 system, except that I need to set mixxx/src/waveform/waveformwidgetfactory.cpp Line 1218 in 71dceb2
But I noticed that the CPU load increased significiantly (from ~33% to 50% on my laptop which 8 physical cores). Than I played further with the value of setSwapInterval, and noticed that higher values than 1 reduce the CPU load, but the lagging comes back - looks similar as with ST_TIMER mode. mixxx/src/waveform/waveformwidgetfactory.cpp Line 802 in 71dceb2
I could imagine that one reason for the lag we see in Mixxx is that swapBuffers() does not always work. Maybe this is related to the behavior described here: https://stackoverflow.com/questions/74613205/qopenglwidget-swapbuffers-happening-without-redrawing-the-scene |
No idea why using setSwapInterval(1) increases your CPU. Definitely not the case on macOS. Now about Qt triggering additional update + swap, yes, I am aware of this behaviour, and it is also the reason why I do this:
I am not sure if we can avoid Qt triggering the update on resize (so the paint and swapBuffers is exclusively done through the vsyncthread). Since it only happens when resizing, I don't think it's a big problem, but I agree it's not ideal. I remember looking into this and getting stuck, but I can give it another go. |
AFAIK, this is done by sending the event QEvent::UpdateRequest to QOpenGLWindow. I guess if we handle it there, it shouldn't swap automatically. |
But maybe the opposite is the right thing to do - sending QEvent::UpdateRequest instead of calling swapBuffers() ? |
No, we need swapBuffers because relying on Qt event delivery results in bad frame rate and jitter. Intercepting the QEvent::UpdateRequest event doesn't avoid the additional update/swap on resize. |
In any case, this now works smooth both on my M1 and on my 2015 MBP, so I think this is ready for merge, at least for macOS. IIUC, @JoergAtGithub , for window and linux we should also setSwapInterval(1) if ST_PLL is the selected VSync mode right? |
Yes, ST_PLL works on Windows only with reasonable framerate, if setSwapInterval is set to 1. Which somehow matchs to the Qt docu: "Setting an interval value of 0 will turn the vertical refresh syncing off, any value higher than 0 will turn the vertical syncing on." |
Do you think it's worth it to make ST_PLL usable on windows and linux? It requires setting setSwapInterval(1) only if ST_PLL is selected, and that is not trivial due to the order of initialization. |
On my fast Windows 11 laptop, it shows the smoothest waveform animation I've seen yet. Therefore I think it makes sense. |
@JoergAtGithub setSwapInterval is now set to 1 if the mode is ST_PLL (VSync 5 in the config) |
Nice!
Yes, I feel comfortable making ST_PLL the default on macOS. For 2.4.1 we can decide it to make it the default on Windows as well, and/or to expose it to the user. |
Without VSync it's like before, mostly smooth waveforms. With this (without VSync 5) and with 2.4 I see jitter and about 20 dropped frames during the first 30 seconds of a session, after that it stabilizes and there are a lot less dropped frames. Tested with 60 Hz and 4.6 (Compatibility Profile) Mesa 21.2.6 (Mesa Intel(R) UHD Graphics 620 (KBL GT2)), no sound output configured. |
This might be #12470 |
Hmm no, #12470 doesn't fix that unfortunately. |
That is not surprising, because that code didn't change at all (but still good to confirm that I didn't accidentally touch anything)
Ok, that's too bad. I'd like to investigate this a bit more at some point but now. Anyway, this doesn't change anything, we stick with ST_TIMER on Linux and Windows, ST_PLL on macOS, and people can test ST_PLL on Linux and Windows if they like. |
I tested this on my old Windows 7 Laptop, with ST_TIMER it runs with 60fps but framedrops - as before. With ST_PLL it runs with ~5.5fps, but at least it works somehow. To be metioned is, that the Mixxx process consumes only 2% of the CPU time - therefore it's not an CPU issue:
|
The code is LGTM and I can confirm that ST_PLL works smoother on my Windows11 laptop (the earlier mentionend high CPU usage, is not reproduceable anymore). |
This PR adds a VSyncMode ST_PLL to the VSyncThread.
With this mode, a phase locked loop is used to determine the vsync time and -interval based on the frameSwapped signal, that is coming from an invisible QOpenGLWindow (that of the WInitialGLWidget, which is used to query OpenGL).
This solves the problem that the phase offset between the vsync and the VSyncThread timer could effect the animation very badly and that timer could drift from the actual vsync.
In the Qt documentation we can read that using the frameSwapped signal is the preferred mechanism for animation:
Unfortunately, this doesn't play nice with our multiple QOpenGLWindow-based widgets, for which we still need to use the manual and coordinated call to swapBuffers().
Notes:
to
[Waveform]
inmixxx.cfg
Question: