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

ControllerManager: poll every 1ms on Linux #2966

Closed
wants to merge 1 commit into from

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Jul 26, 2020

Polling at 5ms causes a noticable lag with the GUI. If the audio
buffer is set low, it also produces a lag with the audio. The
bug report ( https://bugs.launchpad.net/mixxx/+bug/990992 )
referenced for why this was increased to 5 ms on Linux is 8 years
old. The root cause of that bug was not identified, but increasing
the controller polling latency to 5ms worked around it. After
setting the polling latency back to 1ms, I can not reproduce the
bug with high CPU usage on modern Linux (Fedora 32 running kernel
5.4.39-rt23.fc32.x86_64 with realtime patch set).

Polling at 5ms causes a noticable lag with the GUI. If the audio
buffer is set low, it also produces a lag with the audio. The
bug report ( https://bugs.launchpad.net/mixxx/+bug/990992 )
referenced for why this was increased to 5 ms on Linux is 8 years
old. The root cause of that bug was not identified, but increasing
the controller polling latency to 5ms worked around it. After
setting the polling latency back to 1ms, I can not reproduce the
bug with high CPU usage on modern Linux (Fedora 32 running kernel
5.4.39-rt23.fc32.x86_64 with realtime patch set).
@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 26, 2020

Also kScratchTimerMs in ControllerEngine assumes 1ms polling.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have never understand why we need a higher polling frequency than the audio thread.

This is IMHO only useless CPU burning,
Because the values are only evaluated during the next audio thread.

We should work to fix the root cause and not rush into a already fixed bug.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 26, 2020

We need a higher polling frequency than the audio thread frequency so the GUI does not lag.

@daschuer
Copy link
Member

Please describe exactly the GUI lag and audio lag you experience.

@daschuer
Copy link
Member

At least this is nothing to be changed in a rush just before the 2.3 release.

@daschuer
Copy link
Member

The GUI is updated with 60 Hz.
That is 16 ms and we poll with 5 ms, so what is the point?

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 26, 2020

If I move the volume fader quickly with a 5 ms polling timer, I can see a lag before the fader on screen responds. IMO timing the controller polling with the audio thread is not a good solution because the GUI would become very unresponsive with high audio buffers.

@daschuer
Copy link
Member

How long is the lag you see?

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 26, 2020

Interestingly I notice no lag using my A&H Xone K2 MIDI controller with a 5 ms polling timer. But using my friend's NI Kontrol S2 Mk2 HID controller, the lag is very noticable.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 26, 2020

I tested with a proof-of-concept bypassing the whole common-hid-packet-parser.js library and only mapping a single fader using a DataView on the ArrayBuffer to verify the problem was in C++, not the controller script. I also tested cherry-picking e8e2487 with that minimal proof-of-concept HID script. Only changing the polling timer fixed the lag.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 26, 2020

How long is the lag you see?

I cannot quantify it exactly, but it is very noticeable. It was the first thing my friend noticed about Mixxx when trying to use Mixxx with her controller.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 26, 2020

As far as I can tell whatever caused that bug with 100% CPU usage 8 years ago is no longer relevant. It may have been a kernel bug that has since been fixed or a kernel configuration in distributions which is no longer used. With a 1 ms polling timer, Mixxx is using less CPU (~5-6%) than Firefox running in the background (~6-8%). Please test on other Linux distros to verify this.

Can we get people on different distributions to test this and report Mixxx's CPU usage? @Holzhaus can you test on Arch?

@daschuer
Copy link
Member

I cannot quantify it exactly, but it is very noticeable.

If we consider a 33 ms waveform as smooth, why can a 5 ms -> to 1 ms change make a difference.
There must be an issue elsewhere.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 27, 2020

I cannot reproduce the lag with Mixxx 2.2 using the Kontrol S2 Mk2 on Linux. I suspect #2179 introduced this bug, so IMO this is a high priority bug to fix before releasing 2.3.

There must be an issue elsewhere.

Quite possibly. But this simple change works around it and I do not notice any downside. If we get bug reports from beta users about high CPU usage we can revert it and try other approaches.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 27, 2020

@ywwg are you able to reproduce the lag with the Kontrol S3 on Linux using Mixxx 2.3 and confirm that there is not lag with 2.2?

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 27, 2020

As far as I can tell whatever caused that bug with 100% CPU usage 8 years ago is no longer relevant. It may have been a kernel bug that has since been fixed or a kernel configuration in distributions which is no longer used.

It is also possible that changes in Qt in the last 8 years have made the CPU usage issue obsolete.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 27, 2020

I tested both 2.3 and master to confirm the bug was not introduced by the switch to QJSEngine (#2682).

@codecat
Copy link
Contributor

codecat commented Jul 27, 2020

Interesting, I suppose the synchronous HID polling could've changed this behavior. I believe #2179 has several different solutions to the problem, it might be worth testing if using those instead of the one that's being used right now fixes the problem. (Particularly the "loop until no more messages" thing that ended up not being super useful at the time.)

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 27, 2020

Interesting, I tested a proof of concept with a 5 ms poll timer that loops 5 times per call to HidController::poll and the GUI is as responsive as the 1 ms timer.

Be-ing added a commit to Be-ing/mixxx that referenced this pull request Jul 27, 2020
Otherwise, it seems messages build up in a queue which produces
a dramatic lag on Linux when moving faders quickly. This
regression was introduced between Mixxx 2.2 and 2.3 beta, likely
by switching HidController to nonblocking polling in PR mixxxdj#2966.
@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 27, 2020

When I set the poll timer very high (100 ms) with #2970, I do not see the dramatic GUI lag as the current 2.3 branch. Instead, when I move the fader quickly, I see choppy discrete movements, but I do not perceive a dramatic delay between moving the fader and seeing a response. So I think maybe we should go with @daschuer's idea to couple the controller polling timer to the audio buffer size. Polling controllers at 5 ms doesn't make sense if the audio buffer is smaller than that.

@Be-ing Be-ing closed this Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants