[glyf] More complete fixup for composite glyphs with empty components.

This attempts to handle composite-glyph fixup more thoroughly, including the
ability to drop the last component of a composite. [not yet well-tested]

src/glyf.cc

@@ -231,7 +231,7 @@ bool OpenTypeGLYF::ParseSimpleGlyph(Buffer &glyph,
       this->iov.push_back(std::make_pair(glyph.buffer(), 2));
       // output a fixed-up version of the bounding box
       uint8_t* fixed_bbox = new uint8_t[8];
-      fixed_bboxes.push_back(fixed_bbox);
+      replacements.push_back(fixed_bbox);
       xmin = ots_htons(xmin);
       std::memcpy(fixed_bbox, &xmin, 2);
       ymin = ots_htons(ymin);
@@ -266,9 +266,19 @@ bool OpenTypeGLYF::ParseCompositeGlyph(
     unsigned* skip_count) {
   uint16_t flags = 0;
   uint16_t gid = 0;
-  std::vector<std::pair<unsigned, unsigned>> skip_ranges;
+  enum class edit_t : uint8_t {
+    skip_bytes,  // param is number of bytes to skip from offset
+    set_flag,    // param is flag to be set (in the 16-bit field at offset)
+    clear_flag,  // param is flag to be cleared
+  };
+  // List of glyph data edits to be applied: first value is offset in the data,
+  // second is a pair of <edit-action, param>.
+  typedef std::pair<unsigned, std::pair<edit_t, unsigned>> edit_rec;
+  std::vector<edit_rec> edits;
+  unsigned prev_start = 0;
+  bool we_have_instructions = false;
   do {
-    unsigned int start = glyph.offset();
+    unsigned start = glyph.offset();
 
     if (!glyph.ReadU16(&flags) || !glyph.ReadU16(&gid)) {
       return Error("Can't read composite glyph flags or glyphIndex");
@@ -317,28 +327,49 @@ bool OpenTypeGLYF::ParseCompositeGlyph(
     }
 
     if (this->loca->offsets[gid] == this->loca->offsets[gid + 1]) {
+      Warning("empty gid %u used as component in glyph %u", gid, glyph_id);
       // DirectWrite chokes on composite glyphs that have a completely empty glyph
       // as a component; see https://github.com/mozilla/pdf.js/issues/18848.
       // To work around this, we attempt to drop empty components.
-      Font* font = this->GetFont();
-      if (flags & MORE_COMPONENTS) {
-        OTS_WARNING("skipping empty gid %u used as component in composite glyph %u", gid, glyph_id);
-        skip_ranges.push_back(std::make_pair(start, glyph.offset()));
-      } else {
-        // We can't drop the empty glyph if it is the last component, because that would
-        // invalidate the previous component's flags.
-        // TODO: Handle this case? Not currently observed in the wild, so maybe not a priority.
-        OTS_WARNING("empty gid %u used as last component in glyph %u; "
-                    "may fail to render in Windows", gid, glyph_id);
+      // But we don't drop the component if it's the only (remaining) one in the composite.
+      if (prev_start > 0 || (flags & MORE_COMPONENTS)) {
+        if (!(flags & MORE_COMPONENTS)) {
+          // We're dropping the last component, so we need to clear the MORE_COMPONENTS flag
+          // on the previous one.
+          edits.push_back(edit_rec{prev_start, std::make_pair(edit_t::clear_flag, MORE_COMPONENTS)});
+        }
+        // If this component was the first to provide WE_HAVE_INSTRUCTIONS, set it on the previous (if any).
+        if ((flags & WE_HAVE_INSTRUCTIONS) && !we_have_instructions && prev_start > 0) {
+          edits.push_back(edit_rec{prev_start, std::make_pair(edit_t::set_flag, WE_HAVE_INSTRUCTIONS)});
+        }
+        // Finally, skip the actual bytes of this component.
+        edits.push_back(edit_rec{start, std::make_pair(edit_t::skip_bytes, glyph.offset() - start)});
       }
+    } else {
+      // If this is the first component we're keeping, but we already saw WE_HAVE_INSTRUCTIONS
+      // (on a dropped component), we need to ensure that flag is set here.
+      if (prev_start == 0 && we_have_instructions && !(flags & WE_HAVE_INSTRUCTIONS)) {
+        edits.push_back(edit_rec{start, std::make_pair(edit_t::set_flag, WE_HAVE_INSTRUCTIONS)});
+      }
+      prev_start = start;
     }
 
-    // Push inital components on stack at level 1
+    we_have_instructions = we_have_instructions || (flags & WE_HAVE_INSTRUCTIONS);
+
+    // Push initial components on stack at level 1
     // to traverse them in parent function.
     component_point_count->gid_stack.push_back({gid, 1});
   } while (flags & MORE_COMPONENTS);
 
-  if (flags & WE_HAVE_INSTRUCTIONS) {
+  // Sort any required edits by offset in the glyph data.
+  struct {
+    bool operator() (const edit_rec& a, const edit_rec& b) const {
+      return a.first < b.first;
+    }
+  } cmp;
+  std::sort(edits.begin(), edits.end(), cmp);
+
+  if (we_have_instructions) {
     uint16_t bytecode_length;
     if (!glyph.ReadU16(&bytecode_length)) {
       return Error("Can't read instructions size");
@@ -357,18 +388,66 @@ bool OpenTypeGLYF::ParseCompositeGlyph(
     }
   }
 
-  // Append the glyph data to this->iov, skipping any ranges recorded in skip_ranges.
+  // Record the glyph data in this->iov, accounting for any required edits.
   *skip_count = 0;
   unsigned offset = 0;
-  while (!skip_ranges.empty()) {
-    auto& range = skip_ranges.front();
-    if (range.first > offset) {
-      this->iov.push_back(std::make_pair(glyph.buffer() + offset, range.first - offset));
+  while (!edits.empty()) {
+    auto& edit = edits.front();
+    // Handle any glyph data between current offset and the next edit position.
+    if (edit.first > offset) {
+      this->iov.push_back(std::make_pair(glyph.buffer() + offset, edit.first - offset));
+      offset = edit.first;
+    }
+
+    // Handle the edit. Note that there may be multiple set_flag/clear_flag edits
+    // at the same offset, but skip_bytes will never coincide with another edit.
+    auto& action = edit.second;
+    switch (action.first) {
+      case edit_t::set_flag:
+      case edit_t::clear_flag: {
+        // Read the existing flags word.
+        uint16_t flags;
+        std::memcpy(&flags, glyph.buffer() + offset, 2);
+        flags = ots_ntohs(flags);
+        // Apply all flag changes for the current offset.
+        while (!edits.empty() && edits.front().first == offset) {
+          auto& e = edits.front();
+          switch (e.second.first) {
+            case edit_t::set_flag:
+              flags |= e.second.second;
+              break;
+            case edit_t::clear_flag:
+              flags &= ~e.second.second;
+              break;
+            default:
+              assert(false);
+              break;
+          }
+          edits.erase(edits.begin());
+        }
+        // Record the modified flags word.
+        flags = ots_htons(flags);
+        uint8_t* flags_data = new uint8_t[2];
+        std::memcpy(flags_data, &flags, 2);
+        replacements.push_back(flags_data);
+        this->iov.push_back(std::make_pair(flags_data, 2));
+        offset += 2;
+        break;
+      }
+
+      case edit_t::skip_bytes:
+        offset = edit.first + action.second;
+        *skip_count += action.second;
+        edits.erase(edits.begin());
+        break;
+
+      default:
+        assert(false);
+        break;
     }
-    offset = range.second;
-    *skip_count += range.second - range.first;
-    skip_ranges.erase(skip_ranges.begin());
   }
+
+  // Handle any remaining glyph data after the last edit.
   if (glyph.offset() > offset) {
     this->iov.push_back(std::make_pair(glyph.buffer() + offset, glyph.offset() - offset));
   }

src/glyf.h

@@ -21,7 +21,7 @@ class OpenTypeGLYF : public Table {
       : Table(font, tag, tag), maxp(NULL) { }
 
   ~OpenTypeGLYF() {
-    for (auto* p : fixed_bboxes) {
+    for (auto* p : replacements) {
       delete[] p;
     }
   }
@@ -80,7 +80,9 @@ class OpenTypeGLYF : public Table {
 
   std::vector<std::pair<const uint8_t*, size_t> > iov;
 
-  std::vector<uint8_t*> fixed_bboxes;
+  // Any blocks of replacement data created during parsing are stored here
+  // to be available during serialization.
+  std::vector<uint8_t*> replacements;
 };
 
 }  // namespace ots