Merge pull request #4279 from birarda/android

fix cleanup of AudioClient, avoid gvr-interface crash on OS X

gvr-interface/src/Client.cpp

@@ -9,8 +9,6 @@
 //  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
 //
 
-#include <QtCore/QThread>
-
 #include <AccountManager.h>
 #include <AddressManager.h>
 #include <HifiSockAddr.h>

gvr-interface/src/Client.h

@@ -16,13 +16,14 @@
 
 #include <HifiSockAddr.h>
 
-class QThread;
-
 class Client : public QObject {
     Q_OBJECT
 public:
     Client(QObject* parent = 0);
+    
+    virtual void cleanupBeforeQuit() = 0;
 protected:
+    
     void setupNetworking();
     virtual void processVerifiedPacket(const HifiSockAddr& senderSockAddr, const QByteArray& incomingPacket);
 private slots:

gvr-interface/src/GVRInterface.cpp

@@ -86,6 +86,13 @@ GVRInterface::GVRInterface(int argc, char* argv[]) :
     QTimer* idleTimer = new QTimer(this);
     connect(idleTimer, &QTimer::timeout, this, &GVRInterface::idle);
     idleTimer->start(0);
+    
+    // call our quit handler before we go down
+    connect(this, &QCoreApplication::aboutToQuit, this, &GVRInterface::handleApplicationQuit);
+}
+
+void GVRInterface::handleApplicationQuit() {
+    _client->cleanupBeforeQuit();
 }
 
 void GVRInterface::idle() {

gvr-interface/src/GVRInterface.h

@@ -53,6 +53,7 @@ private slots:
     void handleApplicationStateChange(Qt::ApplicationState state);
     void idle();
 private:
+    void handleApplicationQuit();
     
     void enterVRMode();
     void leaveVRMode();

gvr-interface/src/RenderingClient.cpp

@@ -37,7 +37,7 @@ RenderingClient::RenderingClient(QObject *parent, const QString& launchURLString
     DependencyManager::set<AvatarHashMap>();
     
     // get our audio client setup on its own thread
-    QThread* audioThread = new QThread(this);
+    QThread* audioThread = new QThread();
     auto audioClient = DependencyManager::set<AudioClient>();
     
     audioClient->setPositionGetter(getPositionForAudio);
@@ -45,6 +45,8 @@ RenderingClient::RenderingClient(QObject *parent, const QString& launchURLString
     
     audioClient->moveToThread(audioThread);
     connect(audioThread, &QThread::started, audioClient.data(), &AudioClient::start);
+    connect(audioClient.data(), &AudioClient::destroyed, audioThread, &QThread::quit);
+    connect(audioThread, &QThread::finished, audioThread, &QThread::deleteLater);
 
     audioThread->start();
     
@@ -68,15 +70,13 @@ void RenderingClient::sendAvatarPacket() {
     _fakeAvatar.sendIdentityPacket();
 }
 
-RenderingClient::~RenderingClient() {
-    auto audioClient = DependencyManager::get<AudioClient>();
-
-    // stop the audio client
-    QMetaObject::invokeMethod(audioClient.data(), "stop", Qt::BlockingQueuedConnection);
+void RenderingClient::cleanupBeforeQuit() {
     
-    // ask the audio thread to quit and wait until it is done
-    audioClient->thread()->quit();
-    audioClient->thread()->wait();
+    QMetaObject::invokeMethod(DependencyManager::get<AudioClient>().data(),
+                              "stop", Qt::BlockingQueuedConnection);
+    
+    // destroy the AudioClient so it and its thread will safely go down
+    DependencyManager::destroy<AudioClient>();
 }
 
 void RenderingClient::processVerifiedPacket(const HifiSockAddr& senderSockAddr, const QByteArray& incomingPacket) {

gvr-interface/src/RenderingClient.h

@@ -26,7 +26,6 @@ class RenderingClient : public Client {
     Q_OBJECT
 public:
     RenderingClient(QObject* parent = 0, const QString& launchURLString = QString());
-    ~RenderingClient();
     
     const glm::vec3& getPosition() const { return _position; }
     const glm::quat& getOrientation() const { return _orientation; }
@@ -35,6 +34,8 @@ public:
     static glm::vec3 getPositionForAudio() { return _instance->getPosition(); }
     static glm::quat getOrientationForAudio() { return _instance->getOrientation(); }
     
+    virtual void cleanupBeforeQuit();
+    
 private slots:
     void goToLocation(const glm::vec3& newPosition,
                       bool hasOrientationChange, const glm::quat& newOrientation,

interface/src/Application.cpp

@@ -328,7 +328,7 @@ Application::Application(int& argc, char** argv, QElapsedTimer &startup_time) :
     connect(&nodeList->getNodeSocket(), SIGNAL(readyRead()), &_datagramProcessor, SLOT(processDatagrams()));
 
     // put the audio processing on a separate thread
-    QThread* audioThread = new QThread(this);
+    QThread* audioThread = new QThread();
     audioThread->setObjectName("Audio Thread");
     
     auto audioIO = DependencyManager::get<AudioClient>();
@@ -338,7 +338,9 @@ Application::Application(int& argc, char** argv, QElapsedTimer &startup_time) :
     
     audioIO->moveToThread(audioThread);
     connect(audioThread, &QThread::started, audioIO.data(), &AudioClient::start);
-    connect(audioIO.data(), SIGNAL(muteToggled()), this, SLOT(audioMuteToggled()));
+    connect(audioIO.data(), &AudioClient::destroyed, audioThread, &QThread::quit);
+    connect(audioThread, &QThread::finished, audioThread, &QThread::deleteLater);
+    connect(audioIO.data(), &AudioClient::muteToggled, this, &Application::audioMuteToggled);
 
     audioThread->start();
     
@@ -516,21 +518,33 @@ Application::Application(int& argc, char** argv, QElapsedTimer &startup_time) :
 
 void Application::aboutToQuit() {
     _aboutToQuit = true;
+    
+    cleanupBeforeQuit();
 }
 
-Application::~Application() {
+void Application::cleanupBeforeQuit() {
     QMetaObject::invokeMethod(&_settingsTimer, "stop", Qt::BlockingQueuedConnection);
     _settingsThread.quit();
     saveSettings();
     
+    // TODO: now that this is in cleanupBeforeQuit do we really need it to stop and force
+    // an event loop to send the packet?
+    UserActivityLogger::getInstance().close();
+    
+    // stop the AudioClient
+    QMetaObject::invokeMethod(DependencyManager::get<AudioClient>().data(),
+                              "stop", Qt::BlockingQueuedConnection);
+    
+    // destroy the AudioClient so it and its thread have a chance to go down safely
+    DependencyManager::destroy<AudioClient>();
+}
+
+Application::~Application() {    
     _entities.getTree()->setSimulation(NULL);
     qInstallMessageHandler(NULL);
     
     _window->saveGeometry();
     
-    int DELAY_TIME = 1000;
-    UserActivityLogger::getInstance().close(DELAY_TIME);
-    
     // make sure we don't call the idle timer any more
     delete idleTimer;
     
@@ -541,15 +555,6 @@ Application::~Application() {
     _nodeThread->quit();
     _nodeThread->wait();
     
-    auto audioIO = DependencyManager::get<AudioClient>();
-
-    // stop the audio process
-    QMetaObject::invokeMethod(audioIO.data(), "stop", Qt::BlockingQueuedConnection);
-    
-    // ask the audio thread to quit and wait until it is done
-    audioIO->thread()->quit();
-    audioIO->thread()->wait();
-    
     _octreeProcessor.terminate();
     _entityEditSender.terminate();
 

interface/src/Application.h

@@ -410,6 +410,8 @@ private:
 
     void initDisplay();
     void init();
+    
+    void cleanupBeforeQuit();
 
     void update(float deltaTime);
 

libraries/audio-client/src/AudioClient.cpp

@@ -136,6 +136,8 @@ AudioClient::AudioClient() :
 }
 
 AudioClient::~AudioClient() {
+    stop();
+    
     if (_gverbLocal) {
         gverb_free(_gverbLocal);
     }
@@ -489,7 +491,6 @@ void AudioClient::start() {
 }
 
 void AudioClient::stop() {
-
     _inputFrameBuffer.finalize();
     _inputGain.finalize();
     _sourceGain.finalize();

libraries/audio-client/src/AudioClient.h

@@ -188,6 +188,10 @@ protected:
     AudioClient();
     ~AudioClient();
     
+    virtual void customDeleter() {
+        deleteLater();
+    }
+    
 private:
     void outputFormatChanged();
 

libraries/networking/src/UserActivityLogger.cpp

@@ -86,17 +86,9 @@ void UserActivityLogger::launch(QString applicationVersion) {
     logAction(ACTION_NAME, actionDetails);
 }
 
-void UserActivityLogger::close(int delayTime) {
+void UserActivityLogger::close() {
     const QString ACTION_NAME = "close";
-    
-    // In order to get the end of the session, we need to give the account manager enough time to send the packet.
-    QEventLoop loop;
-    QTimer timer;
-    connect(&timer, &QTimer::timeout, &loop, &QEventLoop::quit);
-    // Now we can log it
     logAction(ACTION_NAME, QJsonObject());
-    timer.start(delayTime);
-    loop.exec();
 }
 
 void UserActivityLogger::changedDisplayName(QString displayName) {

libraries/networking/src/UserActivityLogger.h

@@ -30,7 +30,7 @@ public slots:
     void logAction(QString action, QJsonObject details = QJsonObject(), JSONCallbackParameters params = JSONCallbackParameters());
     
     void launch(QString applicationVersion);
-    void close(int delayTime);
+    void close();
     void changedDisplayName(QString displayName);
     void changedModel(QString typeOfModel, QString modelURL);
     void changedDomain(QString domainURL);