Closed Bug 700023 Opened 9 years ago Closed 8 years ago

enable Graphite font shaping by default

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Depends on: 772640
Depends on: 831277
Depends on: 831687
Depends on: 833297
Depends on: 836225
Depends on: 843588
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.
Attachment #717191 - Flags: review?(jdaggett)
Assignee: nobody → jfkthame
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).
Attachment #717191 - Flags: review?(jdaggett) → review+
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).
That was bug 844439 (for details, see the sad story of b2g-R6 on inbound over the weekend).
https://hg.mozilla.org/mozilla-central/rev/cdc3588c8ce2
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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 → ---
(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.)
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.
(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.
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.
OK, here's an updated patch that disables it in mobile.js.
Attachment #720757 - Flags: review?(bugmail.mozilla)
Attachment #720579 - Attachment is obsolete: true
Attachment #720579 - Flags: review?(jdaggett)
Attachment #720757 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/6a0bcaa622f0
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.