Subsetting fails on certain fonts

[jackyjjc-canva]

$ hb-subset GlowSansSC-Normal-Bold.otf --text=abc -o glowsansj.subset.otf
error: Operation failed. Probably a bug. File github issue.

The font was an open source font downloaded from https://github.com/welai/glow-sans/releases. (GlowSansSC-Normal-Bold.otf from "GlowSansSC-Normal-v0.93.zip")

It might be related to this previous issue since the error is the same: #3959

[behdad]

Thanks. Unlikely to be the same issue. The error message is generic.

[martyla]

Was digging into this a little bit... It looks like a regression introduced in the 8.0.0 release (hb-subset command succeeds for release <=7.3.0). It also succeeds in >=8.0.0 if I disable CFF subsetting (via setting cpp flag -DHB_NO_SUBSET_CFF)

Seems to have an issue subsetting the CFF table:

harfbuzz/src/hb-subset.cc

Line 510 in bc90b29

case HB_OT_TAG_CFF1: return _subset<const OT::cff1> (plan, buf);

Debugging with -DHB_DEBUG_SUBSET=100:

$ ./build/util/hb-subset GlowSansSC-Normal-Bold.otf "abc" -o out.otf
SUBSET                          ├╴: subset BASE
SUBSET                          ├╴: OT::BASE initial estimated table size: 8194 bytes.
... <removed for brevity>
SUBSET    (     0x10419011c)  1 │├╯OT::BASE::subset: return true (line 779)
SUBSET                          ├╴: OT::BASE final subset table size: 222 bytes.
SUBSET                          ├╴: add table BASE, dest 222 bytes, source 222 bytes
SUBSET                          ├╴: OT::BASE::subset success
SUBSET                          ├╴: subset CFF
SUBSET                          ├╴: OT::CFF ::subset sanitize failed on source table.
error: Operation failed. Probably a bug. File github issue.

Unfortunately I'm not familiar enough yet with the code to understand how to fix this

[behdad]

Can you share the font with us?

[martyla]

GlowSansSC-Normal-v0.93.zip -> GlowSansSC-Normal-Bold.otf

[behdad]

Reproduces for me. Thanks. @garretrieger can you look into it?

[behdad]

This is curious:

SUBSET                          ├╴: OT::CFF ::subset sanitize failed on source table.

[behdad]

Indeed, hb-view doesn't show anything either. So the faulty code is on the CFF sanitize side. Time for a huge bisect.

[behdad]

Okay; the offending commit is: 43ec78f

[behdad]

Okay; the offending commit is: 43ec78f

Nah. The bug is older. 7.3.0 was creating an empty subset, ie. CFF sanitize was failing back then as well. It's just that the commit above exposed the sanitization fail. We probably should print a better error message there.

[behdad]

I checked as far back as 4.0.0; the bug is there. I don't think it's a regression.

[behdad]

Okay. Looks like the font uses operators (like FontBBox) in FontDicts, whereas those only belong to TopDict...

FontTools also detects, and ignores, that:

          </Private> 
          <!-- some keys were ignored: FontBBox ItalicAngle StrokeWidth UnderlinePosition UnderlineThickness isFixedPitch --> 
        </FontDict> 
      </FDArray> 

I think we should simply ignore those. cc @jfkthame

[behdad]

This does it:

diff --git a/src/hb-cff-interp-dict-common.hh b/src/hb-cff-interp-dict-common.hh
index a08b10b5f..fc128e4da 100644
--- a/src/hb-cff-interp-dict-common.hh
+++ b/src/hb-cff-interp-dict-common.hh
@@ -71,6 +71,25 @@ struct dict_opset_t : opset_t<number_t>
        env.argStack.push_real (parse_bcd (env.str_ref));
        break;
 
+      case OpCode_isFixedPitch:
+      case OpCode_ItalicAngle:
+      case OpCode_UnderlinePosition:
+      case OpCode_UnderlineThickness:
+      case OpCode_PaintType:
+      case OpCode_CharstringType:
+      case OpCode_UniqueID:
+      case OpCode_StrokeWidth:
+      case OpCode_SyntheticBase:
+      case OpCode_CIDFontVersion:
+      case OpCode_CIDFontRevision:
+      case OpCode_CIDFontType:
+      case OpCode_UIDBase:
+      case OpCode_FontBBox:
+      case OpCode_XUID:
+      case OpCode_BaseFontBlend:
+       env.clear_args ();
+       break;
+
       default:
        opset_t<number_t>::process_op (op, env);
        break;

I'm not sure which of those we should include though. I copied from TopDict...

[behdad]

Or we can change the catch-all to not err in general. I think that's a better way to handle broken fonts.

[behdad]

Or we can change the catch-all to not err in general. I think that's a better way to handle broken fonts.

Using a hammer:

diff --git a/src/hb-cff-interp-common.hh b/src/hb-cff-interp-common.hh
index 1d1f10f2b..6ca7500af 100644
--- a/src/hb-cff-interp-common.hh
+++ b/src/hb-cff-interp-common.hh
@@ -624,7 +624,6 @@ struct opset_t
        } else {
          /* invalid unknown operator */
          env.clear_args ();
-         env.set_error ();
        }
        break;
     }

[martyla]

Thanks for your help on this @behdad, appreciate it! 😄

[behdad]

I'm curious how those fonts were built.