From 0853e5d9d7bd15fc7782b111dcf9815d25c6d031 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Mon, 12 Dec 2022 19:43:31 +0000 Subject: [PATCH 1/5] [subset] if table dependencies can't be resolved fail the subset. Avoids getting stuck in an infinite loop. --- src/hb-subset.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/hb-subset.cc b/src/hb-subset.cc index f169314b9..e4f164aea 100644 --- a/src/hb-subset.cc +++ b/src/hb-subset.cc @@ -604,6 +604,12 @@ hb_subset_plan_execute_or_fail (hb_subset_plan_t *plan) success = _subset_table (plan, buf, tag); if (unlikely (!success)) goto end; } + if (revisit_set == revisit_temp) { + DEBUG_MSG (SUBSET, nullptr, "Table dependencies unable to be satisfied. Subset failed."); + success = false; + goto end; + } + revisit_set = revisit_temp; } offset += num_tables; From 38a962888512da088eb35ddee4f58c2a06392b73 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Mon, 12 Dec 2022 20:13:17 +0000 Subject: [PATCH 2/5] [subset] simplify handling of table subsetting depedencies. Allow the dependency checker to see all tables that will be subset. Use this to fix the HMTX/VMTX dep check against glyf. Don't delay hmtx/vmtx subsetting if no glyf table is present. --- src/hb-subset.cc | 77 ++++++++++++++++++++++++++---------------------- 1 file changed, 41 insertions(+), 36 deletions(-) diff --git a/src/hb-subset.cc b/src/hb-subset.cc index e4f164aea..353a3a5cf 100644 --- a/src/hb-subset.cc +++ b/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,47 +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 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 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; - } - if (revisit_set == revisit_temp) { - DEBUG_MSG (SUBSET, nullptr, "Table dependencies unable to be satisfied. Subset failed."); - success = false; - 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) { From 9fbe52b88d07557197191b8080bdd591de4318b6 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Mon, 12 Dec 2022 20:24:24 +0000 Subject: [PATCH 3/5] [subset] enable instancing tests by default. --- test/subset/data/Makefile.sources | 8 ++++---- test/subset/generate-expected-outputs.py | 1 - test/subset/meson.build | 15 ++++----------- util/hb-subset.cc | 4 ---- 4 files changed, 8 insertions(+), 20 deletions(-) diff --git a/test/subset/data/Makefile.sources b/test/subset/data/Makefile.sources index 83f3e5ab0..845a77e73 100644 --- a/test/subset/data/Makefile.sources +++ b/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) diff --git a/test/subset/generate-expected-outputs.py b/test/subset/generate-expected-outputs.py index 2e22dcc3b..2b7a87f29 100755 --- a/test/subset/generate-expected-outputs.py +++ b/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) diff --git a/test/subset/meson.build b/test/subset/meson.build index f9b765a41..77a2029d7 100644 --- a/test/subset/meson.build +++ b/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 diff --git a/util/hb-subset.cc b/util/hb-subset.cc index e02cff8d8..bd4dea2d3 100644 --- a/util/hb-subset.cc +++ b/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 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} }; From 0da59f86a8b33672a3da31e3e2c4bf62fd4cac24 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Mon, 12 Dec 2022 20:26:11 +0000 Subject: [PATCH 4/5] [subset] note that CFF/CFF2 instancing is not yet supported. --- src/hb-subset-input.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/hb-subset-input.cc b/src/hb-subset-input.cc index aa1634309..70754411f 100644 --- a/src/hb-subset-input.cc +++ b/src/hb-subset-input.cc @@ -402,6 +402,8 @@ hb_subset_input_get_user_data (const hb_subset_input_t *input, * @axis_tag: Tag of the axis to be pinned * * 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. * * Return value: `true` if success, `false` otherwise * @@ -426,6 +428,8 @@ hb_subset_input_pin_axis_to_default (hb_subset_input_t *input, * @axis_value: Location on the axis to be pinned at * * 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. * * Return value: `true` if success, `false` otherwise * From 64cbe8b96273ef2268111c03950610efe3f6e5e5 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Mon, 12 Dec 2022 20:41:40 +0000 Subject: [PATCH 5/5] [subset] Also note that only full instancing works. --- src/hb-subset-input.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/hb-subset-input.cc b/src/hb-subset-input.cc index 70754411f..38c38c10d 100644 --- a/src/hb-subset-input.cc +++ b/src/hb-subset-input.cc @@ -402,8 +402,9 @@ hb_subset_input_get_user_data (const hb_subset_input_t *input, * @axis_tag: Tag of the axis to be pinned * * 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. + * yet supported. Additionally all axes in a font must be pinned. * * Return value: `true` if success, `false` otherwise * @@ -428,8 +429,9 @@ hb_subset_input_pin_axis_to_default (hb_subset_input_t *input, * @axis_value: Location on the axis to be pinned at * * 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. + * yet supported. Additionally all axes in a font must be pinned. * * Return value: `true` if success, `false` otherwise *