Default vsyncmode #12489

m0dB · 2023-12-31T14:22:36Z

Make VSync mode 0 refer to the default mode; make ST_PLL the default on macOS, ST_TIMER otherwise
Write the mode to the config

[Waveform]
VSync 0

Users can overwrite the default in the config

[Waveform]
VSync <value>

where valid are 0 (DEFAULT), 4 (ST_FREE), 5 (ST_PLL), 6 (ST_TIMER)

JoergAtGithub · 2024-01-02T06:40:13Z

@fwcd Could you test this on macOS?

ywwg · 2024-01-03T16:45:15Z

We will need code as part of the config load to change the deprecated values to zero

m0dB · 2024-01-03T18:01:45Z

We will need code as part of the config load to change the deprecated values to zero

Not sure what you mean by that. The deprecrated values are interpreted as default.

m0dB · 2024-01-07T19:28:38Z

@ywwg I have moved the upgrade of the deprecated values to preferences/upgrade.cpp. I left the decision of converting ST_DEFAULT to either ST_PLL or ST_TIMER to VSyncThread, but maybe you prefer to move it to WaveformWidgetFactory, where the config value is read (WaveformWidgetFactory::startVSync)

ywwg

Looks good, thank you! Yes this is what I imagined for how to handle the values. I made a few small notes.

ywwg · 2024-01-08T00:33:23Z

src/preferences/upgrade.cpp

+
+VSyncThread::VSyncMode upgradeDeprecatedVSyncModes(int configVSyncMode) {
+    using VT = VSyncThread;
+    if (configVSyncMode >= 0 || configVSyncMode <= static_cast<int>(VT::ST_COUNT)) {


this is perfect thanks

ywwg · 2024-01-08T00:34:24Z

src/preferences/upgrade.cpp

+            return VT::ST_TIMER;
+        case VT::ST_PLL:
+            return VT::ST_PLL;
+        case VT::ST_COUNT:


let's remove this case and then change the range check above to < instead of <=. That's a little more clear.

ywwg · 2024-01-08T00:34:35Z

src/waveform/vsyncthread.cpp

@@ -4,13 +4,22 @@
 #include "util/math.h"
 #include "util/performancetimer.h"

-VSyncThread::VSyncThread(QObject* pParent)
+namespace {
+VSyncThread::VSyncMode defaultVSyncMode() {


ywwg · 2024-01-08T00:35:44Z

src/waveform/vsyncthread.h

-        ST_PLL,
-        ST_COUNT // Dummy Type at last, counting possible types
+        ST_DEFAULT = 0,
+        ST_MESA_VBLANK_MODE_1_DEPRECATED, // 1


seems unnecessary to say what numbers the enums correspond to.

fwcd · 2024-01-08T23:20:27Z

Haven't dug into the details yet, just my observation from quickly test-driving this with the default configuration: While this mostly works well, I also get considerably more frame drops with this build (testing on an M1 Mac):

Screen.Recording.2024-01-08.at.23.14.34.mov

For comparison, here's 2.4, which gives me mostly consistent 60 fps:

Screen.Recording.2024-01-08.at.23.30.57.mov

Same goes for main:

Screen.Recording.2024-01-08.at.23.13.57.mov

fwcd · 2024-01-08T23:41:14Z

Perhaps it would make sense to defer this for now, i.e. remove it from the 2.4.0 milestone, to make sure that we have enough time to test this new mode on macOS.

m0dB · 2024-01-09T07:47:30Z

I am not against that, as in keeping the default ST_TIMER and documenting ST_PLL, but I would like to understand why you are seeing different results than me on an M1.

For me with ST_TIMER having good or bad framerate varies per execution, with ST_PLL it is always stable once the PLL is (which is pretty fast).

Are you using fixed framerate or ProMotion in your macOS display settings? Are you using the internal (laptop) or an external monitor? If you turn on logging, do you see the PLL settling around 16667 ms?

ywwg · 2024-01-09T14:35:03Z

I think we can give this another few rounds of testing, but if indeed the results are inconsistent, let's leave the default to ST_TIMER for all platforms. Now that we have a default, we can always enable PLL on mac (or elsewhere) in a minor/patch release

daschuer · 2024-01-12T09:24:19Z

I will move it to 2.4.1 for now, because it can be considered a bug fix.

fwcd · 2024-01-12T13:57:25Z

Are you using fixed framerate or ProMotion in your macOS display settings? Are you using the internal (laptop) or an external monitor?

This is using the built-in ProMotion laptop display with variable frame rate.

If you turn on logging, do you see the PLL settling around 16667 ms?

I'll have to investigate that, you mean the regular debug logging, right?

m0dB · 2024-01-19T23:44:26Z

Are you using fixed framerate or ProMotion in your macOS display settings? Are you using the internal (laptop) or an external monitor?

This is using the built-in ProMotion laptop display with variable frame rate.

Can you try with a fixed framerate?

If you turn on logging, do you see the PLL settling around 16667 ms?

I'll have to investigate that, you mean the regular debug logging, right?

Yes. It will periodically log the interval resulting from the PLL

m0dB · 2024-01-20T01:26:06Z

It might be good though to merge this MR anyway, with the minor change of keeping the default ST_TIMER for all platforms. The main reason is that it will write the

[Waveform]
VSync 0

to the settings file. This will be easier and less error prone to instruct users to change the value 0 to 5 to try the PLL mode, rather than asking them to add the missing VSync key.

m0dB · 2024-01-20T02:52:09Z

Haven't dug into the details yet, just my observation from quickly test-driving this with the default configuration: While this mostly works well, I also get considerably more frame drops with this build (testing on an M1 Mac):

Hm, I just experienced lots of frame drops with this and logging the PLL showed it was not settling on 16666 microseconds. After changing the refresh rate in the macOS display settings between promotion and back to 60 Hz the problem the problem was fixed and rock solid again. Can you try if that works for you too, @fwcd ?

m0dB · 2024-01-20T13:31:29Z

@fwcd I found the problem with ProMotion.

If the user selected framerate (in mixxx settings) is lower than the actual (PLL) framerate, I run the swap at the nearest multiple interval. So, If the PLL is 60, the user selected frame rate is 30 (or in fact, between 25 and 40), I swap every 2 x 1/60 interval. The implementation to do this was wrong and this become noticeable at 120 fps.

I have a fix ready, and will create a separate PR.

daschuer · 2024-01-20T14:54:02Z

Do we really need the frame rate slider in case of the PLL?

m0dB · 2024-01-20T18:15:48Z

No, and I'd be happy to remove the logic to deal with the frame rate value. Better keep it simple.

In the meantime I have come to the conclusion that with ProMotion driving the continuous swapped->update iteration that triggers the PLL (at 120 fps) I keep getting more frame drops (at 60 fps) than when configuring the display at fixed frame rate. I will continue working on this but in the meantime the conclusion is: to use PLL, configure the display at fixed framerate. @fwcd can you test that?

fwcd · 2024-01-20T18:20:54Z

I'll test-drive it once I find some time.

in the meantime the conclusion is: to use PLL, configure the display at fixed framerate

In any case we should make sure that the default vsync mode works well, regardless of whether the user uses ProMotion or has set the display to a fixed framerate.

m0dB · 2024-01-20T23:05:30Z

Yes, I have it working now for both. Just need to do some final cleanup.

m0dB · 2024-01-21T17:41:56Z

Can you please check #12611 ?

daschuer · 2024-01-21T20:41:33Z

Oh sorry, I have just realized that I have pushed your changes to 2.4.
Shall we revert that commit? Probably not. What do you think?

m0dB · 2024-01-25T14:27:38Z

No, I don't see a need to revert it.

…e, TIMER otherwise, upgrade deprecated modes

m0dB · 2024-01-28T11:55:01Z

@fwcd has confirmed that now that #12611 has been merged to 2.4, ST_PLL behaves smoothly (and better than ST_TIMER) on macOS, so I would like to make a last minute attempt to get this PR merged for the release. I have rebased it on latest 2.4.

JoergAtGithub · 2024-01-28T12:34:26Z

I tested this on Windows7 and Windows11. The configuration behavior is as intended. Code looking good to me and was before approved by Owen.
If I overwrite the default setting on my Windows7 laptop with Intel GPU with VSync 5 the framerate drops from 60fps to 5.5fps, while it works well on my Windows11 laptop with AMD GPU.
Since the dafault is only changed for macOS very the hardware variance is low, and two developers tested this successful on various macOS hardware, lets merge this!
Thank you!

m0dB · 2024-01-28T14:07:01Z

Much appreciated! I will edit the wiki page and the blogpost accordingly (mentioning that ST_PLL is now the default on macOS, and how to select ST_TIMER or ST_PLL explicitly)

Out of curiosity, when you say "works very well on windows11", is that better than ST_TIMER or the same? (i.e. when playing, but also when scrolling the library).

github-actions bot added the ui label Dec 31, 2023

m0dB force-pushed the default-vsyncmode branch from c6b4c32 to a0a78d4 Compare January 2, 2024 00:48

m0dB force-pushed the default-vsyncmode branch from a0a78d4 to e8a41d5 Compare January 3, 2024 23:00

m0dB added this to the 2.4.0 milestone Jan 3, 2024

github-actions bot added waveform preferences labels Jan 3, 2024

ywwg approved these changes Jan 8, 2024

View reviewed changes

daschuer modified the milestones: 2.4.0, 2.4.1 Jan 12, 2024

default vsync mode when not given (or invalid) in config: PLL on appl…

f998eaa

…e, TIMER otherwise, upgrade deprecated modes

m0dB force-pushed the default-vsyncmode branch from c64e793 to f998eaa Compare January 28, 2024 11:52

JoergAtGithub merged commit d801637 into mixxxdj:2.4 Jan 28, 2024
14 checks passed

JoergAtGithub modified the milestones: 2.4.1, 2.4.0 Jan 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default vsyncmode #12489

Default vsyncmode #12489

m0dB commented Dec 31, 2023 •

edited

Loading

JoergAtGithub commented Jan 2, 2024

ywwg commented Jan 3, 2024

m0dB commented Jan 3, 2024

m0dB commented Jan 7, 2024

ywwg left a comment

ywwg Jan 8, 2024

ywwg Jan 8, 2024

ywwg Jan 8, 2024

ywwg Jan 8, 2024

fwcd commented Jan 8, 2024 •

edited

Loading

fwcd commented Jan 8, 2024

m0dB commented Jan 9, 2024

ywwg commented Jan 9, 2024

daschuer commented Jan 12, 2024

fwcd commented Jan 12, 2024

m0dB commented Jan 19, 2024

m0dB commented Jan 20, 2024

m0dB commented Jan 20, 2024

m0dB commented Jan 20, 2024

daschuer commented Jan 20, 2024

m0dB commented Jan 20, 2024

fwcd commented Jan 20, 2024

m0dB commented Jan 20, 2024

m0dB commented Jan 21, 2024

daschuer commented Jan 21, 2024

m0dB commented Jan 25, 2024

m0dB commented Jan 28, 2024 •

edited

Loading

JoergAtGithub commented Jan 28, 2024

m0dB commented Jan 28, 2024

Default vsyncmode #12489

Default vsyncmode #12489

Conversation

m0dB commented Dec 31, 2023 • edited Loading

JoergAtGithub commented Jan 2, 2024

ywwg commented Jan 3, 2024

m0dB commented Jan 3, 2024

m0dB commented Jan 7, 2024

ywwg left a comment

Choose a reason for hiding this comment

ywwg Jan 8, 2024

Choose a reason for hiding this comment

ywwg Jan 8, 2024

Choose a reason for hiding this comment

ywwg Jan 8, 2024

Choose a reason for hiding this comment

ywwg Jan 8, 2024

Choose a reason for hiding this comment

fwcd commented Jan 8, 2024 • edited Loading

fwcd commented Jan 8, 2024

m0dB commented Jan 9, 2024

ywwg commented Jan 9, 2024

daschuer commented Jan 12, 2024

fwcd commented Jan 12, 2024

m0dB commented Jan 19, 2024

m0dB commented Jan 20, 2024

m0dB commented Jan 20, 2024

m0dB commented Jan 20, 2024

daschuer commented Jan 20, 2024

m0dB commented Jan 20, 2024

fwcd commented Jan 20, 2024

m0dB commented Jan 20, 2024

m0dB commented Jan 21, 2024

daschuer commented Jan 21, 2024

m0dB commented Jan 25, 2024

m0dB commented Jan 28, 2024 • edited Loading

JoergAtGithub commented Jan 28, 2024

m0dB commented Jan 28, 2024

m0dB commented Dec 31, 2023 •

edited

Loading

fwcd commented Jan 8, 2024 •

edited

Loading

m0dB commented Jan 28, 2024 •

edited

Loading