134 lines
5.8 KiB
Diff
134 lines
5.8 KiB
Diff
From 9f2d212416c3c718a5661a09438fb413f9cc53b6 Mon Sep 17 00:00:00 2001
|
|
From: Paul Lemire <paul.lemire@kdab.com>
|
|
Date: Fri, 10 Feb 2023 09:47:50 +0100
|
|
Subject: [PATCH] QText2DEntity: fix QTextureAtlas parenting that could lead to
|
|
crashes
|
|
|
|
We rely on a DistanceFieldFont object to manage QTextureAtlas that hold
|
|
the glyphs. The DistanceFieldFont/QTextureAtlas are supposed to be parented
|
|
by the scene root to ensure that a QTextureAtlas lives as long as possible.
|
|
|
|
DistanceFieldFont/QTextureAtlas are stored in a cache global to the scene
|
|
to minimize the use of resources.
|
|
|
|
When adding text elements, we can reuse atlases since the cache is global to
|
|
the scene and only destroy an atlas (and remove it from the cache) when we
|
|
know no more glyphs are referencing it.
|
|
|
|
However we were mistakenly passing a null parenty to DistanceFieldFont instace
|
|
of the scene root. This resulted on the QTextureAtlas not being parented by
|
|
the scene root but rather by the first DistanceFieldRenderer to use the atlas.
|
|
This meants that if the DistanceFieldRenderer were to be destroyed, so would
|
|
the atlas (yet it would still be referenced by the glyph cache leading to
|
|
crashes).
|
|
|
|
Change-Id: Id84f6a651b162a4bb3c571b11388fd2429b231de
|
|
Reviewed-by: Mike Krus <mike.krus@kdab.com>
|
|
(cherry picked from commit b1a135c547f38db0b2ce6b7bc4c4cccc43ef87d3)
|
|
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>
|
|
---
|
|
src/extras/text/qdistancefieldglyphcache.cpp | 23 ++++++++++++-------
|
|
src/extras/text/qtext2dentity.cpp | 3 ++-
|
|
.../qtext2dentity/tst_qtext2dentity.cpp | 2 +-
|
|
3 files changed, 18 insertions(+), 10 deletions(-)
|
|
|
|
diff --git a/src/extras/text/qdistancefieldglyphcache.cpp b/src/extras/text/qdistancefieldglyphcache.cpp
|
|
index a65e7fc63a..85591e6807 100644
|
|
--- a/src/extras/text/qdistancefieldglyphcache.cpp
|
|
+++ b/src/extras/text/qdistancefieldglyphcache.cpp
|
|
@@ -161,6 +161,7 @@ DistanceFieldFont::DistanceFieldFont(const QRawFont &font, bool doubleRes, Qt3DC
|
|
, m_doubleGlyphResolution(doubleRes)
|
|
, m_parentNode(parent)
|
|
{
|
|
+ Q_ASSERT(m_parentNode);
|
|
}
|
|
|
|
DistanceFieldFont::~DistanceFieldFont()
|
|
@@ -197,13 +198,14 @@ StoredGlyph DistanceFieldFont::refGlyph(quint32 glyph)
|
|
// scenarios
|
|
const int size = m_doubleGlyphResolution ? 512 : 256;
|
|
|
|
- QTextureAtlas *atlas = new QTextureAtlas(m_parentNode);
|
|
+ QTextureAtlas *atlas = new QTextureAtlas();
|
|
atlas->setWidth(size);
|
|
atlas->setHeight(size);
|
|
atlas->setFormat(Qt3DRender::QAbstractTexture::R8_UNorm);
|
|
atlas->setPixelFormat(QOpenGLTexture::Red);
|
|
atlas->setMinificationFilter(Qt3DRender::QAbstractTexture::Linear);
|
|
atlas->setMagnificationFilter(Qt3DRender::QAbstractTexture::Linear);
|
|
+ atlas->setParent(m_parentNode);
|
|
m_atlasses << atlas;
|
|
|
|
if (!storedGlyph.addToTextureAtlas(atlas))
|
|
@@ -236,7 +238,12 @@ void DistanceFieldFont::derefGlyph(quint32 glyph)
|
|
Q_ASSERT(m_atlasses.contains(atlas));
|
|
|
|
m_atlasses.removeAll(atlas);
|
|
- delete atlas;
|
|
+
|
|
+ // This function might have been called as a result of destroying
|
|
+ // the scene root which traverses the entire scene tree. Calling
|
|
+ // delete on the atlas here could lead to dangling pointers in the
|
|
+ // least of children being traversed for destruction.
|
|
+ atlas->deleteLater();
|
|
}
|
|
|
|
m_glyphs.erase(it);
|
|
@@ -287,7 +294,8 @@ DistanceFieldFont* QDistanceFieldGlyphCache::getOrCreateDistanceFieldFont(const
|
|
// create new font cache
|
|
// we set the parent node to nullptr, since the parent node of QTextureAtlasses
|
|
// will be set when we pass them to QText2DMaterial later
|
|
- DistanceFieldFont *dff = new DistanceFieldFont(actualFont, useDoubleRes, nullptr);
|
|
+ Q_ASSERT(m_rootNode);
|
|
+ DistanceFieldFont *dff = new DistanceFieldFont(actualFont, useDoubleRes, m_rootNode);
|
|
m_fonts.insert(key, dff);
|
|
return dff;
|
|
}
|
|
@@ -324,11 +332,10 @@ QDistanceFieldGlyphCache::Glyph refAndGetGlyph(DistanceFieldFont *dff, quint32 g
|
|
if (dff) {
|
|
const auto entry = dff->refGlyph(glyph);
|
|
|
|
- if (entry.atlas()) {
|
|
- ret.glyphPathBoundingRect = entry.glyphPathBoundingRect();
|
|
- ret.texCoords = entry.texCoords();
|
|
- ret.texture = entry.atlas();
|
|
- }
|
|
+ Q_ASSERT(entry.atlas());
|
|
+ ret.glyphPathBoundingRect = entry.glyphPathBoundingRect();
|
|
+ ret.texCoords = entry.texCoords();
|
|
+ ret.texture = entry.atlas();
|
|
}
|
|
|
|
return ret;
|
|
diff --git a/src/extras/text/qtext2dentity.cpp b/src/extras/text/qtext2dentity.cpp
|
|
index 59e8284e10..e3d3dad2e2 100644
|
|
--- a/src/extras/text/qtext2dentity.cpp
|
|
+++ b/src/extras/text/qtext2dentity.cpp
|
|
@@ -304,8 +304,9 @@ void QText2DEntityPrivate::setCurrentGlyphRuns(const QVector<QGlyphRun> &runs)
|
|
delete m_renderers.takeLast();
|
|
|
|
while (m_renderers.size() < renderData.size()) {
|
|
- DistanceFieldTextRenderer *renderer = new DistanceFieldTextRenderer(q_func());
|
|
+ DistanceFieldTextRenderer *renderer = new DistanceFieldTextRenderer();
|
|
renderer->setColor(m_color);
|
|
+ renderer->setParent(q_func());
|
|
m_renderers << renderer;
|
|
}
|
|
|
|
diff --git a/tests/auto/extras/qtext2dentity/tst_qtext2dentity.cpp b/tests/auto/extras/qtext2dentity/tst_qtext2dentity.cpp
|
|
index 6fcc2e6370..35e241839b 100644
|
|
--- a/tests/auto/extras/qtext2dentity/tst_qtext2dentity.cpp
|
|
+++ b/tests/auto/extras/qtext2dentity/tst_qtext2dentity.cpp
|
|
@@ -90,7 +90,7 @@ void tst_qtext2dentity::checkChangeArbiter()
|
|
auto atlases = lookupNodeByClassName(rootEntity.data(), "Qt3DExtras::QTextureAtlas");
|
|
QVERIFY(atlases.length() == 1);
|
|
auto atlas = atlases[0];
|
|
- QTRY_VERIFY(Qt3DCore::QNodePrivate::get(atlas)->m_changeArbiter);
|
|
+ QVERIFY(Qt3DCore::QNodePrivate::get(atlas)->m_changeArbiter);
|
|
#endif
|
|
}
|
|
|
|
--
|
|
GitLab
|
|
|