1270640 - Clean up comments regarding OS X LCD Font Smoothing and Hinting

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Mason Chang [Inactive] [:mchang]]

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.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: hinting.patch

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Fixup comments

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.

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Reftest fuzzing

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

Comment 5

[Mason Chang [Inactive] [:mchang]]

Try looks good - https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e1a60ac2deb

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10c89c139959
https://hg.mozilla.org/integration/mozilla-inbound/rev/8873ad2e976e

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10c89c139959
https://hg.mozilla.org/mozilla-central/rev/8873ad2e976e