From cad63ad1071e9a3eb7f742ac7543d9107c089682 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 1 May 2018 14:15:12 -0700 Subject: [PATCH] Revert "Revert "HOTFIX version of PR 12969: Attempt to shutdown web surfaces more consistently"" This reverts commit acb21cc96a459721bf51638bba15855e36892cd2. --- interface/src/Application.cpp | 13 ++++- interface/src/Application.h | 7 ++- interface/src/ui/overlays/Web3DOverlay.cpp | 12 ++-- .../src/RenderableWebEntityItem.cpp | 39 +++++++------ libraries/qml/src/qml/OffscreenSurface.cpp | 6 +- .../qml/src/qml/impl/RenderEventHandler.cpp | 11 ++-- libraries/qml/src/qml/impl/SharedObject.cpp | 56 +++++++++++++------ .../src/AbstractViewStateInterface.h | 19 +++++-- tests/render-perf/src/main.cpp | 6 +- 9 files changed, 101 insertions(+), 68 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 789f60bdb1..c38caca090 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -4750,7 +4750,7 @@ void Application::updateLOD(float deltaTime) const { } } -void Application::pushPostUpdateLambda(void* key, std::function func) { +void Application::pushPostUpdateLambda(void* key, const std::function& func) { std::unique_lock guard(_postUpdateLambdasLock); _postUpdateLambdas[key] = func; } @@ -7360,7 +7360,7 @@ void Application::windowMinimizedChanged(bool minimized) { } } -void Application::postLambdaEvent(std::function f) { +void Application::postLambdaEvent(const std::function& f) { if (this->thread() == QThread::currentThread()) { f(); } else { @@ -7368,6 +7368,15 @@ void Application::postLambdaEvent(std::function f) { } } +void Application::sendLambdaEvent(const std::function& f) { + if (this->thread() == QThread::currentThread()) { + f(); + } else { + LambdaEvent event(f); + QCoreApplication::sendEvent(this, &event); + } +} + void Application::initPlugins(const QStringList& arguments) { QCommandLineOption display("display", "Preferred displays", "displays"); QCommandLineOption disableDisplays("disable-displays", "Displays to disable", "displays"); diff --git a/interface/src/Application.h b/interface/src/Application.h index 769658b0d6..74b0e5a110 100644 --- a/interface/src/Application.h +++ b/interface/src/Application.h @@ -136,7 +136,8 @@ public: Application(int& argc, char** argv, QElapsedTimer& startup_time, bool runningMarkerExisted); ~Application(); - void postLambdaEvent(std::function f) override; + void postLambdaEvent(const std::function& f) override; + void sendLambdaEvent(const std::function& f) override; QString getPreviousScriptLocation(); void setPreviousScriptLocation(const QString& previousScriptLocation); @@ -240,7 +241,7 @@ public: qint64 getCurrentSessionRuntime() const { return _sessionRunTimer.elapsed(); } - bool isAboutToQuit() const override { return _aboutToQuit; } + bool isAboutToQuit() const { return _aboutToQuit; } bool isPhysicsEnabled() const { return _physicsEnabled; } // the isHMDMode is true whenever we use the interface from an HMD and not a standard flat display @@ -264,7 +265,7 @@ public: render::EnginePointer getRenderEngine() override { return _renderEngine; } gpu::ContextPointer getGPUContext() const { return _gpuContext; } - virtual void pushPostUpdateLambda(void* key, std::function func) override; + virtual void pushPostUpdateLambda(void* key, const std::function& func) override; void updateMyAvatarLookAtPosition(); diff --git a/interface/src/ui/overlays/Web3DOverlay.cpp b/interface/src/ui/overlays/Web3DOverlay.cpp index 1e0b50f091..fcdac8c00c 100644 --- a/interface/src/ui/overlays/Web3DOverlay.cpp +++ b/interface/src/ui/overlays/Web3DOverlay.cpp @@ -65,14 +65,10 @@ const QString Web3DOverlay::TYPE = "web3d"; const QString Web3DOverlay::QML = "Web3DOverlay.qml"; static auto qmlSurfaceDeleter = [](OffscreenQmlSurface* surface) { - AbstractViewStateInterface::instance()->postLambdaEvent([surface] { - if (AbstractViewStateInterface::instance()->isAboutToQuit()) { - // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown - // if the application has already stopped its event loop, delete must be explicit - delete surface; - } else { - surface->deleteLater(); - } + AbstractViewStateInterface::instance()->sendLambdaEvent([surface] { + // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown + // if the application has already stopped its event loop, delete must be explicit + delete surface; }); }; diff --git a/libraries/entities-renderer/src/RenderableWebEntityItem.cpp b/libraries/entities-renderer/src/RenderableWebEntityItem.cpp index 3b46c530cc..f333e805ce 100644 --- a/libraries/entities-renderer/src/RenderableWebEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableWebEntityItem.cpp @@ -25,7 +25,7 @@ #include #include "EntitiesRendererLogging.h" - +#include using namespace render; using namespace render::entities; @@ -45,6 +45,7 @@ static int DEFAULT_MAX_FPS = 10; static int YOUTUBE_MAX_FPS = 30; static QTouchDevice _touchDevice; +static const char* URL_PROPERTY = "url"; WebEntityRenderer::ContentType WebEntityRenderer::getContentType(const QString& urlString) { if (urlString.isEmpty()) { @@ -52,7 +53,7 @@ WebEntityRenderer::ContentType WebEntityRenderer::getContentType(const QString& } const QUrl url(urlString); - if (url.scheme() == "http" || url.scheme() == "https" || + if (url.scheme() == URL_SCHEME_HTTP || url.scheme() == URL_SCHEME_HTTPS || urlString.toLower().endsWith(".htm") || urlString.toLower().endsWith(".html")) { return ContentType::HtmlContent; } @@ -164,6 +165,8 @@ void WebEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scene if (urlChanged) { if (newContentType != ContentType::HtmlContent || currentContentType != ContentType::HtmlContent) { destroyWebSurface(); + // If we destroyed the surface, the URL change will be implicitly handled by the re-creation + urlChanged = false; } withWriteLock([&] { @@ -185,8 +188,8 @@ void WebEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scene return; } - if (urlChanged) { - _webSurface->getRootItem()->setProperty("url", _lastSourceUrl); + if (urlChanged && _contentType == ContentType::HtmlContent) { + _webSurface->getRootItem()->setProperty(URL_PROPERTY, _lastSourceUrl); } if (_contextPosition != entity->getWorldPosition()) { @@ -257,6 +260,14 @@ bool WebEntityRenderer::hasWebSurface() { return (bool)_webSurface && _webSurface->getRootItem(); } +static const auto WebSurfaceDeleter = [](OffscreenQmlSurface* webSurface) { + AbstractViewStateInterface::instance()->sendLambdaEvent([webSurface] { + // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown + // if the application has already stopped its event loop, delete must be explicit + delete webSurface; + }); +}; + bool WebEntityRenderer::buildWebSurface(const TypedEntityPointer& entity) { if (_currentWebCount >= MAX_CONCURRENT_WEB_VIEWS) { qWarning() << "Too many concurrent web views to create new view"; @@ -264,20 +275,9 @@ bool WebEntityRenderer::buildWebSurface(const TypedEntityPointer& entity) { } ++_currentWebCount; - auto deleter = [](OffscreenQmlSurface* webSurface) { - AbstractViewStateInterface::instance()->postLambdaEvent([webSurface] { - if (AbstractViewStateInterface::instance()->isAboutToQuit()) { - // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown - // if the application has already stopped its event loop, delete must be explicit - delete webSurface; - } else { - webSurface->deleteLater(); - } - }); - }; // FIXME use the surface cache instead of explicit creation - _webSurface = QSharedPointer(new OffscreenQmlSurface(), deleter); + _webSurface = QSharedPointer(new OffscreenQmlSurface(), WebSurfaceDeleter); // FIXME, the max FPS could be better managed by being dynamic (based on the number of current surfaces // and the current rendering load) _webSurface->setMaxFps(DEFAULT_MAX_FPS); @@ -305,7 +305,7 @@ bool WebEntityRenderer::buildWebSurface(const TypedEntityPointer& entity) { _webSurface->setMaxFps(DEFAULT_MAX_FPS); } _webSurface->load("controls/WebEntityView.qml", [this](QQmlContext* context, QObject* item) { - item->setProperty("url", _lastSourceUrl); + item->setProperty(URL_PROPERTY, _lastSourceUrl); }); } else if (_contentType == ContentType::QmlContent) { _webSurface->load(_lastSourceUrl, [this](QQmlContext* context, QObject* item) { @@ -330,6 +330,11 @@ void WebEntityRenderer::destroyWebSurface() { if (webSurface) { --_currentWebCount; QQuickItem* rootItem = webSurface->getRootItem(); + // Explicitly set the web URL to an empty string, in an effort to get a + // faster shutdown of any chromium processes interacting with audio + if (rootItem && _contentType == ContentType::HtmlContent) { + rootItem->setProperty(URL_PROPERTY, ""); + } if (rootItem && rootItem->objectName() == "tabletRoot") { auto tabletScriptingInterface = DependencyManager::get(); diff --git a/libraries/qml/src/qml/OffscreenSurface.cpp b/libraries/qml/src/qml/OffscreenSurface.cpp index 2da1c41340..688f3fdb5f 100644 --- a/libraries/qml/src/qml/OffscreenSurface.cpp +++ b/libraries/qml/src/qml/OffscreenSurface.cpp @@ -66,14 +66,10 @@ OffscreenSurface::OffscreenSurface() } OffscreenSurface::~OffscreenSurface() { - disconnect(qApp); - _sharedObject->destroy(); + delete _sharedObject; } bool OffscreenSurface::fetchTexture(TextureAndFence& textureAndFence) { - if (!_sharedObject) { - return false; - } hifi::qml::impl::TextureAndFence typedTextureAndFence; bool result = _sharedObject->fetchTexture(typedTextureAndFence); textureAndFence = typedTextureAndFence; diff --git a/libraries/qml/src/qml/impl/RenderEventHandler.cpp b/libraries/qml/src/qml/impl/RenderEventHandler.cpp index 6b66ce9314..945a469611 100644 --- a/libraries/qml/src/qml/impl/RenderEventHandler.cpp +++ b/libraries/qml/src/qml/impl/RenderEventHandler.cpp @@ -49,8 +49,8 @@ RenderEventHandler::RenderEventHandler(SharedObject* shared, QThread* targetThre qFatal("Unable to create new offscreen GL context"); } - moveToThread(targetThread); _canvas.moveToThreadWithContext(targetThread); + moveToThread(targetThread); } void RenderEventHandler::onInitalize() { @@ -160,11 +160,8 @@ void RenderEventHandler::onQuit() { } _shared->shutdownRendering(_canvas, _currentSize); - // Release the reference to the shared object. This will allow it to - // be destroyed (should happen on it's own thread). - _shared->deleteLater(); - - deleteLater(); - + _canvas.doneCurrent(); + _canvas.moveToThreadWithContext(qApp->thread()); + moveToThread(qApp->thread()); QThread::currentThread()->quit(); } diff --git a/libraries/qml/src/qml/impl/SharedObject.cpp b/libraries/qml/src/qml/impl/SharedObject.cpp index b2057117f6..9253c41b39 100644 --- a/libraries/qml/src/qml/impl/SharedObject.cpp +++ b/libraries/qml/src/qml/impl/SharedObject.cpp @@ -72,26 +72,35 @@ SharedObject::SharedObject() { QObject::connect(qApp, &QCoreApplication::aboutToQuit, this, &SharedObject::onAboutToQuit); } + SharedObject::~SharedObject() { - if (_quickWindow) { - _quickWindow->destroy(); - _quickWindow = nullptr; + // After destroy returns, the rendering thread should be gone + destroy(); + + // _renderTimer is created with `this` as the parent, so need no explicit destruction + + // Destroy the event hand + if (_renderObject) { + delete _renderObject; + _renderObject = nullptr; } if (_renderControl) { - _renderControl->deleteLater(); + delete _renderControl; _renderControl = nullptr; } - if (_renderThread) { - _renderThread->quit(); - _renderThread->deleteLater(); + if (_quickWindow) { + _quickWindow->destroy(); + delete _quickWindow; + _quickWindow = nullptr; } - if (_rootItem) { - _rootItem->deleteLater(); - _rootItem = nullptr; - } + // _rootItem is parented to the quickWindow, so needs no explicit destruction + //if (_rootItem) { + // delete _rootItem; + // _rootItem = nullptr; + //} releaseEngine(_qmlContext->engine()); } @@ -119,6 +128,10 @@ void SharedObject::create(OffscreenSurface* surface) { } void SharedObject::setRootItem(QQuickItem* rootItem) { + if (_quit) { + return; + } + _rootItem = rootItem; _rootItem->setSize(_quickWindow->size()); @@ -127,7 +140,6 @@ void SharedObject::setRootItem(QQuickItem* rootItem) { _renderThread->setObjectName(objectName()); _renderThread->start(); - // Create event handler for the render thread _renderObject = new RenderEventHandler(this, _renderThread); QCoreApplication::postEvent(this, new OffscreenEvent(OffscreenEvent::Initialize)); @@ -137,35 +149,43 @@ void SharedObject::setRootItem(QQuickItem* rootItem) { } void SharedObject::destroy() { + // `destroy` is idempotent, it can be called multiple times without issues if (_quit) { return; } if (!_rootItem) { - deleteLater(); return; } - _paused = true; if (_renderTimer) { + _renderTimer->stop(); QObject::disconnect(_renderTimer); - _renderTimer->deleteLater(); } - QObject::disconnect(_renderControl); + if (_renderControl) { + QObject::disconnect(_renderControl); + } + QObject::disconnect(qApp); { QMutexLocker lock(&_mutex); _quit = true; - QCoreApplication::postEvent(_renderObject, new OffscreenEvent(OffscreenEvent::Quit), Qt::HighEventPriority); + if (_renderObject) { + QCoreApplication::postEvent(_renderObject, new OffscreenEvent(OffscreenEvent::Quit), Qt::HighEventPriority); + } } // Block until the rendering thread has stopped // FIXME this is undesirable because this is blocking the main thread, // but I haven't found a reliable way to do this only at application // shutdown - _renderThread->wait(); + if (_renderThread) { + _renderThread->wait(); + delete _renderThread; + _renderThread = nullptr; + } } diff --git a/libraries/render-utils/src/AbstractViewStateInterface.h b/libraries/render-utils/src/AbstractViewStateInterface.h index 96e9f4d222..54fdc903ca 100644 --- a/libraries/render-utils/src/AbstractViewStateInterface.h +++ b/libraries/render-utils/src/AbstractViewStateInterface.h @@ -37,15 +37,25 @@ public: virtual glm::vec3 getAvatarPosition() const = 0; - virtual bool isAboutToQuit() const = 0; - virtual void postLambdaEvent(std::function f) = 0; + // Unfortunately, having this here is a bad idea. Lots of objects connect to + // the aboutToQuit signal, and it's impossible to know the order in which + // the receivers will be called, so this might return false negatives + //virtual bool isAboutToQuit() const = 0; + + // Queue code to execute on the main thread. + // If called from the main thread, the lambda will execute synchronously + virtual void postLambdaEvent(const std::function& f) = 0; + // Synchronously execute code on the main thread. This function will + // not return until the code is executed, regardles of which thread it + // is called from + virtual void sendLambdaEvent(const std::function& f) = 0; virtual qreal getDevicePixelRatio() = 0; virtual render::ScenePointer getMain3DScene() = 0; virtual render::EnginePointer getRenderEngine() = 0; - virtual void pushPostUpdateLambda(void* key, std::function func) = 0; + virtual void pushPostUpdateLambda(void* key, const std::function& func) = 0; virtual bool isHMDMode() const = 0; @@ -54,5 +64,4 @@ public: static void setInstance(AbstractViewStateInterface* instance); }; - -#endif // hifi_AbstractViewStateInterface_h +#endif // hifi_AbstractViewStateInterface_h diff --git a/tests/render-perf/src/main.cpp b/tests/render-perf/src/main.cpp index 93672cc5a2..9249b3d957 100644 --- a/tests/render-perf/src/main.cpp +++ b/tests/render-perf/src/main.cpp @@ -453,8 +453,8 @@ protected: return vec3(); } - bool isAboutToQuit() const override { return false; } - void postLambdaEvent(std::function f) override {} + void postLambdaEvent(const std::function& f) override {} + void sendLambdaEvent(const std::function& f) override {} qreal getDevicePixelRatio() override { return 1.0f; @@ -469,7 +469,7 @@ protected: } std::map> _postUpdateLambdas; - void pushPostUpdateLambda(void* key, std::function func) override { + void pushPostUpdateLambda(void* key, const std::function& func) override { _postUpdateLambdas[key] = func; }