From e882e7b648460a67c8a75eb183f740e0478d7903 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Fri, 5 May 2017 15:36:38 -0400 Subject: [PATCH 1/5] rm extra lock in audio read --- libraries/audio-client/src/AudioClient.cpp | 10 +++++++--- libraries/audio-client/src/AudioClient.h | 4 +--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/libraries/audio-client/src/AudioClient.cpp b/libraries/audio-client/src/AudioClient.cpp index b684aac89c..fb0220c24d 100644 --- a/libraries/audio-client/src/AudioClient.cpp +++ b/libraries/audio-client/src/AudioClient.cpp @@ -1115,7 +1115,7 @@ void AudioClient::prepareLocalAudioInjectors() { 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); + Lock lock(_localAudioMutex); samplesNeeded = bufferCapacity - _localSamplesAvailable.load(std::memory_order_relaxed); if (samplesNeeded <= 0) { @@ -1452,7 +1452,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 +1681,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); diff --git a/libraries/audio-client/src/AudioClient.h b/libraries/audio-client/src/AudioClient.h index 139749e8e8..aaedee7456 100644 --- a/libraries/audio-client/src/AudioClient.h +++ b/libraries/audio-client/src/AudioClient.h @@ -96,8 +96,6 @@ public: using AudioPositionGetter = std::function; using AudioOrientationGetter = std::function; - using RecursiveMutex = std::recursive_mutex; - using RecursiveLock = std::unique_lock; using Mutex = std::mutex; using Lock = std::unique_lock; @@ -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 }; From cdf6c332a6b347eea32eeb645f5ae031edafac0c Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Fri, 5 May 2017 15:42:22 -0400 Subject: [PATCH 2/5] prevent overflow of local injectors buffer --- libraries/audio-client/src/AudioClient.cpp | 32 +++++++++++----------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/libraries/audio-client/src/AudioClient.cpp b/libraries/audio-client/src/AudioClient.cpp index fb0220c24d..f61429812f 100644 --- a/libraries/audio-client/src/AudioClient.cpp +++ b/libraries/audio-client/src/AudioClient.cpp @@ -1097,26 +1097,26 @@ 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::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 + // 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(); + 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; + } + samplesNeeded = bufferCapacity - _localSamplesAvailable.load(std::memory_order_relaxed); if (samplesNeeded <= 0) { break; From 48af83a960bd231d2574a7de6c5a91dbc2f7c562 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Fri, 5 May 2017 17:05:40 -0400 Subject: [PATCH 3/5] guard against audio injector and local buffer datarace --- libraries/audio-client/src/AudioClient.cpp | 24 +++++++++++-------- libraries/audio/src/AbstractAudioInterface.h | 4 ++++ libraries/audio/src/AudioInjector.cpp | 23 ++++++++++++------ libraries/audio/src/AudioInjector.h | 2 ++ .../audio/src/AudioInjectorLocalBuffer.cpp | 3 +-- .../audio/src/AudioInjectorLocalBuffer.h | 2 +- 6 files changed, 38 insertions(+), 20 deletions(-) diff --git a/libraries/audio-client/src/AudioClient.cpp b/libraries/audio-client/src/AudioClient.cpp index f61429812f..0710250c6a 100644 --- a/libraries/audio-client/src/AudioClient.cpp +++ b/libraries/audio-client/src/AudioClient.cpp @@ -1168,16 +1168,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; + if ((injectorBuffer = injector->getLocalBuffer())) { 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 +1319,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; + if ((injectorBuffer = injector->getLocalBuffer())) { + // 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 +1337,7 @@ bool AudioClient::outputLocalInjector(AudioInjector* injector) { return true; } else { - // no local buffer or audio + // no local buffer return false; } } diff --git a/libraries/audio/src/AbstractAudioInterface.h b/libraries/audio/src/AbstractAudioInterface.h index d61b8d60ca..2e4611cd4e 100644 --- a/libraries/audio/src/AbstractAudioInterface.h +++ b/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; diff --git a/libraries/audio/src/AudioInjector.cpp b/libraries/audio/src/AudioInjector.cpp index 86cbc98d84..ce98225190 100644 --- a/libraries/audio/src/AudioInjector.cpp +++ b/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; diff --git a/libraries/audio/src/AudioInjector.h b/libraries/audio/src/AudioInjector.h index 7d57708738..a901c2520f 100644 --- a/libraries/audio/src/AudioInjector.h +++ b/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; diff --git a/libraries/audio/src/AudioInjectorLocalBuffer.cpp b/libraries/audio/src/AudioInjectorLocalBuffer.cpp index 049adb0cc6..7834644baf 100644 --- a/libraries/audio/src/AudioInjectorLocalBuffer.cpp +++ b/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), diff --git a/libraries/audio/src/AudioInjectorLocalBuffer.h b/libraries/audio/src/AudioInjectorLocalBuffer.h index 07d8ae5b9f..673966ff09 100644 --- a/libraries/audio/src/AudioInjectorLocalBuffer.h +++ b/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(); From ed3fc004a6a2a1f55026e01032fbbc2fbf7c0abc Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Fri, 5 May 2017 18:15:56 -0400 Subject: [PATCH 4/5] avoid assignment-as-test --- libraries/audio-client/src/AudioClient.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libraries/audio-client/src/AudioClient.cpp b/libraries/audio-client/src/AudioClient.cpp index 0710250c6a..927000d614 100644 --- a/libraries/audio-client/src/AudioClient.cpp +++ b/libraries/audio-client/src/AudioClient.cpp @@ -1169,8 +1169,8 @@ bool AudioClient::mixLocalAudioInjectors(float* mixBuffer) { for (AudioInjector* injector : _activeLocalAudioInjectors) { // the lock guarantees that injectorBuffer, if found, is invariant - AudioInjectorLocalBuffer* injectorBuffer; - if ((injectorBuffer = injector->getLocalBuffer())) { + AudioInjectorLocalBuffer* injectorBuffer = injector->getLocalBuffer(); + if (injectorBuffer) { static const int HRTF_DATASET_INDEX = 1; @@ -1319,8 +1319,8 @@ void AudioClient::setIsStereoInput(bool isStereoInput) { } bool AudioClient::outputLocalInjector(AudioInjector* injector) { - AudioInjectorLocalBuffer* injectorBuffer; - if ((injectorBuffer = injector->getLocalBuffer())) { + AudioInjectorLocalBuffer* injectorBuffer = injector->getLocalBuffer(); + if (injectorBuffer) { // local injectors are on the AudioInjectorsThread, so we must guard access Lock lock(_injectorsMutex); if (!_activeLocalAudioInjectors.contains(injector)) { From 7db404eba01c7af86a400f4384523775e54b27cc Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Fri, 5 May 2017 18:16:37 -0400 Subject: [PATCH 5/5] recast avoid overwriting buffer logic --- libraries/audio-client/src/AudioClient.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/libraries/audio-client/src/AudioClient.cpp b/libraries/audio-client/src/AudioClient.cpp index 927000d614..680e9129aa 100644 --- a/libraries/audio-client/src/AudioClient.cpp +++ b/libraries/audio-client/src/AudioClient.cpp @@ -1108,17 +1108,16 @@ void AudioClient::prepareLocalAudioInjectors() { } int bufferCapacity = _localInjectorsStream.getSampleCapacity(); + int maxOutputSamples = AudioConstants::NETWORK_FRAME_SAMPLES_PER_CHANNEL * AudioConstants::STEREO; if (_localToOutputResampler) { - // avoid overwriting the buffer, - // instead of failing on writes because the buffer is used as a lock-free pipe - bufferCapacity -= + maxOutputSamples = _localToOutputResampler->getMaxOutput(AudioConstants::NETWORK_FRAME_SAMPLES_PER_CHANNEL) * AudioConstants::STEREO; - bufferCapacity += 1; } samplesNeeded = bufferCapacity - _localSamplesAvailable.load(std::memory_order_relaxed); - if (samplesNeeded <= 0) { + if (samplesNeeded < maxOutputSamples) { + // avoid overwriting the buffer to prevent losing frames break; }