From 4a4a92c009662daced888d38a26cb141025a17d0 Mon Sep 17 00:00:00 2001 From: Roxanne Skelly Date: Mon, 6 May 2019 15:46:06 -0700 Subject: [PATCH 1/4] BUGZ-85 - audio pipeline interpolation on ring buffer starve Kick the PLC so that it generates a more pleasing 'fade' frame when ring buffer starves. --- libraries/audio/src/AudioSRC.cpp | 4 ++-- libraries/audio/src/AudioSRC.h | 3 +++ libraries/audio/src/InboundAudioStream.cpp | 11 +++++++---- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/libraries/audio/src/AudioSRC.cpp b/libraries/audio/src/AudioSRC.cpp index d488eccb6a..1accc8565d 100644 --- a/libraries/audio/src/AudioSRC.cpp +++ b/libraries/audio/src/AudioSRC.cpp @@ -1491,7 +1491,7 @@ AudioSRC::~AudioSRC() { // int AudioSRC::render(const int16_t* input, int16_t* output, int inputFrames) { int outputFrames = 0; - + QMutexLocker lock(&_renderMutex); while (inputFrames) { int ni = MIN(inputFrames, _inputBlock); @@ -1516,7 +1516,7 @@ int AudioSRC::render(const int16_t* input, int16_t* output, int inputFrames) { // int AudioSRC::render(const float* input, float* output, int inputFrames) { int outputFrames = 0; - + QMutexLocker lock(&_renderMutex); while (inputFrames) { int ni = MIN(inputFrames, _inputBlock); diff --git a/libraries/audio/src/AudioSRC.h b/libraries/audio/src/AudioSRC.h index d5971e5838..5299d9da7f 100644 --- a/libraries/audio/src/AudioSRC.h +++ b/libraries/audio/src/AudioSRC.h @@ -13,6 +13,7 @@ #define hifi_AudioSRC_h #include +#include static const int SRC_MAX_CHANNELS = 4; @@ -55,6 +56,8 @@ public: int getMaxInput(int outputFrames); private: + QMutex _renderMutex; + float* _polyphaseFilter; int* _stepTable; diff --git a/libraries/audio/src/InboundAudioStream.cpp b/libraries/audio/src/InboundAudioStream.cpp index 8c5388e222..9844159c1a 100644 --- a/libraries/audio/src/InboundAudioStream.cpp +++ b/libraries/audio/src/InboundAudioStream.cpp @@ -215,7 +215,6 @@ int InboundAudioStream::parseData(ReceivedMessage& message) { if (framesAvailable > _desiredJitterBufferFrames + MAX_FRAMES_OVER_DESIRED) { int framesToDrop = framesAvailable - (_desiredJitterBufferFrames + DESIRED_JITTER_BUFFER_FRAMES_PADDING); _ringBuffer.shiftReadPosition(framesToDrop * _ringBuffer.getNumFrameSamples()); - _framesAvailableStat.reset(); _currentJitterBufferFrames = 0; @@ -250,7 +249,7 @@ int InboundAudioStream::lostAudioData(int numPackets) { if (_decoder) { _decoder->lostFrame(decodedBuffer); } else { - decodedBuffer.resize(AudioConstants::NETWORK_FRAME_BYTES_STEREO); + decodedBuffer.resize(AudioConstants::NETWORK_FRAME_BYTES_PER_CHANNEL * _numChannels); memset(decodedBuffer.data(), 0, decodedBuffer.size()); } _ringBuffer.writeData(decodedBuffer.data(), decodedBuffer.size()); @@ -338,10 +337,14 @@ int InboundAudioStream::popSamples(int maxSamples, bool allOrNothing) { popSamplesNoCheck(samplesAvailable); samplesPopped = samplesAvailable; } else { - // we can't pop any samples, set this stream to starved + // we can't pop any samples, set this stream to starved for jitter + // buffer calculations. setToStarved(); _consecutiveNotMixedCount++; - _lastPopSucceeded = false; + //Kick PLC to generate a filler frame, reducing 'click' + lostAudioData(allOrNothing ? (maxSamples - samplesAvailable) / _ringBuffer.getNumFrameSamples() : 1); + samplesPopped = _ringBuffer.samplesAvailable(); + popSamplesNoCheck(samplesPopped); } } return samplesPopped; From dc7ec355444cc78307661576a3d2d499de7f7ac2 Mon Sep 17 00:00:00 2001 From: Roxanne Skelly Date: Wed, 8 May 2019 16:17:18 -0700 Subject: [PATCH 2/4] BUGZ-85 - handle thread safety issues with calling the decoder from the real-time thread. --- .../src/audio/AvatarAudioStream.cpp | 1 + libraries/audio/src/AudioSRC.cpp | 2 - libraries/audio/src/AudioSRC.h | 2 - libraries/audio/src/InboundAudioStream.cpp | 54 +++++++++++++++---- libraries/audio/src/InboundAudioStream.h | 1 + .../audio/src/MixedProcessedAudioStream.cpp | 15 +++++- 6 files changed, 59 insertions(+), 16 deletions(-) diff --git a/assignment-client/src/audio/AvatarAudioStream.cpp b/assignment-client/src/audio/AvatarAudioStream.cpp index 1b3ca9a8b1..24b14ac9e5 100644 --- a/assignment-client/src/audio/AvatarAudioStream.cpp +++ b/assignment-client/src/audio/AvatarAudioStream.cpp @@ -45,6 +45,7 @@ int AvatarAudioStream::parseStreamProperties(PacketType type, const QByteArray& : AudioConstants::NETWORK_FRAME_SAMPLES_PER_CHANNEL); // restart the codec if (_codec) { + QMutexLocker lock(&_decoderMutex); if (_decoder) { _codec->releaseDecoder(_decoder); } diff --git a/libraries/audio/src/AudioSRC.cpp b/libraries/audio/src/AudioSRC.cpp index 1accc8565d..867416871e 100644 --- a/libraries/audio/src/AudioSRC.cpp +++ b/libraries/audio/src/AudioSRC.cpp @@ -1491,7 +1491,6 @@ AudioSRC::~AudioSRC() { // int AudioSRC::render(const int16_t* input, int16_t* output, int inputFrames) { int outputFrames = 0; - QMutexLocker lock(&_renderMutex); while (inputFrames) { int ni = MIN(inputFrames, _inputBlock); @@ -1516,7 +1515,6 @@ int AudioSRC::render(const int16_t* input, int16_t* output, int inputFrames) { // int AudioSRC::render(const float* input, float* output, int inputFrames) { int outputFrames = 0; - QMutexLocker lock(&_renderMutex); while (inputFrames) { int ni = MIN(inputFrames, _inputBlock); diff --git a/libraries/audio/src/AudioSRC.h b/libraries/audio/src/AudioSRC.h index 5299d9da7f..111c2105b7 100644 --- a/libraries/audio/src/AudioSRC.h +++ b/libraries/audio/src/AudioSRC.h @@ -13,7 +13,6 @@ #define hifi_AudioSRC_h #include -#include static const int SRC_MAX_CHANNELS = 4; @@ -56,7 +55,6 @@ public: int getMaxInput(int outputFrames); private: - QMutex _renderMutex; float* _polyphaseFilter; int* _stepTable; diff --git a/libraries/audio/src/InboundAudioStream.cpp b/libraries/audio/src/InboundAudioStream.cpp index 9844159c1a..482991626a 100644 --- a/libraries/audio/src/InboundAudioStream.cpp +++ b/libraries/audio/src/InboundAudioStream.cpp @@ -246,12 +246,20 @@ int InboundAudioStream::lostAudioData(int numPackets) { QByteArray decodedBuffer; while (numPackets--) { + if (!_decoderMutex.tryLock()) { + // an incoming packet is being processed, + // and will likely be on the ring buffer shortly, + // so don't bother generating more data + qCInfo(audiostream, "Packet currently being unpacked or lost frame already being generated. Not generating lost frame."); + return 0; + } if (_decoder) { _decoder->lostFrame(decodedBuffer); } else { decodedBuffer.resize(AudioConstants::NETWORK_FRAME_BYTES_PER_CHANNEL * _numChannels); memset(decodedBuffer.data(), 0, decodedBuffer.size()); } + _decoderMutex.unlock(); _ringBuffer.writeData(decodedBuffer.data(), decodedBuffer.size()); } return 0; @@ -259,6 +267,12 @@ int InboundAudioStream::lostAudioData(int numPackets) { int InboundAudioStream::parseAudioData(PacketType type, const QByteArray& packetAfterStreamProperties) { QByteArray decodedBuffer; + + // may block on the real-time thread, which is acceptible as + // parseAudioData is only called by the packet processing + // thread which, while high performance, is not as sensitive to + // delays as the real-time thread. + QMutexLocker lock(&_decoderMutex); if (_decoder) { _decoder->decode(packetAfterStreamProperties, decodedBuffer); } else { @@ -277,16 +291,23 @@ int InboundAudioStream::writeDroppableSilentFrames(int silentFrames) { // case we will call the decoder's lostFrame() method, which indicates // that it should interpolate from its last known state down toward // silence. - if (_decoder) { - // FIXME - We could potentially use the output from the codec, in which - // case we might get a cleaner fade toward silence. NOTE: The below logic - // attempts to catch up in the event that the jitter buffers have grown. - // The better long term fix is to use the output from the decode, detect - // when it actually reaches silence, and then delete the silent portions - // of the jitter buffers. Or petentially do a cross fade from the decode - // output to silence. - QByteArray decodedBuffer; - _decoder->lostFrame(decodedBuffer); + { + // may block on the real-time thread, which is acceptible as + // writeDroppableSilentFrames is only called by the packet processing + // thread which, while high performance, is not as sensitive to + // delays as the real-time thread. + QMutexLocker lock(&_decoderMutex); + if (_decoder) { + // FIXME - We could potentially use the output from the codec, in which + // case we might get a cleaner fade toward silence. NOTE: The below logic + // attempts to catch up in the event that the jitter buffers have grown. + // The better long term fix is to use the output from the decode, detect + // when it actually reaches silence, and then delete the silent portions + // of the jitter buffers. Or petentially do a cross fade from the decode + // output to silence. + QByteArray decodedBuffer; + _decoder->lostFrame(decodedBuffer); + } } // calculate how many silent frames we should drop. @@ -344,7 +365,16 @@ int InboundAudioStream::popSamples(int maxSamples, bool allOrNothing) { //Kick PLC to generate a filler frame, reducing 'click' lostAudioData(allOrNothing ? (maxSamples - samplesAvailable) / _ringBuffer.getNumFrameSamples() : 1); samplesPopped = _ringBuffer.samplesAvailable(); - popSamplesNoCheck(samplesPopped); + if (samplesPopped) { + popSamplesNoCheck(samplesPopped); + } else { + // No samples available means a packet is currently being + // processed, so we don't generate lost audio data, and instead + // just wait for the packet to come in. This prevents locking + // the real-time audio thread at the cost of a potential (but rare) + // 'click' + _lastPopSucceeded = false; + } } } return samplesPopped; @@ -531,6 +561,7 @@ void InboundAudioStream::setupCodec(CodecPluginPointer codec, const QString& cod _codec = codec; _selectedCodecName = codecName; if (_codec) { + QMutexLocker lock(&_decoderMutex); _decoder = codec->createDecoder(AudioConstants::SAMPLE_RATE, numChannels); } } @@ -538,6 +569,7 @@ void InboundAudioStream::setupCodec(CodecPluginPointer codec, const QString& cod void InboundAudioStream::cleanupCodec() { // release any old codec encoder/decoder first... if (_codec) { + QMutexLocker lock(&_decoderMutex); if (_decoder) { _codec->releaseDecoder(_decoder); _decoder = nullptr; diff --git a/libraries/audio/src/InboundAudioStream.h b/libraries/audio/src/InboundAudioStream.h index 5ff9e2c84c..c10a86cb69 100644 --- a/libraries/audio/src/InboundAudioStream.h +++ b/libraries/audio/src/InboundAudioStream.h @@ -187,6 +187,7 @@ protected: CodecPluginPointer _codec; QString _selectedCodecName; + QMutex _decoderMutex; Decoder* _decoder { nullptr }; int _mismatchedAudioCodecCount { 0 }; }; diff --git a/libraries/audio/src/MixedProcessedAudioStream.cpp b/libraries/audio/src/MixedProcessedAudioStream.cpp index 082977246b..ff2deac73e 100644 --- a/libraries/audio/src/MixedProcessedAudioStream.cpp +++ b/libraries/audio/src/MixedProcessedAudioStream.cpp @@ -36,13 +36,20 @@ int MixedProcessedAudioStream::lostAudioData(int numPackets) { QByteArray outputBuffer; while (numPackets--) { + if (!_decoderMutex.tryLock()) { + // an incoming packet is being processed, + // and will likely be on the ring buffer shortly, + // so don't bother generating more data + qCInfo(audiostream, "Packet currently being unpacked or lost frame already being generated. Not generating lost frame."); + return 0; + } if (_decoder) { _decoder->lostFrame(decodedBuffer); } else { decodedBuffer.resize(AudioConstants::NETWORK_FRAME_BYTES_STEREO); memset(decodedBuffer.data(), 0, decodedBuffer.size()); } - + _decoderMutex.unlock(); emit addedStereoSamples(decodedBuffer); emit processSamples(decodedBuffer, outputBuffer); @@ -55,6 +62,12 @@ int MixedProcessedAudioStream::lostAudioData(int numPackets) { int MixedProcessedAudioStream::parseAudioData(PacketType type, const QByteArray& packetAfterStreamProperties) { QByteArray decodedBuffer; + + // may block on the real-time thread, which is acceptible as + // parseAudioData is only called by the packet processing + // thread which, while high performance, is not as sensitive to + // delays as the real-time thread. + QMutexLocker lock(&_decoderMutex); if (_decoder) { _decoder->decode(packetAfterStreamProperties, decodedBuffer); } else { From 3a4241b290cfe6fc9456e91a351d0de141467235 Mon Sep 17 00:00:00 2001 From: Roxanne Skelly Date: Wed, 8 May 2019 16:31:00 -0700 Subject: [PATCH 3/4] Unnecessary spacing changes. --- libraries/audio/src/AudioSRC.cpp | 2 ++ libraries/audio/src/AudioSRC.h | 1 - libraries/audio/src/InboundAudioStream.cpp | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/libraries/audio/src/AudioSRC.cpp b/libraries/audio/src/AudioSRC.cpp index 867416871e..d488eccb6a 100644 --- a/libraries/audio/src/AudioSRC.cpp +++ b/libraries/audio/src/AudioSRC.cpp @@ -1491,6 +1491,7 @@ AudioSRC::~AudioSRC() { // int AudioSRC::render(const int16_t* input, int16_t* output, int inputFrames) { int outputFrames = 0; + while (inputFrames) { int ni = MIN(inputFrames, _inputBlock); @@ -1515,6 +1516,7 @@ int AudioSRC::render(const int16_t* input, int16_t* output, int inputFrames) { // int AudioSRC::render(const float* input, float* output, int inputFrames) { int outputFrames = 0; + while (inputFrames) { int ni = MIN(inputFrames, _inputBlock); diff --git a/libraries/audio/src/AudioSRC.h b/libraries/audio/src/AudioSRC.h index 111c2105b7..d5971e5838 100644 --- a/libraries/audio/src/AudioSRC.h +++ b/libraries/audio/src/AudioSRC.h @@ -55,7 +55,6 @@ public: int getMaxInput(int outputFrames); private: - float* _polyphaseFilter; int* _stepTable; diff --git a/libraries/audio/src/InboundAudioStream.cpp b/libraries/audio/src/InboundAudioStream.cpp index 482991626a..44f204c037 100644 --- a/libraries/audio/src/InboundAudioStream.cpp +++ b/libraries/audio/src/InboundAudioStream.cpp @@ -215,6 +215,7 @@ int InboundAudioStream::parseData(ReceivedMessage& message) { if (framesAvailable > _desiredJitterBufferFrames + MAX_FRAMES_OVER_DESIRED) { int framesToDrop = framesAvailable - (_desiredJitterBufferFrames + DESIRED_JITTER_BUFFER_FRAMES_PADDING); _ringBuffer.shiftReadPosition(framesToDrop * _ringBuffer.getNumFrameSamples()); + _framesAvailableStat.reset(); _currentJitterBufferFrames = 0; From 7a4e3557b0592accefd7369f14b6e548aa4d8122 Mon Sep 17 00:00:00 2001 From: Roxanne Skelly Date: Thu, 9 May 2019 15:21:43 -0700 Subject: [PATCH 4/4] Use TryLocker instead of explicitly using tryLock on mutexes --- libraries/audio/src/InboundAudioStream.cpp | 5 +++-- libraries/audio/src/MixedProcessedAudioStream.cpp | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/libraries/audio/src/InboundAudioStream.cpp b/libraries/audio/src/InboundAudioStream.cpp index 44f204c037..5ac3996029 100644 --- a/libraries/audio/src/InboundAudioStream.cpp +++ b/libraries/audio/src/InboundAudioStream.cpp @@ -10,6 +10,7 @@ // #include "InboundAudioStream.h" +#include "TryLocker.h" #include @@ -247,7 +248,8 @@ int InboundAudioStream::lostAudioData(int numPackets) { QByteArray decodedBuffer; while (numPackets--) { - if (!_decoderMutex.tryLock()) { + MutexTryLocker lock(_decoderMutex); + if (!lock.isLocked()) { // an incoming packet is being processed, // and will likely be on the ring buffer shortly, // so don't bother generating more data @@ -260,7 +262,6 @@ int InboundAudioStream::lostAudioData(int numPackets) { decodedBuffer.resize(AudioConstants::NETWORK_FRAME_BYTES_PER_CHANNEL * _numChannels); memset(decodedBuffer.data(), 0, decodedBuffer.size()); } - _decoderMutex.unlock(); _ringBuffer.writeData(decodedBuffer.data(), decodedBuffer.size()); } return 0; diff --git a/libraries/audio/src/MixedProcessedAudioStream.cpp b/libraries/audio/src/MixedProcessedAudioStream.cpp index ff2deac73e..6510f0bfc9 100644 --- a/libraries/audio/src/MixedProcessedAudioStream.cpp +++ b/libraries/audio/src/MixedProcessedAudioStream.cpp @@ -11,6 +11,7 @@ #include "MixedProcessedAudioStream.h" #include "AudioLogging.h" +#include "TryLocker.h" MixedProcessedAudioStream::MixedProcessedAudioStream(int numFramesCapacity, int numStaticJitterFrames) : InboundAudioStream(AudioConstants::STEREO, AudioConstants::NETWORK_FRAME_SAMPLES_PER_CHANNEL, @@ -36,7 +37,8 @@ int MixedProcessedAudioStream::lostAudioData(int numPackets) { QByteArray outputBuffer; while (numPackets--) { - if (!_decoderMutex.tryLock()) { + MutexTryLocker lock(_decoderMutex); + if (!lock.isLocked()) { // an incoming packet is being processed, // and will likely be on the ring buffer shortly, // so don't bother generating more data @@ -49,7 +51,6 @@ int MixedProcessedAudioStream::lostAudioData(int numPackets) { decodedBuffer.resize(AudioConstants::NETWORK_FRAME_BYTES_STEREO); memset(decodedBuffer.data(), 0, decodedBuffer.size()); } - _decoderMutex.unlock(); emit addedStereoSamples(decodedBuffer); emit processSamples(decodedBuffer, outputBuffer);