QRawFont: improve performance and safety of glyphIndexesForString()

As of 98c1eb1750498cdff9d3b26658e5e5be9c026c92, partially initialized
QGlyphLayout is ok for stringToCMap() if GlyphIndicesOnly flag is set,
thus we can use the glyphIndexes buffer directly and avoid copying.
Also add some checks to guarantee we're not falling into an undefined behavior
for the empty text or NULL buffer.

Change-Id: I662953703e4c65edbebabbe4b753972417d963f3
Reviewed-by: Eskil Abrahamsen Blomfeldt <eskil.abrahamsen-blomfeldt@digia.com>

diff --git a/src/gui/text/qrawfont.cpp b/src/gui/text/qrawfont.cpp
index dee1abc..f1191bc 100644 (file)
--- a/src/gui/text/qrawfont.cpp
+++ b/src/gui/text/qrawfont.cpp
@@ -388,10 +388,7 @@ qreal QRawFont::unitsPerEm() const
  */
 qreal QRawFont::lineThickness() const
 {
-    if (!isValid())
-        return 0.0;
-
-    return d->fontEngine->lineThickness().toReal();
+    return d->isValid() ? d->fontEngine->lineThickness().toReal() : 0.0;
 }
 
 /*!
@@ -400,10 +397,7 @@ qreal QRawFont::lineThickness() const
  */
 qreal QRawFont::underlinePosition() const
 {
-    if (!isValid())
-        return 0.0;
-
-    return d->fontEngine->underlinePosition().toReal();
+    return d->isValid() ? d->fontEngine->underlinePosition().toReal() : 0.0;
 }
 
 /*!
@@ -459,23 +453,28 @@ int QRawFont::weight() const
 */
 QVector<quint32> QRawFont::glyphIndexesForString(const QString &text) const
 {
-    if (!d->isValid())
-        return QVector<quint32>();
+    QVector<quint32> glyphIndexes;
+    if (!d->isValid() || text.isEmpty())
+        return glyphIndexes;
+
+    int numGlyphs = text.size();
+    glyphIndexes.resize(numGlyphs);
 
-    int nglyphs = text.size();
-    QVarLengthGlyphLayoutArray glyphs(nglyphs);
-    if (!glyphIndexesForChars(text.data(), text.size(), glyphs.glyphs, &nglyphs)) {
-        glyphs.resize(nglyphs);
-        if (!glyphIndexesForChars(text.data(), text.size(), glyphs.glyphs, &nglyphs)) {
+    QGlyphLayout glyphs;
+    glyphs.numGlyphs = numGlyphs;
+    glyphs.glyphs = glyphIndexes.data();
+    if (!d->fontEngine->stringToCMap(text.data(), text.size(), &glyphs, &numGlyphs, QFontEngine::GlyphIndicesOnly)) {
+        glyphIndexes.resize(numGlyphs);
+
+        glyphs.numGlyphs = numGlyphs;
+        glyphs.glyphs = glyphIndexes.data();
+        if (!d->fontEngine->stringToCMap(text.data(), text.size(), &glyphs, &numGlyphs, QFontEngine::GlyphIndicesOnly)) {
             Q_ASSERT_X(false, Q_FUNC_INFO, "stringToCMap shouldn't fail twice");
             return QVector<quint32>();
         }
     }
 
-    QVector<quint32> glyphIndexes;
-    for (int i=0; i<nglyphs; ++i)
-        glyphIndexes.append(glyphs.glyphs[i]);
-
+    glyphIndexes.resize(numGlyphs);
     return glyphIndexes;
 }
 
@@ -491,38 +490,32 @@ QVector<quint32> QRawFont::glyphIndexesForString(const QString &text) const
 */
 bool QRawFont::glyphIndexesForChars(const QChar *chars, int numChars, quint32 *glyphIndexes, int *numGlyphs) const
 {
-    if (!d->isValid())
+    Q_ASSERT(numGlyphs);
+    if (!d->isValid() || numChars <= 0) {
+        *numGlyphs = 0;
+        return false;
+    }
+
+    if (*numGlyphs <= 0 || !glyphIndexes) {
+        *numGlyphs = numChars;
         return false;
+    }
 
     QGlyphLayout glyphs;
+    glyphs.numGlyphs = *numGlyphs;
     glyphs.glyphs = glyphIndexes;
     return d->fontEngine->stringToCMap(chars, numChars, &glyphs, numGlyphs, QFontEngine::GlyphIndicesOnly);
 }
 
 /*!
+    \fn QVector<QPointF> QRawFont::advancesForGlyphIndexes(const QVector<quint32> &glyphIndexes) const
+
    Returns the QRawFont's advances for each of the \a glyphIndexes in pixel units. The advances
    give the distance from the position of a given glyph to where the next glyph should be drawn
    to make it appear as if the two glyphs are unspaced.
 
    \sa QTextLine::horizontalAdvance(), QFontMetricsF::width()
 */
-QVector<QPointF> QRawFont::advancesForGlyphIndexes(const QVector<quint32> &glyphIndexes) const
-{
-    if (!d->isValid())
-        return QVector<QPointF>();
-
-    int numGlyphs = glyphIndexes.size();
-    QVarLengthGlyphLayoutArray glyphs(numGlyphs);
-    memcpy(glyphs.glyphs, glyphIndexes.data(), numGlyphs * sizeof(quint32));
-
-    d->fontEngine->recalcAdvances(&glyphs, 0);
-
-    QVector<QPointF> advances;
-    for (int i=0; i<numGlyphs; ++i)
-        advances.append(QPointF(glyphs.advances_x[i].toReal(), glyphs.advances_y[i].toReal()));
-
-    return advances;
-}
 
 /*!
    Returns the QRawFont's advances for each of the \a glyphIndexes in pixel units. The advances
@@ -535,7 +528,8 @@ QVector<QPointF> QRawFont::advancesForGlyphIndexes(const QVector<quint32> &glyph
 */
 bool QRawFont::advancesForGlyphIndexes(const quint32 *glyphIndexes, QPointF *advances, int numGlyphs) const
 {
-    if (!d->isValid())
+    Q_ASSERT(glyphIndexes && advances);
+    if (!d->isValid() || numGlyphs <= 0)
         return false;
 
     QGlyphLayout glyphs;

diff --git a/src/gui/text/qrawfont.h b/src/gui/text/qrawfont.h
index 1dbde27..9831480 100644 (file)
--- a/src/gui/text/qrawfont.h
+++ b/src/gui/text/qrawfont.h
@@ -94,7 +94,7 @@ public:
     int weight() const;
 
     QVector<quint32> glyphIndexesForString(const QString &text) const;
-    QVector<QPointF> advancesForGlyphIndexes(const QVector<quint32> &glyphIndexes) const;
+    inline QVector<QPointF> advancesForGlyphIndexes(const QVector<quint32> &glyphIndexes) const;
     bool glyphIndexesForChars(const QChar *chars, int numChars, quint32 *glyphIndexes, int *numGlyphs) const;
     bool advancesForGlyphIndexes(const quint32 *glyphIndexes, QPointF *advances, int numGlyphs) const;
 
@@ -147,6 +147,14 @@ private:
 
 Q_DECLARE_SHARED(QRawFont)
 
+inline QVector<QPointF> QRawFont::advancesForGlyphIndexes(const QVector<quint32> &glyphIndexes) const
+{
+    QVector<QPointF> advances(glyphIndexes.size());
+    if (advancesForGlyphIndexes(glyphIndexes.constData(), advances.data(), glyphIndexes.size()))
+        return advances;
+    return QVector<QPointF>();
+}
+
 QT_END_NAMESPACE
 
 QT_END_HEADER

diff --git a/tests/auto/gui/text/qrawfont/tst_qrawfont.cpp b/tests/auto/gui/text/qrawfont/tst_qrawfont.cpp
index 7a25454..c26964e 100644 (file)
--- a/tests/auto/gui/text/qrawfont/tst_qrawfont.cpp
+++ b/tests/auto/gui/text/qrawfont/tst_qrawfont.cpp
@@ -267,6 +267,24 @@ void tst_QRawFont::glyphIndices()
     expectedGlyphIndices << 44 << 83 << 83 << 70 << 69 << 86;
 
     QCOMPARE(glyphIndices, expectedGlyphIndices);
+
+    glyphIndices = font.glyphIndexesForString(QString());
+    QVERIFY(glyphIndices.isEmpty());
+
+    QString str(QLatin1String("Foobar"));
+    int numGlyphs = str.size();
+    glyphIndices.resize(numGlyphs);
+
+    QVERIFY(!font.glyphIndexesForChars(str.constData(), 0, glyphIndices.data(), &numGlyphs));
+    QCOMPARE(numGlyphs, 0);
+
+    QVERIFY(!font.glyphIndexesForChars(str.constData(), str.size(), glyphIndices.data(), &numGlyphs));
+    QCOMPARE(numGlyphs, str.size());
+
+    QVERIFY(font.glyphIndexesForChars(str.constData(), str.size(), glyphIndices.data(), &numGlyphs));
+    QCOMPARE(numGlyphs, str.size());
+
+    QCOMPARE(glyphIndices, expectedGlyphIndices);
 }
 
 void tst_QRawFont::advances_data()
@@ -310,6 +328,33 @@ void tst_QRawFont::advances()
 
         QVERIFY(qFuzzyIsNull(advances.at(i).y()));
     }
+
+    advances = font.advancesForGlyphIndexes(QVector<quint32>());
+    QVERIFY(advances.isEmpty());
+
+    int numGlyphs = glyphIndices.size();
+    advances.resize(numGlyphs);
+
+    QVERIFY(!font.advancesForGlyphIndexes(glyphIndices.constData(), advances.data(), 0));
+
+    QVERIFY(font.advancesForGlyphIndexes(glyphIndices.constData(), advances.data(), numGlyphs));
+
+    for (int i=0; i<glyphIndices.size(); ++i) {
+#ifdef Q_OS_WIN
+        // In Windows, freetype engine returns advance of 9 when full hinting is used (default) for
+        // some of the glyphs.
+        if (font_d->fontEngine->type() == QFontEngine::Freetype
+            && (hintingPreference == QFont::PreferFullHinting || hintingPreference == QFont::PreferDefaultHinting)
+            && (i == 0 || i == 5)) {
+            QEXPECT_FAIL("", "Advance for some glyphs is not the expected with Windows Freetype engine (9 instead of 8)", Continue);
+        }
+#endif
+        QVERIFY(qFuzzyCompare(qRound(advances.at(i).x()), 8.0));
+        if (supportsSubPixelPositions)
+            QVERIFY(advances.at(i).x() > 8.0);
+
+        QVERIFY(qFuzzyIsNull(advances.at(i).y()));
+    }
 }
 
 void tst_QRawFont::textLayout()