Dependency graph: recompute store incoming edge counts when segment definitions change.

Author: garretrieger

ift/encoder/dependency_closure.cc

@@ -213,6 +213,9 @@ DependencyClosure::ComputeIncomingEdgeCount() const {
 
   btree_set<Node> nodes;
   for (segment_index_t s = 0; s < segmentation_info_->Segments().size(); s++) {
+    if (segmentation_info_->Segments().at(s).Definition().Empty()) {
+      continue;
+    }
     nodes.insert(Node::Segment(s));
   }
 
@@ -292,6 +295,11 @@ StatusOr<DependencyClosure::AnalysisAccuracy> DependencyClosure::AnalyzeSegment(
     }
 
     const Segment& segment = segmentation_info_->Segments().at(segment_id);
+    if (segment.Definition().Empty()) {
+      // Empty segments are ignored.
+      continue;
+    }
+
     if (!segment.Definition().feature_tags.empty()) {
       // Feature based segments not yet handled.
       inaccurate_results_++;

ift/encoder/dependency_closure.h

@@ -67,6 +67,13 @@ class DependencyClosure {
                       common::GlyphSet& or_gids,
                       common::GlyphSet& exclusive_gids);
 
+  // This structure caches information derived from the segmentation info segments.
+  // This function signals that segmentation info segments have changed and recomputes
+  // the internal cached information.
+  void SegmentsChanged() {
+    incoming_edge_count_ = ComputeIncomingEdgeCount();
+  }
+
   uint64_t AccurateResults() const { return accurate_results_; }
   uint64_t InaccurateResults() const { return inaccurate_results_; }
 
@@ -198,7 +205,6 @@ class DependencyClosure {
 
   absl::flat_hash_map<hb_codepoint_t, glyph_id_t> unicode_to_gid_;
   std::unique_ptr<hb_depend_t, decltype(&hb_depend_destroy)> dependency_graph_;
-  // TODO XXXXX this needs to be rebuilt whenever segments in seg info are modified.
   absl::flat_hash_map<Node, unsigned> incoming_edge_count_;
 
   // These glyphs may participate in complex substitutions and as a result we can't

ift/encoder/dependency_closure_test.cc

@@ -312,6 +312,28 @@ TEST_F(DependencyClosureTest, BidiMirroring) {
   ASSERT_TRUE(s.ok()) << s;
 }
 
+TEST_F(DependencyClosureTest, SegmentsChanged) {
+  // Test that the dep graph analysis accounts for bidi mirroring in the
+  // harfbuzz closure
+  Reconfigure({}, {
+    {{'a'}, ProbabilityBound::Zero()},
+    {{'b'}, ProbabilityBound::Zero()},
+  });
+
+  Status s = CompareAnalysis({0});
+  ASSERT_TRUE(s.ok()) << s;
+  s = CompareAnalysis({1});
+  ASSERT_TRUE(s.ok()) << s;
+
+  segmentation_info.ReassignInitSubset(closure_cache, {'a'});
+  dependency_closure->SegmentsChanged();
+
+  s = CompareAnalysis({0});
+  ASSERT_TRUE(s.ok()) << s;
+  s = CompareAnalysis({1});
+  ASSERT_TRUE(s.ok()) << s;
+}
+
 }  // namespace ift::encoder
 
 // TODO(garretrieger) more tests (once functionality is available):

ift/encoder/segmentation_context.cc

@@ -103,6 +103,7 @@ Status SegmentationContext::ReassignInitSubset(SubsetDefinition new_def) {
   GlyphSet changed_gids = SegmentationInfo().NonInitFontGlyphs();
   SegmentSet changed_segments = segmentation_info_->ReassignInitSubset(
       glyph_closure_cache, std::move(new_def));
+  depedency_closure_->SegmentsChanged();
 
   // Consider all glyphs moved to the init font as changed.
   changed_gids.subtract(SegmentationInfo().NonInitFontGlyphs());