Merge pull request #4452 from googlefonts/fix_test

[subset] bug fixes for GDEF

src/OT/Layout/GDEF/GDEF.hh

@@ -441,6 +441,20 @@ struct MarkGlyphSetsFormat1
   bool covers (unsigned int set_index, hb_codepoint_t glyph_id) const
   { return (this+coverage[set_index]).get_coverage (glyph_id) != NOT_COVERED; }
 
+  void collect_used_mark_sets (const hb_set_t& glyph_set,
+                               hb_set_t& used_mark_sets /* OUT */) const
+  {
+    unsigned i = 0;
+    for (const auto &offset : coverage)
+     {
+       const auto &cov = this+offset;
+       if (cov.intersects (&glyph_set))
+         used_mark_sets.add (i);
+
+       i++;
+     }
+  }
+
   template <typename set_t>
   void collect_coverage (hb_vector_t<set_t> &sets) const
   {
@@ -461,6 +475,7 @@ struct MarkGlyphSetsFormat1
     bool ret = true;
     for (const Offset32To<Coverage>& offset : coverage.iter ())
     {
+      auto snap = c->serializer->snapshot ();
       auto *o = out->coverage.serialize_append (c->serializer);
       if (unlikely (!o))
       {
@@ -468,11 +483,17 @@ struct MarkGlyphSetsFormat1
 	break;
       }
 
-      //not using o->serialize_subset (c, offset, this, out) here because
-      //OTS doesn't allow null offset.
-      //See issue: https://github.com/khaledhosny/ots/issues/172
+      //skip empty coverage
       c->serializer->push ();
-      c->dispatch (this+offset);
+      bool res = false;
+      if (offset) res = c->dispatch (this+offset);
+      if (!res)
+      {
+        c->serializer->pop_discard ();
+        c->serializer->revert (snap);
+        (out->coverage.len)--;
+        continue;
+      }
       c->serializer->add_link (*o, c->serializer->pop_pack ());
     }
 
@@ -513,6 +534,15 @@ struct MarkGlyphSets
     }
   }
 
+  void collect_used_mark_sets (const hb_set_t& glyph_set,
+                               hb_set_t& used_mark_sets /* OUT */) const
+  {
+    switch (u.format) {
+    case 1: u.format1.collect_used_mark_sets (glyph_set, used_mark_sets); return;
+    default:return;
+    }
+  }
+
   bool subset (hb_subset_context_t *c) const
   {
     TRACE_SUBSET (this);
@@ -627,23 +657,29 @@ struct GDEFVersion1_2
   bool subset (hb_subset_context_t *c) const
   {
     TRACE_SUBSET (this);
-    auto *out = c->serializer->embed (*this);
-    if (unlikely (!out)) return_trace (false);
+    auto *out = c->serializer->start_embed (*this);
+    if (unlikely (!c->serializer->extend_min (out))) return_trace (false);
 
+    out->version.major = version.major;
+    out->version.minor = version.minor;
     bool subset_glyphclassdef = out->glyphClassDef.serialize_subset (c, glyphClassDef, this, nullptr, false, true);
     bool subset_attachlist = out->attachList.serialize_subset (c, attachList, this);
     bool subset_ligcaretlist = out->ligCaretList.serialize_subset (c, ligCaretList, this);
     bool subset_markattachclassdef = out->markAttachClassDef.serialize_subset (c, markAttachClassDef, this, nullptr, false, true);
 
     bool subset_markglyphsetsdef = false;
+    auto snapshot_version0 = c->serializer->snapshot ();
     if (version.to_int () >= 0x00010002u)
     {
+      if (unlikely (!c->serializer->embed (markGlyphSetsDef))) return_trace (false);
       subset_markglyphsetsdef = out->markGlyphSetsDef.serialize_subset (c, markGlyphSetsDef, this);
     }
 
     bool subset_varstore = false;
+    auto snapshot_version2 = c->serializer->snapshot ();
     if (version.to_int () >= 0x00010003u)
     {
+      if (unlikely (!c->serializer->embed (varStore))) return_trace (false);
       if (c->plan->all_axes_pinned)
         out->varStore = 0;
       else if (c->plan->normalized_coords)
@@ -671,8 +707,10 @@ struct GDEFVersion1_2
       out->version.minor = 3;
     } else if (subset_markglyphsetsdef) {
       out->version.minor = 2;
+      c->serializer->revert (snapshot_version2);
     } else  {
       out->version.minor = 0;
+      c->serializer->revert (snapshot_version0);
     }
 
     return_trace (subset_glyphclassdef || subset_attachlist ||

src/hb-ot-layout-common.hh

@@ -1371,10 +1371,20 @@ struct Lookup
 
     if (lookupFlag & LookupFlag::UseMarkFilteringSet)
     {
-      if (unlikely (!c->serializer->extend (out))) return_trace (false);
       const HBUINT16 &markFilteringSet = StructAfter<HBUINT16> (subTable);
-      HBUINT16 &outMarkFilteringSet = StructAfter<HBUINT16> (out->subTable);
-      outMarkFilteringSet = markFilteringSet;
+      hb_codepoint_t *idx;
+      if (!c->plan->used_mark_sets_map.has (markFilteringSet, &idx))
+      {
+        unsigned new_flag = lookupFlag;
+        new_flag &= ~LookupFlag::UseMarkFilteringSet;
+        out->lookupFlag = new_flag;
+      }
+      else
+      {
+        if (unlikely (!c->serializer->extend (out))) return_trace (false);
+        HBUINT16 &outMarkFilteringSet = StructAfter<HBUINT16> (out->subTable);
+        outMarkFilteringSet = *idx;
+      }
     }
 
     // Always keep the lookup even if it's empty. The rest of layout subsetting depends on lookup

src/hb-subset-plan-member-list.hh

@@ -70,6 +70,9 @@ HB_SUBSET_PLAN_MEMBER (hb_set_t, _glyphset_colred)
 HB_SUBSET_PLAN_MEMBER (hb_map_t, gsub_lookups)
 HB_SUBSET_PLAN_MEMBER (hb_map_t, gpos_lookups)
 
+//use_mark_sets mapping: old->new
+HB_SUBSET_PLAN_MEMBER (hb_map_t, used_mark_sets_map)
+
 //active langsys we'd like to retain
 HB_SUBSET_PLAN_MEMBER (hb_hashmap_t E(<unsigned, hb::unique_ptr<hb_set_t>>), gsub_langsys)
 HB_SUBSET_PLAN_MEMBER (hb_hashmap_t E(<unsigned, hb::unique_ptr<hb_set_t>>), gpos_langsys)

src/hb-subset-plan.cc

@@ -465,6 +465,24 @@ _math_closure (hb_subset_plan_t *plan,
   math.destroy ();
 }
 
+static inline void
+_remap_used_mark_sets (hb_subset_plan_t *plan,
+                       hb_map_t& used_mark_sets_map)
+{
+  hb_blob_ptr_t<OT::GDEF> gdef = plan->source_table<OT::GDEF> ();
+
+  if (!gdef->has_data () || !gdef->has_mark_glyph_sets ())
+  {
+    gdef.destroy ();
+    return;
+  }
+
+  hb_set_t used_mark_sets;
+  gdef->get_mark_glyph_sets ().collect_used_mark_sets (plan->_glyphset_gsub, used_mark_sets);
+  gdef.destroy ();
+
+  _remap_indexes (&used_mark_sets, &used_mark_sets_map);
+}
 
 static inline void
 _remove_invalid_gids (hb_set_t *glyphs,
@@ -1160,6 +1178,9 @@ hb_subset_plan_t::hb_subset_plan_t (hb_face_t *face,
   for (auto &v : bounds_height_vec)
     v = 0xFFFFFFFF;
 
+  if (!drop_tables.has (HB_OT_TAG_GDEF))
+    _remap_used_mark_sets (this, used_mark_sets_map);
+
   if (unlikely (in_error ()))
     return;
 

test/subset/data/Makefile.am

@@ -73,7 +73,6 @@ EXTRA_DIST += \
 	expected/collect_name_ids \
 	expected/instantiate_colrv1 \
 	expected/instantiate_cff2_update_metrics \
-	expected/empty_region_vardata \
 	fonts \
 	profiles \
 	$(NULL)

test/subset/data/Makefile.sources

@@ -64,7 +64,6 @@ TESTS = \
 	tests/collect_name_ids.tests \
 	tests/instantiate_colrv1.tests \
 	tests/instantiate_cff2_update_metrics.tests \
-	tests/empty_region_vardata.tests \
 	$(NULL)
 
 # TODO: re-enable once colrv1 subsetting is stabilized.