Closed Bug 1036981 Opened 5 years ago Closed 5 years ago

update harfbuzz to upstream release 0.9.34

Categories

(Core :: Graphics: Text, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: ionnv, Assigned: poiru)

References

()

Details

Attachments

(2 files, 2 obsolete files)

http://cgit.freedesktop.org/harfbuzz/commit/?id=ea001374b86c4f1b24246c08a3d66d2a0e95a827

Overview of changes leading to 0.9.30
Wednesday, July 9, 2014
=====================================
- Update to Unicode 7.0.0:
 * New scripts Manichaean and Psalter Pahlavi are shaped using Arabic shaper.
 * All the other new scripts to through the generic shaper for now.
- Minor Indic improvements.
- Fix graphite2 backend cluster mapping [crasher!]
- API changes:
 * New HB_SCRIPT_* values for Unicode 7.0 scripts.
 * New function hb_ot_layout_language_get_required_feature().
- Build fixes.
(In reply to Jonathan Kew (:jfkthame) from bug 998961 comment #7)
> I'd like to take one soon. Behdad did a release (0.9.30) a few days ago, so
> we should update to that, ...

Done. Try push: https://tbpl.mozilla.org/?tree=Try&rev=3bc0afbbb6e5

The previous updates seem to have left e.g. autogen.sh and configure.ac untouched so this patch doesn't touch them either (however, I updated AUTHORS and COPYING). Perhaps we should remove these outdated files entirely in a follow-up?

This patch is based on: http://www.freedesktop.org/software/harfbuzz/release/harfbuzz-0.9.30.tar.bz2 (configured with bare `configure`)
Assignee: nobody → birunthan
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8456636 - Flags: review?(jfkthame)
There's 0.9.31 out :).
Summary: update harfbuzz to upstream release 0.9.30 → update harfbuzz to upstream release 0.9.31
I made a followup 0.9.32 release because the previous release didn't build on the Mac.  It does include a couple fixes though.  Jonathan wants to run the test suite on it.  It's not urgent to update.  Just letting you know.
Summary: update harfbuzz to upstream release 0.9.31 → update harfbuzz to upstream release 0.9.32
We're currently running 1132a7dd0ecf1c425078e39e5471330bace42659 through the big test suite, to check for any unexpected shaping regressions there; that'll take a couple of days or so. Once those results are in, we can review and land a patch here. Leaving r? for now so that bugzilla will nag me about it...
(In reply to Jonathan Kew (:jfkthame) from comment #5)
> We're currently running 1132a7dd0ecf1c425078e39e5471330bace42659 through the
> big test suite, to check for any unexpected shaping regressions there;
> that'll take a couple of days or so. Once those results are in, we can
> review and land a patch here. Leaving r? for now so that bugzilla will nag
> me about it...

Cool. Should I update the patch to the latest commit[0] (or some other rev/tag)?

[0]: https://github.com/behdad/harfbuzz/commit/0fc0a1022854324261fea8893678a3e9fd9443eb
Summary: update harfbuzz to upstream release 0.9.32 → update harfbuzz to upstream release 0.9.33
We'll want to take upstream commit 0a5ae9336231c4d189e1682e4fd9c9c4552b8bbf, so as to fix bug 1045139 with this update.
I feel like rolling a tarball today.  Anything else you want to see in?
How about removing 'liga' from default vertical features? I think that'd be better, on the whole.
I agree.  Doing now.
What should we do with the shapers in vertical??  Or is that not our concern?

Also, this:

  https://github.com/behdad/harfbuzz/blob/master/src/hb-ot-shape-complex-hangul.cc#L127

Might need more thought for vertical.  I'm leaning towards doing no reordering for vertical.  I don't know what Noto Sans CJK / Source Han Sans expect.  I know Ken wrote to me about it, have to check.
If a normally-horizontal script is rendered vertically (with text-orientation:upright, i.e. not by writing it with 90° rotation), then I'd guess most complex-script shaping should be disabled. E.g. Arabic letters would be written in their isolate forms; you wouldn't stack initial, medial and final forms above each other. (CSS Writing Modes mentions this: "characters from horizontal cursive scripts (such as Arabic) are shaped in their isolated forms when typeset upright".)

So perhaps we should re-route all horizontal scripts through the generic shaper if the direction is vertical?
(In reply to Jonathan Kew (:jfkthame) from comment #12)
> If a normally-horizontal script is rendered vertically (with
> text-orientation:upright, i.e. not by writing it with 90° rotation), then
> I'd guess most complex-script shaping should be disabled. E.g. Arabic
> letters would be written in their isolate forms; you wouldn't stack initial,
> medial and final forms above each other. (CSS Writing Modes mentions this:
> "characters from horizontal cursive scripts (such as Arabic) are shaped in
> their isolated forms when typeset upright".)

I actually don't think I agree with that.  I think it's best to consider such contexts like really narrow columns, and in that scenario shaping should still happen...


> So perhaps we should re-route all horizontal scripts through the generic
> shaper if the direction is vertical?

That's what Pango used to do indeed.  I'm leaning towards that too.  Though, it's not easy to decide.  Lets see:

  - Hangul shaper should be used, and adjusted if needed,

  - Hebrew shaper should be used, it does composition only,

  - Thai shaper should be used, though don't know what happens to the SARA AM stuff, but PUA shaping is good,

  - Tibetan should be used I believe,

  - So that leaves us the Indic / Myanmar / Sea,

  - Arabic, which as I said, I disagree with fantasai / CSS's recommendation.

I feel like we shouldn't touch that until you get it wired up to Firefox, then we experiment and decide / discuss with CSS people.
Release out.
(In reply to Behdad Esfahbod from comment #13)
> (In reply to Jonathan Kew (:jfkthame) from comment #12)
> > So perhaps we should re-route all horizontal scripts through the generic
> > shaper if the direction is vertical?
> 
> That's what Pango used to do indeed.  I'm leaning towards that too.  Though,
> it's not easy to decide.  Lets see:
> 
>   - Hangul shaper should be used, and adjusted if needed,
> 
>   - Hebrew shaper should be used, it does composition only,
> 
>   - Thai shaper should be used, though don't know what happens to the SARA
> AM stuff, but PUA shaping is good,
> 
>   - Tibetan should be used I believe,
> 
>   - So that leaves us the Indic / Myanmar / Sea,
> 
>   - Arabic, which as I said, I disagree with fantasai / CSS's recommendation.
> 
> I feel like we shouldn't touch that until you get it wired up to Firefox,
> then we experiment and decide / discuss with CSS people.

That sounds fine; we can evaluate once it's possible to experiment with real examples.

(In reply to Behdad Esfahbod from comment #14)
> Release out.

Awesome, thanks.

So we should update Gecko to 0.9.34. :poiru, are you happy to update the patch here (again), please? And a fresh tryserver push to check for surprises would be good. Then I think we'll be ready to land this.
Blocks: 1045139
Flags: needinfo?(birunthan)
Summary: update harfbuzz to upstream release 0.9.33 → update harfbuzz to upstream release 0.9.34
(In reply to Jonathan Kew (:jfkthame) from comment #15)
> So we should update Gecko to 0.9.34. :poiru, are you happy to update the
> patch here (again), please? And a fresh tryserver push to check for
> surprises would be good. Then I think we'll be ready to land this.

Here you go. Try push: https://tbpl.mozilla.org/?tree=Try&rev=a8bffb83a156

Note that the arabic-fallback-{1,2,3,4}.html reftests failed. Not sure what to do with those.
Attachment #8457643 - Attachment is obsolete: true
Attachment #8457643 - Flags: review?(jfkthame)
Attachment #8466799 - Flags: review?(jfkthame)
Flags: needinfo?(birunthan)
(In reply to Birunthan Mohanathas [:poiru] from comment #16)

> Here you go. Try push: https://tbpl.mozilla.org/?tree=Try&rev=a8bffb83a156

Great, thanks.

> 
> Note that the arabic-fallback-{1,2,3,4}.html reftests failed. Not sure what
> to do with those.

Drat, that's not good. :( I'll look into it.
Fallback Arabic shaping (using Unicode presentation forms) was inadvertently broken in upstream commit 615d00ea252739da57edbd980ff27e573f88ee7e. (Sigh.) So we need to get that fixed before we take the update.
(In reply to Birunthan Mohanathas [:poiru] from comment #16)

> Here you go. Try push: https://tbpl.mozilla.org/?tree=Try&rev=a8bffb83a156

I can reproduce the bug 1045139 with the build from http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/birunthan@mohanathas.com-a8bffb83a156/
Submitted https://github.com/behdad/harfbuzz/pull/48 which should fix the fallback shaping; Behdad, please check and merge (or fix) as appropriate - thanks.
I've submitted a new tryserver run that includes https://github.com/behdad/harfbuzz/pull/48; blinky, please re-test bug 1045139 with that build once it is available.

See https://tbpl.mozilla.org/?tree=Try&rev=5f9c2c9f8d67.
(In reply to Jonathan Kew (:jfkthame) from comment #21)
> I've submitted a new tryserver run that includes
> https://github.com/behdad/harfbuzz/pull/48; blinky, please re-test bug
> 1045139 with that build once it is available.
> 
> See https://tbpl.mozilla.org/?tree=Try&rev=5f9c2c9f8d67.

I can not reproduce the bug 1045139 with build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jkew@mozilla.com-5f9c2c9f8d67/, thanks.
Blocks: 1047265
Duplicate of this bug: 1047265
Behdad, please check https://github.com/behdad/harfbuzz/pull/48 and confirm whether this is OK as a fix for the Arabic-fallback-shaping breakage; once that's resolved, we'll be ready to take this update.
Flags: needinfo?(mozilla)
Ouch.  Fixed, and added test.
Flags: needinfo?(mozilla)
Comment on attachment 8466799 [details] [diff] [review]
Update HarfBuzz to upstream release 0.9.34

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

r=me, provided commit d5e61470fa8e5046c35a79988e00e012ae4fff0f from upstream is added to fix the Arabic fallback shaping problem.
Attachment #8466799 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/mozilla-central/rev/e885d8128d81
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
HarfBuzz update patch for uplift to Aurora (from mozilla-central e885d8128d81).
Comment on attachment 8470847 [details] [diff] [review]
Update HarfBuzz to upstream commit d5e6147 (0.9.34).

Approval Request Comment
[Feature/regressing bug #]: This fixes bug 1045139, an Arabic-specific regression from bug 985220. (It also fixes bug 986802, an older regression from bug 695857.)

[User impact if declined]: Windows users with Arabic system locale, and without hardware acceleration (i.e. using GDI fonts) will see broken text rendering on some websites.

[Describe test coverage new/current, TBPL]: We have reftests for a lot of shaping behavior, including non-Latin scripts. (Issues found by these tests on tryserver jobs are normally fixed upstream in harfbuzz before we accept the update in Gecko.) The library behavior is also tested extensively outside of Gecko (harfbuzz in-tree tests, and separate test suite at http://ec2-54-226-13-158.compute-1.amazonaws.com/index.html) and regressions found there have been fixed upstream.

[Risks and why]: Fairly low. Although this is a large patch, it's purely dropping in a new library version from upstream, without affecting the rest of Gecko outside of glyph shaping. This landed on mozilla-central last week without issues.

An alternative fix for the main regression we're concerned about here (bug 1045139) would be to cherry-pick just the Arabic fallback shaping fixes from harfbuzz trunk onto the older library version we have in Aurora. However, although this would be a much smaller patch overall, I believe it would be a riskier approach because some rebasing of the harfbuzz commits would be needed, and the result would not have the benefit of all upstream's testing.

[String/UUID change made/needed]: none
Attachment #8470847 - Flags: review+
Attachment #8470847 - Flags: approval-mozilla-aurora?
Comment on attachment 8470847 [details] [diff] [review]
Update HarfBuzz to upstream commit d5e6147 (0.9.34).

OK. I am taking it because we still have a few weeks in aurora + the whole beta cycle in case of problem.

However, if it starts to break more things than it fixesn I might ask for a backout + a cherry-pick of the Arabic changes.
Attachment #8470847 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1113070
You need to log in before you can comment on or make changes to this bug.