[subset] discard extra copies of a table in face builder.

Fixes #2361. Stores tables in the builder in a hashmap so you end up with at most one copy of each table. Table serialization order is now based on tag sort order instead of order of insertion into the builder.

src/hb-face.cc

@@ -33,6 +33,7 @@
 #include "hb-open-file.hh"
 #include "hb-ot-face.hh"
 #include "hb-ot-cmap-table.hh"
+#include "hb-map.hh"
 
 
 /**
@@ -636,7 +637,7 @@ struct hb_face_builder_data_t
     hb_blob_t *blob;
   };
 
-  hb_vector_t<table_entry_t> tables;
+  hb_hashmap_t<hb_tag_t, hb_blob_t*, (unsigned)-1, nullptr> tables;
 };
 
 static hb_face_builder_data_t *
@@ -656,8 +657,8 @@ _hb_face_builder_data_destroy (void *user_data)
 {
   hb_face_builder_data_t *data = (hb_face_builder_data_t *) user_data;
 
-  for (unsigned int i = 0; i < data->tables.length; i++)
+  for (hb_blob_t* b : data->tables.values())
-    hb_blob_destroy (data->tables[i].blob);
+    hb_blob_destroy (b);
 
   data->tables.fini ();
 
@@ -668,11 +669,11 @@ static hb_blob_t *
 _hb_face_builder_data_reference_blob (hb_face_builder_data_t *data)
 {
 
-  unsigned int table_count = data->tables.length;
+  unsigned int table_count = data->tables.get_population ();
   unsigned int face_length = table_count * 16 + 12;
 
-  for (unsigned int i = 0; i < table_count; i++)
+  for (hb_blob_t* b : data->tables.values())
-    face_length += hb_ceil_to_4 (hb_blob_get_length (data->tables[i].blob));
+    face_length += hb_ceil_to_4 (hb_blob_get_length (b));
 
   char *buf = (char *) hb_malloc (face_length);
   if (unlikely (!buf))
@@ -682,10 +683,23 @@ _hb_face_builder_data_reference_blob (hb_face_builder_data_t *data)
   c.propagate_error (data->tables);
   OT::OpenTypeFontFile *f = c.start_serialize<OT::OpenTypeFontFile> ();
 
-  bool is_cff = data->tables.lsearch (HB_TAG ('C','F','F',' ')) || data->tables.lsearch (HB_TAG ('C','F','F','2'));
+  bool is_cff = (data->tables.has (HB_TAG ('C','F','F',' '))
+                 || data->tables.has (HB_TAG ('C','F','F','2')));
   hb_tag_t sfnt_tag = is_cff ? OT::OpenTypeFontFile::CFFTag : OT::OpenTypeFontFile::TrueTypeTag;
 
-  bool ret = f->serialize_single (&c, sfnt_tag, data->tables.as_array ());
+  // Sort the tags so that produced face is deterministic.
+  hb_set_t tags;
+  + data->tables.keys()
+  | hb_sink (tags)
+  ;
+
+  auto it =
+  + tags.iter()
+  | hb_map ([&] (hb_tag_t _) {
+    return hb_pair (_, data->tables[_]);
+  })
+  ;
+  bool ret = f->serialize_single (&c, sfnt_tag, it);
 
   c.end_serialize ();
 
@@ -706,9 +720,8 @@ _hb_face_builder_reference_table (hb_face_t *face HB_UNUSED, hb_tag_t tag, void
   if (!tag)
     return _hb_face_builder_data_reference_blob (data);
 
-  hb_face_builder_data_t::table_entry_t *entry = data->tables.lsearch (tag);
+  if (data->tables.has (tag))
-  if (entry)
+    return hb_blob_reference (data->tables[tag]);
-    return hb_blob_reference (entry->blob);
 
   return nullptr;
 }
@@ -755,12 +768,11 @@ hb_face_builder_add_table (hb_face_t *face, hb_tag_t tag, hb_blob_t *blob)
 
   hb_face_builder_data_t *data = (hb_face_builder_data_t *) face->user_data;
 
-  hb_face_builder_data_t::table_entry_t *entry = data->tables.push ();
+  if (!data->tables.set (tag, hb_blob_reference (blob)))
-  if (unlikely (data->tables.in_error()))
+  {
+    hb_blob_destroy (blob);
     return false;
-
+  }
-  entry->tag = tag;
-  entry->blob = hb_blob_reference (blob);
 
   return true;
 }

src/hb-open-file.hh

@@ -119,10 +119,10 @@ typedef struct OpenTypeOffsetTable
 
   public:
 
-  template <typename item_t>
+  template <typename Iterator>
   bool serialize (hb_serialize_context_t *c,
 		  hb_tag_t sfnt_tag,
-		  hb_array_t<item_t> items)
+		  Iterator it)
   {
     TRACE_SERIALIZE (this);
     /* Alloc 12 for the OTHeader. */
@@ -131,15 +131,17 @@ typedef struct OpenTypeOffsetTable
     sfnt_version = sfnt_tag;
     /* Take space for numTables, searchRange, entrySelector, RangeShift
      * and the TableRecords themselves.  */
-    if (unlikely (!tables.serialize (c, items.length))) return_trace (false);
+    unsigned num_items = it.len ();
+    if (unlikely (!tables.serialize (c, num_items))) return_trace (false);
 
     const char *dir_end = (const char *) c->head;
     HBUINT32 *checksum_adjustment = nullptr;
 
     /* Write OffsetTables, alloc for and write actual table blobs. */
-    for (unsigned int i = 0; i < tables.len; i++)
+    unsigned i = 0;
+    for (hb_pair_t<hb_tag_t, hb_blob_t*> entry : it)
     {
-      hb_blob_t *blob = items[i].blob;
+      hb_blob_t *blob = entry.second;
       unsigned len = blob->length;
 
       /* Allocate room for the table and copy it. */
@@ -147,7 +149,7 @@ typedef struct OpenTypeOffsetTable
       if (unlikely (!start)) return false;
 
       TableRecord &rec = tables.arrayZ[i];
-      rec.tag = items[i].tag;
+      rec.tag = entry.first;
       rec.length = len;
       rec.offset = 0;
       if (unlikely (!c->check_assign (rec.offset,
@@ -162,7 +164,7 @@ typedef struct OpenTypeOffsetTable
       c->align (4);
       const char *end = (const char *) c->head;
 
-      if (items[i].tag == HB_OT_TAG_head &&
+      if (entry.first == HB_OT_TAG_head &&
 	  (unsigned) (end - start) >= head::static_size)
       {
 	head *h = (head *) start;
@@ -171,6 +173,7 @@ typedef struct OpenTypeOffsetTable
       }
 
       rec.checkSum.set_for_data (start, end - start);
+      i++;
     }
 
     tables.qsort ();
@@ -182,7 +185,7 @@ typedef struct OpenTypeOffsetTable
       /* The following line is a slower version of the following block. */
       //checksum.set_for_data (this, (const char *) c->head - (const char *) this);
       checksum.set_for_data (this, dir_end - (const char *) this);
-      for (unsigned int i = 0; i < items.length; i++)
+      for (unsigned int i = 0; i < num_items; i++)
       {
 	TableRecord &rec = tables.arrayZ[i];
 	checksum = checksum + rec.checkSum;
@@ -489,10 +492,10 @@ struct OpenTypeFontFile
     }
   }
 
-  template <typename item_t>
+  template <typename Iterator>
   bool serialize_single (hb_serialize_context_t *c,
 			 hb_tag_t sfnt_tag,
-			 hb_array_t<item_t> items)
+			 Iterator items)
   {
     TRACE_SERIALIZE (this);
     assert (sfnt_tag != TTCTag);