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

Audio dropouts #1717

Closed
ambanmba opened this issue Jun 22, 2023 · 22 comments
Closed

Audio dropouts #1717

ambanmba opened this issue Jun 22, 2023 · 22 comments
Milestone

Comments

@ambanmba
Copy link

ambanmba commented Jun 22, 2023

When playing back DAB audio, the sound glitches when the UX is "under load" - I put that in quotes, because it's not really load - just moving windows around and whatnot. Just sitting with nothing changing on the screen, the audio is fine. It's repeatable by simply moving a window around. I have set Pre-Fill to 5s just to confirm it's not a buffering issue.

Also, tested on the same Mac with welle.io which can also do DAB playback, and there it's all fine. So it appears to be an issue with sdrangel.

Running a Mac Mini 2018, 3.2 Ghz 6-Core i7 and an external AMD Radeon RX580 egpu

Other apps work fine, overall system load is about 5%

@srcejon
Copy link
Collaborator

srcejon commented Jun 22, 2023

I think this is a more general problem, so will update the subject.

On Windows, there will also be audio drop-outs if the SDRangel window is moved and it happens with all demods. E.g. with BFM Demod, grab the title bar and move window continuously in a circle and the audio will stop completely. This isn't a problem on Linux, however. On Linux, you can get shorter dropouts when resizing a window (but not always). Also there can be audio drop outs when stopping/starting other SDRs.

I think, as discussed in a previous issue, part of the problem may be that audio output may be being performed on the UI thread, and so it can be blocked by slow UI operations.

@srcejon srcejon changed the title macOS glitchy DAB playback Audio dropouts Jun 22, 2023
@srcejon
Copy link
Collaborator

srcejon commented Jun 22, 2023

#1522 is the other issue.

@f4exb
Copy link
Owner

f4exb commented Jun 23, 2023

audio output may be being performed on the UI thread

What is the proof of this? Knowing that UI thread is the main thread this is a Qt fact.

@f4exb
Copy link
Owner

f4exb commented Jun 23, 2023

Well I think I can answer myself...

The critical part is here https://github.com/f4exb/sdrangel/blob/master/sdrbase/audio/audiooutputdevice.cpp#L323 where the audio mix is created. You would not want this to happen on the main thread.

I have put in place some debug messages to get the main thread ID in the MainWindow constructor and in the readData method also taking note of what audio device was concerned.

It turns out that the main thread is being used for the system default device but not the others. Unfortunately this is the one that is used most of the time. An interesting test would be to select the same device but not as the system default and see if the same issue occurs.

@srcejon
Copy link
Collaborator

srcejon commented Jun 23, 2023

An interesting test would be to select the same device but not as the system default and see if the same issue occurs.

Yep, audio does not cut out when moving the window, when using the non default device.

@f4exb
Copy link
Owner

f4exb commented Jun 23, 2023

Interesting... I think this is what may happen... The AudioOutput object handling the readData() method gets created here whenever there is none for the given audio device: https://github.com/f4exb/sdrangel/blob/master/sdrbase/audio/audiodevicemanager.cpp#L261

Now... when a channel plugin is created it is first assigned the default device therefore the first created channel will trigger the creation of the corresponding AudioOutput and it happens in the constructor of the "baseband" object that is later put on its own thread. If I take the example of the SSB demod this happens here: https://github.com/f4exb/sdrangel/blob/master/plugins/channelrx/demodssb/ssbdemodbaseband.cpp#L44

The thing is at the time of the construction of the "baseband" object it is still owned by the "demod" object here SSBDemod that lives on the main thread.

If later you change the audio device this is done in the applySettings() method of the "baseband" object via message passing and therefore on its own thread: https://github.com/f4exb/sdrangel/blob/master/plugins/channelrx/demodssb/ssbdemodbaseband.cpp#L176

One could remove the code in the constructor since appluSettings() will always be called once the "baseband" thread is started but this raises another issue...

In most cases when the default audio device is used the main thread will own the corresponding AudioOutput object (as described previously) and whatever happens to the channel plugins later do not matter since the main thread is always there. Now if you let the "baseband" thread allocate the AudioOutput object and that "baseband" is deleted later then we have an issue. In fact this results in the readData() method to be inoperative. In the scenario where you have one demod from the default configuration, you create a new one (via "clone" for example) and delete the first one the audio FIFO of the second and now the only one cannot be drained to effective audio output:

2023-06-23 12:18:14.333 (C) AudioFifo::write: (SSBDemod [0:0]) overflow 16384 samples
2023-06-23 12:18:14.333 (D) SSBDemodSink::processOneSample: 0/16384 samples written
2023-06-23 12:18:14.674 (C) AudioFifo::write: (SSBDemod [0:0]) overflow 16384 samples
2023-06-23 12:18:14.674 (D) SSBDemodSink::processOneSample: 0/16384 samples written
2023-06-23 12:18:15.016 (C) AudioFifo::write: (SSBDemod [0:0]) overflow 16384 samples
2023-06-23 12:18:15.016 (D) SSBDemodSink::processOneSample: 0/16384 samples written
2023-06-23 12:18:15.357 (C) AudioFifo::write: (SSBDemod [0:0]) overflow 16384 samples

Therefore the AudioOutput object should have its own thread not even mentioning the fact that whatever happens on the thread of the channel that allocated the AudioOutput corresponding to an audio device will impact all other channels that use the same audio device.

@srcejon
Copy link
Collaborator

srcejon commented Jun 23, 2023

the first created channel will trigger the creation of the corresponding AudioOutput and it happens in the constructor of the "baseband" object that is later put on its own thread.

I see QAudioSink and QAudioOutput are created with parent as null. Perhaps if parent is set to the baseband object, then the call to moveToThread will move them to the baseband thread as well (That is what happens with QTimers and the like).

(Sorry for editing your comment - somehow pressed edit rather than quote!)

@f4exb
Copy link
Owner

f4exb commented Jun 23, 2023

np... I see it has been restored.

But for the reason I mentioned last I don't think this is even a good idea to use the baseband thread since it may be deleted at some point or if it gets blocked then all other channels using the same audio device will be blocked.

@f4exb
Copy link
Owner

f4exb commented Jun 23, 2023

Unfortunately it is not possible to have the AudioOutputDevice running on its own thread because it inherits of QIODevice so this issue cannot be solved.

@srcejon
Copy link
Collaborator

srcejon commented Jun 23, 2023

How so? When using the non-default device, from what you've written (and the fact the problem disappears), it appears to be on a different thread.

This Qt bug report also suggests it should be possible to use a different thread: https://bugreports.qt.io/browse/QTBUG-43690

@srcejon
Copy link
Collaborator

srcejon commented Jun 23, 2023

Do we just need some sort of worker object that contains the AudioOutputDevice if it's some sort of inheritance problem?

@f4exb
Copy link
Owner

f4exb commented Jun 23, 2023

Sorry I was probably missing the "public" inheritance from QIODevice. I will try again...

@f4exb
Copy link
Owner

f4exb commented Jun 24, 2023

Unfortunately putting AudioOutputDevice (inherits from QIODevice) or QAudioOutput on its own thread has no effect. The thread creating AudioOutputDevice is still the one running the readData method of AudioOutputDevice.

@srcejon
Copy link
Collaborator

srcejon commented Jun 24, 2023

That was with calling QObject::moveToThread for all objects?

Is it possible to create a worker object that when run, creates AudioOutputDevice/QAudioOutput, so those objects are created on the thread you want to run them in?

@f4exb
Copy link
Owner

f4exb commented Jun 24, 2023

This involves a massive rewrite of AudioDeviceManager and AudioOutputDevice for a very uncertain result. I think this is a Qt bug.

@f4exb
Copy link
Owner

f4exb commented Jun 24, 2023

Well I was doing things the wrong way. At some point I put a parent to AudioOutputDevice but then with a parent you cannot move to thread. Before that I realized that the start and stop methods of AudioOutputDevice should not be called directly else later the readData method will be invoked from the thread that did the start and in that case this is the main thread. Instead the start and stop should be called from the event loop and thus through message queuing.

So maybe in the end we could get something working with minimal changes that in addition fixes the issue with multiple channels.

I have put some debug messages and if I select only the ones with the thread address doing the readData this is indeed the one allocated for AudioOutputDevice and not the main thread:

2023-06-24 12:57:16.636 (D) AudioDeviceManager::addAudioSink: thread: 0x5603f087de10
2023-06-24 12:57:16.637 (D) AudioOutputDevice::start: device: -1 rate: -1 thread: 0x5603f087de10
2023-06-24 12:57:16.658 (D) AudioOutputDevice::readData: thread 0x5603f087de10 (System default)
2023-06-24 12:57:16.918 (D) AudioOutputDevice::readData: thread 0x5603f087de10 (System default)
2023-06-24 12:57:17.438 (D) AudioOutputDevice::readData: thread 0x5603f087de10 (System default)

@f4exb
Copy link
Owner

f4exb commented Jun 25, 2023

Unfortunately audio is broken
Edit: I have to check this again it might be a different issue since the sample rate for the device I am using is set at 8000 Hz which is a bit of a corner case. I already noticed that the Audio FIFO size should be set relative to the sample rate else you get an excessively large FIFO and then the audio device cannot cope with excessively large fill-up intervals.

@f4exb f4exb closed this as completed in e79dfd4 Jun 28, 2023
@srcejon srcejon reopened this Jun 29, 2023
@srcejon
Copy link
Collaborator

srcejon commented Jun 29, 2023

Unfortunately, this patch isn't quite working for me on Windows. With my main sound card (and also virtual audio cable), audio is always very choppy, and I continually get overflows in all demods:

2023-06-29 08:35:22.516 (C) AudioFifo::write: (BFMDemod [1:0]) overflow 12288 samples
2023-06-29 08:35:22.517 (D) BFMDemodSink::feed: 4096/16384 audio samples written
2023-06-29 08:35:22.848 (C) AudioFifo::write: (BFMDemod [1:0]) overflow 8192 samples
2023-06-29 08:35:22.848 (D) BFMDemodSink::feed: 8192/16384 audio samples written
2023-06-29 08:35:23.192 (C) AudioFifo::write: (BFMDemod [1:0]) overflow 12288 samples

2023-06-29 08:40:32.991 (D) DABDemodSink::audio: 15232/16384 audio samples written
2023-06-29 08:40:34.365 (C) AudioFifo::write: (DABDemod [1:0]) overflow 1152 samples
2023-06-29 08:40:34.365 (D) DABDemodSink::audio: 15232/16384 audio samples written
2023-06-29 08:40:35.731 (C) AudioFifo::write: (DABDemod [1:0]) overflow 1152 samples
2023-06-29 08:40:35.732 (D) DABDemodSink::audio: 15232/16384 audio samples written

2023-06-29 08:42:35.182 (D) SSBDemodSink::processOneSample: 4096/4800 samples written
2023-06-29 08:42:35.289 (C) AudioFifo::write: (SSBDemod [1:0]) overflow 704 samples
2023-06-29 08:42:35.289 (D) SSBDemodSink::processOneSample: 4096/4800 samples written
2023-06-29 08:42:35.385 (C) AudioFifo::write: (SSBDemod [1:0]) overflow 4800 samples
2023-06-29 08:42:35.385 (D) SSBDemodSink::processOneSample: 0/4800 samples written
2023-06-29 08:42:35.482 (C) AudioFifo::write: (SSBDemod [1:0]) overflow 4800 samples

Running on Linux as a guest VM on the same Windows PC (so with same audio h/w) is fine, however.

Also, if I switch to HDMI audio on Windows (so sound is from monitor rather than speakers connected to sound card), then the audio is fine.

I have tried adjusting demod audioBuffer size up and down, but it doesn't seem to help.

Will try to add some more debug, to see if I can see what's the difference between the soundcard and HDMI.

On the subject of audio FIFO size, would it be useful to have this as a user-setting in Audio Preferences (Perhaps specified in milliseconds)? This way, users with fast computers and low latency sound cards, could try to reduce latency.

@srcejon
Copy link
Collaborator

srcejon commented Jun 29, 2023

The problem seems to be the following function call added in the patch:

m_audioOutput->setBufferSize(m_audioFormat.sampleRate() / 5); // min 200ms

This is setting the QAudioOutput buffer size to 9600 (as my sample rate is 48000). If I remove that function call, the default buffer size appears to be 38400 bytes and audio is fine.

(It seems we have three buffers/FIFOs. First, audioBuffer in the demod sinks. Then the AudioFifo to copy data from the demod to the AudioOutputDevice to be mixed. And then this buffer in the Qt QAudioOutput)

@f4exb
Copy link
Owner

f4exb commented Jun 29, 2023

The QAudioOutput buffer size is not necessarily equal to the number of bytes you give to setBufferSize(). In my case when I give 9600 I obtain slightly more so I was under the impression that the system will manage for the best (a debug message shows the actual size). Apparently not...

A buffer size of 38400 at 48000 S/s is 0.8s. In my case this is 48000 so just exactly 1s. At the same time I was using SDRangel on the side taking the SSB demod output to JTDX to overcome the poor receiver quality of my X6100 (an aviary of birdies...) and noticed I had excessively large time offsets on the message received (over 1s). By reducing the QAudioOutput buffer size I went well below the second (~0.7s) which is more acceptable. So the size of the QAudioOutput buffer has a direct influence on the overall audio latency. I also found that the audioBuffer had not a prominent influence if any. In fact if you reduce the delay the FIFO will be written more often. If you do so you can make sure that the FIFO will not be empty between too many writes for too few reads so it has to be somehow in line with the QAudioOutput buffer size. The FIFO is fed from the audio buffer and drained by the QAudioOutput. Its size has not much influence as long as it is large enough to accommodate the flow variations.

It is a pity that Qt does not actually choose the best size. In order to close this ticket because it still contains an important enhancement with the threads mechanism I will remove the setBufferSize() call and treat the audio latency issue in another ticket.

@f4exb
Copy link
Owner

f4exb commented Jun 29, 2023

Maybe of some interest for the audio latency issue: https://forum.qt.io/topic/115689/how-to-minimise-audio-output-latency/7

@srcejon
Copy link
Collaborator

srcejon commented Jun 30, 2023

Thanks - this is a small but great improvement.

@f4exb f4exb added this to the v7.15.1 milestone Jul 11, 2023
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

No branches or pull requests

3 participants