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
Patch (work in progress) (22.71 KB, patch)
2011-01-06 07:31 PST, Andreas Kling
no flags
Proposed patch (26.31 KB, patch)
2011-03-18 14:24 PDT, Andreas Kling
no flags
Proposed patch (28.87 KB, patch)
2011-03-18 14:27 PDT, Andreas Kling
no flags
Proposed patch v2 (31.64 KB, patch)
2011-05-02 09:29 PDT, Andreas Kling
no flags
Proposed patch v3 (31.70 KB, patch)
2011-05-02 09:53 PDT, Andreas Kling
no flags
Proposed patch v4 (32.39 KB, patch)
2011-05-04 06:06 PDT, Andreas Kling
hausmann: review+
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
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
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
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
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.