When check for init font merges, recompute the segmentation on each iteration.

This gives much more accurate answers for cost delta computations since it takes into account changes made to the init font segment in prior iterations.

Author: garretrieger

ift/encoder/closure_glyph_segmenter.cc

@@ -509,54 +509,51 @@ static StatusOr<std::vector<Segment>> ToSegments(
   return segments;
 }
 
-StatusOr<SegmentationContext> ClosureGlyphSegmenter::MoveSegmentsToInitFont(SegmentationContext& context) const {
+Status ClosureGlyphSegmenter::MoveSegmentsToInitFont(SegmentationContext& context) const {
   if (!context.GetMergeStrategy().InitFontMergeThreshold().has_value()) {
     return absl::FailedPreconditionError("Cannot be called when there is no merge threshold configured.");
   }
 
   double threshold = *context.GetMergeStrategy().InitFontMergeThreshold();
 
   VLOG(0) << "Checking if there are any segments which should be moved into the initial font.";
-  SegmentSet segments_to_move;
-  for (const auto& [c, glyphs] : context.glyph_groupings.ConditionsAndGlyphs()) {
-    SegmentSet candidate_segments = c.TriggeringSegments();
-    if (!candidate_segments.intersects(context.ActiveSegments())) {
-      // Only do this check for things involving active segments, this let's us skip
-      // checks for conditions are are extremely unlikely to benefit from merging
-      // into the init font.
-      continue;
-    }
-
-    double delta = TRY(CandidateMerge::ComputeCostDelta(context, candidate_segments, std::nullopt, 0));
-    if (delta >= threshold * (double) candidate_segments.size()) {
-      // Merging doesn't improve cost, skip.
-      continue;
-    }
-
-    // TODO(garretrieger): to get a more accurate picture we should consider comparing
-    //   to an updated init subset definition on each iteration.
-    segments_to_move.union_set(candidate_segments);
-    VLOG(0) << "  Moving segments " << candidate_segments.ToString() << " into the initial font (cost delta = " << delta << ")";
-  }
 
   SubsetDefinition initial_segment = context.SegmentationInfo().InitFontSegment();
-  std::vector<Segment> segments;
-  for (unsigned i = 0; i < context.SegmentationInfo().Segments().size(); i++) {
-    const Segment& segment = context.SegmentationInfo().Segments()[i];
-    if (segments_to_move.contains(i)) {
-      initial_segment.Union(segment.Definition());
-    } else if (!segment.Definition().Empty()) {
-      segments.push_back(segment);
+  bool change_made;
+  do {
+    change_made = false;
+    SegmentSet segments_to_move;
+    for (const auto& [c, glyphs] : context.glyph_groupings.ConditionsAndGlyphs()) {
+      SegmentSet candidate_segments = c.TriggeringSegments();
+      if (!candidate_segments.intersects(context.ActiveSegments())) {
+        // Only do this check for things involving active segments, this let's us skip
+        // checks for conditions are are extremely unlikely to benefit from merging
+        // into the init font.
+        continue;
+      }
+
+      double delta = TRY(CandidateMerge::ComputeCostDelta(context, candidate_segments, std::nullopt, 0));
+      if (delta >= threshold * (double) candidate_segments.size()) {
+        // Merging doesn't improve cost, skip.
+        continue;
+      }
+
+      // TODO(garretrieger): to get a more accurate picture we should consider comparing
+      //   to an updated init subset definition on each iteration.
+      segments_to_move.union_set(candidate_segments);
+      VLOG(0) << "  Moving segments " << candidate_segments.ToString() << " into the initial font (cost delta = " << delta << ")";
+      for (segment_index_t s : segments_to_move) {
+        initial_segment.Union(context.SegmentationInfo().Segments()[s].Definition());
+      }
+
+      TRYV(context.ReassignInitSubset(initial_segment, segments_to_move));
+      change_made = true;
+      break;
     }
-  }
+  } while (change_made);
 
-  // Reset the context using the new initial font and segment definitions.
-  VLOG(0) << segments_to_move.size() << " segments moved into the initial font. "
-          << "Initial font now has " << initial_segment.codepoints.size() << " codepoints. "
-          << "Initial segmentation plan will be recomputed." << std::endl;
-  MergeStrategy strategy = context.GetMergeStrategy();
-  return TRY(InitializeSegmentationContext(
-      context.original_face.get(), initial_segment, std::move(segments), std::move(strategy)));
+  VLOG(0) << "Initial font now has " << initial_segment.codepoints.size() << " codepoints.";
+  return absl::OkStatus();
 }
 
 StatusOr<GlyphSegmentation> ClosureGlyphSegmenter::CodepointToGlyphSegments(
@@ -585,7 +582,7 @@ StatusOr<GlyphSegmentation> ClosureGlyphSegmenter::CodepointToGlyphSegments(
   // ### First phase of merging is to check for any patches which should be moved to the initial font
   //     (eg. cases where the probability of a patch is ~1.0).
   if (context.GetMergeStrategy().UseCosts() && context.GetMergeStrategy().InitFontMergeThreshold().has_value()) {
-    context = TRY(MoveSegmentsToInitFont(context));
+    TRYV(MoveSegmentsToInitFont(context));
   }
 
   // ### Iteratively merge segments and incrementally reprocess affected data.

ift/encoder/closure_glyph_segmenter.h

@@ -65,7 +65,7 @@ class ClosureGlyphSegmenter {
       const freq::ProbabilityCalculator& probability_calculator) const;
 
   private:
-    absl::StatusOr<SegmentationContext> MoveSegmentsToInitFont(SegmentationContext& context) const;
+    absl::Status MoveSegmentsToInitFont(SegmentationContext& context) const;
 };
 
 }  // namespace ift::encoder

ift/encoder/closure_glyph_segmenter_test.cc

@@ -806,6 +806,75 @@ if (s0) then p0
 )");
 }
 
+
+TEST_F(ClosureGlyphSegmenterTest, InitFontMerging) {
+  // In this test we enable merging of segments into the init font
+  UnicodeFrequencies frequencies{
+      {{'a', 'a'}, 100},
+      {{'b', 'b'},  50},
+      {{'c', 'c'},  50},
+      {{'d', 'd'}, 100},
+
+      // b and c co-occur
+      {{'b', 'c'},  50},
+  };
+
+  MergeStrategy strategy = *MergeStrategy::BigramCostBased(std::move(frequencies));
+  strategy.SetInitFontMergeThreshold(-75);
+
+  auto segmentation = segmenter.CodepointToGlyphSegments(
+      roboto.get(), {},
+      {{'a'}, {'d'}, {'b'}, {'c'}},
+      strategy);
+  ASSERT_TRUE(segmentation.ok()) << segmentation.status();
+
+  //  'a' and 'd' will be moved to the init font, leaving only two segments 'b', 'c'
+  std::vector<SubsetDefinition> expected_segments = {
+      {},
+      {},
+      {'b', 'c'},
+      {},
+  };
+  ASSERT_EQ(segmentation->Segments(), expected_segments);
+
+  ASSERT_EQ(segmentation->ToString(),
+            R"(initial font: { gid0, gid69, gid72 }
+p0: { gid70, gid71 }
+if (s2) then p0
+)");
+}
+
+TEST_F(ClosureGlyphSegmenterTest, InitFontMerging_CommonGlyphs) {
+  UnicodeFrequencies frequencies{
+      {{'A', 'A'}, 1},
+      {{'C', 'C'}, 1},
+      {{0x106, 0x106}, 100}, // Cacute (contains C glyph)
+  };
+
+  MergeStrategy strategy = *MergeStrategy::CostBased(std::move(frequencies));
+  strategy.SetInitFontMergeThreshold(-75);
+
+  auto segmentation = segmenter.CodepointToGlyphSegments(
+      roboto.get(), {},
+      {{0x106}, {'A'}, {'C'}},
+      strategy);
+  ASSERT_TRUE(segmentation.ok()) << segmentation.status();
+
+  std::vector<SubsetDefinition> expected_segments = {
+      {},
+      {'A'},
+      {'C'},
+  };
+  ASSERT_EQ(segmentation->Segments(), expected_segments);
+
+  // C get's covered by the Cacute merge into int, so only A is left in the patch.
+  ASSERT_EQ(segmentation->ToString(),
+            R"(initial font: { gid0, gid39, gid117, gid700 }
+p0: { gid37 }
+if (s1) then p0
+)");
+}
+
 // TODO(garretrieger): add test where or_set glyphs are moved back to unmapped
 // due to found "additional conditions".
 

ift/encoder/requested_segmentation_information.cc

@@ -10,24 +10,8 @@ RequestedSegmentationInformation::RequestedSegmentationInformation(
     std::vector<Segment> segments, SubsetDefinition init_font_segment,
     GlyphClosureCache& closure_cache)
     : segments_(std::move(segments)),
-      init_font_segment_(std::move(init_font_segment)) {
-  SubsetDefinition all;
-  all.Union(init_font_segment_);
-  for (const auto& s : segments_) {
-    all.Union(s.Definition());
-  }
-
-  {
-    auto closure = closure_cache.GlyphClosure(init_font_segment_);
-    if (closure.ok()) {
-      init_font_glyphs_ = std::move(*closure);
-    }
-  }
-
-  auto closure = closure_cache.GlyphClosure(all);
-  if (closure.ok()) {
-    full_closure_ = std::move(*closure);
-  }
+      init_font_segment_() {
+  ReassignInitSubset(closure_cache, std::move(init_font_segment), {});
 }
 
 }  // namespace ift::encoder

ift/encoder/requested_segmentation_information.h

@@ -33,6 +33,32 @@ class RequestedSegmentationInformation {
     return base_segment.Definition().codepoints.size();
   }
 
+  void ReassignInitSubset(GlyphClosureCache& closure_cache, SubsetDefinition new_def, const common::SegmentSet& removed_segments) {
+    for (segment_index_t s : removed_segments) {
+      segments_[s].Clear();
+    }
+
+    init_font_segment_ = std::move(new_def);
+
+    SubsetDefinition all;
+    all.Union(init_font_segment_);
+    for (const auto& s : segments_) {
+      all.Union(s.Definition());
+    }
+
+    {
+      auto closure = closure_cache.GlyphClosure(init_font_segment_);
+      if (closure.ok()) {
+        init_font_glyphs_ = std::move(*closure);
+      }
+    }
+
+    auto closure = closure_cache.GlyphClosure(all);
+    if (closure.ok()) {
+      full_closure_ = std::move(*closure);
+    }
+  }
+
   const SubsetDefinition& InitFontSegment() const { return init_font_segment_; }
 
   // Returns the init font segment with all default always included items

ift/encoder/segmentation_context.h

@@ -52,11 +52,7 @@ class SegmentationContext {
         glyph_groupings(segments),
         merge_strategy_(std::move(strategy)),
         optimization_cutoff_segment_(UINT32_MAX) {
-    for (unsigned i = 0; i < segments.size(); i++) {
-      if (!segments[i].Definition().Empty()) {
-        active_segments_.insert(i);
-      }
-    }
+    ComputeActiveSegments();
   }
 
   // Convert the information in this context into a finalized GlyphSegmentation
@@ -141,6 +137,39 @@ class SegmentationContext {
     glyph_condition_set.InvalidateGlyphInformation(glyphs, segments);
   }
 
+  /*
+   * Invalidates all grouping information and fully reprocesses all segments.
+   */
+  absl::Status ReassignInitSubset(SubsetDefinition new_def, const common::SegmentSet& removed_segments) {
+    unsigned glyph_count = hb_face_get_glyph_count(original_face.get());
+
+    segmentation_info_.ReassignInitSubset(glyph_closure_cache, std::move(new_def), removed_segments);
+    active_segments_.clear();
+    ComputeActiveSegments();
+
+    // All segments depend on the init subset def, so we must reprocess everything.
+    // First reset grouping information:
+    glyph_condition_set = GlyphConditionSet(glyph_count);
+    glyph_groupings = GlyphGroupings(SegmentationInfo().Segments());
+    inert_segments_.clear();
+
+    // Then reprocess segments:
+    for (segment_index_t segment_index = 0;
+       segment_index < SegmentationInfo().Segments().size();
+       segment_index++) {
+      TRY(ReprocessSegment(segment_index));
+    }
+
+    common::GlyphSet all_glyphs;
+    all_glyphs.insert_range(0, glyph_count - 1);
+    TRYV(GroupGlyphs(all_glyphs));
+    glyph_closure_cache.LogClosureCount("Segmentation reprocess for init def change.");
+
+    TRYV(InitOptimizationCutoff());
+
+    return absl::OkStatus();
+  }
+
   // Performs a closure analysis on codepoints and returns the associated
   // and, or, and exclusive glyph sets.
   absl::Status AnalyzeSegment(const common::SegmentSet& segment_ids,
@@ -166,6 +195,15 @@ class SegmentationContext {
   }
 
  private:
+
+  void ComputeActiveSegments() {
+    for (unsigned i = 0; i < segmentation_info_.Segments().size(); i++) {
+      if (!segmentation_info_.Segments()[i].Definition().Empty()) {
+        active_segments_.insert(i);
+      }
+    }
+  }
+
   /*
    * Ensures that the produce segmentation is:
    * - Disjoint (no duplicated glyphs) and doesn't overlap what's in the initial