From 1f83d04423d476b25310783ecb1c6a2b5ff2c2e3 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Tue, 1 Sep 2015 14:55:09 -0700 Subject: [PATCH] Fix oculus crash on switching display plugin to something else --- interface/src/Application.cpp | 133 +++++++++++------- interface/src/Application.h | 1 + interface/src/PluginContainerProxy.cpp | 12 -- .../src/display-plugins/DisplayPlugin.h | 6 + .../src/display-plugins/NullDisplayPlugin.cpp | 1 + .../src/display-plugins/NullDisplayPlugin.h | 1 + .../display-plugins/OpenGLDisplayPlugin.cpp | 11 +- .../src/display-plugins/OpenGLDisplayPlugin.h | 3 +- .../oculus/OculusDisplayPlugin.cpp | 6 - libraries/shared/src/Finally.h | 27 ++++ 10 files changed, 133 insertions(+), 68 deletions(-) create mode 100644 libraries/shared/src/Finally.h diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index e0726311c9..391ba748d0 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -64,6 +64,7 @@ #include #include +#include #include #include #include @@ -1009,6 +1010,16 @@ void Application::paintGL() { if (nullptr == _displayPlugin) { 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(); displayPlugin->preRender(); _offscreenContext->makeCurrent(); @@ -1203,7 +1214,7 @@ void Application::paintGL() { // Ensure all operations from the previous context are complete before we try to read the fbo glWaitSync(sync, 0, GL_TIMEOUT_IGNORED); glDeleteSync(sync); - + { PROFILE_RANGE(__FUNCTION__ "/pluginDisplay"); displayPlugin->display(finalTexture, toGlm(size)); @@ -1219,7 +1230,6 @@ void Application::paintGL() { _frameCount++; Stats::getInstance()->setRenderDetails(renderArgs._details); - // Reset the gpu::Context Stages // Back to the default framebuffer; gpu::Batch batch; @@ -4703,53 +4713,68 @@ void Application::updateDisplayMode() { auto offscreenUi = DependencyManager::get(); DisplayPluginPointer oldDisplayPlugin = _displayPlugin; - if (oldDisplayPlugin != newDisplayPlugin) { - if (!_currentDisplayPluginActions.isEmpty()) { - auto menu = Menu::getInstance(); - foreach(auto itemInfo, _currentDisplayPluginActions) { - menu->removeMenuItem(itemInfo.first, itemInfo.second); - } - _currentDisplayPluginActions.clear(); - } - - if (newDisplayPlugin) { - _offscreenContext->makeCurrent(); - _activatingDisplayPlugin = true; - newDisplayPlugin->activate(); - _activatingDisplayPlugin = false; - _offscreenContext->makeCurrent(); - offscreenUi->resize(fromGlm(newDisplayPlugin->getRecommendedUiSize())); - _offscreenContext->makeCurrent(); - } - - oldDisplayPlugin = _displayPlugin; - _displayPlugin = newDisplayPlugin; - - // If the displayPlugin is a screen based HMD, then it will want the HMDTools displayed - // Direct Mode HMDs (like windows Oculus) will be isHmd() but will have a screen of -1 - bool newPluginWantsHMDTools = newDisplayPlugin ? - (newDisplayPlugin->isHmd() && (newDisplayPlugin->getHmdScreen() >= 0)) : false; - bool oldPluginWantedHMDTools = oldDisplayPlugin ? - (oldDisplayPlugin->isHmd() && (oldDisplayPlugin->getHmdScreen() >= 0)) : false; - - // Only show the hmd tools after the correct plugin has - // been activated so that it's UI is setup correctly - if (newPluginWantsHMDTools) { - _pluginContainer->showDisplayPluginsTools(); - } - - if (oldDisplayPlugin) { - oldDisplayPlugin->deactivate(); - _offscreenContext->makeCurrent(); - - // if the old plugin was HMD and the new plugin is not HMD, then hide our hmdtools - if (oldPluginWantedHMDTools && !newPluginWantsHMDTools) { - DependencyManager::get()->hmdTools(false); - } - } - emit activeDisplayPluginChanged(); - resetSensors(); + if (newDisplayPlugin == oldDisplayPlugin) { + return; } + + // Some plugins *cough* Oculus *cough* process message events from inside their + // display function, and we don't want to change the display plugin underneath + // the paintGL call, so we need to guard against that + if (_inPaint) { + qDebug() << "Deferring plugin switch until out of painting"; + // Have the old plugin stop requesting renders + oldDisplayPlugin->stop(); + QCoreApplication::postEvent(this, new LambdaEvent([this] { + updateDisplayMode(); + })); + return; + } + + if (!_currentDisplayPluginActions.isEmpty()) { + auto menu = Menu::getInstance(); + foreach(auto itemInfo, _currentDisplayPluginActions) { + menu->removeMenuItem(itemInfo.first, itemInfo.second); + } + _currentDisplayPluginActions.clear(); + } + + if (newDisplayPlugin) { + _offscreenContext->makeCurrent(); + _activatingDisplayPlugin = true; + newDisplayPlugin->activate(); + _activatingDisplayPlugin = false; + _offscreenContext->makeCurrent(); + offscreenUi->resize(fromGlm(newDisplayPlugin->getRecommendedUiSize())); + _offscreenContext->makeCurrent(); + } + + oldDisplayPlugin = _displayPlugin; + _displayPlugin = newDisplayPlugin; + + // If the displayPlugin is a screen based HMD, then it will want the HMDTools displayed + // Direct Mode HMDs (like windows Oculus) will be isHmd() but will have a screen of -1 + bool newPluginWantsHMDTools = newDisplayPlugin ? + (newDisplayPlugin->isHmd() && (newDisplayPlugin->getHmdScreen() >= 0)) : false; + bool oldPluginWantedHMDTools = oldDisplayPlugin ? + (oldDisplayPlugin->isHmd() && (oldDisplayPlugin->getHmdScreen() >= 0)) : false; + + // Only show the hmd tools after the correct plugin has + // been activated so that it's UI is setup correctly + if (newPluginWantsHMDTools) { + _pluginContainer->showDisplayPluginsTools(); + } + + if (oldDisplayPlugin) { + oldDisplayPlugin->deactivate(); + _offscreenContext->makeCurrent(); + + // if the old plugin was HMD and the new plugin is not HMD, then hide our hmdtools + if (oldPluginWantedHMDTools && !newPluginWantsHMDTools) { + DependencyManager::get()->hmdTools(false); + } + } + emit activeDisplayPluginChanged(); + resetSensors(); Q_ASSERT_X(_displayPlugin, "Application::updateDisplayMode", "could not find an activated display plugin"); } @@ -5052,3 +5077,15 @@ void Application::crashApplication() { qCDebug(interfaceapp) << "Intentionally crashed Interface"; } + +void Application::setActiveDisplayPlugin(const QString& pluginName) { + auto menu = Menu::getInstance(); + foreach(DisplayPluginPointer displayPlugin, PluginManager::getInstance()->getDisplayPlugins()) { + QString name = displayPlugin->getName(); + QAction* action = menu->getActionForOption(name); + if (pluginName == name) { + action->setChecked(true); + } + } + updateDisplayMode(); +} diff --git a/interface/src/Application.h b/interface/src/Application.h index 9b819eae53..6c3b68cf6f 100644 --- a/interface/src/Application.h +++ b/interface/src/Application.h @@ -673,6 +673,7 @@ private: int _simsPerSecondReport = 0; quint64 _lastSimsPerSecondUpdate = 0; bool _isForeground = true; // starts out assumed to be in foreground + bool _inPaint = false; friend class PluginContainerProxy; }; diff --git a/interface/src/PluginContainerProxy.cpp b/interface/src/PluginContainerProxy.cpp index e172dbbd9e..4ef755bd1b 100644 --- a/interface/src/PluginContainerProxy.cpp +++ b/interface/src/PluginContainerProxy.cpp @@ -147,15 +147,3 @@ void PluginContainerProxy::showDisplayPluginsTools() { QGLWidget* PluginContainerProxy::getPrimarySurface() { return qApp->_glWidget; } - -void Application::setActiveDisplayPlugin(const QString& pluginName) { - auto menu = Menu::getInstance(); - foreach(DisplayPluginPointer displayPlugin, PluginManager::getInstance()->getDisplayPlugins()) { - QString name = displayPlugin->getName(); - QAction* action = menu->getActionForOption(name); - if (pluginName == name) { - action->setChecked(true); - } - } - updateDisplayMode(); -} diff --git a/libraries/display-plugins/src/display-plugins/DisplayPlugin.h b/libraries/display-plugins/src/display-plugins/DisplayPlugin.h index a9220d68f6..86cfabe724 100644 --- a/libraries/display-plugins/src/display-plugins/DisplayPlugin.h +++ b/libraries/display-plugins/src/display-plugins/DisplayPlugin.h @@ -57,6 +57,11 @@ public: // Rendering support + // Stop requesting renders, but don't do full deactivation + // needed to work around the issues caused by Oculus + // processing messages in the middle of submitFrame + virtual void stop() = 0; + /** * Called by the application before the frame rendering. Can be used for * render timing related calls (for instance, the Oculus begin frame timing @@ -120,6 +125,7 @@ public: virtual void resetSensors() {} virtual float devicePixelRatio() { return 1.0; } + static const QString& MENU_PATH(); signals: void recommendedFramebufferSizeChanged(const QSize & size); diff --git a/libraries/display-plugins/src/display-plugins/NullDisplayPlugin.cpp b/libraries/display-plugins/src/display-plugins/NullDisplayPlugin.cpp index e5a96d167e..1f8242f081 100644 --- a/libraries/display-plugins/src/display-plugins/NullDisplayPlugin.cpp +++ b/libraries/display-plugins/src/display-plugins/NullDisplayPlugin.cpp @@ -30,3 +30,4 @@ void NullDisplayPlugin::finishFrame() {} void NullDisplayPlugin::activate() {} void NullDisplayPlugin::deactivate() {} +void NullDisplayPlugin::stop() {} diff --git a/libraries/display-plugins/src/display-plugins/NullDisplayPlugin.h b/libraries/display-plugins/src/display-plugins/NullDisplayPlugin.h index 90e717b5ee..bb1ab2d97f 100644 --- a/libraries/display-plugins/src/display-plugins/NullDisplayPlugin.h +++ b/libraries/display-plugins/src/display-plugins/NullDisplayPlugin.h @@ -17,6 +17,7 @@ public: void activate() override; void deactivate() override; + void stop() override; virtual glm::uvec2 getRecommendedRenderSize() const override; virtual bool hasFocus() const override; diff --git a/libraries/display-plugins/src/display-plugins/OpenGLDisplayPlugin.cpp b/libraries/display-plugins/src/display-plugins/OpenGLDisplayPlugin.cpp index 4101bebfa8..cfe101e1ab 100644 --- a/libraries/display-plugins/src/display-plugins/OpenGLDisplayPlugin.cpp +++ b/libraries/display-plugins/src/display-plugins/OpenGLDisplayPlugin.cpp @@ -16,7 +16,9 @@ OpenGLDisplayPlugin::OpenGLDisplayPlugin() { connect(&_timer, &QTimer::timeout, this, [&] { - emit requestRender(); + if (_active) { + emit requestRender(); + } }); } @@ -56,10 +58,17 @@ void OpenGLDisplayPlugin::customizeContext() { } void OpenGLDisplayPlugin::activate() { + _active = true; _timer.start(1); } +void OpenGLDisplayPlugin::stop() { + _active = false; + _timer.stop(); +} + void OpenGLDisplayPlugin::deactivate() { + _active = false; _timer.stop(); makeCurrent(); diff --git a/libraries/display-plugins/src/display-plugins/OpenGLDisplayPlugin.h b/libraries/display-plugins/src/display-plugins/OpenGLDisplayPlugin.h index 0dc94b72f5..52715ebde7 100644 --- a/libraries/display-plugins/src/display-plugins/OpenGLDisplayPlugin.h +++ b/libraries/display-plugins/src/display-plugins/OpenGLDisplayPlugin.h @@ -25,7 +25,7 @@ public: virtual void activate() override; virtual void deactivate() override; - + virtual void stop() override; virtual bool eventFilter(QObject* receiver, QEvent* event) override; virtual void display(GLuint sceneTexture, const glm::uvec2& sceneSize) override; @@ -44,6 +44,7 @@ protected: mutable QTimer _timer; ProgramPtr _program; ShapeWrapperPtr _plane; + bool _active{ false }; bool _vsyncSupported{ false }; }; diff --git a/libraries/display-plugins/src/display-plugins/oculus/OculusDisplayPlugin.cpp b/libraries/display-plugins/src/display-plugins/oculus/OculusDisplayPlugin.cpp index ff218987ec..c214b7e627 100644 --- a/libraries/display-plugins/src/display-plugins/oculus/OculusDisplayPlugin.cpp +++ b/libraries/display-plugins/src/display-plugins/oculus/OculusDisplayPlugin.cpp @@ -350,11 +350,6 @@ void OculusDisplayPlugin::deactivate() { } void OculusDisplayPlugin::display(GLuint finalTexture, const glm::uvec2& sceneSize) { - static bool inDisplay = false; - if (inDisplay) { - return; - } - inDisplay = true; #if (OVR_MAJOR_VERSION >= 6) using namespace oglplus; // Need to make sure only the display plugin is responsible for @@ -420,7 +415,6 @@ void OculusDisplayPlugin::display(GLuint finalTexture, const glm::uvec2& sceneSi ++_frameIndex; #endif - inDisplay = false; } // Pass input events on to the application diff --git a/libraries/shared/src/Finally.h b/libraries/shared/src/Finally.h new file mode 100644 index 0000000000..59e8be0228 --- /dev/null +++ b/libraries/shared/src/Finally.h @@ -0,0 +1,27 @@ +// +// Created by Bradley Austin Davis on 2015/09/01 +// Copyright 2013-2105 High Fidelity, Inc. +// +// Distributed under the Apache License, Version 2.0. +// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html +// + +// Simulates a java finally block by executing a lambda when an instance leaves +// scope + +#include + +#pragma once +#ifndef hifi_Finally_h +#define hifi_Finally_h + +class Finally { +public: + template + Finally(F f) : _f(f) {} + ~Finally() { _f(); } +private: + std::function _f; +}; + +#endif