Last Comment Bug 758236 - update harfbuzz to pick up (in-progress) Indic shaper
: update harfbuzz to pick up (in-progress) Indic shaper
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Text (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on: 745780 767249
Blocks: 631159 723966 619524 758241 761019 782908
  Show dependency treegraph
 
Reported: 2012-05-24 08:54 PDT by Jonathan Kew (:jfkthame)
Modified: 2016-04-07 11:42 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
harfbuzz from upstream commit a6de53664df9549a5dc93752647ea1d3bb336f7b (329.64 KB, patch)
2012-05-24 08:54 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
fix for HB non-multithreaded build breakage (670 bytes, patch)
2012-05-24 08:55 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Splinter Review
re-apply patch from bug 727736 after harfbuzz update (4.95 KB, patch)
2012-05-24 08:57 PDT, Jonathan Kew (:jfkthame)
jfkthame: review+
Details | Diff | Splinter Review
harfbuzz from upstream commit a3547330fa88e30a138f6f17e60d9c7d1e316622 (333.19 KB, patch)
2012-05-27 11:06 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Splinter Review
force non-spacing marks to have zero width (brought forward from bug 727736) (4.48 KB, patch)
2012-05-27 11:08 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Splinter Review
simple reftests for some Indic shaping features (57.56 KB, patch)
2012-05-27 11:13 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
similar reftests using Gujarati script (52.79 KB, patch)
2012-05-27 14:14 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Splinter Review
some reftests for Bengali script shaping (96.09 KB, patch)
2012-05-28 04:34 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Splinter Review
updated devanagari-2 test so it actually passes with Lohit Hindi (57.56 KB, patch)
2012-05-28 05:52 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Splinter Review
mark hb-indic tests that fail on OS X < 10.7 (2.79 KB, patch)
2012-05-29 01:03 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Splinter Review
update the comment for the harfbuzz.scripts pref (904 bytes, patch)
2012-05-31 00:04 PDT, John Daggett (:jtd)
no flags Details | Diff | Splinter Review

Description Jonathan Kew (:jfkthame) 2012-05-24 08:54:02 PDT
Created attachment 626831 [details] [diff] [review]
harfbuzz from upstream commit a6de53664df9549a5dc93752647ea1d3bb336f7b

Time for another harfbuzz update; in particular, the Indic shaper has made great strides, along with miscellaneous other fixes.
Comment 1 Jonathan Kew (:jfkthame) 2012-05-24 08:55:38 PDT
Created attachment 626832 [details] [diff] [review]
fix for HB non-multithreaded build breakage

Minor fix needed for harfbuzz with the HB_NO_MT option (as reported to the mailing list).
Comment 2 Jonathan Kew (:jfkthame) 2012-05-24 08:57:46 PDT
Created attachment 626833 [details] [diff] [review]
re-apply patch from bug 727736 after harfbuzz update

Fix for fonts with non-zerowidth mark glyphs; carrying forward r+ from bug 727736, just posting it here so I don't forget that it needs to be re-applied on top of the HB update.
Comment 3 John Daggett (:jtd) 2012-05-24 20:27:03 PDT
Comment on attachment 626832 [details] [diff] [review]
fix for HB non-multithreaded build breakage

I'm assuming this is going upstream also?
Comment 4 Jonathan Kew (:jfkthame) 2012-05-25 00:58:25 PDT
It just did, as commit cde1c0114ba66a45d907e81a49bf625e0dc946b0.
Comment 5 Jonathan Kew (:jfkthame) 2012-05-27 11:06:46 PDT
Created attachment 627573 [details] [diff] [review]
harfbuzz from upstream commit a3547330fa88e30a138f6f17e60d9c7d1e316622

Refreshed harfbuzz patch, now at upstream commit a3547330fa88e30a138f6f17e60d9c7d1e316622, incorporates the HB_NO_MT build fix.
Comment 6 Jonathan Kew (:jfkthame) 2012-05-27 11:08:10 PDT
Created attachment 627574 [details] [diff] [review]
force non-spacing marks to have zero width (brought forward from bug 727736)
Comment 7 Jonathan Kew (:jfkthame) 2012-05-27 11:13:54 PDT
Created attachment 627575 [details] [diff] [review]
simple reftests for some Indic shaping features

This adds a few reftests designed to check that specific features of Indic shaping are behaving as expected. Extensive testing of the Indic shaper itself belongs upstream in harfbuzz (and Behdad is collecting a large regression-testing suite), but this checks that it is being properly activated within Gecko.

Whether the tests would pass or fail with harfbuzz disabled is of course dependent on the capabilities of the platform shapers we'd be using, but with harfbuzz enabled (using the reftest pref() annotation) and an in-tree font, results should be consistent across platforms.

This first set of tests use Devanagari; I'll put together a few similar examples with other Indic scripts as separate patches.
Comment 8 Jonathan Kew (:jfkthame) 2012-05-27 14:14:51 PDT
Created attachment 627592 [details] [diff] [review]
similar reftests using Gujarati script
Comment 9 Jonathan Kew (:jfkthame) 2012-05-28 04:34:40 PDT
Created attachment 627663 [details] [diff] [review]
some reftests for Bengali script shaping
Comment 10 Jonathan Kew (:jfkthame) 2012-05-28 05:52:18 PDT
Created attachment 627678 [details] [diff] [review]
updated devanagari-2 test so it actually passes with Lohit Hindi

Oops, the devanagari-2 test made a bad assumption about glyphs that I thought would have the same width, but in Lohit they're slightly different, so it failed on tryserver. This version should work properly.
Comment 11 Jonathan Kew (:jfkthame) 2012-05-28 05:56:31 PDT
Another issue is that a number of the Gujarati and Bengali tests fail on OS X 10.6 because our masking of "unsupported" Indic ranges from the cmap on that OS version means that the intended reftest font isn't actually used, and the platform lacks font support for these scripts so they don't fall back to working with an available AAT font.

Until we're ready to switch on harfbuzz for Indic on OS X (which we probably shouldn't do until it's been in the tree for a while and had some testing by knowledgeable people willing to explicitly enable it and check the resulting behavior), we probably just need to mark those tests as failing.
Comment 12 Jonathan Kew (:jfkthame) 2012-05-29 01:03:59 PDT
Created attachment 627869 [details] [diff] [review]
mark hb-indic tests that fail on OS X < 10.7

This annotates the tests that fail on OS X prior to 10.7, due to the masking of the Indic-script codepoints in the cmap. (We'll want to remove that when we're ready to switch on harfbuzz for those scripts, so this is a temporary situation.)
Comment 13 John Daggett (:jtd) 2012-05-30 23:39:56 PDT
Comment on attachment 627869 [details] [diff] [review]
mark hb-indic tests that fail on OS X < 10.7

Please log a follow-on bug for these and mark the tests with that.
Comment 14 John Daggett (:jtd) 2012-05-31 00:04:16 PDT
Created attachment 628614 [details] [diff] [review]
update the comment for the harfbuzz.scripts pref

Hmm, the harfbuzz.scripts pref seems to have become a confusing mess.  Trying to enable Indic means doing a bit of spelunking through various parts of the code.

So setting gfx.font_rendering.harfbuzz.scripts to 255 should mean "enable for all scripts", right?

The code in all.js is out of date, it points the viewer to gfx/thebes/gfxUnicodeProperties.h:

http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#194

But the constants were moved here:

http://mxr.mozilla.org/mozilla-central/source/intl/unicharutil/util/nsUnicodeProperties.h#122
Comment 15 Jonathan Kew (:jfkthame) 2012-05-31 00:37:38 PDT
Yes, 255 would enable for all (so far, anyhow, unless we subdivide further and allocate more bits). Simplest suggestion, though, would be to use -1 for "all scripts"; that's what I generally set in about:config when I want to test.
Comment 16 Jonathan Kew (:jfkthame) 2012-05-31 03:34:20 PDT
Actually, the patch in bug 758241 already includes a revision of the out-of-date comment in all.js, though perhaps mentioning the mxr identifier search would be good too.
Comment 17 John Daggett (:jtd) 2012-05-31 12:58:49 PDT
Comment on attachment 628614 [details] [diff] [review]
update the comment for the harfbuzz.scripts pref

Covered by changes in bug 758241.
Comment 20 neil@parkwaycc.co.uk 2012-06-04 05:33:32 PDT
gcc4.3.2 complains about trailing commas in enumerator lists...
Comment 21 John Daggett (:jtd) 2012-06-04 06:15:27 PDT
(In reply to neil@parkwaycc.co.uk from comment #20)
> gcc4.3.2 complains about trailing commas in enumerator lists...

See bug 761019

Note You need to log in before you can comment on or make changes to this bug.