Support Harfbuzz in Metrics View #5522

iorsh · 2025-01-09T21:18:25Z

This PR introduces HarfBuzz shaping in Metrics view.

The current shaping functionality is preserved and can be invoked from View menu as "Built-in shaper". It's very complex, and I'm pretty sure I didn't manage to fully support everything it provides, so some people might find it better in some aspects.
The new shaper comes with some generic infrastructure which can be eventually expanded to support more shapers (e.g. Uniscribe under Windows)
The new code is written mostly in C++17. This version seems reasonably conservative to me.
Editing of kerning values, bearings, and width is supported.
The font is generated for HarfBuzz use every time the Metrics View is opened or shaper engine is switched (in the View menu). This could be slow for large fonts.

After:

Before:

Menu:

Fixes #3733, fixes #3837, fixes #4158, fixes #4511, fixes #5343, fixes #5513

…haper

…++ framework

This reverts commit 66e191d.

iorsh · 2025-01-09T21:27:52Z

CI is broken.

Linux: ubuntu-latest moved from 22 to 24
Windows - pacman issues again.

I'll take a look.

khaledhosny · 2025-01-10T10:00:27Z

fontforgeexe/shapers/harfbuzz.cpp

+    if (rtl) {
+        // HarfBuzz reverses the order of an RTL output buffer
+        std::reverse(glyphs_after_gpos, glyphs_after_gpos + glyph_count);
+        std::reverse(width_deltas.begin(), width_deltas.end());


You can use hb_buffer_reverse() earlier instead.

This doesn't work for me. The RTL buffer before the reversing contains glyphs tail-to-head, and hb_buffer_get_glyph_positions() returns values with geometrically correct left-to-right advances and offsets.

In the case of mixed base glyphs (positive advance) and marks (positive offset, since the mark comes before the glyph), the function hb_buffer_reverse() correctly reorders the glyphs, but hb_buffer_get_glyph_positions() would just return the same values in reverse order, and this breaks the geometric relation.

If the current code works for you, then fine. hb_shape() returns the glyphs in visual order (that is, for RTL text the buffer is reversed), so that code that is drawing the text wouldn‘t need to worry about the buffer direction. hb_buffer_reverse() undoes that. Either case the individual glyph advances will be the same since there are no RTL glyph advances in OpenType.

I don’t know what FontForge expects here, but it is probably some FontForge-specific thing, not HarfBuzz that is being unexpected.

khaledhosny · 2025-01-10T10:04:33Z

fontforgeexe/shapers/harfbuzz.cpp

+        // functions.
+        hb_bool_t found =
+            hb_font_get_glyph_name(hb_ttf_font, glyph_info.codepoint,
+                                   glyph_name, sizeof(glyph_name) - 1);


Note that glyph names are not required to be unique, and FontForge does not enforce them being unique either (you can have two glyphs with the same name in FontForge). You probably need to maintain a map between splinechar indices and generated font indices (I think they are usually the same, but FontForge might be re-ordering some glyphs when generating the font).

It looks to me that FF does enforce unique names, at least this is what I got when trying to rename a glyph to existing name:

Do you have in mind some specific scenario where this doesn't hold?

Here is an SFD file with duplicate glyph names (two glyphs both named test), FontForge will open it without any complaint and it will also generate a font with the duplicate glyph name: test.sfd.zip

khaledhosny · 2025-01-10T10:06:44Z

fontforgeexe/shapers/harfbuzz.cpp

+    char* utf8_str = u2utf8_copy(u_vec.data());
+
+    hb_buffer_t* hb_buffer = hb_buffer_create();
+    hb_buffer_add_utf8(hb_buffer, utf8_str, -1, 0, -1);


You can use hb_buffer_add_utf32(), instead of converting to UTF-8 as HarfBuzz will internally convert it back to UTF-32 anyway.

khaledhosny · 2025-01-10T10:58:49Z

fontforgeexe/shapers/harfbuzz.cpp

+    hb_buffer_set_direction(hb_buffer, hb_dir);
+
+    // Shape the text
+    hb_shape(hb_ttf_font, hb_buffer, NULL, 0);


Features are never passed to HarfBuzz, so any non-default features will not be applied.

In general, default features are always applied and selecting/unselecting features on the features list has no effect.

khaledhosny · 2025-01-10T11:05:32Z

fontforgeexe/shapers/harfbuzz.cpp

+        hb_glyph_extents_t extents;
+        hb_bool_t res = hb_font_get_glyph_extents(
+            hb_ttf_font, glyph_info.codepoint, &extents);
+        assert(res);


Calculating glyph extents can be slow (especially for CFF fonts), and I don’t see extents used anywhere so it seems wasteful.

khaledhosny · 2025-01-10T11:07:31Z

fontforgeexe/shapers/harfbuzz.cpp

+
+        // Fill unscaled metrics in font units
+        hb_position_t h_advance =
+            hb_font_get_glyph_h_advance(hb_ttf_font, glyph_info.codepoint);


You can get the original width from splinechar, shaping does not change it.

khaledhosny · 2025-01-10T11:17:17Z

fontforgeexe/shapers/harfbuzz.cpp

+    hb_language_t hb_lang = hb_language_from_string((const char*)lang, -1);
+    hb_buffer_set_language(hb_buffer, hb_lang);
+
+    bool rtl = !u_vec.empty() && isrighttoleft(u_vec[0]);


Checking first character is not enough, for example if text starts with punctuation. HarfBuzz has hb_buffer_guess_segment_properties which works slightly better here.

Ideally, there should be a UI to set the direction, since it is possible the text is entirely composed of unencoded glyphs which will be given PUA encoding and all PUA codepoints has strong LTR direction.

khaledhosny · 2025-01-10T11:32:37Z

fontforgeexe/shapers/harfbuzz.cpp

+
+    _WriteTTFFont(
+        ttf_file, context_->sf, ff_ttf, NULL, bf_ttf,
+        ttf_flag_otmode | ttf_flag_oldkernmappedonly | ttf_flag_fake_map,


I don’t think ttf_flag_oldkernmappedonly does anything unless ttf_flag_oldkern is also set. But you don’t want old kern table anyway.

khaledhosny · 2025-01-10T11:39:13Z

fontforgeexe/shapers/harfbuzz.cpp

+    _WriteTTFFont(
+        ttf_file, context_->sf, ff_ttf, NULL, bf_ttf,
+        ttf_flag_otmode | ttf_flag_oldkernmappedonly | ttf_flag_fake_map,
+        context_->get_enc_map(context_->sf), ly_fore);


You can probably speed things up by skipping generating tables that HarfBuzz does not use, e.g. glyph outlines (glyf and CFF table) which is usually the larges font tables and the slowest to build.

khaledhosny · 2025-01-10T11:56:48Z

Selecting builtin shaper always crashes for me.

iorsh · 2025-01-18T21:57:30Z

Windows CI fixed with jtanx/fontforgebuilds#23

khaledhosny · 2025-01-26T16:03:40Z

fontforgeexe/shapers/harfbuzz.cpp

+                             HB_FEATURE_GLOBAL_END};
+        hb_feature_vec.push_back(hb_feat);
+    }
+    return hb_feature_vec;


Some work is needed to handle features that are enabled by default. Features like init, medi, etc. are enabled by default and HarfBuzz applies them selectively based on text analysis, but when enabled manually they will be applied unconditionally which will break their intended use.

What should happen is that if a default feature is selected in the UI, it should not be in the features list, but if it is unselected it should be in the features list with value set to 0 (disable).

HarfBuzz does not have an API that tells which features are enabled by default, so you will have to check the HarfBuzz sources. The list of default features depend on the script, I don’t know how FontForge decides which features to enable by default, but I think you will have to sync that with HarfBuzz so that features pre-selected in the UI match what HarfBuzz does.

Some applications use a tri-state checkbox for features, the default is neither selected or unselected what omits the feature from the features list, and selected set the feature to 1 and unselcted sets it to 0.

The HarfBuzz sources are still the best reference, but here's a quick guide based on recent research on which features are considered enabled by default "in general": https://w3c.github.io/IFT/Overview.html#feature-tag-list

I added support for default feature list (copied from https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-subset-input.cc#L82), but couldn't see any differentiation by script. @khaledhosny, is there any additional place for default features per script?

iorsh · 2025-01-28T21:01:49Z

Selecting builtin shaper always crashes for me.

@khaledhosny, I tested it with a few fonts, but couldn't reproduce. Could you please attach a crash stack, or the font file if possible?

khaledhosny · 2025-01-29T00:36:46Z

Selecting builtin shaper always crashes for me.

@khaledhosny, I tested it with a few fonts, but couldn't reproduce. Could you please attach a crash stack, or the font file if possible?

It happens with any font, when ff::shapers::IShaper::~IShaper() is called, this warning seems to be relevant:

/fontforge/fontforgeexe/shapers/shaper_shim.cpp:75:5: warning: delete called on 'ff::shapers::IShaper' that is abstract but has non-virtual destructor [-Wdelete-abstract-non-virtual-dtor]
   75 |     delete shaper;

This patch makes the warning and the crash go away:

diff --git a/fontforgeexe/shapers/i_shaper.hpp b/fontforgeexe/shapers/i_shaper.hpp
index 1884b05c1..f44e6f3cb 100644
--- a/fontforgeexe/shapers/i_shaper.hpp
+++ b/fontforgeexe/shapers/i_shaper.hpp
@@ -36,6 +36,8 @@ namespace ff::shapers {

 class IShaper {
  public:
+    virtual ~IShaper() = default;
+
     // The internal shaper name is non-localizable and serves to identify the
     // shaper in the system.
     virtual const char* name() const = 0;

iorsh added 30 commits November 8, 2024 12:43

Add HarfBuzz to CMake with PkgConfig

4883503

Make HarfBuzz dependency optional

0289358

shapers subdir; sample HB library call

8a06d1d

clang-format

6709139

Add struct ShaperDef, func get_shaper_defs()

444ce0c

Add get_default_shaper()

1d3c499

Add menu items for shapers

7b687cb

IShaper interface with dummy implementations HarfBuzzShaper, BuiltInS…

25fe717

…haper

C functions for shaper objects

fbbde28

Add and activate dummy shapers in Metrics view

3cd8b9d

Add class Tag

90c5aec

Add C struct ShaperContext

76b37d0

Refresh in MVSetShaper()

3db1b81

Legacy call ApplyTickedFeatures() from shaper_apply_features() with C…

0b2ffb7

…++ framework

Extract splinechar.h

50a8fed

Build font and invoke HarfBuzz shaper

b472af1

Extract ShapeMetrics from struct metricchar

f71578e

Extract ShapeMetrics to metrics.h

737c477

Manage MetricsView::metrics buffer memory inside shaper

824f189

Extract MVCharWidth()

bd3f99f

Move legacy metrics scaling code into BuiltInShaper::scale_metrics()

c09b7c8

Fix shadowed var

6ec2540

Calculate metrics from HarfBuzz output

f796a07

Fix crash

f360469

Fix TTF generation for HarfBuzz

2ad2169

Support RTL shaping in HarfBuzz

00f493c

Extract HarfBuzzShaper::extract_shaped_data()

2b7bb9d

Extract HarfBuzzShaper::reverse_rtl_metrics()

42a668e

Fix vertical metrics in RTL shaping

f663eab

Allow ApplyTickedFeatures() without GSUB

aa6b4db

iorsh added 12 commits December 28, 2024 01:09

Suppress const warning

6387322

Use GFileTmpfile() in HarfBuzz shaper

ea52dce

Extract splinefont_enums.h

ceab0ea

Fix crash due to unitialized ShaperContext member

a10882b

Extract SFFakeUnicodeBase()

209dfa1

Show unmapped glyphs in HarfBuzz with fake encoding

cbe2bc4

[TEMP] Force ubuntu-22.04 CI runner

66e191d

Remove unused code from MVCreateFields()

3b394e8

Support vertical metrics in HarfBuzz shaper

9034f42

Merge branch 'master' into harfbuzz_metrics

18681b7

Typo

107afbb

Revert "[TEMP] Force ubuntu-22.04 CI runner"

cd075bf

This reverts commit 66e191d.

khaledhosny reviewed Jan 10, 2025

View reviewed changes

iorsh added 4 commits January 19, 2025 00:04

Remove unused hb_font_get_glyph_extents() call

5943d34

Use hb_buffer_add_utf32()

1f91cbd

Propagate selected features to HarfBuzz shaper

6d24bea

Use SplineChar::width

0938f9d

khaledhosny reviewed Jan 26, 2025

View reviewed changes

iorsh added 4 commits January 28, 2025 20:53

Add HarfBuzzShaper::default_features_ list

4ac95df

Smart application of OT features in HarfBuzz

f037a7e

Check RTL with hb_buffer_guess_segment_properties()

263ea8b

Fix _WriteTTFFont() flags

9b5002e

iorsh added 3 commits January 29, 2025 12:28

virtual ~IShaper()

976b798

Add virtual IShaper::default_features()

3288fd6

Determine default enabled features by shaper

12b3488

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Harfbuzz in Metrics View #5522

Support Harfbuzz in Metrics View #5522

iorsh commented Jan 9, 2025

iorsh commented Jan 9, 2025

khaledhosny Jan 10, 2025

iorsh Jan 25, 2025

khaledhosny Jan 28, 2025

khaledhosny Jan 10, 2025

iorsh Jan 25, 2025

khaledhosny Jan 26, 2025

khaledhosny Jan 10, 2025

khaledhosny Jan 10, 2025

khaledhosny Jan 10, 2025

khaledhosny Jan 10, 2025

khaledhosny Jan 10, 2025

khaledhosny Jan 10, 2025 •

edited

Loading

khaledhosny Jan 10, 2025

khaledhosny Jan 10, 2025

khaledhosny commented Jan 10, 2025

iorsh commented Jan 18, 2025

khaledhosny Jan 26, 2025

skef Jan 26, 2025

iorsh Jan 29, 2025

iorsh commented Jan 28, 2025

khaledhosny commented Jan 29, 2025

Support Harfbuzz in Metrics View #5522

Are you sure you want to change the base?

Support Harfbuzz in Metrics View #5522

Conversation

iorsh commented Jan 9, 2025

iorsh commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khaledhosny Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khaledhosny commented Jan 10, 2025

iorsh commented Jan 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iorsh commented Jan 28, 2025

khaledhosny commented Jan 29, 2025

khaledhosny Jan 10, 2025 •

edited

Loading