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.

Sign up for GitHub

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

Jump to bottom

JACK buffer size fix. #11121

Merged

JoergAtGithub merged 21 commits into mixxxdj:2.3 from daschuer:jack-api-check-rebase

Feb 4, 2023

Member

daschuer commented Dec 8, 2022

This PR disposes all stored buffer size values and uses consequently the value passed as callback parameter.
This is required because the JACK buffer size is externally controlled and can change at any time.
Current main uses the crazed out buffer size form hardware preferences.

Because if that the timing compensation with two soundcards or the network clock fails and is now grayed out in the hardware preferences.

Note: Multi soundcard setup is working, but because if the joint stream in JACK and the split-up in Portaudio. It adds an extra buffer latency for the non main device. It is also known that the clock synchronisation in native JACK is broken and the situation is not clear in case in pipewire.

So if you want a minimum latency setup, use ALSA directly.

This PR is on top of #11092

Many thanks to @robbert-vdh for finding the original issues and for the initial PR

robbert-vdh and others added 14 commits

December 6, 2022 20:11


          Fix condition in JACK backend check

c2b1af8

`m_deviceInfo->hostApi` is an index in the list of supported APIs on
this platform. So on Linux 0 is ALSA, 2 is JACK. This is also mentioned
in the comment on the `hostApi` field. We should be checking the device
type ID instead, as is done elsewhere in the code.

This fixes the buffer size when using JACK clients. With JACK the buffer
size should be coming from the server (and PortAudio chops it up into
1024 sample blocks if it's larger). Before this change it would use the
now grayed out latency value set for the previous audio backend before
switching to JACK (defaulting to 1024 samples).


          Replace m_framesPerBuffer uses with runtime size

82c7f71

In any operation that works with audio buffers passed from PortAudio.
This is needed because PortAudio may pass us buffers that are smaller or
larger than the size we request. And when using JACK the requested size
is 0, which will cause PortAudio to pick a buffer size matching the JACK
server.


          Use std::unique_ptr for FIFOs in PortAudio backend

fefbfd2

The network backend already does this.


          Remove unused getProcessingLatency() function

cfeffa7


          renamed m_framesPerBuffer to m_configFramesPerBuffer

to clarify that thisis not the actually used value. The actual value is
passied via every audi callback.


          Improve the comment that explains paFramesPerBufferUnspecified

b1b14b1


          Don't use the m_configFramesPerBuffe and improve debug output and com…

8f97592

…ments.


          dispose m_iBufferSize and use always the callback value as parameter

4c1eb24

This clarifies that the value is a subject of change in case of JACK


          Always use the real framesPerBuffer value received via the callback

e52de39

This allows following the server buffer size that is not always const.


          Deduplicate code in VisualPlayPosition

75d0e29


          Apply a better offset limit in VisualPlayposition

f1bc670


          Dispose audio_buffer_size and use directly the callback parameter

096089d


          Use MAX_BUFFER_LEN for all non clock reference devices in case of JACK.

6d24c26


          Don't allow setting device sync and network clock with the JACK API

705e097

github-actions bot added code quality ui labels

daschuer mentioned this pull request

Fix condition in JACK backend check, previously resulting in too large buffers #11092

Closed

Swiftb0y previously requested changes

View reviewed changes

Member

Swiftb0y left a comment

A couple preliminary comments

src/soundio/sounddevicenetwork.cpp Outdated

		m_audioBufferTime = mixxx::Duration::fromSeconds(
		const auto requestedBufferTime = mixxx::Duration::fromSeconds(

Member

Swiftb0y Dec 8, 2022

I appreciate the use of const auto here 👍

src/soundio/sounddevicenetwork.cpp

Comment on lines +486 to 488

+                  m_targetTime += static_cast<qint64>(framesPerBuffer / m_dSampleRate * 1000000);
                   double callbackEntrytoDacSecs = (m_targetTime - currentTime) / 1000000.0;
                   callbackEntrytoDacSecs = math_max(callbackEntrytoDacSecs, 0.0001);

Member

Swiftb0y Dec 8, 2022

Didn't we have an abstraction for sample and frame related time calculations?

Member Author

daschuer Dec 8, 2022

I am not aware of something for this special case.

src/engine/enginemaster.cpp Outdated Show resolved Hide resolved

src/engine/enginemaster.cpp Show resolved Hide resolved

src/soundio/sounddeviceportaudio.cpp Outdated Show resolved Hide resolved

src/soundio/sounddeviceportaudio.cpp Outdated Show resolved Hide resolved

src/waveform/visualplayposition.cpp

                   data.m_positionStep = positionStep;
                   data.m_slipPosition = slipPosition;
                   data.m_tempoTrackSeconds = tempoTrackSeconds;
+                  data.m_audioBufferMicroS = audioBufferMicroS;

Member

Swiftb0y Dec 8, 2022

why does the waveform need to know about the buffer size??

Be-ing reviewed

View reviewed changes

src/soundio/sounddeviceportaudio.cpp Outdated Show resolved Hide resolved

Be-ing reviewed

View reviewed changes

src/soundio/sounddeviceportaudio.cpp Outdated

+                      // PortAudio's JACK back end has its own buffering to split or merge the buffer
+                      // received from JACK to the desired size.
+                      // However, we use here paFramesPerBufferUnspecified to use the JACK buffer size
+                      // which offers the best responds time without additional jitter.

Contributor

Be-ing Dec 8, 2022

The issue here is extra latency, not extra jitter.

Member Author

daschuer Dec 8, 2022

The jitter is introduced if Portaudio composes new buffers from the native JACK buffer. In this case we have two callbacks in a row and than the pause. I will improve the comment.

Be-ing reviewed

View reviewed changes

src/soundio/sounddeviceportaudio.cpp

+                      // For every sound hardware. Clock drift can not happen, but the buffers
+                      // are processed: In 1 - Out 1 - In 2 - Out 2
+                      // This causes even with the "Experimental (no delay)" setting
+                      // one extra buffer delay for the non clock reference device

Contributor

Be-ing Dec 8, 2022 •

edited

Loading

There's no way to avoid this extra latency when using PortAudio with multiple JACK clients? If so, I think that's a good justification to not use PortAudio for JACK. (That's beyond the scope of this bugfix, of course.)

Member Author

daschuer Dec 8, 2022

I see no way. And yes that is only one reason why Mixxx should become a native JACK client.

daschuer added 4 commits

December 8, 2022 23:03


          Avoid implicit conversion from double to bool

e15cda3


          Improve comment about vectorization

b48e06d


          Improve comments and debug output

185a681


          check channel count for > 0 instead of implicit bool conversion

44fa755

Member Author

daschuer commented Dec 9, 2022

Done.

robbert-vdh reviewed

View reviewed changes

src/soundio/sounddeviceportaudio.cpp Outdated Show resolved Hide resolved

Member

Swiftb0y commented Dec 9, 2022

My general worry with this PR is that we're disconnecting the buffer size from the actual buffer, this dramatically increases the chance for programmer error resulting in out-of-bounds accesses.

Contributor

robbert-vdh commented Dec 9, 2022

Isn't it the other way around? This PR (and #11092 leading up to it) causes the actual buffer size to be used for computations working on the audio buffer, instead of the configured buffer size (which may or may not match the actual buffer size).

Member

Swiftb0y commented Dec 9, 2022

I guess it depends on what buffer we're talking about. I'm not familiar enough with the surrounding code. It just worries me in general to see the size disconnected from the buffer (or rather vice versa).

Member Author

daschuer commented Dec 9, 2022

Also I'm more worried that we're using iBufferSize to access a foreign buffer thats not allocated with MAX_BUFFER_LEN.

That is the essence of this PR, using the buffer size that is provided along with the pointer to the buffer. In the current main branch this is not the case, we rely on Portaudio that we receive the requested buffer size, which is luckily reliable the case.

Also, we don't use iBufferSize to access the buffets allocated with MAX_BUFFER_LEN in this PR. We use them to pass sample across the two streams in case of two soundcards.

mixxx/src/soundio/sounddeviceportaudio.cpp

Line 589 in 44fa755

int outChunkSize = framesPerBuffer * m_outputParams.channelCount;

The FIFOs have there own save read and write size values. In the unlikely case the FIFO buffer is to small, the buffer from Portaudio is padded with zeros.

Member

Swiftb0y commented Dec 9, 2022

I'm sorry, I can't comment on this, I don't understand how your explanation relates to the code you linked.

Swiftb0y dismissed their stale review

December 9, 2022 20:54

I'm distancing myself from this PR

Member Author

daschuer commented Dec 9, 2022

That is the code where the FIFO of MAX_BUFFER_LEN used. You can see that it does not relate to iBufferSize.


          Remove unused variable

32379a6

Member Author

daschuer commented Dec 11, 2022

Ready

Member Author

daschuer commented Dec 14, 2022

@robbert-vdh can you confirm that this is working for you and do a brief review of my code?
With that in place we can ask another team member to merge.

robbert-vdh reviewed

View reviewed changes

Contributor

robbert-vdh left a comment

@daschuer I haven't tested it too extensively, but the JACK seems to still function the same as #11092. And while the waveforms from multiple tracks still aren't entirely in sync, they at least seem to move around less than they did before.

I'm not too familiar with these parts of Mixxx but it looks good to me. I grepped for m_iBufferSize to find more remaining uses and I stumbled upon src/analyzer/analyzergain.h. That class has a m_iBufferSize field that's initialized to 0 and then used in a comparison that always returns true if the audio buffer being analyzed isn't empty. That's probably a remnant of some older behavior and can also be inlined.

src/soundio/soundmanager.h Outdated

@@ @@ -126,6 +125,9 @@ class SoundManager : public QObject { @@
                   void closeDevices(bool sleepAfterClosing);
                   void setJACKName() const;
+                  bool jackApiUsed() const {
+                      return m_jackSampleRate.isValid();

Contributor

robbert-vdh Dec 14, 2022

Is this correct? m_jackSampleRate seems to be set in SoundManager::queryDevicesPortaudio() when the JACK API is available. So it sounds like it will also be set when Mixxx uses ALSA, WasAPI, CoreAudio, etc. while a JACK server is available. I have not yet checked if this is actually the case here.

Member Author

daschuer Dec 16, 2022

Ups, good catch. Fixed.


          Fix jackApiUsed() implementation

31ba28e

Member Author

daschuer commented Jan 10, 2023

@robbert-vdh Is there anything left to do or can you formally approve this PR?

Contributor

robbert-vdh commented Jan 10, 2023

It's been a hot minute since I looked at it but it looked good to me! I tested this before and it seemed to work fine. I've been using the original waveform change from #11092 in my own branch since rebasing this PR seemed a bit more involved so I have not tested it super extensively beyond that.

Did you take a look at the removing that unused m_iBufferSize field I mentioned yet? That was the last remaining reference to m_iBufferSize. #11121 (review)

Member Author

daschuer commented Jan 11, 2023

Yes, the fix has already been merged via eb0339f

JoergAtGithub added engine soundio labels

JoergAtGithub reviewed

View reviewed changes

src/soundio/sounddevice.cpp Show resolved Hide resolved

JoergAtGithub reviewed

View reviewed changes

src/soundio/sounddevicenetwork.cpp

                       ControlObject::set(ConfigKey("[Master]", "samplerate"), m_dSampleRate);
-                      ControlObject::set(ConfigKey("[Master]", "audio_buffer_size"),
-                              m_audioBufferTime.toDoubleMillis());

Member

JoergAtGithub Jan 24, 2023

I guess it's highly unlikely that this CO is used somewhere in mappings or skins - and therefore can be removed.

Member

Holzhaus Jan 24, 2023

I wanted to mention that it also needs to be removed from the Mixxx Controls section in the manual, but that CO does not seem to be documented anyway. 🤷

JoergAtGithub reviewed

View reviewed changes

src/soundio/sounddevicenetwork.cpp

                   m_clkRefTimer.start();
                   qint64 currentTime = m_pNetworkStream->getInputStreamTimeUs();
-                  m_targetTime += m_audioBufferTime.toIntegerMicros();
+                  // This deadline for the next buffer in microseconds since the Unix epoch
+                  m_targetTime += static_cast<qint64>(framesPerBuffer / m_dSampleRate * 1000000);
                   double callbackEntrytoDacSecs = (m_targetTime - currentTime) / 1000000.0;
                   callbackEntrytoDacSecs = math_max(callbackEntrytoDacSecs, 0.0001);
                   VisualPlayPosition::setCallbackEntryToDacSecs(callbackEntrytoDacSecs, m_clkRefTimer);

Member

JoergAtGithub Jan 24, 2023

I understand that for rate syncing with the frequency of the network clock framesPerBuffer is enough. But to show the visual waveform, not only rate, but also latency is needed.
I don't understand this calculation. Shouldn't the waveform be in sync with the sound that the DJ hears (headphones or booth speakers) even if he's broadcasting a network stream?

Member Author

daschuer Jan 25, 2023

In this case the latency of the main DAC is lost. It would be an extra feature to compensate this.

This calculation is only to compensate the jitter that happens due to the different cycles of the audio and video thread.

At least this is unrelated to this PR. Do we want to file a bug for that?

Member

JoergAtGithub Jan 25, 2023

Yes, please create a bug for this. It would be great, if you could add some pointers to this report, how a possible fix could look like.
I agree, that this is not related to this PR.

Member Author

daschuer Jan 26, 2023

Here it is: #11219


          Use the correct term frames per period

f49bf12

daschuer added this to the 2.3.4 milestone

Member Author

daschuer commented Jan 30, 2023

@Swiftb0y I consider this a 2.3.4 fix. Is it now good to merge?

Member

Swiftb0y commented Jan 30, 2023

I'm sorry, I can't approve this PR

I'm sorry, I can't comment on this, I don't understand how your explanation relates to the code you linked.

Member

JoergAtGithub commented Jan 30, 2023

I read through the code and it looks clean to me.
The change to pass framesPerBuffer as function argument instead of using the member variable doesn't help readability, but ensures, that the code runs always with the correct buffer size, which can change with every callback in case of JACK.
I can't test it myself, but since it solves the problem for @daschuer and @robbert-vdh, I rate this as properly tested.
Removing the undocumented CO [Master],audio_buffer_size which is not used in any official mapping or skin, seems to be Ok for me, if it's mentioned in the changelog.

If nobody has objections, I plan to merge this PR therefore.

JoergAtGithub added changelog and removed ui labels

JoergAtGithub merged commit 0f53f91 into mixxxdj:2.3

Member

JoergAtGithub commented Feb 4, 2023

Thank you @robbert-vdh and @daschuer !

JoergAtGithub added a commit to JoergAtGithub/mixxx that referenced this pull request


          Fix regression from merging 2.3 branch with PR mixxxdj#11121 into 2.4…

1450a57

… with PR mixxxdj#4641

JoergAtGithub mentioned this pull request

Fix regression from merging 2.3 branch into 2.4 #11266

Merged

atskler mentioned this pull request

Buffer underflow in every second with JACK @96K #11341

Closed

daschuer deleted the jack-api-check-rebase branch

May 4, 2023 11:09

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

JoergAtGithub JoergAtGithub left review comments

Swiftb0y Swiftb0y left review comments

Be-ing Be-ing left review comments

Holzhaus Holzhaus left review comments

robbert-vdh robbert-vdh left review comments

Labels

changelog code quality engine soundio