| Summary: | [Cocoa] Single characters don't get shaped in the fast text codepath | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Laurence Penney <lorp> | ||||||||||||||||||||||||||||
| Component: | CSS | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | arrowtypeco, changseok, dbarton, esprehn+autocc, ews-watchlist, fred.wang, glenn, jbedard, kondapallykalyan, mmaxfield, pdr, webkit-bug-importer, zalan | ||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||
| Version: | Safari 11 | ||||||||||||||||||||||||||||||
| Hardware: | Mac | ||||||||||||||||||||||||||||||
| OS: | macOS 10.13 | ||||||||||||||||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=189448 | ||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||
| Bug Blocks: | 206208, 229367 | ||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
|
Description
Laurence Penney
2018-06-19 07:35:33 PDT
*** Bug 186803 has been marked as a duplicate of this bug. *** Created attachment 343054 [details]
2D FeatureVariations conditions in Chrome: good
Created attachment 343055 [details]
2D FeatureVariations conditions in Safari: bad
Can you provide the .html file for your test? I'd like to see specifically what is going on. 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.
(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 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. 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. rvrn can't be disabled, so the fact that Chrome lets it be disabled shows a Chrome bug. ... and by "can't be disabled" I mean "is a required feature" I got a second duplicate report for this. Created attachment 343284 [details]
Second duplicate bug report
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. *** Bug 203768 has been marked as a duplicate of this bug. *** Created attachment 434271 [details]
Patch
Comment on attachment 434271 [details]
Patch
Marking patch as r- because it needs a test.
Created attachment 434274 [details]
Patch
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). Apache 2.0 Thank you so much, Lawrence! *Laurence Right, we already have abseil-cpp checked into the tree, and it is Apache 2.0. So we should be good to go. Created attachment 435318 [details]
WIP
Created attachment 435320 [details]
Patch
Created attachment 435327 [details]
Patch
The patch is passing all the tests, but I'm doing additional performance testing before this is ready to be marked as r?. 0.2% of the calls to the shaper end up calling it with a single glyph. 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. Created attachment 435950 [details]
Patch
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. 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. 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). Every font which is being asked to shape just one single glyph has shaping tables (morx, GPOS, or GSUB). 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! rdar://82195405 will help too. Created attachment 436062 [details]
Improved performance
Created attachment 436063 [details]
Improved performance
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]. *** Bug 229367 has been marked as a duplicate of this bug. *** |