The default bug view has changed. See this FAQ.

update harfbuzz to pick up (in-progress) Indic shaper

RESOLVED FIXED in mozilla15

Status

()

Core
Graphics: Text
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 4 obsolete attachments)

333.19 KB, patch
jtd
: review+
Details | Diff | Splinter Review
4.48 KB, patch
jtd
: review+
Details | Diff | Splinter Review
52.79 KB, patch
jtd
: review+
Details | Diff | Splinter Review
96.09 KB, patch
jtd
: review+
Details | Diff | Splinter Review
57.56 KB, patch
jtd
: review+
Details | Diff | Splinter Review
2.79 KB, patch
jtd
: review+
Details | Diff | Splinter Review
904 bytes, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
Attachment #626831 - Flags: review?(jdaggett)
(Assignee)

Comment 1

5 years ago
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).
Attachment #626832 - Flags: review?(jdaggett)
(Assignee)

Comment 2

5 years ago
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.
Attachment #626833 - Flags: review+
(Assignee)

Updated

5 years ago
Blocks: 758241

Comment 3

5 years ago
Comment on attachment 626832 [details] [diff] [review]
fix for HB non-multithreaded build breakage

I'm assuming this is going upstream also?
Attachment #626832 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 4

5 years ago
It just did, as commit cde1c0114ba66a45d907e81a49bf625e0dc946b0.
(Assignee)

Comment 5

5 years ago
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.
Attachment #626831 - Attachment is obsolete: true
Attachment #626832 - Attachment is obsolete: true
Attachment #626831 - Flags: review?(jdaggett)
Attachment #627573 - Flags: review?(jdaggett)
(Assignee)

Comment 6

5 years ago
Created attachment 627574 [details] [diff] [review]
force non-spacing marks to have zero width (brought forward from bug 727736)
Attachment #626833 - Attachment is obsolete: true
Attachment #627574 - Flags: review?(jdaggett)
(Assignee)

Comment 7

5 years ago
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.
Attachment #627575 - Flags: review?(jdaggett)
(Assignee)

Comment 8

5 years ago
Created attachment 627592 [details] [diff] [review]
similar reftests using Gujarati script
Attachment #627592 - Flags: review?(jdaggett)
(Assignee)

Comment 9

5 years ago
Created attachment 627663 [details] [diff] [review]
some reftests for Bengali script shaping
Attachment #627663 - Flags: review?(jdaggett)
(Assignee)

Comment 10

5 years ago
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.
Attachment #627575 - Attachment is obsolete: true
Attachment #627575 - Flags: review?(jdaggett)
(Assignee)

Updated

5 years ago
Attachment #627678 - Flags: review?(jdaggett)
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Comment 12

5 years ago
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.)
Attachment #627869 - Flags: review?(jdaggett)

Updated

5 years ago
Attachment #627574 - Flags: review?(jdaggett) → review+

Comment 13

5 years ago
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.
Attachment #627869 - Flags: review?(jdaggett) → review+

Comment 14

5 years ago
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
Attachment #628614 - Flags: review?(jfkthame)
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Comment 16

5 years ago
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

5 years ago
Comment on attachment 628614 [details] [diff] [review]
update the comment for the harfbuzz.scripts pref

Covered by changes in bug 758241.
Attachment #628614 - Flags: review?(jfkthame)

Updated

5 years ago
Attachment #627573 - Flags: review?(jdaggett) → review+

Updated

5 years ago
Attachment #627592 - Flags: review?(jdaggett) → review+

Updated

5 years ago
Attachment #627663 - Flags: review?(jdaggett) → review+

Updated

5 years ago
Attachment #627678 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/11c6d420c23c (hb from upstream)
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfae6ae40e66 (zero-width marks)
https://hg.mozilla.org/integration/mozilla-inbound/rev/964ccc39cde9 (indic reftests)
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/11c6d420c23c
https://hg.mozilla.org/mozilla-central/rev/bfae6ae40e66
https://hg.mozilla.org/mozilla-central/rev/964ccc39cde9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 761019
gcc4.3.2 complains about trailing commas in enumerator lists...

Comment 21

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #20)
> gcc4.3.2 complains about trailing commas in enumerator lists...

See bug 761019
Depends on: 767249
Blocks: 782908
Depends on: 745780
You need to log in before you can comment on or make changes to this bug.