[repacker] use mutable copies of Coverage/ClassDef in MarkBasePos shrink operation.

Also make mutable copies (when needed) of the top level subtables during a split operation.

src/graph/graph.hh

@@ -495,6 +495,12 @@ struct graph_t
     return as_table_from_index<T> (index_for_offset (parent, offset), std::forward<Ts>(ds)...);
   }
 
+  template <typename T, typename ...Ts>
+  vertex_and_table_t<T> as_mutable_table (unsigned parent, const void* offset, Ts... ds)
+  {
+    return as_table_from_index<T> (mutable_index_for_offset (parent, offset), std::forward<Ts>(ds)...);
+  }
+
   template <typename T, typename ...Ts>
   vertex_and_table_t<T> as_table_from_index (unsigned index, Ts... ds)
   {
@@ -833,7 +839,20 @@ struct graph_t
    * parent to the clone. The copy is a shallow copy, objects
    * linked from child are not duplicated.
    */
-  bool duplicate (unsigned parent_idx, unsigned child_idx)
+  unsigned duplicate_if_shared (unsigned parent_idx, unsigned child_idx)
+  {
+    unsigned new_idx = duplicate (parent_idx, child_idx);
+    if (new_idx == (unsigned) -1) return child_idx;
+    return new_idx;
+  }
+
+
+  /*
+   * Creates a copy of child and re-assigns the link from
+   * parent to the clone. The copy is a shallow copy, objects
+   * linked from child are not duplicated.
+   */
+  unsigned duplicate (unsigned parent_idx, unsigned child_idx)
   {
     update_parents ();
 
@@ -849,7 +868,7 @@ struct graph_t
       // to child are from parent.
       DEBUG_MSG (SUBSET_REPACK, nullptr, "  Not duplicating %d => %d",
                  parent_idx, child_idx);
-      return false;
+      return -1;
     }
 
     DEBUG_MSG (SUBSET_REPACK, nullptr, "  Duplicating %d => %d",
@@ -869,7 +888,7 @@ struct graph_t
       reassign_link (l, parent_idx, clone_idx);
     }
 
-    return true;
+    return clone_idx;
   }
 
 

src/graph/gsubgpos-graph.hh

@@ -131,8 +131,10 @@ struct Lookup : public OT::Lookup
     for (unsigned i = 0; i < subTable.len; i++)
     {
       unsigned subtable_index = c.graph.index_for_offset (this_index, &subTable[i]);
+      unsigned parent_index = this_index;
       if (is_ext) {
         unsigned ext_subtable_index = subtable_index;
+        parent_index = ext_subtable_index;
         ExtensionFormat1<OT::Layout::GSUB_impl::ExtensionSubst>* extension =
             (ExtensionFormat1<OT::Layout::GSUB_impl::ExtensionSubst>*)
             c.graph.object (ext_subtable_index).head;
@@ -150,9 +152,9 @@ struct Lookup : public OT::Lookup
       switch (type)
       {
       case 2:
-        new_sub_tables = split_subtable<PairPos> (c, subtable_index); break;
+        new_sub_tables = split_subtable<PairPos> (c, parent_index, subtable_index); break;
       case 4:
-        new_sub_tables = split_subtable<MarkBasePos> (c, subtable_index); break;
+        new_sub_tables = split_subtable<MarkBasePos> (c, parent_index, subtable_index); break;
       default:
         break;
       }
@@ -172,13 +174,14 @@ struct Lookup : public OT::Lookup
 
   template<typename T>
   hb_vector_t<unsigned> split_subtable (gsubgpos_graph_context_t& c,
+                                        unsigned parent_idx,
                                         unsigned objidx)
   {
     T* sub_table = (T*) c.graph.object (objidx).head;
     if (!sub_table || !sub_table->sanitize (c.graph.vertices_[objidx]))
       return hb_vector_t<unsigned> ();
 
-    return sub_table->split_subtables (c, objidx);
+    return sub_table->split_subtables (c, parent_idx, objidx);
   }
 
   void add_sub_tables (gsubgpos_graph_context_t& c,

src/graph/markbasepos-graph.hh

@@ -209,7 +209,9 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2<S
     return vertex_len >= MarkBasePosFormat1::static_size;
   }
 
-  hb_vector_t<unsigned> split_subtables (gsubgpos_graph_context_t& c, unsigned this_index)
+  hb_vector_t<unsigned> split_subtables (gsubgpos_graph_context_t& c,
+                                         unsigned parent_index,
+                                         unsigned this_index)
   {
     hb_set_t visited;
 
@@ -261,7 +263,7 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2<S
     split_context_t split_context {
       c,
       this,
-      this_index,
+      c.graph.duplicate_if_shared (parent_index, this_index),
       std::move (class_to_info),
       c.graph.vertices_[mark_array_id].position_to_index_map (),
     };
@@ -365,8 +367,8 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2<S
 
     classCount = count;
 
-    auto mark_coverage = sc.c.graph.as_table<Coverage> (this_index,
-                                                        &markCoverage);
+    auto mark_coverage = sc.c.graph.as_mutable_table<Coverage> (this_index,
+                                                                &markCoverage);
     if (!mark_coverage) return false;
     hb_set_t marks = sc.marks_for (0, count);
     auto new_coverage =
@@ -380,17 +382,17 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2<S
       return false;
 
 
-    auto base_array = sc.c.graph.as_table<AnchorMatrix> (this_index,
-                                                         &baseArray,
-                                                         old_count);
+    auto base_array = sc.c.graph.as_mutable_table<AnchorMatrix> (this_index,
+                                                                 &baseArray,
+                                                                 old_count);
     if (!base_array || !base_array.table->shrink (sc.c,
                                                   base_array.index,
                                                   old_count,
                                                   count))
       return false;
 
-    auto mark_array = sc.c.graph.as_table<MarkArray> (this_index,
-                                                      &markArray);
+    auto mark_array = sc.c.graph.as_mutable_table<MarkArray> (this_index,
+                                                              &markArray);
     if (!mark_array || !mark_array.table->shrink (sc.c,
                                                   sc.mark_array_links,
                                                   mark_array.index,
@@ -469,11 +471,12 @@ struct MarkBasePosFormat1 : public OT::Layout::GPOS_impl::MarkBasePosFormat1_2<S
 struct MarkBasePos : public OT::Layout::GPOS_impl::MarkBasePos
 {
   hb_vector_t<unsigned> split_subtables (gsubgpos_graph_context_t& c,
+                                         unsigned parent_index,
                                          unsigned this_index)
   {
     switch (u.format) {
     case 1:
-      return ((MarkBasePosFormat1*)(&u.format1))->split_subtables (c, this_index);
+      return ((MarkBasePosFormat1*)(&u.format1))->split_subtables (c, parent_index, this_index);
 #ifndef HB_NO_BORING_EXPANSION
     case 2: HB_FALLTHROUGH;
       // Don't split 24bit PairPos's.

src/graph/pairpos-graph.hh

@@ -47,7 +47,9 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3<SmallType
         min_size + pairSet.get_size () - pairSet.len.get_size();
   }
 
-  hb_vector_t<unsigned> split_subtables (gsubgpos_graph_context_t& c, unsigned this_index)
+  hb_vector_t<unsigned> split_subtables (gsubgpos_graph_context_t& c,
+                                         unsigned parent_index,
+                                         unsigned this_index)
   {
     hb_set_t visited;
 
@@ -81,7 +83,7 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3<SmallType
     split_context_t split_context {
       c,
       this,
-      this_index,
+      c.graph.duplicate_if_shared (parent_index, this_index),
     };
 
     return actuate_subtable_split<split_context_t> (split_context, split_points);
@@ -125,23 +127,19 @@ struct PairPosFormat1 : public OT::Layout::GPOS_impl::PairPosFormat1_3<SmallType
     pairSet.len = count;
     c.graph.vertices_[this_index].obj.tail -= (old_count - count) * SmallTypes::size;
 
-    unsigned coverage_id = c.graph.mutable_index_for_offset (this_index, &coverage);
-    unsigned coverage_size = c.graph.vertices_[coverage_id].table_size ();
-    auto& coverage_v = c.graph.vertices_[coverage_id];
-
-    Coverage* coverage_table = (Coverage*) coverage_v.obj.head;
-    if (!coverage_table || !coverage_table->sanitize (coverage_v))
-      return false;
+    auto coverage = c.graph.as_mutable_table<Coverage> (this_index, &this->coverage);
+    if (!coverage) return false;
 
+    unsigned coverage_size = coverage.vertex->table_size ();
     auto new_coverage =
-        + hb_zip (coverage_table->iter (), hb_range ())
+        + hb_zip (coverage.table->iter (), hb_range ())
         | hb_filter ([&] (hb_pair_t<unsigned, unsigned> p) {
           return p.second < count;
         })
         | hb_map_retains_sorting (hb_first)
         ;
 
-    return Coverage::make_coverage (c, new_coverage, coverage_id, coverage_size);
+    return Coverage::make_coverage (c, new_coverage, coverage.index, coverage_size);
   }
 
   // Create a new PairPos including PairSet's from start (inclusive) to end (exclusive).
@@ -206,7 +204,9 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4<SmallType
         min_size + class1_count * get_class1_record_size ();
   }
 
-  hb_vector_t<unsigned> split_subtables (gsubgpos_graph_context_t& c, unsigned this_index)
+  hb_vector_t<unsigned> split_subtables (gsubgpos_graph_context_t& c,
+                                         unsigned parent_index,
+                                         unsigned this_index)
   {
     const unsigned base_size = OT::Layout::GPOS_impl::PairPosFormat2_4<SmallTypes>::min_size;
     const unsigned class_def_2_size = size_of (c, this_index, &classDef2);
@@ -287,7 +287,7 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4<SmallType
     split_context_t split_context {
       c,
       this,
-      this_index,
+      c.graph.duplicate_if_shared (parent_index, this_index),
       class1_record_size,
       total_value_len,
       value_1_len,
@@ -508,24 +508,18 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4<SmallType
     graph.vertices_[split_context.this_index].obj.tail -=
         (old_count - count) * split_context.class1_record_size;
 
-    unsigned coverage_id =
-        graph.mutable_index_for_offset (split_context.this_index, &coverage);
-    unsigned class_def_1_id =
-        graph.mutable_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
-        || !coverage_table->sanitize (coverage_v)
-        || !class_def_1_table
-        || !class_def_1_table->sanitize (class_def_1_v))
-      return false;
+    auto coverage =
+        graph.as_mutable_table<Coverage> (split_context.this_index, &this->coverage);
+    if (!coverage) return false;
+
+    auto class_def_1 =
+        graph.as_mutable_table<ClassDef> (split_context.this_index, &classDef1);
+    if (!class_def_1) return false;
 
     auto klass_map =
-    + coverage_table->iter ()
+    + coverage.table->iter ()
     | hb_map_retains_sorting ([&] (hb_codepoint_t gid) {
-      return hb_pair_t<hb_codepoint_t, hb_codepoint_t> (gid, class_def_1_table->get_class (gid));
+      return hb_pair_t<hb_codepoint_t, hb_codepoint_t> (gid, class_def_1.table->get_class (gid));
     })
     | hb_filter ([&] (hb_codepoint_t klass) {
       return klass < count;
@@ -534,14 +528,14 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4<SmallType
 
     if (!Coverage::make_coverage (split_context.c,
                                   + klass_map | hb_map_retains_sorting (hb_first),
-                                  coverage_id,
-                                  coverage_v.table_size ()))
+                                  coverage.index,
+                                  coverage.vertex->table_size ()))
       return false;
 
     return ClassDef::make_class_def (split_context.c,
                                      + klass_map,
-                                     class_def_1_id,
-                                     class_def_1_v.table_size ());
+                                     class_def_1.index,
+                                     class_def_1.vertex->table_size ());
   }
 
   hb_hashmap_t<unsigned, unsigned>
@@ -605,13 +599,15 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4<SmallType
 
 struct PairPos : public OT::Layout::GPOS_impl::PairPos
 {
-  hb_vector_t<unsigned> split_subtables (gsubgpos_graph_context_t& c, unsigned this_index)
+  hb_vector_t<unsigned> split_subtables (gsubgpos_graph_context_t& c,
+                                         unsigned parent_index,
+                                         unsigned this_index)
   {
     switch (u.format) {
     case 1:
-      return ((PairPosFormat1*)(&u.format1))->split_subtables (c, this_index);
+      return ((PairPosFormat1*)(&u.format1))->split_subtables (c, parent_index, this_index);
     case 2:
-      return ((PairPosFormat2*)(&u.format2))->split_subtables (c, this_index);
+      return ((PairPosFormat2*)(&u.format2))->split_subtables (c, parent_index, this_index);
 #ifndef HB_NO_BORING_EXPANSION
     case 3: HB_FALLTHROUGH;
     case 4: HB_FALLTHROUGH;

src/hb-repacker.hh

@@ -244,7 +244,7 @@ bool _process_overflows (const hb_vector_t<graph::overflow_record_t>& overflows,
     {
       // The child object is shared, we may be able to eliminate the overflow
       // by duplicating it.
-      if (!sorted_graph.duplicate (r.parent, r.child)) continue;
+      if (sorted_graph.duplicate (r.parent, r.child) == (unsigned) -1) continue;
       return true;
     }