Bug 186804 - [Cocoa] Single characters don't get shaped in the fast text codepath
Summary: [Cocoa] Single characters don't get shaped in the fast text codepath
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari 11
Hardware: Mac macOS 10.13
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 186803 203768 229367 (view as bug list)
Depends on:
Blocks: 206208 229367
  Show dependency treegraph
 
Reported: 2018-06-19 07:35 PDT by Laurence Penney
Modified: 2023-06-26 22:49 PDT (History)
13 users (show)

See Also:


Attachments
A TrueType font with 2 variation axes and 2-dimensional FeatureVariations (2.81 KB, application/x-font-ttf)
2018-06-19 07:35 PDT, Laurence Penney
no flags Details
2D FeatureVariations conditions in Chrome: good (199.38 KB, image/png)
2018-06-19 07:39 PDT, Laurence Penney
no flags Details
2D FeatureVariations conditions in Safari: bad (148.51 KB, image/png)
2018-06-19 07:39 PDT, Laurence Penney
no flags Details
HTML + CSS for a 2D grid representing 2 axes in the font, where FeatureVariations substitute the "u" glyph for "u.alt" in certain parts of the designspace (1.96 KB, application/zip)
2018-06-20 04:15 PDT, Laurence Penney
no flags Details
Second duplicate bug report (32.02 KB, application/zip)
2018-06-21 16:14 PDT, Myles C. Maxfield
no flags Details
Patch (5.51 KB, patch)
2021-07-26 22:34 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (5.51 KB, patch)
2021-07-26 23:23 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (8.12 KB, patch)
2021-08-10 19:53 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (12.10 KB, patch)
2021-08-10 20:32 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (13.47 KB, patch)
2021-08-11 00:48 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (15.82 KB, patch)
2021-08-20 01:23 PDT, Myles C. Maxfield
zalan: review+
Details | Formatted Diff | Diff
Improved performance (17.22 KB, patch)
2021-08-20 21:11 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Improved performance (17.58 KB, patch)
2021-08-20 21:20 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laurence Penney 2018-06-19 07:35:33 PDT
Created attachment 343053 [details]
A TrueType font with 2 variation axes and 2-dimensional FeatureVariations

FeatureVariations in variable fonts allow glyph substitution to happen based on location in the designspace, as specified by CSS font-variation-settings (and other methods of setting axis positions). The spec allows substitutions to be made in any "rectangular" region of the font's designspace, by means of sets of conditions. For example, a small rectangle within a 2-dimensional designspace can be defined by giving a min and max in axis 0 and a min and max in axis 1. In the language of the spec, this would be one ConditionSet containing two ConditionTables. From the spec:

https://docs.microsoft.com/en-us/typography/opentype/spec/chapter2#featurevariations-table
“For a given condition set, conditions are conjunctively related (boolean AND): all of the specified conditions must be met in order for the associated feature table substitution to be applied.”

As the spec states, for such multi-dimensional ConditionSets to resolve correctly, the ConditionTables must process the ConditionTables in a chain of AND relations. All of the ConditionTables need to resolve to true for the ConditionSet to resolve to true.

However, Safari seems to use a chain of OR relations, so that ConditionSets are resolveing to true too often.

This CodePen uses a test font that contains two glyphs: u and u.alt (a square looking like a missing glyph), to demonstrate the issue. When handled correctly, FeatureVariations in the font cause the "u" in parts of the 2-dimensional designspace to switch to "u.alt". The total region for the substitutions in this font are non-rectangular and non-contiguous, are are defined by multiple ConditionSets.

https://codepen.io/lorp/pen/RMmRVB

It works on Chrome, but on Safari we see no substitutions. See screengrabs and attached font.
Comment 1 Laurence Penney 2018-06-19 07:36:42 PDT
*** Bug 186803 has been marked as a duplicate of this bug. ***
Comment 2 Laurence Penney 2018-06-19 07:39:36 PDT
Created attachment 343054 [details]
2D FeatureVariations conditions in Chrome: good
Comment 3 Laurence Penney 2018-06-19 07:39:57 PDT
Created attachment 343055 [details]
2D FeatureVariations conditions in Safari: bad
Comment 4 Myles C. Maxfield 2018-06-19 22:55:20 PDT
Can you provide the .html file for your test? I'd like to see specifically what is going on.
Comment 5 Radar WebKit Bug Importer 2018-06-19 22:55:41 PDT
<rdar://problem/41277621>
Comment 6 Laurence Penney 2018-06-20 04:15:33 PDT
Created attachment 343149 [details]
HTML + CSS for a 2D grid representing 2 axes in the font, where FeatureVariations substitute the "u" glyph for "u.alt" in certain parts of the designspace

The JS simply offers a toggle for the 'rvrn' feature.
Comment 7 Myles C. Maxfield 2018-06-20 16:58:06 PDT
(In reply to Laurence Penney from comment #6)
> Created attachment 343149 [details]
> HTML + CSS for a 2D grid representing 2 axes in the font, where
> FeatureVariations substitute the "u" glyph for "u.alt" in certain parts of
> the designspace
> 
> The JS simply offers a toggle for the 'rvrn' feature.

Oh, you already provided a CodePen earlier... sorry for asking you to attach an .html file :( :( :( :( I clearly didn't read the description well enough
Comment 8 Myles C. Maxfield 2018-06-20 17:12:39 PDT
We don’t run shaping in our simple text codepath for single characters. This is because the simple text codepath is intended only for kerning and ligatures, both of which require multiple characters. If you change all the “u” characters to “uu” then it looks the right way.

If you use font-feature-settings, we will take the hint that “hey, something complicated is happening” and will use our complex text codepath instead, which does run shaping for single characters. So, if you make the style say “font-feature-settings: ‘XYZW’” (a feature the font doesn’t actually have) it will also look as expected.

When the content in the codepen says “rvrn 0” we are passing that down into CoreText, but it looks like CoreText performs the substitution even if rvrn is disabled.
Comment 9 Myles C. Maxfield 2018-06-20 17:21:23 PDT
I've filed <rdar://problem/41311572> about disabling rvrn still continues to perform substitutions. We can use this Bugzilla bug about how single characters don't perform shaping.
Comment 10 Myles C. Maxfield 2018-06-20 18:11:34 PDT
rvrn can't be disabled, so the fact that Chrome lets it be disabled shows a Chrome bug.
Comment 11 Myles C. Maxfield 2018-06-20 18:11:59 PDT
... and by "can't be disabled" I mean "is a required feature"
Comment 12 Myles C. Maxfield 2018-06-21 16:14:34 PDT
I got a second duplicate report for this.
Comment 13 Myles C. Maxfield 2018-06-21 16:14:55 PDT
Created attachment 343284 [details]
Second duplicate bug report
Comment 14 Laurence Penney 2018-06-25 04:20:40 PDT
Thanks Myles. A few months back I was trying to work out how 'rvrn' worked on other browsers. It turns out that Edge behaves similarly to Safari, performing 'rvrn' only on "non-simple" text. As in Safari, you can force execution by adding random font-feature-settings.

I was also surprised that Edge and Chrome allow 'rvrn' to be disabled using font-feature-settings, given that the spec states: "Application of the 'rvrn' feature is mandatory in implementations that support OpenType Font Variations whenever a variable font is in use." The opinion of Edge and Chrome representatives seems to be that since font-feature-settings is low-level, it should be disableable (set to 0 or off) just like other OpenType feature, even if applications should never expose such a switch to users. I found this surprising, and believe Apple made the correct choice here.
Comment 15 Myles C. Maxfield 2019-11-01 15:51:24 PDT
*** Bug 203768 has been marked as a duplicate of this bug. ***
Comment 16 Myles C. Maxfield 2021-07-26 22:34:41 PDT
Created attachment 434271 [details]
Patch
Comment 17 Myles C. Maxfield 2021-07-26 22:35:20 PDT
Comment on attachment 434271 [details]
Patch

Marking patch as r- because it needs a test.
Comment 18 Myles C. Maxfield 2021-07-26 23:23:34 PDT
Created attachment 434274 [details]
Patch
Comment 19 Myles C. Maxfield 2021-07-27 10:01:35 PDT
Laurence Penney says that this test font is licensed under Apache 2, and therefore can be committed to the WebKit repo (as a test font).
Comment 20 Myles C. Maxfield 2021-07-27 10:01:49 PDT
Apache 2.0
Comment 21 Myles C. Maxfield 2021-07-27 10:02:09 PDT
Thank you so much, Lawrence!
Comment 22 Myles C. Maxfield 2021-07-27 10:02:30 PDT
*Laurence
Comment 23 Myles C. Maxfield 2021-07-27 10:18:53 PDT
Right, we already have abseil-cpp checked into the tree, and it is Apache 2.0. So we should be good to go.
Comment 24 Myles C. Maxfield 2021-08-10 19:53:16 PDT
Created attachment 435318 [details]
WIP
Comment 25 Myles C. Maxfield 2021-08-10 20:32:29 PDT
Created attachment 435320 [details]
Patch
Comment 26 Myles C. Maxfield 2021-08-11 00:48:53 PDT
Created attachment 435327 [details]
Patch
Comment 27 Myles C. Maxfield 2021-08-13 14:55:52 PDT
The patch is passing all the tests, but I'm doing additional performance testing before this is ready to be marked as r?.
Comment 28 Myles C. Maxfield 2021-08-20 00:32:21 PDT
0.2% of the calls to the shaper end up calling it with a single glyph.
Comment 29 Myles C. Maxfield 2021-08-20 00:37:03 PDT
Given that:
1. This patch is having us call into the shaper 0.2% more often
2. The new calls are being called with a workload of 1 glyph, which is a smaller load than any of the other times the shaper is being called
3. The sum total of all shaping is a tiny fraction of all the time spent loading pages in our benchmarks

... It seems extremely unlikely that this patch would have any effect on performance.
Comment 30 Myles C. Maxfield 2021-08-20 01:23:38 PDT
Created attachment 435950 [details]
Patch
Comment 31 Myles C. Maxfield 2021-08-20 12:11:25 PDT
Oh, I measured it wrong before. The real number is 26%, not 0.2%. So it looks like there's more performance investigation that needs to be done.
Comment 32 Myles C. Maxfield 2021-08-20 16:02:21 PDT
I recorded all the shaping calls in our loading benchmark (https://bugs.webkit.org/show_bug.cgi?id=207243) and found that this patch causes an additional 45 milliseconds of time spent shaping, across 13 pages, for an average of an additional 3.5 milliseconds per page. This seems like it could definitely cause a significant (1-2%) regression in the test.
Comment 33 Myles C. Maxfield 2021-08-20 18:20:52 PDT
It looks like, of the shaping calls called with just one glyph, only 5% actually did any work (changed the glyph or advance or whatever).
Comment 34 Myles C. Maxfield 2021-08-20 18:59:18 PDT
Every font which is being asked to shape just one single glyph has shaping tables (morx, GPOS, or GSUB).
Comment 35 Myles C. Maxfield 2021-08-20 19:33:48 PDT
Comment on attachment 435950 [details]
Patch

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

> Source/WebCore/platform/graphics/WidthIterator.cpp:-98
> -    if (!force && glyphBufferSize <= lastGlyphCount + 1)

Clearing out kCTFontShapeWithKerning if there's only one glyph will improve performance to an acceptable level. We might even be able to do better, though!
Comment 36 Myles C. Maxfield 2021-08-20 21:02:49 PDT
rdar://82195405 will help too.
Comment 37 Myles C. Maxfield 2021-08-20 21:11:00 PDT
Created attachment 436062 [details]
Improved performance
Comment 38 Myles C. Maxfield 2021-08-20 21:20:38 PDT
Created attachment 436063 [details]
Improved performance
Comment 39 EWS 2021-08-21 12:18:58 PDT
Committed r281378 (240793@main): <https://commits.webkit.org/240793@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436063 [details].
Comment 40 Myles C. Maxfield 2023-06-26 22:49:18 PDT
*** Bug 229367 has been marked as a duplicate of this bug. ***