Update harfbuzzjs to ^0.2.0 (harfbuzz 3.0.0) #13

papandreou · 2021-11-04T10:09:33Z

Discussion here: #12

coveralls · 2021-11-04T10:10:17Z

Pull Request Test Coverage Report for Build 1443083249

13 of 14 (92.86%) changed or added relevant lines in 1 file are covered.
5 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-12.1%) to 87.879%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
index.js	13	14	92.86%

Files with Coverage Reduction	New Missed Lines	%
index.js	5	87.88%

Totals
Change from base Build 997199723:	-12.1%
Covered Lines:	48
Relevant Lines:	54

💛 - Coveralls

ebraminio · 2021-11-04T10:12:18Z

index.js

@@ -57,7 +60,7 @@ async function subsetFont(
    exports.hb_set_add(inputUnicodes, c.codePointAt(0));
  }

-  const subset = exports.hb_subset(face, input);
+  const subset = exports.hb_subset_or_fail(face, input);


now that it can return zero, feel free to check it and fail gracefully if it returns 0.

also maybe on hb_subset_input_create_or_fail or any API named '_or_fail` meaning they can return zero, but it is very unlikely.

That makes sense! Is there a way to get a better error than "it failed" when that happens?

Oh, if the convention here is to return exception as below, that can be done here also, it should be unlikely anyway, still.

Something like bb2a0c5?

The try { throw... } finally { ... } construct will see to it that we destroy the input, but don't execute the const result = exports.hb_face_reference_blob(subset); line?

something like that, now I regret bringing it up! a cleanup just like below if (subsetByteLength === 0) { block would be cool I think :)

But I already did implement it like that? Doesn't it look OK as-is? 😼

I meant I think before throw these are needed also,

exports.hb_face_destroy(face); exports.free(fontBuffer);

Ah, I get it, fixed in e2deb41

#13 (comment)

ebraminio · 2021-11-05T02:40:54Z

index.js

@@ -43,9 +43,17 @@ async function subsetFont(
  exports.hb_blob_destroy(blob);

  const input = exports.hb_subset_input_create_or_fail();
+  if (input === 0) {
+    throw new Error(


This also can benefit those memory cleanups just for the sake completeness, thanks you very much :)

Oh yeah, moved some stuff around so the other things get allocated after input instead: 2e3baa2

papandreou · 2021-11-05T10:28:23Z

@ebraminio, thanks for the help! I'm still a bit worried about that failing reference image test in the subfont test suite, so I tried creating the subset font before/after upgrading to harfbuzzjs 0.2.0. Here's the diff of the ttx representations as well as the files themselves: https://gist.github.com/papandreou/cca1b1a17adb7f60b1f7794bea608f6f

Any idea what's going on?

Rendering of the original web page (or when subset with harfbuzzjs 0.1.5)

Rendering of the web page when subset with harfbuzzjs 0.2.0

subfont --debug says that these glyphs are being included in the subset, in case that helps:

[
  {
    assetFileName: 'testdata/referenceImages/fontVariant/index.html',
    fontUsages: [
      {
        smallestOriginalSize: 77420,
        smallestOriginalFormat: 'woff',
        texts: [
          '   a á ă â ä ạ à ả ā ą å ǻ ã ắ ặ ằ ẳ ẵ ấ ậ ầ ẩ ẫ ğ ĝ ģ ġ ß α ά а ӓ ӑ ¯ ˙ ¨ ˝ ´ ` ˆ ˇ ˚ ΄ ̉     g g 0 0 ˜ ˜ ˜ ˘ ˘ ˘ ',
          '   0 1 2 3 4 5 6 7 8 9 ',
          '   0 1 2 3 4 5 6 7 8 9 /   ⁄ ₀ ₁ ₂ ₃ ₄ ₅ ₆ ₇ ₈ ₉   ² ³ ¹ ⁰ ⁴ ⁵ ⁶ ⁷ ⁸ ⁹   ⁄² ⁄³ ⁄¹ ⁄⁰ ⁄⁴ ⁄⁵ ⁄⁶ ⁄⁷ ⁄⁸ ⁄⁹ ₀² ₀³ ₀¹ ₀⁰ ₀⁴ ₀⁵ ₀⁶ ₀⁷ ₀⁸ ₀⁹ ₁² ₁³ ₁¹ ₁⁰ ₁⁴ ₁⁵ ₁⁶ ₁⁷ ₁⁸ ₁⁹ ₂² ₂³ ₂¹ ₂⁰ ₂⁴ ₂⁵ ₂⁶ ₂⁷ ₂⁸ ₂⁹ ₃² ₃³ ₃¹ ₃⁰ ₃⁴ ₃⁵ ₃⁶ ₃⁷ ₃⁸ ₃⁹ ₅² ₅³ ₅¹ ₅⁰ ₅⁴ ₅⁵ ₅⁶ ₅⁷ ₅⁸ ₅⁹ ₆² ₆³ ₆¹ ₆⁰ ₆⁴ ₆⁵ ₆⁶ ₆⁷ ₆⁸ ₆⁹ ₇² ₇³ ₇¹ ₇⁰ ₇⁴ ₇⁵ ₇⁶ ₇⁷ ₇⁸ ₇⁹ ₈² ₈³ ₈¹ ₈⁰ ₈⁴ ₈⁵ ₈⁶ ₈⁷ ₈⁸ ₈⁹ ₉² ₉³ ₉¹ ₉⁰ ₉⁴ ₉⁵ ₉⁶ ₉⁷ ₉⁸ ₉⁹ ',
          '   a o ',
          '   a g 0 á ă â ä ạ à ả ā ą å ǻ ã ắ ặ ằ ẳ ẵ ấ ậ ầ ẩ ẫ ğ ĝ ģ ġ ß α ά а ӓ ӑ ',
          '   a á ă â ä ạ à ả ā ą å ǻ ã ắ ặ ằ ẳ ẵ ấ ậ ầ ẩ ẫ α ά а ӓ ӑ ',
          '   g ğ ĝ ģ ġ ',
          '   0 ',
          '   ß '
        ],
        pageText: ' /0123456789`ago¨¯²³´¹ßàáâãäåāăąĝğġģǻˆˇ˘˙˚˜˝̉΄άαаӑӓạảấầẩẫậắằẳẵặ⁄⁰⁴⁵⁶⁷⁸⁹₀₁₂₃₄₅₆₇₈₉',
        text: ' /0123456789`ago¨¯²³´¹ßàáâãäåāăąĝğġģǻˆˇ˘˙˚˜˝̉΄άαаӑӓạảấầẩẫậắằẳẵặ⁄⁰⁴⁵⁶⁷⁸⁹₀₁₂₃₄₅₆₇₈₉',
        props: {
          'font-stretch': 'normal',
          'font-weight': 'normal',
          'font-style': 'normal',
          'font-family': 'IBMPlexSans',
          src: 'url(IBMPlexSans-Regular.woff) format("woff")',
          'font-display': 'swap'
        },
        fontUrl: 'file:///home/andreas/work/subfont/testdata/referenceImages/fontVariant/IBMPlexSans-Regular.woff',
        fontFamilies: Set { 'IBMPlexSans' },
        preload: true,
        codepoints: {
          original: [
              0,  13,  32,  33,  34,  35,  36,  37,  38,  39,  40,  41,
             42,  43,  44,  45,  46,  47,  48,  49,  50,  51,  52,  53,
             54,  55,  56,  57,  58,  59,  60,  61,  62,  63,  64,  65,
             66,  67,  68,  69,  70,  71,  72,  73,  74,  75,  76,  77,
             78,  79,  80,  81,  82,  83,  84,  85,  86,  87,  88,  89,
             90,  91,  92,  93,  94,  95,  96,  97,  98,  99, 100, 101,
            102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113,
            114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125,
            126, 160, 161, 162,
            ... 727 more items
          ],
          used: [
               32,    47,   48,   49,   50,   51,   52,   53,   54,
               55,    56,   57,   96,   97,  103,  111,  168,  175,
              178,   179,  180,  185,  223,  224,  225,  226,  227,
              228,   229,  257,  259,  261,  285,  287,  289,  291,
              507,   710,  711,  728,  729,  730,  732,  733,  777,
              900,   940,  945, 1072, 1233, 1235, 7841, 7843, 7845,
             7847,  7849, 7851, 7853, 7855, 7857, 7859, 7861, 7863,
             8260,  8304, 8308, 8309, 8310, 8311, 8312, 8313, 8320,
             8321,  8322, 8323, 8324, 8325, 8326, 8327, 8328, 8329,
            63191, 63192
          ],
          unused: [
              0,  13,  33,  34,  35,  36,  37,  38,  39,  40,  41,  42,
             43,  44,  45,  46,  58,  59,  60,  61,  62,  63,  64,  65,
             66,  67,  68,  69,  70,  71,  72,  73,  74,  75,  76,  77,
             78,  79,  80,  81,  82,  83,  84,  85,  86,  87,  88,  89,
             90,  91,  92,  93,  94,  95,  98,  99, 100, 101, 102, 104,
            105, 106, 107, 108, 109, 110, 112, 113, 114, 115, 116, 117,
            118, 119, 120, 121, 122, 123, 124, 125, 126, 160, 161, 162,
            163, 164, 165, 166, 167, 169, 170, 171, 172, 173, 174, 176,
            177, 181, 182, 183,
            ... 644 more items
          ],
          page: [
               32,    47,   48,   49,   50,   51,   52,   53,   54,
               55,    56,   57,   96,   97,  103,  111,  168,  175,
              178,   179,  180,  185,  223,  224,  225,  226,  227,
              228,   229,  257,  259,  261,  285,  287,  289,  291,
              507,   710,  711,  728,  729,  730,  732,  733,  777,
              900,   940,  945, 1072, 1233, 1235, 7841, 7843, 7845,
             7847,  7849, 7851, 7853, 7855, 7857, 7859, 7861, 7863,
             8260,  8304, 8308, 8309, 8310, 8311, 8312, 8313, 8320,
             8321,  8322, 8323, 8324, 8325, 8326, 8327, 8328, 8329,
            63191, 63192
          ]
        },
        smallestSubsetSize: 6752,
        smallestSubsetFormat: 'woff2'
      }
    ]
  }
]

…_input_create_or_fail returns 0 #13 (comment)

ebraminio · 2021-11-05T15:29:44Z

Any idea what's going on?

Unfortunately I'm no longer involved with the upstream :/ comparing fonttools's ttx result of the two should give a good clue.

papandreou · 2021-11-06T12:07:08Z

@ebraminio, oh, sorry to hear that 😿

The above gist does include a ttx diff, but it's rather big, so it isn't clear to me what the actual issue might be. I guess I'll spend some time looking at it and maybe report it upstream if I can reproduce it with the hb_subset binary.

papandreou · 2021-11-09T23:42:58Z

Hmm, I can reproduce it with just the hb-subset binary wired into subfont instead. That setup also reproduces it with harfbuzz 3.1.1 and 3.0.0, but 2.8.1 (which harfbuzzjs was using before) is fine.

git bisect says that the behavior changed in harfbuzz/harfbuzz@cb5a6b5:

cb5a6b5a27cfe616113bafe7f23ad33f1b0d0a1e is the first bad commit
commit cb5a6b5a27cfe616113bafe7f23ad33f1b0d0a1e
Author: Qunxin Liu <[email protected]>
Date:   Wed May 19 17:33:46 2021 -0700

    [subset] support option --layout-features

Passing --layout-features=aalt,dnom,frac,kern fixes it for the particular test case, but I guess we should just pass --layout-features=* as we did to pyftsubset -- that also works and should be safer.

We can ape what hb-subset --layout-features=* does:

diff --git a/index.js b/index.js
index ba08ff2..e079161 100644
--- a/index.js
+++ b/index.js
@@ -49,6 +49,14 @@ async function subsetFont(
   const face = exports.hb_face_create(blob, 0);
   exports.hb_blob_destroy(blob);
 
+  // Do the equivalent of --font-features=*
+  const layoutFeatures = exports.hb_subset_input_set(
+    input,
+    6 // HB_SUBSET_SETS_LAYOUT_FEATURE_TAG
+  );
+  exports.hb_set_clear(layoutFeatures);
+  exports.hb_set_invert(layoutFeatures);
+
   if (preserveNameIds) {
     const inputNameIds = exports.hb_subset_input_set(
       input,

... but that requires harfbuzzjs to expose hb_set_clear and hb_set_invert.

ebraminio · 2021-11-10T06:54:58Z

but that requires harfbuzzjs to expose hb_set_clear and hb_set_invert.

just published 0.2.1 with them :)

papandreou · 2021-11-10T07:32:55Z

Released in 1.4.0. Thanks for all the help, @ebraminio 🤗

papandreou added 2 commits November 4, 2021 10:27

Update harfbuzzjs to ^0.2.0

4e8757e

Adapt to the harfbuzz 3 api

68ce78c

#12 (comment)

papandreou self-assigned this Nov 4, 2021

papandreou mentioned this pull request Nov 4, 2021

Upgrade harfbuzzjs 0.2.0 / harfbuzz 3.0.0 #12

Closed

ebraminio reviewed Nov 4, 2021

View reviewed changes

papandreou added 2 commits November 4, 2021 11:21

Handle a return value of 0 from the _or_fail functions

bb2a0c5

#13 (comment)

Destroy face and free fontBuffer whwn hb_subset_or_fail fails

e2deb41

#13 (comment)

ebraminio approved these changes Nov 5, 2021

View reviewed changes

ebraminio reviewed Nov 5, 2021

View reviewed changes

Move allocations around so there's nothing to clean up when hb_subset…

2e3baa2

…_input_create_or_fail returns 0 #13 (comment)

papandreou mentioned this pull request Nov 9, 2021

Expose hb_set_{clear,invert} in hb-subset.wasm harfbuzz/harfbuzzjs#49

Closed

papandreou added 2 commits November 10, 2021 08:29

Update harfbuzzjs to ^0.2.1

a8786cb

Do the equivalent of --font-features=*

d1275c5

papandreou merged commit 3f39e6e into master Nov 10, 2021

papandreou deleted the feature/harfbuzz3 branch November 10, 2021 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update harfbuzzjs to ^0.2.0 (harfbuzz 3.0.0) #13

Update harfbuzzjs to ^0.2.0 (harfbuzz 3.0.0) #13

papandreou commented Nov 4, 2021 •

edited

Loading

coveralls commented Nov 4, 2021 •

edited

Loading

ebraminio Nov 4, 2021

ebraminio Nov 4, 2021

papandreou Nov 4, 2021

ebraminio Nov 4, 2021

papandreou Nov 4, 2021

papandreou Nov 4, 2021

ebraminio Nov 4, 2021 •

edited

Loading

papandreou Nov 4, 2021

ebraminio Nov 4, 2021

papandreou Nov 5, 2021

ebraminio Nov 5, 2021

papandreou Nov 5, 2021

papandreou commented Nov 5, 2021 •

edited

Loading

ebraminio commented Nov 5, 2021

papandreou commented Nov 6, 2021

papandreou commented Nov 9, 2021 •

edited

Loading

ebraminio commented Nov 10, 2021 •

edited

Loading

papandreou commented Nov 10, 2021

Update harfbuzzjs to ^0.2.0 (harfbuzz 3.0.0) #13

Update harfbuzzjs to ^0.2.0 (harfbuzz 3.0.0) #13

Conversation

papandreou commented Nov 4, 2021 • edited Loading

coveralls commented Nov 4, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1443083249

💛 - Coveralls

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

ebraminio Nov 4, 2021 • edited Loading

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

papandreou commented Nov 5, 2021 • edited Loading

Rendering of the original web page (or when subset with harfbuzzjs 0.1.5)

Rendering of the web page when subset with harfbuzzjs 0.2.0

ebraminio commented Nov 5, 2021

papandreou commented Nov 6, 2021

papandreou commented Nov 9, 2021 • edited Loading

ebraminio commented Nov 10, 2021 • edited Loading

papandreou commented Nov 10, 2021

papandreou commented Nov 4, 2021 •

edited

Loading

coveralls commented Nov 4, 2021 •

edited

Loading

ebraminio Nov 4, 2021 •

edited

Loading

papandreou commented Nov 5, 2021 •

edited

Loading

papandreou commented Nov 9, 2021 •

edited

Loading

ebraminio commented Nov 10, 2021 •

edited

Loading