[subset] Merge remote-tracking branch 'googlefonts/master'

src/hb-open-file-private.hh

@@ -133,10 +133,15 @@ typedef struct OffsetTable
 			 unsigned int table_count)
   {
     TRACE_SERIALIZE (this);
+    // alloc 12 for the OTHeader
     if (unlikely (!c->extend_min (*this))) return_trace (false);
+    // write sfntVersion (bytes 0..3)
     sfnt_version.set (sfnt_tag);
+    // take space for numTables, searchRange, entrySelector, RangeShift
+    // and the TableRecords themselves
     if (unlikely (!tables.serialize (c, table_count))) return_trace (false);
 
+    // write OffsetTables, alloc for and write actual table blobs
     for (unsigned int i = 0; i < table_count; i++)
     {
       TableRecord &rec = tables.array[i];
@@ -145,9 +150,12 @@ typedef struct OffsetTable
       rec.length.set (hb_blob_get_length (blob));
       rec.checkSum.set_for_data (hb_blob_get_data (blob, nullptr), rec.length);
       rec.offset.serialize (c, this);
+      // take room for the table
       void *p = c->allocate_size<void> (rec.length);
       if (unlikely (!p)) {return false;}
+      // copy the actual table
       memcpy (p, hb_blob_get_data (blob, nullptr), rec.length);
+      // 4-byte allignment
       if (rec.length % 4)
 	p = c->allocate_size<void> (4 - rec.length % 4);
     }

src/hb-ot-cmap-table.hh

@@ -193,6 +193,7 @@ struct CmapSubtableLongGroup
 {
   friend struct CmapSubtableFormat12;
   friend struct CmapSubtableFormat13;
+  friend struct cmap;
 
   int cmp (hb_codepoint_t codepoint) const
   {
@@ -253,6 +254,8 @@ struct CmapSubtableFormat10 : CmapSubtableTrimmed<HBUINT32 > {};
 template <typename T>
 struct CmapSubtableLongSegmented
 {
+  friend struct cmap;
+
   inline bool get_glyph (hb_codepoint_t codepoint, hb_codepoint_t *glyph) const
   {
     int i = groups.bsearch (codepoint);
@@ -268,6 +271,20 @@ struct CmapSubtableLongSegmented
     return_trace (c->check_struct (this) && groups.sanitize (c));
   }
 
+  inline bool serialize(hb_serialize_context_t *context,
+                        unsigned int group_count,
+                        Supplier<CmapSubtableLongGroup> &group_supplier)
+  {
+    TRACE_SERIALIZE (this);
+    if (unlikely(!context->extend_min (*this))) return_trace (false);
+    if (unlikely(!groups.serialize(context, group_count))) return_trace (false);
+    for (unsigned int i = 0; i < group_count; i++) {
+      const CmapSubtableLongGroup &group = group_supplier[i];
+      memcpy(&groups[i], &group, sizeof(group));
+    }
+    return true;
+  }
+
   protected:
   HBUINT16	format;		/* Subtable format; set to 12. */
   HBUINT16	reservedZ;	/* Reserved; set to 0. */
@@ -504,25 +521,106 @@ struct cmap
 		  encodingRecord.sanitize (c, this));
   }
 
-  inline bool subset (hb_subset_plan_t *plan, hb_face_t *source, hb_face_t *dest) const
+  inline void populate_groups(hb_auto_array_t<hb_codepoint_t> &codepoints,
+                              hb_auto_array_t<CmapSubtableLongGroup> *groups) const
+  {
+    CmapSubtableLongGroup *group = nullptr;
+    for (unsigned int i = 0; i < codepoints.len; i++) {
+      hb_codepoint_t cp = codepoints[i];
+      if (!group)
+      {
+        group = groups->push();
+        group->startCharCode.set(cp);
+        group->endCharCode.set(cp);
+        group->glyphID.set(i);  // index in codepoints is new gid
+      } else if (cp -1 == group->endCharCode)
+      {
+        group->endCharCode.set(cp);
+      } else
+      {
+        group = nullptr;
+      }
+    }
+
+    DEBUG_MSG(SUBSET, nullptr, "cmap");
+    for (unsigned int i = 0; i < groups->len; i++) {
+      CmapSubtableLongGroup& group = (*groups)[i];
+      DEBUG_MSG(SUBSET, nullptr, "  %d: U+%04X-U+%04X, gid %d-%d", i, (uint32_t) group.startCharCode, (uint32_t) group.endCharCode, (uint32_t) group.glyphID, (uint32_t) group.glyphID + ((uint32_t) group.endCharCode - (uint32_t) group.startCharCode));
+    }
+  }
+
+  hb_bool_t _subset (hb_auto_array_t<CmapSubtableLongGroup> &groups,
+                     size_t dest_sz,
+                     void *dest) const
   {
-    // TODO something useful re: memory, write to dest
-    size_t dest_sz = 64536; // as much as anyone would ever need
-    void *dest_buf = malloc(dest_sz);
-    OT::hb_serialize_context_t context(dest_buf, dest_sz);
+    hb_serialize_context_t context(dest, dest_sz);
+
+    OT::cmap *cmap = context.start_serialize<OT::cmap> ();
+    if (unlikely(!context.extend_min(*cmap)))
+    {
+      return false;
+    }
 
-    // Same version
-    OT::cmap new_cmap;
-    new_cmap.version = version;
-    new_cmap.encodingRecord.len.set(1); // one format 12 subtable
+    cmap->version.set(0);
 
-    // TODO we need to actually build the format 12 subtable
+    if (unlikely(!cmap->encodingRecord.serialize(&context, /* numTables */ 1)))
+    {
+      return false;
+    }
+
+    EncodingRecord &rec = cmap->encodingRecord[0];
+    rec.platformID.set (3); // Windows
+    rec.encodingID.set (1); // Unicode BMP
+
+    CmapSubtable &subtable = rec.subtable.serialize(&context, &rec.subtable);
+    subtable.u.format.set(12);
+
+    CmapSubtableFormat12 &format12 = subtable.u.format12;
+    format12.format.set(12);
+    format12.reservedZ.set(0);
+
+    OT::Supplier<CmapSubtableLongGroup> group_supplier  (&groups[0], groups.len, sizeof (CmapSubtableLongGroup));
+    if (unlikely(!format12.serialize(&context, groups.len, group_supplier)))
+    {
+      return false;
+    }
 
-    // TODO: this fails
-    // out->extend_min(new_cmap);
+    context.end_serialize ();
     return true;
   }
 
+  hb_blob_t * subset (hb_subset_plan_t *plan, hb_face_t *source) const
+  {
+    hb_auto_array_t<CmapSubtableLongGroup> groups;
+
+    populate_groups(plan->codepoints, &groups);
+
+    // We now know how big our blob needs to be
+    // TODO use APIs from the structs to get size?
+    size_t dest_sz = 4 // header
+                   + 8 // 1 EncodingRecord
+                   + 16 // Format 12 header
+                   + 12 * groups.len; // SequentialMapGroup records
+    void *dest = calloc(dest_sz, 1);
+    if (unlikely(!dest)) {
+      DEBUG_MSG(SUBSET, nullptr, "Unable to alloc %ld for cmap subset output", dest_sz);
+      return nullptr;
+    }
+
+    if (unlikely(!_subset(groups, dest_sz, dest)))
+    {
+      free(dest);
+      return nullptr;
+    }
+
+    // all done, write the blob into dest
+    return hb_blob_create((const char *)dest, 
+                          dest_sz,
+                          HB_MEMORY_MODE_READONLY,
+                          /* userdata */ nullptr,
+                          free);
+  }
+
   struct accelerator_t
   {
     inline void init (hb_face_t *face)

src/hb-private.hh

@@ -425,34 +425,46 @@ struct hb_prealloced_array_t
     return &array[len - 1];
   }
 
-  inline bool resize (unsigned int size)
+  // Alloc enouch for size if size < allocated. Don't adjust len.
+  inline bool alloc(unsigned int size)
   {
-    if (unlikely (size > allocated))
+    if (likely (size <= allocated)) 
     {
-      /* Need to reallocate */
-
-      unsigned int new_allocated = allocated;
-      while (size >= new_allocated)
-        new_allocated += (new_allocated >> 1) + 8;
-
-      Type *new_array = nullptr;
-
-      if (array == static_array) {
-	new_array = (Type *) calloc (new_allocated, sizeof (Type));
-	if (new_array)
-	  memcpy (new_array, array, len * sizeof (Type));
-      } else {
-	bool overflows = (new_allocated < allocated) || _hb_unsigned_int_mul_overflows (new_allocated, sizeof (Type));
-	if (likely (!overflows)) {
-	  new_array = (Type *) realloc (array, new_allocated * sizeof (Type));
-	}
+      return true;
+    }
+    /* Need to reallocate */
+
+    unsigned int new_allocated = allocated;
+    while (size >= new_allocated)
+      new_allocated += (new_allocated >> 1) + 8;
+
+    Type *new_array = nullptr;
+
+    if (array == static_array) {
+      new_array = (Type *) calloc (new_allocated, sizeof (Type));
+      if (new_array)
+        memcpy (new_array, array, len * sizeof (Type));
+          } else {
+      bool overflows = (new_allocated < allocated) || _hb_unsigned_int_mul_overflows (new_allocated, sizeof (Type));
+      if (likely (!overflows)) {
+        new_array = (Type *) realloc (array, new_allocated * sizeof (Type));
       }
+    }
 
-      if (unlikely (!new_array))
-	return false;
+    if (unlikely (!new_array))
+      return false;
+
+    array = new_array;
+    allocated = new_allocated;
+    
+    return true;
+  }
 
-      array = new_array;
-      allocated = new_allocated;
+  inline bool resize (unsigned int size)
+  {
+    if (!alloc(size)) 
+    {
+      return false;
     }
 
     len = size;
@@ -495,6 +507,11 @@ struct hb_prealloced_array_t
     return nullptr;
   }
 
+  inline void qsort (int (*cmp)(const void*, const void*))
+  {
+    ::qsort (array, len, sizeof (Type), cmp);
+  }
+
   inline void qsort (void)
   {
     ::qsort (array, len, sizeof (Type), Type::cmp);

src/hb-subset-glyf.cc

@@ -31,14 +31,15 @@
 
 bool
 _calculate_glyf_and_loca_prime_size (const OT::glyf::accelerator_t &glyf,
-                                     hb_set_t *glyph_ids,
-                                     unsigned int *glyf_size /* OUT */,
+                                     hb_auto_array_t<unsigned int> &glyph_ids,
+                                     bool *use_short_loca, /* OUT */
+                                     unsigned int *glyf_size, /* OUT */
                                      unsigned int *loca_size /* OUT */)
 {
   unsigned int total = 0;
   unsigned int count = 0;
-  hb_codepoint_t next_glyph = -1;
-  while (hb_set_next(glyph_ids, &next_glyph)) {
+  for (unsigned int i = 0; i < glyph_ids.len; i++) {
+    hb_codepoint_t next_glyph = glyph_ids[i];
     unsigned int start_offset, end_offset;
     if (unlikely (!glyf.get_offsets (next_glyph, &start_offset, &end_offset))) {
       *glyf_size = 0;
@@ -51,61 +52,82 @@ _calculate_glyf_and_loca_prime_size (const OT::glyf::accelerator_t &glyf,
   }
 
   *glyf_size = total;
-  *loca_size = (count + 1) * sizeof(OT::HBUINT32);
+  *use_short_loca = (total <= 131070);
+  *loca_size = (count + 1)
+      * (*use_short_loca ? sizeof(OT::HBUINT16) : sizeof(OT::HBUINT32));
+
+  DEBUG_MSG(SUBSET, nullptr, "preparing to subset glyf: final size %d, loca size %d, using %s loca",
+            total,
+            *loca_size,
+            *use_short_loca ? "short" : "long");
   return true;
 }
 
+void
+_write_loca_entry (unsigned int id, unsigned int offset, bool is_short, void *loca_prime) {
+  if (is_short) {
+    ((OT::HBUINT16*) loca_prime) [id].set (offset / 2);
+  } else {
+    ((OT::HBUINT32*) loca_prime) [id].set (offset);
+  }
+}
+
 bool
 _write_glyf_and_loca_prime (const OT::glyf::accelerator_t &glyf,
                             const char                    *glyf_data,
-                            const hb_set_t                *glyph_ids,
+                            hb_auto_array_t<unsigned int> &glyph_ids,
+                            bool                           use_short_loca,
                             int                            glyf_prime_size,
                             char                          *glyf_prime_data /* OUT */,
                             int                            loca_prime_size,
                             char                          *loca_prime_data /* OUT */)
 {
-  // TODO(grieger): Handle the missing character glyf and outline.
-
   char *glyf_prime_data_next = glyf_prime_data;
-  OT::HBUINT32 *loca_prime = (OT::HBUINT32*) loca_prime_data;
 
   hb_codepoint_t next_glyph = -1;
   hb_codepoint_t new_glyph_id = 0;
 
-  while (hb_set_next(glyph_ids, &next_glyph)) {
-    unsigned int start_offset, end_offset;
-    if (unlikely (!glyf.get_offsets (next_glyph, &start_offset, &end_offset))) {
+  unsigned int end_offset;
+  for (unsigned int i = 0; i < glyph_ids.len; i++) {
+    unsigned int start_offset;
+    if (unlikely (!glyf.get_offsets (glyph_ids[i], &start_offset, &end_offset))) {
       return false;
     }
 
     int length = end_offset - start_offset;
     memcpy (glyf_prime_data_next, glyf_data + start_offset, length);
-    loca_prime[new_glyph_id].set(start_offset);
+
+    _write_loca_entry (i, start_offset, use_short_loca, loca_prime_data);
 
     glyf_prime_data_next += length;
     new_glyph_id++;
   }
 
+  // Add the last loca entry which doesn't correspond to a specific glyph
+  // but identifies the end of the last glyphs data.
+  _write_loca_entry (new_glyph_id, end_offset, use_short_loca, loca_prime_data);
+
   return true;
 }
 
 bool
 _hb_subset_glyf_and_loca (const OT::glyf::accelerator_t  &glyf,
                           const char                     *glyf_data,
-                          hb_set_t                       *glyphs_to_retain,
+                          hb_auto_array_t<unsigned int>  &glyphs_to_retain,
+                          bool                           *use_short_loca,
                           hb_blob_t                     **glyf_prime /* OUT */,
                           hb_blob_t                     **loca_prime /* OUT */)
 {
   // TODO(grieger): Sanity check writes to make sure they are in-bounds.
   // TODO(grieger): Sanity check allocation size for the new table.
-  // TODO(grieger): Subset loca simultaneously.
   // TODO(grieger): Don't fail on bad offsets, just dump them.
-  // TODO(grieger): Support short loca output.
 
   unsigned int glyf_prime_size;
   unsigned int loca_prime_size;
+
   if (unlikely (!_calculate_glyf_and_loca_prime_size (glyf,
                                                       glyphs_to_retain,
+                                                      use_short_loca,
                                                       &glyf_prime_size,
                                                       &loca_prime_size))) {
     return false;
@@ -114,6 +136,7 @@ _hb_subset_glyf_and_loca (const OT::glyf::accelerator_t  &glyf,
   char *glyf_prime_data = (char *) calloc (glyf_prime_size, 1);
   char *loca_prime_data = (char *) calloc (loca_prime_size, 1);
   if (unlikely (!_write_glyf_and_loca_prime (glyf, glyf_data, glyphs_to_retain,
+                                             *use_short_loca,
                                              glyf_prime_size, glyf_prime_data,
                                              loca_prime_size, loca_prime_data))) {
     free (glyf_prime_data);
@@ -144,7 +167,8 @@ _hb_subset_glyf_and_loca (const OT::glyf::accelerator_t  &glyf,
 bool
 hb_subset_glyf_and_loca (hb_subset_plan_t *plan,
                          hb_face_t        *face,
-                         hb_blob_t       **glyf_prime /* OUT */,
+                         bool             *use_short_loca, /* OUT */
+                         hb_blob_t       **glyf_prime, /* OUT */
                          hb_blob_t       **loca_prime /* OUT */)
 {
   hb_blob_t *glyf_blob = OT::Sanitizer<OT::glyf>().sanitize (face->reference_table (HB_OT_TAG_glyf));
@@ -152,10 +176,15 @@ hb_subset_glyf_and_loca (hb_subset_plan_t *plan,
 
   OT::glyf::accelerator_t glyf;
   glyf.init(face);
-  bool result = _hb_subset_glyf_and_loca (glyf, glyf_data, plan->glyphs_to_retain, glyf_prime, loca_prime);
+  bool result = _hb_subset_glyf_and_loca (glyf,
+                                          glyf_data,
+                                          plan->gids_to_retain_sorted,
+                                          use_short_loca,
+                                          glyf_prime,
+                                          loca_prime);
   glyf.fini();
 
-  // TODO(grieger): Subset loca
+  *use_short_loca = false;
 
   return result;
 }

src/hb-subset-glyf.hh

@@ -34,6 +34,7 @@
 bool
 hb_subset_glyf_and_loca (hb_subset_plan_t *plan,
                          hb_face_t        *face,
+                         bool             *use_short_loca, /* OUT */
                          hb_blob_t       **glyf_prime /* OUT */,
                          hb_blob_t       **loca_prime /* OUT */);
 

src/hb-subset-plan.cc

@@ -29,47 +29,90 @@
 #include "hb-subset-plan.hh"
 #include "hb-ot-cmap-table.hh"
 
+int
+_hb_codepoint_t_cmp (const void *l, const void *r) {
+  return *((hb_codepoint_t *) l) - *((hb_codepoint_t *) r);
+}
+
 hb_bool_t
-hb_subset_plan_new_gid_for_old_id(hb_subset_plan_t *plan,
-                                  hb_codepoint_t old_gid,
-                                  hb_codepoint_t *new_gid) {
-  // TODO(Q1) lookup in map from old:new gid
-  // TEMPORARY: just loop over ids to retain and count up
-  hb_codepoint_t current = -1;
-  hb_codepoint_t count = 0;
-  while (hb_set_next(plan->glyphs_to_retain, &current)) {
-    if (old_gid == current) {
-      *new_gid = count;
+hb_subset_plan_new_gid_for_old_id (hb_subset_plan_t *plan,
+                                   hb_codepoint_t old_gid,
+                                   hb_codepoint_t *new_gid)
+{
+  // the index in old_gids is the new gid; only up to codepoints.len are valid
+  for (unsigned int i = 0; i < plan->gids_to_retain_sorted.len; i++) {
+    if (plan->gids_to_retain_sorted[i] == old_gid) {
+      *new_gid = i;
       return true;
     }
-    count++;
   }
   return false;
 }
 
-hb_set_t *
-glyph_ids_to_retain (hb_face_t *face,
-                     hb_set_t  *codepoints)
+void
+_populate_codepoints (hb_set_t *input_codepoints,
+                      hb_auto_array_t<hb_codepoint_t>& plan_codepoints)
+{
+  plan_codepoints.alloc (hb_set_get_population (input_codepoints));
+  hb_codepoint_t cp = -1;
+  while (hb_set_next (input_codepoints, &cp)) {
+    hb_codepoint_t *wr = plan_codepoints.push();
+    *wr = cp;
+  }
+  plan_codepoints.qsort (_hb_codepoint_t_cmp);
+}
+
+void
+_populate_gids_to_retain (hb_face_t *face,
+                          hb_auto_array_t<hb_codepoint_t>& codepoints,
+                          hb_auto_array_t<hb_codepoint_t>& old_gids,
+                          hb_auto_array_t<hb_codepoint_t>& old_gids_sorted)
 {
   OT::cmap::accelerator_t cmap;
   cmap.init (face);
-  hb_codepoint_t cp = -1;
-  hb_set_t *gids = hb_set_create();
-  while (hb_set_next(codepoints, &cp)) {
+
+  hb_auto_array_t<unsigned int> bad_indices;
+
+  old_gids.alloc (codepoints.len);
+  bool has_zero = false;
+  for (unsigned int i = 0; i < codepoints.len; i++) {
     hb_codepoint_t gid;
-    if (cmap.get_nominal_glyph(cp, &gid)) {
-      DEBUG_MSG(SUBSET, nullptr, "gid for U+%04X is %d", cp, gid);
-      hb_set_add(gids, gid);
-    } else {
-      DEBUG_MSG(SUBSET, nullptr, "Unable to resolve gid for U+%04X", cp);
+    if (!cmap.get_nominal_glyph (codepoints[i], &gid)) {
+      gid = -1;
+      *(bad_indices.push ()) = i;
     }
+    if (gid == 0) {
+      has_zero = true;
+    }
+    *(old_gids.push ()) = gid;
+  }
+
+  while (bad_indices.len > 0) {
+    unsigned int i = bad_indices[bad_indices.len - 1];
+    bad_indices.pop ();
+    DEBUG_MSG(SUBSET, nullptr, "Drop U+%04X; no gid", codepoints[i]);
+    codepoints.remove (i);
+    old_gids.remove (i);
+  }
+
+  // Populate a second glyph id array that is sorted by glyph id
+  // and is gauranteed to contain 0.
+  old_gids_sorted.alloc (old_gids.len + (has_zero ? 0 : 1));
+  for (unsigned int i = 0; i < old_gids.len; i++) {
+    *(old_gids_sorted.push ()) = old_gids[i];
+  }
+  if (!has_zero)
+    *(old_gids_sorted.push ()) = 0;
+  old_gids_sorted.qsort (_hb_codepoint_t_cmp);
+
+  for (unsigned int i = 0; i < codepoints.len; i++) {
+      DEBUG_MSG(SUBSET, nullptr, " U+%04X, old_gid %d, new_gid %d", codepoints[i], old_gids[i], i);
   }
 
   // TODO(Q1) expand with glyphs that make up complex glyphs
   // TODO expand with glyphs reached by G*
   //
   cmap.fini ();
-  return gids;
 }
 
 /**
@@ -88,7 +131,11 @@ hb_subset_plan_create (hb_face_t           *face,
                        hb_subset_input_t   *input)
 {
   hb_subset_plan_t *plan = hb_object_create<hb_subset_plan_t> ();
-  plan->glyphs_to_retain = glyph_ids_to_retain (face, input->codepoints);
+  _populate_codepoints (input->codepoints, plan->codepoints);
+  _populate_gids_to_retain (face,
+                            plan->codepoints,
+                            plan->gids_to_retain,
+                            plan->gids_to_retain_sorted);
   return plan;
 }
 
@@ -96,7 +143,6 @@ hb_subset_plan_t *
 hb_subset_plan_get_empty ()
 {
   hb_subset_plan_t *plan = hb_object_create<hb_subset_plan_t> ();
-  plan->glyphs_to_retain = hb_set_get_empty();
   return plan;
 }
 
@@ -110,6 +156,8 @@ hb_subset_plan_destroy (hb_subset_plan_t *plan)
 {
   if (!hb_object_destroy (plan)) return;
 
-  hb_set_destroy (plan->glyphs_to_retain);
+  plan->codepoints.finish ();
+  plan->gids_to_retain.finish ();
+  plan->gids_to_retain_sorted.finish ();
   free (plan);
 }

src/hb-subset-plan.hh

@@ -21,7 +21,7 @@
  * ON AN "AS IS" BASIS, AND THE COPYRIGHT HOLDER HAS NO OBLIGATION TO
  * PROVIDE MAINTENANCE, SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS.
  *
- * Google Author(s): Garret Rieger
+ * Google Author(s): Garret Rieger, Roderick Sheeter
  */
 
 #ifndef HB_SUBSET_PLAN_HH
@@ -35,7 +35,12 @@ struct hb_subset_plan_t {
   hb_object_header_t header;
   ASSERT_POD ();
 
-  hb_set_t *glyphs_to_retain;
+  // TODO(Q1) actual map, drop this crap
+  // Look at me ma, I'm a poor mans map codepoint : new gid
+  // codepoints is sorted and aligned with gids_to_retain.
+  hb_auto_array_t<hb_codepoint_t> codepoints;
+  hb_auto_array_t<hb_codepoint_t> gids_to_retain;
+  hb_auto_array_t<hb_codepoint_t> gids_to_retain_sorted;
 };
 
 typedef struct hb_subset_plan_t hb_subset_plan_t;

src/hb-subset.cc

@@ -108,22 +108,22 @@ hb_subset_input_destroy(hb_subset_input_t *subset_input)
 }
 
 template<typename TableType>
-hb_bool_t
-subset (hb_subset_plan_t *plan, hb_face_t *source, hb_face_t *dest)
+hb_blob_t *
+_subset (hb_subset_plan_t *plan, hb_face_t *source)
 {
     OT::Sanitizer<TableType> sanitizer;
-    hb_blob_t *table_blob = sanitizer.sanitize (source->reference_table (TableType::tableTag));
-    if (unlikely(!table_blob)) {
+    hb_blob_t *source_blob = sanitizer.sanitize (source->reference_table (TableType::tableTag));
+    if (unlikely(!source_blob)) {
       DEBUG_MSG(SUBSET, nullptr, "Failed to reference table for tag %d", TableType::tableTag);
-      return false;
+      return nullptr;
     }
-    const TableType *table = OT::Sanitizer<TableType>::lock_instance (table_blob);
-    hb_bool_t result = table->subset(plan, source, dest);
+    const TableType *table = OT::Sanitizer<TableType>::lock_instance (source_blob);
+    hb_blob_t *result = table->subset(plan, source);
 
-    hb_blob_destroy (table_blob);
+    hb_blob_destroy (source_blob);
 
-    // TODO string not numeric tag
-    DEBUG_MSG(SUBSET, nullptr, "Subset %d %s", TableType::tableTag, result ? "success" : "FAILED!");
+    hb_tag_t tag = TableType::tableTag;
+    DEBUG_MSG(SUBSET, nullptr, "Subset %c%c%c%c %s", HB_UNTAG(tag), result ? "success" : "FAILED!");
     return result;
 }
 
@@ -242,7 +242,6 @@ hb_subset_face_add_table (hb_face_t *face, hb_tag_t tag, hb_blob_t *blob)
     return false;
 
   hb_subset_face_data_t *data = (hb_subset_face_data_t *) face->user_data;
-
   hb_subset_face_data_t::table_entry_t *entry = data->tables.push ();
   if (unlikely (!entry))
     return false;
@@ -253,6 +252,87 @@ hb_subset_face_add_table (hb_face_t *face, hb_tag_t tag, hb_blob_t *blob)
   return true;
 }
 
+bool
+_add_head_and_set_loca_version (hb_face_t *source, bool use_short_loca, hb_face_t *dest)
+{
+  hb_blob_t *head_blob = OT::Sanitizer<OT::head>().sanitize (hb_face_reference_table (source, HB_OT_TAG_head));
+  const OT::head *head = OT::Sanitizer<OT::head>::lock_instance (head_blob);
+  bool has_head = (head != nullptr);
+
+  if (has_head) {
+    OT::head *head_prime = (OT::head *) calloc (OT::head::static_size, 1);
+    memcpy (head_prime, head, OT::head::static_size);
+    head_prime->indexToLocFormat.set (use_short_loca ? 0 : 1);
+
+    hb_blob_t *head_prime_blob = hb_blob_create ((const char*) head_prime,
+                                                 OT::head::static_size,
+                                                 HB_MEMORY_MODE_WRITABLE,
+                                                 head_prime,
+                                                 free);
+    has_head = has_head && hb_subset_face_add_table (dest, HB_OT_TAG_head, head_prime_blob);
+
+    hb_blob_destroy (head_prime_blob);
+  }
+
+  hb_blob_destroy (head_blob);
+
+  return has_head;
+}
+
+bool
+_subset_glyf (hb_subset_plan_t *plan, hb_face_t *source, hb_face_t *dest)
+{
+  hb_blob_t *glyf_prime = nullptr;
+  hb_blob_t *loca_prime = nullptr;
+
+  bool success = true;
+  bool use_short_loca = false;
+  // TODO(grieger): Migrate to subset function on the table like cmap.
+  if (hb_subset_glyf_and_loca (plan, source, &use_short_loca, &glyf_prime, &loca_prime)) {
+    success = success && hb_subset_face_add_table (dest, HB_OT_TAG_glyf, glyf_prime);
+    success = success && hb_subset_face_add_table (dest, HB_OT_TAG_loca, loca_prime);
+    success = success && _add_head_and_set_loca_version (source, use_short_loca, dest);
+  } else {
+    success = false;
+  }
+  hb_blob_destroy (loca_prime);
+  hb_blob_destroy (glyf_prime);
+
+  return success;
+}
+
+bool
+_subset_table (hb_subset_plan_t *plan,
+               hb_face_t        *source,
+               hb_tag_t          tag,
+               hb_blob_t        *source_blob,
+               hb_face_t        *dest)
+{
+  // TODO (grieger): Handle updating the head table (loca format + num glyphs)
+  DEBUG_MSG(SUBSET, nullptr, "begin subset %c%c%c%c", HB_UNTAG(tag));
+  hb_blob_t *dest_blob;
+  switch (tag) {
+    case HB_OT_TAG_glyf:
+      return _subset_glyf (plan, source, dest);
+    case HB_OT_TAG_head:
+      // SKIP head, it's handled by glyf
+      return true;
+    case HB_OT_TAG_loca:
+      // SKIP loca, it's handle by glyf
+      return true;
+    case HB_OT_TAG_cmap:
+      dest_blob = _subset<const OT::cmap> (plan, source);
+      break;
+    default:
+      dest_blob = source_blob;
+      break;
+  } 
+  DEBUG_MSG(SUBSET, nullptr, "subset %c%c%c%c %s", HB_UNTAG(tag), dest_blob ? "ok" : "FAILED");
+  if (unlikely(!dest_blob)) return false;
+  if (unlikely(!hb_subset_face_add_table (dest, tag, dest_blob))) return false;
+  return true;
+}
+
 /**
  * hb_subset:
  * @source: font face data to be subset.
@@ -266,61 +346,28 @@ hb_subset (hb_face_t *source,
 	   hb_subset_profile_t *profile,
            hb_subset_input_t *input)
 {
-  if (unlikely (!profile || !input || !source)) return nullptr;
+  if (unlikely (!profile || !input || !source)) return hb_face_get_empty();
 
   hb_subset_plan_t *plan = hb_subset_plan_create (source, profile, input);
 
-  hb_face_t *face = hb_subset_face_create ();
-
-  /* Copy tables to new face. */
-  {
-    hb_tag_t table_tags[32];
-    unsigned int offset = 0, count;
-    do {
-      count = ARRAY_LENGTH (table_tags);
-      hb_face_get_table_tags (source, offset, &count, table_tags);
-      for (unsigned int i = 0; i < count; i++)
-      {
-	hb_tag_t tag = table_tags[i];
-	hb_blob_t *blob = hb_face_reference_table (source, tag);
-	hb_subset_face_add_table (face, tag, blob);
-	hb_blob_destroy (blob);
-      }
-    } while (count == ARRAY_LENGTH (table_tags));
-  }
-
-  hb_codepoint_t old_gid = -1;
-  while (hb_set_next (plan->glyphs_to_retain, &old_gid)) {
-    hb_codepoint_t new_gid;
-    if (hb_subset_plan_new_gid_for_old_id (plan, old_gid, &new_gid)) {
-      DEBUG_MSG (SUBSET, nullptr, "Remap %d : %d", old_gid, new_gid);
-    } else {
-      DEBUG_MSG (SUBSET, nullptr, "Remap %d : DOOM! No new ID", old_gid);
-    }
-  }
-  // TODO:
-  // - Create initial header + table directory
-  // - Loop through the set of tables to be kept:
-  //   - Perform table specific subsetting if defined.
-  //   - copy the table into the output.
-  // - Fix header + table directory.
-
+  hb_face_t *dest = hb_subset_face_create ();
+  hb_tag_t table_tags[32];
+  unsigned int offset = 0, count;
   bool success = true;
+  do {
+    count = ARRAY_LENGTH (table_tags);
+    hb_face_get_table_tags (source, offset, &count, table_tags);
+    for (unsigned int i = 0; i < count; i++)
+    {
+      hb_tag_t tag = table_tags[i];
+      hb_blob_t *blob = hb_face_reference_table (source, tag);
+      success = success && _subset_table (plan, source, tag, blob, dest);
+      hb_blob_destroy (blob);
+    }
+  } while (count == ARRAY_LENGTH (table_tags));
 
-  hb_face_t *dest = nullptr; // TODO allocate dest
-
-  hb_blob_t *glyf_prime = nullptr;
-  hb_blob_t *loca_prime = nullptr;
-  if (hb_subset_glyf_and_loca (plan, source, &glyf_prime, &loca_prime)) {
-    // TODO: write new glyf and loca to new face.
-  } else {
-    success = false;
-  }
-  hb_blob_destroy (glyf_prime);
-
-  success = success && subset<const OT::cmap>(plan, source, dest);
-
-  hb_subset_plan_destroy (plan);
-
-  return face;
+  // TODO(grieger): Remove once basic subsetting is working + tests updated.
+  hb_face_destroy (dest);
+  hb_face_reference (source);
+  return success ? source : hb_face_get_empty();
 }

util/hb-subset.cc

@@ -59,7 +59,6 @@ struct subset_consumer_t
       gunichar cp = g_utf8_get_char(c);
       hb_codepoint_t hb_cp = cp; // TODO(Q1) is this safe?
       hb_set_add(codepoints, hb_cp);
-      g_print ("  U+%04X %" G_GINT32_FORMAT "\n", cp, cp);
     } while ((c = g_utf8_find_next_char(c, text + text_len)) != nullptr);
   }