[repacker] PairPosFormat2 splitting - fix coverage and classdef splitting.

The old code was splitting based on coverage index, but should have been splitting on class value.

src/graph/classdef-graph.hh

@@ -57,35 +57,22 @@ struct ClassDefFormat2 : public OT::ClassDefFormat2_4<SmallTypes>
 struct ClassDef : public OT::ClassDef
 {
   template<typename It>
-  static bool clone_class_def (gsubgpos_graph_context_t& c,
+  static bool add_class_def (gsubgpos_graph_context_t& c,
-                               unsigned class_def_id,
+                             unsigned parent_id,
-                               unsigned new_parent_id,
+                             unsigned link_position,
-                               unsigned link_position,
+                             It glyph_and_class,
-                               It glyphs)
+                             unsigned max_size)
   {
-    unsigned class_def_size = c.graph.vertices_[class_def_id].table_size ();
-    auto& class_def_v = c.graph.vertices_[class_def_id];
-    ClassDef* class_def_table = (ClassDef*) class_def_v.obj.head;
-    if (!class_def_table->sanitize (class_def_v))
-      return false;
-
-    auto new_class_def =
-        + glyphs
-        | hb_map_retains_sorting ([&] (hb_codepoint_t gid) {
-          return hb_pair (gid, class_def_table->get_class (gid));
-        })
-        ;
-
     unsigned class_def_prime_id = c.graph.new_node (nullptr, nullptr);
     auto& class_def_prime_vertex = c.graph.vertices_[class_def_prime_id];
-    if (!make_class_def (c, new_class_def, class_def_prime_id, class_def_size))
+    if (!make_class_def (c, glyph_and_class, class_def_prime_id, max_size))
       return false;
 
-    auto* class_def_link = c.graph.vertices_[new_parent_id].obj.real_links.push ();
+    auto* class_def_link = c.graph.vertices_[parent_id].obj.real_links.push ();
     class_def_link->width = SmallTypes::size;
     class_def_link->objidx = class_def_prime_id;
     class_def_link->position = link_position;
-    class_def_prime_vertex.parents.push (new_parent_id);
+    class_def_prime_vertex.parents.push (parent_id);
 
     return true;
   }

src/graph/coverage-graph.hh

@@ -77,16 +77,26 @@ struct Coverage : public OT::Layout::Common::Coverage
         | hb_map_retains_sorting (hb_first)
         ;
 
+    return add_coverage (c, new_parent_id, link_position, new_coverage, coverage_size);
+  }
+
+  template<typename It>
+  static Coverage* add_coverage (gsubgpos_graph_context_t& c,
+                                 unsigned parent_id,
+                                 unsigned link_position,
+                                 It glyphs,
+                                 unsigned max_size)
+  {
     unsigned coverage_prime_id = c.graph.new_node (nullptr, nullptr);
     auto& coverage_prime_vertex = c.graph.vertices_[coverage_prime_id];
-    if (!make_coverage (c, new_coverage, coverage_prime_id, coverage_size))
+    if (!make_coverage (c, glyphs, coverage_prime_id, max_size))
       return nullptr;
 
-    auto* coverage_link = c.graph.vertices_[new_parent_id].obj.real_links.push ();
+    auto* coverage_link = c.graph.vertices_[parent_id].obj.real_links.push ();
     coverage_link->width = SmallTypes::size;
     coverage_link->objidx = coverage_prime_id;
     coverage_link->position = link_position;
-    coverage_prime_vertex.parents.push (new_parent_id);
+    coverage_prime_vertex.parents.push (parent_id);
 
     return (Coverage*) coverage_prime_vertex.obj.head;
   }

src/graph/pairpos-graph.hh

@@ -320,6 +320,8 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4<SmallType
     DEBUG_MSG (SUBSET_REPACK, nullptr,
                "  Cloning PairPosFormat2 (%u) range [%u, %u).", split_context.this_index, start, end);
 
+    graph_t& graph = split_context.c.graph;
+
     unsigned num_records = end - start;
     unsigned prime_size = OT::Layout::GPOS_impl::PairPosFormat2_4<SmallTypes>::min_size
                           + num_records * split_context.class1_record_size;
@@ -328,7 +330,7 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4<SmallType
     if (pair_pos_prime_id == (unsigned) -1) return -1;
 
     PairPosFormat2* pair_pos_prime =
-        (PairPosFormat2*) split_context.c.graph.object (pair_pos_prime_id).head;
+        (PairPosFormat2*) graph.object (pair_pos_prime_id).head;
     pair_pos_prime->format = this->format;
     pair_pos_prime->valueFormat1 = this->valueFormat1;
     pair_pos_prime->valueFormat2 = this->valueFormat2;
@@ -340,35 +342,55 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4<SmallType
                           end);
 
     unsigned coverage_id =
-        split_context.c.graph.index_for_offset (split_context.this_index, &coverage);
+        graph.index_for_offset (split_context.this_index, &coverage);
+    unsigned class_def_1_id =
+        graph.index_for_offset (split_context.this_index, &classDef1);
+    auto& coverage_v = graph.vertices_[coverage_id];
+    auto& class_def_1_v = graph.vertices_[class_def_1_id];
+    Coverage* coverage_table = (Coverage*) coverage_v.obj.head;
+    ClassDef* class_def_1_table = (ClassDef*) class_def_1_v.obj.head;
+    if (!coverage_table->sanitize (coverage_v)
+        || !class_def_1_table->sanitize (class_def_1_v))
+      return false;
 
-    Coverage* new_coverage =  Coverage::clone_coverage (split_context.c,
+    auto klass_map =
-                                                        coverage_id,
+    + coverage_table->iter ()
-                                                        pair_pos_prime_id,
+    | hb_map_retains_sorting ([&] (hb_codepoint_t gid) {
-                                                        2,
+      return hb_pair (gid, class_def_1_table->get_class (gid));
-                                                        start, end);
+    })
-    if (!new_coverage)
+    | hb_filter ([&] (hb_codepoint_t klass) {
+      return klass >= start && klass < end;
+    }, hb_second)
+    | hb_map_retains_sorting ([&] (hb_pair_t<hb_codepoint_t, hb_codepoint_t> gid_and_class) {
+      // Classes must be from 0...N so subtract start
+      return hb_pair (gid_and_class.first, gid_and_class.second - start);
+    })
+    ;
+
+    if (!Coverage::add_coverage (split_context.c,
+                                 pair_pos_prime_id,
+                                 2,
+                                 + klass_map | hb_map_retains_sorting (hb_first),
+                                 coverage_v.table_size ()))
       return -1;
 
     // classDef1
-    unsigned class_def_1_id =
+    if (!ClassDef::add_class_def (split_context.c,
-        split_context.c.graph.index_for_offset (split_context.this_index, &classDef1);
+                                  pair_pos_prime_id,
-    if (!ClassDef::clone_class_def (split_context.c,
+                                  8,
-                                    class_def_1_id,
+                                  + klass_map,
-                                    pair_pos_prime_id,
+                                  class_def_1_v.table_size ()))
-                                    8,
-                                    new_coverage->iter ()))
       return -1;
 
     // classDef2
     unsigned class_def_2_id =
-        split_context.c.graph.index_for_offset (split_context.this_index, &classDef2);
+        graph.index_for_offset (split_context.this_index, &classDef2);
-    auto* class_def_link = split_context.c.graph.vertices_[pair_pos_prime_id].obj.real_links.push ();
+    auto* class_def_link = graph.vertices_[pair_pos_prime_id].obj.real_links.push ();
     class_def_link->width = SmallTypes::size;
     class_def_link->objidx = class_def_2_id;
     class_def_link->position = 10;
-    split_context.c.graph.vertices_[class_def_2_id].parents.push (pair_pos_prime_id);
+    graph.vertices_[class_def_2_id].parents.push (pair_pos_prime_id);
-    split_context.c.graph.duplicate (pair_pos_prime_id, class_def_2_id);
+    graph.duplicate (pair_pos_prime_id, class_def_2_id);
 
     return pair_pos_prime_id;
   }
@@ -450,48 +472,43 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4<SmallType
     if (count >= old_count)
       return true;
 
+    graph_t& graph = split_context.c.graph;
     class1Count = count;
-    split_context.c.graph.vertices_[split_context.this_index].obj.tail -=
+    graph.vertices_[split_context.this_index].obj.tail -=
         (old_count - count) * split_context.class1_record_size;
 
     unsigned coverage_id =
-        split_context.c.graph.index_for_offset (split_context.this_index, &coverage);
+        graph.index_for_offset (split_context.this_index, &coverage);
-    auto& coverage_v = split_context.c.graph.vertices_[coverage_id];
+    unsigned class_def_1_id =
+        graph.index_for_offset (split_context.this_index, &classDef1);
+    auto& coverage_v = graph.vertices_[coverage_id];
+    auto& class_def_1_v = graph.vertices_[class_def_1_id];
     Coverage* coverage_table = (Coverage*) coverage_v.obj.head;
-    if (!coverage_table->sanitize (coverage_v))
+    ClassDef* class_def_1_table = (ClassDef*) class_def_1_v.obj.head;
+    if (!coverage_table->sanitize (coverage_v)
+        || !class_def_1_table->sanitize (class_def_1_v))
       return false;
 
-    auto new_coverage =
+    auto klass_map =
-        + hb_zip (coverage_table->iter (), hb_range ())
+    + coverage_table->iter ()
-        | hb_filter ([&] (hb_pair_t<unsigned, unsigned> p) {
+    | hb_map_retains_sorting ([&] (hb_codepoint_t gid) {
-          return p.second < count;
+      return hb_pair (gid, class_def_1_table->get_class (gid));
-        })
+    })
-        | hb_map_retains_sorting (hb_first)
+    | hb_filter ([&] (hb_codepoint_t klass) {
-        ;
+      return klass < count;
+    }, hb_second)
+    ;
 
-    if (!Coverage::make_coverage (split_context.c, new_coverage, coverage_id, coverage_v.table_size ()))
+    if (!Coverage::make_coverage (split_context.c,
+                                  + klass_map | hb_map_retains_sorting (hb_first),
+                                  coverage_id,
+                                  coverage_v.table_size ()))
       return false;
 
-    // classDef1
-    coverage_table = (Coverage*) coverage_v.obj.head; // get the new table
-    unsigned class_def_id = split_context.c.graph.index_for_offset (split_context.this_index,
-                                                                    &classDef1);
-    auto& class_def_v = split_context.c.graph.vertices_[class_def_id];
-    ClassDef* class_def_table = (ClassDef*) class_def_v.obj.head;
-    if (!class_def_table->sanitize (class_def_v))
-      return false;
-
-    auto new_class_def =
-        + coverage_table->iter ()
-        | hb_map_retains_sorting ([&] (hb_codepoint_t gid) {
-          return hb_pair (gid, class_def_table->get_class (gid));
-        })
-        ;
-
     return ClassDef::make_class_def (split_context.c,
-                                     new_class_def,
+                                     + klass_map,
-                                     class_def_id,
+                                     class_def_1_id,
-                                     class_def_v.table_size ());
+                                     class_def_1_v.table_size ());
   }
 
   hb_hashmap_t<void*, unsigned>

src/test-repacker.cc

@@ -143,6 +143,7 @@ static unsigned add_extension (unsigned child,
 
 }
 
+// Adds coverage table fro [start, end]
 static unsigned add_coverage (char start, char end,
                               hb_serialize_context_t* c)
 {
@@ -167,24 +168,29 @@ static unsigned add_coverage (char start, char end,
   return add_object (coverage, 10, c);
 }
 
-static unsigned add_class_def (uint8_t start_glyph,
+// Adds a class that maps glyphs from [start_glyph, end_glyph)
-                               uint16_t class_count,
+// to classes 1...n
+static unsigned add_class_def (uint16_t start_glyph,
+                               uint16_t end_glyph,
                                hb_serialize_context_t* c)
 {
+  unsigned count = end_glyph - start_glyph;
   uint8_t header[] = {
     0, 1, // format
-    0, start_glyph, // startGlyphID
+
-    (uint8_t) ((class_count >> 8) & 0xFF),
+    (uint8_t) ((start_glyph >> 8) & 0xFF),
-    (uint8_t) (class_count & 0xFF), // count
+    (uint8_t) (start_glyph & 0xFF), // start_glyph
+
+    (uint8_t) ((count >> 8) & 0xFF),
+    (uint8_t) (count & 0xFF), // count
   };
 
   start_object ((char*) header, 6, c);
-  for (uint16_t i = 1; i <= class_count; i++)
+  for (uint16_t i = 1; i <= count; i++)
   {
-    unsigned klass = start_glyph + 10 + i;
     uint8_t class_value[] = {
-      (uint8_t) ((klass >> 8) & 0xFF),
+      (uint8_t) ((i >> 8) & 0xFF),
-      (uint8_t) (klass & 0xFF), // count
+      (uint8_t) (i & 0xFF), // count
     };
     extend ((char*) class_value, 2, c);
   }
@@ -1216,19 +1222,27 @@ populate_serializer_with_large_pair_pos_2 (hb_serialize_context_t* c,
   unsigned* device_tables = (unsigned*) calloc (num_pair_pos_2 * num_class_1 * num_class_2,
                                                 sizeof(unsigned));
 
+  // Total glyphs = num_class_1 * num_pair_pos_2
   for (int i = num_pair_pos_2 - 1; i >= 0; i--)
   {
+    unsigned start_glyph = 5 + i * num_class_1;
     if (num_class_2 >= num_class_1)
     {
-      class_def_2[i] = add_class_def (10, num_class_2, c);
+      class_def_2[i] = add_class_def (11,
-      class_def_1[i] = add_class_def (5 + i * num_class_1, num_class_1, c);
+                                      10 + num_class_2, c);
+      class_def_1[i] = add_class_def (start_glyph + 1,
+                                      start_glyph + num_class_1,
+                                      c);
     } else {
-      class_def_1[i] = add_class_def (5 + i * num_class_1, num_class_1, c);
+      class_def_1[i] = add_class_def (start_glyph + 1,
-      class_def_2[i] = add_class_def (10, num_class_2, c);
+                                      start_glyph + num_class_1,
+                                      c);
+      class_def_2[i] = add_class_def (11,
+                                      10 + num_class_2, c);
     }
 
-    coverage[i] = add_coverage (5 + i * num_class_1,
+    coverage[i] = add_coverage (start_glyph,
-                                5 + (i + 1) * num_class_1 - 1,
+                                start_glyph + num_class_1 - 1,
                                 c);
 
     if (with_device_tables)