[subset] Update PairPosFormat2 subsetting to match fontTools updated subsetting (https://github.com/fonttools/fonttools/pull/2221)

- subset class def 1 against the coverage table.
- Don't re-use class 0 in class def 2.
- Ignore class 0 glyphs for the purposes of determing format 1 vs format 2 encoding for ClassDef.

Add an additional test case which demonstrates these issues.

src/hb-ot-layout-common.hh

@@ -92,6 +92,7 @@ static void ClassDef_remap_and_serialize (hb_serialize_context_t *c,
 					  const hb_map_t &gid_klass_map,
 					  hb_sorted_vector_t<HBGlyphID> &glyphs,
 					  const hb_set_t &klasses,
+                                          bool use_class_zero,
 					  hb_map_t *klass_map /*INOUT*/);
 
 struct hb_subset_layout_context_t :
@@ -1649,6 +1650,7 @@ static void ClassDef_remap_and_serialize (hb_serialize_context_t *c,
 					  const hb_map_t &gid_klass_map,
 					  hb_sorted_vector_t<HBGlyphID> &glyphs,
 					  const hb_set_t &klasses,
+                                          bool use_class_zero,
 					  hb_map_t *klass_map /*INOUT*/)
 {
   if (!klass_map)
@@ -1660,7 +1662,7 @@ static void ClassDef_remap_and_serialize (hb_serialize_context_t *c,
 
   /* any glyph not assigned a class value falls into Class zero (0),
    * if any glyph assigned to class 0, remapping must start with 0->0*/
-  if (glyphset.get_population () > gid_klass_map.get_population ())
+  if (!use_class_zero || glyphset.get_population () > gid_klass_map.get_population ())
     klass_map->set (0, 0);
 
   unsigned idx = klass_map->has (0) ? 1 : 0;
@@ -1730,10 +1732,12 @@ struct ClassDefFormat1
   }
 
   bool subset (hb_subset_context_t *c,
-	       hb_map_t *klass_map = nullptr /*OUT*/) const
+	       hb_map_t *klass_map = nullptr /*OUT*/,
+               bool use_class_zero = true,
+               const hb_set_t* glyph_filter = nullptr) const
   {
     TRACE_SUBSET (this);
-    const hb_set_t &glyphset = *c->plan->glyphset_gsub ();
+    const hb_set_t *glyphset = glyph_filter ? glyph_filter : c->plan->glyphset_gsub ();
     const hb_map_t &glyph_map = *c->plan->glyph_map;
 
     hb_sorted_vector_t<HBGlyphID> glyphs;
@@ -1753,8 +1757,8 @@ struct ClassDefFormat1
       orig_klasses.add (klass);
     }
 
-    ClassDef_remap_and_serialize (c->serializer, glyphset, gid_org_klass_map,
-				  glyphs, orig_klasses, klass_map);
+    ClassDef_remap_and_serialize (c->serializer, *glyphset, gid_org_klass_map,
+				  glyphs, orig_klasses, use_class_zero, klass_map);
     return_trace ((bool) glyphs);
   }
 
@@ -1903,10 +1907,12 @@ struct ClassDefFormat2
   }
 
   bool subset (hb_subset_context_t *c,
-	       hb_map_t *klass_map = nullptr /*OUT*/) const
+	       hb_map_t *klass_map = nullptr /*OUT*/,
+               bool use_class_zero = true,
+               const hb_set_t* glyph_filter = nullptr) const
   {
     TRACE_SUBSET (this);
-    const hb_set_t &glyphset = *c->plan->glyphset_gsub ();
+    const hb_set_t *glyphset = glyph_filter != nullptr ? glyph_filter : c->plan->glyphset_gsub ();
     const hb_map_t &glyph_map = *c->plan->glyph_map;
 
     hb_sorted_vector_t<HBGlyphID> glyphs;
@@ -1922,15 +1928,15 @@ struct ClassDefFormat2
       hb_codepoint_t end   = rangeRecord[i].last + 1;
       for (hb_codepoint_t g = start; g < end; g++)
       {
-	if (!glyphset.has (g)) continue;
+	if (!glyphset->has (g)) continue;
 	glyphs.push (glyph_map[g]);
 	gid_org_klass_map.set (glyph_map[g], klass);
 	orig_klasses.add (klass);
       }
     }
 
-    ClassDef_remap_and_serialize (c->serializer, glyphset, gid_org_klass_map,
-				  glyphs, orig_klasses, klass_map);
+    ClassDef_remap_and_serialize (c->serializer, *glyphset, gid_org_klass_map,
+				  glyphs, orig_klasses, use_class_zero, klass_map);
     return_trace ((bool) glyphs);
   }
 
@@ -2045,10 +2051,9 @@ struct ClassDef
     if (likely (it))
     {
       hb_codepoint_t glyph_min = (*it).first;
-      hb_codepoint_t glyph_max = + it
-				 | hb_map (hb_first)
-				 | hb_reduce (hb_max, 0u);
+      hb_codepoint_t glyph_max = glyph_min;
 
+      unsigned num_glyphs = 0;
       unsigned num_ranges = 1;
       hb_codepoint_t prev_gid = glyph_min;
       unsigned prev_klass = (*it).second;
@@ -2057,7 +2062,9 @@ struct ClassDef
       {
 	hb_codepoint_t cur_gid = gid_klass_pair.first;
 	unsigned cur_klass = gid_klass_pair.second;
+        if (cur_klass) num_glyphs++;
 	if (cur_gid == glyph_min || !cur_klass) continue;
+        if (cur_gid > glyph_max) glyph_max = cur_gid;
 	if (cur_gid != prev_gid + 1 ||
 	    cur_klass != prev_klass)
 	  num_ranges++;
@@ -2066,7 +2073,7 @@ struct ClassDef
 	prev_klass = cur_klass;
       }
 
-      if (1 + (glyph_max - glyph_min + 1) <= num_ranges * 3)
+      if (num_glyphs && 1 + (glyph_max - glyph_min + 1) <= num_ranges * 3)
 	format = 1;
     }
     u.format = format;
@@ -2080,12 +2087,14 @@ struct ClassDef
   }
 
   bool subset (hb_subset_context_t *c,
-	       hb_map_t *klass_map = nullptr /*OUT*/) const
+	       hb_map_t *klass_map = nullptr /*OUT*/,
+               bool use_class_zero = true,
+               const hb_set_t* glyph_filter = nullptr) const
   {
     TRACE_SUBSET (this);
     switch (u.format) {
-    case 1: return_trace (u.format1.subset (c, klass_map));
-    case 2: return_trace (u.format2.subset (c, klass_map));
+    case 1: return_trace (u.format1.subset (c, klass_map, use_class_zero, glyph_filter));
+    case 2: return_trace (u.format2.subset (c, klass_map, use_class_zero, glyph_filter));
     default:return_trace (false);
     }
   }

src/hb-ot-layout-gpos-table.hh

@@ -1366,7 +1366,10 @@ struct PairPosFormat2
       class2_set.add (klass2);
     }
 
-    if (class1_set.is_empty () || class2_set.is_empty ()) return;
+    if (class1_set.is_empty ()
+        || class2_set.is_empty ()
+        || (class2_set.get_population() == 1 && class2_set.has(0)))
+      return;
 
     unsigned len1 = valueFormat1.get_len ();
     unsigned len2 = valueFormat2.get_len ();
@@ -1434,12 +1437,17 @@ struct PairPosFormat2
     out->valueFormat1 = valueFormat1;
     out->valueFormat2 = valueFormat2;
 
+    hb_set_t coverage_glyphs;
+    + hb_iter (this + coverage)
+    | hb_filter (c->plan->glyphset_gsub())
+    | hb_sink (coverage_glyphs);
+
     hb_map_t klass1_map;
-    out->classDef1.serialize_subset (c, classDef1, this, &klass1_map);
+    out->classDef1.serialize_subset (c, classDef1, this, &klass1_map, true, &coverage_glyphs);
     out->class1Count = klass1_map.get_population ();
 
     hb_map_t klass2_map;
-    out->classDef2.serialize_subset (c, classDef2, this, &klass2_map);
+    out->classDef2.serialize_subset (c, classDef2, this, &klass2_map, false);
     out->class2Count = klass2_map.get_population ();
 
     unsigned len1 = valueFormat1.get_len ();

test/subset/data/tests/layout.gpos2.tests

@@ -9,5 +9,6 @@ keep-layout-retain-gids.txt
 SUBSETS:
 !#
 !#%
+.#
 ABC
 *