From c9d991de1ff32a7993aae90b50badfaa39323a61 Mon Sep 17 00:00:00 2001 From: Eskil Abrahamsen Blomfeldt Date: Mon, 28 Nov 2022 14:24:45 +0100 Subject: [PATCH] Fix missing text when Harfbuzz fails to shape a substring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This amends fccd419dd632306a4bd85928223e0a56a59510ef. If Harfbuzz failed on one of the items in a string and returned zero glyphs, then we would exit the shaping loop. The mentioned change fixed a crash related to this when the ignored character was the only character in the string, but it occurred in a subitem of a longer string, then we would return and fail to lay out the rest of the string. This popped up recently because an update to Harfbuzz has caused it to return zero glyphs when applying the Apple emoji font to an isolated variant character (see bug report). When we matched the symbol to the main font and only the variant character to the emoji font, we would get in this situation, and end up exiting the shaping early. [ChangeLog][QtGui][Text] Fixed a regression which would sometimes cause text to disappear if the string contained an unmatched variation selector character. Pick-to: 6.4 Fixes: QTBUG-108799 Change-Id: I616ab1b2d33c2df731419c5ce06fbc578a625a32 Reviewed-by: Tor Arne Vestbø --- src/gui/text/qtextengine.cpp | 65 +++++++++++-------- .../auto/gui/text/qglyphrun/tst_qglyphrun.cpp | 40 +++++++++--- 2 files changed, 71 insertions(+), 34 deletions(-) diff --git a/src/gui/text/qtextengine.cpp b/src/gui/text/qtextengine.cpp index 4300b8a9bc7..2dfb7f0073e 100644 --- a/src/gui/text/qtextengine.cpp +++ b/src/gui/text/qtextengine.cpp @@ -1528,7 +1528,6 @@ void QTextEngine::shapeText(int item) const QGlyphLayout g = availableGlyphs(&si); g.glyphs[0] = 0; g.attributes[0].clusterStart = true; - g.attributes[0].dontPrint = true; ushort *log_clusters = logClusters(&si); for (int i = 0; i < itemLength; ++i) @@ -1675,9 +1674,14 @@ int QTextEngine::shapeTextWithHarfbuzzNG(const QScriptItem &si, hb_buffer_reverse(buffer); } - const uint num_glyphs = hb_buffer_get_length(buffer); + uint num_glyphs = hb_buffer_get_length(buffer); + const bool has_glyphs = num_glyphs > 0; + // If Harfbuzz returns zero glyphs, we have to manually add a missing glyph + if (Q_UNLIKELY(!has_glyphs)) + num_glyphs = 1; + // ensure we have enough space for shaped glyphs and metrics - if (Q_UNLIKELY(num_glyphs == 0 || !ensureSpace(glyphs_shaped + num_glyphs))) { + if (Q_UNLIKELY(!ensureSpace(glyphs_shaped + num_glyphs))) { hb_buffer_destroy(buffer); return 0; } @@ -1685,35 +1689,44 @@ int QTextEngine::shapeTextWithHarfbuzzNG(const QScriptItem &si, // fetch the shaped glyphs and metrics QGlyphLayout g = availableGlyphs(&si).mid(glyphs_shaped, num_glyphs); ushort *log_clusters = logClusters(&si) + item_pos; + if (Q_LIKELY(has_glyphs)) { + hb_glyph_info_t *infos = hb_buffer_get_glyph_infos(buffer, nullptr); + hb_glyph_position_t *positions = hb_buffer_get_glyph_positions(buffer, nullptr); + uint str_pos = 0; + uint last_cluster = ~0u; + uint last_glyph_pos = glyphs_shaped; + for (uint i = 0; i < num_glyphs; ++i, ++infos, ++positions) { + g.glyphs[i] = infos->codepoint; - hb_glyph_info_t *infos = hb_buffer_get_glyph_infos(buffer, nullptr); - hb_glyph_position_t *positions = hb_buffer_get_glyph_positions(buffer, nullptr); - uint str_pos = 0; - uint last_cluster = ~0u; - uint last_glyph_pos = glyphs_shaped; - for (uint i = 0; i < num_glyphs; ++i, ++infos, ++positions) { - g.glyphs[i] = infos->codepoint; + g.advances[i] = QFixed::fromFixed(positions->x_advance); + g.offsets[i].x = QFixed::fromFixed(positions->x_offset); + g.offsets[i].y = QFixed::fromFixed(positions->y_offset); - g.advances[i] = QFixed::fromFixed(positions->x_advance); - g.offsets[i].x = QFixed::fromFixed(positions->x_offset); - g.offsets[i].y = QFixed::fromFixed(positions->y_offset); + uint cluster = infos->cluster; + if (Q_LIKELY(last_cluster != cluster)) { + g.attributes[i].clusterStart = true; - uint cluster = infos->cluster; - if (Q_LIKELY(last_cluster != cluster)) { - g.attributes[i].clusterStart = true; + // fix up clusters so that the cluster indices will be monotonic + // and thus we never return out-of-order indices + while (last_cluster++ < cluster && str_pos < item_length) + log_clusters[str_pos++] = last_glyph_pos; + last_glyph_pos = i + glyphs_shaped; + last_cluster = cluster; - // fix up clusters so that the cluster indices will be monotonic - // and thus we never return out-of-order indices - while (last_cluster++ < cluster && str_pos < item_length) - log_clusters[str_pos++] = last_glyph_pos; - last_glyph_pos = i + glyphs_shaped; - last_cluster = cluster; - - applyVisibilityRules(string[item_pos + str_pos], &g, i, actualFontEngine); + applyVisibilityRules(string[item_pos + str_pos], &g, i, actualFontEngine); + } } + while (str_pos < item_length) + log_clusters[str_pos++] = last_glyph_pos; + } else { // Harfbuzz did not return a glyph for the character, so we add a placeholder + g.glyphs[0] = 0; + g.advances[0] = QFixed{}; + g.offsets[0].x = QFixed{}; + g.offsets[0].y = QFixed{}; + g.attributes[0].clusterStart = true; + g.attributes[0].dontPrint = true; + log_clusters[0] = glyphs_shaped; } - while (str_pos < item_length) - log_clusters[str_pos++] = last_glyph_pos; if (Q_UNLIKELY(engineIdx != 0)) { for (quint32 i = 0; i < num_glyphs; ++i) diff --git a/tests/auto/gui/text/qglyphrun/tst_qglyphrun.cpp b/tests/auto/gui/text/qglyphrun/tst_qglyphrun.cpp index 8275b1f427b..b0d6efc065e 100644 --- a/tests/auto/gui/text/qglyphrun/tst_qglyphrun.cpp +++ b/tests/auto/gui/text/qglyphrun/tst_qglyphrun.cpp @@ -608,15 +608,39 @@ void tst_QGlyphRun::multiLineBoundingRect() void tst_QGlyphRun::defaultIgnorables() { - QTextLayout layout; - layout.setFont(QFont("QtsSpecialTestFont")); - layout.setText(QChar(0x200D)); - layout.beginLayout(); - layout.createLine(); - layout.endLayout(); + { + QTextLayout layout; + layout.setFont(QFont("QtsSpecialTestFont")); + layout.setText(QChar(0x200D)); + layout.beginLayout(); + layout.createLine(); + layout.endLayout(); - QList runs = layout.glyphRuns(); - QCOMPARE(runs.size(), 0); + QList runs = layout.glyphRuns(); + QCOMPARE(runs.size(), 0); + } + + { + QTextLayout layout; + layout.setFont(QFont("QtsSpecialTestFont")); + layout.setText(QStringLiteral("AAA") + QChar(0xFE0F) + QStringLiteral("111")); + layout.beginLayout(); + layout.createLine(); + layout.endLayout(); + + QList runs = layout.glyphRuns(); + QVERIFY(!runs.isEmpty()); + + bool hasFullMainFontRun = false; + for (const QGlyphRun &run : runs) { + if (run.rawFont().familyName() == QStringLiteral("QtsSpecialTestFont") + && run.glyphIndexes().size() == 6) { + hasFullMainFontRun = true; + break; + } + } + QVERIFY(hasFullMainFontRun); + } } void tst_QGlyphRun::stringIndexes()