Bug 51106 - [Qt] Implement the fast font path for Qt
Summary: [Qt] Implement the fast font path for Qt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: Performance, Qt, QtTriaged
Depends on: 60544
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-15 06:33 PST by Andreas Kling
Modified: 2011-05-10 04:39 PDT (History)
12 users (show)

See Also:


Attachments
Patch (work in progress) (20.45 KB, patch)
2010-12-15 08:02 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (work in progress) (22.71 KB, patch)
2011-01-06 07:31 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch (26.31 KB, patch)
2011-03-18 14:24 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch (28.87 KB, patch)
2011-03-18 14:27 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch v2 (31.64 KB, patch)
2011-05-02 09:29 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch v3 (31.70 KB, patch)
2011-05-02 09:53 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch v4 (32.39 KB, patch)
2011-05-04 06:06 PDT, Andreas Kling
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2010-12-15 06:33:31 PST
We should implement WebKit's fast font path using the new QGlyphs API in Qt 4.8.
Comment 1 Andreas Kling 2010-12-15 08:02:17 PST
Created attachment 76646 [details]
Patch (work in progress)

Uploading my current WIP for feedback.
Comment 2 Ariya Hidayat 2010-12-15 09:41:06 PST
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?
Comment 3 Andreas Kling 2010-12-16 04:30:16 PST
> > 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.
Comment 4 Csaba Osztrogonác 2010-12-22 04:00:37 PST
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
Comment 5 Andreas Kling 2011-01-06 07:31:32 PST
Created attachment 78118 [details]
Patch (work in progress)

Rebased patch.
Comment 6 Andreas Kling 2011-03-18 14:24:38 PDT
Created attachment 86212 [details]
Proposed patch
Comment 7 Andreas Kling 2011-03-18 14:27:20 PDT
Created attachment 86213 [details]
Proposed patch
Comment 8 WebKit Review Bot 2011-03-18 14:30:14 PDT
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.
Comment 9 Early Warning System Bot 2011-03-18 14:46:12 PDT
Attachment 86213 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8203370
Comment 10 Kenneth Rohde Christiansen 2011-03-18 14:56:27 PDT
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?
Comment 11 Andreas Kling 2011-03-19 09:54:24 PDT
(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%. :)
Comment 12 Kenneth Rohde Christiansen 2011-03-19 09:58:21 PDT
> > > +\
> > 
> > What is this for?
> 
> It improves performance about 200%. :)

Then let's add some more of those \ in the code :-)
Comment 13 Simon Hausmann 2011-03-21 09:09:51 PDT
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? :)
Comment 14 Andreas Kling 2011-03-27 06:40:56 PDT
(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. :)
Comment 15 Andreas Kling 2011-05-02 09:29:42 PDT
Created attachment 91928 [details]
Proposed patch v2

This patch uses the new QRawFont APIs in the fire-team branch of Qt 4.8.
Comment 16 Early Warning System Bot 2011-05-02 09:42:34 PDT
Attachment 91928 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8534073
Comment 17 Andreas Kling 2011-05-02 09:53:08 PDT
Created attachment 91933 [details]
Proposed patch v3

Let's try to make that build on Qt 4.7..
Comment 18 Early Warning System Bot 2011-05-02 10:11:26 PDT
Attachment 91933 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8531707
Comment 19 Andreas Kling 2011-05-02 10:13:41 PDT
(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.
Comment 20 Andreas Kling 2011-05-04 06:06:43 PDT
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.
Comment 21 Eric Seidel 2011-05-04 06:08:11 PDT
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.
Comment 22 Andreas Kling 2011-05-04 06:11:51 PDT
(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. :)
Comment 23 Simon Hausmann 2011-05-05 07:46:56 PDT
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.
Comment 24 Simon Hausmann 2011-05-05 07:47:36 PDT
(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 :)
Comment 25 Andreas Kling 2011-05-05 08:20:23 PDT
Committed r85853: <http://trac.webkit.org/changeset/85853>
Comment 26 Csaba Osztrogonác 2011-05-05 09:03:59 PDT
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?
Comment 27 Andreas Kling 2011-05-05 09:09:29 PDT
(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?
Comment 28 Jiang Jiang 2011-05-05 09:17:21 PDT
(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.
Comment 29 Csaba Osztrogonác 2011-05-05 09:52:26 PDT
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.
Comment 30 Kenneth Rohde Christiansen 2011-05-05 10:36:17 PDT
kling++! woot!