Closed
Bug 947812
Opened 11 years ago
Closed 11 years ago
use DirectWrite API's for looking up font postscript/fullnames
Categories
(Core :: Graphics: Text, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: jtd, Assigned: jtd)
References
(Blocks 1 open bug)
Details
(Whiteboard: [qa-])
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
When local fonts are looked up via src: local(xxx) in @font-face rules, we need to do a mapping of facename to font. Under DirectWrite, no such service is provided by the OS, so gfxPlatformFontList::InitFaceNameLists scans over fonts on the system and reads in the postscript/fullnames for all fonts by reading the name table of each font.
More recent versions of DirectWrite now support the ability to pull the fullname/postscript name via a DirectWrite call rather than reading it directly from the name table. My tests indicate that this is significantly faster and appears to be cached by DirectWrite at the system level.
http://msdn.microsoft.com/en-us/library/windows/desktop/dd368094%28v=vs.85%29.aspx
DWRITE_INFORMATIONAL_STRING_FULL_NAME
DWRITE_INFORMATIONAL_STRING_POSTSCRIPT_NAME
Note: the fullname value is in the dwrite.h header file but not in the MSDN docs, gak! These are in the Win SDK8 headers but not the Win SDK7 headers which means we'll have to have some #ifdef nastiness until builds are based on Win SDK8+.
It's not entirely clear what the version of DirectWrite is that supports this but since DirectWrite updates are pushed automatically by Windows Update, I think it makes sense to update the code to use the DirectWrite calls when provided.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #8344527 -
Flags: review?
Assignee | ||
Updated•11 years ago
|
Attachment #8344527 -
Flags: review? → review?(bas)
Assignee | ||
Comment 2•11 years ago
|
||
Comment on attachment 8344527 [details] [diff] [review]
patch, use DirectWrite API methods to lookup postscript/fullnames
Seeing a bunch of mochitest/reftest failures, need to track those down before review.
Attachment #8344527 -
Flags: review?(bas)
Assignee | ||
Comment 3•11 years ago
|
||
The reftests were failing because they use the version of DirectWrite that doesn't support accessing the fullname/postscript names and the code I had written wasn't handling that case correctly. I loaded up an earlier version of Windows 7 in a VM and was able to duplicate the problem. I've updated the patch and tested it against Windows 7 SP1, Windows 7 current and Windows 8.1.
Attachment #8344527 -
Attachment is obsolete: true
Attachment #8345159 -
Flags: review?(bas)
Assignee | ||
Comment 4•11 years ago
|
||
Assignee | ||
Comment 5•11 years ago
|
||
We already record this via Telemetry but recording to a log is useful for diagnosing when InitFaceNames/InitOtherNames are slow.
Attachment #8345163 -
Flags: review?(bas)
Assignee | ||
Comment 6•11 years ago
|
||
Running with a single browser window configured to open up with a testcase that uses src local:
With GDI (gfx.direct2d disabled, direct table reads):
cold: 2863ms
warm: 113ms
With DirectWrite and patch:
cold: 34ms
warm: 3ms
Where "cold" is first time after system restart, and "warm" is the next run.
Comment 7•11 years ago
|
||
Comment on attachment 8345159 [details] [diff] [review]
patch, use DirectWrite API methods to lookup postscript/fullnames
Review of attachment 8345159 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/thebes/gfxDWriteFontList.cpp
@@ +69,5 @@
> + return hr;
> + }
> +
> + BOOL exists;
> + nsAutoTArray<WCHAR,32> faceName;
should we use wchar_t here? also, should we maybe use std::vector instead of nsAutoTArray? I was under the impression that was preferred nowawadays but I might be wrong.
@@ +85,5 @@
> + hr = names->GetStringLength(englishIdx, &length);
> + if (FAILED(hr)) {
> + return hr;
> + }
> + if (!faceName.SetLength(length + 1)) {
Isn't nsAutoTArray infallible?
@@ +139,5 @@
> + hr = infostrings->GetStringLength(englishIdx, &length);
> + if (FAILED(hr)) {
> + return hr;
> + }
> + if (!faceName.SetLength(length + 1)) {
idem
Attachment #8345159 -
Flags: review?(bas) → review+
Comment 8•11 years ago
|
||
Comment on attachment 8345163 [details] [diff] [review]
patch, log font name loading times
Review of attachment 8345163 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/thebes/gfxPlatformFontList.cpp
@@ +224,5 @@
> mFontFamilies.Enumerate(gfxPlatformFontList::InitFaceNameListsProc, this);
> +
> + TimeStamp end = TimeStamp::Now();
> + Telemetry::AccumulateTimeDelta(Telemetry::FONTLIST_INITFACENAMELISTS,
> + start, end);
nit: indent
Attachment #8345163 -
Flags: review?(bas) → review+
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to John Daggett (:jtd) from comment #6)
> Running with a single browser window configured to open up with a testcase
> that uses src local:
>
> With GDI (gfx.direct2d disabled, direct table reads):
> cold: 2863ms
> warm: 113ms
>
> With DirectWrite and patch:
> cold: 34ms
> warm: 3ms
>
> Where "cold" is first time after system restart, and "warm" is the next run.
Tryserver build with only logging enabled to measure existing performance:
Existing DirectWrite implementation:
cold: 3183ms
warm: 126ms
Assignee | ||
Comment 10•11 years ago
|
||
Pushed to inbound, with changes based on review comments:
https://hg.mozilla.org/integration/mozilla-inbound/rev/775dfb379e04
https://hg.mozilla.org/integration/mozilla-inbound/rev/75c565fcfa50
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/775dfb379e04
https://hg.mozilla.org/mozilla-central/rev/75c565fcfa50
Assignee: nobody → jdaggett
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 12•11 years ago
|
||
I get compiler errors in gfxPlatformFontList.cpp with logging disabled in configure. The LOG_FONTINIT_ENABLED lines might need to be inside #ifdef PR_LOGGING.
Updated•11 years ago
|
Attachment #8346245 -
Flags: review?(cam) → review+
Assignee | ||
Comment 14•11 years ago
|
||
Pushed follow-up patch to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec5e10f50740
Comment 15•11 years ago
|
||
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to Brad Jackson from comment #12)
> I get compiler errors in gfxPlatformFontList.cpp with logging disabled in
> configure. The LOG_FONTINIT_ENABLED lines might need to be inside #ifdef
> PR_LOGGING.
Just to confirm, does the follow-up patch solve your compile problems?
Comment 17•11 years ago
|
||
Yes, the patch had the same fixes as the temporary changes I put into my local source tree to fix my build.
Updated•11 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•