New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: overly thin font rendering on mojave #15007
Merged
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
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 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,168 @@ | ||
diff --git a/src/ports/SkFontHost_mac.cpp b/src/ports/SkFontHost_mac.cpp | ||
index 3cc51d43f6..c0b606c004 100644 | ||
--- a/src/ports/SkFontHost_mac.cpp | ||
+++ b/src/ports/SkFontHost_mac.cpp | ||
@@ -377,17 +377,28 @@ static constexpr const uint8_t kSpiderSymbol_ttf[] = { | ||
0x00, 0x02, 0x00, 0x00 | ||
}; | ||
|
||
+enum class SmoothBehavior { | ||
+ none, // SmoothFonts produces no effect. | ||
+ some, // SmoothFonts produces some effect, but not subpixel coverage. | ||
+ subpixel, // SmoothFonts produces some effect and provides subpixel coverage. | ||
+}; | ||
+ | ||
/** | ||
* There does not appear to be a publicly accessable API for determining if lcd | ||
* font smoothing will be applied if we request it. The main issue is that if | ||
* smoothing is applied a gamma of 2.0 will be used, if not a gamma of 1.0. | ||
*/ | ||
-static bool supports_LCD() { | ||
- static bool gSupportsLCD = []{ | ||
- uint32_t bitmap[16][16] = {}; | ||
+static SmoothBehavior smooth_behavior() { | ||
+ static SmoothBehavior gSmoothBehavior = []{ | ||
+ uint32_t noSmoothBitmap[16][16] = {}; | ||
+ uint32_t smoothBitmap[16][16] = {}; | ||
UniqueCFRef<CGColorSpaceRef> colorspace(CGColorSpaceCreateDeviceRGB()); | ||
- UniqueCFRef<CGContextRef> cgContext( | ||
- CGBitmapContextCreate(&bitmap, 16, 16, 8, 16*4, colorspace.get(), BITMAP_INFO_RGB)); | ||
+ SkUniqueCFRef<CGContextRef> noSmoothContext( | ||
+ CGBitmapContextCreate(&noSmoothBitmap, 16, 16, 8, 16*4, | ||
+ colorspace.get(), BITMAP_INFO_RGB)); | ||
+ SkUniqueCFRef<CGContextRef> smoothContext( | ||
+ CGBitmapContextCreate(&smoothBitmap, 16, 16, 8, 16*4, | ||
+ colorspace.get(), BITMAP_INFO_RGB)); | ||
|
||
UniqueCFRef<CGDataProviderRef> data( | ||
CGDataProviderCreateWithData(nullptr, kSpiderSymbol_ttf, | ||
@@ -398,31 +409,39 @@ static bool supports_LCD() { | ||
CTFontCreateWithGraphicsFont(cgFont.get(), 16, nullptr, nullptr)); | ||
SkASSERT(ctFont); | ||
|
||
- CGContextSetShouldSmoothFonts(cgContext.get(), true); | ||
- CGContextSetShouldAntialias(cgContext.get(), true); | ||
- CGContextSetTextDrawingMode(cgContext.get(), kCGTextFill); | ||
- CGContextSetGrayFillColor(cgContext.get(), 1, 1); | ||
+ CGContextSetShouldSmoothFonts(noSmoothContext.get(), false); | ||
+ CGContextSetShouldAntialias(noSmoothContext.get(), true); | ||
+ CGContextSetTextDrawingMode(noSmoothContext.get(), kCGTextFill); | ||
+ CGContextSetGrayFillColor(noSmoothContext.get(), 1, 1); | ||
+ | ||
+ CGContextSetShouldSmoothFonts(smoothContext.get(), true); | ||
+ CGContextSetShouldAntialias(smoothContext.get(), true); | ||
+ CGContextSetTextDrawingMode(smoothContext.get(), kCGTextFill); | ||
+ CGContextSetGrayFillColor(smoothContext.get(), 1, 1); | ||
CGPoint point = CGPointMake(0, 3); | ||
CGGlyph spiderGlyph = 3; | ||
- CTFontDrawGlyphs(ctFont.get(), &spiderGlyph, &point, 1, cgContext.get()); | ||
+ CTFontDrawGlyphs(ctFont.get(), &spiderGlyph, &point, 1, noSmoothContext.get()); | ||
+ CTFontDrawGlyphs(ctFont.get(), &spiderGlyph, &point, 1, smoothContext.get()); | ||
|
||
// For debugging. | ||
- //UniqueCFRef<CGImageRef> image(CGBitmapContextCreateImage(cgContext.get())); | ||
- | ||
+ //SkUniqueCFRef<CGImageRef> image(CGBitmapContextCreateImage(noSmoothContext())); | ||
+ //SkUniqueCFRef<CGImageRef> image(CGBitmapContextCreateImage(smoothContext())); | ||
+ | ||
+ SmoothBehavior smoothBehavior = SmoothBehavior::none; | ||
for (int x = 0; x < 16; ++x) { | ||
for (int y = 0; y < 16; ++y) { | ||
- uint32_t pixel = bitmap[x][y]; | ||
- uint32_t r = (pixel >> 16) & 0xFF; | ||
- uint32_t g = (pixel >> 8) & 0xFF; | ||
- uint32_t b = (pixel >> 0) & 0xFF; | ||
+ uint32_t smoothPixel = smoothBitmap[x][y]; | ||
+ uint32_t r = (smoothPixel >> 16) & 0xFF; | ||
+ uint32_t g = (smoothPixel >> 8) & 0xFF; | ||
+ uint32_t b = (smoothPixel >> 0) & 0xFF; | ||
if (r != g || r != b) { | ||
- return true; | ||
+ return SmoothBehavior::subpixel; | ||
} | ||
} | ||
} | ||
- return false; | ||
+ return smoothBehavior; | ||
}(); | ||
- return gSupportsLCD; | ||
+ return gSmoothBehavior; | ||
} | ||
|
||
class Offscreen { | ||
@@ -1005,7 +1024,7 @@ CGRGBPixel* Offscreen::getCG(const SkScalerContext_Mac& context, const SkGlyph& | ||
if (!fRGBSpace) { | ||
//It doesn't appear to matter what color space is specified. | ||
//Regular blends and antialiased text are always (s*a + d*(1-a)) | ||
- //and smoothed text is always g=2.0. | ||
+ //and subpixel antialiased text is always g=2.0. | ||
fRGBSpace.reset(CGColorSpaceCreateDeviceRGB()); | ||
} | ||
|
||
@@ -1260,7 +1279,7 @@ static constexpr uint8_t sk_pow2_table(size_t i) { | ||
* This will invert the gamma applied by CoreGraphics, so we can get linear | ||
* values. | ||
* | ||
- * CoreGraphics obscurely defaults to 2.0 as the smoothing gamma value. | ||
+ * CoreGraphics obscurely defaults to 2.0 as the subpixel coverage gamma value. | ||
* The color space used does not appear to affect this choice. | ||
*/ | ||
static constexpr auto gLinearCoverageFromCGLCDValue = SkMakeArray<256>(sk_pow2_table); | ||
@@ -1350,18 +1369,20 @@ void SkScalerContext_Mac::generateImage(const SkGlyph& glyph) { | ||
CGGlyph cgGlyph = SkTo<CGGlyph>(glyph.getGlyphID()); | ||
|
||
// FIXME: lcd smoothed un-hinted rasterization unsupported. | ||
- bool generateA8FromLCD = fRec.getHinting() != SkPaint::kNo_Hinting; | ||
+ bool requestSmooth = fRec.getHinting() != SkPaint::kNo_Hinting; | ||
|
||
// Draw the glyph | ||
size_t cgRowBytes; | ||
- CGRGBPixel* cgPixels = fOffscreen.getCG(*this, glyph, cgGlyph, &cgRowBytes, generateA8FromLCD); | ||
+ CGRGBPixel* cgPixels = fOffscreen.getCG(*this, glyph, cgGlyph, &cgRowBytes, requestSmooth); | ||
if (cgPixels == nullptr) { | ||
return; | ||
} | ||
|
||
// Fix the glyph | ||
if ((glyph.fMaskFormat == SkMask::kLCD16_Format) || | ||
- (glyph.fMaskFormat == SkMask::kA8_Format && supports_LCD() && generateA8FromLCD)) | ||
+ (glyph.fMaskFormat == SkMask::kA8_Format | ||
+ && requestSmooth | ||
+ && smooth_behavior() == SmoothBehavior::subpixel)) | ||
{ | ||
const uint8_t* linear = gLinearCoverageFromCGLCDValue.data(); | ||
|
||
@@ -2257,14 +2278,14 @@ void SkTypeface_Mac::onFilterRec(SkScalerContextRec* rec) const { | ||
|
||
rec->fFlags &= ~flagsWeDontSupport; | ||
|
||
- bool lcdSupport = supports_LCD(); | ||
+ SmoothBehavior smoothBehavior = smooth_behavior(); | ||
|
||
// Only two levels of hinting are supported. | ||
// kNo_Hinting means avoid CoreGraphics outline dilation. | ||
// kNormal_Hinting means CoreGraphics outline dilation is allowed. | ||
// If there is no lcd support, hinting (dilation) cannot be supported. | ||
SkPaint::Hinting hinting = rec->getHinting(); | ||
- if (SkPaint::kSlight_Hinting == hinting || !lcdSupport) { | ||
+ if (SkPaint::kSlight_Hinting == hinting || smoothBehavior == SmoothBehavior::none) { | ||
hinting = SkPaint::kNo_Hinting; | ||
} else if (SkPaint::kFull_Hinting == hinting) { | ||
hinting = SkPaint::kNormal_Hinting; | ||
@@ -2291,12 +2312,15 @@ void SkTypeface_Mac::onFilterRec(SkScalerContextRec* rec) const { | ||
// [LCD][yes-hint]: generate LCD using CoreGraphic's LCD output. | ||
|
||
if (rec->fMaskFormat == SkMask::kLCD16_Format) { | ||
- if (lcdSupport) { | ||
+ if (smoothBehavior == SmoothBehavior::subpixel) { | ||
//CoreGraphics creates 555 masks for smoothed text anyway. | ||
rec->fMaskFormat = SkMask::kLCD16_Format; | ||
rec->setHinting(SkPaint::kNormal_Hinting); | ||
} else { | ||
rec->fMaskFormat = SkMask::kA8_Format; | ||
+ if (smoothBehavior == SmoothBehavior::some) { | ||
+ rec->setHinting(SkPaint::kNormal_Hinting); | ||
+ } | ||
} | ||
} | ||
|
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SkUniqueCFRef
was added recently in https://skia-review.googlesource.com/c/skia/+/155612, the skia version on our master doesn't have it, it should beUniqueCFRef
in this patch.