From acb21cc96a459721bf51638bba15855e36892cd2 Mon Sep 17 00:00:00 2001 From: Bradley Austin Davis Date: Wed, 25 Apr 2018 09:43:20 -0700 Subject: [PATCH] Revert "HOTFIX version of PR 12969: Attempt to shutdown web surfaces more consistently" --- 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 | 62 +++++++------------ .../src/AbstractViewStateInterface.h | 19 ++---- tests/render-perf/src/main.cpp | 6 +- 9 files changed, 71 insertions(+), 104 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index c38caca090..789f60bdb1 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, const std::function& func) { +void Application::pushPostUpdateLambda(void* key, std::function func) { std::unique_lock guard(_postUpdateLambdasLock); _postUpdateLambdas[key] = func; } @@ -7360,7 +7360,7 @@ void Application::windowMinimizedChanged(bool minimized) { } } -void Application::postLambdaEvent(const std::function& f) { +void Application::postLambdaEvent(std::function f) { if (this->thread() == QThread::currentThread()) { f(); } else { @@ -7368,15 +7368,6 @@ void Application::postLambdaEvent(const 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 74b0e5a110..769658b0d6 100644 --- a/interface/src/Application.h +++ b/interface/src/Application.h @@ -136,8 +136,7 @@ public: Application(int& argc, char** argv, QElapsedTimer& startup_time, bool runningMarkerExisted); ~Application(); - void postLambdaEvent(const std::function& f) override; - void sendLambdaEvent(const std::function& f) override; + void postLambdaEvent(std::function f) override; QString getPreviousScriptLocation(); void setPreviousScriptLocation(const QString& previousScriptLocation); @@ -241,7 +240,7 @@ public: qint64 getCurrentSessionRuntime() const { return _sessionRunTimer.elapsed(); } - bool isAboutToQuit() const { return _aboutToQuit; } + bool isAboutToQuit() const override { 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 @@ -265,7 +264,7 @@ public: render::EnginePointer getRenderEngine() override { return _renderEngine; } gpu::ContextPointer getGPUContext() const { return _gpuContext; } - virtual void pushPostUpdateLambda(void* key, const std::function& func) override; + virtual void pushPostUpdateLambda(void* key, std::function func) override; void updateMyAvatarLookAtPosition(); diff --git a/interface/src/ui/overlays/Web3DOverlay.cpp b/interface/src/ui/overlays/Web3DOverlay.cpp index fcdac8c00c..1e0b50f091 100644 --- a/interface/src/ui/overlays/Web3DOverlay.cpp +++ b/interface/src/ui/overlays/Web3DOverlay.cpp @@ -65,10 +65,14 @@ const QString Web3DOverlay::TYPE = "web3d"; const QString Web3DOverlay::QML = "Web3DOverlay.qml"; static auto qmlSurfaceDeleter = [](OffscreenQmlSurface* surface) { - 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; + 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(); + } }); }; diff --git a/libraries/entities-renderer/src/RenderableWebEntityItem.cpp b/libraries/entities-renderer/src/RenderableWebEntityItem.cpp index f333e805ce..3b46c530cc 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,7 +45,6 @@ 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()) { @@ -53,7 +52,7 @@ WebEntityRenderer::ContentType WebEntityRenderer::getContentType(const QString& } const QUrl url(urlString); - if (url.scheme() == URL_SCHEME_HTTP || url.scheme() == URL_SCHEME_HTTPS || + if (url.scheme() == "http" || url.scheme() == "https" || urlString.toLower().endsWith(".htm") || urlString.toLower().endsWith(".html")) { return ContentType::HtmlContent; } @@ -165,8 +164,6 @@ 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([&] { @@ -188,8 +185,8 @@ void WebEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scene return; } - if (urlChanged && _contentType == ContentType::HtmlContent) { - _webSurface->getRootItem()->setProperty(URL_PROPERTY, _lastSourceUrl); + if (urlChanged) { + _webSurface->getRootItem()->setProperty("url", _lastSourceUrl); } if (_contextPosition != entity->getWorldPosition()) { @@ -260,14 +257,6 @@ 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"; @@ -275,9 +264,20 @@ 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(), WebSurfaceDeleter); + _webSurface = QSharedPointer(new OffscreenQmlSurface(), deleter); // 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_PROPERTY, _lastSourceUrl); + item->setProperty("url", _lastSourceUrl); }); } else if (_contentType == ContentType::QmlContent) { _webSurface->load(_lastSourceUrl, [this](QQmlContext* context, QObject* item) { @@ -330,11 +330,6 @@ 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 688f3fdb5f..2da1c41340 100644 --- a/libraries/qml/src/qml/OffscreenSurface.cpp +++ b/libraries/qml/src/qml/OffscreenSurface.cpp @@ -66,10 +66,14 @@ OffscreenSurface::OffscreenSurface() } OffscreenSurface::~OffscreenSurface() { - delete _sharedObject; + disconnect(qApp); + _sharedObject->destroy(); } 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 945a469611..6b66ce9314 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"); } - _canvas.moveToThreadWithContext(targetThread); moveToThread(targetThread); + _canvas.moveToThreadWithContext(targetThread); } void RenderEventHandler::onInitalize() { @@ -160,8 +160,11 @@ void RenderEventHandler::onQuit() { } _shared->shutdownRendering(_canvas, _currentSize); - _canvas.doneCurrent(); - _canvas.moveToThreadWithContext(qApp->thread()); - moveToThread(qApp->thread()); + // Release the reference to the shared object. This will allow it to + // be destroyed (should happen on it's own thread). + _shared->deleteLater(); + + deleteLater(); + QThread::currentThread()->quit(); } diff --git a/libraries/qml/src/qml/impl/SharedObject.cpp b/libraries/qml/src/qml/impl/SharedObject.cpp index 9253c41b39..b2057117f6 100644 --- a/libraries/qml/src/qml/impl/SharedObject.cpp +++ b/libraries/qml/src/qml/impl/SharedObject.cpp @@ -72,35 +72,26 @@ SharedObject::SharedObject() { QObject::connect(qApp, &QCoreApplication::aboutToQuit, this, &SharedObject::onAboutToQuit); } - SharedObject::~SharedObject() { - // 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) { - delete _renderControl; - _renderControl = nullptr; - } - if (_quickWindow) { _quickWindow->destroy(); - delete _quickWindow; _quickWindow = nullptr; } - // _rootItem is parented to the quickWindow, so needs no explicit destruction - //if (_rootItem) { - // delete _rootItem; - // _rootItem = nullptr; - //} + if (_renderControl) { + _renderControl->deleteLater(); + _renderControl = nullptr; + } + + if (_renderThread) { + _renderThread->quit(); + _renderThread->deleteLater(); + } + + if (_rootItem) { + _rootItem->deleteLater(); + _rootItem = nullptr; + } releaseEngine(_qmlContext->engine()); } @@ -128,10 +119,6 @@ void SharedObject::create(OffscreenSurface* surface) { } void SharedObject::setRootItem(QQuickItem* rootItem) { - if (_quit) { - return; - } - _rootItem = rootItem; _rootItem->setSize(_quickWindow->size()); @@ -140,6 +127,7 @@ 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)); @@ -149,43 +137,35 @@ 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(); } - if (_renderControl) { - QObject::disconnect(_renderControl); - } - + QObject::disconnect(_renderControl); QObject::disconnect(qApp); { QMutexLocker lock(&_mutex); _quit = true; - if (_renderObject) { - QCoreApplication::postEvent(_renderObject, new OffscreenEvent(OffscreenEvent::Quit), Qt::HighEventPriority); - } + 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 - if (_renderThread) { - _renderThread->wait(); - delete _renderThread; - _renderThread = nullptr; - } + _renderThread->wait(); } diff --git a/libraries/render-utils/src/AbstractViewStateInterface.h b/libraries/render-utils/src/AbstractViewStateInterface.h index 54fdc903ca..96e9f4d222 100644 --- a/libraries/render-utils/src/AbstractViewStateInterface.h +++ b/libraries/render-utils/src/AbstractViewStateInterface.h @@ -37,25 +37,15 @@ public: virtual glm::vec3 getAvatarPosition() const = 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 bool isAboutToQuit() const = 0; + virtual void postLambdaEvent(std::function f) = 0; virtual qreal getDevicePixelRatio() = 0; virtual render::ScenePointer getMain3DScene() = 0; virtual render::EnginePointer getRenderEngine() = 0; - virtual void pushPostUpdateLambda(void* key, const std::function& func) = 0; + virtual void pushPostUpdateLambda(void* key, std::function func) = 0; virtual bool isHMDMode() const = 0; @@ -64,4 +54,5 @@ 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 9249b3d957..93672cc5a2 100644 --- a/tests/render-perf/src/main.cpp +++ b/tests/render-perf/src/main.cpp @@ -453,8 +453,8 @@ protected: return vec3(); } - void postLambdaEvent(const std::function& f) override {} - void sendLambdaEvent(const std::function& f) override {} + bool isAboutToQuit() const override { return false; } + void postLambdaEvent(std::function f) override {} qreal getDevicePixelRatio() override { return 1.0f; @@ -469,7 +469,7 @@ protected: } std::map> _postUpdateLambdas; - void pushPostUpdateLambda(void* key, const std::function& func) override { + void pushPostUpdateLambda(void* key, std::function func) override { _postUpdateLambdas[key] = func; }