[repacker] insert new subtables immediately after the subtable they split from in the lookup.

src/graph/gsubgpos-graph.hh

@@ -124,7 +124,7 @@ struct Lookup : public OT::Lookup
     if (!is_ext && type != OT::Layout::GPOS_impl::PosLookupSubTable::Type::Pair)
       return true;
 
-    hb_vector_t<unsigned> all_new_subtables;
+    hb_vector_t<hb_pair_t<unsigned, hb_vector_t<unsigned>>> all_new_subtables;
     for (unsigned i = 0; i < subTable.len; i++)
     {
       unsigned subtable_index = c.graph.index_for_offset (this_index, &subTable[i]);
@@ -147,11 +147,14 @@ struct Lookup : public OT::Lookup
 
       hb_vector_t<unsigned> new_sub_tables = pairPos->split_subtables (c, subtable_index);
       if (new_sub_tables.in_error ()) return false;
-      + new_sub_tables.iter() | hb_sink (all_new_subtables);
+      hb_pair_t<unsigned, hb_vector_t<unsigned>>* entry = all_new_subtables.push ();
+      entry->first = i;
+      entry->second = std::move (new_sub_tables);
     }
 
-    if (all_new_subtables)
+    if (all_new_subtables) {
       add_sub_tables (c, this_index, type, all_new_subtables);
+    }
 
     return true;
   }
@@ -159,13 +162,18 @@ struct Lookup : public OT::Lookup
   void add_sub_tables (gsubgpos_graph_context_t& c,
                        unsigned this_index,
                        unsigned type,
-                       hb_vector_t<unsigned>& subtable_indices)
+                       hb_vector_t<hb_pair_t<unsigned, hb_vector_t<unsigned>>>& subtable_ids)
   {
     bool is_ext = is_extension (c.table_tag);
     auto& v = c.graph.vertices_[this_index];
+    fix_existing_subtable_links (c, this_index, subtable_ids);
+
+    unsigned new_subtable_count = 0;
+    for (const auto& p : subtable_ids)
+      new_subtable_count += p.second.length;
 
     size_t new_size = v.table_size ()
-                      + subtable_indices.length * OT::Offset16::static_size;
+                      + new_subtable_count * OT::Offset16::static_size;
     char* buffer = (char*) hb_calloc (1, new_size);
     c.add_buffer (buffer);
     memcpy (buffer, v.obj.head, v.table_size());
@@ -175,30 +183,56 @@ struct Lookup : public OT::Lookup
 
     Lookup* new_lookup = (Lookup*) buffer;
 
-    new_lookup->subTable.len = subTable.len + subtable_indices.length;
-    unsigned offset_index = subTable.len;
-    for (unsigned subtable_id : subtable_indices)
+    new_lookup->subTable.len = subTable.len + new_subtable_count;
+    for (const auto& p : subtable_ids)
     {
-      if (is_ext)
+      unsigned offset_index = p.first + 1;
+      for (unsigned subtable_id : p.second)
       {
-        unsigned ext_id = create_extension_subtable (c, subtable_id, type);
-        c.graph.vertices_[subtable_id].parents.push (ext_id);
-        subtable_id = ext_id;
+        if (is_ext)
+        {
+          unsigned ext_id = create_extension_subtable (c, subtable_id, type);
+          c.graph.vertices_[subtable_id].parents.push (ext_id);
+          subtable_id = ext_id;
+        }
+
+        auto* link = v.obj.real_links.push ();
+        link->width = 2;
+        link->objidx = subtable_id;
+        link->position = (char*) &new_lookup->subTable[offset_index++] -
+                         (char*) new_lookup;
+        c.graph.vertices_[subtable_id].parents.push (this_index);
       }
-
-      auto* link = v.obj.real_links.push ();
-      link->width = 2;
-      link->objidx = subtable_id;
-      link->position = (char*) &new_lookup->subTable[offset_index++] -
-                       (char*) new_lookup;
-      c.graph.vertices_[subtable_id].parents.push (this_index);
     }
 
+    // Repacker sort order depends on link order, which we've messed up so resort it.
+    v.obj.real_links.qsort ();
+
     // The head location of the lookup has changed, invalidating the lookups map entry
     // in the context. Update the map.
     c.lookups.set (this_index, new_lookup);
   }
 
+  void fix_existing_subtable_links (gsubgpos_graph_context_t& c,
+                                    unsigned this_index,
+                                    hb_vector_t<hb_pair_t<unsigned, hb_vector_t<unsigned>>>& subtable_ids)
+  {
+    auto& v = c.graph.vertices_[this_index];
+    Lookup* lookup = (Lookup*) v.obj.head;
+
+    for (const auto& p : subtable_ids)
+    {
+      unsigned insert_index = p.first;
+      unsigned pos_offset = p.second.length * OT::Offset16::static_size;
+      unsigned insert_offset = (char*) &lookup->subTable[insert_index] - (char*) lookup;
+
+      for (auto& l : v.obj.all_links_writer ())
+      {
+        if (l.position > insert_offset) l.position += pos_offset;
+      }
+    }
+  }
+
   unsigned create_extension_subtable (gsubgpos_graph_context_t& c,
                                       unsigned subtable_index,
                                       unsigned type)

src/hb-serialize.hh

@@ -139,6 +139,11 @@ struct hb_serialize_context_t
         objidx = o.objidx;
       }
 #endif
+
+      HB_INTERNAL static int cmp (const void* a, const void* b)
+      {
+        return ((const link_t*)a)->position - ((const link_t*)b)->position;
+      }
     };
 
     char *head;

src/test-repacker.cc

@@ -1183,18 +1183,16 @@ populate_serializer_with_large_pair_pos_1 (hb_serialize_context_t* c,
   unsigned pair_pos_2 = add_object (large_string.c_str(), 200, c);
 
   if (as_extension) {
-
+    pair_pos_2 = add_extension (pair_pos_2, 2, c);
     for (int i = num_pair_pos_1 - 1; i >= 0; i--)
       pair_pos_1[i] = add_extension (pair_pos_1[i], 2, c);
-    pair_pos_2 = add_extension (pair_pos_2, 2, c);
   }
 
   start_lookup (as_extension ? 9 : 2, 1 + num_pair_pos_1, c);
 
-  add_offset (pair_pos_2, c);
   for (int i = 0; i < num_pair_pos_1; i++)
     add_offset (pair_pos_1[i], c);
-
+  add_offset (pair_pos_2, c);
 
   unsigned lookup = finish_lookup (c);