Timo Jyrinki has proposed merging lp:~timo-jyrinki/kubuntu-packaging/qtbase_sync_from_archives_and_add_harfbuzz_patches into lp:~kubuntu-packagers/kubuntu-packaging/qtbase-opensource-src.
Requested reviews: Kubuntu Packagers (kubuntu-packagers) Related bugs: Bug #1285184 in Ubuntu UI Toolkit: "Squares in text where carriage returns should be" https://bugs.launchpad.net/ubuntu-ui-toolkit/+bug/1285184 For more details, see: https://code.launchpad.net/~timo-jyrinki/kubuntu-packaging/qtbase_sync_from_archives_and_add_harfbuzz_patches/+merge/211501 Release 5.2.1+dfsg-1ubuntu9 for landing * debian/patches/Fix-access-after-delete-with-Harfbuzz-NG-code-path.patch * debian/patches/Fix-log_clusters-calculation-in-HarfBuzz-NG-code-pat.patch * debian/patches/HarfBuzz-NG-Hide-characters-that-should-normally-be-.patch * debian/patches/Minor-optimization-for-QTextEngine-shapeText.patch - Cherry-pick latest HarfBuzz-NG changes from upstream including the one fixing the glyph issue https://codereview.qt-project.org/#change,81150 (LP: #1285184) -- https://code.launchpad.net/~timo-jyrinki/kubuntu-packaging/qtbase_sync_from_archives_and_add_harfbuzz_patches/+merge/211501 Your team Kubuntu Packagers is requested to review the proposed merge of lp:~timo-jyrinki/kubuntu-packaging/qtbase_sync_from_archives_and_add_harfbuzz_patches into lp:~kubuntu-packagers/kubuntu-packaging/qtbase-opensource-src.
=== modified file 'debian/changelog' --- debian/changelog 2014-03-18 05:45:20 +0000 +++ debian/changelog 2014-03-18 11:51:23 +0000 @@ -1,3 +1,15 @@ +qtbase-opensource-src (5.2.1+dfsg-1ubuntu9) trusty; urgency=medium + + * debian/patches/Fix-access-after-delete-with-Harfbuzz-NG-code-path.patch + * debian/patches/Fix-log_clusters-calculation-in-HarfBuzz-NG-code-pat.patch + * debian/patches/HarfBuzz-NG-Hide-characters-that-should-normally-be-.patch + * debian/patches/Minor-optimization-for-QTextEngine-shapeText.patch + - Cherry-pick latest HarfBuzz-NG changes from upstream including the one + fixing the glyph issue https://codereview.qt-project.org/#change,81150 + (LP: #1285184) + + -- Timo Jyrinki <[email protected]> Tue, 18 Mar 2014 06:00:49 +0000 + qtbase-opensource-src (5.2.1+dfsg-1ubuntu8) trusty; urgency=medium * debian/patches/Add-workaround-for-GL-on-Android-emulator.patch: === added file 'debian/patches/Fix-access-after-delete-with-Harfbuzz-NG-code-path.patch' --- debian/patches/Fix-access-after-delete-with-Harfbuzz-NG-code-path.patch 1970-01-01 00:00:00 +0000 +++ debian/patches/Fix-access-after-delete-with-Harfbuzz-NG-code-path.patch 2014-03-18 11:51:23 +0000 @@ -0,0 +1,33 @@ +From de6d260ace56f7245ebd1e2e4454ff1c84f5f89a Mon Sep 17 00:00:00 2001 +From: Allan Sandfeld Jensen <[email protected]> +Date: Wed, 29 Jan 2014 16:47:08 +0100 +Subject: [PATCH] Fix access after delete with Harfbuzz NG code path. + +Remove reference to released font-engine so we don't risk returning it +later. + +Change-Id: I741a741567a079818c7f414ac1f9c0b5a9677322 +Task-number: QTBUG-36522 +--- + src/gui/text/qtextengine.cpp | 4 +++- + 1 file changed, 3 insertions(+), 1 deletion(-) + +diff --git a/src/gui/text/qtextengine.cpp b/src/gui/text/qtextengine.cpp +index 06c5e24..c2e352e 100644 +--- a/src/gui/text/qtextengine.cpp ++++ b/src/gui/text/qtextengine.cpp +@@ -1875,8 +1875,10 @@ QFontEngine *QTextEngine::fontEngine(const QScriptItem &si, QFixed *ascent, QFix + feCache.prevFontEngine = engine; + feCache.prevScript = script; + engine->ref.ref(); +- if (feCache.prevScaledFontEngine) ++ if (feCache.prevScaledFontEngine) { + releaseCachedFontEngine(feCache.prevScaledFontEngine); ++ feCache.prevScaledFontEngine = 0; ++ } + } + if (si.analysis.flags & QFont::SmallCaps) { + if (feCache.prevScaledFontEngine) { +-- +1.9.0 + === added file 'debian/patches/Fix-log_clusters-calculation-in-HarfBuzz-NG-code-pat.patch' --- debian/patches/Fix-log_clusters-calculation-in-HarfBuzz-NG-code-pat.patch 1970-01-01 00:00:00 +0000 +++ debian/patches/Fix-log_clusters-calculation-in-HarfBuzz-NG-code-pat.patch 2014-03-18 11:51:23 +0000 @@ -0,0 +1,112 @@ +From ba1113881910e3e7a6a79dd723a822638b3818c1 Mon Sep 17 00:00:00 2001 +From: Konstantin Ritt <[email protected]> +Date: Sun, 9 Feb 2014 00:46:40 +0200 +Subject: [PATCH] Fix log_clusters calculation in HarfBuzz-NG code path + +The old code wasn't good enough to catch all the glyph (de)composition cases, +thus leading to an assertion in QTextLayout's addNextCluster() helper. + +The new code catches all the corner cases and introduces somewhat +better performance to the HB-NG shaper backend. + +Change-Id: I5b6c673395a4a039dc55b200abbf74b0ba5d0829 +--- + src/gui/text/qtextengine.cpp | 53 +++++++++++++------------------------------- + 1 file changed, 16 insertions(+), 37 deletions(-) + +diff --git a/src/gui/text/qtextengine.cpp b/src/gui/text/qtextengine.cpp +index c2e352e..607fe95 100644 +--- a/src/gui/text/qtextengine.cpp ++++ b/src/gui/text/qtextengine.cpp +@@ -1105,18 +1105,6 @@ int QTextEngine::shapeTextWithHarfbuzzNG(const QScriptItem &si, const ushort *st + buffer_flags |= HB_BUFFER_FLAG_PRESERVE_DEFAULT_IGNORABLES; + hb_buffer_set_flags(buffer, hb_buffer_flags_t(buffer_flags)); + +- const uint num_codes = hb_buffer_get_length(buffer); +- { +- // adjust clusters +- hb_glyph_info_t *infos = hb_buffer_get_glyph_infos(buffer, 0); +- const ushort *uc = string + item_pos; +- for (uint i = 0, code_pos = 0; i < item_length; ++i, ++code_pos) { +- if (QChar::isHighSurrogate(uc[i]) && i + 1 < item_length && QChar::isLowSurrogate(uc[i + 1])) +- ++i; +- infos[code_pos].cluster = code_pos + item_glyph_pos; +- } +- } +- + + // shape + bool shapedOk = false; +@@ -1139,8 +1127,7 @@ int QTextEngine::shapeTextWithHarfbuzzNG(const QScriptItem &si, const ushort *st + if (si.analysis.bidiLevel % 2) + hb_buffer_reverse(buffer); + +- +- remaining_glyphs -= num_codes; ++ remaining_glyphs -= item_glyph_pos; + + // ensure we have enough space for shaped glyphs and metrics + const uint num_glyphs = hb_buffer_get_length(buffer); +@@ -1151,44 +1138,36 @@ int QTextEngine::shapeTextWithHarfbuzzNG(const QScriptItem &si, const ushort *st + + // fetch the shaped glyphs and metrics + QGlyphLayout g = availableGlyphs(&si).mid(glyphs_shaped, num_glyphs); +- if (num_glyphs > num_codes) +- moveGlyphData(g.mid(num_glyphs), g.mid(num_codes), remaining_glyphs); ++ if (num_glyphs != item_glyph_pos) ++ moveGlyphData(g.mid(num_glyphs), g.mid(item_glyph_pos), remaining_glyphs); + ushort *log_clusters = logClusters(&si) + item_pos; + + hb_glyph_info_t *infos = hb_buffer_get_glyph_infos(buffer, 0); + hb_glyph_position_t *positions = hb_buffer_get_glyph_positions(buffer, 0); +- uint last_cluster = -1; ++ uint str_pos = 0; ++ uint last_cluster = ~0u; ++ uint last_glyph_pos = glyphs_shaped; + for (uint i = 0; i < num_glyphs; ++i) { + g.glyphs[i] = infos[i].codepoint; +- log_clusters[i] = infos[i].cluster; + + g.advances_x[i] = QFixed::fromFixed(positions[i].x_advance); + g.advances_y[i] = QFixed::fromFixed(positions[i].y_advance); + g.offsets[i].x = QFixed::fromFixed(positions[i].x_offset); + g.offsets[i].y = QFixed::fromFixed(positions[i].y_offset); + +- if (infos[i].cluster != last_cluster) { +- last_cluster = infos[i].cluster; ++ uint cluster = infos[i].cluster; ++ if (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; + g.attributes[i].clusterStart = true; + } + } +- +- { +- // adjust clusters +- uint glyph_pos = 0; +- for (uint i = 0; i < item_length; ++i) { +- if (i + item_pos != infos[glyph_pos].cluster) { +- for (uint j = glyph_pos + 1; j < num_glyphs; ++j) { +- if (i + item_pos <= infos[j].cluster) { +- if (i + item_pos == infos[j].cluster) +- glyph_pos = j; +- break; +- } +- } +- } +- log_clusters[i] = glyph_pos + item_glyph_pos; +- } +- } ++ while (str_pos < item_length) ++ log_clusters[str_pos++] = last_glyph_pos; + + if (engineIdx != 0) { + for (quint32 i = 0; i < num_glyphs; ++i) +-- +1.9.0 + === added file 'debian/patches/HarfBuzz-NG-Hide-characters-that-should-normally-be-.patch' --- debian/patches/HarfBuzz-NG-Hide-characters-that-should-normally-be-.patch 1970-01-01 00:00:00 +0000 +++ debian/patches/HarfBuzz-NG-Hide-characters-that-should-normally-be-.patch 2014-03-18 11:51:23 +0000 @@ -0,0 +1,50 @@ +From 80588b02d81bed0ed225054055075505a9096d84 Mon Sep 17 00:00:00 2001 +From: Konstantin Ritt <[email protected]> +Date: Tue, 18 Mar 2014 02:42:47 +0200 +Subject: [PATCH] HarfBuzz-NG: Hide characters that should normally be + invisible + +These are non-ambigue NLF characters that should only imply the +sctructure of the document. +For details, see http://www.unicode.org/reports/tr13/ . + +The issue could be reproduced with use of multi-line QML Text element. + +This is a backport of 6a7747f30cc853f07501770a8704 from 5.3 + +Task-number: QTBUG-37475 + +Change-Id: Ie32bdf18f4a0225496895d5d29e94e4497dcc150 +--- + src/gui/text/qtextengine.cpp | 15 +++++++++++++++ + 1 file changed, 15 insertions(+) + +diff --git a/src/gui/text/qtextengine.cpp b/src/gui/text/qtextengine.cpp +index febdaaa..84197af 100644 +--- a/src/gui/text/qtextengine.cpp ++++ b/src/gui/text/qtextengine.cpp +@@ -1146,6 +1146,21 @@ int QTextEngine::shapeTextWithHarfbuzzNG(const QScriptItem &si, const ushort *st + + uint cluster = infos[i].cluster; + if (last_cluster != cluster) { ++ if (Q_UNLIKELY(g.glyphs[i] == 0)) { ++ // hide characters that should normally be invisible ++ switch (string[item_pos + str_pos]) { ++ case QChar::LineFeed: ++ case 0x000c: // FormFeed ++ case QChar::CarriageReturn: ++ case QChar::LineSeparator: ++ case QChar::ParagraphSeparator: ++ g.attributes[i].dontPrint = true; ++ break; ++ default: ++ break; ++ } ++ } ++ + // 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) +-- +1.9.0 + === added file 'debian/patches/Minor-optimization-for-QTextEngine-shapeText.patch' --- debian/patches/Minor-optimization-for-QTextEngine-shapeText.patch 1970-01-01 00:00:00 +0000 +++ debian/patches/Minor-optimization-for-QTextEngine-shapeText.patch 2014-03-18 11:51:23 +0000 @@ -0,0 +1,189 @@ +From f5dab563288de0d834225a51b03fd7043c5339cf Mon Sep 17 00:00:00 2001 +From: Konstantin Ritt <[email protected]> +Date: Sun, 9 Feb 2014 06:37:51 +0200 +Subject: [PATCH] Minor optimization for QTextEngine::shapeText() + +Remember the engine index for each sub-item and avoid moveGlyphData() +where possible (ie. when there are no glyph indexes to care about). +Also don't memmove data we didn't ever initialize. + +Change-Id: Ib8e5fd937a10e4e3c8c0e18961a2e2c1a4167924 +--- + src/gui/text/qtextengine.cpp | 79 ++++++++++++++++++++------------------------ + 1 file changed, 36 insertions(+), 43 deletions(-) + +diff --git a/src/gui/text/qtextengine.cpp b/src/gui/text/qtextengine.cpp +index 607fe95..febdaaa 100644 +--- a/src/gui/text/qtextengine.cpp ++++ b/src/gui/text/qtextengine.cpp +@@ -918,12 +918,9 @@ void QTextEngine::shapeText(int item) const + QFontEngine *fontEngine = this->fontEngine(si, &si.ascent, &si.descent, &si.leading); + + // split up the item into parts that come from different font engines ++ // k * 3 entries, array[k] == index in string, array[k + 1] == index in glyphs, array[k + 2] == engine index + QVector<uint> itemBoundaries; +- itemBoundaries.reserve(16); +- // k * 2 entries, array[k] == index in string, array[k + 1] == index in glyphs +- itemBoundaries.append(0); +- itemBoundaries.append(0); +- ++ itemBoundaries.reserve(24); + if (fontEngine->type() == QFontEngine::Multi) { + // ask the font engine to find out which glyphs (as an index in the specific font) + // to use for the text in one item. +@@ -947,22 +944,31 @@ void QTextEngine::shapeText(int item) const + } + } + +- uint lastEngine = 0; ++ uint lastEngine = ~0u; + for (int i = 0, glyph_pos = 0; i < itemLength; ++i, ++glyph_pos) { + const uint engineIdx = initialGlyphs.glyphs[glyph_pos] >> 24; +- if (lastEngine != engineIdx && glyph_pos > 0) { ++ if (lastEngine != engineIdx) { + itemBoundaries.append(i); + itemBoundaries.append(glyph_pos); ++ itemBoundaries.append(engineIdx); ++ ++ if (engineIdx != 0) { ++ QFontEngine *actualFontEngine = static_cast<QFontEngineMulti *>(fontEngine)->engine(engineIdx); ++ si.ascent = qMax(actualFontEngine->ascent(), si.ascent); ++ si.descent = qMax(actualFontEngine->descent(), si.descent); ++ si.leading = qMax(actualFontEngine->leading(), si.leading); ++ } + +- QFontEngine *actualFontEngine = static_cast<QFontEngineMulti *>(fontEngine)->engine(engineIdx); +- si.ascent = qMax(actualFontEngine->ascent(), si.ascent); +- si.descent = qMax(actualFontEngine->descent(), si.descent); +- si.leading = qMax(actualFontEngine->leading(), si.leading); ++ lastEngine = engineIdx; + } +- lastEngine = engineIdx; ++ + if (QChar::isHighSurrogate(string[i]) && i + 1 < itemLength && QChar::isLowSurrogate(string[i + 1])) + ++i; + } ++ } else { ++ itemBoundaries.append(0); ++ itemBoundaries.append(0); ++ itemBoundaries.append(0); + } + + bool kerningEnabled; +@@ -1039,16 +1045,6 @@ void QTextEngine::shapeText(int item) const + si.width += glyphs.advances_x[i] * !glyphs.attributes[i].dontPrint; + } + +-static inline void moveGlyphData(const QGlyphLayout &destination, const QGlyphLayout &source, int num) +-{ +- if (num > 0 && destination.glyphs != source.glyphs) { +- memmove(destination.glyphs, source.glyphs, num * sizeof(glyph_t)); +- memmove(destination.attributes, source.attributes, num * sizeof(QGlyphAttributes)); +- memmove(destination.advances_x, source.advances_x, num * sizeof(QFixed)); +- memmove(destination.offsets, source.offsets, num * sizeof(QFixedPoint)); +- } +-} +- + #ifdef QT_ENABLE_HARFBUZZ_NG + + QT_BEGIN_INCLUDE_NAMESPACE +@@ -1075,20 +1071,15 @@ int QTextEngine::shapeTextWithHarfbuzzNG(const QScriptItem &si, const ushort *st + uint glyphs_shaped = 0; + int remaining_glyphs = itemLength; + +- for (int k = 0; k < itemBoundaries.size(); k += 2) { // for the +2, see the comment at the definition of itemBoundaries ++ for (int k = 0; k < itemBoundaries.size(); k += 3) { + uint item_pos = itemBoundaries[k]; +- uint item_length = itemLength; ++ uint item_length = (k + 4 < itemBoundaries.size() ? itemBoundaries[k + 3] : itemLength) - item_pos; + uint item_glyph_pos = itemBoundaries[k + 1]; +- if (k + 3 < itemBoundaries.size()) +- item_length = itemBoundaries[k + 2]; +- item_length -= item_pos; ++ uint engineIdx = itemBoundaries[k + 2]; + + QFontEngine *actualFontEngine = fontEngine; +- uint engineIdx = 0; +- if (fontEngine->type() == QFontEngine::Multi) { +- engineIdx = availableGlyphs(&si).glyphs[glyphs_shaped] >> 24; ++ if (fontEngine->type() == QFontEngine::Multi) + actualFontEngine = static_cast<QFontEngineMulti *>(fontEngine)->engine(engineIdx); +- } + + + // prepare buffer +@@ -1138,8 +1129,6 @@ int QTextEngine::shapeTextWithHarfbuzzNG(const QScriptItem &si, const ushort *st + + // fetch the shaped glyphs and metrics + QGlyphLayout g = availableGlyphs(&si).mid(glyphs_shaped, num_glyphs); +- if (num_glyphs != item_glyph_pos) +- moveGlyphData(g.mid(num_glyphs), g.mid(item_glyph_pos), remaining_glyphs); + ushort *log_clusters = logClusters(&si) + item_pos; + + hb_glyph_info_t *infos = hb_buffer_get_glyph_infos(buffer, 0); +@@ -1196,6 +1185,12 @@ Q_STATIC_ASSERT(sizeof(HB_GlyphAttributes) == sizeof(QGlyphAttributes)); + Q_STATIC_ASSERT(sizeof(HB_Fixed) == sizeof(QFixed)); + Q_STATIC_ASSERT(sizeof(HB_FixedPoint) == sizeof(QFixedPoint)); + ++static inline void moveGlyphData(const QGlyphLayout &destination, const QGlyphLayout &source, int num) ++{ ++ if (num > 0 && destination.glyphs != source.glyphs) ++ memmove(destination.glyphs, source.glyphs, num * sizeof(glyph_t)); ++} ++ + int QTextEngine::shapeTextWithHarfbuzz(const QScriptItem &si, const ushort *string, int itemLength, QFontEngine *fontEngine, const QVector<uint> &itemBoundaries, bool kerningEnabled) const + { + HB_ShaperItem entire_shaper_item; +@@ -1224,14 +1219,12 @@ int QTextEngine::shapeTextWithHarfbuzz(const QScriptItem &si, const ushort *stri + int remaining_glyphs = entire_shaper_item.num_glyphs; + int glyph_pos = 0; + // for each item shape using harfbuzz and store the results in our layoutData's glyphs array. +- for (int k = 0; k < itemBoundaries.size(); k += 2) { // for the +2, see the comment at the definition of itemBoundaries +- ++ for (int k = 0; k < itemBoundaries.size(); k += 3) { + HB_ShaperItem shaper_item = entire_shaper_item; +- + shaper_item.item.pos = itemBoundaries[k]; +- if (k < itemBoundaries.size() - 3) { +- shaper_item.item.length = itemBoundaries[k + 2] - shaper_item.item.pos; +- shaper_item.num_glyphs = itemBoundaries[k + 3] - itemBoundaries[k + 1]; ++ if (k + 4 < itemBoundaries.size()) { ++ shaper_item.item.length = itemBoundaries[k + 3] - shaper_item.item.pos; ++ shaper_item.num_glyphs = itemBoundaries[k + 4] - itemBoundaries[k + 1]; + } else { // last combo in the list, avoid out of bounds access. + shaper_item.item.length -= shaper_item.item.pos - entire_shaper_item.item.pos; + shaper_item.num_glyphs -= itemBoundaries[k + 1]; +@@ -1240,10 +1233,9 @@ int QTextEngine::shapeTextWithHarfbuzz(const QScriptItem &si, const ushort *stri + if (shaper_item.num_glyphs < shaper_item.item.length) + shaper_item.num_glyphs = shaper_item.item.length; + ++ uint engineIdx = itemBoundaries[k + 2]; + QFontEngine *actualFontEngine = fontEngine; +- uint engineIdx = 0; + if (fontEngine->type() == QFontEngine::Multi) { +- engineIdx = uint(availableGlyphs(&si).glyphs[glyph_pos] >> 24); + actualFontEngine = static_cast<QFontEngineMulti *>(fontEngine)->engine(engineIdx); + + shaper_item.glyphIndicesPresent = true; +@@ -1259,7 +1251,7 @@ int QTextEngine::shapeTextWithHarfbuzz(const QScriptItem &si, const ushort *stri + return 0; + + const QGlyphLayout g = availableGlyphs(&si).mid(glyph_pos); +- if (shaper_item.num_glyphs > shaper_item.item.length) ++ if (fontEngine->type() == QFontEngine::Multi && shaper_item.num_glyphs > shaper_item.item.length) + moveGlyphData(g.mid(shaper_item.num_glyphs), g.mid(shaper_item.initialGlyphCount), remaining_glyphs); + + shaper_item.glyphs = reinterpret_cast<HB_Glyph *>(g.glyphs); +@@ -1276,7 +1268,8 @@ int QTextEngine::shapeTextWithHarfbuzz(const QScriptItem &si, const ushort *stri + } while (!qShapeItem(&shaper_item)); // this does the actual shaping via harfbuzz. + + QGlyphLayout g = availableGlyphs(&si).mid(glyph_pos, shaper_item.num_glyphs); +- moveGlyphData(g.mid(shaper_item.num_glyphs), g.mid(shaper_item.initialGlyphCount), remaining_glyphs); ++ if (fontEngine->type() == QFontEngine::Multi) ++ moveGlyphData(g.mid(shaper_item.num_glyphs), g.mid(shaper_item.initialGlyphCount), remaining_glyphs); + + for (quint32 i = 0; i < shaper_item.item.length; ++i) + shaper_item.log_clusters[i] += glyph_pos; +-- +1.9.0 + === modified file 'debian/patches/series' --- debian/patches/series 2014-03-18 05:45:20 +0000 +++ debian/patches/series 2014-03-18 11:51:23 +0000 @@ -24,3 +24,7 @@ Add-workaround-for-GL-on-Android-emulator.patch enable-tests.patch +Fix-access-after-delete-with-Harfbuzz-NG-code-path.patch +Fix-log_clusters-calculation-in-HarfBuzz-NG-code-pat.patch +Minor-optimization-for-QTextEngine-shapeText.patch +HarfBuzz-NG-Hide-characters-that-should-normally-be-.patch
-- kubuntu-devel mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/kubuntu-devel
