From 85a126c288c608132e6254a51f43a0146d380fef Mon Sep 17 00:00:00 2001 From: Laszlo Agocs Date: Mon, 11 May 2020 20:42:59 +0200 Subject: rhi: Fix crashing Controls autotests due to shadereffect lifetime issues Make the QSGShaderEffectNode base class a QObject, the subclass (QSGRhiShaderEffectNode) is already a QObject so we lose nothing. This way we can get rid of the textureChanged() signal from the shadereffectmanager (the object living on the gui thread), and so the node (living on the render thread) does not need to do emit m_mgr->textureChanged() on the render thread. The signal emission in itself was perfectly fine, except when m_mgr happened to be destroyed already in case the node outlived the gui thread's item (and m_mgr). After the change we have something that is closer to what the direct OpenGL implementation did - and some of the interfaces in QSGContext get a lot cleaner in fact because the node can be instantiated on its own, without the involving the shadereffectmanager object that does not even live on the render thread. Change-Id: Ibb918cc3eaa08faddb8b8e5dfa3e34b212161703 Reviewed-by: Eirik Aavitsland --- src/quick/items/qquickgenericshadereffect.cpp | 5 +++-- src/quick/scenegraph/qsgadaptationlayer_p.h | 9 ++++++--- src/quick/scenegraph/qsgcontext.cpp | 2 +- src/quick/scenegraph/qsgcontext_p.h | 3 +-- src/quick/scenegraph/qsgdefaultcontext.cpp | 9 +++------ src/quick/scenegraph/qsgdefaultcontext_p.h | 3 +-- src/quick/scenegraph/qsgrhishadereffectnode.cpp | 8 +++----- src/quick/scenegraph/qsgrhishadereffectnode_p.h | 6 ++---- 8 files changed, 20 insertions(+), 25 deletions(-) diff --git a/src/quick/items/qquickgenericshadereffect.cpp b/src/quick/items/qquickgenericshadereffect.cpp index a3d04f5dd3..b7f52a8146 100644 --- a/src/quick/items/qquickgenericshadereffect.cpp +++ b/src/quick/items/qquickgenericshadereffect.cpp @@ -269,12 +269,14 @@ QSGNode *QQuickGenericShaderEffect::handleUpdatePaintNode(QSGNode *oldNode, QQui if (!node) { QSGRenderContext *rc = QQuickWindowPrivate::get(m_item->window())->context; - node = rc->sceneGraphContext()->createShaderEffectNode(rc, mgr); + node = rc->sceneGraphContext()->createShaderEffectNode(rc); if (!node) { qWarning("No shader effect node"); return nullptr; } m_dirty = QSGShaderEffectNode::DirtyShaderAll; + connect(node, &QSGShaderEffectNode::textureChanged, + this, &QQuickGenericShaderEffect::markGeometryDirtyAndUpdateIfSupportsAtlas); } QSGShaderEffectNode::SyncData sd; @@ -369,7 +371,6 @@ QSGGuiThreadShaderEffectManager *QQuickGenericShaderEffect::shaderEffectManager( if (m_mgr) { connect(m_mgr, SIGNAL(logAndStatusChanged()), m_item, SIGNAL(logChanged())); connect(m_mgr, SIGNAL(logAndStatusChanged()), m_item, SIGNAL(statusChanged())); - connect(m_mgr, SIGNAL(textureChanged()), this, SLOT(markGeometryDirtyAndUpdateIfSupportsAtlas())); connect(m_mgr, &QSGGuiThreadShaderEffectManager::shaderCodePrepared, this, &QQuickGenericShaderEffect::shaderCodePrepared); } } diff --git a/src/quick/scenegraph/qsgadaptationlayer_p.h b/src/quick/scenegraph/qsgadaptationlayer_p.h index 1db49feb5d..6376317427 100644 --- a/src/quick/scenegraph/qsgadaptationlayer_p.h +++ b/src/quick/scenegraph/qsgadaptationlayer_p.h @@ -299,7 +299,6 @@ public: Q_SIGNALS: void shaderCodePrepared(bool ok, ShaderInfo::Type typeHint, const QByteArray &src, ShaderInfo *result); - void textureChanged(); void logAndStatusChanged(); }; @@ -307,8 +306,10 @@ Q_SIGNALS: Q_QUICK_PRIVATE_EXPORT QDebug operator<<(QDebug debug, const QSGGuiThreadShaderEffectManager::ShaderInfo::Variable &v); #endif -class Q_QUICK_PRIVATE_EXPORT QSGShaderEffectNode : public QSGVisitableNode +class Q_QUICK_PRIVATE_EXPORT QSGShaderEffectNode : public QObject, public QSGVisitableNode { + Q_OBJECT + public: enum DirtyShaderFlag { DirtyShaders = 0x01, @@ -355,12 +356,14 @@ public: }; // Each ShaderEffect item has one node (render thread) and one manager (gui thread). - QSGShaderEffectNode(QSGGuiThreadShaderEffectManager *) { } virtual QRectF updateNormalizedTextureSubRect(bool supportsAtlasTextures) = 0; virtual void syncMaterial(SyncData *syncData) = 0; void accept(QSGNodeVisitorEx *visitor) override { if (visitor->visit(this)) visitor->visitChildren(this); visitor->endVisit(this); } + +Q_SIGNALS: + void textureChanged(); }; Q_DECLARE_OPERATORS_FOR_FLAGS(QSGShaderEffectNode::DirtyShaderFlags) diff --git a/src/quick/scenegraph/qsgcontext.cpp b/src/quick/scenegraph/qsgcontext.cpp index e3c951e5ed..9baf530889 100644 --- a/src/quick/scenegraph/qsgcontext.cpp +++ b/src/quick/scenegraph/qsgcontext.cpp @@ -284,7 +284,7 @@ QSGGuiThreadShaderEffectManager *QSGContext::createGuiThreadShaderEffectManager( valid as long as the backend does not claim SupportsShaderEffectNode or ignoring ShaderEffect elements is acceptable. */ -QSGShaderEffectNode *QSGContext::createShaderEffectNode(QSGRenderContext *, QSGGuiThreadShaderEffectManager *) +QSGShaderEffectNode *QSGContext::createShaderEffectNode(QSGRenderContext *) { return nullptr; } diff --git a/src/quick/scenegraph/qsgcontext_p.h b/src/quick/scenegraph/qsgcontext_p.h index d389420907..a2cb21a60c 100644 --- a/src/quick/scenegraph/qsgcontext_p.h +++ b/src/quick/scenegraph/qsgcontext_p.h @@ -128,8 +128,7 @@ public: virtual QSGGlyphNode *createGlyphNode(QSGRenderContext *rc, bool preferNativeGlyphNode) = 0; virtual QSGLayer *createLayer(QSGRenderContext *renderContext) = 0; virtual QSGGuiThreadShaderEffectManager *createGuiThreadShaderEffectManager(); - virtual QSGShaderEffectNode *createShaderEffectNode(QSGRenderContext *renderContext, - QSGGuiThreadShaderEffectManager *mgr); + virtual QSGShaderEffectNode *createShaderEffectNode(QSGRenderContext *renderContext); #if QT_CONFIG(quick_sprite) virtual QSGSpriteNode *createSpriteNode() = 0; #endif diff --git a/src/quick/scenegraph/qsgdefaultcontext.cpp b/src/quick/scenegraph/qsgdefaultcontext.cpp index 1d70c826de..f7701d8f7f 100644 --- a/src/quick/scenegraph/qsgdefaultcontext.cpp +++ b/src/quick/scenegraph/qsgdefaultcontext.cpp @@ -297,13 +297,10 @@ QSGGuiThreadShaderEffectManager *QSGDefaultContext::createGuiThreadShaderEffectM return nullptr; } -QSGShaderEffectNode *QSGDefaultContext::createShaderEffectNode(QSGRenderContext *renderContext, - QSGGuiThreadShaderEffectManager *mgr) +QSGShaderEffectNode *QSGDefaultContext::createShaderEffectNode(QSGRenderContext *renderContext) { - if (QSGRhiSupport::instance()->isRhiEnabled()) { - return new QSGRhiShaderEffectNode(static_cast(renderContext), - static_cast(mgr)); - } + if (QSGRhiSupport::instance()->isRhiEnabled()) + return new QSGRhiShaderEffectNode(static_cast(renderContext)); return nullptr; } diff --git a/src/quick/scenegraph/qsgdefaultcontext_p.h b/src/quick/scenegraph/qsgdefaultcontext_p.h index 8fdd29caee..414a4151f1 100644 --- a/src/quick/scenegraph/qsgdefaultcontext_p.h +++ b/src/quick/scenegraph/qsgdefaultcontext_p.h @@ -80,8 +80,7 @@ public: QSGSpriteNode *createSpriteNode() override; #endif QSGGuiThreadShaderEffectManager *createGuiThreadShaderEffectManager() override; - QSGShaderEffectNode *createShaderEffectNode(QSGRenderContext *renderContext, - QSGGuiThreadShaderEffectManager *mgr) override; + QSGShaderEffectNode *createShaderEffectNode(QSGRenderContext *renderContext) override; void setDistanceFieldEnabled(bool enabled); bool isDistanceFieldEnabled() const; diff --git a/src/quick/scenegraph/qsgrhishadereffectnode.cpp b/src/quick/scenegraph/qsgrhishadereffectnode.cpp index 9ec9e040fb..5d3b158de8 100644 --- a/src/quick/scenegraph/qsgrhishadereffectnode.cpp +++ b/src/quick/scenegraph/qsgrhishadereffectnode.cpp @@ -568,10 +568,8 @@ void QSGRhiShaderEffectMaterial::updateTextureProviders(bool layoutChange) } } -QSGRhiShaderEffectNode::QSGRhiShaderEffectNode(QSGDefaultRenderContext *rc, QSGRhiGuiThreadShaderEffectManager *mgr) - : QSGShaderEffectNode(mgr), - m_rc(rc), - m_mgr(mgr), +QSGRhiShaderEffectNode::QSGRhiShaderEffectNode(QSGDefaultRenderContext *rc) + : m_rc(rc), m_material(this) { setFlag(UsePreprocess, true); @@ -749,7 +747,7 @@ void QSGRhiShaderEffectNode::syncMaterial(SyncData *syncData) void QSGRhiShaderEffectNode::handleTextureChange() { markDirty(QSGNode::DirtyMaterial); - emit m_mgr->textureChanged(); + emit textureChanged(); } void QSGRhiShaderEffectNode::handleTextureProviderDestroyed(QObject *object) diff --git a/src/quick/scenegraph/qsgrhishadereffectnode_p.h b/src/quick/scenegraph/qsgrhishadereffectnode_p.h index 26460d24b2..fb98bbf10e 100644 --- a/src/quick/scenegraph/qsgrhishadereffectnode_p.h +++ b/src/quick/scenegraph/qsgrhishadereffectnode_p.h @@ -59,7 +59,6 @@ QT_BEGIN_NAMESPACE class QSGDefaultRenderContext; class QSGPlainTexture; class QSGRhiShaderEffectNode; -class QSGRhiGuiThreadShaderEffectManager; class QFileSelector; class QSGRhiShaderLinker @@ -121,12 +120,12 @@ public: QSGPlainTexture *m_dummyTexture = nullptr; }; -class QSGRhiShaderEffectNode : public QObject, public QSGShaderEffectNode +class QSGRhiShaderEffectNode : public QSGShaderEffectNode { Q_OBJECT public: - QSGRhiShaderEffectNode(QSGDefaultRenderContext *rc, QSGRhiGuiThreadShaderEffectManager *mgr); + QSGRhiShaderEffectNode(QSGDefaultRenderContext *rc); QRectF updateNormalizedTextureSubRect(bool supportsAtlasTextures) override; void syncMaterial(SyncData *syncData) override; @@ -140,7 +139,6 @@ private Q_SLOTS: private: QSGDefaultRenderContext *m_rc; - QSGRhiGuiThreadShaderEffectManager *m_mgr; QSGRhiShaderEffectMaterial m_material; }; -- cgit v1.2.3