1287552 - SkFontHost_cairo does not properly interact with Fontconfig

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Lee Salzman [:lsalzman]]

Our handling of fontconfig options in SkFontHost_cairo does not match up with cairo-ft's, causing various incompatibilities that make it unsuitable for Skia content and possible regressions for Skia canvas relative to Cairo canvas.

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: Bug 1287552 - part 1 - backport upstream Cairo fix for computing transform of bitmap fonts.

Review commit: https://reviewboard.mozilla.org/r/65028/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65028/

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: Bug 1287552 - part 2 - fix transformed bitmaps with Skia and FreeType.

Review commit: https://reviewboard.mozilla.org/r/65030/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65030/

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: Bug 1287552 - part 3 - revise SkFontHost_cairo to properly parse Fontconfig options.

Review commit: https://reviewboard.mozilla.org/r/65032/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65032/

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: Bug 1287552 - part 4 - add ScaledFontFontconfig to remember generating FcPattern.

Review commit: https://reviewboard.mozilla.org/r/65034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65034/

Comment 5

[Lee Salzman [:lsalzman]]

Comment on attachment 8772132 [details]
Bug 1287552 - part 4 - add ScaledFontFontconfig to remember generating FcPattern.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65034/diff/1-2/

Comment 6

[Lee Salzman [:lsalzman]]

(In reply to Lee Salzman [:lsalzman] from comment #2)
> Created attachment 8772130 [details]
> Bug 1287552 - part 2 - fix transformed bitmaps with Skia and FreeType.
> 
> Review commit: https://reviewboard.mozilla.org/r/65030/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/65030/

The main problem this is solving is that FT_Set_Transform does not affect bitmap glyphs at all. It only works on outline glyphs, and some metrics like the advance (even if the glyph is a bitmap). So the code needs to make sure to transform the bitmap's dimensions as well as transform the actual rendered bitmap.

Comment 7

[Lee Salzman [:lsalzman]]

(In reply to Lee Salzman [:lsalzman] from comment #3)
> Created attachment 8772131 [details]
> Bug 1287552 - part 3 - revise SkFontHost_cairo to properly parse Fontconfig
> options.
> 
> Review commit: https://reviewboard.mozilla.org/r/65032/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/65032/

The overall structure of this is to preserve the FcPattern until it actually arrives at the SkScalerContext. The reason we need to do this is because we need to set the "pixelsize" field in the FcPattern before we finally resolve it, so that Fontconfig can use this to influence its matching. This is required for full compatibility with cairo-ft and Fontconfig in general.

Other snags are that because FT_Set_Transform does not have an accompanying FT_Get_Transform or similar call, we have to make sure to compute our own shape transform in a compatible way with what cairo-ft does, rather than rely upon Skia's code for this.

Support for vertical layout and emboldening options of Fontconfig were added.

Skia also did not have any support for setting the lcd filter, so that had to be massaged in as well.

Most of the hell with GlyphRenderingOptionsCairo has just been removed, and we let the font host override most options with Fontconfig's. The input options to the SkPaint only affect this in small ways, like allowing hinting or antialiasing to be disabled.

Also, for Android, since autohinting being forced off is a global setting, rather than try to pass this in every time via GlyphRenderingOptionsCairo, I just changed this to an initialization parameter to allow GlyphRenderingOptionsCairo to completely die. 

Also simplified the SkCreateTypefaceFromCairoFont entry points to avoid locking the cairo_scaled_font_t when necessary, as that can be somewhat expensive.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8772129 [details]
Bug 1287552 - part 1 - backport upstream Cairo fix for computing transform of bitmap fonts.

https://reviewboard.mozilla.org/r/65028/#review62030

Comment 9

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8772130 [details]
Bug 1287552 - part 2 - fix transformed bitmaps with Skia and FreeType.

https://reviewboard.mozilla.org/r/65030/#review62282

Comment 10

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8772131 [details]
Bug 1287552 - part 3 - revise SkFontHost_cairo to properly parse Fontconfig options.

https://reviewboard.mozilla.org/r/65032/#review62310

looks fine to me, but you may want to run this by karlt as well

::: gfx/skia/skia/src/ports/SkFontHost_cairo.cpp:267
(Diff revision 1)
>  
> -    SkCairoFTTypeface(cairo_font_face_t* fontFace, const SkFontStyle& style, SkFontID id, bool isFixedWidth)
> +    SkCairoFTTypeface(const SkFontStyle& style, SkFontID id, bool isFixedWidth,
> +                      cairo_font_face_t* fontFace, FcPattern* pattern)
>          : SkTypeface(style, id, isFixedWidth)
>          , fFontFace(fontFace)
> +        , fPattern(SkSafeRef(pattern))

I think I'd rather have us lock/unlock this using a locking class. Something like SkLockedFcPattern that refs in the ctor and unrefs in the dtor. In any case, I don't really like the overloading of the "SkSafeRef/SkSafeUnref" naming for non-Skia objects.

Comment 11

[Lee Salzman [:lsalzman]]

Comment on attachment 8772131 [details]
Bug 1287552 - part 3 - revise SkFontHost_cairo to properly parse Fontconfig options.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65032/diff/1-2/

Comment 12

[Lee Salzman [:lsalzman]]

Comment on attachment 8772132 [details]
Bug 1287552 - part 4 - add ScaledFontFontconfig to remember generating FcPattern.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65034/diff/2-3/

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8772132 [details]
Bug 1287552 - part 4 - add ScaledFontFontconfig to remember generating FcPattern.

https://reviewboard.mozilla.org/r/65034/#review62644

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4db532e50f20
part 1 - backport upstream Cairo fix for computing transform of bitmap fonts. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/3406cf2a9590
part 2 - fix transformed bitmaps with Skia and FreeType. r=gw280
https://hg.mozilla.org/integration/autoland/rev/1831b7db03a4
part 3 - revise SkFontHost_cairo to properly parse Fontconfig options. r=gw280
https://hg.mozilla.org/integration/autoland/rev/df5b884bc592
part 4 - add ScaledFontFontconfig to remember generating FcPattern. r=jrmuizel

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4db532e50f20
https://hg.mozilla.org/mozilla-central/rev/3406cf2a9590
https://hg.mozilla.org/mozilla-central/rev/1831b7db03a4
https://hg.mozilla.org/mozilla-central/rev/df5b884bc592

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/836321463a26
only set FC_PIXEL_SIZE in pattern if not already supplied. r=me

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/836321463a26