Add ClosureGlyphSegmenter test which uses dep graph analysis.

Author: garretrieger

ift/encoder/closure_glyph_segmenter_test.cc

@@ -24,9 +24,6 @@ using ift::freq::UnigramProbabilityCalculator;
 
 namespace ift::encoder {
 
-// TODO XXXXX add full roboto type test which uses the alternate closure analysis modes.
-// verify result is the same as pure closure.
-
 class ClosureGlyphSegmenterTest : public ::testing::Test {
  protected:
   ClosureGlyphSegmenterTest()
@@ -35,7 +32,8 @@ class ClosureGlyphSegmenterTest : public ::testing::Test {
         noto_sans_jp(make_hb_face(nullptr)),
         segmenter(8, 8, PATCH, CLOSURE_ONLY),
         segmenter_find_conditions(8, 8, FIND_CONDITIONS, CLOSURE_ONLY),
-        segmenter_move_to_init_font(8, 8, MOVE_TO_INIT_FONT, CLOSURE_ONLY) {
+        segmenter_move_to_init_font(8, 8, MOVE_TO_INIT_FONT, CLOSURE_ONLY),
+        segmenter_dep_graph(8, 8, PATCH, CLOSURE_AND_DEP_GRAPH) {
     roboto = from_file("common/testdata/Roboto-Regular.ttf");
     noto_nastaliq_urdu =
         from_file("common/testdata/NotoNastaliqUrdu.subset.ttf");
@@ -59,6 +57,7 @@ class ClosureGlyphSegmenterTest : public ::testing::Test {
   ClosureGlyphSegmenter segmenter;
   ClosureGlyphSegmenter segmenter_find_conditions;
   ClosureGlyphSegmenter segmenter_move_to_init_font;
+  ClosureGlyphSegmenter segmenter_dep_graph;
 };
 
 TEST_F(ClosureGlyphSegmenterTest, SimpleSegmentation) {
@@ -515,7 +514,7 @@ if ((s2 OR s3)) then p5
 )");
 }
 
-TEST_F(ClosureGlyphSegmenterTest, FullRoboto_WithFeatures) {
+TEST_F(ClosureGlyphSegmenterTest, FullRoboto_WithFeaturesAndDepGraph) {
   auto codepoints = common::FontHelper::ToCodepointsSet(roboto.get());
 
   uint32_t num_segments = 412;
@@ -545,6 +544,14 @@ TEST_F(ClosureGlyphSegmenterTest, FullRoboto_WithFeatures) {
   auto segmentation = segmenter.CodepointToGlyphSegments(
       roboto.get(), {}, segments, MergeStrategy::Heuristic(4000, 12000));
   ASSERT_TRUE(segmentation.ok()) << segmentation.status();
+
+  auto segmentation_dep_graph = segmenter_dep_graph.CodepointToGlyphSegments(
+      roboto.get(), {}, segments, MergeStrategy::Heuristic(4000, 12000));
+  ASSERT_TRUE(segmentation_dep_graph.ok()) << segmentation_dep_graph.status();
+
+  // Dep graph should produce identical results.
+  ASSERT_EQ(segmentation->Segments(), segmentation_dep_graph->Segments());
+  ASSERT_EQ(segmentation->GidSegments(), segmentation_dep_graph->GidSegments());
 }
 
 TEST_F(ClosureGlyphSegmenterTest, CostRequiresFrequencies) {

ift/encoder/dependency_closure.h

@@ -41,17 +41,27 @@ class DependencyClosure {
   };
 
   // Attempts to analyze the given segment using a glyph dependency graph
-  // from harfbuzz. Returns true if a accurate analysis is possible, otherwise
-  // false.
+  // from harfbuzz. The return value signals if an analysis was able to be performed
+  // or not.
   //
-  // When false is returned GlyphClosureCache should be used instead to analyze
-  // the segment.
+  // If ACCURATE is returned then the dep graph is able to produce an analysis
+  // which should match GlyphClosureCache::AnalyzeSegment(). In this case
+  // the three output sets (and_gids, or_gids, and exclusive_gids) will be populated
+  // with the anaysis results.
   //
-  // If true is returned then the input sets *_gids will have glyphs appended to
-  // them based on the analysis classification.
+  // Otherwise if INACCURATE is returned then the dep graph analysis was found
+  // to possibly not match GlyphClosureCache::AnalyzeSegment(). The three
+  // output sets will not be modified in this case.
   //
-  // TODO XXXX explain the meaning behind the three gid sets.
-  // TODO XXXX explain the return type.
+  // The three output sets have the following interpretation:
+  // and_gids: these gids have the union of input segments as a conjunctive condition.
+  //           ie. (s_1 U ... U s_n) AND ... -> and_gids
+  //
+  // or_gids: these gids have the union of input segments as a disjunctive condition.
+  //          ie. (s_1 U ... U s_n) OR ... -> or_gids
+  //
+  // exclusive_gids: these gids are exclusively needed by the union of input segments.
+  //                 ie. (s_1 U ... U s_n) -> exclusive_gids
   absl::StatusOr<AnalysisAccuracy> AnalyzeSegment(const common::SegmentSet& segments,
                       common::GlyphSet& and_gids,
                       common::GlyphSet& or_gids,

ift/encoder/segmentation_context.cc

@@ -148,7 +148,6 @@ Status SegmentationContext::AnalyzeSegment(const SegmentSet& segment_ids,
                                            GlyphSet& and_gids,
                                            GlyphSet& or_gids,
                                            GlyphSet& exclusive_gids) {
-  // TODO XXXXX count number of times we were able to use dep analysis.
   ConditionAnalysisMode effective_mode = condition_analysis_mode_;
   GlyphSet dep_and_gids = and_gids;
   GlyphSet dep_or_gids = or_gids;

ift/encoder/subset_definition.cc

@@ -181,9 +181,6 @@ void SubsetDefinition::ConfigureInput(hb_subset_input_t* input,
                                       hb_face_t* face) const {
   codepoints.union_into(hb_subset_input_unicode_set(input));
 
-  // TODO XXXX should we clear the harfbuzz default feature set so that we are
-  //           always working with a known set of features? we would
-  //           use the IFT spec defaults instead.
   hb_set_t* features =
       hb_subset_input_set(input, HB_SUBSET_SETS_LAYOUT_FEATURE_TAG);
   // hb_input_t has a set of layout featurs configured by default, we