Merge pull request #3933 from googlefonts/cff

[subset] Fix infinite loop when instancing CFF fonts

src/hb-subset-input.cc

@@ -403,6 +403,9 @@ hb_subset_input_get_user_data (const hb_subset_input_t *input,
  *
  * Pin an axis to its default location in the given subset input object.
  *
+ * Currently only works for fonts with 'glyf' tables. CFF and CFF2 is not
+ * yet supported. Additionally all axes in a font must be pinned.
+ *
  * Return value: `true` if success, `false` otherwise
  *
  * Since: REPLACEME
@@ -427,6 +430,9 @@ hb_subset_input_pin_axis_to_default (hb_subset_input_t  *input,
  *
  * Pin an axis to a fixed location in the given subset input object.
  *
+ * Currently only works for fonts with 'glyf' tables. CFF and CFF2 is not
+ * yet supported. Additionally all axes in a font must be pinned.
+ *
  * Return value: `true` if success, `false` otherwise
  *
  * Since: REPLACEME

src/hb-subset.cc

@@ -413,20 +413,14 @@ _passthrough (hb_subset_plan_t *plan, hb_tag_t tag)
 
 static bool
 _dependencies_satisfied (hb_subset_plan_t *plan, hb_tag_t tag,
-                         hb_set_t &visited_set, hb_set_t &revisit_set)
+                         const hb_set_t &subsetted_tags,
+                         const hb_set_t &pending_subset_tags)
 {
   switch (tag)
   {
   case HB_OT_TAG_hmtx:
   case HB_OT_TAG_vmtx:
-    if (!plan->pinned_at_default &&
-        !visited_set.has (HB_OT_TAG_glyf))
-    {
-      revisit_set.add (tag);
-      return false;
-    }
-    return true;
-
+    return plan->pinned_at_default || !pending_subset_tags.has (HB_OT_TAG_glyf);
   default:
     return true;
   }
@@ -572,41 +566,58 @@ hb_subset_plan_execute_or_fail (hb_subset_plan_t *plan)
     return nullptr;
   }
 
-  hb_set_t tags_set, revisit_set;
-  bool success = true;
   hb_tag_t table_tags[32];
   unsigned offset = 0, num_tables = ARRAY_LENGTH (table_tags);
-  hb_vector_t<char> buf;
-  buf.alloc (4096 - 16);
 
+  hb_set_t subsetted_tags, pending_subset_tags;
   while (((void) _get_table_tags (plan, offset, &num_tables, table_tags), num_tables))
   {
     for (unsigned i = 0; i < num_tables; ++i)
     {
       hb_tag_t tag = table_tags[i];
-      if (_should_drop_table (plan, tag) && !tags_set.has (tag)) continue;
-      if (!_dependencies_satisfied (plan, tag, tags_set, revisit_set)) continue;
-      tags_set.add (tag);
+      if (_should_drop_table (plan, tag)) continue;
+      pending_subset_tags.add (tag);
+    }
+
+    offset += num_tables;
+  }
+
+  hb_vector_t<char> buf;
+  buf.alloc (4096 - 16);
+
+
+  bool success = true;
+
+  while (!pending_subset_tags.is_empty ())
+  {
+    bool made_changes = false;
+    for (hb_tag_t tag : pending_subset_tags)
+    {
+
+      if (!_dependencies_satisfied (plan, tag,
+                                    subsetted_tags,
+                                    pending_subset_tags))
+      {
+        // delayed subsetting for some tables since they might have dependency on other tables
+        // in some cases: e.g: during instantiating glyf tables, hmetrics/vmetrics are updated
+        // and saved in subset plan, hmtx/vmtx subsetting need to use these updated metrics values
+        continue;
+      }
+
+      pending_subset_tags.del (tag);
+      subsetted_tags.add (tag);
+      made_changes = true;
+
       success = _subset_table (plan, buf, tag);
       if (unlikely (!success)) goto end;
     }
 
-    /*delayed subsetting for some tables since they might have dependency on other tables in some cases:
-    e.g: during instantiating glyf tables, hmetrics/vmetrics are updated and saved in subset plan,
-    hmtx/vmtx subsetting need to use these updated metrics values*/
-    while (!revisit_set.is_empty ())
+    if (!made_changes)
     {
-      hb_set_t revisit_temp;
-      for (hb_tag_t tag : revisit_set)
-      {
-        if (!_dependencies_satisfied (plan, tag, tags_set, revisit_temp)) continue;
-        tags_set.add (tag);
-        success = _subset_table (plan, buf, tag);
-        if (unlikely (!success)) goto end;
-      }
-      revisit_set = revisit_temp;
+      DEBUG_MSG (SUBSET, nullptr, "Table dependencies unable to be satisfied. Subset failed.");
+      success = false;
+      goto end;
     }
-    offset += num_tables;
   }
 
   if (success && plan->attach_accelerator_data) {

test/subset/data/Makefile.sources

@@ -47,6 +47,10 @@ TESTS = \
 	tests/math.tests \
 	tests/math_coverage_offset.tests \
 	tests/post.tests \
+	tests/full_instance.tests \
+	tests/instance_feature_variations.tests \
+	tests/instantiate_glyf.tests \
+	tests/pin_all_at_default.tests \
 	$(NULL)
 
 # TODO: re-enable once colrv1 subsetting is stabilized.
@@ -59,8 +63,4 @@ XFAIL_TESTS = \
 
 # Disabled because instancing is only available w/ experimental API on.
 DISABLED_TESTS = \
-	tests/full_instance.tests \
-	tests/instance_feature_variations.tests \
-	tests/instantiate_glyf.tests \
-	tests/pin_all_at_default.tests \
 	$(NULL)

test/subset/generate-expected-outputs.py

@@ -35,7 +35,6 @@ def generate_expected_output(input_file, unicodes, profile_flags, instance_flags
 			"--no-overlap-flag",
 			"--no-recalc-bounds",
 			"--no-recalc-timestamp",
-			"--no-harfbuzz-repacker", # disable harfbuzz repacker so we aren't comparing to ourself.
 			"--output=%s" % instance_path,
 			input_file]
 		args.extend(instance_flags)

test/subset/meson.build

@@ -49,18 +49,12 @@ tests = [
   'glyph_names',
   'post',
   '32bit_var_store',
+  'pin_all_at_default',
+  'instantiate_glyf',
+  'full_instance',
+  'instance_feature_variations',
 ]
 
-if get_option('experimental_api')
-  # instacing tests, only supported in experimental api
-  tests += [
-    'pin_all_at_default',
-    'instantiate_glyf',
-    'full_instance',
-    'instance_feature_variations',
-  ]
-endif
-
 repack_tests = [
   'basic',
   'prioritization',
@@ -70,7 +64,6 @@ repack_tests = [
   'space_splitting',
 ]
 
-
 run_test = find_program('run-tests.py')
 
 foreach t : tests

util/hb-subset.cc

@@ -660,7 +660,6 @@ parse_drop_tables (const char *name,
   return true;
 }
 
-#ifdef HB_EXPERIMENTAL_API
 #ifndef HB_NO_VAR
 static gboolean
 parse_instance (const char *name,
@@ -725,7 +724,6 @@ parse_instance (const char *name,
   return true;
 }
 #endif
-#endif
 
 template <GOptionArgFunc line_parser, bool allow_comments=true>
 static gboolean
@@ -897,7 +895,6 @@ subset_main_t::add_options ()
     {"drop-tables",	0, 0, G_OPTION_ARG_CALLBACK, (gpointer) &parse_drop_tables,	"Drop the specified tables. Use --drop-tables-=... to subtract from the current set.", "list of string table tags or *"},
     {"drop-tables+",	0, G_OPTION_FLAG_HIDDEN, G_OPTION_ARG_CALLBACK, (gpointer) &parse_drop_tables,	"Drop the specified tables.", "list of string table tags or *"},
     {"drop-tables-",	0, G_OPTION_FLAG_HIDDEN, G_OPTION_ARG_CALLBACK, (gpointer) &parse_drop_tables,	"Drop the specified tables.", "list of string table tags or *"},
-#ifdef HB_EXPERIMENTAL_API
 #ifndef HB_NO_VAR
     {"instance",	0, 0, G_OPTION_ARG_CALLBACK, (gpointer) &parse_instance,
      "(Partially|Fully) Instantiate a variable font. A location consists of the tag of a variation axis, followed by '=', followed by a\n"
@@ -906,7 +903,6 @@ subset_main_t::add_options ()
      "For example: --instance=\"wdth=100 wght=200\" or --instance=\"wdth=drop\"\n"
      "Note: currently only fully instancing to the default location is supported\n",
      "list of comma separated axis-locations"},
-#endif
 #endif
     {nullptr}
   };