1325775 - update harfbuzz to upstream release 1.4.1

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Krasnaya Ploshchad’]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161208153507

Steps to reproduce:

https://github.com/behdad/harfbuzz/commit/b843c6d8b66c2833cd35407ee494546465e6d775

Overview of changes leading to 1.3.4
Monday, December 5, 2016
====================================

- Fix vertical glyph origin in hb-ot-font.
- Implement CBDT/CBLC color font glyph extents in hb-ot-font.

Comment 1

[Krasnaya Ploshchad’]

With this release you’ll get some benifits for vertical layout, especially for CJK.

Comment 2

[Frédéric Wang (:fredw)]

See also bug 1324780

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Attachment: update harfbuzz to version 1.3.4

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b286e9bdb0ff979fd5ae09b03712f6f4a4b220b7

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8823497 [details] [diff] [review]
update harfbuzz to version 1.3.4

Looking good enough on Try for a review request, I reckon.

Comment 5

[Frédéric Wang (:fredw)]

@Ryan: Can you please keep the changes from https://hg.mozilla.org/mozilla-central/rev/384f988caf6e in this update? It seems that they are not included in 1.3.4

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Attachment: update harfbuzz to version 1.3.4

Comment 7

[Jonathan Kew [:jfkthame]]

Haha - turns out Behdad has released 1.4.0 today, right after I r+'d the update here. Want to grab the new version instead?

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Try push looks pretty busted, unless you see an easy fix.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be4fbc8b1416de74d3263e5f74eb9ccfc37c7169

Comment 9

[Jonathan Kew [:jfkthame]]

Ouch! OK, I'll have a look and see if there's something simple...

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Frédéric may have an opinion too since it appears to be from his upstream commit.
https://github.com/behdad/harfbuzz/pull/384

Comment 11

[Frédéric Wang (:fredw)]

I guess it's due to local declaration "HB_SHAPER_DATA_ENSURE_DECLARE(ot, face)" in hb-ot-math.cc which duplicates the one in hb-ot-layout.cc ; what happens if you remove it?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Oh, probably because we do unified builds! I'll update the patch and re-push.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Winds up at other bustage:
https://treeherder.mozilla.org/logviewer.html#?job_id=66423451&repo=try&lineNumber=13073

Comment 14

[Frédéric Wang (:fredw)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Oh, probably because we do unified builds! I'll update the patch and re-push.

Yes, that's what I meant.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)
> Winds up at other bustage:
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=66423451&repo=try&lineNumber=13073

It seems you removed the definition from hb-ot-layout.cc and kept the one in hb-ot-math.cc. I was thinking of doing it the other way around: remove it from hb-ot-math.cc (where the redefition error happens) and keep it in hb-ot-layout.cc. Sorry, for the confusion.

Comment 15

[Krasnaya Ploshchad’]

https://github.com/behdad/harfbuzz/commit/f3397069479cae34e6bdc658e2875fb178b03e43

Overview of changes leading to 1.4.0
Thursday, January 5, 2017
====================================

- Merged "OpenType GX" branch which adds core of support for
  OpenType 1.8 Font Variations.  To that extent, the relevant
  new API is:

New API:
hb_font_set_var_coords_normalized()

  with supporting API:

New API:
HB_OT_LAYOUT_NO_VARIATIONS_INDEX
hb_ot_layout_table_find_feature_variations()
hb_ot_layout_feature_with_variations_get_lookups()
hb_shape_plan_create2()
hb_shape_plan_create_cached2()

  Currently variations in GSUB/GPOS/GDEF are fully supported,
  and no other tables are supported.  In particular, fvar/avar
  are NOT supported, hence the hb_font_set_var_coords_normalized()
  taking normalized coordinates.  API to take design coordinates
  will be added in the future.

  HVAR/VVAR/MVAR support will also be added to hb-ot-font in the
  future.

- Fix regression in GDEF glyph class processing.
- Add decompositions for Chakma, Limbu, and Balinese in USE shaper.
- Misc fixes.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Yeah, that appears to be working better :)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Attachment: update harfbuzz to version 1.4.0

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceed070f9de7bd51b0f9fdeca8db92bdcba763c4

Comment 18

[Jonathan Kew [:jfkthame]]

(In reply to Frédéric Wang (:fredw) from comment #14)
> It seems you removed the definition from hb-ot-layout.cc and kept the one in
> hb-ot-math.cc. I was thinking of doing it the other way around: remove it
> from hb-ot-math.cc (where the redefition error happens) and keep it in
> hb-ot-layout.cc. Sorry, for the confusion.

The trouble with this is that the same thing will presumably happen on future updates, and we'll have to re-discover the issue and repeat the fix. And we don't currently have a fully-automated update process for HB that carries forward local patches...

Probably better to take hb-ot-math.cc out of UNIFIED_SOURCES and put it in SOURCES instead.

(Or is there a fix we can request upstream that will allow unified compilation of these files?)

Comment 19

[Frédéric Wang (:fredw)]

(In reply to Jonathan Kew (:jfkthame) from comment #18)
> (In reply to Frédéric Wang (:fredw) from comment #14)
> > It seems you removed the definition from hb-ot-layout.cc and kept the one in
> > hb-ot-math.cc. I was thinking of doing it the other way around: remove it
> > from hb-ot-math.cc (where the redefition error happens) and keep it in
> > hb-ot-layout.cc. Sorry, for the confusion.
> 
> The trouble with this is that the same thing will presumably happen on
> future updates, and we'll have to re-discover the issue and repeat the fix.
> And we don't currently have a fully-automated update process for HB that
> carries forward local patches...
> 
> Probably better to take hb-ot-math.cc out of UNIFIED_SOURCES and put it in
> SOURCES instead.

Yes, I was merely just proposing a temporary solution. Probably putting hb-ot-math out of UNIFIED_SOURCES is better, though.

> (Or is there a fix we can request upstream that will allow unified
> compilation of these files?)

I guess we should ask Behdad's opinion, here...

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Reftests are looking good across all platforms at least. I meant to ask earlier, is there anything special we need to do with respect to the newly-added APIs or is just having them available enough for now?

Comment 21

[Jonathan Kew [:jfkthame]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> Reftests are looking good across all platforms at least. I meant to ask
> earlier, is there anything special we need to do with respect to the
> newly-added APIs or is just having them available enough for now?

I expect we'll start using some of the new APIs in connection with bug 1302685 (and dependencies), so getting the update into our tree is helpful for that; but there's nothing we need to do that would block taking this update.

Comment 22

[Krasnaya Ploshchad’]

(In reply to Krasnaya Ploshchad from comment #15)
> https://github.com/behdad/harfbuzz/commit/
> f3397069479cae34e6bdc658e2875fb178b03e43
> 
> Overview of changes leading to 1.4.0
> Thursday, January 5, 2017
> ====================================
> 
> - Merged "OpenType GX" branch which adds core of support for
>   OpenType 1.8 Font Variations.  To that extent, the relevant
>   new API is:
> 
> New API:
> hb_font_set_var_coords_normalized()
> 
>   with supporting API:
> 
> New API:
> HB_OT_LAYOUT_NO_VARIATIONS_INDEX
> hb_ot_layout_table_find_feature_variations()
> hb_ot_layout_feature_with_variations_get_lookups()
> hb_shape_plan_create2()
> hb_shape_plan_create_cached2()
> 
>   Currently variations in GSUB/GPOS/GDEF are fully supported,
>   and no other tables are supported.  In particular, fvar/avar
>   are NOT supported, hence the hb_font_set_var_coords_normalized()
>   taking normalized coordinates.  API to take design coordinates
>   will be added in the future.
> 
>   HVAR/VVAR/MVAR support will also be added to hb-ot-font in the
>   future.
> 
> - Fix regression in GDEF glyph class processing.
> - Add decompositions for Chak

I’m sorry, this version has regression that affect Marchen. See: https://github.com/behdad/harfbuzz/issues/385

Comment 23

[Behdad Esfahbod]

So, this exact issue, is why originally I didn't put this code in a separate file.  The comment there indeed said:

/* TODO Move this to hb-ot-math.cc and separate it from hb_ot_layout_t. */

It's the separation that has not happened :).  I can revert the commit until I fix it properly.

Also, re the UNIFIED_SOURCES, where can I see how Firefox is building HB now?  I like to have that replicated upstream as well, for testing.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

The basic gist of UNIFIED_SOURCES is that it creates a single cpp file that #includes all the other ones (up to 16 per file), which speeds up compilation significantly. Though it comes with the pitfalls of occasionally running into compilation issues like we're seeing here.

The output from the build system takes the form of the files below (though they're for the current in-tree 1.3.3 version):
https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/gfx/harfbuzz/src/Unified_cpp_gfx_harfbuzz_src0.cpp
https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/gfx/harfbuzz/src/Unified_cpp_gfx_harfbuzz_src1.cpp

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Also, as shown in our moz.build file, we already omit a few source files from being unified due to other build issues:
https://dxr.mozilla.org/mozilla-central/source/gfx/harfbuzz/src/moz.build

Comment 26

[Krasnaya Ploshchad’]

The Marchen error in v1.4.0 have already been confirmed by HB developer, I guess waiting for newer version if we want to avoid this regression.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Overview of changes leading to 1.4.1
Thursday, January 5, 2017
====================================

- Always build and use UCDN for Unicode data by default.
  Reduces dependence on version of Unicode data in glib,
  specially in the Windows bundles we are shipping, which
have very old glib.

Comment 28

[Behdad Esfahbod]

There was no error in 1.4.0 code whatsoever.  It was just a different build configuration, resulting in using Unicode data from old glib.  Doesn't affect Firefox.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Sounds like Behdad is going to work on a better long-term fix for the unified build bustage we hit, so I'm going to stick with the workaround from comment 11 for now. My concern is that if I put hb-ot-math.cc in SOURCES instead, it'll just get stuck there in perpetuity even after the initial problem is resolved.

Comment 30

[Behdad Esfahbod]

Yes, please put it there.  I'll eventually go over the issues and make some more files UNIFIED-buildable.  You can revise this then.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Attachment: update harfbuzz to version 1.4.1

Per Behdad's request, I'm just putting hb-ot-math.cc in SOURCES for now and leaving the upstream source files untouched. Since the 1.4.0 Try push was green and 1.4.1 doesn't actually change anything relevant to Firefox, I just ran another build-only Try push for sanity checking.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9106d2ed6042ab105743409ffd4fe0f7721a6cda

Comment 32

[Jonathan Kew [:jfkthame]]

Comment on attachment 8824320 [details] [diff] [review]
update harfbuzz to version 1.4.1

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

Yep, I think this is the right thing to do, thanks.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff080d7f8628
Update harfbuzz to version 1.4.1. r=jfkthame

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8824320 [details] [diff] [review]
update harfbuzz to version 1.4.1

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: stability issues
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not really
[Why is the change risky/not risky?]: fixed for a month now with no known regressions
[String changes made/needed]: none

Comment 37

[Gerry Chang [:gchang]]

Comment on attachment 8824320 [details] [diff] [review]
update harfbuzz to version 1.4.1

Fix a stability issue by updating an upstream library. ESR45+.

Comment 38

[Julien Cristau [:jcristau]]

Comment on attachment 8824320 [details] [diff] [review]
update harfbuzz to version 1.4.1

update harfbuzz in beta52

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8c77d1712605

Comment 40

[Ryan VanderMeulen [:RyanVM]]

In retrospect, this is going to be way too big of an uplift for esr45.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/8c77d1712605

Comment 42

[Julien Cristau [:jcristau]]

Comment on attachment 8824320 [details] [diff] [review]
update harfbuzz to version 1.4.1

This isn't going in esr45 after all.