rhi: Make it safe to always call deleteLater on a resource

...even when the QRhi is already gone. This should not happen in
well-written applications and libraries, but we handle this
gracefully in the regular dtor and destroy() for resources that
register themselves to their creator QRhi, so by registering
everything we can offer this to all QRhiResource subclasses.

We still want to differentiate between native resource owning
QRhiResources and others (that do not create native graphics
objects), so do this via a flag passed to registerResource().
This way the behavior with QT_RHI_LEAK_CHECK=1 does not change.

Pick-to: 6.5
Fixes: QTBUG-112914
Change-Id: I9bafc81ef7a4ae76f356fc5f6248628d1f8791e0
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Christian Strømme <christian.stromme@qt.io>
This commit is contained in:
Laszlo Agocs 2023-04-17 15:44:42 +02:00
parent e3c9dce0b6
commit 62a4ca773a
8 changed files with 192 additions and 47 deletions

View File

@ -2119,11 +2119,17 @@ QRhiResource::~QRhiResource()
requirement of not altering QRhiResource objects that are referenced by the
frame being recorded.
If the QRhi that created this object is already destroyed, the object is
deleted immediately.
\sa destroy()
*/
void QRhiResource::deleteLater()
{
m_rhi->addDeleteLater(this);
if (m_rhi)
m_rhi->addDeleteLater(this);
else
delete this;
}
/*!
@ -2172,10 +2178,13 @@ quint64 QRhiResource::globalResourceId() const
/*!
\return the QRhi that created this resource.
If the QRhi that created this object is already destroyed, the result is
\nullptr.
*/
QRhi *QRhiResource::rhi() const
{
return m_rhi->q;
return m_rhi ? m_rhi->q : nullptr;
}
/*!
@ -5100,7 +5109,7 @@ QRhiResource::Type QRhiCommandBuffer::resourceType() const
return CommandBuffer;
}
static const char *resourceTypeStr(QRhiResource *res)
static const char *resourceTypeStr(const QRhiResource *res)
{
switch (res->resourceType()) {
case QRhiResource::Buffer:
@ -5151,8 +5160,10 @@ QRhiImplementation::~QRhiImplementation()
qWarning("QRhi %p going down with %d unreleased resources that own native graphics objects. This is not nice.",
q, int(resources.size()));
}
for (QRhiResource *res : std::as_const(resources)) {
if (leakCheck)
for (auto it = resources.cbegin(), end = resources.cend(); it != end; ++it) {
QRhiResource *res = it.key();
const bool ownsNativeResources = it.value();
if (leakCheck && ownsNativeResources)
qWarning(" %s resource %p (%s)", resourceTypeStr(res), res, res->m_objectName.constData());
// Null out the resource's rhi pointer. This is why it makes sense to do null

View File

@ -149,10 +149,14 @@ public:
void textureFormatInfo(QRhiTexture::Format format, const QSize &size,
quint32 *bpl, quint32 *byteSize, quint32 *bytesPerPixel) const;
// only really care about resources that own native graphics resources underneath
void registerResource(QRhiResource *res)
void registerResource(QRhiResource *res, bool ownsNativeResources = true)
{
resources.insert(res);
// The ownsNativeResources is relevant for the (graphics resource) leak
// check in ~QRhiImplementation; when false, the registration's sole
// purpose is to automatically null out the resource's m_rhi pointer in
// case the rhi goes away first. (which should not happen in
// well-written applications but we try to be graceful)
resources.insert(res, ownsNativeResources);
}
void unregisterResource(QRhiResource *res)
@ -160,11 +164,6 @@ public:
resources.remove(res);
}
QSet<QRhiResource *> activeResources() const
{
return resources;
}
void addDeleteLater(QRhiResource *res)
{
if (inFrame)
@ -251,7 +250,7 @@ private:
QVarLengthArray<QRhiResourceUpdateBatch *, 4> resUpdPool;
quint64 resUpdPoolMap = 0;
int lastResUpdIdx = -1;
QSet<QRhiResource *> resources;
QHash<QRhiResource *, bool> resources;
QSet<QRhiResource *> pendingDeleteResources;
QVarLengthArray<QRhi::CleanupCallback, 4> cleanupCallbacks;
QVarLengthArray<QRhi::GpuFrameTimeCallback, 4> gpuFrameTimeCallbacks;

View File

@ -3588,7 +3588,9 @@ QD3D11RenderPassDescriptor::~QD3D11RenderPassDescriptor()
void QD3D11RenderPassDescriptor::destroy()
{
// nothing to do here
QRHI_RES_RHI(QRhiD3D11);
if (rhiD)
rhiD->unregisterResource(this);
}
bool QD3D11RenderPassDescriptor::isCompatible(const QRhiRenderPassDescriptor *other) const
@ -3599,7 +3601,10 @@ bool QD3D11RenderPassDescriptor::isCompatible(const QRhiRenderPassDescriptor *ot
QRhiRenderPassDescriptor *QD3D11RenderPassDescriptor::newCompatibleRenderPassDescriptor() const
{
return new QD3D11RenderPassDescriptor(m_rhi);
QD3D11RenderPassDescriptor *rpD = new QD3D11RenderPassDescriptor(m_rhi);
QRHI_RES_RHI(QRhiD3D11);
rhiD->registerResource(rpD, false);
return rpD;
}
QVector<quint32> QD3D11RenderPassDescriptor::serializedFormat() const
@ -3681,7 +3686,10 @@ void QD3D11TextureRenderTarget::destroy()
QRhiRenderPassDescriptor *QD3D11TextureRenderTarget::newCompatibleRenderPassDescriptor()
{
return new QD3D11RenderPassDescriptor(m_rhi);
QD3D11RenderPassDescriptor *rpD = new QD3D11RenderPassDescriptor(m_rhi);
QRHI_RES_RHI(QRhiD3D11);
rhiD->registerResource(rpD, false);
return rpD;
}
bool QD3D11TextureRenderTarget::create()
@ -3845,6 +3853,10 @@ void QD3D11ShaderResourceBindings::destroy()
{
sortedBindings.clear();
boundResourceData.clear();
QRHI_RES_RHI(QRhiD3D11);
if (rhiD)
rhiD->unregisterResource(this);
}
bool QD3D11ShaderResourceBindings::create()
@ -3876,6 +3888,7 @@ bool QD3D11ShaderResourceBindings::create()
}
generation += 1;
rhiD->registerResource(this, false);
return true;
}
@ -4796,7 +4809,10 @@ QRhiSwapChainHdrInfo QD3D11SwapChain::hdrInfo()
QRhiRenderPassDescriptor *QD3D11SwapChain::newCompatibleRenderPassDescriptor()
{
return new QD3D11RenderPassDescriptor(m_rhi);
QD3D11RenderPassDescriptor *rpD = new QD3D11RenderPassDescriptor(m_rhi);
QRHI_RES_RHI(QRhiD3D11);
rhiD->registerResource(rpD, false);
return rpD;
}
bool QD3D11SwapChain::newColorBuffer(const QSize &size, DXGI_FORMAT format, DXGI_SAMPLE_DESC sampleDesc,

View File

@ -4168,6 +4168,10 @@ QD3D12Sampler::~QD3D12Sampler()
void QD3D12Sampler::destroy()
{
shaderVisibleDescriptor = {};
QRHI_RES_RHI(QRhiD3D12);
if (rhiD)
rhiD->unregisterResource(this);
}
static inline D3D12_FILTER toD3DFilter(QRhiSampler::Filter minFilter, QRhiSampler::Filter magFilter, QRhiSampler::Filter mipFilter)
@ -4248,6 +4252,9 @@ bool QD3D12Sampler::create()
desc.MaxAnisotropy = 1.0f;
desc.ComparisonFunc = toD3DTextureComparisonFunc(m_compareOp);
desc.MaxLOD = m_mipmapMode == None ? 0.0f : 10000.0f;
QRHI_RES_RHI(QRhiD3D12);
rhiD->registerResource(this, false);
return true;
}
@ -4326,6 +4333,8 @@ QRhiRenderPassDescriptor *QD3D12TextureRenderTarget::newCompatibleRenderPassDesc
rpD->updateSerializedFormat();
QRHI_RES_RHI(QRhiD3D12);
rhiD->registerResource(rpD);
return rpD;
}
@ -4499,6 +4508,10 @@ QD3D12ShaderResourceBindings::~QD3D12ShaderResourceBindings()
void QD3D12ShaderResourceBindings::destroy()
{
sortedBindings.clear();
QRHI_RES_RHI(QRhiD3D12);
if (rhiD)
rhiD->unregisterResource(this);
}
bool QD3D12ShaderResourceBindings::create()
@ -4533,6 +4546,7 @@ bool QD3D12ShaderResourceBindings::create()
// therefore impossible.
generation += 1;
rhiD->registerResource(this, false);
return true;
}
@ -5464,7 +5478,9 @@ QD3D12RenderPassDescriptor::~QD3D12RenderPassDescriptor()
void QD3D12RenderPassDescriptor::destroy()
{
// nothing to do here
QRHI_RES_RHI(QRhiD3D12);
if (rhiD)
rhiD->unregisterResource(this);
}
bool QD3D12RenderPassDescriptor::isCompatible(const QRhiRenderPassDescriptor *other) const
@ -5507,13 +5523,17 @@ void QD3D12RenderPassDescriptor::updateSerializedFormat()
QRhiRenderPassDescriptor *QD3D12RenderPassDescriptor::newCompatibleRenderPassDescriptor() const
{
QD3D12RenderPassDescriptor *rp = new QD3D12RenderPassDescriptor(m_rhi);
rp->colorAttachmentCount = colorAttachmentCount;
rp->hasDepthStencil = hasDepthStencil;
memcpy(rp->colorFormat, colorFormat, sizeof(colorFormat));
rp->dsFormat = dsFormat;
rp->updateSerializedFormat();
return rp;
QD3D12RenderPassDescriptor *rpD = new QD3D12RenderPassDescriptor(m_rhi);
rpD->colorAttachmentCount = colorAttachmentCount;
rpD->hasDepthStencil = hasDepthStencil;
memcpy(rpD->colorFormat, colorFormat, sizeof(colorFormat));
rpD->dsFormat = dsFormat;
rpD->updateSerializedFormat();
QRHI_RES_RHI(QRhiD3D12);
rhiD->registerResource(rpD);
return rpD;
}
QVector<quint32> QD3D12RenderPassDescriptor::serializedFormat() const
@ -5756,6 +5776,9 @@ QRhiRenderPassDescriptor *QD3D12SwapChain::newCompatibleRenderPassDescriptor()
rpD->colorFormat[0] = int(srgbAdjustedColorFormat);
rpD->dsFormat = QD3D12RenderBuffer::DS_FORMAT;
rpD->updateSerializedFormat();
QRHI_RES_RHI(QRhiD3D12);
rhiD->registerResource(rpD);
return rpD;
}

View File

@ -5334,7 +5334,9 @@ QGles2Sampler::~QGles2Sampler()
void QGles2Sampler::destroy()
{
// nothing to do here
QRHI_RES_RHI(QRhiGles2);
if (rhiD)
rhiD->unregisterResource(this);
}
bool QGles2Sampler::create()
@ -5347,6 +5349,8 @@ bool QGles2Sampler::create()
d.gltexcomparefunc = toGlTextureCompareFunc(m_compareOp);
generation += 1;
QRHI_RES_RHI(QRhiGles2);
rhiD->registerResource(this, false);
return true;
}
@ -5363,7 +5367,9 @@ QGles2RenderPassDescriptor::~QGles2RenderPassDescriptor()
void QGles2RenderPassDescriptor::destroy()
{
// nothing to do here
QRHI_RES_RHI(QRhiGles2);
if (rhiD)
rhiD->unregisterResource(this);
}
bool QGles2RenderPassDescriptor::isCompatible(const QRhiRenderPassDescriptor *other) const
@ -5374,7 +5380,10 @@ bool QGles2RenderPassDescriptor::isCompatible(const QRhiRenderPassDescriptor *ot
QRhiRenderPassDescriptor *QGles2RenderPassDescriptor::newCompatibleRenderPassDescriptor() const
{
return new QGles2RenderPassDescriptor(m_rhi);
QGles2RenderPassDescriptor *rpD = new QGles2RenderPassDescriptor(m_rhi);
QRHI_RES_RHI(QRhiGles2);
rhiD->registerResource(rpD, false);
return rpD;
}
QVector<quint32> QGles2RenderPassDescriptor::serializedFormat() const
@ -5447,7 +5456,10 @@ void QGles2TextureRenderTarget::destroy()
QRhiRenderPassDescriptor *QGles2TextureRenderTarget::newCompatibleRenderPassDescriptor()
{
return new QGles2RenderPassDescriptor(m_rhi);
QGles2RenderPassDescriptor *rpD = new QGles2RenderPassDescriptor(m_rhi);
QRHI_RES_RHI(QRhiGles2);
rhiD->registerResource(rpD, false);
return rpD;
}
bool QGles2TextureRenderTarget::create()
@ -5594,7 +5606,9 @@ QGles2ShaderResourceBindings::~QGles2ShaderResourceBindings()
void QGles2ShaderResourceBindings::destroy()
{
// nothing to do here
QRHI_RES_RHI(QRhiGles2);
if (rhiD)
rhiD->unregisterResource(this);
}
bool QGles2ShaderResourceBindings::create()
@ -5617,6 +5631,7 @@ bool QGles2ShaderResourceBindings::create()
rhiD->updateLayoutDesc(this);
generation += 1;
rhiD->registerResource(this, false);
return true;
}
@ -5981,7 +5996,10 @@ bool QGles2SwapChain::isFormatSupported(Format f)
QRhiRenderPassDescriptor *QGles2SwapChain::newCompatibleRenderPassDescriptor()
{
return new QGles2RenderPassDescriptor(m_rhi);
QGles2RenderPassDescriptor *rpD = new QGles2RenderPassDescriptor(m_rhi);
QRHI_RES_RHI(QRhiGles2);
rhiD->registerResource(rpD, false);
return rpD;
}
void QGles2SwapChain::initSwapChainRenderTarget(QGles2SwapChainRenderTarget *rt)
@ -6030,7 +6048,7 @@ bool QGles2SwapChain::createOrResize()
// implement a safe destroy().
if (needsRegistration) {
QRHI_RES_RHI(QRhiGles2);
rhiD->registerResource(this);
rhiD->registerResource(this, false);
}
return true;

View File

@ -3930,7 +3930,9 @@ QMetalRenderPassDescriptor::~QMetalRenderPassDescriptor()
void QMetalRenderPassDescriptor::destroy()
{
// nothing to do here
QRHI_RES_RHI(QRhiMetal);
if (rhiD)
rhiD->unregisterResource(this);
}
bool QMetalRenderPassDescriptor::isCompatible(const QRhiRenderPassDescriptor *other) const
@ -3973,13 +3975,17 @@ void QMetalRenderPassDescriptor::updateSerializedFormat()
QRhiRenderPassDescriptor *QMetalRenderPassDescriptor::newCompatibleRenderPassDescriptor() const
{
QMetalRenderPassDescriptor *rp = new QMetalRenderPassDescriptor(m_rhi);
rp->colorAttachmentCount = colorAttachmentCount;
rp->hasDepthStencil = hasDepthStencil;
memcpy(rp->colorFormat, colorFormat, sizeof(colorFormat));
rp->dsFormat = dsFormat;
rp->updateSerializedFormat();
return rp;
QMetalRenderPassDescriptor *rpD = new QMetalRenderPassDescriptor(m_rhi);
rpD->colorAttachmentCount = colorAttachmentCount;
rpD->hasDepthStencil = hasDepthStencil;
memcpy(rpD->colorFormat, colorFormat, sizeof(colorFormat));
rpD->dsFormat = dsFormat;
rpD->updateSerializedFormat();
QRHI_RES_RHI(QRhiMetal);
rhiD->registerResource(rpD, false);
return rpD;
}
QVector<quint32> QMetalRenderPassDescriptor::serializedFormat() const
@ -4035,7 +4041,9 @@ QMetalTextureRenderTarget::~QMetalTextureRenderTarget()
void QMetalTextureRenderTarget::destroy()
{
// nothing to do here
QRHI_RES_RHI(QRhiMetal);
if (rhiD)
rhiD->unregisterResource(this);
}
QRhiRenderPassDescriptor *QMetalTextureRenderTarget::newCompatibleRenderPassDescriptor()
@ -4058,6 +4066,9 @@ QRhiRenderPassDescriptor *QMetalTextureRenderTarget::newCompatibleRenderPassDesc
rpD->dsFormat = int(QRHI_RES(QMetalRenderBuffer, m_desc.depthStencilBuffer())->d->format);
rpD->updateSerializedFormat();
QRHI_RES_RHI(QRhiMetal);
rhiD->registerResource(rpD, false);
return rpD;
}
@ -4131,6 +4142,7 @@ bool QMetalTextureRenderTarget::create()
QRhiRenderTargetAttachmentTracker::updateResIdList<QMetalTexture, QMetalRenderBuffer>(m_desc, &d->currentResIdList);
rhiD->registerResource(this, false);
return true;
}
@ -4166,6 +4178,10 @@ void QMetalShaderResourceBindings::destroy()
{
sortedBindings.clear();
maxBinding = -1;
QRHI_RES_RHI(QRhiMetal);
if (rhiD)
rhiD->unregisterResource(this);
}
bool QMetalShaderResourceBindings::create()
@ -4192,6 +4208,7 @@ bool QMetalShaderResourceBindings::create()
memset(&bd, 0, sizeof(BoundResourceData));
generation += 1;
rhiD->registerResource(this, false);
return true;
}
@ -6053,6 +6070,8 @@ bool QMetalSwapChain::isFormatSupported(Format f)
QRhiRenderPassDescriptor *QMetalSwapChain::newCompatibleRenderPassDescriptor()
{
QRHI_RES_RHI(QRhiMetal);
chooseFormats(); // ensure colorFormat and similar are filled out
QMetalRenderPassDescriptor *rpD = new QMetalRenderPassDescriptor(m_rhi);
@ -6063,7 +6082,6 @@ QRhiRenderPassDescriptor *QMetalSwapChain::newCompatibleRenderPassDescriptor()
#ifdef Q_OS_MACOS
// m_depthStencil may not be built yet so cannot rely on computed fields in it
QRHI_RES_RHI(QRhiMetal);
rpD->dsFormat = rhiD->d->dev.depth24Stencil8PixelFormatSupported
? MTLPixelFormatDepth24Unorm_Stencil8 : MTLPixelFormatDepth32Float_Stencil8;
#else
@ -6071,6 +6089,8 @@ QRhiRenderPassDescriptor *QMetalSwapChain::newCompatibleRenderPassDescriptor()
#endif
rpD->updateSerializedFormat();
rhiD->registerResource(rpD, false);
return rpD;
}

View File

@ -716,10 +716,15 @@ QNullSampler::~QNullSampler()
void QNullSampler::destroy()
{
QRHI_RES_RHI(QRhiNull);
if (rhiD)
rhiD->unregisterResource(this);
}
bool QNullSampler::create()
{
QRHI_RES_RHI(QRhiNull);
rhiD->registerResource(this);
return true;
}
@ -735,6 +740,9 @@ QNullRenderPassDescriptor::~QNullRenderPassDescriptor()
void QNullRenderPassDescriptor::destroy()
{
QRHI_RES_RHI(QRhiNull);
if (rhiD)
rhiD->unregisterResource(this);
}
bool QNullRenderPassDescriptor::isCompatible(const QRhiRenderPassDescriptor *other) const
@ -745,7 +753,10 @@ bool QNullRenderPassDescriptor::isCompatible(const QRhiRenderPassDescriptor *oth
QRhiRenderPassDescriptor *QNullRenderPassDescriptor::newCompatibleRenderPassDescriptor() const
{
return new QNullRenderPassDescriptor(m_rhi);
QNullRenderPassDescriptor *rpD = new QNullRenderPassDescriptor(m_rhi);
QRHI_RES_RHI(QRhiNull);
rhiD->registerResource(rpD, false);
return rpD;
}
QVector<quint32> QNullRenderPassDescriptor::serializedFormat() const
@ -798,11 +809,17 @@ QNullTextureRenderTarget::~QNullTextureRenderTarget()
void QNullTextureRenderTarget::destroy()
{
QRHI_RES_RHI(QRhiNull);
if (rhiD)
rhiD->unregisterResource(this);
}
QRhiRenderPassDescriptor *QNullTextureRenderTarget::newCompatibleRenderPassDescriptor()
{
return new QNullRenderPassDescriptor(m_rhi);
QNullRenderPassDescriptor *rpD = new QNullRenderPassDescriptor(m_rhi);
QRHI_RES_RHI(QRhiNull);
rhiD->registerResource(rpD, false);
return rpD;
}
bool QNullTextureRenderTarget::create()
@ -820,6 +837,7 @@ bool QNullTextureRenderTarget::create()
d.pixelSize = m_desc.depthTexture()->pixelSize();
}
QRhiRenderTargetAttachmentTracker::updateResIdList<QNullTexture, QNullRenderBuffer>(m_desc, &d.currentResIdList);
rhiD->registerResource(this);
return true;
}
@ -853,6 +871,9 @@ QNullShaderResourceBindings::~QNullShaderResourceBindings()
void QNullShaderResourceBindings::destroy()
{
QRHI_RES_RHI(QRhiNull);
if (rhiD)
rhiD->unregisterResource(this);
}
bool QNullShaderResourceBindings::create()
@ -863,6 +884,7 @@ bool QNullShaderResourceBindings::create()
rhiD->updateLayoutDesc(this);
rhiD->registerResource(this, false);
return true;
}
@ -883,6 +905,9 @@ QNullGraphicsPipeline::~QNullGraphicsPipeline()
void QNullGraphicsPipeline::destroy()
{
QRHI_RES_RHI(QRhiNull);
if (rhiD)
rhiD->unregisterResource(this);
}
bool QNullGraphicsPipeline::create()
@ -891,6 +916,7 @@ bool QNullGraphicsPipeline::create()
if (!rhiD->sanityCheckGraphicsPipeline(this))
return false;
rhiD->registerResource(this);
return true;
}
@ -906,10 +932,15 @@ QNullComputePipeline::~QNullComputePipeline()
void QNullComputePipeline::destroy()
{
QRHI_RES_RHI(QRhiNull);
if (rhiD)
rhiD->unregisterResource(this);
}
bool QNullComputePipeline::create()
{
QRHI_RES_RHI(QRhiNull);
rhiD->registerResource(this);
return true;
}
@ -969,7 +1000,10 @@ bool QNullSwapChain::isFormatSupported(Format f)
QRhiRenderPassDescriptor *QNullSwapChain::newCompatibleRenderPassDescriptor()
{
return new QNullRenderPassDescriptor(m_rhi);
QNullRenderPassDescriptor *rpD = new QNullRenderPassDescriptor(m_rhi);
QRHI_RES_RHI(QRhiNull);
rhiD->registerResource(rpD, false);
return rpD;
}
bool QNullSwapChain::createOrResize()

View File

@ -5450,12 +5450,36 @@ void tst_QRhi::leakedResourceDestroy()
rt->setRenderPassDescriptor(rpDesc.data());
QVERIFY(rt->create());
QRhiRenderBuffer *rb = rhi->newRenderBuffer(QRhiRenderBuffer::DepthStencil, QSize(512, 512));
QVERIFY(rb->create());
QRhiShaderResourceBindings *srb = rhi->newShaderResourceBindings();
QVERIFY(srb->create());
if (impl == QRhi::Vulkan)
qDebug("Vulkan validation layer warnings may be printed below - this is expected");
if (impl == QRhi::D3D12)
qDebug("QD3D12CpuDescriptorPool warnings may be printed below - this is expected");
qDebug("QRhi resource leak check warnings may be printed below - this is expected");
// make the QRhi go away early
rhi.reset();
// let the scoped ptr do its job with the resources
// see if the internal rhi backpointer got nulled out
QVERIFY(buffer->rhi() == nullptr);
QVERIFY(texture->rhi() == nullptr);
QVERIFY(rt->rhi() == nullptr);
QVERIFY(rpDesc->rhi() == nullptr);
QVERIFY(rb->rhi() == nullptr);
QVERIFY(srb->rhi() == nullptr);
// test out deleteLater on some of the resources
rb->deleteLater();
srb->deleteLater();
// let the scoped ptr do its job with the rest
}
void tst_QRhi::renderToFloatTexture_data()