From 72fc686ec1c1172d72947864c948d169ecbb23fc Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Mon, 24 Sep 2018 14:52:13 -0700 Subject: [PATCH 1/3] Fix timer managmement during shutdown --- interface/src/Application.cpp | 6 +++-- libraries/audio-client/src/AudioClient.cpp | 27 ++++++++++++++++--- libraries/audio-client/src/AudioClient.h | 2 ++ .../audio-client/src/AudioPeakValues.cpp | 6 +++++ 4 files changed, 36 insertions(+), 5 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 46cebc1661..4eac967428 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -1759,10 +1759,12 @@ Application::Application(int& argc, char** argv, QElapsedTimer& startupTimer, bo // Make sure we don't time out during slow operations at startup updateHeartbeat(); - QTimer* settingsTimer = new QTimer(); moveToNewNamedThread(settingsTimer, "Settings Thread", [this, settingsTimer]{ - connect(qApp, &Application::beforeAboutToQuit, [this, settingsTimer]{ + // This needs to run on the settings thread, so we need to pass the `settingsTimer` as the + // receiver object, otherwise it will run on the application thread and trigger a warning + // about trying to kill the timer on the main thread. + connect(qApp, &Application::beforeAboutToQuit, settingsTimer, [this, settingsTimer]{ // Disconnect the signal from the save settings QObject::disconnect(settingsTimer, &QTimer::timeout, this, &Application::saveSettings); // Stop the settings timer diff --git a/libraries/audio-client/src/AudioClient.cpp b/libraries/audio-client/src/AudioClient.cpp index 3a33eccc8a..6af74bc8e8 100644 --- a/libraries/audio-client/src/AudioClient.cpp +++ b/libraries/audio-client/src/AudioClient.cpp @@ -101,6 +101,13 @@ QList getAvailableDevices(QAudio::Mode mode) { // now called from a background thread, to keep blocking operations off the audio thread void AudioClient::checkDevices() { + // Make sure we're not shutting down + Lock timerMutex(_checkDevicesMutex); + // If we HAVE shut down after we were queued, but prior to execution, early exit + if (nullptr == _checkDevicesTimer) { + return; + } + auto inputDevices = getAvailableDevices(QAudio::AudioInput); auto outputDevices = getAvailableDevices(QAudio::AudioOutput); @@ -278,8 +285,8 @@ void AudioClient::customDeleter() { _shouldRestartInputSetup = false; #endif stop(); - _checkDevicesTimer->stop(); - _checkPeakValuesTimer->stop(); + //_checkDevicesTimer->stop(); + //_checkPeakValuesTimer->stop(); deleteLater(); } @@ -648,12 +655,26 @@ void AudioClient::start() { } void AudioClient::stop() { - qCDebug(audioclient) << "AudioClient::stop(), requesting switchInputToAudioDevice() to shut down"; switchInputToAudioDevice(QAudioDeviceInfo(), true); qCDebug(audioclient) << "AudioClient::stop(), requesting switchOutputToAudioDevice() to shut down"; switchOutputToAudioDevice(QAudioDeviceInfo(), true); + + // Stop triggering the checks + QObject::disconnect(_checkPeakValuesTimer, &QTimer::timeout, nullptr, nullptr); + QObject::disconnect(_checkDevicesTimer, &QTimer::timeout, nullptr, nullptr); + + // Destruction of the pointers will occur when the parent object (this) is destroyed) + { + Lock lock(_checkDevicesMutex); + _checkDevicesTimer = nullptr; + } + { + Lock lock(_checkPeakValuesMutex); + _checkPeakValuesTimer = nullptr; + } + #if defined(Q_OS_ANDROID) _checkInputTimer.stop(); disconnect(&_checkInputTimer, &QTimer::timeout, 0, 0); diff --git a/libraries/audio-client/src/AudioClient.h b/libraries/audio-client/src/AudioClient.h index 4640d7c045..f5fee6184c 100644 --- a/libraries/audio-client/src/AudioClient.h +++ b/libraries/audio-client/src/AudioClient.h @@ -432,7 +432,9 @@ private: bool _shouldRestartInputSetup { true }; // Should we restart the input device because of an unintended stop? #endif + Mutex _checkDevicesMutex; QTimer* _checkDevicesTimer { nullptr }; + Mutex _checkPeakValuesMutex; QTimer* _checkPeakValuesTimer { nullptr }; bool _isRecording { false }; diff --git a/libraries/audio-client/src/AudioPeakValues.cpp b/libraries/audio-client/src/AudioPeakValues.cpp index 0b8921a117..a50567da7f 100644 --- a/libraries/audio-client/src/AudioPeakValues.cpp +++ b/libraries/audio-client/src/AudioPeakValues.cpp @@ -40,6 +40,12 @@ void release(IAudioClient* audioClient) { } void AudioClient::checkPeakValues() { + // Guard against running during shutdown + Lock timerMutex(_checkPeakValuesMutex); + if (nullptr == _checkPeakValuesTimer) { + return; + } + // prepare the windows environment CoInitialize(NULL); From 5adc4c2290905a37b9668447ee0cbb2cb092022f Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Tue, 25 Sep 2018 13:17:49 -0700 Subject: [PATCH 2/3] Protect against null pointer usage in sound scripting wrapper --- libraries/audio/src/Sound.cpp | 7 +++++-- libraries/audio/src/Sound.h | 8 ++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/libraries/audio/src/Sound.cpp b/libraries/audio/src/Sound.cpp index 67f9952771..da284f19a3 100644 --- a/libraries/audio/src/Sound.cpp +++ b/libraries/audio/src/Sound.cpp @@ -43,8 +43,11 @@ void soundSharedPointerFromScriptValue(const QScriptValue& object, SharedSoundPo } } -SoundScriptingInterface::SoundScriptingInterface(SharedSoundPointer sound) : _sound(sound) { - QObject::connect(sound.data(), &Sound::ready, this, &SoundScriptingInterface::ready); +SoundScriptingInterface::SoundScriptingInterface(const SharedSoundPointer& sound) : _sound(sound) { + // During shutdown we can sometimes get an empty sound pointer back + if (_sound) { + QObject::connect(_sound.data(), &Sound::ready, this, &SoundScriptingInterface::ready); + } } Sound::Sound(const QUrl& url, bool isStereo, bool isAmbisonic) : diff --git a/libraries/audio/src/Sound.h b/libraries/audio/src/Sound.h index 348600e4ae..a0544870d0 100644 --- a/libraries/audio/src/Sound.h +++ b/libraries/audio/src/Sound.h @@ -105,11 +105,11 @@ class SoundScriptingInterface : public QObject { Q_PROPERTY(float duration READ getDuration) public: - SoundScriptingInterface(SharedSoundPointer sound); - SharedSoundPointer getSound() { return _sound; } + SoundScriptingInterface(const SharedSoundPointer& sound); + const SharedSoundPointer& getSound() { return _sound; } - bool isReady() const { return _sound->isReady(); } - float getDuration() { return _sound->getDuration(); } + bool isReady() const { return _sound ? _sound->isReady() : false; } + float getDuration() { return _sound ? _sound->getDuration() : 0.0f; } /**jsdoc * Triggered when the sound has been downloaded and is ready to be played. From 15de0590e8ceaa44419781b4177b493d7851bfef Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Tue, 25 Sep 2018 13:18:49 -0700 Subject: [PATCH 3/3] Cleaning up commented out code --- libraries/audio-client/src/AudioClient.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/libraries/audio-client/src/AudioClient.cpp b/libraries/audio-client/src/AudioClient.cpp index 6af74bc8e8..68a68633e6 100644 --- a/libraries/audio-client/src/AudioClient.cpp +++ b/libraries/audio-client/src/AudioClient.cpp @@ -285,9 +285,6 @@ void AudioClient::customDeleter() { _shouldRestartInputSetup = false; #endif stop(); - //_checkDevicesTimer->stop(); - //_checkPeakValuesTimer->stop(); - deleteLater(); }