From e1212c54cbc5931030aefba4132d67e07db49f0f Mon Sep 17 00:00:00 2001 From: David Kelly Date: Fri, 15 Jul 2016 09:06:13 -0700 Subject: [PATCH 1/5] Updated to deal with streaming out of both buffers So gotta keep track of when finished streaming to network and locally separately. That means the State needed to be more of a bitflag and less of an enum. --- libraries/audio-client/src/AudioClient.cpp | 1 - libraries/audio/src/AudioInjector.cpp | 105 ++++++++++++++------- libraries/audio/src/AudioInjector.h | 34 +++++-- 3 files changed, 95 insertions(+), 45 deletions(-) diff --git a/libraries/audio-client/src/AudioClient.cpp b/libraries/audio-client/src/AudioClient.cpp index dd72125d93..bec30edb4e 100644 --- a/libraries/audio-client/src/AudioClient.cpp +++ b/libraries/audio-client/src/AudioClient.cpp @@ -954,7 +954,6 @@ void AudioClient::processReceivedSamples(const QByteArray& decodedBuffer, QByteA if (hasReverb) { assert(_outputFormat.channelCount() == 2); updateReverbOptions(); - qDebug() << "handling reverb"; _listenerReverb.render(outputSamples, outputSamples, numDeviceOutputSamples/2); } } diff --git a/libraries/audio/src/AudioInjector.cpp b/libraries/audio/src/AudioInjector.cpp index 08143b8491..f94890ea7d 100644 --- a/libraries/audio/src/AudioInjector.cpp +++ b/libraries/audio/src/AudioInjector.cpp @@ -28,6 +28,15 @@ int audioInjectorPtrMetaTypeId = qRegisterMetaType(); +AudioInjectorState operator& (AudioInjectorState lhs, AudioInjectorState rhs) { + return static_cast(static_cast(lhs) | static_cast(rhs)); +}; + +AudioInjectorState& operator&= (AudioInjectorState& lhs, AudioInjectorState rhs) { + lhs = static_cast(static_cast(lhs) & static_cast(rhs)); + return lhs; +}; + AudioInjector::AudioInjector(QObject* parent) : QObject(parent) { @@ -56,10 +65,26 @@ void AudioInjector::setOptions(const AudioInjectorOptions& options) { _options.stereo = currentlyStereo; } +void AudioInjector::finishNetworkInjection() { + _state &= AudioInjectorState::NetworkInjectionFinished; + + // if we are already finished with local + // injection, then we are finished + if((_state & AudioInjectorState::LocalInjectionFinished) == AudioInjectorState::LocalInjectionFinished) { + finish(); + } +} + +void AudioInjector::finishLocalInjection() { + _state &= AudioInjectorState::LocalInjectionFinished; + if(_options.localOnly || ((_state & AudioInjectorState::NetworkInjectionFinished) == AudioInjectorState::NetworkInjectionFinished)) { + finish(); + } +} void AudioInjector::finish() { - bool shouldDelete = (_state == State::NotFinishedWithPendingDelete); - _state = State::Finished; + bool shouldDelete = ((_state & AudioInjectorState::PendingDelete) == AudioInjectorState::PendingDelete); + _state &= AudioInjectorState::Finished; emit finished(); @@ -121,23 +146,31 @@ void AudioInjector::restart() { _hasSentFirstFrame = false; // check our state to decide if we need extra handling for the restart request - if (_state == State::Finished) { + if ((_state & AudioInjectorState::Finished) == AudioInjectorState::Finished) { // we finished playing, need to reset state so we can get going again _hasSetup = false; _shouldStop = false; - _state = State::NotFinished; + _state = AudioInjectorState::NotFinished; // call inject audio to start injection over again setupInjection(); - // if we're a local injector, just inject again - if (_options.localOnly) { - injectLocally(); - } else { - // wake the AudioInjectorManager back up if it's stuck waiting - if (!injectorManager->restartFinishedInjector(this)) { - _state = State::Finished; // we're not playing, so reset the state used by isPlaying. + // inject locally + if(injectLocally()) { + + // if not localOnly, wake the AudioInjectorManager back up if it is stuck waiting + if (!_options.localOnly) { + + if (!injectorManager->restartFinishedInjector(this)) { + // TODO: this logic seems to remove the pending delete, + // which makes me wonder about the deleteLater calls + _state = AudioInjectorState::Finished; // we're not playing, so reset the state used by isPlaying. + } } + } else { + // TODO: this logic seems to remove the pending delete, + // which makes me wonder about the deleteLater calls + _state = AudioInjectorState::Finished; // we failed to play, so we are finished again } } } @@ -183,7 +216,7 @@ static const int64_t NEXT_FRAME_DELTA_ERROR_OR_FINISHED = -1; static const int64_t NEXT_FRAME_DELTA_IMMEDIATELY = 0; int64_t AudioInjector::injectNextFrame() { - if (_state == AudioInjector::State::Finished) { + if ((_state & AudioInjectorState::NetworkInjectionFinished) == AudioInjectorState::NetworkInjectionFinished) { qDebug() << "AudioInjector::injectNextFrame called but AudioInjector has finished and was not restarted. Returning."; return NEXT_FRAME_DELTA_ERROR_OR_FINISHED; } @@ -234,8 +267,10 @@ int64_t AudioInjector::injectNextFrame() { // pack the stereo/mono type of the stream audioPacketStream << _options.stereo; - // pack the flag for loopback - uchar loopbackFlag = (uchar)true; + // pack the flag for loopback. Now, we don't loopback + // and _always_ play locally, so loopbackFlag should be + // false always. + uchar loopbackFlag = (uchar)false; audioPacketStream << loopbackFlag; // pack the position for injected audio @@ -333,7 +368,7 @@ int64_t AudioInjector::injectNextFrame() { } if (_currentSendOffset >= _audioData.size() && !_options.loop) { - finish(); + finishNetworkInjection(); return NEXT_FRAME_DELTA_ERROR_OR_FINISHED; } @@ -372,10 +407,10 @@ void AudioInjector::triggerDeleteAfterFinish() { return; } - if (_state == State::Finished) { + if (_state == AudioInjectorState::Finished) { stopAndDeleteLater(); } else { - _state = State::NotFinishedWithPendingDelete; + _state &= AudioInjectorState::PendingDelete; } } @@ -421,7 +456,7 @@ AudioInjector* AudioInjector::playSoundAndDelete(const QByteArray& buffer, const AudioInjector* sound = playSound(buffer, options, localInterface); if (sound) { - sound->_state = AudioInjector::State::NotFinishedWithPendingDelete; + sound->_state &= AudioInjectorState::PendingDelete; } return sound; @@ -438,21 +473,23 @@ AudioInjector* AudioInjector::playSound(const QByteArray& buffer, const AudioInj // setup parameters required for injection injector->setupInjection(); - if (options.localOnly) { - if (injector->injectLocally()) { - // local injection succeeded, return the pointer to injector - return injector; - } else { - // unable to inject locally, return a nullptr - return nullptr; - } - } else { - // attempt to thread the new injector - if (injectorManager->threadInjector(injector)) { - return injector; - } else { - // we failed to thread the new injector (we are at the max number of injector threads) - return nullptr; - } + // we always inject locally + // + if (!injector->injectLocally()) { + // failed, so don't bother sending to server + qDebug() << "AudioInjector::playSound failed to inject locally"; + return nullptr; } + // if localOnly, we are done, just return injector. + if (options.localOnly) { + return injector; + } + + // send off to server for everyone else + if (!injectorManager->threadInjector(injector)) { + // we failed to thread the new injector (we are at the max number of injector threads) + qDebug() << "AudioInjector::playSound failed to thread injector"; + } + return injector; + } diff --git a/libraries/audio/src/AudioInjector.h b/libraries/audio/src/AudioInjector.h index 1af733ace6..e13fc15c26 100644 --- a/libraries/audio/src/AudioInjector.h +++ b/libraries/audio/src/AudioInjector.h @@ -32,24 +32,35 @@ class AbstractAudioInterface; class AudioInjectorManager; + +enum class AudioInjectorState : uint8_t { + NotFinished = 1, + Finished = 2, + PendingDelete = 4, + LocalInjectionFinished = 8, + NetworkInjectionFinished = 16 +}; + +AudioInjectorState operator& (AudioInjectorState lhs, AudioInjectorState rhs); +AudioInjectorState& operator&= (AudioInjectorState& lhs, AudioInjectorState rhs); + // In order to make scripting cleaner for the AudioInjector, the script now holds on to the AudioInjector object // until it dies. - class AudioInjector : public QObject { Q_OBJECT public: - enum class State : uint8_t { - NotFinished, - NotFinishedWithPendingDelete, - Finished - }; - + static const uint8_t NotFinished = 1; + static const uint8_t Finished = 2; + static const uint8_t PendingDelete = 4; + static const uint8_t LocalInjectionFinished = 8; + static const uint8_t NetworkInjectionFinished = 16; + AudioInjector(QObject* parent); AudioInjector(const Sound& sound, const AudioInjectorOptions& injectorOptions); AudioInjector(const QByteArray& audioData, const AudioInjectorOptions& injectorOptions); - bool isFinished() const { return _state == State::Finished; } + bool isFinished() const { return (_state & AudioInjectorState::Finished) == AudioInjectorState::Finished; } int getCurrentSendOffset() const { return _currentSendOffset; } void setCurrentSendOffset(int currentSendOffset) { _currentSendOffset = currentSendOffset; } @@ -78,8 +89,10 @@ public slots: void setOptions(const AudioInjectorOptions& options); float getLoudness() const { return _loudness; } - bool isPlaying() const { return _state == State::NotFinished || _state == State::NotFinishedWithPendingDelete; } + bool isPlaying() const { return (_state & AudioInjectorState::NotFinished) == AudioInjectorState::NotFinished; } void finish(); + void finishLocalInjection(); + void finishNetworkInjection(); signals: void finished(); @@ -92,7 +105,7 @@ private: QByteArray _audioData; AudioInjectorOptions _options; - State _state { State::NotFinished }; + AudioInjectorState _state { AudioInjectorState::NotFinished }; bool _hasSentFirstFrame { false }; bool _hasSetup { false }; bool _shouldStop { false }; @@ -111,4 +124,5 @@ private: friend class AudioInjectorManager; }; + #endif // hifi_AudioInjector_h From 8df4ed01d92b26411f1d7c2e7e565792aa411dc3 Mon Sep 17 00:00:00 2001 From: David Kelly Date: Fri, 15 Jul 2016 16:22:41 -0700 Subject: [PATCH 2/5] fixed typo which oddly I was sure I already did. Seems I didn't push it? OK now it is pushed. --- libraries/audio/src/AudioInjector.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/audio/src/AudioInjector.cpp b/libraries/audio/src/AudioInjector.cpp index f94890ea7d..0b0be9353b 100644 --- a/libraries/audio/src/AudioInjector.cpp +++ b/libraries/audio/src/AudioInjector.cpp @@ -29,7 +29,7 @@ int audioInjectorPtrMetaTypeId = qRegisterMetaType(); AudioInjectorState operator& (AudioInjectorState lhs, AudioInjectorState rhs) { - return static_cast(static_cast(lhs) | static_cast(rhs)); + return static_cast(static_cast(lhs) & static_cast(rhs)); }; AudioInjectorState& operator&= (AudioInjectorState& lhs, AudioInjectorState rhs) { From fb99828e30302f2c615e74926dbe62a647def972 Mon Sep 17 00:00:00 2001 From: David Kelly Date: Fri, 15 Jul 2016 17:09:27 -0700 Subject: [PATCH 3/5] PR feedback Code a bit more readable. Sadly (and I guess it makes sense), a enum class XXX is not a class, so you cannot have member functions for it. I can imagine no way to have a vtable if you are really representing it as a uint8_t or whatever. So, I put a stateHas function in the AudioInjector instead. Definite improvement. --- libraries/audio/src/AudioInjector.cpp | 15 +++++++++------ libraries/audio/src/AudioInjector.h | 5 +++-- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/libraries/audio/src/AudioInjector.cpp b/libraries/audio/src/AudioInjector.cpp index 0b0be9353b..e992e3c541 100644 --- a/libraries/audio/src/AudioInjector.cpp +++ b/libraries/audio/src/AudioInjector.cpp @@ -57,6 +57,10 @@ AudioInjector::AudioInjector(const QByteArray& audioData, const AudioInjectorOpt } +bool AudioInjector::stateHas(AudioInjectorState state) const { + return (_state & state) == state; +} + void AudioInjector::setOptions(const AudioInjectorOptions& options) { // since options.stereo is computed from the audio stream, // we need to copy it from existing options just in case. @@ -70,20 +74,19 @@ void AudioInjector::finishNetworkInjection() { // if we are already finished with local // injection, then we are finished - if((_state & AudioInjectorState::LocalInjectionFinished) == AudioInjectorState::LocalInjectionFinished) { + if(stateHas(AudioInjectorState::LocalInjectionFinished)) { finish(); } } void AudioInjector::finishLocalInjection() { _state &= AudioInjectorState::LocalInjectionFinished; - if(_options.localOnly || ((_state & AudioInjectorState::NetworkInjectionFinished) == AudioInjectorState::NetworkInjectionFinished)) { + if(_options.localOnly || stateHas(AudioInjectorState::NetworkInjectionFinished)) { finish(); } } void AudioInjector::finish() { - bool shouldDelete = ((_state & AudioInjectorState::PendingDelete) == AudioInjectorState::PendingDelete); _state &= AudioInjectorState::Finished; emit finished(); @@ -94,7 +97,7 @@ void AudioInjector::finish() { _localBuffer = NULL; } - if (shouldDelete) { + if (stateHas(AudioInjectorState::PendingDelete)) { // we've been asked to delete after finishing, trigger a deleteLater here deleteLater(); } @@ -146,7 +149,7 @@ void AudioInjector::restart() { _hasSentFirstFrame = false; // check our state to decide if we need extra handling for the restart request - if ((_state & AudioInjectorState::Finished) == AudioInjectorState::Finished) { + if (stateHas(AudioInjectorState::Finished)) { // we finished playing, need to reset state so we can get going again _hasSetup = false; _shouldStop = false; @@ -216,7 +219,7 @@ static const int64_t NEXT_FRAME_DELTA_ERROR_OR_FINISHED = -1; static const int64_t NEXT_FRAME_DELTA_IMMEDIATELY = 0; int64_t AudioInjector::injectNextFrame() { - if ((_state & AudioInjectorState::NetworkInjectionFinished) == AudioInjectorState::NetworkInjectionFinished) { + if (stateHas(AudioInjectorState::NetworkInjectionFinished)) { qDebug() << "AudioInjector::injectNextFrame called but AudioInjector has finished and was not restarted. Returning."; return NEXT_FRAME_DELTA_ERROR_OR_FINISHED; } diff --git a/libraries/audio/src/AudioInjector.h b/libraries/audio/src/AudioInjector.h index e13fc15c26..ce0c88247e 100644 --- a/libraries/audio/src/AudioInjector.h +++ b/libraries/audio/src/AudioInjector.h @@ -60,7 +60,7 @@ public: AudioInjector(const Sound& sound, const AudioInjectorOptions& injectorOptions); AudioInjector(const QByteArray& audioData, const AudioInjectorOptions& injectorOptions); - bool isFinished() const { return (_state & AudioInjectorState::Finished) == AudioInjectorState::Finished; } + bool isFinished() const { return (stateHas(AudioInjectorState::Finished)); } int getCurrentSendOffset() const { return _currentSendOffset; } void setCurrentSendOffset(int currentSendOffset) { _currentSendOffset = currentSendOffset; } @@ -74,6 +74,7 @@ public: bool isStereo() const { return _options.stereo; } void setLocalAudioInterface(AbstractAudioInterface* localAudioInterface) { _localAudioInterface = localAudioInterface; } + bool stateHas(AudioInjectorState state) const ; static AudioInjector* playSoundAndDelete(const QByteArray& buffer, const AudioInjectorOptions options, AbstractAudioInterface* localInterface); static AudioInjector* playSound(const QByteArray& buffer, const AudioInjectorOptions options, AbstractAudioInterface* localInterface); static AudioInjector* playSound(SharedSoundPointer sound, const float volume, const float stretchFactor, const glm::vec3 position); @@ -89,7 +90,7 @@ public slots: void setOptions(const AudioInjectorOptions& options); float getLoudness() const { return _loudness; } - bool isPlaying() const { return (_state & AudioInjectorState::NotFinished) == AudioInjectorState::NotFinished; } + bool isPlaying() const { return stateHas(AudioInjectorState::NotFinished); } void finish(); void finishLocalInjection(); void finishNetworkInjection(); From 8c0eb1e4d27f4d925317a09fedf7632138f9172e Mon Sep 17 00:00:00 2001 From: David Kelly Date: Fri, 15 Jul 2016 19:48:15 -0700 Subject: [PATCH 4/5] Fixed some bad logic When I "fixed" my or instead of and issue, I did it in the wrong direction. But it looked right :) Now it is. Sigh. Long story how it got there, but it seems good now. --- libraries/audio/src/AudioInjector.cpp | 14 +++++++------- libraries/audio/src/AudioInjector.h | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/libraries/audio/src/AudioInjector.cpp b/libraries/audio/src/AudioInjector.cpp index e992e3c541..9c49ce66d8 100644 --- a/libraries/audio/src/AudioInjector.cpp +++ b/libraries/audio/src/AudioInjector.cpp @@ -32,8 +32,8 @@ AudioInjectorState operator& (AudioInjectorState lhs, AudioInjectorState rhs) { return static_cast(static_cast(lhs) & static_cast(rhs)); }; -AudioInjectorState& operator&= (AudioInjectorState& lhs, AudioInjectorState rhs) { - lhs = static_cast(static_cast(lhs) & static_cast(rhs)); +AudioInjectorState& operator|= (AudioInjectorState& lhs, AudioInjectorState rhs) { + lhs = static_cast(static_cast(lhs) | static_cast(rhs)); return lhs; }; @@ -70,7 +70,7 @@ void AudioInjector::setOptions(const AudioInjectorOptions& options) { } void AudioInjector::finishNetworkInjection() { - _state &= AudioInjectorState::NetworkInjectionFinished; + _state |= AudioInjectorState::NetworkInjectionFinished; // if we are already finished with local // injection, then we are finished @@ -80,14 +80,14 @@ void AudioInjector::finishNetworkInjection() { } void AudioInjector::finishLocalInjection() { - _state &= AudioInjectorState::LocalInjectionFinished; + _state |= AudioInjectorState::LocalInjectionFinished; if(_options.localOnly || stateHas(AudioInjectorState::NetworkInjectionFinished)) { finish(); } } void AudioInjector::finish() { - _state &= AudioInjectorState::Finished; + _state |= AudioInjectorState::Finished; emit finished(); @@ -413,7 +413,7 @@ void AudioInjector::triggerDeleteAfterFinish() { if (_state == AudioInjectorState::Finished) { stopAndDeleteLater(); } else { - _state &= AudioInjectorState::PendingDelete; + _state |= AudioInjectorState::PendingDelete; } } @@ -459,7 +459,7 @@ AudioInjector* AudioInjector::playSoundAndDelete(const QByteArray& buffer, const AudioInjector* sound = playSound(buffer, options, localInterface); if (sound) { - sound->_state &= AudioInjectorState::PendingDelete; + sound->_state |= AudioInjectorState::PendingDelete; } return sound; diff --git a/libraries/audio/src/AudioInjector.h b/libraries/audio/src/AudioInjector.h index ce0c88247e..9bdfcacb5c 100644 --- a/libraries/audio/src/AudioInjector.h +++ b/libraries/audio/src/AudioInjector.h @@ -42,7 +42,7 @@ enum class AudioInjectorState : uint8_t { }; AudioInjectorState operator& (AudioInjectorState lhs, AudioInjectorState rhs); -AudioInjectorState& operator&= (AudioInjectorState& lhs, AudioInjectorState rhs); +AudioInjectorState& operator|= (AudioInjectorState& lhs, AudioInjectorState rhs); // In order to make scripting cleaner for the AudioInjector, the script now holds on to the AudioInjector object // until it dies. From 3df373252f8cbe7ae7e227e5bd545a7472085886 Mon Sep 17 00:00:00 2001 From: David Kelly Date: Mon, 18 Jul 2016 14:00:41 -0700 Subject: [PATCH 5/5] Several minor things We could only partially fill the _scratchBuffer - .wav files may not be exactly N frames long. Doh. While at it, I needed to call finishLocalInjection() after local injectors are done, and the access to the injector vector needs to be locked, given that we do a QtDirectConnection with the networking and thus the outputLocalInjectors is on a different thread. The clicking was just 0-ing out the _scratchBuffer. --- libraries/audio-client/src/AudioClient.cpp | 9 +++++++-- libraries/audio-client/src/AudioClient.h | 5 +++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/libraries/audio-client/src/AudioClient.cpp b/libraries/audio-client/src/AudioClient.cpp index b5a7c8c0cf..7cf8574529 100644 --- a/libraries/audio-client/src/AudioClient.cpp +++ b/libraries/audio-client/src/AudioClient.cpp @@ -862,6 +862,9 @@ void AudioClient::mixLocalAudioInjectors(int16_t* inputBuffer) { static const float INT16_TO_FLOAT_SCALE_FACTOR = 1/32768.0f; bool injectorsHaveData = false; + + // lock the injector vector + Lock lock(_injectorsMutex); for (AudioInjector* injector : getActiveLocalAudioInjectors()) { if (injector->getLocalBuffer()) { @@ -871,6 +874,7 @@ void AudioClient::mixLocalAudioInjectors(int16_t* inputBuffer) { AudioConstants::NETWORK_FRAME_BYTES_PER_CHANNEL; // get one frame from the injector (mono or stereo) + memset(_scratchBuffer, 0, sizeof(_scratchBuffer)); if (0 < injector->getLocalBuffer()->readData((char*)_scratchBuffer, samplesToRead)) { injectorsHaveData = true; @@ -894,14 +898,14 @@ void AudioClient::mixLocalAudioInjectors(int16_t* inputBuffer) { } else { qDebug() << "injector has no more data, marking finished for removal"; - injector->finish(); + injector->finishLocalInjection(); injectorsToRemove.append(injector); } } else { qDebug() << "injector has no local buffer, marking as finished for removal"; - injector->finish(); + injector->finishLocalInjection(); injectorsToRemove.append(injector); } } @@ -1003,6 +1007,7 @@ void AudioClient::setIsStereoInput(bool isStereoInput) { bool AudioClient::outputLocalInjector(bool isStereo, AudioInjector* injector) { + Lock lock(_injectorsMutex); if (injector->getLocalBuffer() && _audioInput ) { // just add it to the vector of active local injectors, if // not already there. diff --git a/libraries/audio-client/src/AudioClient.h b/libraries/audio-client/src/AudioClient.h index 3e4aa931a6..472092163b 100644 --- a/libraries/audio-client/src/AudioClient.h +++ b/libraries/audio-client/src/AudioClient.h @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -83,6 +84,9 @@ public: using AudioPositionGetter = std::function; using AudioOrientationGetter = std::function; + using Mutex = std::mutex; + using Lock = std::unique_lock; + class AudioOutputIODevice : public QIODevice { public: AudioOutputIODevice(MixedProcessedAudioStream& receivedAudioStream, AudioClient* audio) : @@ -219,6 +223,7 @@ private: float azimuthForSource(const glm::vec3& relativePosition); float gainForSource(float distance, float volume); + Mutex _injectorsMutex; QByteArray firstInputFrame; QAudioInput* _audioInput; QAudioFormat _desiredInputFormat;