Always desubroutinize CFF fonts in IFT encoder.

This ensures all glyphs are fully self contained. Also add a integration test for a CFF font extension.

Author: garretrieger

ift/encoder/encoder.cc

@@ -229,10 +229,21 @@ StatusOr<Encoder::Encoding> Encoder::Encode() const {
     //                     extra space in the initial font. However, it would
     //                     require us to examine conditions against each subset
     //                     to determine patch reachability.
+    //
+    // TODO(garretrieger): in the mean time we can use the max glyph id from
+    // fully
+    //                     expanded subset instead. this will at least prune
+    //                     glyphs not used at any extension level.
     uint32_t gid_count = hb_face_get_glyph_count(face_.get());
     if (gid_count > 0) context.base_subset_.gids.insert(gid_count - 1);
   }
 
+  // TODO(garretrieger): when generating the fully expanded subset don't use
+  // retain
+  //                     gids. Save the resulting glyph mapping and use it to
+  //                     translate encoder config gids into the space used by
+  //                     fully expanded subset. This will optimize for cases
+  //                     that don't include the entire original font.
   context.force_long_loca_and_gvar_ = false;
   auto expanded = FullyExpandedSubset(context);
   if (!expanded.ok()) {
@@ -241,6 +252,11 @@ StatusOr<Encoder::Encoding> Encoder::Encode() const {
 
   context.fully_expanded_subset_.shallow_copy(*expanded);
   auto expanded_face = expanded->face();
+
+  // TODO(garretrieger): we don't need to force long gvar anymore. The client is
+  // now capable of
+  //                     upgrading the offset size as needed. Forcing long loca
+  //                     is still needed though.
   context.force_long_loca_and_gvar_ =
       FontHelper::HasLongLoca(expanded_face.get()) ||
       FontHelper::HasWideGvar(expanded_face.get());
@@ -313,8 +329,9 @@ Status Encoder::EnsureGlyphKeyedPatchesPopulated(
     instance.shallow_copy(*result);
   }
 
-  GlyphKeyedDiff differ(instance, compat_id,
-                        {FontHelper::kGlyf, FontHelper::kGvar, FontHelper::kCFF});
+  GlyphKeyedDiff differ(
+      instance, compat_id,
+      {FontHelper::kGlyf, FontHelper::kGvar, FontHelper::kCFF});
 
   for (uint32_t index : reachable_segments) {
     auto e = glyph_data_patches_.find(index);
@@ -565,7 +582,7 @@ StatusOr<FontData> Encoder::GenerateBaseGvar(
 
   // Step 3: extract gvar table.
   hb_blob_unique_ptr gvar_blob = make_hb_blob(
-      hb_face_reference_table(face_builder->get(), HB_TAG('g', 'v', 'a', 'r')));
+      hb_face_reference_table(face_builder->get(), FontHelper::kGvar));
   FontData result(gvar_blob.get());
   return result;
 }
@@ -575,9 +592,18 @@ void Encoder::SetMixedModeSubsettingFlagsIfNeeded(
   if (IsMixedMode()) {
     // Mixed mode requires stable gids set flags accordingly.
     hb_subset_input_set_flags(
-        input, hb_subset_input_get_flags(input) | HB_SUBSET_FLAGS_RETAIN_GIDS |
-                   HB_SUBSET_FLAGS_NOTDEF_OUTLINE |
-                   HB_SUBSET_FLAGS_PASSTHROUGH_UNRECOGNIZED);
+        input,
+        hb_subset_input_get_flags(input) | HB_SUBSET_FLAGS_RETAIN_GIDS |
+            HB_SUBSET_FLAGS_NOTDEF_OUTLINE |
+            HB_SUBSET_FLAGS_PASSTHROUGH_UNRECOGNIZED |
+            // CFF tables are always desubroutinized for mixed mode encoding.
+            // This ensures that for each glyph all data for that glyph is fully
+            // self contained. See: https://w3c.github.io/IFT/Overview.html#cff
+            //
+            // Note: a non desubroutinized mode could be supported, but a
+            // special base CFF table would need to be generated in a similar
+            // style to "GenerateBaseGvar()"
+            HB_SUBSET_FLAGS_DESUBROUTINIZE);
 
     if (context.force_long_loca_and_gvar_) {
       // IFTB requirements flag has the side effect of forcing long loca and
@@ -596,7 +622,9 @@ StatusOr<FontData> Encoder::CutSubset(const ProcessingContext& context,
     return result.status();
   }
 
-  if (IsMixedMode() && def.IsVariable()) {
+  auto tags = FontHelper::GetTags(font);
+  if (IsMixedMode() && def.IsVariable() &&
+      tags.contains(FontHelper::kGvar)) {
     // In mixed mode glyph keyed patches handles gvar, except for when design
     // space is expanded, in which case a gvar table should be patched in that
     // only has coverage of the base (root) subset definition + the current
@@ -605,13 +633,9 @@ StatusOr<FontData> Encoder::CutSubset(const ProcessingContext& context,
     // Create such a gvar table here and overwrite the one that was otherwise
     // generated by the normal subsetting operation. The patch generation will
     // handle including a replacement gvar patch when needed.
-    auto base_gvar = GenerateBaseGvar(context, font, def.design_space);
-    if (!base_gvar.ok()) {
-      return base_gvar.status();
-    }
-
-    hb_blob_unique_ptr gvar_blob = base_gvar->blob();
-    hb_face_builder_add_table(result->get(), HB_TAG('g', 'v', 'a', 'r'),
+    auto base_gvar = TRY(GenerateBaseGvar(context, font, def.design_space));
+    hb_blob_unique_ptr gvar_blob = base_gvar.blob();
+    hb_face_builder_add_table(result->get(), FontHelper::kGvar,
                               gvar_blob.get());
   }
 

ift/encoder/encoder.h

@@ -211,7 +211,8 @@ class Encoder {
 
   static ift::TableKeyedDiff* MixedModeTableKeyedDiff(
       common::CompatId base_compat_id) {
-    return new TableKeyedDiff(base_compat_id, {"IFTX", "glyf", "loca", "gvar", "CFF "});
+    return new TableKeyedDiff(base_compat_id,
+                              {"IFTX", "glyf", "loca", "gvar", "CFF "});
   }
 
   static ift::TableKeyedDiff* ReplaceIftMapTableKeyedDiff(

ift/glyph_keyed_diff_test.cc

@@ -266,11 +266,10 @@ TEST_F(GlyphKeyedDiffTest, CreatePatch_Cff) {
   FontData empty;
   FontData compressed_stream(patch->str(29));
   FontData uncompressed_stream;
-  auto status =
-     unbrotli.Patch(empty, compressed_stream, &uncompressed_stream);
-   ASSERT_TRUE(status.ok()) << status;
-   ASSERT_EQ(uncompressed_stream.str(),
-           absl::string_view((const char*)data_stream_u16_cff, 57));
+  auto status = unbrotli.Patch(empty, compressed_stream, &uncompressed_stream);
+  ASSERT_TRUE(status.ok()) << status;
+  ASSERT_EQ(uncompressed_stream.str(),
+            absl::string_view((const char*)data_stream_u16_cff, 57));
 }
 
 TEST_F(GlyphKeyedDiffTest, CreatePatch_Gvar) {

ift/integration_test.cc

@@ -72,6 +72,10 @@ class IntegrationTest : public ::testing::Test {
         hb_blob_create_from_file("ift/testdata/NotoSansJP-Regular.subset.ttf"));
     noto_sans_jp_.set(blob.get());
 
+    blob = make_hb_blob(
+        hb_blob_create_from_file("common/testdata/NotoSansJP-Regular.otf"));
+    noto_sans_jp_cff_.set(blob.get());
+
     // Noto Sans JP VF
     blob = make_hb_blob(
         hb_blob_create_from_file("ift/testdata/NotoSansJP[wght].subset.ttf"));
@@ -119,6 +123,20 @@ class IntegrationTest : public ::testing::Test {
     return init_segment;
   }
 
+  Status InitEncoderForMixedModeCff(Encoder& encoder) {
+    auto face = noto_sans_jp_cff_.face();
+    encoder.SetFace(face.get());
+
+    auto sc = encoder.AddGlyphDataPatch(1, {34, 35, 46, 47});   // A, B, M, N
+    sc.Update(encoder.AddGlyphDataPatch(2, {41, 42, 43, 59}));  // H, I, J, Z
+
+    if (!sc.ok()) {
+      return sc;
+    }
+
+    return absl::OkStatus();
+  }
+
   StatusOr<btree_set<uint32_t>> InitEncoderForVfMixedMode(Encoder& encoder) {
     auto face = noto_sans_vf_.face();
     encoder.SetFace(face.get());
@@ -202,6 +220,7 @@ class IntegrationTest : public ::testing::Test {
   }
 
   FontData noto_sans_jp_;
+  FontData noto_sans_jp_cff_;
   FontData noto_sans_vf_;
 
   FontData feature_test_;
@@ -1160,4 +1179,92 @@ TEST_F(IntegrationTest, MixedMode_DesignSpaceAugmentation_DropsUnusedPatches) {
   ASSERT_GT(FontHelper::GvarData(extended_face.get(), chunk4_gid)->size(), 0);
 }
 
+StatusOr<FontData> desubroutinize(hb_face_t* font) {
+  hb_subset_input_t* input = hb_subset_input_create_or_fail();
+  if (!input) {
+    return absl::InternalError("failed to create subset input.");
+  }
+
+  hb_subset_input_keep_everything(input);
+  hb_subset_input_set_flags(
+      input, hb_subset_input_get_flags(input) | HB_SUBSET_FLAGS_DESUBROUTINIZE);
+
+  hb_face_t* subset = hb_subset_or_fail(font, input);
+  hb_subset_input_destroy(input);
+
+  if (!subset) {
+    return absl::InternalError("subset operation failed.");
+  }
+
+  FontData result(subset);
+  hb_face_destroy(subset);
+
+  return result;
+}
+
+TEST_F(IntegrationTest, MixedMode_Cff) {
+  Encoder encoder;
+  auto sc = InitEncoderForMixedModeCff(encoder);
+  ASSERT_TRUE(sc.ok()) << sc;
+
+  ASSERT_TRUE(encoder.SetBaseSubset(btree_set<uint32_t>()).ok());
+
+  auto all_codepoints =
+      btree_set<uint32_t>{'A', 'B', 'H', 'I', 'J', 'M', 'N', 'Z'};
+  auto face = noto_sans_jp_cff_.face();
+  encoder.AddNonGlyphDataSegment(all_codepoints);
+
+  // Setup activations for patches 1 and 2
+  sc.Update(encoder.AddGlyphDataPatchCondition(Condition::SimpleCondition(
+      SubsetDefinition::Codepoints(btree_set<uint32_t>{'A', 'B', 'M', 'N'}),
+      1)));
+  sc.Update(encoder.AddGlyphDataPatchCondition(Condition::SimpleCondition(
+      SubsetDefinition::Codepoints(btree_set<uint32_t>{'H', 'I', 'J', 'Z'}),
+      2)));
+
+  auto encoding = encoder.Encode();
+  ASSERT_TRUE(encoding.ok()) << encoding.status();
+  auto encoded_face = encoding->init_font.face();
+
+  ASSERT_EQ(FontHelper::CffData(encoded_face.get(), 34).size(),
+            1);  // empty glyphs in cff are one byte long
+  ASSERT_EQ(FontHelper::CffData(encoded_face.get(), 43).size(),
+            1);  // empty glyphs in cff are one byte long
+
+  auto codepoints = FontHelper::ToCodepointsSet(encoded_face.get());
+  ASSERT_TRUE(codepoints.empty());
+
+  auto extended = Extend(*encoding, {'M'});
+  ASSERT_TRUE(extended.ok()) << extended.status();
+  auto extended_face = extended->face();
+
+  // The encoder desubroutinizes CFF fonts, so generate a desubroutinized
+  // copy of the input face to use for comparisons.
+  auto desubroutinized = desubroutinize(face.get());
+  ASSERT_TRUE(desubroutinized.ok()) << desubroutinized.status();
+  auto desubroutinized_face = desubroutinized->face();
+
+  codepoints = FontHelper::ToCodepointsSet(extended_face.get());
+  ASSERT_EQ(codepoints, all_codepoints);
+
+  // patch 2 gids not present
+  ASSERT_EQ(FontHelper::CffData(encoded_face.get(), 43).size(),
+            1);  // empty glyphs in cff are one byte long
+
+  // patch 1 gids present and match the desubroutinized face.
+  ASSERT_EQ(FontHelper::CffData(extended_face.get(), 34).span(),
+            FontHelper::CffData(desubroutinized_face.get(), 34).span());
+
+  // Second extension
+  encoding->init_font.shallow_copy(*extended);
+  extended = Extend(*encoding, {'H'});
+  ASSERT_TRUE(extended.ok()) << extended.status();
+  extended_face = extended->face();
+
+  ASSERT_EQ(FontHelper::CffData(extended_face.get(), 43).span(),
+            FontHelper::CffData(desubroutinized_face.get(), 43).span());
+  ASSERT_EQ(FontHelper::CffData(extended_face.get(), 34).span(),
+            FontHelper::CffData(desubroutinized_face.get(), 34).span());
+}
+
 }  // namespace ift