Last Comment Bug 695857 - update harfbuzz to latest code from upstream
: update harfbuzz to latest code from upstream
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla13
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on: 603879 727736 729626 986802
Blocks: 662055 665352 680692 745780 832486
  Show dependency treegraph
 
Reported: 2011-10-19 14:10 PDT by Jonathan Kew (:jfkthame)
Modified: 2016-04-07 11:42 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - harfbuzz code from upstream repo (89d89646e8163b6c0874b9a3c14d4da974ea8219) (887.83 KB, patch)
2011-10-19 14:10 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
part 2 - update thebes code to account for harfbuzz api changes (91.34 KB, patch)
2011-10-19 14:14 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
part 2 - update thebes code to account for harfbuzz api changes (91.83 KB, patch)
2011-10-25 04:34 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
harfbuzz library code from upstream, rev 3d0ddd12801689b4093ffca97da4dd9ca669b64a (16-01-2012) (899.58 KB, patch)
2012-01-16 12:11 PST, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
part 2 - update thebes code to account for harfbuzz api changes (98.02 KB, patch)
2012-01-16 12:13 PST, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
remove trailing commas in enums (1.96 KB, patch)
2012-01-16 12:20 PST, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
fix indexing in mark-skipping loops (9.98 KB, patch)
2012-01-16 12:22 PST, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
harfbuzz library code from upstream (25-01-2012) (901.93 KB, patch)
2012-01-25 09:41 PST, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
update thebes code to account for harfbuzz api changes (98.01 KB, patch)
2012-01-25 09:46 PST, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
harfbuzz library code from upstream (2012-02-13) (903.13 KB, patch)
2012-02-13 03:06 PST, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Splinter Review
update thebes code to account for harfbuzz api changes, refreshed (105.30 KB, patch)
2012-02-13 03:46 PST, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Splinter Review

Description Jonathan Kew (:jfkthame) 2011-10-19 14:10:05 PDT
Created attachment 568191 [details] [diff] [review]
part 1 - harfbuzz code from upstream repo (89d89646e8163b6c0874b9a3c14d4da974ea8219)
Comment 1 Jonathan Kew (:jfkthame) 2011-10-19 14:14:58 PDT
Created attachment 568193 [details] [diff] [review]
part 2 - update thebes code to account for harfbuzz api changes

There have been quite a few API changes in harfbuzz since the older release we're currently using, so we can't just drop in a new snapshot of the code - we also need to update our code to match the revised API.
Comment 2 Jonathan Kew (:jfkthame) 2011-10-19 14:18:49 PDT
BTW, I also included "generated" copies of hb-version.h and hb-ot-shape-complex-indic-machine.hh here, as that seemed much less painful than hacking our build system to generate them - in the latter case, that would also introduce a build-time dependency on the ragel processor. Simpler to check in the generated file, then every developer doesn't need to install an extra tool.

(For "real" harfbuzz releases, as opposed to git snapshots, perhaps we can persuade Behdad to include the generated state-machine file in the release package; in the meantime, we'll need to remember to regenerate it manually when we take future code updates.)
Comment 3 John Daggett (:jtd) 2011-10-25 03:57:46 PDT
Comment on attachment 568193 [details] [diff] [review]
part 2 - update thebes code to account for harfbuzz api changes

> +    // FIXME!!!! aRunScript is now a script TAG
>      // Pango, GLib, and HarfBuzz all happen to use the same script codes.
>      const PangoScript script = static_cast<PangoScript>(aRunScript);
>      // Might be nice to call pango_language_includes_script only once for the
>  
> +    // FIXME!!!!!!!!!!!!!
>      const PangoScript script = static_cast<PangoScript>(aRunScript);

Either fix these or file a bug and include a comment in place of FIXME.
Comment 4 Jonathan Kew (:jfkthame) 2011-10-25 04:04:49 PDT
Huh, I thought I'd dealt with that! Maybe I posted a stale version. Update coming....
Comment 5 Jonathan Kew (:jfkthame) 2011-10-25 04:34:01 PDT
Created attachment 569327 [details] [diff] [review]
part 2 - update thebes code to account for harfbuzz api changes

Removed obsolete "FIXME" comments.

So the code was actually OK; the "FIXME" comments were residue from an earlier approach, when I was going to switch gfx over to follow the new HarfBuzz API using tags for script identification throughout, in place of the existing sequentially-allocated codes. But I backed off on that, as it involved extra work and complexity for no benefit, particularly for the Linux implementation; maybe we could reconsider it when we're ready to drop Pango altogether and use HB exclusively. For now, though, we're better off staying with script codes, and only mapping them to tags (a trivial lookup) when we actually call into harfbuzz.

As a bonus, I've added a handful of static assertions (for the Pango backend only) to verify that the different script code enumerations do in fact match, as the comments claim. That way, if we do decide to switch our script codes to be HB-style tags some day, we'll get a heads-up that these casts are no longer correct.
Comment 6 Jonathan Kew (:jfkthame) 2011-10-26 02:57:33 PDT
This should fix bug 680692, btw.
Comment 7 Behdad Esfahbod 2012-01-16 07:30:19 PST
Jonathan: yes, for tarball releases the state machine will be in the tarball.  Any other changes upstream that would make your life easier?
Comment 8 Jonathan Kew (:jfkthame) 2012-01-16 12:11:50 PST
Created attachment 588955 [details] [diff] [review]
harfbuzz library code from upstream, rev 3d0ddd12801689b4093ffca97da4dd9ca669b64a (16-01-2012)
Comment 9 Jonathan Kew (:jfkthame) 2012-01-16 12:13:51 PST
Created attachment 588956 [details] [diff] [review]
part 2 - update thebes code to account for harfbuzz api changes

Updated for changes in thebes font code in the past few months.
Comment 10 Jonathan Kew (:jfkthame) 2012-01-16 12:20:23 PST
Created attachment 588958 [details] [diff] [review]
remove trailing commas in enums

Harfbuzz patch needed to avoid build problems with older/stricter compilers; I expect this to be fixed upstream soon, so this patch will become obsolete.
Comment 11 Jonathan Kew (:jfkthame) 2012-01-16 12:22:11 PST
Created attachment 588960 [details] [diff] [review]
fix indexing in mark-skipping loops

This is the patch from bug 701637, plus correction from bug 714067, ported to current harfbuzz trunk; I expect this to be committed upstream shortly, so the separate patch here will then become obsolete.
Comment 12 Jonathan Kew (:jfkthame) 2012-01-25 09:41:56 PST
Created attachment 591508 [details] [diff] [review]
harfbuzz library code from upstream (25-01-2012)

Refreshed to pick up latest fixes/enhancements from upstream.
Comment 13 Jonathan Kew (:jfkthame) 2012-01-25 09:46:37 PST
Created attachment 591512 [details] [diff] [review]
update thebes code to account for harfbuzz api changes

Un-bitrotted the gfxHarfBuzzShaper changes.
Comment 14 John Daggett (:jtd) 2012-01-29 18:40:49 PST
Did we push a harfbuzz change recently?  The library patch doesn't apply cleanly to latest trunk with a failure in hb-ot-layout.cc.  No biggy, just thought I would note in case it's important.
Comment 15 Jonathan Kew (:jfkthame) 2012-01-30 04:10:18 PST
That'll be the patch from bug 719366 (which has already been pushed upstream as well).
Comment 16 Jonathan Kew (:jfkthame) 2012-02-07 04:36:04 PST
jdaggett: r? ping...
Comment 17 John Daggett (:jtd) 2012-02-07 05:05:33 PST
I still need to do some testing but I'm on the road this week for meetings.  I'll finish testing it next Monday.
Comment 18 John Daggett (:jtd) 2012-02-12 23:48:34 PST
There are a bunch of compiler warnings generated by or related to HarfBuzz code that I think we should clean up:

> /builds/harfbuzz-update/gfx/thebes/gfxHarfBuzzShaper.cpp: In member function ‘nsresult gfxHarfBuzzShaper::SetGlyphsFromRun(gfxContext*, gfxShapedWord*, hb_buffer_t*)’:
> /builds/harfbuzz-update/gfx/thebes/gfxHarfBuzzShaper.cpp:1045: warning: comparison between signed and unsigned integer expressions

Shouldn't glyphStart and charStart be PRUint32 instead of PRInt32?

> In file included from /builds/harfbuzz-update/gfx/harfbuzz/src/hb-unicode.cc:31:
> /builds/harfbuzz-update/gfx/harfbuzz/src/hb-private.hh:489:1: warning: "DEBUG" redefined

Undefine DEBUG for HarfBuzz in the makefile, since it's used as a macro function within this code.

Would it make sense to enable HB_DEBUG for debug builds?

> In file included from /builds/harfbuzz-update/gfx/harfbuzz/src/hb-object-private.hh:37,
>                  from /builds/harfbuzz-update/gfx/harfbuzz/src/hb-unicode-private.hh:37,
>                  from /builds/harfbuzz-update/gfx/harfbuzz/src/hb-unicode.cc:33:
> /builds/harfbuzz-update/gfx/harfbuzz/src/hb-mutex-private.hh:69:2: warning: #warning "Could not find any system to define platform macros, library will NOT be thread-safe"
> In file included from /builds/harfbuzz-update/gfx/harfbuzz/src/hb-unicode-private.hh:37,
>                  from /builds/harfbuzz-update/gfx/harfbuzz/src/hb-unicode.cc:33:
> /builds/harfbuzz-update/gfx/harfbuzz/src/hb-object-private.hh:81:2: warning: #warning "Could not find any system to define atomic_int macros, library will NOT be thread-safe"

Not much point to this since our code is not using this code across threads.

Bracket the #warning with a mozilla-specific #ifdef.
Comment 19 John Daggett (:jtd) 2012-02-12 23:55:02 PST
It looks like the data in gfxUnicodePropertyData.cpp was generated using recent-ish data but there's no way of knowing this.  How about including the header date from say the Scripts.txt file in the data?

Something like the comment in hb-ot-shape-complex-arabic-table.hh:

/*
 * The following table is generated by running:
 *
 *   ./gen-arabic-table.py ArabicShaping.txt
 *
 * on files with these headers:
 *
 * # ArabicShaping-6.1.0.txt
 * # Date: 2011-04-15, 23:16:00 GMT [KW]
 */
Comment 20 Jonathan Kew (:jfkthame) 2012-02-13 03:06:54 PST
Created attachment 596636 [details] [diff] [review]
harfbuzz library code from upstream (2012-02-13)
Comment 21 Jonathan Kew (:jfkthame) 2012-02-13 03:46:04 PST
Created attachment 596644 [details] [diff] [review]
update thebes code to account for harfbuzz api changes, refreshed
Comment 22 Jonathan Kew (:jfkthame) 2012-02-13 03:50:39 PST
(In reply to John Daggett (:jtd) from comment #18)
> Shouldn't glyphStart and charStart be PRUint32 instead of PRInt32?

They're signed because we use -1 as an "invalid" indicator (e.g. the NO_GLYPH value used to initialize charToGlyphArray), so I've just added a typecast here.

> > In file included from /builds/harfbuzz-update/gfx/harfbuzz/src/hb-unicode.cc:31:
> > /builds/harfbuzz-update/gfx/harfbuzz/src/hb-private.hh:489:1: warning: "DEBUG" redefined
> 
> Undefine DEBUG for HarfBuzz in the makefile, since it's used as a macro
> function within this code.

OK, done.

> Would it make sense to enable HB_DEBUG for debug builds?

I don't think so, that's designed to offer very detailed tracing - suitable for debugging harfbuzz itself, but not really appropriate or useful within our context.

> > /builds/harfbuzz-update/gfx/harfbuzz/src/hb-mutex-private.hh:69:2: warning: #warning "Could not find any system to define platform macros, library will NOT be thread-safe"
> > /builds/harfbuzz-update/gfx/harfbuzz/src/hb-object-private.hh:81:2: warning: #warning "Could not find any system to define atomic_int macros, library will NOT be thread-safe"
> 
> Not much point to this since our code is not using this code across threads.

True, but I'd prefer not to maintain a local modification just to suppress the messages - let's ask Behdad to consider providing a HB_SINGLE_THREADED symbol or something like that, which we can then set from our makefile.
Comment 23 John Daggett (:jtd) 2012-02-13 04:08:28 PST
(In reply to Jonathan Kew (:jfkthame) from comment #22)
> True, but I'd prefer not to maintain a local modification just to suppress
> the messages - let's ask Behdad to consider providing a HB_SINGLE_THREADED
> symbol or something like that, which we can then set from our makefile.

That sounds reasonable.
Comment 24 Jonathan Kew (:jfkthame) 2012-02-14 00:07:08 PST
Pushed both patches together, as neither one can build without the other:
https://hg.mozilla.org/integration/mozilla-inbound/rev/392319d8c1fa
Comment 25 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-02-15 08:45:58 PST
https://hg.mozilla.org/mozilla-central/rev/392319d8c1fa
Comment 26 Behdad Esfahbod 2012-02-29 17:22:44 PST
I've added support for HB_NO_MT upstream, also made the warning happen once only, also implemented native Apple MT macros.  Pick your preferred solution :).

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