Bug 203548 – [FTW] Adopt DirectWrite in place of Uniscribe

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

This patch is the first step in switching away from the GDI-based Uniscribe controller to DirectWrite.

This provides sufficient features to support web fonts handling and rendering text. Subsequent patches will build out full support for the font layout tests.

[Radar WebKit Bug Importer]

<rdar://problem/56695130>

[Brent Fulgham]

Created attachment 382153 [details]
Patch

[Fujii Hironori]

Comment on attachment 382153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382153&action=review

> Source/WebCore/platform/graphics/Font.cpp:130
> +    m_zeroWidthSpaceGlyph = zeroWidthSpaceGlyph;

You don't need to restore m_zeroWidthSpaceGlyph because there is a hack to clear m_zeroWidthSpaceGlyph if m_zeroWidthSpaceGlyph == m_spaceGlyph.
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/Font.cpp?rev=248846#L133

I think these logic should be reordered to be more comprehensible:

    if (glyphPageSpace)
        m_spaceGlyph = glyphPageSpace->glyphDataForCharacter(space).glyph;
    if (glyphPageZeroWidthSpace)
        m_zeroWidthSpaceGlyph = glyphPageZeroWidthSpace->glyphDataForCharacter(zeroWidthSpaceCharacter).glyph;
    if (m_zeroWidthSpaceGlyph == m_spaceGlyph)
        m_zeroWidthSpaceGlyph = 0;
    float width = widthForGlyph(m_spaceGlyph);
    m_spaceWidth = width;

> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:80
> +    COMPtr<IDWriteTextFormat> textFormat;

Where is 'textFormat' used?

[Fujii Hironori]

Can you implement Direct2D integration without using HFONT and LOGFONT at all?

[Fujii Hironori]

Comment on attachment 382153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382153&action=review

> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:76
> +    int localeLength = ::GetUserDefaultLocaleName(reinterpret_cast<LPWSTR>(&localeName), LOCALE_NAME_MAX_LENGTH);

GetUserDefaultLocaleName. Should it be "user default locale" rather than the locale of the text?

> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:78
> +    localeName[localeLength] = 0;

According to the spec, you don't need to zero-terminate explicitly.
https://docs.microsoft.com/en-us/windows/win32/api/winnls/nf-winnls-getuserdefaultlocalename

    wchar_t localeName[LOCALE_NAME_MAX_LENGTH];
    int localeLength = ::GetUserDefaultLocaleName(localeName, LOCALE_NAME_MAX_LENGTH);

[Fujii Hironori]

Comment on attachment 382153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382153&action=review

> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:53
> +    int localeLength = ::GetUserDefaultLocaleName(reinterpret_cast<LPWSTR>(&localeName), LOCALE_NAME_MAX_LENGTH);

I believe this unnecessary '::' prefix doesn't conform to WebKit Code Style Guidelines even though it doesn't disallow it explicitly because nobody prepend a unnecessary :: prefix to other global names, WTF, WebCore, WCHAR, UChar.
https://webkit.org/code-style-guideline/

[Fujii Hironori]

Comment on attachment 382153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382153&action=review

> Source/WebCore/platform/graphics/FontPlatformData.h:62
> +#include <wtf/text/StringImpl.h>

Do you need to include StringImpl.h here?

[Fujii Hironori]

Comment on attachment 382153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382153&action=review

> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:80
> +        hr = analyzer->GetGlyphs(text, run.length, fontPlatformData.dwFontFace(), fontPlatformData.orientation() == FontOrientation::Vertical, false,

Should the first argument be text + run.startPosition?

> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:87
> +        text += returnedCount;

Is this right? I think returnedCount is the number of glyphs, not characters. Spec says "Note that the mapping from characters to glyphs is, in general, many-to-many. " https://docs.microsoft.com/en-us/windows/win32/api/dwrite/nf-dwrite-idwritetextanalyzer-getglyphs

[Fujii Hironori]

Comment on attachment 382153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382153&action=review

> Source/WebCore/platform/graphics/win/FontPlatformDataDirect2D.cpp:46
> +        auto platformFontPair = DirectWrite::createWithPlatformFont(logfont);

you want to use C++17 here? 
auto [font, collection] = DirectWrite::createWithPlatformFont(logfont);

[Fujii Hironori]

Comment on attachment 382153 [details]
Patch

win and wincairo EWS can't compile.

[Brent Fulgham]

Comment on attachment 382153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382153&action=review

>> Source/WebCore/platform/graphics/Font.cpp:130
>> +    m_zeroWidthSpaceGlyph = zeroWidthSpaceGlyph;
> 
> You don't need to restore m_zeroWidthSpaceGlyph because there is a hack to clear m_zeroWidthSpaceGlyph if m_zeroWidthSpaceGlyph == m_spaceGlyph.
> https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/Font.cpp?rev=248846#L133
> 
> I think these logic should be reordered to be more comprehensible:
> 
>     if (glyphPageSpace)
>         m_spaceGlyph = glyphPageSpace->glyphDataForCharacter(space).glyph;
>     if (glyphPageZeroWidthSpace)
>         m_zeroWidthSpaceGlyph = glyphPageZeroWidthSpace->glyphDataForCharacter(zeroWidthSpaceCharacter).glyph;
>     if (m_zeroWidthSpaceGlyph == m_spaceGlyph)
>         m_zeroWidthSpaceGlyph = 0;
>     float width = widthForGlyph(m_spaceGlyph);
>     m_spaceWidth = width;

I was afraid of modifying the code too much, but this seems reasonable. I'll revise the patch.

>> Source/WebCore/platform/graphics/FontPlatformData.h:62
>> +#include <wtf/text/StringImpl.h>
> 
> Do you need to include StringImpl.h here?

No -- I'll remove it.

>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:76
>> +    int localeLength = ::GetUserDefaultLocaleName(reinterpret_cast<LPWSTR>(&localeName), LOCALE_NAME_MAX_LENGTH);
> 
> GetUserDefaultLocaleName. Should it be "user default locale" rather than the locale of the text?

I'm not sure. I don't see a text-specific locale in the call stack here, or on our other ports. Is there a way to get this information?

>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:80
>> +    COMPtr<IDWriteTextFormat> textFormat;
> 
> Where is 'textFormat' used?

In an older version of the patch. This is no longer needed -- I will remove it.

>> Source/WebCore/platform/graphics/win/FontPlatformDataDirect2D.cpp:46
>> +        auto platformFontPair = DirectWrite::createWithPlatformFont(logfont);
> 
> you want to use C++17 here? 
> auto [font, collection] = DirectWrite::createWithPlatformFont(logfont);

Oh! Good idea.

>> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:53
>> +    int localeLength = ::GetUserDefaultLocaleName(reinterpret_cast<LPWSTR>(&localeName), LOCALE_NAME_MAX_LENGTH);
> 
> I believe this unnecessary '::' prefix doesn't conform to WebKit Code Style Guidelines even though it doesn't disallow it explicitly because nobody prepend a unnecessary :: prefix to other global names, WTF, WebCore, WCHAR, UChar.
> https://webkit.org/code-style-guideline/

Okay. We used to do this for MS API calls, but I guess it's not really done anymore.

>> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:80
>> +        hr = analyzer->GetGlyphs(text, run.length, fontPlatformData.dwFontFace(), fontPlatformData.orientation() == FontOrientation::Vertical, false,
> 
> Should the first argument be text + run.startPosition?

Yes!

>> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:87
>> +        text += returnedCount;
> 
> Is this right? I think returnedCount is the number of glyphs, not characters. Spec says "Note that the mapping from characters to glyphs is, in general, many-to-many. " https://docs.microsoft.com/en-us/windows/win32/api/dwrite/nf-dwrite-idwritetextanalyzer-getglyphs

I think you are right. The other glyph-related counts are correct (even 'textPropertiesData', which seems to be per-glyph) but the text position is not. Your suggestion above fixes that problem.

[Brent Fulgham]

(In reply to Fujii Hironori from comment #4)
> Can you implement Direct2D integration without using HFONT and LOGFONT at
> all?

Yes, but not quite yet. There are still a number of code paths that expect HFONT/LOGFONT access.

I intend to remove them in a subsequent patch once the initial logic is in place.

[Brent Fulgham]

Created attachment 382204 [details]
Patch v2

Update with Fujii's suggestions.

[Fujii Hironori]

Comment on attachment 382204 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review

> Source/WebCore/platform/graphics/ComplexTextController.cpp:154
> +#if PLATFORM(WIN) && !USE(DIRECT2D)

This line can be removed because this is in #else of line #46 condition.

> Source/WebCore/platform/graphics/FontPlatformData.h:242
> +    COMPtr<IDWriteFontCollection1> m_dwFontCollection;

You added 'm_dwFontCollection', but not used in this patch. Remove it until it is really needed?

> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:80
> +    localeName[localeLength] = 0;

I think this line is not needed. See Comment 5.

[Fujii Hironori]

Comment on attachment 382204 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review

> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:115
> +        const UChar* currentCP = cp + run.startPosition;

This variable 'currentCP' hides outer 'currentCP'. Either should be renamed.

[Brent Fulgham]

Comment on attachment 382204 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review

>> Source/WebCore/platform/graphics/ComplexTextController.cpp:154
>> +#if PLATFORM(WIN) && !USE(DIRECT2D)
> 
> This line can be removed because this is in #else of line #46 condition.

Done.

>> Source/WebCore/platform/graphics/FontPlatformData.h:242
>> +    COMPtr<IDWriteFontCollection1> m_dwFontCollection;
> 
> You added 'm_dwFontCollection', but not used in this patch. Remove it until it is really needed?

For that matter, I don't really use faceName either, so I should remove both.

>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:80
>> +    localeName[localeLength] = 0;
> 
> I think this line is not needed. See Comment 5.

Fixed.

[Fujii Hironori]

Comment on attachment 382204 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review

> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:113
> +        Vector<DWRITE_SHAPING_GLYPH_PROPERTIES> glyphProperties(suggestedCount);

These Vector is allocating and deallocating for every runs because you put them in this loop.
I think these Vector should put outside of the loop.
Or, if you want to put them in the loop, suggestedCount should be (3 * run.length / 2 + 16) instead of (3 * length / 2 + 16).

[Fujii Hironori]

Comment on attachment 382204 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review

>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:113
>> +        Vector<DWRITE_SHAPING_GLYPH_PROPERTIES> glyphProperties(suggestedCount);
> 
> These Vector is allocating and deallocating for every runs because you put them in this loop.
> I think these Vector should put outside of the loop.
> Or, if you want to put them in the loop, suggestedCount should be (3 * run.length / 2 + 16) instead of (3 * length / 2 + 16).

Ah, you call shrink in shape. I think these vectors should not shrink for every runs.

[Fujii Hironori]

Comment on attachment 382204 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review

> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:148
> +                glyphs[clusterMap[k]] = font->spaceGlyph();

clusterMap[k] looks wrong. clusterMap takes glyph index, but k is a char index.

[Fujii Hironori]

Comment on attachment 382204 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review

> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:151
> +                    spaceCharacters[clusterMap[k]] = m_currentCharacter + k + run.startPosition;

Is 'spaceCharacters' used anywhere?

[Brent Fulgham]

Comment on attachment 382204 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review

>>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:113
>>> +        Vector<DWRITE_SHAPING_GLYPH_PROPERTIES> glyphProperties(suggestedCount);
>> 
>> These Vector is allocating and deallocating for every runs because you put them in this loop.
>> I think these Vector should put outside of the loop.
>> Or, if you want to put them in the loop, suggestedCount should be (3 * run.length / 2 + 16) instead of (3 * length / 2 + 16).
> 
> Ah, you call shrink in shape. I think these vectors should not shrink for every runs.

I'm not sure. The length of each text run varies at each step of the loop, so these buffers need adjust size too match or else the call to 'ComplexTextRun::create' will fail because it uses the 'size' of the glyphs vector inside its constructor.

I suppose I could adjust ONLY the glyphs vector, but I was trying to be consistent.

>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:115
>> +        const UChar* currentCP = cp + run.startPosition;
> 
> This variable 'currentCP' hides outer 'currentCP'. Either should be renamed.

Only the inner one is needed. And the old 'textStart' variable is unnecessary, too. Fixed.

>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:148
>> +                glyphs[clusterMap[k]] = font->spaceGlyph();
> 
> clusterMap[k] looks wrong. clusterMap takes glyph index, but k is a char index.

This comes form the existing Uniscribe logic, so I believe it is correct. The documentation in https://docs.microsoft.com/en-us/windows/win32/api/dwrite/nf-dwrite-idwritetextanalyzer-getglyphs says that clusterMap is a mapping from character positions to glyphs, so I think that is correct.

> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:166
> +        currentCP += run.length;

And this isn't needed anymore.

[Brent Fulgham]

Created attachment 382269 [details]
Patch

[Fujii Hironori]

Comment on attachment 382204 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review

>>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:148
>>> +                glyphs[clusterMap[k]] = font->spaceGlyph();
>> 
>> clusterMap[k] looks wrong. clusterMap takes glyph index, but k is a char index.
> 
> This comes form the existing Uniscribe logic, so I believe it is correct. The documentation in https://docs.microsoft.com/en-us/windows/win32/api/dwrite/nf-dwrite-idwritetextanalyzer-getglyphs says that clusterMap is a mapping from character positions to glyphs, so I think that is correct.

Oh. the length of clusterMap should be the length of text.

[Fujii Hironori]

Did you see comment 20?

[Brent Fulgham]

Comment on attachment 382204 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review

>>>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:148
>>>> +                glyphs[clusterMap[k]] = font->spaceGlyph();
>>> 
>>> clusterMap[k] looks wrong. clusterMap takes glyph index, but k is a char index.
>> 
>> This comes form the existing Uniscribe logic, so I believe it is correct. The documentation in https://docs.microsoft.com/en-us/windows/win32/api/dwrite/nf-dwrite-idwritetextanalyzer-getglyphs says that clusterMap is a mapping from character positions to glyphs, so I think that is correct.
> 
> Oh. the length of clusterMap should be the length of text.

Ah! Good point. I will fix.

>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:151
>> +                    spaceCharacters[clusterMap[k]] = m_currentCharacter + k + run.startPosition;
> 
> Is 'spaceCharacters' used anywhere?

No. I have removed it in my local build and will upload a new patch.

This was from the UniscribeController, so I might need to bring it back if I learn that it is needed once I complete other work on DirectWrite support.

[Brent Fulgham]

Created attachment 382282 [details]
Patch

[Fujii Hironori]

Comment on attachment 382282 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382282&action=review

> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:71
> +    wchar_t localeName[LOCALE_NAME_MAX_LENGTH + 1] = { };

Remove '+ 1'.

> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:118
> +    stringIndices.reserveCapacity(totalSuggestedCount);

You resize these Vectors for every runs. I think you don't need to put these outside of the loop. And, you don't need 'totalSuggestedCount'.

[Fujii Hironori]

Comment on attachment 382204 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review

>>>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:113
>>>> +        Vector<DWRITE_SHAPING_GLYPH_PROPERTIES> glyphProperties(suggestedCount);
>>> 
>>> These Vector is allocating and deallocating for every runs because you put them in this loop.
>>> I think these Vector should put outside of the loop.
>>> Or, if you want to put them in the loop, suggestedCount should be (3 * run.length / 2 + 16) instead of (3 * length / 2 + 16).
>> 
>> Ah, you call shrink in shape. I think these vectors should not shrink for every runs.
> 
> I'm not sure. The length of each text run varies at each step of the loop, so these buffers need adjust size too match or else the call to 'ComplexTextRun::create' will fail because it uses the 'size' of the glyphs vector inside its constructor.
> 
> I suppose I could adjust ONLY the glyphs vector, but I was trying to be consistent.

Sorry. you are right. My comment 17 is wrong. Those Vectors can not be reused.

[Brent Fulgham]

Comment on attachment 382282 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382282&action=review

>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:71
>> +    wchar_t localeName[LOCALE_NAME_MAX_LENGTH + 1] = { };
> 
> Remove '+ 1'.

Will do.

>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:118
>> +    stringIndices.reserveCapacity(totalSuggestedCount);
> 
> You resize these Vectors for every runs. I think you don't need to put these outside of the loop. And, you don't need 'totalSuggestedCount'.

This reserves sufficient storage for the largest possible run inside the loop. The 'resize' and 'shrink' operations just adjust the size of the vector, without changing the actual memory allocation. So I think this is correct as-is.

[Fujii Hironori]

Comment on attachment 382282 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382282&action=review

> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:175
> +        m_complexTextRuns.append(ComplexTextRun::create(advances, origins, glyphs, stringIndices, FloatSize(), *font, currentCP, 0, run.length, 0, run.length, m_run.ltr()));

If you reuse Vectors, all ComplexTextRun shares same Vectors for advances, origins, glyphs, stringIndices.

[Brent Fulgham]

Comment on attachment 382282 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382282&action=review

>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:175
>> +        m_complexTextRuns.append(ComplexTextRun::create(advances, origins, glyphs, stringIndices, FloatSize(), *font, currentCP, 0, run.length, 0, run.length, m_run.ltr()));
> 
> If you reuse Vectors, all ComplexTextRun shares same Vectors for advances, origins, glyphs, stringIndices.

I believe the constructor for ComplextTextRun makes a copy of the vectors it receives, so I think each will be distinct.

[Fujii Hironori]

Comment on attachment 382282 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382282&action=review

>>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:175
>>> +        m_complexTextRuns.append(ComplexTextRun::create(advances, origins, glyphs, stringIndices, FloatSize(), *font, currentCP, 0, run.length, 0, run.length, m_run.ltr()));
>> 
>> If you reuse Vectors, all ComplexTextRun shares same Vectors for advances, origins, glyphs, stringIndices.
> 
> I believe the constructor for ComplextTextRun makes a copy of the vectors it receives, so I think each will be distinct.

Ah. You are right. Sorry for the noise.

[Brent Fulgham]

Created attachment 382523 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 382523 [details]
Patch for landing

Clearing flags on attachment: 382523

Committed r251900: <https://trac.webkit.org/changeset/251900>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.