Bug 215059 – [Cocoa] Migrate from CTFontTransformGlyphsWithLanguage() to CTFontShapeGlyphs()

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Myles C. Maxfield]

[Cocoa] Migrate from CTFontTransformGlyphsWithLanguage() to CTFontShapeGlyphs()

[Myles C. Maxfield]

Created attachment 405781 [details]
WIP

[Myles C. Maxfield]

Created attachment 405906 [details]
WIP

[Myles C. Maxfield]

Regressions: Unexpected image-only failures (8)
  fast/ruby/ruby-expansion-cjk-2.html [ ImageOnlyFailure ]
  fast/ruby/ruby-expansion-cjk-4.html [ ImageOnlyFailure ]
  fast/ruby/ruby-expansion-cjk.html [ ImageOnlyFailure ]
  fast/text/isolate-ignore.html [ ImageOnlyFailure ]
  fast/text/ruby-justification-flush.html [ ImageOnlyFailure ]
  fast/text/soft-hyphen-min-preferred-width.html [ ImageOnlyFailure ]
  fast/text/vertical-displacement-simple-codepath.html [ ImageOnlyFailure ]
  imported/blink/fast/ruby/ruby-first-letter.html [ ImageOnlyFailure ]

[Myles C. Maxfield]

Comment on attachment 405906 [details]
WIP

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

> Source/WebCore/platform/graphics/WidthIterator.cpp:243
>          if (font != lastFontData && width) {

the "&& width" part of this seems wrong.

[Myles C. Maxfield]

Comment on attachment 405906 [details]
WIP

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

>> Source/WebCore/platform/graphics/WidthIterator.cpp:243
>>          if (font != lastFontData && width) {
> 
> the "&& width" part of this seems wrong.

Removing that part causes these tests to fail:

Regressions: Unexpected text-only failures (3)
  fast/text/soft-hyphen-2.html [ Failure ]
  fast/text/soft-hyphen-3.html [ Failure ]
  fast/text/soft-hyphen-4.html [ Failure ]

Regressions: Unexpected image-only failures (7)
  fast/ruby/ruby-expansion-cjk-2.html [ ImageOnlyFailure ]
  fast/ruby/ruby-expansion-cjk-4.html [ ImageOnlyFailure ]
  fast/ruby/ruby-expansion-cjk.html [ ImageOnlyFailure ]
  fast/text/ruby-justification-flush.html [ ImageOnlyFailure ]
  fast/text/soft-hyphen-min-preferred-width.html [ ImageOnlyFailure ]
  fast/text/vertical-displacement-simple-codepath.html [ ImageOnlyFailure ]
  imported/blink/fast/ruby/ruby-first-letter.html [ ImageOnlyFailure ]

[Myles C. Maxfield]

Comment on attachment 405906 [details]
WIP

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

>>> Source/WebCore/platform/graphics/WidthIterator.cpp:243
>>>          if (font != lastFontData && width) {
>> 
>> the "&& width" part of this seems wrong.
> 
> Removing that part causes these tests to fail:
> 
> Regressions: Unexpected text-only failures (3)
>   fast/text/soft-hyphen-2.html [ Failure ]
>   fast/text/soft-hyphen-3.html [ Failure ]
>   fast/text/soft-hyphen-4.html [ Failure ]
> 
> Regressions: Unexpected image-only failures (7)
>   fast/ruby/ruby-expansion-cjk-2.html [ ImageOnlyFailure ]
>   fast/ruby/ruby-expansion-cjk-4.html [ ImageOnlyFailure ]
>   fast/ruby/ruby-expansion-cjk.html [ ImageOnlyFailure ]
>   fast/text/ruby-justification-flush.html [ ImageOnlyFailure ]
>   fast/text/soft-hyphen-min-preferred-width.html [ ImageOnlyFailure ]
>   fast/text/vertical-displacement-simple-codepath.html [ ImageOnlyFailure ]
>   imported/blink/fast/ruby/ruby-first-letter.html [ ImageOnlyFailure ]

The 3 new failures look like they are rebaselines.

fast/text/soft-hyphen-2.html [ Failure ]
fast/text/soft-hyphen-3.html [ Failure ]
fast/text/soft-hyphen-4.html [ Failure ]

[Myles C. Maxfield]

Comment on attachment 405906 [details]
WIP

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

> Source/WebCore/platform/graphics/WidthIterator.cpp:125
>          advances[i].setHeight(-advances[i].height());

We need to do the same thing to the origins, too.

[Myles C. Maxfield]

Created attachment 405909 [details]
WIP

[Myles C. Maxfield]

Created attachment 405912 [details]
WIP (Passes tests)

[Myles C. Maxfield]

Created attachment 405966 [details]
Patch

[Myles C. Maxfield]

Created attachment 405967 [details]
Patch

[Myles C. Maxfield]

Created attachment 405978 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 405978 [details]
Patch

I should do some performance testing before marking this as r?.

[Myles C. Maxfield]

Comment on attachment 405978 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:590
> +        glyphBuffer.offsetsInString(beginningGlyphIndex)[beginningGlyphIndex + i] += beginningStringIndex;

beginningGlyphIndex is listed twice

[Myles C. Maxfield]

Created attachment 406101 [details]
Patch

[Myles C. Maxfield]

Created attachment 406134 [details]
Patch

[Myles C. Maxfield]

Created attachment 406135 [details]
Patch

[Myles C. Maxfield]

$ compare-results -a ~/Build-Vanilla/Products/Release/plt-results-1596796412 -b ~/Build-CTFontShapeGlyphs/Products/Release/plt-results-1596797276 
a mean = 741.00884
b mean = 736.28295
pValue = 0.7559888497
(Smaller means are better.)
1.006 times better
Results ARE NOT significant

[Radar WebKit Bug Importer]

<rdar://problem/66721009>

[Darin Adler]

Comment on attachment 406135 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCascade.h:119
> +    float widthForSimpleText(StringView text, TextDirection = TextDirection::LTR /* matches default argument of TextRun constructor */) const;

Why does it matter that these defaults match?

> Source/WebCore/platform/graphics/TextRun.h:46
> +    // LTR default argument matches FontCascade::widthForSimpleText()'s default argument

Need period at end of comment.

Why does it matter that these defaults match?

> Source/WebCore/platform/graphics/TextRun.h:63
> +    // LTR default argument matches FontCascade::widthForSimpleText()'s default argument

Need period at end of comment.

Why does it matter that these defaults match?

> Source/WebCore/platform/graphics/WidthIterator.cpp:116
> +    // CTFontTransformGlyphs() operates in visual order, but WidthIterator iterates in logical order.
> +    // Temporarily put us in visual order just for the call, then put us back into logical order when
> +    // the call is done.
> +    // We don't have a global view of the entire GlyphBuffer; we're just operating on a single chunk of it.
> +    // WidthIterator encounters the chunks out in logical order, so we have to maintain that invariant.
> +    // Eventually, FontCascade::layoutSimpleText() will reverse the whole buffer to put the entire thing
> +    // in visual order, but that's okay because it has a view of the entire GlyphBuffer.
> +    // On the other hand, CTFontShapeGlyphs() accepts the buffer in logical order but returns it in physical
> +    // order, which means the second reverse() in this function still needs to execute when
> +    // CTFontShapeGlyphs() is being used.
> +#if !USE(CTFONTSHAPEGLYPHS)

This is strange. It’s cross platform code, but the comment seems to imply that all platforms use either CTFontTransformGlyphs or CTFontShapeGlyphs. Can we create some kind of cross-platform abstraction here? Typically the cross-platform code doesn’t need long comments about the specific platform-dependent functions used. In modern code could even do something like:

    if constexpr (Font::applyTransformsTakesGlyphsInVisualOrder)

Maybe it’s "CTFontTransformGlyphs and all the other functions used for this purpose on all the other platforms?"

> LayoutTests/ChangeLog:13
> +        * fast/text/soft-hyphen-2-expected.txt: Removed. Platform-specific rendertree dumps
> +        need to be in platform/ subfolders.
> +        * fast/text/soft-hyphen-3-expected.txt: Removed.
> +        * fast/text/soft-hyphen-4-expected.txt: Removed.

Can any of these tests be changed into ref tests?

[Myles C. Maxfield]

Created attachment 406263 [details]
WIP

Some tests are failing on Big Sur that aren't failing on Catalina. (EWS doesn't run on Big Sur so it didn't catch them.) I'm investigating now.

[Myles C. Maxfield]

Comment on attachment 406135 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontCascade.h:119
>> +    float widthForSimpleText(StringView text, TextDirection = TextDirection::LTR /* matches default argument of TextRun constructor */) const;
> 
> Why does it matter that these defaults match?

There are a few places in WebKit where we have things like:

if (canUseSimplePath)
    result = font.widthForSimpleText(theString);
else {
    TextRun textRun(theString);
    result = font.width(run);
}

The TextDirection argument to TextRun's constructor needs to match the TextDirection argument to widthForSimpleText(). Both arguments have default values, though.

[Myles C. Maxfield]

Created attachment 406276 [details]
Rebased

[Darin Adler]

Comment on attachment 406135 [details]
Patch

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

>>> Source/WebCore/platform/graphics/FontCascade.h:119
>>> +    float widthForSimpleText(StringView text, TextDirection = TextDirection::LTR /* matches default argument of TextRun constructor */) const;
>> 
>> Why does it matter that these defaults match?
> 
> There are a few places in WebKit where we have things like:
> 
> if (canUseSimplePath)
>     result = font.widthForSimpleText(theString);
> else {
>     TextRun textRun(theString);
>     result = font.width(run);
> }
> 
> The TextDirection argument to TextRun's constructor needs to match the TextDirection argument to widthForSimpleText(). Both arguments have default values, though.

Sure, but "left to right" being the default, while possibly culturally biased, hardly needs a comment citing some particular other function where we often use the same default, even if idiomatically they are often used side by side or interchangeably as above. The comment just doesn’t add much; not even obvious why it’s relevant from the way it’s written.

[Myles C. Maxfield]

Created attachment 406282 [details]
Rebased

[Myles C. Maxfield]

I see what's happening. The old code had a bug where, if the last character in the run had a 0 width and was a fallback font, it wouldn't get added to m_fallbackFonts.

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #26)
> I see what's happening. The old code had a bug where, if the last character
> in the run had a 0 width and was a fallback font, it wouldn't get added to
> m_fallbackFonts.

** There's a sequence of fallback characters, each of which has 0 width.

Also, this is doubly bad because the glyphIDs from the fallback characters would get shaped with the wrong font :(

[Myles C. Maxfield]

Created attachment 406289 [details]
Rebased and addressed comments

[Myles C. Maxfield]

Created attachment 406291 [details]
Rebased and addressed comments

[Myles C. Maxfield]

Created attachment 406332 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 406333 [details]
Patch for committing

[Myles C. Maxfield]

Looks like imported/blink/fast/text/international/kana-voiced-sound-marks.html passes on Catalina and Big Sur, but not on Mojave. I'll investigate.

[Myles C. Maxfield]

Created attachment 406358 [details]
Patch for committing

[Myles C. Maxfield]

Committed r265487: <https://trac.webkit.org/changeset/265487>