Bug 51106 – [Qt] Implement the fast font path for Qt

• Component: Text
• Status: RESOLVED FIXED

[Andreas Kling]

We should implement WebKit's fast font path using the new QGlyphs API in Qt 4.8.

[Andreas Kling]

Created attachment 76646 [details]
Patch (work in progress)

Uploading my current WIP for feedback.

[Ariya Hidayat]

Comment on attachment 76646 [details]
Patch (work in progress)

View in context: https://bugs.webkit.org/attachment.cgi?id=76646&action=review

> WebCore/WebCore.pro:3399
> +greaterThan(QT_MAJOR_VERSION,3):greaterThan(QT_MINOR_VERSION, 7):HAVE_QGLYPHS=1

Hmm, Qt > 3.7 ?

> WebCore/platform/graphics/qt/FontQt.cpp:119
> +	textFillPen = fillPenForContext(ctx);

Tab?

> WebCore/platform/graphics/qt/FontQt.cpp:314
> +    float x1 = line.cursorToX(from);
> +    float x2 = line.cursorToX(to);
> +    if (x2 < x1)
> +        qSwap(x1, x2);

Why not taking the min and max?

[Andreas Kling]

> > WebCore/WebCore.pro:3399
> > +greaterThan(QT_MAJOR_VERSION,3):greaterThan(QT_MINOR_VERSION, 7):HAVE_QGLYPHS=1
> 
> Hmm, Qt > 3.7 ?

>= 4.8 actually. Don't get me started on qmake syntax...

> Tab?

grmbl.. oops :)

> > WebCore/platform/graphics/qt/FontQt.cpp:314
> > +    float x1 = line.cursorToX(from);
> > +    float x2 = line.cursorToX(to);
> > +    if (x2 < x1)
> > +        qSwap(x1, x2);
> 
> Why not taking the min and max?

This code was just moved in this patch.

[Csaba Osztrogonác]

Comment on attachment 76646 [details]
Patch (work in progress)

View in context: https://bugs.webkit.org/attachment.cgi?id=76646&action=review

>> WebCore/WebCore.pro:3399
>> +greaterThan(QT_MAJOR_VERSION,3):greaterThan(QT_MINOR_VERSION, 7):HAVE_QGLYPHS=1
> 
> Hmm, Qt > 3.7 ?

a more readable version check: !lessThan(QT_MAJOR_VERSION,4):!lessThan(QT_MINOR_VERSION, 8):HAVE_QGLYPHS=1

[Andreas Kling]

Created attachment 78118 [details]
Patch (work in progress)

Rebased patch.

[Andreas Kling]

Created attachment 86212 [details]
Proposed patch

[Andreas Kling]

Created attachment 86213 [details]
Proposed patch

[WebKit Review Bot]

Attachment 86213 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/qt/FontQt.cpp:329:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/qt/FontQt.cpp:331:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 86213 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8203370

[Kenneth Rohde Christiansen]

Comment on attachment 86213 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86213&action=review

Would be nice if this compiled on Qt 4.7 as well, which is what I suppose the buildbots are running?

> Source/WebCore/WebCore.pro:3086
> +!lessThan(QT_MAJOR_VERSION,4):!lessThan(QT_MINOR_VERSION, 8):HAVE_QGLYPHS=1

why no space before 4?

> Source/WebCore/platform/graphics/Font.cpp:151
> +#if PLATFORM(QT) && HAVE(QGLYPHS)
> +    if (context->textDrawingMode() & TextModeStroke || context->contextShadow()->m_type != ContextShadow::NoShadow)
> +        codePathToUse = Complex;
> +#endif

Why is this not in the codePath method? any particular reason?

> Source/WebCore/platform/graphics/qt/FontQt.cpp:353
> +    // Shadowed text should always take the complex path.

Text with shadow? Shadowed sounds a bit strange to me

> Source/WebCore/platform/graphics/qt/FontQt.cpp:359
> +    QVector<quint32> glyphIndexes;

Indices?

> Source/WebCore/platform/graphics/qt/GlyphPageTreeNodeQt.cpp:54
> +\

What is this for?

[Andreas Kling]

(In reply to comment #10)
> Would be nice if this compiled on Qt 4.7 as well, which is what I suppose the buildbots are running?

That's a must-have obviously.

> > Source/WebCore/WebCore.pro:3086
> > +!lessThan(QT_MAJOR_VERSION,4):!lessThan(QT_MINOR_VERSION, 8):HAVE_QGLYPHS=1
> 
> why no space before 4?

Will fix. Though now that I think of it, we could just use QT_VERSION_CHECK and check for 4.8 instead of using a HAVE(QGLYPHS) macro..

> > Source/WebCore/platform/graphics/Font.cpp:151
> > +#if PLATFORM(QT) && HAVE(QGLYPHS)
> > +    if (context->textDrawingMode() & TextModeStroke || context->contextShadow()->m_type != ContextShadow::NoShadow)
> > +        codePathToUse = Complex;
> > +#endif
> 
> Why is this not in the codePath method? any particular reason?

The codePath method doesn't have access to a GraphicsContext.

> > Source/WebCore/platform/graphics/qt/FontQt.cpp:353
> > +    // Shadowed text should always take the complex path.
> 
> Text with shadow? Shadowed sounds a bit strange to me

Agree, will change.

> > Source/WebCore/platform/graphics/qt/FontQt.cpp:359
> > +    QVector<quint32> glyphIndexes;
> 
> Indices?

Both are valid words, I would normally call it Indices myself, but in this case the Qt API calls them glyphIndexes so I used the same name for the variable.

> > Source/WebCore/platform/graphics/qt/GlyphPageTreeNodeQt.cpp:54
> > +\
> 
> What is this for?

It improves performance about 200%. :)

[Kenneth Rohde Christiansen]

> > > +\
> > 
> > What is this for?
> 
> It improves performance about 200%. :)

Then let's add some more of those \ in the code :-)

[Simon Hausmann]

Comment on attachment 86213 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86213&action=review

> Source/WebCore/platform/graphics/qt/GlyphPageTreeNodeQt.cpp:67
> +    for (unsigned i = 0; i < length; ++i) {
> +        Glyph glyph = getGlyphForCharacterAndFont(qstring.at(i), font);

Wouldn't it be nice to get the glyphs for the page in one shot, instead of creating a QTextLayout (that'll heap alloc in the impl) for each character? :)

[Andreas Kling]

(In reply to comment #13)
> (From update of attachment 86213 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86213&action=review
> 
> > Source/WebCore/platform/graphics/qt/GlyphPageTreeNodeQt.cpp:67
> > +    for (unsigned i = 0; i < length; ++i) {
> > +        Glyph glyph = getGlyphForCharacterAndFont(qstring.at(i), font);
> 
> Wouldn't it be nice to get the glyphs for the page in one shot, instead of creating a QTextLayout (that'll heap alloc in the impl) for each character? :)

Most definitely.

For anyone passing by, this bug is currently pending new API in Qt before deciding how to continue. :)

[Andreas Kling]

Created attachment 91928 [details]
Proposed patch v2

This patch uses the new QRawFont APIs in the fire-team branch of Qt 4.8.

[Early Warning System Bot]

Attachment 91928 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8534073

[Andreas Kling]

Created attachment 91933 [details]
Proposed patch v3

Let's try to make that build on Qt 4.7..

[Early Warning System Bot]

Attachment 91933 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8531707

[Andreas Kling]

(In reply to comment #18)
> Attachment 91933 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/8531707

Sigh. I'll leave this here for Simon to look at, since it will likely need multiple iterations anyway.

[Andreas Kling]

Created attachment 92225 [details]
Proposed patch v4

Updated patch after IRL review with Simon.
Renamed HAVE(QGLYPHS) to HAVE(QRAWFONT) for inexplicable aesthetic reasons.
The GlyphPage::fill() friend hack is removed in favor of implementing SimpleFontData::platformWidthForGlyph().
As a side-effect, advances for zero-size fonts are now 0 (as they should be.)
I have requested an API to disable unneeded caching in Qt's font engines, but this is not yet available, and will be used in a follow-up patch.

[Eric Seidel (no email)]

Attachment 92225 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/qt/FontCacheQt.cpp:101:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

(In reply to comment #21)
> Attachment 92225 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> 
> Source/WebCore/platform/graphics/qt/FontCacheQt.cpp:101:  Should have a space between // and comment  [whitespace/comments] [4]

Whoops, that's my bad. Would not land with this, obviously. :)

[Simon Hausmann]

Comment on attachment 92225 [details]
Proposed patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=92225&action=review

r=me

Follow-up stuff:

    1) Implement stroking and shadowing for drawGlyphs (maybe in Qt)
    2) Extend QRawFont APIs so that we can pass const QChar* str, int len and get glyphs/advances in out-arrays, so that we can use QVarLengthArray on the WebKit side
    3) Other stuff I forgot ;)

>> Source/WebCore/platform/graphics/qt/FontCacheQt.cpp:101
>> +    //return getCachedFontData(fontDescription, fallbackFamily);
> 
> Should have a space between // and comment  [whitespace/comments] [4]

Remove this before landing :)

> Source/WebCore/platform/graphics/qt/FontQt.cpp:337
> +    QVector<quint32> glyphIndexes;
> +    QVector<QPointF> positions;

These should be QVarLengthArrays. Fix before landing I'd say.

> Source/WebCore/platform/graphics/qt/FontQt.cpp:441
> +bool Font::primaryFontHasGlyphForCharacter(UChar32) const
> +{
> +    notImplemented();
> +    return true;
> +}

This should be pretty easy to implement now, no?

> Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:138
> +    QVector<quint32> indexes = rawFont.glyphIndexesForString(QLatin1String(" "));

Urgh, I think we really need QRawFont::glyphIndexForCharacter, or better: an overload that takes a const QChar* and int len.

[Simon Hausmann]

(In reply to comment #23)
> > Source/WebCore/platform/graphics/qt/FontQt.cpp:441
> > +bool Font::primaryFontHasGlyphForCharacter(UChar32) const
> > +{
> > +    notImplemented();
> > +    return true;
> > +}
> 
> This should be pretty easy to implement now, no?

Forget this comment please :)

[Andreas Kling]

Committed r85853: <http://trac.webkit.org/changeset/85853>

[Csaba Osztrogonác]

With the latest Qt 4.8 I got this fail:

../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp: In member function ‘void WebCore::SimpleFontData::platformInit()’:
../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:135: error: ‘class QRawFont’ has no member named ‘xHeight’
../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:136: error: ‘class QRawFont’ has no member named ‘leading’
../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp: In member function ‘void WebCore::SimpleFontData::platformCharWidthInit()’:
../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:178: error: ‘class QRawFont’ has no member named ‘averageCharWidth’
../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:179: error: ‘class QRawFont’ has no member named ‘maxCharWidth’


What kind of Qt do you use? :o Is it a hyper secret internal version or what?

[Andreas Kling]

(In reply to comment #26)
> With the latest Qt 4.8 I got this fail:
> 
> ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp: In member function ‘void WebCore::SimpleFontData::platformInit()’:
> ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:135: error: ‘class QRawFont’ has no member named ‘xHeight’
> ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:136: error: ‘class QRawFont’ has no member named ‘leading’
> ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp: In member function ‘void WebCore::SimpleFontData::platformCharWidthInit()’:
> ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:178: error: ‘class QRawFont’ has no member named ‘averageCharWidth’
> ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:179: error: ‘class QRawFont’ has no member named ‘maxCharWidth’
> 
> 
> What kind of Qt do you use? :o Is it a hyper secret internal version or what?

I'm using the fire-team pre-integration repository, not sure when it will be merged into mainline Qt. Jiang?

[Jiang Jiang]

(In reply to comment #27)
> (In reply to comment #26)
> > With the latest Qt 4.8 I got this fail:
> > 
> > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp: In member function ‘void WebCore::SimpleFontData::platformInit()’:
> > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:135: error: ‘class QRawFont’ has no member named ‘xHeight’
> > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:136: error: ‘class QRawFont’ has no member named ‘leading’
> > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp: In member function ‘void WebCore::SimpleFontData::platformCharWidthInit()’:
> > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:178: error: ‘class QRawFont’ has no member named ‘averageCharWidth’
> > ../../../Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:179: error: ‘class QRawFont’ has no member named ‘maxCharWidth’
> > 
> > 
> > What kind of Qt do you use? :o Is it a hyper secret internal version or what?
> 
> I'm using the fire-team pre-integration repository, not sure when it will be merged into mainline Qt. Jiang?

It seems that last integration attempt was blocked by some configuration problem of our internal CI server, my guess is that it will be integrated to mainline Qt in the next round, which means a couple of days or so.

[Csaba Osztrogonác]

I absolutely confused ... Is there a policy somewhere for what can be landed in WebKit trunk or not? I thought Qt 4.8 means 4.8 branch on gitorious. But now we
can't get regressions on trunk until QRawFont changes goes to the 4.8 branch and I rebuild Qt and install it to the bot.

It would be better for buildbot if we require that WebKit trunk 
should be build with Qt 4.8 weekly build from 4.8 branch.

[Kenneth Rohde Christiansen]

kling++! woot!