This repository has been archived by the owner on Aug 8, 2023. It is now read-only.
Allow specifying multiple fonts or font families for local font rendering #16253
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Mar 1, 2020
|
A relevant test is crashing on macOS on CI: Locally, it doesn’t crash, but I do get a failure: (To build mbgl-test-runner as part of macOS.xcworkspace in mapbox-gl-native-ios, I had to modify that target’s |
|
|
|
At least on my machine, the failure appears to be related to the specific weight that Core Text has chosen:
|
|
I’m guessing the new choice of PingFangTC-Regular is correct, based on the expected Qt rendering. The old code seems to have chosen PingFangTC-Thin by default. The Regular weight more closely matches the weight of the Latin text too.
|
This was referenced Mar 2, 2020
|
|
Here’s the crash trace from the CI-only crash in #16253 (comment): TFontCascade::CreateFallback(__CTFont const*, __CFString const*, CTEmojiPolicy) const + 1194To grab that crash trace, SSH into the build bot and: cd project/build/
./mbgl-test-runner --gtest_filter=LocalGlyphRasterizer.*
cat ~/Library/Logs/DiagnosticReports/mbgl-test-runner_2020-03-02-145929_Distillers-Mac-4.crash |
|
The PingFang font families are installed on the build machine at /System/Library/Fonts/PingFang.ttc, and no font is disabled: I seem to have no problem running this equivalent Swift code in interactive mode, so I wonder if there’s a memory management problem that only surfaces on the build machine for some reason. import Foundation
import CoreText
let attributes = [
kCTFontSizeAttribute: 24,
kCTFontFamilyNameAttribute: "PingFang",
] as [CFString : Any]
let desc = CTFontDescriptorCreateWithAttributes(attributes as CFDictionary)
let font = CTFontCreateWithFontDescriptor(desc, 0, nil)
let attrs = [kCTFontAttributeName: font]
let attrStr = CFAttributedStringCreate(kCFAllocatorDefault, "\u{8EAB}\u{4EC0}\u{6230}\u{30A2}\u{3042}1" as CFString, attrs as CFDictionary)
print(CTLineCreateWithAttributedString(attrStr!)) |
|
The iOS failure is similar to the macOS failure (not the crash) in #16253 (comment): <testcase name="PingFang" status="run" time="0.056" classname="LocalGlyphRasterizer">
<failure message="/Users/distiller/project/test/src/mbgl/test/util.cpp:49 Expected: (pixels / (expected.size.width * expected.size.height)) <= (imageThreshold), actual: 0.015213 vs 0.015" type="">
/Users/distiller/project/test/src/mbgl/test/util.cpp:49 Expected: (pixels / (expected.size.width * expected.size.height)) <= (imageThreshold), actual: 0.015213 vs 0.015
</failure>
</testcase>The test was passing on iOS before 305df25, so the previous fixture was correct for iOS. |
|
I’m unable to reproduce the
The test passes with the modified test fixture, unlike on CI: |
This comment has been minimized.
This comment has been minimized.
09c809f to
3d1ad67
Compare
|
e8384c2 gets glyph metrics from the font. It’s a mixture of parts of coretext-hack (#7862 (comment)) and the code removed in 155804b, but using the correct Core Text APIs so there aren’t any gaps in glyph metrics. Some rendering tests currently fail with these changes. For the most part, I think the new rendering is superior in terms of making the baseline more consistent with mixed CJK/Latin text and improving kerning of Hangul. (The Hangul fonts that ship with Apple platforms don’t have perfectly square glyphs.) However, vertical text isn’t centered correctly. The metrics need to be reinterpreted for vertical text, which will require changes to the platform-agnostic interface. As well, Latin followed by CJK has too little kerning, probably because of an inaccurate advance metric coming from non-locally rasterized glyphs.
I think these changes might help to address some of the suboptimal placement reported in mapbox/mapbox-gl-js#9134. |
22ab715 to
603e650
Compare
1ec5
commented
Apr 13, 2020
There was a problem hiding this comment.
For those following along, this PR currently implements the elegant approach described in mapbox/mapbox-gl-native-ios#105 (comment), not the more comprehensive but more convoluted approach in mapbox/mapbox-gl-native-ios#105 (comment). Either way, the new LocalGlyphRasterizer implementation is sufficiently generic that we’re two steps away from extending LocalGlyphRasterizer support to all scripts, not just monospaced CJK.
However, including the font stack in the font cascade list could have performance implementations, so we may need to introduce optimizations like font descriptor caching. If polishing the font stack–aware implementation drags on, we can split out 93809da into a separate PR and first land a non–font stack–aware implementation.
| { | ||
| fallbackFontNames = [[NSUserDefaults standardUserDefaults] stringArrayForKey:@"MGLIdeographicFontFamilyName"]; | ||
| if (fontFamily_) { | ||
| fallbackFontNames = [fallbackFontNames ?: @[] arrayByAddingObjectsFromArray:[@(fontFamily_->c_str()) componentsSeparatedByString:@"\n"]]; |
There was a problem hiding this comment.
The primary thing I’m looking for feedback on is what to do about this single-string argument to the LocalGlyphRasterizer constructor. My understanding is that other platforms like Android need to select and validate a single font at the SDK level, whereas iOS/macOS has outgrown that approach and needs to maintain a whole list of fonts that can only be validated by the system on the fly.
Should we change the signature of this constructor across all platforms to allow for a std::vector of font names? Alternatively, would it be OK for the iOS/macOS implementation to ignore the single-string name or treat it as just one more fallback (as currently implemented in this PR)?
There was a problem hiding this comment.
I prefer the idea of using std::vector here. Though could we land this PR as is and address that later?
There was a problem hiding this comment.
Perhaps – it just feels wrong to smuggle a newline-delimited list through a single-string API to split it apart on the other end.
There was a problem hiding this comment.
The primary thing I’m looking for feedback on is what to do about this single-string argument to the LocalGlyphRasterizer constructor.
Should we change the signature of this constructor across all platforms to allow for a std::vector of font names? Alternatively, would it be OK for the iOS/macOS implementation to ignore the single-string name or treat it as just one more fallback (as currently implemented in this PR)?
@alexshalamov I think you're the most familiar with this part of the code. Is there any reason GL Native can't accept a whole list of fonts here?
If we agree to make this change at the core level, I also think that it would be better in a separate PR.
There was a problem hiding this comment.
I had been trying to minimize changes to the cross-platform interface, but this constructor is only called from platform-specific code, so the cross-platform code in mbgl doesn’t particularly care about the signature. If folks are OK with a little additional churn, it would be great to change this method to take a std::optional<std::vector<std::string>>, with nullptr disabling local glyph rasterization and an empty vector using the system’s default fallback.
| } | ||
|
|
||
| private: | ||
| optional<std::string> fontFamily; | ||
| CTFontRef fontHandle; |
There was a problem hiding this comment.
This PR no longer memoizes the resolved font. The font cascade list starts with the font stack from the style, which means the font descriptor can potentially differ for every invocation of LocalGlyphRasterizer::rasterizeGlyph(). If performance turns out to be an issue, this class could build up a cache mapping FontStacks to CTFontDescriptorRefs.
There was a problem hiding this comment.
Yep, let's check the performance after this PR.
| @@ -96,14 +122,30 @@ CTFontRef getFont() { | |||
| {} | |||
|
|
|||
| bool LocalGlyphRasterizer::canRasterizeGlyph(const FontStack&, GlyphID glyphID) { | |||
| return util::i18n::allowsFixedWidthGlyphGeneration(glyphID) && impl->getFont(); | |||
| return util::i18n::allowsFixedWidthGlyphGeneration(glyphID) && impl->isEnabled(); | |||
There was a problem hiding this comment.
This diff is a bit hard to follow. Before, glyphs within the limited CJK ranges would be rasterized locally unless no local font family was specified (disabling the feature). In the case where getFont() failed to create a CTFontRef, it would throw an exception, not return false. Moreover, that case would never occur, because CTFontCreateWithFontDescriptor() always returns a best-match font (Helvetica as a last resort fallback).
The new code makes the disabled case more explicit.
|
|
||
| // Start drawing a little bit below the top of the bitmap | ||
| CGContextSetTextPosition(*context, 0.0, 5.0); | ||
| CGContextSetTextPosition(*context, 0.0, descent); | ||
| CTLineDraw(*line, *context); |
There was a problem hiding this comment.
This new code is pretty robust, even doing away with the hard-coded left, top, and advance metrics that were in the old code. But it falls short of the coretext-hack branch (#7862 (comment)) in that individual characters are still being rasterized in isolation. If we were to enable local glyph rasterizing for all scripts today, that would cause some combining characters to be rendered with unsightly spacing dotted circles (◌) underneath.
coretext-hack didn’t have these artifacts. I think it’s because CTFontDrawGlyphs() already knows the glyphs will be reassembled into a line, whereas CTLineDraw() assumes the glyphs have already been reassembled. I think replacing CTLineDraw() with CTFontDrawGlyphs() would do the trick. Ideally, though, making GlyphManager::generateLocalSDF() operate on an entire string, not individual glyphs, would not only address the dotted circle problem but also weaken our incorrect reliance on a one-to-one mapping between characters and glyphs.
There was a problem hiding this comment.
Using CTFontDrawGlyphs() resulted in the same rendered output, including the dotted circles. It’s definitely because we’re rendering one character at a time.
The coretext-hack branch’s approach does require rendering one glyph at a time using CTFontDrawGlyphs(), but getGlyphDependencies() calculates those glyph IDs based on an entire run of text ahead of time. In fact, that branch redefines GlyphID to pair the numeric glyph ID with a font name, since a given codepoint can be represented by a different glyph depending on the font.
Essentially, we need this code that currently conflates codepoints with glyphs to ask a platform-specific implementation to perform shaping and map these codepoints to actual glyph IDs, and the font name is important for preventing collisions between different glyphs in different fonts that happen to have the same ID:
mapbox-gl-native/src/mbgl/layout/symbol_layout.cpp
Lines 179 to 182 in 66ac554
I can’t think of a less invasive way to solve the combining character issue, because shaping happens so far from glyph rendering in mbgl’s current architecture. The font name could be optional for platforms that don’t perform shaping, but the diff will still be large. Clearly, that refactoring will need to take place in a separate PR.
julianrex
approved these changes
Apr 14, 2020
| { | ||
| fallbackFontNames = [[NSUserDefaults standardUserDefaults] stringArrayForKey:@"MGLIdeographicFontFamilyName"]; | ||
| if (fontFamily_) { | ||
| fallbackFontNames = [fallbackFontNames ?: @[] arrayByAddingObjectsFromArray:[@(fontFamily_->c_str()) componentsSeparatedByString:@"\n"]]; |
There was a problem hiding this comment.
I prefer the idea of using std::vector here. Though could we land this PR as is and address that later?
| } | ||
|
|
||
| private: | ||
| optional<std::string> fontFamily; | ||
| CTFontRef fontHandle; |
There was a problem hiding this comment.
Yep, let's check the performance after this PR.
603e650 to
12d530a
Compare
20c5fb3 to
1281b9f
Compare
1ec5
commented
Apr 22, 2020
|
Finally figured out the macOS crash. The gory details are in #16253 (comment). 5ed0d85 fixes the crash by changing how the font descriptor’s cascade list is built. Previously, the main font descriptor was created by copying the first font descriptor in what would become the main font descriptor’s cascade list, effectively making the same font appear twice in the cascade list. Now each font appears only as many times as it appears in the font stack or I don’t know for sure why it only occurs on the build machine. I can’t reproduce the issue on MacBook Pros running either macOS 10.13.6 or 10.15.4, but the build machine happens to be running macOS 10.14.4. |
5ed0d85 to
b905aef
Compare
|
After passing for a while, |
😢 Does this test win some kind of award for flakiness? |
|
@1ec5 I checked failures for ios unit runners, and the diff is quite small. If you think rendering result is good enough, let's bump threshold to reasonable value that would satisfy ios / macos tests. What do you think?
|
|
Looking at those differences, I think iOS is choosing PingFangTC-Thin instead of PingFangTC-Regular, but I can’t reproduce the issue in the simulator, maybe because it’s using a macOS build of Core Text under the hood or something. (I’ve been unable to run the tests on a device due to #16259 (comment).) |
alexshalamov
approved these changes
Apr 23, 2020
|
So far we’ve been able to verify on a few devices:
To run on the device:
|
|
307fba5 relaxes the criteria for matching a font, which should fix the failures we’ve been seeing on iOS. The following screenshots are from an iPhone 8 running iOS 13.4 and iosapp with the
mbgl is also capable of displaying downloadable fonts. Hiragino Sans W7 doesn’t come preinstalled on iOS, but it comes from Pages as a downloadable font:
|
|
That change makes the difference grow even more in the failing test on CI: |
|
In mapbox/mapbox-gl-native-ios#189 (comment), I’ve added a snapshot test in Objective-C powered by MGLMapSnapshotter as an alternative to the C++ test in mbgl that’s much easier to test on a device. To run it:
As it happens, this snapshot test fails, tripping this assertion, even in an iPhone 8 / iOS 13.4 simulator:
To inspect the results:
|
|
In the end, I could find no Core Text object model differences between iOS and macOS, yet each glyph run was being rendered more faintly on iOS than on macOS. What we’re seeing here seems to reproduce some of the observations in this Medium post. Like UITextView, we’re relying on an NSAttributedString to draw itself. For now, I’ve increased the tolerance on the test to 0.0161, as suggested in #16253 (comment), and also disabled the snapshot test on iOS. Although it’s unfortunate that iOS renders the same text differently than other platforms, including macOS, it can still render a noticeable distinction between various font weights, as seen in #16253 (comment). |
d45d344 to
ddd39fb
Compare
PingFangTC-Regular is better than PingFangTC-Thin. There is no font by the name “PingFang”.
…al font rendering mbgl::Renderer and mbgl::MapSnapshotter can now contain a list of font family names, font display names, and font PostScript names, each name separated by a newline.
Take font family names from user defaults before falling back to font family names in Info.plist.
Prefer local fonts that match the names specified in the font stack (from the text-font layout property), except for the last resort fonts that mbgl hard-codes. Fall back to the list of fallback CJK fonts in user defaults, then the fonts passed in through the platform-agnostic interface (that come from Info.plist). Explicitly use the first font descriptor in the cascade list instead of the system default of Helvetica. Since the font stack can vary from one rasterization operation to the next, avoid caching the resolved font for now. Removed null checks that are unrealistic given the Core Text API contract.
A font descriptor should not be a fallback for itself.
Get glyph metrics from the font in the process of drawing each glyph into a bitmap context. These metrics result in more accurate kerning and better aligned baselines than the previous hard-coded values. Align iOS/macOS local glyph rasterization test fixture to Qt.
Co-authored-by: Alexander Shalamov <alexander.shalamov@mapbox.com>
ddd39fb to
3d3b021
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By default, on iOS and macOS, CJK characters are now set in the font specified by the
text-fontlayout property. If the named font is not installed on the device or bundled with the application, the characters are set in one of the fallback fonts passed into thelocalFontFamilyparameter ofmbgl::Renderer::Renderer()andmbgl::MapSnapshotter::MapSnapshotter(). This parameter can now contain a list of font family names, font display names, and font PostScript names, each name separated by a newline. These names form a cascade list that Core Text can use as fallbacks.CJK characters are now laid out according to the font, so fonts with nonsquare glyphs have the correct kerning. This also fixes an issue where the baseline for CJK characters was too low compared to non-CJK characters.
Here’s a map where
Noto Sans CJK JP Blackis specified as the local font:This PR has been developed in tandem with mapbox/mapbox-gl-native-ios#189.
The big to do list:
/cc @mapbox/gl-native @mapbox/maps-ios