Refactor GlyphGroupings to use separate exclusive glyphs grouping.

Prior to this change exclusive glyphs were being kep in the 'and glyph' groupings and down stream users needed to know that 'and glyph' groupings of size 1 are exclusive patches. This change moves exclusive groupings to their own map to make it clear and reduce the need for special casing in various other places.

Author: garretrieger

ift/encoder/candidate_merge.cc

@@ -258,8 +258,7 @@ StatusOr<double> CandidateMerge::ComputeCostDelta(
                               modified_conditions));
 
   double cost_delta = 0.0;
-  VLOG(1) << "cost_delta for merge of " << merged_segments.ToString()
-            << " =";
+  VLOG(1) << "cost_delta for merge of " << merged_segments.ToString() << " =";
   if (!moving_to_init_font) {
     // Merge will introduce a new patch (merged_segment) with size
     // "new_patch_size", add the associated cost.

ift/encoder/closure_glyph_segmenter.cc

@@ -202,11 +202,11 @@ StatusOr<std::optional<GlyphSet>> TryMergingABaseSegment(
 
 StatusOr<std::optional<GlyphSet>> MergeSegmentWithHeuristic(
     SegmentationContext& context, uint32_t base_segment_index) {
-  auto base_segment_glyphs = context.glyph_groupings.AndGlyphGroups().find(
-      SegmentSet{base_segment_index});
-  if (base_segment_glyphs == context.glyph_groupings.AndGlyphGroups().end() ||
+  auto base_segment_glyphs =
+      context.glyph_groupings.ExclusiveGlyphs(base_segment_index);
+  if (base_segment_glyphs.empty() ||
       !TRY(CandidateMerge::IsPatchTooSmall(context, base_segment_index,
-                                           base_segment_glyphs->second))) {
+                                           base_segment_glyphs))) {
     // Patch is big enough, no merge is needed.
     return std::nullopt;
   }
@@ -293,16 +293,15 @@ Status CollectExclusiveCandidateMerges(
       return absl::OkStatus();
     }
 
-    SegmentSet triggering_segments{segment_index};
     auto segment_glyphs =
-        context.glyph_groupings.AndGlyphGroups().find(triggering_segments);
-    if (segment_glyphs == context.glyph_groupings.AndGlyphGroups().end() ||
-        segment_glyphs->second.empty()) {
+        context.glyph_groupings.ExclusiveGlyphs(segment_index);
+    if (segment_glyphs.empty()) {
       // This segment has no exclusive glyphs, so no need to consider it for a
       // merge.
       continue;
     }
 
+    SegmentSet triggering_segments{segment_index};
     auto candidate_merge = TRY(CandidateMerge::AssessMerge(
         context, base_segment_index, triggering_segments,
         smallest_candidate_merge));
@@ -361,10 +360,9 @@ StatusOr<std::optional<GlyphSet>> MergeSegmentWithCosts(
   // would be to gather some frequency data, test this approach as is, and then
   // refine it potentially using some of the proposals noted above.
 
-  auto base_segment_glyphs = context.glyph_groupings.AndGlyphGroups().find(
-      SegmentSet{base_segment_index});
-  if (base_segment_glyphs == context.glyph_groupings.AndGlyphGroups().end() ||
-      base_segment_glyphs->second.empty()) {
+  auto base_segment_glyphs =
+      context.glyph_groupings.ExclusiveGlyphs(base_segment_index);
+  if (base_segment_glyphs.empty()) {
     // This base segment has no exclusive glyphs, there's no need to to compute
     // merges.
     return std::nullopt;
@@ -379,8 +377,8 @@ StatusOr<std::optional<GlyphSet>> MergeSegmentWithCosts(
     // If min group size is met, then we will no longer consider merge's that
     // have a positive cost delta so start with an existing smallest candidate
     // set to cost delta 0 which will filter out positive cost delta candidates.
-    unsigned base_size = TRY(
-        context.patch_size_cache->GetPatchSize(base_segment_glyphs->second));
+    unsigned base_size =
+        TRY(context.patch_size_cache->GetPatchSize(base_segment_glyphs));
     smallest_candidate_merge = CandidateMerge::BaselineCandidate(
         base_segment_index, 0.0, base_size, base_segment.Probability(),
         context.GetMergeStrategy().NetworkOverheadCost());
@@ -572,14 +570,8 @@ Status ValidateIncrementalGroupings(hb_face_t* face,
     return absl::FailedPreconditionError("glyph_condition_set isn't correct.");
   }
 
-  if (non_incremental_context.glyph_groupings.AndGlyphGroups() !=
-      context.glyph_groupings.AndGlyphGroups()) {
-    return absl::FailedPreconditionError("and_glyph groups aren't correct.");
-  }
-
-  if (non_incremental_context.glyph_groupings.OrGlyphGroups() !=
-      context.glyph_groupings.OrGlyphGroups()) {
-    return absl::FailedPreconditionError("or_glyph groups aren't correct.");
+  if (non_incremental_context.glyph_groupings != context.glyph_groupings) {
+    return absl::FailedPreconditionError("glyph groups aren't correct.");
   }
 
   return absl::OkStatus();
@@ -694,18 +686,18 @@ Status ClosureGlyphSegmenter::MoveSegmentsToInitFont(
       continue;
     }
 
-    for (const auto& [candidate_segments, _] :
-         context.glyph_groupings.AndGlyphGroups()) {
-      if (candidate_segments.size() <= 1 ||
-          candidate_segments.intersects(excluded)) {
-        // All size 1 segments handled in the previous loop.
-        // Since this is conjunction, having an excluded segment in it's
+    for (const auto& [condition, _] :
+         context.glyph_groupings.ConditionsAndGlyphs()) {
+      if (condition.conditions().size() <= 1 ||
+          condition.TriggeringSegments().intersects(excluded)) {
+        // All size 1 conditions are disjunctive and handled in the previous
+        // loop. Since this is conjunction, having an excluded segment in it's
         // condition makes the probability near 0.
         continue;
       }
 
-      change_made = TRY(CheckAndApplyInitFontMove(candidate_segments, context,
-                                                  initial_segment));
+      change_made = TRY(CheckAndApplyInitFontMove(
+          condition.TriggeringSegments(), context, initial_segment));
       if (change_made) {
         break;
       }

ift/encoder/closure_glyph_segmenter_test.cc

@@ -738,7 +738,8 @@ TEST_F(ClosureGlyphSegmenterTest, TotalCost) {
 
   // Basic no segment case.
   GlyphSegmentation segmentation1({'a', 'b', 'c'}, {}, {});
-  auto sc = GlyphSegmentation::GroupsToSegmentation({}, {}, {}, segmentation1);
+  auto sc =
+      GlyphSegmentation::GroupsToSegmentation({}, {}, {}, {}, segmentation1);
   ASSERT_TRUE(sc.ok()) << sc;
 
   ClosureGlyphSegmenter segmenter;
@@ -750,12 +751,12 @@ TEST_F(ClosureGlyphSegmenterTest, TotalCost) {
 
   // Add some patches
   GlyphSegmentation segmentation2({'a', 'b', 'c'}, {}, {});
-  sc = GlyphSegmentation::GroupsToSegmentation(
-      {
-          {{0}, {100, 101, 102}},
-          {{1}, {103, 104, 105}},
-      },
-      {}, {}, segmentation2);
+  sc = GlyphSegmentation::GroupsToSegmentation({}, {},
+                                               {
+                                                   {0, {100, 101, 102}},
+                                                   {1, {103, 104, 105}},
+                                               },
+                                               {}, segmentation2);
   ASSERT_TRUE(sc.ok()) << sc;
 
   std::vector<SubsetDefinition> segments{

ift/encoder/glyph_groupings.cc

@@ -6,6 +6,7 @@
 #include "ift/encoder/glyph_closure_cache.h"
 #include "ift/encoder/glyph_condition_set.h"
 #include "ift/encoder/requested_segmentation_information.h"
+#include "ift/encoder/types.h"
 
 using absl::btree_set;
 using absl::Status;
@@ -16,15 +17,24 @@ namespace ift::encoder {
 
 void GlyphGroupings::InvalidateGlyphInformation(
     const GlyphConditions& condition, uint32_t gid) {
+  if (condition.and_segments.size() == 1) {
+    segment_index_t s = *condition.and_segments.begin();
+    exclusive_glyph_group_[s].erase(gid);
+    ActivationCondition activation_condition =
+        ActivationCondition::exclusive_segment(s, 0);
+    conditions_and_glyphs_[activation_condition].erase(gid);
+
+    if (exclusive_glyph_group_[s].empty()) {
+      exclusive_glyph_group_.erase(s);
+      RemoveConditionAndGlyphs(activation_condition);
+    }
+  }
+
   auto it = and_glyph_groups_.find(condition.and_segments);
   if (it != and_glyph_groups_.end()) {
     it->second.erase(gid);
     ActivationCondition activation_condition =
         ActivationCondition::and_segments(condition.and_segments, 0);
-    if (condition.and_segments.size() == 1) {
-      activation_condition = ActivationCondition::exclusive_segment(
-          *condition.and_segments.begin(), 0);
-    }
     conditions_and_glyphs_[activation_condition].erase(gid);
 
     if (it->second.empty()) {
@@ -55,15 +65,23 @@ Status GlyphGroupings::GroupGlyphs(
     GlyphClosureCache& closure_cache, const GlyphSet& glyphs) {
   const auto& initial_closure = segmentation_info.InitFontGlyphs();
 
+  SegmentSet modified_exclusive_segments;
   btree_set<SegmentSet> modified_and_groups;
   btree_set<SegmentSet> modified_or_groups;
   for (glyph_id_t gid : glyphs) {
     const auto& condition = glyph_condition_set.ConditionsFor(gid);
 
     if (!condition.and_segments.empty()) {
-      and_glyph_groups_[condition.and_segments].insert(gid);
-      modified_and_groups.insert(condition.and_segments);
+      if (condition.and_segments.size() == 1) {
+        segment_index_t s = *condition.and_segments.begin();
+        exclusive_glyph_group_[s].insert(gid);
+        modified_exclusive_segments.insert(s);
+      } else {
+        and_glyph_groups_[condition.and_segments].insert(gid);
+        modified_and_groups.insert(condition.and_segments);
+      }
     }
+
     if (!condition.or_segments.empty()) {
       or_glyph_groups_[condition.or_segments].insert(gid);
       modified_or_groups.insert(condition.or_segments);
@@ -76,14 +94,12 @@ Status GlyphGroupings::GroupGlyphs(
     }
   }
 
-  for (const auto& and_group : modified_and_groups) {
-    if (and_group.size() == 1) {
-      auto condition =
-          ActivationCondition::exclusive_segment(*and_group.begin(), 0);
-      AddConditionAndGlyphs(condition, and_glyph_groups_[and_group]);
-      continue;
-    }
+  for (segment_index_t s : modified_exclusive_segments) {
+    auto condition = ActivationCondition::exclusive_segment(s, 0);
+    AddConditionAndGlyphs(condition, exclusive_glyph_group_[s]);
+  }
 
+  for (const auto& and_group : modified_and_groups) {
     auto condition = ActivationCondition::and_segments(and_group, 0);
     AddConditionAndGlyphs(condition, and_glyph_groups_[and_group]);
   }

ift/encoder/glyph_groupings.h

@@ -33,6 +33,14 @@ class GlyphGroupings {
     }
   }
 
+  bool operator==(const GlyphGroupings& other) {
+    return and_glyph_groups_ == other.and_glyph_groups_ &&
+           or_glyph_groups_ == other.or_glyph_groups_ &&
+           exclusive_glyph_group_ == other.exclusive_glyph_group_;
+  }
+
+  bool operator!=(const GlyphGroupings& other) { return !(*this == other); }
+
   const absl::btree_map<ActivationCondition, common::GlyphSet>&
   ConditionsAndGlyphs() const {
     return conditions_and_glyphs_;
@@ -54,14 +62,13 @@ class GlyphGroupings {
     return result;
   }
 
-  const absl::btree_map<common::SegmentSet, common::GlyphSet>& AndGlyphGroups()
-      const {
-    return and_glyph_groups_;
-  }
-
-  const absl::btree_map<common::SegmentSet, common::GlyphSet>& OrGlyphGroups()
-      const {
-    return or_glyph_groups_;
+  const common::GlyphSet& ExclusiveGlyphs(segment_index_t s) const {
+    static const common::GlyphSet empty{};
+    auto it = exclusive_glyph_group_.find(s);
+    if (it != exclusive_glyph_group_.end()) {
+      return it->second;
+    }
+    return empty;
   }
 
   // Returns a list of conditions which include segment.
@@ -95,8 +102,8 @@ class GlyphGroupings {
   // segment).
   void AddGlyphsToExclusiveGroup(segment_index_t exclusive_segment,
                                  const common::GlyphSet& glyphs) {
-    auto& and_glyphs = and_glyph_groups_[common::SegmentSet{exclusive_segment}];
-    and_glyphs.union_set(glyphs);
+    auto& exc_glyphs = exclusive_glyph_group_[exclusive_segment];
+    exc_glyphs.union_set(glyphs);
 
     ActivationCondition condition =
         ActivationCondition::exclusive_segment(exclusive_segment, 0);
@@ -119,8 +126,11 @@ class GlyphGroupings {
         segmentation_info.InitFontSegmentWithoutDefaults(),
         segmentation_info.InitFontGlyphs(), unmapped_glyphs_);
     segmentation.CopySegments(segmentation_info.SegmentSubsetDefinitions());
+
     TRYV(GlyphSegmentation::GroupsToSegmentation(
-        and_glyph_groups_, or_glyph_groups_, fallback_segments_, segmentation));
+        and_glyph_groups_, or_glyph_groups_, exclusive_glyph_group_,
+        fallback_segments_, segmentation));
+
     return segmentation;
   }
 
@@ -143,6 +153,7 @@ class GlyphGroupings {
 
   absl::btree_map<common::SegmentSet, common::GlyphSet> and_glyph_groups_;
   absl::btree_map<common::SegmentSet, common::GlyphSet> or_glyph_groups_;
+  absl::btree_map<segment_index_t, common::GlyphSet> exclusive_glyph_group_;
 
   // An alternate representation of and/or_glyph_groups_, derived from them.
   absl::btree_map<ActivationCondition, common::GlyphSet> conditions_and_glyphs_;

ift/encoder/glyph_segmentation.cc

@@ -35,30 +35,20 @@ namespace ift::encoder {
 Status GlyphSegmentation::GroupsToSegmentation(
     const btree_map<SegmentSet, GlyphSet>& and_glyph_groups,
     const btree_map<SegmentSet, GlyphSet>& or_glyph_groups,
+    const btree_map<segment_index_t, GlyphSet>& exclusive_glyph_groups,
     const SegmentSet& fallback_group, GlyphSegmentation& segmentation) {
   patch_id_t next_id = 0;
   segmentation.patches_.clear();
   segmentation.conditions_.clear();
 
   // Map segments into patch ids
-  for (const auto& [and_segments, glyphs] : and_glyph_groups) {
-    if (and_segments.size() != 1) {
-      continue;
-    }
-
-    segment_index_t segment = *and_segments.begin();
+  for (const auto& [segment, glyphs] : exclusive_glyph_groups) {
     segmentation.patches_.insert(std::pair(next_id, glyphs));
-    // All 1 segment and conditions are considered to be exclusive
     segmentation.conditions_.insert(
         ActivationCondition::exclusive_segment(segment, next_id++));
   }
 
   for (const auto& [and_segments, glyphs] : and_glyph_groups) {
-    if (and_segments.size() == 1) {
-      // already processed above
-      continue;
-    }
-
     segmentation.patches_.insert(std::pair(next_id, glyphs));
     segmentation.conditions_.insert(
         ActivationCondition::and_segments(and_segments, next_id));

ift/encoder/glyph_segmentation.h

@@ -96,6 +96,8 @@ class GlyphSegmentation {
           and_glyph_groups,
       const absl::btree_map<common::SegmentSet, common::GlyphSet>&
           or_glyph_groups,
+      const absl::btree_map<segment_index_t, common::GlyphSet>&
+          exclusive_glyph_groups,
       const common::SegmentSet& fallback_group,
       GlyphSegmentation& segmentation);
 

ift/encoder/segmentation_context.cc

@@ -96,12 +96,12 @@ StatusOr<segment_index_t> SegmentationContext::ComputeSegmentCutoff() const {
   double total_cost = 0.0;
   double overhead = merge_strategy_.NetworkOverheadCost();
   for (segment_index_t s : active_segments_) {
-    auto segment_glyphs = glyph_groupings.AndGlyphGroups().find(SegmentSet{s});
-    if (segment_glyphs == glyph_groupings.AndGlyphGroups().end()) {
+    auto segment_glyphs = glyph_groupings.ExclusiveGlyphs(s);
+    if (segment_glyphs.empty()) {
       continue;
     }
 
-    double size = TRY(patch_size_cache->GetPatchSize(segment_glyphs->second));
+    double size = TRY(patch_size_cache->GetPatchSize(segment_glyphs));
     double probability = segmentation_info_.Segments()[s].Probability();
     total_cost += probability * (size + overhead);
   }
@@ -112,12 +112,12 @@ StatusOr<segment_index_t> SegmentationContext::ComputeSegmentCutoff() const {
   for (auto it = active_segments_.rbegin(); it != active_segments_.rend();
        it++) {
     segment_index_t s = *it;
-    auto segment_glyphs = glyph_groupings.AndGlyphGroups().find(SegmentSet{s});
-    if (segment_glyphs == glyph_groupings.AndGlyphGroups().end()) {
+    auto segment_glyphs = glyph_groupings.ExclusiveGlyphs(s);
+    if (segment_glyphs.empty()) {
       continue;
     }
 
-    double size = TRY(patch_size_cache->GetPatchSize(segment_glyphs->second));
+    double size = TRY(patch_size_cache->GetPatchSize(segment_glyphs));
     double probability = segmentation_info_.Segments()[s].Probability();
     cutoff_tail_cost -= probability * (size + overhead);
     if (cutoff_tail_cost < 0.0) {