Closed Bug 947812 Opened 6 years ago Closed 6 years ago

use DirectWrite API's for looking up font postscript/fullnames

Categories

(Core :: Graphics: Text, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: jtd, Assigned: jtd)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(3 files, 1 obsolete file)

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.
Attachment #8344527 - Flags: review? → review?(bas)
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)
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)
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)
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 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 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+
(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
https://hg.mozilla.org/mozilla-central/rev/775dfb379e04
https://hg.mozilla.org/mozilla-central/rev/75c565fcfa50
Assignee: nobody → jdaggett
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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.
Argh, right you are!
Attachment #8346245 - Flags: review?(cam)
Attachment #8346245 - Flags: review?(cam) → review+
(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?
Yes, the patch had the same fixes as the temporary changes I put into my local source tree to fix my build.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.