Closed
Bug 1270640
Opened 9 years ago
Closed 9 years ago
Clean up comments regarding OS X LCD Font Smoothing and Hinting
Categories
(Core :: Graphics: Text, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: mchang, Assigned: mchang)
Details
Attachments
(2 files, 1 obsolete file)
2.97 KB,
patch
|
mchang
:
review+
|
Details | Diff | Splinter Review |
33.09 KB,
patch
|
lsalzman
:
review+
|
Details | Diff | Splinter Review |
The old comment was also wrong. Skia basically always tries to enable LCD font smoothing unless explicitly disabled now. The only time we want to actually explicitly disable it is if we have explicitly requested grayscale AA (and not fallen back to it because of a transparent surface). In this case, we want grayscale AA and hinting disabled so that LCD font smoothing is disabled.
In every other case, we want normal hinting. With the default glyph AA options, this will still enable subpixel AA text and LCD font smoothing. In the case where we fallback to grayscale AA due to a transparent background, we'll still be requesting the default AA options with normal hinting. This will still enable LCD font smoothing, but we'll generate an A8 from LCD.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8749410 -
Flags: review?(bugmail.mozilla)
Comment 2•9 years ago
|
||
Comment on attachment 8749410 [details] [diff] [review]
hinting.patch
Review of attachment 8749410 [details] [diff] [review]:
-----------------------------------------------------------------
r+ if you steal my suggested text (assuming I understood what's going on correctly).
::: gfx/2d/DrawTargetSkia.cpp
@@ +640,5 @@
>
> if (aFont->GetType() == FontType::MAC &&
> + aOptions.mAntialiasMode == AntialiasMode::GRAY) {
> + // Normally, Skia enables CGFontSmoothing which creates thicker fonts
> + // and also enables subpixel AA. However, with explicit Grayscale AA,
After "Grayscale AA", insert "(from -moz-osx-font-smoothing:grayscale)"
@@ +644,5 @@
> + // and also enables subpixel AA. However, with explicit Grayscale AA,
> + // we want to disable CGFontSmoothing. To disable font smoothing,
> + // we also have to explicitly disable hinting.
> + // Otherwise, skia will still create grayscale AA fonts but with
> + // LCD font smoothing enabled.
So is the semantics of -moz-osx-font-smoothing:grayscale to not have either CGFontSmoothing or LCD font smoothing? In other words, what's wrong with creating grayscale AA fonts but with LCD font smoothing enabled? From the CSS property name it sounds like the only requirement is that it use grayscale AA, and it should be OK if that comes with LCD font smoothing. If the semantics of the CSS property is to disable all smoothing, that should be a bit more clear, like so:
// Normally, Skia enables CGFontSmoothing which creates thicker fonts
// and also enables subpixel AA. Skia can also disable CGFontSmoothing,
// in which case it falls back to thinner fonts with grayscale AA, but
// also turns on LCD font smoothing.
//
// With explicit Grayscale AA (from -moz-osx-font-smoothing:grayscale),
// we want to have grayscale AA with no smoothing at all. This means
// disabling CGFontSmoothing, and also disabling the LCD font smoothing
// behaviour. To accomplish this we have to explicitly disable hinting,
// in addition to leaving mAntialiasMode as GRAY.
Does that sound good?
Attachment #8749410 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 3•9 years ago
|
||
Carrying r+, updated to only set hinting to off if we explicitly get grayscale AA from layout. Subpixel AA and Font Smoothing are still enabled even with hinting set to normal in skia.
Attachment #8749410 -
Attachment is obsolete: true
Attachment #8750436 -
Flags: review+
Assignee | ||
Comment 4•9 years ago
|
||
We had some reftest failures with part 1 due to always disabling hinting for subpixel AA text as well as grayscale AA text. The previous comment is on longer true [3] where skia would only enable subpixel AA with hinting disabled. Skia now actually inverts our hinting and forces normal hinting [1]. To properly enable subpixel AA text with skia, we actually REQUIRE normal hinting, not no hinting.
For these tests, we'd still disable hinting because layout requested subpixel AA. However, Skia would not force normal hinting in this case because the glyph is too big [2] for subpixel AA and instead set the glyph mask to A8. Because the A8 glyph mask was chosen, and DrawTargetSkia set hinting to off, Skia would actually render the text like -moz-osx-font-smoothing: grayscale where we requested LCD font smoothing off (and hence thinner font), thus the test would pass. The test is failing now because we did not disable hinting, and instead we got LCD font smoothing but the mask converted to grayscale AA. I think this is actually the proper behavior as we generally want LCD font smoothing for all fonts, even if Skia can't render it, unless layout explicitly asks for grayscale AA. In this case, layout asked for subpixel AA, but our hinting disabled font smoothing. Thus, we should actually leave it enabled and fuzz the tests.
[1] https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/ports/SkFontHost_mac.cpp#2017
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/core/SkPaint.cpp?from=SkPaint.cpp#1374
[3] https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/ports/SkFontHost_mac.cpp?from=SkFontHost_mac.cpp#2011
Attachment #8750437 -
Flags: review?(lsalzman)
Updated•9 years ago
|
Attachment #8750437 -
Flags: review?(lsalzman) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Try looks good - https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e1a60ac2deb
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/10c89c139959
https://hg.mozilla.org/integration/mozilla-inbound/rev/8873ad2e976e
Keywords: checkin-needed
Comment 7•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/10c89c139959
https://hg.mozilla.org/mozilla-central/rev/8873ad2e976e
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•