enable Graphite font shaping by default

RESOLVED FIXED in mozilla22

Status

()

Core
Layout: Text
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

Trunk
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
In bug 631479, graphite support will be landed preffed-off; once we have sufficient confidence in the implementation, and test coverage, we should flip the pref to enable it by default so that sites depending on Graphite-enabled fonts will work out-of-the-box.
(Assignee)

Updated

5 years ago
Depends on: 772640
(Assignee)

Updated

4 years ago
Depends on: 831277
(Assignee)

Updated

4 years ago
Depends on: 831687
(Assignee)

Updated

4 years ago
Depends on: 833297
(Assignee)

Updated

4 years ago
Depends on: 836225
(Assignee)

Updated

4 years ago
Depends on: 843588
(Assignee)

Comment 1

4 years ago
We've had the Graphite text-shaping library in Firefox (preffed-off by default) for a year now, since it landed for FF11 in bug 631479.

It's had considerable fuzz-testing, both as part of the Firefox build (see bug 631479 comment 147, and bug 681976 dependencies) and upstream within the SIL graphite project (see the /tests/ directory in the graphite repository). Issues found have been promptly addressed by the team, and by this time the code appears to be both stable and robust.

While Graphite is always likely to be a niche technology, primarily of interest to those with specialized script needs that are not covered by OpenType, there are projects that are actively using it and relying on Gecko's graphite support - e.g. the sign-language example in bug 836225, or the Javanese font at https://sites.google.com/site/jawaunicode/main-page.

So I think it's time to flip the setting of the graphite pref, so that this functionality is available by default - in particular, so that users visiting a site that deploys a Graphite-enabled font will see the correct rendering without having to manually change a setting in about:config first.
(Assignee)

Comment 2

4 years ago
Created attachment 717191 [details] [diff] [review]
enable graphite font shaping by default.
Attachment #717191 - Flags: review?(jdaggett)
(Assignee)

Updated

4 years ago
Assignee: nobody → jfkthame
(Assignee)

Comment 3

4 years ago
Tryserver run in progress: https://tbpl.mozilla.org/?tree=Try&rev=cceeb004fe1d (includes the graphite reftests from bug 700022, which are currently on mozilla-inbound).

Updated

4 years ago
Attachment #717191 - Flags: review?(jdaggett) → review+

Comment 4

4 years ago
Might be a good idea to wait a couple days to see what's going on with B2G R6, just in case there's something there related to Graphite (e.g. memory stomping).  That's failing in your tryserver build.  Seems like it should be unrelated (WebGL error).
(Assignee)

Comment 5

4 years ago
That was bug 844439 (for details, see the sad story of b2g-R6 on inbound over the weekend).
(Assignee)

Comment 6

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdc3588c8ce2
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/cdc3588c8ce2
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Depends on: 846832

Comment 8

4 years ago
Given the 22MB regression on Android (bug 846832) I think we should back this out for now until we can diagnose that.  There's no pressing need for Graphite to be on by default.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 9

4 years ago
(In reply to John Daggett (:jtd) from comment #8)
> Given the 22MB regression on Android (bug 846832) I think we should back
> this out for now until we can diagnose that.  There's no pressing need for
> Graphite to be on by default.

I agree that this is a serious regression, though we should let :jfkthame have a look first. I'm also curious why this is a fennec-only regression. Perhaps querying for Graphite tables in Android fonts keeps the font in RAM. That's just a guess though, and Jonathan can have a better look (and prepare a backout patch if needed.)
(Assignee)

Comment 10

4 years ago
The reason this impacts fennec (only) is that the Charis fonts we're now bundling as the default serif font for FF on Android include graphite layout tables in addition to OpenType (and AAT, actually). The graphite tables are around 300K (per face) in the font files, but it looks like the RAM footprint for actually using the graphite shaper must be substantially higher than for harfbuzz/opentype.

So it's not surprising there'd be an impact here, but I am surprised at how large it looks. So I think for now, at least, we should flip the pref back to false on Android, pending further investigation.
(Assignee)

Comment 11

4 years ago
Created attachment 720579 [details] [diff] [review]
followup - flip the graphite pref back to false on android, due to memory usage regression (see bug 846832)
Attachment #720579 - Flags: review?(jdaggett)
(In reply to Jonathan Kew (:jfkthame) from comment #11)
> Created attachment 720579 [details] [diff] [review]
> followup - flip the graphite pref back to false on android, due to memory
> usage regression (see bug 846832)

Juts to be clear, you don't believe this affects B2G?  We're doing very little testing on m-c these days, so if it did, it's unlikely anyone would notice for a while.
(Assignee)

Comment 13

4 years ago
It only affects android because that's the only place (AFAIK) we have a set of default font faces that include graphite tables. Though if B2G decides to use the same fonts as we're shipping for android, it would become an issue there as well. I notice there are a couple of open bugs about adding/changing default fonts on B2G, so this is something to watch.

Also note that bug 847344 (about optimizing how we use the API) should address the memory impact here.
Comment on attachment 720579 [details] [diff] [review]
followup - flip the graphite pref back to false on android, due to memory usage regression (see bug 846832)

Review of attachment 720579 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpref/src/init/all.js
@@ +238,2 @@
>  pref("gfx.font_rendering.graphite.enabled", true);
> +#endif

Note you can override this pref for android by putting it in mobile/android/app/mobile.js instead. i.e. leave it "true" in all.js and then put the same pref with value "false" in mobile.js. I think that would be preferable to ifdef'ing.
(Assignee)

Comment 15

4 years ago
Created attachment 720757 [details] [diff] [review]
followup - flip the graphite pref back to false on android, due to memory usage regression (see bug 846832)

OK, here's an updated patch that disables it in mobile.js.
Attachment #720757 - Flags: review?(bugmail.mozilla)
(Assignee)

Updated

4 years ago
Attachment #720579 - Attachment is obsolete: true
Attachment #720579 - Flags: review?(jdaggett)
Attachment #720757 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 16

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a0bcaa622f0
https://hg.mozilla.org/mozilla-central/rev/6a0bcaa622f0
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.