Update classdef size estimator to pick the min coverage format.

Previously this just assumed a worst case format 1.

src/graph/classdef-graph.hh

@@ -26,6 +26,7 @@
 
 #include "graph.hh"
 #include "../hb-ot-layout-common.hh"
+#include <cstdint>
 
 #ifndef GRAPH_CLASSDEF_GRAPH_HH
 #define GRAPH_CLASSDEF_GRAPH_HH
@@ -181,16 +182,18 @@ struct class_def_size_estimator_t
     included_classes.clear();
   }
 
-  // Incremental increase in the Coverage and ClassDef table size
-  // (worst case) if all glyphs associated with 'klass' were added.
-  unsigned incremental_coverage_size (unsigned klass) const
+  // Compute the size of coverage for all glyphs added via 'add_class_def_size'.
+  unsigned coverage_size () const
   {
-    // Coverage takes 2 bytes per glyph worst case,
-    return 2 * glyphs_per_class.get (klass).get_population ();
+    // format 1 is 2 bytes per glyph.
+    unsigned format1_size = 4 + 2 * included_glyphs.get_population();
+    // format 2 is 6 bytes per range.
+    unsigned format2_size = 4 + 6 * num_glyph_ranges();
+    return hb_min(format1_size, format2_size);
   }
 
   // Compute the new size of the ClassDef table if all glyphs associated with 'klass' were added.
-  unsigned class_def_size (unsigned klass)
+  unsigned add_class_def_size (unsigned klass)
   {
     if (!included_classes.has(klass)) {
       // ClassDef 1 takes 2 bytes per glyph.
@@ -213,6 +216,17 @@ struct class_def_size_estimator_t
     return hb_min (class_def_1_size, class_def_2_size);
   }
 
+  unsigned num_glyph_ranges() const {
+    hb_codepoint_t start = HB_SET_VALUE_INVALID;
+    hb_codepoint_t end = HB_SET_VALUE_INVALID;
+
+    unsigned count = 0;
+    while (included_glyphs.next_range (&start, &end)) {
+        count++;
+    }
+    return count;
+  }
+
   bool gids_consecutive() const {
     hb_codepoint_t start = HB_SET_VALUE_INVALID;
     hb_codepoint_t end = HB_SET_VALUE_INVALID;

src/graph/pairpos-graph.hh

@@ -247,8 +247,8 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4<SmallType
     for (unsigned i = 0; i < class1_count; i++)
     {
       unsigned accumulated_delta = class1_record_size;
-      coverage_size += estimator.incremental_coverage_size (i);
-      class_def_1_size = estimator.class_def_size (i);
+      class_def_1_size = estimator.add_class_def_size (i);
+      coverage_size += estimator.coverage_size ();
       max_coverage_size = hb_max (max_coverage_size, coverage_size);
       max_class_def_1_size = hb_max (max_class_def_1_size, class_def_1_size);
 
@@ -282,8 +282,8 @@ struct PairPosFormat2 : public OT::Layout::GPOS_impl::PairPosFormat2_4<SmallType
         accumulated = base_size + accumulated_delta;
 
         estimator.reset();
-        coverage_size = 4 + estimator.incremental_coverage_size (i);
-        class_def_1_size = estimator.class_def_size(i);
+        class_def_1_size = estimator.add_class_def_size(i);
+        coverage_size = estimator.coverage_size();
         visited.clear (); // node sharing isn't allowed between splits.
       }
     }

src/graph/test-classdef-graph.cc

@@ -36,20 +36,20 @@ static bool incremental_size_is (const gid_and_class_list_t& list, unsigned klas
 {
   graph::class_def_size_estimator_t estimator (list.iter ());
 
-  unsigned result = estimator.incremental_coverage_size (klass);
-  if (result != cov_expected)
-  {
-    printf ("FAIL: coverage expected size %u but was %u\n", cov_expected, result);
-    return false;
-  }
-
-  result = estimator.class_def_size (klass);
+  unsigned result = estimator.add_class_def_size (klass);
   if (result != class_def_expected)
   {
     printf ("FAIL: class def expected size %u but was %u\n", class_def_expected, result);
     return false;
   }
 
+  result = estimator.coverage_size ();
+  if (result != cov_expected)
+  {
+    printf ("FAIL: coverage expected size %u but was %u\n", cov_expected, result);
+    return false;
+  }
+
   return true;
 }
 
@@ -58,13 +58,13 @@ static void test_class_and_coverage_size_estimates ()
   // TODO(garretrieger): test against the actual serialized sizes of class def tables
   gid_and_class_list_t empty = {
   };
-  assert (incremental_size_is (empty, 0, 0, 4));
-  assert (incremental_size_is (empty, 1, 0, 4));
+  assert (incremental_size_is (empty, 0, 4, 4));
+  assert (incremental_size_is (empty, 1, 4, 4));
 
   gid_and_class_list_t class_zero = {
     {5, 0},
   };
-  assert (incremental_size_is (class_zero, 0, 2, 4));
+  assert (incremental_size_is (class_zero, 0, 6, 4));
 
   gid_and_class_list_t consecutive = {
     {4, 0},
@@ -76,9 +76,9 @@ static void test_class_and_coverage_size_estimates ()
     {10, 2},
     {11, 2},
   };
-  assert (incremental_size_is (consecutive, 0, 4, 4));
-  assert (incremental_size_is (consecutive, 1, 4, 8));
-  assert (incremental_size_is (consecutive, 2, 8, 10));
+  assert (incremental_size_is (consecutive, 0, 8, 4));
+  assert (incremental_size_is (consecutive, 1, 8, 8));
+  assert (incremental_size_is (consecutive, 2, 10, 10));
 
   gid_and_class_list_t non_consecutive = {
     {4, 0},
@@ -92,9 +92,9 @@ static void test_class_and_coverage_size_estimates ()
     {11, 2},
     {13, 2},
   };
-  assert (incremental_size_is (non_consecutive, 0, 4, 4));
-  assert (incremental_size_is (non_consecutive, 1, 4, 4 + 2*6));
-  assert (incremental_size_is (non_consecutive, 2, 8, 4 + 2*6));
+  assert (incremental_size_is (non_consecutive, 0, 8, 4));
+  assert (incremental_size_is (non_consecutive, 1, 8, 4 + 2*6));
+  assert (incremental_size_is (non_consecutive, 2, 12, 4 + 2*6));
 
   gid_and_class_list_t multiple_ranges = {
     {4, 0},
@@ -109,8 +109,8 @@ static void test_class_and_coverage_size_estimates ()
     {12, 1},
     {13, 1},
   };
-  assert (incremental_size_is (multiple_ranges, 0, 4, 4));
-  assert (incremental_size_is (multiple_ranges, 1, 2 * 6, 4 + 3 * 6));
+  assert (incremental_size_is (multiple_ranges, 0, 8, 4));
+  assert (incremental_size_is (multiple_ranges, 1, 4 + 2 * 6, 4 + 3 * 6));
 }
 
 static void test_running_class_and_coverage_size_estimates () {
@@ -136,13 +136,18 @@ static void test_running_class_and_coverage_size_estimates () {
   };
 
   graph::class_def_size_estimator_t estimator1(consecutive_map.iter());
-  assert(estimator1.class_def_size(1) == 4 + 6);  // format 2, 1 range
-  assert(estimator1.class_def_size(2) == 4 + 10); // format 1, 5 glyphs
-  assert(estimator1.class_def_size(3) == 4 + 18); // format 2, 3 ranges
+  assert(estimator1.add_class_def_size(1) == 4 + 6);  // format 2, 1 range
+  assert(estimator1.coverage_size() == 4 + 6);        // format 2, 1 range
+  assert(estimator1.add_class_def_size(2) == 4 + 10); // format 1, 5 glyphs
+  assert(estimator1.coverage_size() == 4 + 6);        // format 2, 1 range
+  assert(estimator1.add_class_def_size(3) == 4 + 18); // format 2, 3 ranges
+  assert(estimator1.coverage_size() == 4 + 6);        // format 2, 1 range
 
   estimator1.reset();
-  assert(estimator1.class_def_size(2) == 4 + 2); // format 1, 1 glyphs
-  assert(estimator1.class_def_size(3) == 4 + 12); // format 2, 2 ranges
+  assert(estimator1.add_class_def_size(2) == 4 + 2);  // format 1, 1 glyph
+  assert(estimator1.coverage_size() == 4 + 2);        // format 1, 1 glyph
+  assert(estimator1.add_class_def_size(3) == 4 + 12); // format 2, 2 ranges
+  assert(estimator1.coverage_size() == 4 + 6);        // format 1, 1 range
 
   // #### With non-consecutive gids: always uses format 2 ###
   gid_and_class_list_t non_consecutive_map = {
@@ -167,13 +172,18 @@ static void test_running_class_and_coverage_size_estimates () {
   };
 
   graph::class_def_size_estimator_t estimator2(non_consecutive_map.iter());
-  assert(estimator2.class_def_size(1) == 4 + 6);  // format 2, 1 range
-  assert(estimator2.class_def_size(2) == 4 + 18); // format 2, 3 ranges
-  assert(estimator2.class_def_size(3) == 4 + 24); // format 2, 4 ranges
+  assert(estimator2.add_class_def_size(1) == 4 + 6);  // format 2, 1 range
+  assert(estimator2.coverage_size() == 4 + 6);        // format 2, 1 range
+  assert(estimator2.add_class_def_size(2) == 4 + 18); // format 2, 3 ranges
+  assert(estimator2.coverage_size() == 4 + 2 * 6);    // format 1, 6 glyphs
+  assert(estimator2.add_class_def_size(3) == 4 + 24); // format 2, 4 ranges
+  assert(estimator2.coverage_size() == 4 + 3 * 6);    // format 2, 3 ranges
 
   estimator2.reset();
-  assert(estimator2.class_def_size(2) == 4 + 12); // format 1, 1 range
-  assert(estimator2.class_def_size(3) == 4 + 18); // format 2, 2 ranges
+  assert(estimator2.add_class_def_size(2) == 4 + 12); // format 1, 1 range
+  assert(estimator2.coverage_size() == 4 + 4);        // format 1, 2 glyphs
+  assert(estimator2.add_class_def_size(3) == 4 + 18); // format 2, 2 ranges
+  assert(estimator2.coverage_size() == 4 + 2 * 6);    // format 2, 2 ranges
 }
 
 static void test_running_class_size_estimates_with_locally_consecutive_glyphs () {
@@ -184,13 +194,13 @@ static void test_running_class_size_estimates_with_locally_consecutive_glyphs ()
   };
 
   graph::class_def_size_estimator_t estimator(consecutive_map.iter());
-  assert(estimator.class_def_size(1) == 4 + 2);  // format 1, 1 glyph
-  assert(estimator.class_def_size(2) == 4 + 12); // format 2, 2 ranges
-  assert(estimator.class_def_size(3) == 4 + 18); // format 2, 3 ranges
+  assert(estimator.add_class_def_size(1) == 4 + 2);  // format 1, 1 glyph
+  assert(estimator.add_class_def_size(2) == 4 + 12); // format 2, 2 ranges
+  assert(estimator.add_class_def_size(3) == 4 + 18); // format 2, 3 ranges
 
   estimator.reset();
-  assert(estimator.class_def_size(2) == 4 + 2); // format 1, 1 glyphs
-  assert(estimator.class_def_size(3) == 4 + 4); // format 1, 2 glyphs
+  assert(estimator.add_class_def_size(2) == 4 + 2); // format 1, 1 glyphs
+  assert(estimator.add_class_def_size(3) == 4 + 4); // format 1, 2 glyphs
 }
 
 int