Closed Bug 1547063 Opened 6 months ago Closed 28 days ago

Remove Cairo usage from Skia FreeType font host

Categories

(Core :: Graphics: Text, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: lsalzman, Assigned: lsalzman)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [wr-q3][wr-sept])

Attachments

(9 files)

SkFontHost_cairo depends on the interposition of Cairo for dealing with creating/loading FreeType faces. This imposes annoying limits on our Skia text rasterization such as a lack of subpixel text positioning if desired on Android and Linux. It also forces us to build and maintain FcPattern structures that cause memory bloat and have performance overhead to interpret.

We need to move the creation of FreeType faces into the Moz2D/thebes code. We will probably need to modify our tree Cairo if necessary to accept the raw FreeType faces and any other parameters necessary to interpret them without having to rely on the FcPatterns.

It might then be subsequently possible to remove SkFontHost_cairo entirely and rely on a small set of modifications to the provided SkFontHost_FreeType instead.

Whiteboard: [wr-q3][wr-july]
Blocks: 1568603
Whiteboard: [wr-q3][wr-july] → [wr-q3][wr-sept]

About 2 months ago subpixel positioning was added to Cairo: https://gitlab.freedesktop.org/cairo/cairo/commits/master

Does that perhaps remove the need to drop Cairo? Or maybe some of the current Linux text rendering issues can be fixed using that until Cairo is removed?

(In reply to Ivan Molodetskikh from comment #1)

About 2 months ago subpixel positioning was added to Cairo: https://gitlab.freedesktop.org/cairo/cairo/commits/master

Does that perhaps remove the need to drop Cairo? Or maybe some of the current Linux text rendering issues can be fixed using that until Cairo is removed?

Getting Cairo out of the way of things it shouldn't be involved in is still the right thing to do. Though, this might help in other cases where we can't once I finish this bug.

It's also worth mentioning that AFAIK we haven't updated mozilla's copy of cairo in a long time, and have done various local patches -- it's basically a forked version at this point -- so merging significant changes from upstream cairo may be non-trivial...

(In reply to Jonathan Kew (:jfkthame) from comment #3)

It's also worth mentioning that AFAIK we haven't updated mozilla's copy of cairo in a long time, and have done various local patches -- it's basically a forked version at this point -- so merging significant changes from upstream cairo may be non-trivial...

The patches here seem small enough that a hand-merge/manual graft seem easily feasible, just that there is no real excuse for Cairo to still be interposed here in the first place. So likely both of these are things we want to accomplish.

Depends on D44493

Depends on D44497

Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e74a67e2afe3
add SharedFTFace abstraction of FT_Face. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/772c3427c791
Cairo support for SharedFTFace. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/ebc71e607938
Remove FcPattern usage from Skia. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/af6e6807ece7
Use SharedFTFace in thebes. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/23892ecc6ef8
implement recording instance data for ScaledFontMac. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/2c7032b4d022
Use SharedFTFace locking instead of Cairo locking. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/003f5a79c6a7
Use FreeType metrics directly instead of querying Cairo. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/c969a93b0ca7
fuzz for SharedFTFace. r=jfkthame
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b54bb7c31f6
fix advance scaling for emoji. r=karlt
Regressions: 1581361
Regressions: 1581466
Status: RESOLVED → REOPENED
Flags: needinfo?(lsalzman)
Resolution: FIXED → ---
Target Milestone: mozilla71 → ---
Backout by csabou@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/be65ff9860e7
Backed out changeset 7b54bb7c31f6 for backing out also the other changes in Bug 1547063.

Fixed the printing crash. Should be fixed when I reland. Doing a try push now to verify.

Flags: needinfo?(lsalzman)
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe7f211ad6a3
add SharedFTFace abstraction of FT_Face. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/0b008480f41c
Cairo support for SharedFTFace. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/e15c477972e6
Remove FcPattern usage from Skia. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/542af2a68a49
Use SharedFTFace in thebes. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/880865a950e5
implement recording instance data for ScaledFontMac. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/99b28f31b550
Use SharedFTFace locking instead of Cairo locking. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/699f10cc0658
Use FreeType metrics directly instead of querying Cairo. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/e2324b6a2b33
fuzz for SharedFTFace. r=jfkthame
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0ff29989db1
fix advance scaling for emoji. r=karlt
Blocks: 1582231
Regressions: 1582749
Regressions: 1583005

== Change summary for alert #23211 (as of Tue, 24 Sep 2019 07:40:20 GMT) ==

Improvements:

5% Heap Unclassified linux64 opt 82,886,558.17 -> 79,008,195.00
4% Heap Unclassified linux64 opt 82,543,552.35 -> 79,023,892.13

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=23211

Blocks: 1583707
Blocks: 1584268
You need to log in before you can comment on or make changes to this bug.