Merge pull request #10394 from zzmp/fix/local-audio-prep

Guard against local audio crashes

libraries/audio-client/src/AudioClient.cpp

@@ -1097,28 +1097,27 @@ void AudioClient::handleRecordedAudioInput(const QByteArray& audio) {
 }
 
 void AudioClient::prepareLocalAudioInjectors() {
-    if (_outputPeriod == 0) {
-        return;
-    }
-
-    int bufferCapacity = _localInjectorsStream.getSampleCapacity();
-    if (_localToOutputResampler) {
-        // avoid overwriting the buffer,
-        // instead of failing on writes because the buffer is used as a lock-free pipe
-        bufferCapacity -=
-            _localToOutputResampler->getMaxOutput(AudioConstants::NETWORK_FRAME_SAMPLES_PER_CHANNEL) *
-            AudioConstants::STEREO;
-        bufferCapacity += 1;
-    }
-
     int samplesNeeded = std::numeric_limits<int>::max();
     while (samplesNeeded > 0) {
-        // lock for every write to avoid locking out the device callback
-        // this lock is intentional - the buffer is only lock-free in its use in the device callback
-        RecursiveLock lock(_localAudioMutex);
+        // unlock between every write to allow device switching
+        Lock lock(_localAudioMutex);
+
+        // in case of a device switch, consider bufferCapacity volatile across iterations
+        if (_outputPeriod == 0) {
+            return;
+        }
+
+        int bufferCapacity = _localInjectorsStream.getSampleCapacity();
+        int maxOutputSamples = AudioConstants::NETWORK_FRAME_SAMPLES_PER_CHANNEL * AudioConstants::STEREO;
+        if (_localToOutputResampler) {
+            maxOutputSamples =
+                _localToOutputResampler->getMaxOutput(AudioConstants::NETWORK_FRAME_SAMPLES_PER_CHANNEL) *
+                AudioConstants::STEREO;
+        }
 
         samplesNeeded = bufferCapacity - _localSamplesAvailable.load(std::memory_order_relaxed);
-        if (samplesNeeded <= 0) {
+        if (samplesNeeded < maxOutputSamples) {
+            // avoid overwriting the buffer to prevent losing frames
             break;
         }
 
@@ -1168,16 +1167,18 @@ bool AudioClient::mixLocalAudioInjectors(float* mixBuffer) {
     memset(mixBuffer, 0, AudioConstants::NETWORK_FRAME_SAMPLES_STEREO * sizeof(float));
 
     for (AudioInjector* injector : _activeLocalAudioInjectors) {
-        if (injector->getLocalBuffer()) {
+        // the lock guarantees that injectorBuffer, if found, is invariant
+        AudioInjectorLocalBuffer* injectorBuffer = injector->getLocalBuffer();
+        if (injectorBuffer) {
 
             static const int HRTF_DATASET_INDEX = 1;
 
             int numChannels = injector->isAmbisonic() ? AudioConstants::AMBISONIC : (injector->isStereo() ? AudioConstants::STEREO : AudioConstants::MONO);
-            qint64 bytesToRead = numChannels * AudioConstants::NETWORK_FRAME_BYTES_PER_CHANNEL;
+            size_t bytesToRead = numChannels * AudioConstants::NETWORK_FRAME_BYTES_PER_CHANNEL;
 
             // get one frame from the injector
             memset(_localScratchBuffer, 0, bytesToRead);
-            if (0 < injector->getLocalBuffer()->readData((char*)_localScratchBuffer, bytesToRead)) {
+            if (0 < injectorBuffer->readData((char*)_localScratchBuffer, bytesToRead)) {
                 
                 if (injector->isAmbisonic()) {
 
@@ -1317,15 +1318,17 @@ void AudioClient::setIsStereoInput(bool isStereoInput) {
 }
 
 bool AudioClient::outputLocalInjector(AudioInjector* injector) {
-    Lock lock(_injectorsMutex);
-    if (injector->getLocalBuffer() && _audioInput ) {
-        // just add it to the vector of active local injectors, if 
-        // not already there.
-        // Since this is invoked with invokeMethod, there _should_ be
-        // no reason to lock access to the vector of injectors.
+    AudioInjectorLocalBuffer* injectorBuffer = injector->getLocalBuffer();
+    if (injectorBuffer) {
+        // local injectors are on the AudioInjectorsThread, so we must guard access
+        Lock lock(_injectorsMutex);
         if (!_activeLocalAudioInjectors.contains(injector)) {
             qCDebug(audioclient) << "adding new injector";
             _activeLocalAudioInjectors.append(injector);
+
+            // move local buffer to the LocalAudioThread to avoid dataraces with AudioInjector (like stop())
+            injectorBuffer->setParent(nullptr);
+            injectorBuffer->moveToThread(&_localAudioThread);
         } else {
             qCDebug(audioclient) << "injector exists in active list already";
         }
@@ -1333,7 +1336,7 @@ bool AudioClient::outputLocalInjector(AudioInjector* injector) {
         return true;
 
     } else {
-        // no local buffer or audio
+        // no local buffer
         return false;
     }
 }
@@ -1452,7 +1455,7 @@ void AudioClient::outputNotify() {
 bool AudioClient::switchOutputToAudioDevice(const QAudioDeviceInfo& outputDeviceInfo) {
     bool supportedFormat = false;
 
-    RecursiveLock lock(_localAudioMutex);
+    Lock lock(_localAudioMutex);
     _localSamplesAvailable.exchange(0, std::memory_order_release);
 
     // cleanup any previously initialized device
@@ -1681,8 +1684,12 @@ qint64 AudioClient::AudioOutputIODevice::readData(char * data, qint64 maxSize) {
 
     int injectorSamplesPopped = 0;
     {
-        RecursiveLock lock(_audio->_localAudioMutex);
         bool append = networkSamplesPopped > 0;
+        // this does not require a lock as of the only two functions adding to _localSamplesAvailable (samples count):
+        // - prepareLocalAudioInjectors will only increase samples count
+        // - switchOutputToAudioDevice will zero samples count
+        //   stop the device, so that readData will exhaust the existing buffer or see a zeroed samples count
+        //   and start the device, which can only see a zeroed samples count
         samplesRequested = std::min(samplesRequested, _audio->_localSamplesAvailable.load(std::memory_order_acquire));
         if ((injectorSamplesPopped = _localInjectorsStream.appendSamples(mixBuffer, samplesRequested, append)) > 0) {
             _audio->_localSamplesAvailable.fetch_sub(injectorSamplesPopped, std::memory_order_release);

libraries/audio-client/src/AudioClient.h

@@ -96,8 +96,6 @@ public:
     using AudioPositionGetter = std::function<glm::vec3()>;
     using AudioOrientationGetter = std::function<glm::quat()>;
 
-    using RecursiveMutex = std::recursive_mutex;
-    using RecursiveLock = std::unique_lock<RecursiveMutex>;
     using Mutex = std::mutex;
     using Lock = std::unique_lock<Mutex>;
 
@@ -345,7 +343,7 @@ private:
     int16_t _localScratchBuffer[AudioConstants::NETWORK_FRAME_SAMPLES_AMBISONIC];
     float* _localOutputMixBuffer { NULL };
     AudioInjectorsThread _localAudioThread;
-    RecursiveMutex _localAudioMutex;
+    Mutex _localAudioMutex;
 
     // for output audio (used by this thread)
     int _outputPeriod { 0 };

libraries/audio/src/AbstractAudioInterface.h

@@ -33,7 +33,11 @@ public:
                                 PacketType packetType, QString codecName = QString(""));
 
 public slots:
+    // threadsafe
+    // moves injector->getLocalBuffer() to another thread (so removes its parent)
+    // take care to delete it when ~AudioInjector, as parenting Qt semantics will not work
     virtual bool outputLocalInjector(AudioInjector* injector) = 0;
+
     virtual bool shouldLoopbackInjectors() { return false; }
     
     virtual void setIsStereoInput(bool stereo) = 0;

libraries/audio/src/AudioInjector.cpp

@@ -51,6 +51,10 @@ AudioInjector::AudioInjector(const QByteArray& audioData, const AudioInjectorOpt
 {
 }
 
+AudioInjector::~AudioInjector() {
+    deleteLocalBuffer();
+}
+
 bool AudioInjector::stateHas(AudioInjectorState state) const {
     return (_state & state) == state;
 }
@@ -87,11 +91,7 @@ void AudioInjector::finish() {
 
     emit finished();
 
-    if (_localBuffer) {
-        _localBuffer->stop();
-        _localBuffer->deleteLater();
-        _localBuffer = NULL;
-    }
+    deleteLocalBuffer();
 
     if (stateHas(AudioInjectorState::PendingDelete)) {
         // we've been asked to delete after finishing, trigger a deleteLater here
@@ -163,7 +163,7 @@ bool AudioInjector::injectLocally() {
     if (_localAudioInterface) {
         if (_audioData.size() > 0) {
 
-            _localBuffer = new AudioInjectorLocalBuffer(_audioData, this);
+            _localBuffer = new AudioInjectorLocalBuffer(_audioData);
 
             _localBuffer->open(QIODevice::ReadOnly);
             _localBuffer->setShouldLoop(_options.loop);
@@ -172,7 +172,8 @@ bool AudioInjector::injectLocally() {
             _localBuffer->setCurrentOffset(_currentSendOffset);
 
             // call this function on the AudioClient's thread
-            success = QMetaObject::invokeMethod(_localAudioInterface, "outputLocalInjector", Q_ARG(AudioInjector*, this));
+            // this will move the local buffer's thread to the LocalInjectorThread
+            success = _localAudioInterface->outputLocalInjector(this);
 
             if (!success) {
                 qCDebug(audio) << "AudioInjector::injectLocally could not output locally via _localAudioInterface";
@@ -185,6 +186,14 @@ bool AudioInjector::injectLocally() {
     return success;
 }
 
+void AudioInjector::deleteLocalBuffer() {
+    if (_localBuffer) {
+        _localBuffer->stop();
+        _localBuffer->deleteLater();
+        _localBuffer = nullptr;
+    }
+}
+
 const uchar MAX_INJECTOR_VOLUME = packFloatGainToByte(1.0f);
 static const int64_t NEXT_FRAME_DELTA_ERROR_OR_FINISHED = -1;
 static const int64_t NEXT_FRAME_DELTA_IMMEDIATELY = 0;

libraries/audio/src/AudioInjector.h

@@ -52,6 +52,7 @@ class AudioInjector : public QObject {
 public:
     AudioInjector(const Sound& sound, const AudioInjectorOptions& injectorOptions);
     AudioInjector(const QByteArray& audioData, const AudioInjectorOptions& injectorOptions);
+    ~AudioInjector();
     
     bool isFinished() const { return (stateHas(AudioInjectorState::Finished)); }
     
@@ -99,6 +100,7 @@ private:
     int64_t injectNextFrame();
     bool inject(bool(AudioInjectorManager::*injection)(AudioInjector*));
     bool injectLocally();
+    void deleteLocalBuffer();
     
     static AbstractAudioInterface* _localAudioInterface;
 

libraries/audio/src/AudioInjectorLocalBuffer.cpp

@@ -11,8 +11,7 @@
 
 #include "AudioInjectorLocalBuffer.h"
 
-AudioInjectorLocalBuffer::AudioInjectorLocalBuffer(const QByteArray& rawAudioArray, QObject* parent) :
-    QIODevice(parent),
+AudioInjectorLocalBuffer::AudioInjectorLocalBuffer(const QByteArray& rawAudioArray) :
     _rawAudioArray(rawAudioArray),
     _shouldLoop(false),
     _isStopped(false),

libraries/audio/src/AudioInjectorLocalBuffer.h

@@ -19,7 +19,7 @@
 class AudioInjectorLocalBuffer : public QIODevice {
     Q_OBJECT
 public:
-    AudioInjectorLocalBuffer(const QByteArray& rawAudioArray, QObject* parent);
+    AudioInjectorLocalBuffer(const QByteArray& rawAudioArray);
 
     void stop();