WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
51106
[Qt] Implement the fast font path for Qt
https://bugs.webkit.org/show_bug.cgi?id=51106
Summary
[Qt] Implement the fast font path for Qt
Andreas Kling
Reported
2010-12-15 06:33:31 PST
We should implement WebKit's fast font path using the new QGlyphs API in Qt 4.8.
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
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Andreas Kling
Comment 1
2010-12-15 08:02:17 PST
Created
attachment 76646
[details]
Patch (work in progress) Uploading my current WIP for feedback.
Ariya Hidayat
Comment 2
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?
Andreas Kling
Comment 3
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.
Csaba Osztrogonác
Comment 4
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
Andreas Kling
Comment 5
2011-01-06 07:31:32 PST
Created
attachment 78118
[details]
Patch (work in progress) Rebased patch.
Andreas Kling
Comment 6
2011-03-18 14:24:38 PDT
Created
attachment 86212
[details]
Proposed patch
Andreas Kling
Comment 7
2011-03-18 14:27:20 PDT
Created
attachment 86213
[details]
Proposed patch
WebKit Review Bot
Comment 8
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.
Early Warning System Bot
Comment 9
2011-03-18 14:46:12 PDT
Attachment 86213
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/8203370
Kenneth Rohde Christiansen
Comment 10
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?
Andreas Kling
Comment 11
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%. :)
Kenneth Rohde Christiansen
Comment 12
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 :-)
Simon Hausmann
Comment 13
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? :)
Andreas Kling
Comment 14
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. :)
Andreas Kling
Comment 15
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.
Early Warning System Bot
Comment 16
2011-05-02 09:42:34 PDT
Attachment 91928
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/8534073
Andreas Kling
Comment 17
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..
Early Warning System Bot
Comment 18
2011-05-02 10:11:26 PDT
Attachment 91933
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/8531707
Andreas Kling
Comment 19
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.
Andreas Kling
Comment 20
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.
Eric Seidel (no email)
Comment 21
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.
Andreas Kling
Comment 22
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. :)
Simon Hausmann
Comment 23
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.
Simon Hausmann
Comment 24
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 :)
Andreas Kling
Comment 25
2011-05-05 08:20:23 PDT
Committed
r85853
: <
http://trac.webkit.org/changeset/85853
>
Csaba Osztrogonác
Comment 26
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?
Andreas Kling
Comment 27
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?
Jiang Jiang
Comment 28
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.
Csaba Osztrogonác
Comment 29
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.
Kenneth Rohde Christiansen
Comment 30
2011-05-05 10:36:17 PDT
kling++! woot!
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug