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

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[John Daggett (:jtd)]

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.

Comment 1

[John Daggett (:jtd)]

Attachment: patch, use DirectWrite API methods to lookup postscript/fullnames

Comment 2

[John Daggett (:jtd)]

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.

Comment 3

[John Daggett (:jtd)]

Attachment: patch, use DirectWrite API methods to lookup postscript/fullnames

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.

Comment 4

[John Daggett (:jtd)]

Tryserver build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-39410286ab62/try-win32/

Comment 5

[John Daggett (:jtd)]

Attachment: patch, log font name loading times

We already record this via Telemetry but recording to a log is useful for diagnosing when InitFaceNames/InitOtherNames are slow.

Comment 6

[John Daggett (:jtd)]

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

[Bas Schouten (:bas.schouten)]

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

Comment 8

[Bas Schouten (:bas.schouten)]

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

Comment 9

[John Daggett (:jtd)]

(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

Comment 10

[John Daggett (:jtd)]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/775dfb379e04
https://hg.mozilla.org/mozilla-central/rev/75c565fcfa50

Comment 12

[Brad Jackson]

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.

Comment 13

[John Daggett (:jtd)]

Attachment: followup patch, bracket logging calls with PR_LOGGING #ifdef

Argh, right you are!

Comment 14

[John Daggett (:jtd)]

Pushed follow-up patch to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec5e10f50740

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec5e10f50740

Comment 16

[John Daggett (:jtd)]

(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

[Brad Jackson]

Yes, the patch had the same fixes as the temporary changes I put into my local source tree to fix my build.