From 48af83a960bd231d2574a7de6c5a91dbc2f7c562 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Fri, 5 May 2017 17:05:40 -0400 Subject: [PATCH] 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();