1400721 - Skia mishandles FreeType 2.8.1 patent-free subpixel rendering

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect, P3)

Description

[Jan Alexander Steffens [:heftig]]

As noted at https://sourceforge.net/projects/freetype/files/freetype2/2.8.1/, FreeType 2.8.1 ships with a new LCD subpixel rendering technique called Harmony that evades the ClearType patents by not requiring (and supporting) filtering:

>  - By default, FreeType now offers high quality LCD-optimized
>    output without resorting to ClearType techniques of resolution
>    tripling and filtering. In this method, called Harmony, each
>    color channel is generated separately after shifting the glyph
>    outline, capitalizing on the fact that the color grids on LCD
>    panels are shifted by a third of a pixel. This output is
>    indistinguishable from ClearType with a light 3-tap filter. 

This technique is enabled by default, and the code urges packagers not to enable the (still available) patented rendering mode.


FreeType 2.8.1 also changed how the padding for LCD bitmaps is calculated:

>  - The internal representation of buffers for LCD rendering has
>    changed (to be more precise, the amount of padding gets computed
>    differently). Applications that use the FreeType API are not
>    affected.

Skia breaks FreeType's API by replacing the rendering buffer with its own. As a result, text rendering broke with some glyphs being shifted and cut off. The padding change happened before the introduction of Harmony; Skia implemented a workaround that was effective for the patented mode. The upstream bug for this is https://bugs.chromium.org/p/skia/issues/detail?id=6663. Firefox received a backport of this in bug 1393467.

Unfortunately rendering breaks again with similar results when FreeType is compiled with the patent-free mode.

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: fix Skia's glyph LCD filter padding for FreeType 2.8.1

The problem stems from the fact that FreeType 2.8.1 built without the previous LCD filtering support will report that LCD filters are unimplemented. This disables our previous workaround that added padding to the glyph to accommodate any padding the LCD filter may add.

Yet even though it reports LCD filters are unimplemented, it will still add padding now because of the new patent-free LCD filtering that is always used.

The simple solution here is to make the check for adding the padding less specific. If we're doing subpixel AA on the glyph, then always try to pad it. This is less brittle and more future-proof than trying to do a version check. Bug 1393467 made it safe to do this as if for any reason the mask renders with less or no padding, things will get clipped appropriately.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a26478b9034
fix Skia's glyph LCD filter padding for FreeType 2.8.1. r=jrmuizel

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a26478b9034

Comment 4

[Lee Salzman [:lsalzman]]

Comment on attachment 8909448 [details] [diff] [review]
fix Skia's glyph LCD filter padding for FreeType 2.8.1

Approval Request Comment
[Feature/Bug causing the regression]: Preexisting condition affecting all releases.
[User impact if declined]: Broken subpixel anti-aliasing of fonts with newer versions of FreeType on Linux.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes, verified by reporter
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: Just expands the scope of the upstream Skia fix we applied in bug 1393467 to work in this new scenario.
[String changes made/needed]: none

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8909448 [details] [diff] [review]
fix Skia's glyph LCD filter padding for FreeType 2.8.1

56 is on mozilla-release now.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8909448 [details] [diff] [review]
fix Skia's glyph LCD filter padding for FreeType 2.8.1

Fix for upcoming version of FreeType, looks pretty safe. Thanks for the explanation in the patch.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/8fe8227e5080

Comment 8

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8909448 [details] [diff] [review]
fix Skia's glyph LCD filter padding for FreeType 2.8.1

I guess it makes sense to get this in esr52 for distros that ship the latest freetype.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/285cde3988335103bd8d60cc09d6fa36db3c4d78

Comment 10

[Andrei Vaida [:avaida]]

(In reply to Lee Salzman [:lsalzman] from comment #4)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes, verified by reporter
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Lee's assessment on manual testing needs and the fact that this fix has automated coverage.