SkFontHost_cairo does not properly interact with Fontconfig

RESOLVED FIXED in Firefox 50

Status

()

Core
Graphics
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: lsalzman, Assigned: lsalzman)

Tracking

unspecified
mozilla50
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8772129 [details]
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/
Attachment #8772129 - Flags: review?(jmuizelaar)
Attachment #8772130 - Flags: review?(gwright)
Attachment #8772131 - Flags: review?(gwright)
Attachment #8772132 - Flags: review?(jmuizelaar)
(Assignee)

Comment 2

a year ago
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/
(Assignee)

Comment 3

a year ago
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/
(Assignee)

Comment 4

a year ago
Created attachment 8772132 [details]
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/
(Assignee)

Comment 5

a year ago
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/
(Assignee)

Comment 6

a year ago
(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.
(Assignee)

Comment 7

a year ago
(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 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
Attachment #8772129 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8772130 [details]
Bug 1287552 - part 2 - fix transformed bitmaps with Skia and FreeType.

https://reviewboard.mozilla.org/r/65030/#review62282
Attachment #8772130 - Flags: review?(gwright) → review+
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.
Attachment #8772131 - Flags: review?(gwright) → review+
(Assignee)

Comment 11

a year ago
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/
(Assignee)

Comment 12

a year ago
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 on attachment 8772132 [details]
Bug 1287552 - part 4 - add ScaledFontFontconfig to remember generating FcPattern.

https://reviewboard.mozilla.org/r/65034/#review62644
Attachment #8772132 - Flags: review?(jmuizelaar) → review+

Comment 14

a year ago
Pushed by lsalzman@mozilla.com:
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

a year ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

a year ago
Depends on: 1288762

Updated

a year ago
Depends on: 1288832

Comment 16

a year ago
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/836321463a26
only set FC_PIXEL_SIZE in pattern if not already supplied. r=me

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/836321463a26
Duplicate of this bug: 1287144
(Assignee)

Updated

a year ago
Depends on: 1294207
(Assignee)

Updated

2 months ago
Depends on: 1393467
You need to log in before you can comment on or make changes to this bug.