700023 - enable Graphite font shaping by default

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

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.

Comment 1

[Jonathan Kew [:jfkthame]]

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.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: enable graphite font shaping by default.

Comment 3

[Jonathan Kew [: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).

Comment 4

[John Daggett (:jtd)]

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).

Comment 5

[Jonathan Kew [:jfkthame]]

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

Comment 6

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdc3588c8ce2

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cdc3588c8ce2

Comment 8

[John Daggett (:jtd)]

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.

Comment 9

[Jet Villegas (inactive)]

(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.)

Comment 10

[Jonathan Kew [:jfkthame]]

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.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: followup - flip the graphite pref back to false on android, due to memory usage regression (see bug 846832)

Comment 12

[Justin Lebar (not reading bugmail)]

(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.

Comment 13

[Jonathan Kew [:jfkthame]]

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 14

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 16

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a0bcaa622f0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a0bcaa622f0