standardize Audio bg thread joins

interface/src/Application.cpp

@@ -1683,7 +1683,6 @@ void Application::updateHeartbeat() const {
 
 void Application::aboutToQuit() {
     emit beforeAboutToQuit();
-    DependencyManager::get<AudioClient>()->beforeAboutToQuit();
 
     foreach(auto inputPlugin, PluginManager::getInstance()->getInputPlugins()) {
         if (inputPlugin->isActive()) {
@@ -1784,14 +1783,12 @@ void Application::cleanupBeforeQuit() {
         _snapshotSoundInjector->stop();
     }
 
-    // stop audio after QML, as there are unexplained audio crashes originating in qtwebengine
-
-    // stop the AudioClient, synchronously
-    QMetaObject::invokeMethod(DependencyManager::get<AudioClient>().data(),
-                              "stop", Qt::BlockingQueuedConnection);
-
+    // FIXME: something else is holding a reference to AudioClient,
+    // so it must be explicitly synchronously stopped here
+    QMetaObject::invokeMethod(DependencyManager::get<AudioClient>().data(), "stop", Qt::BlockingQueuedConnection);
 
     // destroy Audio so it and its threads have a chance to go down safely
+    // this must happen after QML, as there are unexplained audio crashes originating in qtwebengine
     DependencyManager::destroy<AudioClient>();
     DependencyManager::destroy<AudioInjectorManager>();
 

libraries/audio-client/src/AudioClient.cpp

@@ -76,42 +76,58 @@ using Mutex = std::mutex;
 using Lock = std::unique_lock<Mutex>;
 static Mutex _deviceMutex;
 
-// background thread that continuously polls for device changes
-class CheckDevicesThread : public QThread {
+class BackgroundThread : public QThread {
 public:
-    const unsigned long DEVICE_CHECK_INTERVAL_MSECS = 2 * 1000;
+    BackgroundThread(AudioClient* client) : QThread((QObject*)client), _client(client) {}
+    virtual void join() = 0;
+protected:
+    AudioClient* _client;
+};
 
-    CheckDevicesThread(AudioClient* audioClient)
-        : _audioClient(audioClient) {
-    }
+// background thread continuously polling device changes
+class CheckDevicesThread : public BackgroundThread {
+public:
+    CheckDevicesThread(AudioClient* client) : BackgroundThread(client) {}
 
-    void beforeAboutToQuit() {
-        Lock lock(_checkDevicesMutex);
+    void join() override {
         _quit = true;
+        std::unique_lock<std::mutex> lock(mutex);
+        cv.wait(lock, [&]{ return !running; });
     }
 
+protected:
     void run() override {
-        while (true) {
-            {
-                Lock lock(_checkDevicesMutex);
-                if (_quit) {
-                    break;
-                }
-                _audioClient->checkDevices();
-            }
+        while (!_quit) {
+            _client->checkDevices();
+
+            const unsigned long DEVICE_CHECK_INTERVAL_MSECS = 2 * 1000;
             QThread::msleep(DEVICE_CHECK_INTERVAL_MSECS);
         }
+        std::lock_guard<std::mutex> lock(mutex);
+        running = false;
+        cv.notify_one();
     }
 
 private:
-    AudioClient* _audioClient { nullptr };
-    Mutex _checkDevicesMutex;
-    bool _quit { false };
+    std::atomic<bool> _quit { false };
+    bool running { true };
+    std::mutex mutex;
+    std::condition_variable cv;
 };
 
-void AudioInjectorsThread::prepare() {
-    _audio->prepareLocalAudioInjectors();
-}
+// background thread buffering local injectors
+class LocalInjectorsThread : public BackgroundThread {
+    Q_OBJECT
+public:
+    LocalInjectorsThread(AudioClient* client) : BackgroundThread(client) {}
+
+    void join() override { return; }
+
+private slots:
+    void prepare() { _client->prepareLocalAudioInjectors(); }
+};
+
+#include "AudioClient.moc"
 
 static void channelUpmix(int16_t* source, int16_t* dest, int numSamples, int numExtraChannels) {
     for (int i = 0; i < numSamples/2; i++) {
@@ -179,7 +195,6 @@ AudioClient::AudioClient() :
     _inputToNetworkResampler(NULL),
     _networkToOutputResampler(NULL),
     _localToOutputResampler(NULL),
-    _localAudioThread(this),
     _audioLimiter(AudioConstants::SAMPLE_RATE, OUTPUT_CHANNEL_COUNT),
     _outgoingAvatarAudioSequenceNumber(0),
     _audioOutputIODevice(_localInjectorsStream, _receivedAudioStream, this),
@@ -210,13 +225,14 @@ AudioClient::AudioClient() :
 
     // start a thread to detect any device changes
     _checkDevicesThread = new CheckDevicesThread(this);
-    _checkDevicesThread->setObjectName("CheckDevices Thread");
+    _checkDevicesThread->setObjectName("AudioClient CheckDevices Thread");
     _checkDevicesThread->setPriority(QThread::LowPriority);
     _checkDevicesThread->start();
 
     // start a thread to process local injectors
-    _localAudioThread.setObjectName("LocalAudio Thread");
-    _localAudioThread.start();
+    _localInjectorsThread = new LocalInjectorsThread(this);
+    _localInjectorsThread->setObjectName("AudioClient LocalInjectors Thread");
+    _localInjectorsThread->start();
 
     configureReverb();
 
@@ -231,18 +247,23 @@ AudioClient::AudioClient() :
 }
 
 AudioClient::~AudioClient() {
-    delete _checkDevicesThread;
-    stop();
     if (_codec && _encoder) {
         _codec->releaseEncoder(_encoder);
         _encoder = nullptr;
     }
 }
 
-void AudioClient::beforeAboutToQuit() {
-    static_cast<CheckDevicesThread*>(_checkDevicesThread)->beforeAboutToQuit();
-}
+void AudioClient::customDeleter() {
+    stop(); // synchronously
 
+    static_cast<BackgroundThread*>(_checkDevicesThread)->join();
+    delete _checkDevicesThread;
+
+    static_cast<BackgroundThread*>(_localInjectorsThread)->join();
+    delete _localInjectorsThread;
+
+    deleteLater(); // asynchronously
+}
 
 void AudioClient::handleMismatchAudioFormat(SharedNodePointer node, const QString& currentCodec, const QString& recievedCodec) {
     qCDebug(audioclient) << __FUNCTION__ << "sendingNode:" << *node << "currentCodec:" << currentCodec << "recievedCodec:" << recievedCodec;
@@ -1527,8 +1548,8 @@ bool AudioClient::switchOutputToAudioDevice(const QAudioDeviceInfo& outputDevice
                     _outputScratchBuffer = new int16_t[_outputPeriod];
 
                     // size local output mix buffer based on resampled network frame size
-                    _networkPeriod = _localToOutputResampler->getMaxOutput(AudioConstants::NETWORK_FRAME_SAMPLES_STEREO);
-                    _localOutputMixBuffer = new float[_networkPeriod];
+                    int networkPeriod = _localToOutputResampler->getMaxOutput(AudioConstants::NETWORK_FRAME_SAMPLES_STEREO);
+                    _localOutputMixBuffer = new float[networkPeriod];
                     int localPeriod = _outputPeriod * 2;
                     _localInjectorsStream.resizeForFrameSize(localPeriod);
 
@@ -1691,7 +1712,7 @@ qint64 AudioClient::AudioOutputIODevice::readData(char * data, qint64 maxSize) {
     }
 
     // prepare injectors for the next callback
-    QMetaObject::invokeMethod(&_audio->_localAudioThread, "prepare", Qt::QueuedConnection);
+    QMetaObject::invokeMethod(_audio->_localInjectorsThread, "prepare", Qt::QueuedConnection);
 
     int samplesPopped = std::max(networkSamplesPopped, injectorSamplesPopped);
     int framesPopped = samplesPopped / AudioConstants::STEREO;

libraries/audio-client/src/AudioClient.h

@@ -71,19 +71,6 @@ class QIODevice;
 class Transform;
 class NLPacket;
 
-class AudioInjectorsThread : public QThread {
-    Q_OBJECT
-
-public:
-    AudioInjectorsThread(AudioClient* audio) : _audio(audio) {}
-
-public slots :
-    void prepare();
-
-private:
-    AudioClient* _audio;
-};
-
 class AudioClient : public AbstractAudioInterface, public Dependency {
     Q_OBJECT
     SINGLETON_DEPENDENCY
@@ -186,8 +173,6 @@ public slots:
     void audioMixerKilled();
     void toggleMute();
 
-    void beforeAboutToQuit();
-
     virtual void setIsStereoInput(bool stereo) override;
 
     void toggleAudioNoiseReduction() { _isNoiseGateEnabled = !_isNoiseGateEnabled; }
@@ -244,9 +229,7 @@ protected:
     AudioClient();
     ~AudioClient();
 
-    virtual void customDeleter() override {
-        deleteLater();
-    }
+    virtual void customDeleter() override;
 
 private:
     void outputFormatChanged();
@@ -339,19 +322,17 @@ private:
     // for network audio (used by network audio thread)
     int16_t _networkScratchBuffer[AudioConstants::NETWORK_FRAME_SAMPLES_AMBISONIC];
 
-    // for local audio (used by audio injectors thread)
-    int _networkPeriod { 0 };
-    float _localMixBuffer[AudioConstants::NETWORK_FRAME_SAMPLES_STEREO];
-    int16_t _localScratchBuffer[AudioConstants::NETWORK_FRAME_SAMPLES_AMBISONIC];
-    float* _localOutputMixBuffer { NULL };
-    AudioInjectorsThread _localAudioThread;
-    RecursiveMutex _localAudioMutex;
-
     // for output audio (used by this thread)
     int _outputPeriod { 0 };
     float* _outputMixBuffer { NULL };
     int16_t* _outputScratchBuffer { NULL };
 
+    // for local audio (used by audio injectors thread)
+    float _localMixBuffer[AudioConstants::NETWORK_FRAME_SAMPLES_STEREO];
+    int16_t _localScratchBuffer[AudioConstants::NETWORK_FRAME_SAMPLES_AMBISONIC];
+    float* _localOutputMixBuffer { NULL };
+    RecursiveMutex _localAudioMutex;
+
     AudioLimiter _audioLimiter;
 
     // Adds Reverb
@@ -394,12 +375,13 @@ private:
     QString _selectedCodecName;
     Encoder* _encoder { nullptr }; // for outbound mic stream
 
-    QThread* _checkDevicesThread { nullptr };
-
     RateCounter<> _silentOutbound;
     RateCounter<> _audioOutbound;
     RateCounter<> _silentInbound;
     RateCounter<> _audioInbound;
+
+    QThread* _checkDevicesThread { nullptr };
+    QThread* _localInjectorsThread { nullptr };
 };