Improve DAC timing plausibility and add a CPU timer fallback #881
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| if (!m_invalidTimeInfoWarned) { | ||
| qWarning() << "SoundDevicePortAudio: Audio API provides invalid time stamps," | ||
| << "waveform syncing disabled." | ||
| << "waveform by CPU Timer" |
There was a problem hiding this comment.
Maybe clearer if you said "syncing waveform with a CPU timer"
|
fixed |
|
Can we merge this now? |
|
CoreAudio provides reliable timestamps but triggers the warning on startup: Here's a sample of timings while running normally. Printline is inserted in SoundDevicePortAudio::updateCallbackEntryToDacTime.cpp line 1001. |
|
Yes the Data is OK. I will tweak the checks a bit. This sequence looks not that nice. It seams that your sound API insists to use a 0.0928119 cycle even though you have requested a 0.0853333. To catch up it does two in a row to catch up. |
|
Please try again. It should work without any warning in your case. |
|
Hm, still seems to produce the error: |
|
If we compare the data form the first callback with the data of the second one, we have So we facing two issues:
So there is a chance to special case Mac here. But we are talking about only two callbacks after setting up the hardware, so I will just skip calculation. |
…e first two callbacks
|
Ok, This should work now. |
Closed
|
Thanks -- I'll re-test soon. |
|
Still triggers right away upon startup: |
|
I enabled very verbose debugging in portaudio. Here's a normal run where the plausibility check passes: |
|
And here's a run where it fails: |
|
I think the take-home point is that PortAudio delivers multiple callbacks to Mixxx per OS callback in some situations and so making assumptions about 1 callback == 1 DAC period isn't going to work. |
|
Added some logging of the stream times and it looks like currentTime is the same across multiple callbacks from the same CoreAudio callback -- so maybe we can use currentTime to identify a new OS callback? Does this also hold on Linux when user buffer < host buffer? |
|
The target of all this measuring is to sync the Waveform samples with the audio samples. If there is a second thread or buffer or something in Portaudio involved, we cannot trust the currentTime. It represends the moment when Portaudio processed the callback but not the moment when Mixxx processes its callback. This seams to be the case in the CoreAudio wrapper. We can also not rely to the fact that a change in currentTime indecates an immediately Mixxx callback, since there are 3 or 4 Mixxx callback for a Coraudio callback. So it is in the first instance correct to switch back to the the CPU timer. The CPU timer suffers the error produced by the two independent crystals, but this should be minor compared to the jitter introduced by the Portaudio callback implementation. I assume your waveforms are much smoother with this PR compared to master. If we aiming for a low latency vinyl setup, this extra buffer inside port audio is not desired. The jack implementation of PA suffers the same issue, and will be solved one mixxx is a native jack client. Maybe we should become a native Coraudio client as well. |
|
I don't think we want to move away from using PortAudio -- I also don't On Sun, May 8, 2016, 2:24 PM Daniel Schürmann notifications@github.com
|
|
In the Jack case, the issue is IMHO pretty clear. The way Jack works, is completely different compared to the way Portaudio works. Portaudio does not allow to expose streams without connect them to a device. It also does not allow to set up stream connections from any other third party application, nor change the latency. Portaudio has a concept of devices. Jack has a concept of Mono ports. Since Jack is already a wrapper around a Soudn API with a simple API, it does not make sense to wrap it again and turn Portaudio into Jack, to expose the jack features. I have just digged thought the pa_mac_core.c. I did not understand everything, but it looks like the "quite advanced" CoreAudio API is stripped a lot to behave like other APIs. It looks like CoreAudio has its own buffer and latency management. It delivers nice object orientated buffers, independent for input and output devices. PA does then a lot of things and buffering. It finally removes all these features and build a single callback with a fixed buffer size and input and output in one call. By this, the exact timing and the low latency nature seems to be lost. For the original issue here seems to be that PA tries to query a fresh current Time for every callback, but the time is not advanced by CoreAudio. It might not expect such an unusual use of their API ;-) Do you had time to compare the waveform behavior? |
|
This sounds like a bug in Mixxx -- we're using a host buffer size that's smaller than the CoreAudio buffer so we are getting multiple callbacks to fill one CoreAudio buffer or something like this. Anecdotally, CoreAudio is Mixxx's best supported and best performing API. I don't think there's any problem with the way we use PortAudio for CoreAudio support. On a Mac, Mixxx "just works" with no problems. The problem I am describing is that we are assuming every callback from PortAudio means that X milliseconds have passed by -- and what I'm saying is that is an incorrect assumption that we should not rely on. |
Probably yes, but since it works quite good and is still "Mixxx's best supported and best performing API" this cannot be a big issue. Maybe we can treat it like a feature: you have requested a 1.33 ms buffer and you get it :-)
We do not rely on it. Thats why we need a reliable jitter measurement. The code in this PR correctly detects that the timing is not reliable and uses the CPU timer instead. So your test case proves that the code works. Without the patch, Mixxx just uses the "wrong" Timing values which leads to a jerking waveform. Not sure if this is that notable, because we have an other issue with the V-Sync By the way, the original PA author is aware that the timing is not reliable: |
|
Ok -- sounds like this is working-as-intended for OS X -- pending an improvement to PortAudio or our usage of PortAudio. |
|
Thanks for the changes! |
This was referenced Aug 22, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here it is.
It works surprisingly well with the pulse ALSA device. Even if we have a 47 ms cycle for a 43 ms buffer and two callbacks without a gap if the buffer is consumed.