From dc7ec355444cc78307661576a3d2d499de7f7ac2 Mon Sep 17 00:00:00 2001 From: Roxanne Skelly Date: Wed, 8 May 2019 16:17:18 -0700 Subject: [PATCH] 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 {