Merge pull request #7419 from hyperlogic/tony/no-locks-no-problem

Prevent deadlocks in AvatarUpdate

interface/src/Application.cpp

@@ -876,7 +876,7 @@ Application::Application(int& argc, char** argv, QElapsedTimer& startupTimer) :
     _applicationStateDevice = std::make_shared<controller::StateController>();
 
     _applicationStateDevice->addInputVariant(QString("InHMD"), controller::StateController::ReadLambda([]() -> float {
-        return (float)qApp->getAvatarUpdater()->isHMDMode();
+        return (float)qApp->isHMDMode();
     }));
     _applicationStateDevice->addInputVariant(QString("SnapTurn"), controller::StateController::ReadLambda([]() -> float {
         return (float)qApp->getMyAvatar()->getSnapTurn();
@@ -1115,7 +1115,6 @@ void Application::cleanupBeforeQuit() {
 
     // first stop all timers directly or by invokeMethod
     // depending on what thread they run in
-    _avatarUpdate->terminate();
     locationUpdateTimer.stop();
     balanceUpdateTimer.stop();
     identityPacketTimer.stop();
@@ -1376,6 +1375,15 @@ void Application::initializeUi() {
 
 void Application::paintGL() {
 
+    // Some plugins process message events, potentially leading to
+    // re-entering a paint event.  don't allow further processing if this
+    // happens
+    if (_inPaint) {
+        return;
+    }
+    _inPaint = true;
+    Finally clearFlagLambda([this] { _inPaint = false; });
+
     // paintGL uses a queued connection, so we can get messages from the queue even after we've quit
     // and the plugins have shutdown
     if (_aboutToQuit) {
@@ -1408,15 +1416,6 @@ void Application::paintGL() {
         return;
     }
 
-    // Some plugins process message events, potentially leading to
-    // re-entering a paint event.  don't allow further processing if this
-    // happens
-    if (_inPaint) {
-        return;
-    }
-    _inPaint = true;
-    Finally clearFlagLambda([this] { _inPaint = false; });
-
     auto displayPlugin = getActiveDisplayPlugin();
     // FIXME not needed anymore?
     _offscreenContext->makeCurrent();
@@ -1480,7 +1479,6 @@ void Application::paintGL() {
         auto myAvatar = getMyAvatar();
         boomOffset = myAvatar->getScale() * myAvatar->getBoomLength() * -IDENTITY_FRONT;
 
-        myAvatar->startCapture();
         if (_myCamera.getMode() == CAMERA_MODE_FIRST_PERSON || _myCamera.getMode() == CAMERA_MODE_THIRD_PERSON) {
             Menu::getInstance()->setIsOptionChecked(MenuOption::FirstPerson, myAvatar->getBoomLength() <= MyAvatar::ZOOM_MIN);
             Menu::getInstance()->setIsOptionChecked(MenuOption::ThirdPerson, !(myAvatar->getBoomLength() <= MyAvatar::ZOOM_MIN));
@@ -1568,7 +1566,6 @@ void Application::paintGL() {
         if (!isHMDMode()) {
             _myCamera.update(1.0f / _fps);
         }
-        myAvatar->endCapture();
     }
 
     getApplicationCompositor().setFrameInfo(_frameCount, _myCamera.getTransform());
@@ -2499,11 +2496,10 @@ static uint32_t _renderedFrameIndex { INVALID_FRAME };
 
 void Application::idle(uint64_t now) {
 
-    if (_aboutToQuit) {
+    if (_aboutToQuit || _inPaint) {
         return; // bail early, nothing to do here.
     }
 
-
     checkChangeCursor();
 
     Stats::getInstance()->updateStats();
@@ -2912,37 +2908,6 @@ void Application::init() {
     // Make sure any new sounds are loaded as soon as know about them.
     connect(tree.get(), &EntityTree::newCollisionSoundURL, DependencyManager::get<SoundCache>().data(), &SoundCache::getSound);
     connect(getMyAvatar(), &MyAvatar::newCollisionSoundURL, DependencyManager::get<SoundCache>().data(), &SoundCache::getSound);
-
-    setAvatarUpdateThreading();
-}
-
-const bool ENABLE_AVATAR_UPDATE_THREADING = false;
-void Application::setAvatarUpdateThreading() {
-    setAvatarUpdateThreading(ENABLE_AVATAR_UPDATE_THREADING);
-}
-void Application::setRawAvatarUpdateThreading() {
-    setRawAvatarUpdateThreading(ENABLE_AVATAR_UPDATE_THREADING);
-}
-void Application::setRawAvatarUpdateThreading(bool isThreaded) {
-    if (_avatarUpdate) {
-        if (_avatarUpdate->isThreaded() == isThreaded) {
-            return;
-        }
-        _avatarUpdate->terminate();
-    }
-    _avatarUpdate = new AvatarUpdate();
-    _avatarUpdate->initialize(isThreaded);
-}
-void Application::setAvatarUpdateThreading(bool isThreaded) {
-    if (_avatarUpdate && (_avatarUpdate->isThreaded() == isThreaded)) {
-        return;
-    }
-
-    if (_avatarUpdate) {
-        _avatarUpdate->terminate(); // Must be before we shutdown anim graph.
-    }
-    _avatarUpdate = new AvatarUpdate();
-    _avatarUpdate->initialize(isThreaded);
 }
 
 void Application::updateLOD() {
@@ -2970,7 +2935,7 @@ void Application::updateMyAvatarLookAtPosition() {
     auto eyeTracker = DependencyManager::get<EyeTracker>();
 
     bool isLookingAtSomeone = false;
-    bool isHMD = _avatarUpdate->isHMDMode();
+    bool isHMD = qApp->isHMDMode();
     glm::vec3 lookAtSpot;
     if (eyeTracker->isTracking() && (isHMD || eyeTracker->isSimulating())) {
         //  Look at the point that the user is looking at.
@@ -3021,7 +2986,7 @@ void Application::updateMyAvatarLookAtPosition() {
         } else {
             //  I am not looking at anyone else, so just look forward
             if (isHMD) {
-                glm::mat4 headPose = _avatarUpdate->getHeadPose();
+                glm::mat4 headPose = myAvatar->getHMDSensorMatrix();
                 glm::quat headRotation = glm::quat_cast(headPose);
                 lookAtSpot = myAvatar->getPosition() +
                     myAvatar->getOrientation() * (headRotation * glm::vec3(0.0f, 0.0f, -TREE_SCALE));
@@ -3268,9 +3233,10 @@ void Application::update(float deltaTime) {
     updateThreads(deltaTime); // If running non-threaded, then give the threads some time to process...
     updateDialogs(deltaTime); // update various stats dialogs if present
 
+    QSharedPointer<AvatarManager> avatarManager = DependencyManager::get<AvatarManager>();
+
     if (_physicsEnabled) {
         PerformanceTimer perfTimer("physics");
-        AvatarManager* avatarManager = DependencyManager::get<AvatarManager>().data();
 
         {
             PerformanceTimer perfTimer("updateStates)");
@@ -3339,7 +3305,18 @@ void Application::update(float deltaTime) {
         }
     }
 
-    _avatarUpdate->synchronousProcess();
+    // AvatarManager update
+    {
+        PerformanceTimer perfTimer("AvatarManger");
+
+        qApp->setAvatarSimrateSample(1.0f / deltaTime);
+
+        avatarManager->updateOtherAvatars(deltaTime);
+
+        qApp->updateMyAvatarLookAtPosition();
+
+        avatarManager->updateMyAvatar(deltaTime);
+    }
 
     {
         PerformanceTimer perfTimer("overlays");
@@ -3836,9 +3813,7 @@ void Application::displaySide(RenderArgs* renderArgs, Camera& theCamera, bool se
     // FIXME: This preRender call is temporary until we create a separate render::scene for the mirror rendering.
     // Then we can move this logic into the Avatar::simulate call.
     auto myAvatar = getMyAvatar();
-    myAvatar->startRender();
     myAvatar->preRender(renderArgs);
-    myAvatar->endRender();
 
     // Update animation debug draw renderer
     AnimDebugDraw::getInstance().update();
@@ -3920,9 +3895,7 @@ void Application::displaySide(RenderArgs* renderArgs, Camera& theCamera, bool se
         _renderEngine->getRenderContext()->args = renderArgs;
 
         // Before the deferred pass, let's try to use the render engine
-        myAvatar->startRenderRun();
         _renderEngine->run();
-        myAvatar->endRenderRun();
     }
 
     activeRenderingThread = nullptr;

interface/src/Application.h

@@ -40,7 +40,6 @@
 #include <ViewFrustum.h>
 #include <AbstractUriHandler.h>
 
-#include "avatar/AvatarUpdate.h"
 #include "avatar/MyAvatar.h"
 #include "Bookmarks.h"
 #include "Camera.h"
@@ -215,7 +214,6 @@ public:
     const QRect& getMirrorViewRect() const { return _mirrorViewRect; }
 
     void updateMyAvatarLookAtPosition();
-    AvatarUpdate* getAvatarUpdater() { return _avatarUpdate; }
     float getAvatarSimrate();
     void setAvatarSimrateSample(float sample);
 
@@ -253,11 +251,6 @@ public slots:
 
     void openUrl(const QUrl& url);
 
-    void setAvatarUpdateThreading();
-    void setAvatarUpdateThreading(bool isThreaded);
-    void setRawAvatarUpdateThreading();
-    void setRawAvatarUpdateThreading(bool isThreaded);
-
     void resetSensors(bool andReload = false);
     void setActiveFaceTracker();
 
@@ -420,7 +413,6 @@ private:
 
     std::shared_ptr<controller::StateController> _applicationStateDevice; // Default ApplicationDevice reflecting the state of different properties of the session
     std::shared_ptr<KeyboardMouseDevice> _keyboardMouseDevice;   // Default input device, the good old keyboard mouse and maybe touchpad
-    AvatarUpdate* _avatarUpdate {nullptr};
     SimpleMovingAverage _avatarSimsPerSecond {10};
     int _avatarSimsPerSecondReport {0};
     quint64 _lastAvatarSimsPerSecondUpdate {0};

interface/src/avatar/Avatar.cpp

@@ -393,7 +393,6 @@ void Avatar::render(RenderArgs* renderArgs, const glm::vec3& cameraPosition) {
     }
 
     if (!frustum->sphereIntersectsFrustum(getPosition(), boundingRadius)) {
-        endRender();
         return;
     }
 
@@ -512,7 +511,6 @@ void Avatar::render(RenderArgs* renderArgs, const glm::vec3& cameraPosition) {
             renderDisplayName(batch, frustum, textPosition);
         }
     }
-    endRender();
 }
 
 glm::quat Avatar::computeRotationFromBodyToWorldUp(float proportion) const {
@@ -907,7 +905,6 @@ void Avatar::setAttachmentData(const QVector<AttachmentData>& attachmentData) {
 
 
 int Avatar::parseDataFromBuffer(const QByteArray& buffer) {
-    startUpdate();
     if (!_initialized) {
         // now that we have data for this Avatar we are go for init
         init();
@@ -926,7 +923,6 @@ int Avatar::parseDataFromBuffer(const QByteArray& buffer) {
     if (_moving || _hasNewJointRotations || _hasNewJointTranslations) {
         locationChanged();
     }
-    endUpdate();
 
     return bytesRead;
 }

interface/src/avatar/AvatarManager.cpp

@@ -145,9 +145,7 @@ void AvatarManager::updateOtherAvatars(float deltaTime) {
             removeAvatar(avatarIterator.key());
             ++avatarIterator;
         } else {
-            avatar->startUpdate();
             avatar->simulate(deltaTime);
-            avatar->endUpdate();
             ++avatarIterator;
 
             avatar->updateRenderItem(pendingChanges);
@@ -169,7 +167,6 @@ void AvatarManager::simulateAvatarFades(float deltaTime) {
     render::PendingChanges pendingChanges;
     while (fadingIterator != _avatarFades.end()) {
         auto avatar = std::static_pointer_cast<Avatar>(*fadingIterator);
-        avatar->startUpdate();
         avatar->setTargetScale(avatar->getUniformScale() * SHRINK_RATE);
         if (avatar->getTargetScale() <= MIN_FADE_SCALE) {
             avatar->removeFromScene(*fadingIterator, scene, pendingChanges);
@@ -183,7 +180,6 @@ void AvatarManager::simulateAvatarFades(float deltaTime) {
             avatar->simulate(deltaTime);
             ++fadingIterator;
         }
-        avatar->endUpdate();
     }
     scene->enqueuePendingChanges(pendingChanges);
 }

interface/src/avatar/AvatarUpdate.cpp

@@ -1,84 +0,0 @@
-//
-//  AvatarUpdate.cpp
-//  interface/src/avatar
-//
-//  Created by Howard Stearns on 8/18/15.
-//
-//  Distributed under the Apache License, Version 2.0.
-//  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
-//
-//
-
-#include <DependencyManager.h>
-#include "Application.h"
-#include "AvatarManager.h"
-#include "AvatarUpdate.h"
-#include <display-plugins/DisplayPlugin.h>
-#include "InterfaceLogging.h"
-
-AvatarUpdate::AvatarUpdate() : GenericThread(),  _lastAvatarUpdate(0), _isHMDMode(false) {
-    setObjectName("Avatar Update"); // GenericThread::initialize uses this to set the thread name.
-    Settings settings;
-    const int DEFAULT_TARGET_AVATAR_SIMRATE = 60;
-    _targetInterval = USECS_PER_SECOND / settings.value("AvatarUpdateTargetSimrate", DEFAULT_TARGET_AVATAR_SIMRATE).toInt();
-}
-// We could have the constructor call initialize(), but GenericThread::initialize can take parameters.
-// Keeping it separately called by the client allows that client to pass those without our
-// constructor needing to know about them.
-
-void AvatarUpdate::synchronousProcess() {
-
-    // Keep our own updated value, so that our asynchronous code can consult it.
-    _isHMDMode = qApp->isHMDMode();
-
-    QSharedPointer<AvatarManager> manager = DependencyManager::get<AvatarManager>();
-    MyAvatar* myAvatar = manager->getMyAvatar();
-    assert(myAvatar);
-
-    // transform the head pose from the displayPlugin into avatar coordinates.
-    glm::mat4 invAvatarMat = glm::inverse(createMatFromQuatAndPos(myAvatar->getOrientation(), myAvatar->getPosition()));
-    _headPose = invAvatarMat * (myAvatar->getSensorToWorldMatrix() * qApp->getActiveDisplayPlugin()->getHeadPose());
-
-    if (!isThreaded()) {
-        process();
-    }
-}
-
-bool AvatarUpdate::process() {
-    PerformanceTimer perfTimer("AvatarUpdate");
-    quint64 start = usecTimestampNow();
-    quint64 deltaMicroseconds = 10000;
-    if (_lastAvatarUpdate > 0) {
-        deltaMicroseconds = start - _lastAvatarUpdate;
-    } else {
-        deltaMicroseconds = 10000; // 10 ms
-    }
-    float deltaSeconds = (float) deltaMicroseconds / (float) USECS_PER_SECOND;
-    assert(deltaSeconds > 0.0f);
-    _lastAvatarUpdate = start;
-    qApp->setAvatarSimrateSample(1.0f / deltaSeconds);
-
-    QSharedPointer<AvatarManager> manager = DependencyManager::get<AvatarManager>();
-    MyAvatar* myAvatar = manager->getMyAvatar();
-
-    //loop through all the other avatars and simulate them...
-    //gets current lookat data, removes missing avatars, etc.
-    manager->updateOtherAvatars(deltaSeconds);
-
-    myAvatar->startUpdate();
-    qApp->updateMyAvatarLookAtPosition();
-    // Sample hardware, update view frustum if needed, and send avatar data to mixer/nodes
-    manager->updateMyAvatar(deltaSeconds);
-    myAvatar->endUpdate();
-
-    if (!isThreaded()) {
-        return true;
-    }
-    int elapsed = (usecTimestampNow() - start);
-    int usecToSleep =  _targetInterval - elapsed;
-    if (usecToSleep < 0) {
-        usecToSleep = 1; // always yield
-    }
-    usleep(usecToSleep);
-    return true;
-}

interface/src/avatar/AvatarUpdate.h

@@ -1,41 +0,0 @@
-//
-//  AvatarUpdate.h
-//  interface/src/avatar
-//
-//  Created by Howard Stearns on 8/18/15.
-///
-//  Distributed under the Apache License, Version 2.0.
-//  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
-//
-//
-
-#ifndef __hifi__AvatarUpdate__
-#define __hifi__AvatarUpdate__
-
-#include <QtCore/QObject>
-#include <QTimer>
-
-// Home for the avatarUpdate operations (e.g., whether on a separate thread, pipelined in various ways, etc.)
-// This might get folded into AvatarManager.
-class AvatarUpdate : public GenericThread {
-    Q_OBJECT
-public:
-    AvatarUpdate();
-    void synchronousProcess();
-
-private:
-    virtual bool process(); // No reason for other classes to invoke this.
-    quint64 _lastAvatarUpdate; // microsoeconds
-    quint64 _targetInterval; // microseconds
-
-    // Goes away if Application::getActiveDisplayPlugin() and friends are made thread safe:
-public:
-    bool isHMDMode() { return _isHMDMode; }
-    glm::mat4 getHeadPose() { return _headPose; }
-private:
-    bool _isHMDMode;
-    glm::mat4 _headPose;
-};
-
-
-#endif /* defined(__hifi__AvatarUpdate__) */

interface/src/avatar/Head.cpp

@@ -382,7 +382,7 @@ glm::quat Head::getCameraOrientation() const {
     // to change the driving direction while in Oculus mode. It is used to support driving toward where you're
     // head is looking. Note that in oculus mode, your actual camera view and where your head is looking is not
     // always the same.
-    if (qApp->getAvatarUpdater()->isHMDMode()) {
+    if (qApp->isHMDMode()) {
         MyAvatar* myAvatar = dynamic_cast<MyAvatar*>(_owningAvatar);
         if (myAvatar) {
             return glm::quat_cast(myAvatar->getSensorToWorldMatrix()) * myAvatar->getHMDSensorOrientation();

interface/src/avatar/MyAvatar.cpp

@@ -225,9 +225,6 @@ QByteArray MyAvatar::toByteArray(bool cullSmallChanges, bool sendAll) {
 }
 
 void MyAvatar::reset(bool andReload) {
-    if (andReload) {
-        qApp->setRawAvatarUpdateThreading(false);
-    }
 
     // Reset dynamic state.
     _wasPushing = _isPushing = _isBraking = false;
@@ -444,7 +441,7 @@ void MyAvatar::updateSensorToWorldMatrix() {
 void MyAvatar::updateFromTrackers(float deltaTime) {
     glm::vec3 estimatedPosition, estimatedRotation;
 
-    bool inHmd = qApp->getAvatarUpdater()->isHMDMode();
+    bool inHmd = qApp->isHMDMode();
     bool playing = DependencyManager::get<recording::Deck>()->isPlaying();
     if (inHmd && playing) {
         return;
@@ -1041,9 +1038,7 @@ void MyAvatar::useFullAvatarURL(const QUrl& fullAvatarURL, const QString& modelN
 
     const QString& urlString = fullAvatarURL.toString();
     if (urlString.isEmpty() || (fullAvatarURL != getSkeletonModelURL())) {
-        qApp->setRawAvatarUpdateThreading(false);
         setSkeletonModelURL(fullAvatarURL);
-        qApp->setRawAvatarUpdateThreading();
         UserActivityLogger::getInstance().changedModel("skeleton", urlString);
     }
     sendIdentityPacket();
@@ -1477,7 +1472,7 @@ void MyAvatar::updateOrientation(float deltaTime) {
 
     getHead()->setBasePitch(getHead()->getBasePitch() + _driveKeys[PITCH] * _pitchSpeed * deltaTime);
 
-    if (qApp->getAvatarUpdater()->isHMDMode()) {
+    if (qApp->isHMDMode()) {
         glm::quat orientation = glm::quat_cast(getSensorToWorldMatrix()) * getHMDSensorOrientation();
         glm::quat bodyOrientation = getWorldBodyOrientation();
         glm::quat localOrientation = glm::inverse(bodyOrientation) * orientation;

interface/src/avatar/SkeletonModel.cpp

@@ -93,12 +93,12 @@ void SkeletonModel::updateRig(float deltaTime, glm::mat4 parentTransform) {
         const FBXGeometry& geometry = _geometry->getFBXGeometry();
 
         Rig::HeadParameters headParams;
-        headParams.enableLean = qApp->getAvatarUpdater()->isHMDMode();
+        headParams.enableLean = qApp->isHMDMode();
         headParams.leanSideways = head->getFinalLeanSideways();
         headParams.leanForward = head->getFinalLeanForward();
         headParams.torsoTwist = head->getTorsoTwist();
 
-        if (qApp->getAvatarUpdater()->isHMDMode()) {
+        if (qApp->isHMDMode()) {
             headParams.isInHMD = true;
 
             // get HMD position from sensor space into world space, and back into rig space

libraries/avatars/src/AvatarData.cpp

@@ -86,7 +86,6 @@ const QUrl& AvatarData::defaultFullAvatarModelUrl() {
 
 // There are a number of possible strategies for this set of tools through endRender, below.
 void AvatarData::nextAttitude(glm::vec3 position, glm::quat orientation) {
-    avatarLock.lock();
     bool success;
     Transform trans = getTransform(success);
     if (!success) {
@@ -99,33 +98,6 @@ void AvatarData::nextAttitude(glm::vec3 position, glm::quat orientation) {
     if (!success) {
         qDebug() << "Warning -- AvatarData::nextAttitude failed";
     }
-    avatarLock.unlock();
-    updateAttitude();
-}
-void AvatarData::startCapture() {
-    avatarLock.lock();
-}
-void AvatarData::endCapture() {
-    avatarLock.unlock();
-}
-void AvatarData::startUpdate() {
-    avatarLock.lock();
-}
-void AvatarData::endUpdate() {
-    avatarLock.unlock();
-}
-void AvatarData::startRenderRun() {
-    // I'd like to get rid of this and just (un)lock at (end-)startRender.
-    // But somehow that causes judder in rotations.
-    avatarLock.lock();
-}
-void AvatarData::endRenderRun() {
-    avatarLock.unlock();
-}
-void AvatarData::startRender() {
-    updateAttitude();
-}
-void AvatarData::endRender() {
     updateAttitude();
 }
 

libraries/avatars/src/AvatarData.h

@@ -209,14 +209,6 @@ public:
     virtual void setOrientation(const glm::quat& orientation) override;
 
     void nextAttitude(glm::vec3 position, glm::quat orientation); // Can be safely called at any time.
-    void startCapture();    // start/end of the period in which the latest values are about to be captured for camera, etc.
-    void endCapture();
-    void startUpdate();     // start/end of update iteration
-    void endUpdate();
-    void startRender();     // start/end of rendering of this object
-    void startRenderRun();  // start/end of entire scene.
-    void endRenderRun();
-    void endRender();
     virtual void updateAttitude() {} // Tell skeleton mesh about changes
 
     glm::quat getHeadOrientation() const { return _headData->getOrientation(); }
@@ -403,8 +395,6 @@ protected:
 
     SimpleMovingAverage _averageBytesReceived;
 
-    QMutex avatarLock; // Name is redundant, but it aids searches.
-
     // During recording, this holds the starting position, orientation & scale of the recorded avatar
     // During playback, it holds the origin from which to play the relative positions in the clip
     TransformPointer _recordingBasis;