From e0338a9dce2b31b7e6277cbeebcfb2e54b0d1bd7 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Mon, 9 Mar 2026 22:47:53 +0000 Subject: [PATCH 1/5] Begin cleaning up the public api. Adds a script to list public api headers and ensure they are in turn only using other public api headers. --- check_public_headers.sh | 38 +++++++++++++++++ common/BUILD | 14 +++++-- ift/BUILD | 5 +-- ift/encoder/BUILD | 42 ++++++++++++++----- ift/encoder/candidate_merge_test.cc | 30 ++++++++++--- ift/encoder/closure_glyph_segmenter.cc | 38 ++--------------- ift/encoder/closure_glyph_segmenter.h | 16 +++---- ift/encoder/compiler.h | 1 - ift/encoder/complex_condition_finder_test.cc | 7 +++- .../requested_segmentation_information.h | 1 + ift/encoder/segmentation_context.cc | 37 ++++++++++++++++ ift/encoder/segmentation_context.h | 14 +++++++ ift/encoder/subset_definition.cc | 1 + ift/encoder/subset_definition.h | 2 - ift/feature_registry/BUILD | 2 +- ift/feature_registry/feature_registry.h | 2 +- ift/freq/BUILD | 4 +- ift/proto/BUILD | 8 ++-- util/BUILD | 12 +++++- util/load_codepoints.cc | 1 + util/trace_segmentation_plan.cc | 1 + 21 files changed, 194 insertions(+), 82 deletions(-) create mode 100755 check_public_headers.sh diff --git a/check_public_headers.sh b/check_public_headers.sh new file mode 100755 index 00000000..db312904 --- /dev/null +++ b/check_public_headers.sh @@ -0,0 +1,38 @@ +#!/bin/bash +# +# Checks that all headers referenced from public api headers are also +# in the public set. + +public_hdrs=$(bazel query 'labels(hdrs, attr("visibility", ".*//visibility:public.*", kind("cc_.* rule", //...)))' 2>/dev/null | sed -e 's|^//||' -e 's|:|/|') +echo "$public_hdrs" | sort > /tmp/public_hdrs.txt + +echo "Public header list:" +cat /tmp/public_hdrs.txt +echo "" + +exit_code=0 + +if grep -q "^common/try\.h$" /tmp/public_hdrs.txt; then + echo "Error: common/try.h should not be in the public headers list." + exit_code=1 +fi + +for hdr in $public_hdrs; do + if [[ ! -f "$hdr" ]]; then + continue + fi + + includes=$(grep -h -E '^[ \t]*#[ \t]*include[ \t]+"[^"]+"' "$hdr" | sed -E 's/.*"([^"]+)".*/\1/') + + for inc in $includes; do + if [[ -f "$inc" ]]; then + if ! grep -q "^${inc}$" /tmp/public_hdrs.txt; then + echo "Error: Public header '$hdr' includes non-public header '$inc'" + exit_code=1 + fi + fi + done +done + +rm /tmp/public_hdrs.txt +exit $exit_code diff --git a/common/BUILD b/common/BUILD index 8be01e9b..d603c870 100644 --- a/common/BUILD +++ b/common/BUILD @@ -12,7 +12,6 @@ cc_library( "brotli_binary_diff.cc", "brotli_binary_patch.cc", "compat_id.cc", - "compat_id.h", "file_font_provider.cc", "font_helper.cc", "hb_set_unique_ptr.cc", @@ -37,7 +36,6 @@ cc_library( "hb_set_unique_ptr.h", "int_set.h", "sparse_bit_set.h", - "try.h", "woff2.h", ], visibility = [ @@ -55,9 +53,15 @@ cc_library( "@brotli//:brotlienc", "@harfbuzz", "@woff2", + ":try", ], ) +cc_library( + name = "try", + hdrs = ["try.h"], +) + cc_library( name = "mocks", srcs = [ @@ -71,7 +75,7 @@ cc_library( "mock_font_provider.h", ], visibility = [ - "//visibility:public", + ], deps = [ ":common", @@ -83,7 +87,9 @@ filegroup( name = "testdata", srcs = glob(["testdata/**"]), visibility = [ - "//visibility:public", + "//ift:__subpackages__", + "//brotli:__pkg__", + "//util:__pkg__", ], ) diff --git a/ift/BUILD b/ift/BUILD index 983bdd58..090dd46b 100644 --- a/ift/BUILD +++ b/ift/BUILD @@ -5,12 +5,11 @@ cc_library( name = "ift", srcs = [ "glyph_keyed_diff.cc", - "glyph_keyed_diff.h", "table_keyed_diff.cc", - "table_keyed_diff.h", "url_template.cc", ], hdrs = [ + "glyph_keyed_diff.h", "table_keyed_diff.h", "url_template.h", ], @@ -42,7 +41,7 @@ cc_library( "testdata/test_segments.h", ], visibility = [ - "//visibility:public", + "//ift/encoder:__pkg__", ], deps = [ "@abseil-cpp//absl/container:flat_hash_set", diff --git a/ift/encoder/BUILD b/ift/encoder/BUILD index 6fb3e398..7bf76c41 100644 --- a/ift/encoder/BUILD +++ b/ift/encoder/BUILD @@ -5,17 +5,17 @@ cc_library( name = "encoder", srcs = [ "closure_glyph_segmenter.cc", - "closure_glyph_segmenter.h", "compiler.cc", - "compiler.h", + ], + hdrs = [ + "closure_glyph_segmenter.h", + "compiler.h", ], visibility = [ - "//ift:__subpackages__", - "//ift/client:__pkg__", - "//js_client:__pkg__", - "//util:__pkg__", + "//visibility:public", ], deps = [ + ":merge_strategy", ":activation_condition", ":common", ":segmentation_context", @@ -64,8 +64,6 @@ cc_library( "glyph_groupings.cc", "glyph_groupings.h", "invalidation_set.h", - "merge_strategy.cc", - "merge_strategy.h", "merger.cc", "merger.h", "patch_size_cache.h", @@ -84,6 +82,7 @@ cc_library( visibility = [ ], deps = [ + ":merge_strategy", ":segmentation_info", ":activation_condition", ":common", @@ -124,22 +123,29 @@ cc_library( name = "activation_condition", srcs = [ "activation_condition.cc", - "activation_condition.h", "glyph_segmentation.cc", + ], + hdrs = [ + "activation_condition.h", "glyph_segmentation.h", ], deps = [ ":common", "//ift/freq", ], + visibility = [ + "//visibility:public", + ], ) cc_library( name = "common", srcs = [ + "subset_definition.cc", + ], + hdrs = [ "init_subset_defaults.h", "segment.h", - "subset_definition.cc", "subset_definition.h", "types.h", ], @@ -157,6 +163,22 @@ cc_library( ], ) +cc_library( + name = "merge_strategy", + srcs = [ + "merge_strategy.cc", + ], + hdrs = [ + "merge_strategy.h", + ], + visibility = [ + "//visibility:public", + ], + deps = [ + "//ift/freq", + ], +) + cc_test( name = "compiler_test", size = "small", diff --git a/ift/encoder/candidate_merge_test.cc b/ift/encoder/candidate_merge_test.cc index d025fb79..c146af5f 100644 --- a/ift/encoder/candidate_merge_test.cc +++ b/ift/encoder/candidate_merge_test.cc @@ -123,7 +123,10 @@ TEST_F(CandidateMergeTest, AssessMerge_CostDeltas) { ClosureGlyphSegmenter segmenter(8, 8, PATCH, CLOSURE_ONLY); auto context = - segmenter.InitializeSegmentationContext(roboto.get(), {}, segments); + SegmentationContext::InitializeSegmentationContext( + roboto.get(), {}, segments, segmenter.unmapped_glyph_handling(), + segmenter.condition_analysis_mode(), segmenter.brotli_quality(), + segmenter.init_font_merging_brotli_quality()); ASSERT_TRUE(context.ok()) << context.status(); Merger merger = *Merger::New( @@ -189,7 +192,10 @@ TEST_F(CandidateMergeTest, AssessMerge_WithBestCandidate) { ClosureGlyphSegmenter segmenter(8, 8, PATCH, CLOSURE_ONLY); auto context = - segmenter.InitializeSegmentationContext(roboto.get(), {}, segments); + SegmentationContext::InitializeSegmentationContext( + roboto.get(), {}, segments, segmenter.unmapped_glyph_handling(), + segmenter.condition_analysis_mode(), segmenter.brotli_quality(), + segmenter.init_font_merging_brotli_quality()); ASSERT_TRUE(context.ok()) << context.status(); Merger merger = *Merger::New( @@ -254,7 +260,10 @@ TEST_F(CandidateMergeTest, AssessMerge_CostDeltas_Complex) { ClosureGlyphSegmenter segmenter(8, 8, PATCH, CLOSURE_ONLY); auto context = - segmenter.InitializeSegmentationContext(roboto.get(), {}, segments); + SegmentationContext::InitializeSegmentationContext( + roboto.get(), {}, segments, segmenter.unmapped_glyph_handling(), + segmenter.condition_analysis_mode(), segmenter.brotli_quality(), + segmenter.init_font_merging_brotli_quality()); ASSERT_TRUE(context.ok()) << context.status(); Merger merger = *Merger::New( @@ -306,7 +315,10 @@ TEST_F(CandidateMergeTest, AssessMerge_CostDeltas_Complex_ModifiedConditions) { ClosureGlyphSegmenter segmenter(8, 8, PATCH, CLOSURE_ONLY); auto context = - segmenter.InitializeSegmentationContext(roboto.get(), {}, segments); + SegmentationContext::InitializeSegmentationContext( + roboto.get(), {}, segments, segmenter.unmapped_glyph_handling(), + segmenter.condition_analysis_mode(), segmenter.brotli_quality(), + segmenter.init_font_merging_brotli_quality()); ASSERT_TRUE(context.ok()) << context.status(); Merger merger = *Merger::New( @@ -394,7 +406,10 @@ TEST_F(CandidateMergeTest, AssessPatchMerge) { ClosureGlyphSegmenter segmenter(8, 8, PATCH, CLOSURE_ONLY); auto context = - segmenter.InitializeSegmentationContext(roboto.get(), {}, segments); + SegmentationContext::InitializeSegmentationContext( + roboto.get(), {}, segments, segmenter.unmapped_glyph_handling(), + segmenter.condition_analysis_mode(), segmenter.brotli_quality(), + segmenter.init_font_merging_brotli_quality()); ASSERT_TRUE(context.ok()) << context.status(); Merger merger = *Merger::New( @@ -445,7 +460,10 @@ TEST_F(CandidateMergeTest, AssessPatchMerge_RequiresPatches) { ClosureGlyphSegmenter segmenter(8, 8, PATCH, CLOSURE_ONLY); auto context = - segmenter.InitializeSegmentationContext(roboto.get(), {}, segments); + SegmentationContext::InitializeSegmentationContext( + roboto.get(), {}, segments, segmenter.unmapped_glyph_handling(), + segmenter.condition_analysis_mode(), segmenter.brotli_quality(), + segmenter.init_font_merging_brotli_quality()); ASSERT_TRUE(context.ok()) << context.status(); Merger merger = *Merger::New( diff --git a/ift/encoder/closure_glyph_segmenter.cc b/ift/encoder/closure_glyph_segmenter.cc index 72227678..a20e1309 100644 --- a/ift/encoder/closure_glyph_segmenter.cc +++ b/ift/encoder/closure_glyph_segmenter.cc @@ -520,8 +520,10 @@ StatusOr ClosureGlyphSegmenter::CodepointToGlyphSegments( btree_map with_shared; std::vector segments = TRY(ToOrderedSegments(subset_definitions, merge_groups, with_shared)); - SegmentationContext context = TRY(InitializeSegmentationContext( - face, initial_segment, std::move(segments))); + SegmentationContext context = TRY(SegmentationContext::InitializeSegmentationContext( + face, initial_segment, std::move(segments), + unmapped_glyph_handling_, condition_analysis_mode_, + brotli_quality_, init_font_merging_brotli_quality_)); std::vector mergers = TRY(ToMergers(context, with_shared, merge_groups)); @@ -634,38 +636,6 @@ StatusOr ClosureGlyphSegmenter::CodepointToGlyphSegments( return absl::InternalError("unreachable"); } -StatusOr -ClosureGlyphSegmenter::InitializeSegmentationContext( - hb_face_t* face, SubsetDefinition initial_segment, - std::vector segments) const { - if (!hb_face_get_glyph_count(face)) { - return absl::InvalidArgumentError("Provided font has no glyphs."); - } - - // The IFT compiler has a set of defaults always included in the initial font - // add them here so we correctly factor them into the generated segmentation. - AddInitSubsetDefaults(initial_segment); - - // No merging is done during init. - SegmentationContext context = - TRY(SegmentationContext::Create(face, initial_segment, segments, - unmapped_glyph_handling_, condition_analysis_mode_, - brotli_quality_, init_font_merging_brotli_quality_)); - - // ### Generate the initial conditions and groupings by processing all - // segments and glyphs. ### - VLOG(0) << "Forming initial segmentation plan."; - for (segment_index_t segment_index = 0; - segment_index < context.SegmentationInfo().Segments().size(); - segment_index++) { - TRY(context.ReprocessSegment(segment_index)); - } - - TRYV(context.GroupGlyphs(context.SegmentationInfo().NonInitFontGlyphs(), {})); - - return context; -} - StatusOr ClosureGlyphSegmenter::TotalCost( hb_face_t* original_face, const GlyphSegmentation& segmentation, const ProbabilityCalculator& probability_calculator) const { diff --git a/ift/encoder/closure_glyph_segmenter.h b/ift/encoder/closure_glyph_segmenter.h index d29e074c..288e7d92 100644 --- a/ift/encoder/closure_glyph_segmenter.h +++ b/ift/encoder/closure_glyph_segmenter.h @@ -8,7 +8,6 @@ #include "absl/status/statusor.h" #include "ift/encoder/glyph_segmentation.h" #include "ift/encoder/merge_strategy.h" -#include "ift/encoder/segmentation_context.h" #include "ift/encoder/subset_definition.h" #include "ift/freq/probability_calculator.h" #include "util/common.pb.h" @@ -64,16 +63,6 @@ class ClosureGlyphSegmenter { const std::vector& subset_definitions, absl::btree_map merge_groups) const; - /* - * Generates a segmentation context for the provided segmentation input. - * - * This context will contain the initial groupings without doing any merging. - * Useful for writing tests that require a initialized segmentation context. - */ - absl::StatusOr InitializeSegmentationContext( - hb_face_t* face, SubsetDefinition initial_segment, - std::vector segments) const; - /* * Computes the total cost (expected number of bytes transferred) for a given * segmentation with respect to the provided frequency data. @@ -97,6 +86,11 @@ class ClosureGlyphSegmenter { const std::vector& segments, const SubsetDefinition& init_segment); + uint32_t brotli_quality() const { return brotli_quality_; } + uint32_t init_font_merging_brotli_quality() const { return init_font_merging_brotli_quality_; } + UnmappedGlyphHandling unmapped_glyph_handling() const { return unmapped_glyph_handling_; } + ConditionAnalysisMode condition_analysis_mode() const { return condition_analysis_mode_; } + private: uint32_t brotli_quality_; uint32_t init_font_merging_brotli_quality_; diff --git a/ift/encoder/compiler.h b/ift/encoder/compiler.h index 1a96d3ba..87436ef8 100644 --- a/ift/encoder/compiler.h +++ b/ift/encoder/compiler.h @@ -17,7 +17,6 @@ #include "ift/encoder/activation_condition.h" #include "ift/encoder/subset_definition.h" #include "ift/encoder/types.h" -#include "ift/proto/patch_map.h" #include "ift/table_keyed_diff.h" namespace ift::encoder { diff --git a/ift/encoder/complex_condition_finder_test.cc b/ift/encoder/complex_condition_finder_test.cc index e63cc2ad..198ef9f3 100644 --- a/ift/encoder/complex_condition_finder_test.cc +++ b/ift/encoder/complex_condition_finder_test.cc @@ -54,7 +54,7 @@ class ComplexConditionFinderTest : public ::testing::Test { } SegmentationContext TestContext(bool basic_closure_analysis) { - auto context = *segmenter.InitializeSegmentationContext( + auto context = *SegmentationContext::InitializeSegmentationContext( roboto.get(), {'f'}, { /* 0 */ {{0x54}, ProbabilityBound::Zero()}, @@ -65,7 +65,10 @@ class ComplexConditionFinderTest : public ::testing::Test { /* 5 */ {{0x21A}, ProbabilityBound::Zero()}, /* 6 */ {{0xF6C3}, ProbabilityBound::Zero()}, /* 7 */ {{0x69}, ProbabilityBound::Zero()}, - }); + }, + segmenter.unmapped_glyph_handling(), + segmenter.condition_analysis_mode(), segmenter.brotli_quality(), + segmenter.init_font_merging_brotli_quality()); if (!basic_closure_analysis) { // initialaztion populates the basic conditions, clear those diff --git a/ift/encoder/requested_segmentation_information.h b/ift/encoder/requested_segmentation_information.h index 21d34644..9bbd57e5 100644 --- a/ift/encoder/requested_segmentation_information.h +++ b/ift/encoder/requested_segmentation_information.h @@ -9,6 +9,7 @@ #include "ift/encoder/subset_definition.h" #include "ift/encoder/types.h" #include "util/common.pb.h" +#include "common/try.h" namespace ift::encoder { diff --git a/ift/encoder/segmentation_context.cc b/ift/encoder/segmentation_context.cc index 0404c53c..06003a87 100644 --- a/ift/encoder/segmentation_context.cc +++ b/ift/encoder/segmentation_context.cc @@ -8,6 +8,7 @@ #include "ift/encoder/dependency_closure.h" #include "ift/encoder/glyph_condition_set.h" #include "ift/encoder/glyph_segmentation.h" +#include "ift/encoder/init_subset_defaults.h" #include "ift/encoder/types.h" using absl::Status; @@ -214,4 +215,40 @@ Status SegmentationContext::AnalyzeSegment(const SegmentSet& segment_ids, return absl::OkStatus(); } +StatusOr +SegmentationContext::InitializeSegmentationContext( + hb_face_t* face, SubsetDefinition initial_segment, + std::vector segments, + UnmappedGlyphHandling unmapped_glyph_handling, + ConditionAnalysisMode condition_analysis_mode, + uint32_t brotli_quality, + uint32_t init_font_brotli_quality) { + if (!hb_face_get_glyph_count(face)) { + return absl::InvalidArgumentError("Provided font has no glyphs."); + } + + // The IFT compiler has a set of defaults always included in the initial font + // add them here so we correctly factor them into the generated segmentation. + AddInitSubsetDefaults(initial_segment); + + // No merging is done during init. + SegmentationContext context = + TRY(SegmentationContext::Create(face, initial_segment, segments, + unmapped_glyph_handling, condition_analysis_mode, + brotli_quality, init_font_brotli_quality)); + + // ### Generate the initial conditions and groupings by processing all + // segments and glyphs. ### + VLOG(0) << "Forming initial segmentation plan."; + for (segment_index_t segment_index = 0; + segment_index < context.SegmentationInfo().Segments().size(); + segment_index++) { + TRY(context.ReprocessSegment(segment_index)); + } + + TRYV(context.GroupGlyphs(context.SegmentationInfo().NonInitFontGlyphs(), {})); + + return context; +} + } // namespace ift::encoder \ No newline at end of file diff --git a/ift/encoder/segmentation_context.h b/ift/encoder/segmentation_context.h index da7a0f69..b7a59e15 100644 --- a/ift/encoder/segmentation_context.h +++ b/ift/encoder/segmentation_context.h @@ -73,6 +73,20 @@ class SegmentationContext { return context; } + /* + * Generates a segmentation context for the provided segmentation input. + * + * This context will contain the initial groupings without doing any merging. + * Useful for writing tests that require a initialized segmentation context. + */ + static absl::StatusOr InitializeSegmentationContext( + hb_face_t* face, SubsetDefinition initial_segment, + std::vector segments, + UnmappedGlyphHandling unmapped_glyph_handling, + ConditionAnalysisMode condition_analysis_mode, + uint32_t brotli_quality, + uint32_t init_font_brotli_quality); + private: SegmentationContext( hb_face_t* face, diff --git a/ift/encoder/subset_definition.cc b/ift/encoder/subset_definition.cc index 6b3527c5..ea8557d4 100644 --- a/ift/encoder/subset_definition.cc +++ b/ift/encoder/subset_definition.cc @@ -7,6 +7,7 @@ #include "common/axis_range.h" #include "common/font_helper.h" #include "common/int_set.h" +#include "common/try.h" #include "ift/proto/patch_encoding.h" #include "ift/proto/patch_map.h" diff --git a/ift/encoder/subset_definition.h b/ift/encoder/subset_definition.h index f31cf022..7399b161 100644 --- a/ift/encoder/subset_definition.h +++ b/ift/encoder/subset_definition.h @@ -9,9 +9,7 @@ #include "absl/container/flat_hash_map.h" #include "absl/container/flat_hash_set.h" #include "common/axis_range.h" -#include "common/font_helper.h" #include "common/int_set.h" -#include "common/try.h" #include "hb-subset.h" #include "ift/proto/patch_encoding.h" #include "ift/proto/patch_map.h" diff --git a/ift/feature_registry/BUILD b/ift/feature_registry/BUILD index b70ef365..59af7880 100644 --- a/ift/feature_registry/BUILD +++ b/ift/feature_registry/BUILD @@ -23,7 +23,7 @@ cc_library( "feature_registry.h", ], visibility = [ - "//ift/encoder:__pkg__", + "//visibility:public", ], deps = [ "@abseil-cpp//absl/base:no_destructor", diff --git a/ift/feature_registry/feature_registry.h b/ift/feature_registry/feature_registry.h index 5f4b49b5..d92f6255 100644 --- a/ift/feature_registry/feature_registry.h +++ b/ift/feature_registry/feature_registry.h @@ -5,7 +5,7 @@ #include "absl/base/no_destructor.h" #include "absl/container/flat_hash_set.h" -namespace ift::feature_registry { +namespace ift::featurge_registry { static const absl::flat_hash_set& DefaultFeatureTags() { static const absl::NoDestructor> kDefaultFeatures((absl::flat_hash_set) { diff --git a/ift/freq/BUILD b/ift/freq/BUILD index 2870f2b7..442b8d78 100644 --- a/ift/freq/BUILD +++ b/ift/freq/BUILD @@ -66,7 +66,9 @@ cc_test( cc_library( name = "mock_probability_calculator", hdrs = ["mock_probability_calculator.h"], - visibility = ["//visibility:public"], + visibility = [ + "//ift/encoder:__pkg__", + ], deps = [ ":freq", "//ift/encoder:common", diff --git a/ift/proto/BUILD b/ift/proto/BUILD index 659374cc..78c18faa 100644 --- a/ift/proto/BUILD +++ b/ift/proto/BUILD @@ -8,14 +8,14 @@ cc_library( "format_2_patch_map.h", "ift_table.cc", "ift_table.h", - "patch_encoding.h", "patch_map.cc", + ], + hdrs = [ + "patch_encoding.h", "patch_map.h", ], visibility = [ - "//ift:__pkg__", - "//ift/encoder:__pkg__", - "//util:__pkg__", + "//visibility:public", ], deps = [ "//common", diff --git a/util/BUILD b/util/BUILD index 0dcb17d6..cf7afb63 100644 --- a/util/BUILD +++ b/util/BUILD @@ -7,6 +7,9 @@ load("@rules_cc//cc:cc_test.bzl", "cc_test") proto_library( name = "segmentation_plan_proto", srcs = ["segmentation_plan.proto"], + visibility = [ + "//visibility:public", + ], deps = [ ":common_proto", ], @@ -15,7 +18,7 @@ proto_library( cc_proto_library( name = "segmentation_plan_cc_proto", visibility = [ - "//ift/encoder:__pkg__", + "//visibility:public", ], deps = [ ":segmentation_plan_proto", @@ -150,7 +153,6 @@ cc_library( name = "auto_config_flags", srcs = ["auto_config_flags.cc"], hdrs = ["auto_config_flags.h"], - visibility = ["//visibility:public"], deps = [ "@abseil-cpp//absl/flags:flag", ], @@ -188,6 +190,9 @@ cc_library( "@abseil-cpp//absl/container:flat_hash_set", "@harfbuzz", ], + visibility = [ + "//visibility:public", + ] ) cc_library( @@ -228,6 +233,9 @@ cc_library( "@abseil-cpp//absl/status:statusor", "@harfbuzz", ], + visibility = [ + "//visibility:public", + ] ) cc_test( diff --git a/util/load_codepoints.cc b/util/load_codepoints.cc index a8fd20b5..61f529f4 100644 --- a/util/load_codepoints.cc +++ b/util/load_codepoints.cc @@ -11,6 +11,7 @@ #include "absl/strings/strip.h" #include "common/font_data.h" #include "common/int_set.h" +#include "common/try.h" #include "hb.h" #include "ift/freq/unicode_frequencies.h" #include "riegeli/bytes/fd_reader.h" diff --git a/util/trace_segmentation_plan.cc b/util/trace_segmentation_plan.cc index aa40bb41..f7e90e9b 100644 --- a/util/trace_segmentation_plan.cc +++ b/util/trace_segmentation_plan.cc @@ -11,6 +11,7 @@ #include "common/int_set.h" #include "util/load_codepoints.h" #include "util/segmentation_plan.pb.h" +#include "common/try.h" using absl::Status; using absl::StatusOr; From 017455079da392a5b0f564d2c764d0bbbddb53db Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Mon, 9 Mar 2026 23:55:41 +0000 Subject: [PATCH 2/5] Add namespace's to the protobuf messages. --- ift/dep_graph/dependency_graph_test.cc | 3 +++ ift/encoder/activation_condition.cc | 4 ++++ ift/encoder/activation_condition.h | 2 +- ift/encoder/candidate_merge_test.cc | 4 ++++ ift/encoder/closure_glyph_segmenter.cc | 6 ++++++ ift/encoder/closure_glyph_segmenter.h | 14 ++++++------- ift/encoder/closure_glyph_segmenter_test.cc | 7 +++++++ ift/encoder/complex_condition_finder_test.cc | 4 ++++ ift/encoder/dependency_closure.cc | 3 +++ ift/encoder/dependency_closure_test.cc | 3 +++ ift/encoder/glyph_closure_cache.cc | 3 +++ ift/encoder/glyph_closure_cache_test.cc | 3 +++ ift/encoder/glyph_groupings.cc | 3 +++ ift/encoder/glyph_groupings_test.cc | 4 ++++ ift/encoder/glyph_segmentation.cc | 7 +++++++ ift/encoder/glyph_segmentation.h | 4 ++-- ift/encoder/glyph_segmentation_test.cc | 4 ++++ .../requested_segmentation_information.cc | 3 +++ .../requested_segmentation_information.h | 8 ++++---- ift/encoder/segmentation_context.cc | 6 ++++++ ift/encoder/segmentation_context.h | 20 +++++++++---------- ift/feature_registry/feature_registry.h | 2 +- ift/proto/format_2_patch_map_test.cc | 2 ++ util/auto_segmenter_config.cc | 9 +++++++++ util/auto_segmenter_config.h | 2 +- util/auto_segmenter_config_test.cc | 6 ++++++ util/closure_glyph_keyed_segmenter_util.cc | 6 ++++++ util/common.proto | 2 ++ util/convert_iftb.cc | 7 +++++++ util/convert_iftb.h | 2 +- util/font2ift.cc | 5 +++++ util/generate_segmenter_config.cc | 3 +++ util/generate_table_keyed_config.cc | 4 ++++ util/load_codepoints.cc | 2 ++ util/segmentation_plan.proto | 2 ++ util/segmenter_config.proto | 2 ++ util/segmenter_config_util.cc | 9 +++++++++ util/segmenter_config_util.h | 20 +++++++++---------- util/segmenter_config_util_test.cc | 4 ++++ util/trace_segmentation_plan.cc | 5 +++++ 40 files changed, 172 insertions(+), 37 deletions(-) diff --git a/ift/dep_graph/dependency_graph_test.cc b/ift/dep_graph/dependency_graph_test.cc index 5267d3f1..4b4e1a42 100644 --- a/ift/dep_graph/dependency_graph_test.cc +++ b/ift/dep_graph/dependency_graph_test.cc @@ -13,6 +13,9 @@ #include "gtest/gtest.h" +using ift::proto::PATCH; + + using absl::flat_hash_map; using common::hb_face_unique_ptr; using common::CodepointSet; diff --git a/ift/encoder/activation_condition.cc b/ift/encoder/activation_condition.cc index 65038752..34adbbb5 100644 --- a/ift/encoder/activation_condition.cc +++ b/ift/encoder/activation_condition.cc @@ -11,6 +11,10 @@ #include "ift/proto/patch_encoding.h" #include "ift/proto/patch_map.h" +using ift::proto::ActivationConditionProto; +using ift::proto::SegmentsProto; + + using absl::btree_set; using absl::flat_hash_map; using absl::Span; diff --git a/ift/encoder/activation_condition.h b/ift/encoder/activation_condition.h index fc19d97b..c93d84cf 100644 --- a/ift/encoder/activation_condition.h +++ b/ift/encoder/activation_condition.h @@ -119,7 +119,7 @@ class ActivationCondition { const common::SegmentSet& merged_segments, const Segment& merged_segment, const ift::freq::ProbabilityCalculator& calculator) const; - ActivationConditionProto ToConfigProto() const; + ift::proto::ActivationConditionProto ToConfigProto() const; bool operator<(const ActivationCondition& other) const; diff --git a/ift/encoder/candidate_merge_test.cc b/ift/encoder/candidate_merge_test.cc index c146af5f..d00d2376 100644 --- a/ift/encoder/candidate_merge_test.cc +++ b/ift/encoder/candidate_merge_test.cc @@ -15,6 +15,10 @@ #include "ift/freq/probability_bound.h" #include "ift/freq/unicode_frequencies.h" +using ift::proto::CLOSURE_ONLY; +using ift::proto::PATCH; + + using common::CodepointSet; using common::FontData; using common::hb_face_unique_ptr; diff --git a/ift/encoder/closure_glyph_segmenter.cc b/ift/encoder/closure_glyph_segmenter.cc index a20e1309..fdcfe82a 100644 --- a/ift/encoder/closure_glyph_segmenter.cc +++ b/ift/encoder/closure_glyph_segmenter.cc @@ -36,6 +36,12 @@ #include "ift/freq/probability_calculator.h" #include "ift/glyph_keyed_diff.h" +using ift::proto::SegmentationPlan; +using ift::proto::UnmappedGlyphHandling; +using ift::proto::SegmentsProto; +using ift::proto::MOVE_TO_INIT_FONT; + + using absl::btree_map; using absl::btree_set; using absl::flat_hash_map; diff --git a/ift/encoder/closure_glyph_segmenter.h b/ift/encoder/closure_glyph_segmenter.h index 288e7d92..7f002085 100644 --- a/ift/encoder/closure_glyph_segmenter.h +++ b/ift/encoder/closure_glyph_segmenter.h @@ -38,8 +38,8 @@ class ClosureGlyphSegmenter { public: ClosureGlyphSegmenter(uint32_t brotli_quality, uint32_t init_font_merging_brotli_quality, - UnmappedGlyphHandling unmapped_glyph_handling, - ConditionAnalysisMode condition_analysis_mode) + ift::proto::UnmappedGlyphHandling unmapped_glyph_handling, + ift::proto::ConditionAnalysisMode condition_analysis_mode) : brotli_quality_(brotli_quality), init_font_merging_brotli_quality_(init_font_merging_brotli_quality), unmapped_glyph_handling_(unmapped_glyph_handling), @@ -81,21 +81,21 @@ class ClosureGlyphSegmenter { uint32_t& all_glyphs_size) const; static void AddTableKeyedSegments( - SegmentationPlan& plan, + ift::proto::SegmentationPlan& plan, const absl::btree_map& merge_groups, const std::vector& segments, const SubsetDefinition& init_segment); uint32_t brotli_quality() const { return brotli_quality_; } uint32_t init_font_merging_brotli_quality() const { return init_font_merging_brotli_quality_; } - UnmappedGlyphHandling unmapped_glyph_handling() const { return unmapped_glyph_handling_; } - ConditionAnalysisMode condition_analysis_mode() const { return condition_analysis_mode_; } + ift::proto::UnmappedGlyphHandling unmapped_glyph_handling() const { return unmapped_glyph_handling_; } + ift::proto::ConditionAnalysisMode condition_analysis_mode() const { return condition_analysis_mode_; } private: uint32_t brotli_quality_; uint32_t init_font_merging_brotli_quality_; - UnmappedGlyphHandling unmapped_glyph_handling_; - ConditionAnalysisMode condition_analysis_mode_; + ift::proto::UnmappedGlyphHandling unmapped_glyph_handling_; + ift::proto::ConditionAnalysisMode condition_analysis_mode_; }; } // namespace ift::encoder diff --git a/ift/encoder/closure_glyph_segmenter_test.cc b/ift/encoder/closure_glyph_segmenter_test.cc index 62d2f74f..9c4e2368 100644 --- a/ift/encoder/closure_glyph_segmenter_test.cc +++ b/ift/encoder/closure_glyph_segmenter_test.cc @@ -12,6 +12,13 @@ #include "ift/freq/unicode_frequencies.h" #include "ift/freq/unigram_probability_calculator.h" +using ift::proto::CLOSURE_ONLY; +using ift::proto::CLOSURE_AND_VALIDATE_DEP_GRAPH; +using ift::proto::PATCH; +using ift::proto::MOVE_TO_INIT_FONT; +using ift::proto::FIND_CONDITIONS; + + using absl::btree_map; using absl::StatusOr; using common::CodepointSet; diff --git a/ift/encoder/complex_condition_finder_test.cc b/ift/encoder/complex_condition_finder_test.cc index 198ef9f3..ced785ec 100644 --- a/ift/encoder/complex_condition_finder_test.cc +++ b/ift/encoder/complex_condition_finder_test.cc @@ -10,6 +10,10 @@ #include "ift/encoder/types.h" #include "ift/freq/probability_bound.h" +using ift::proto::CLOSURE_ONLY; +using ift::proto::PATCH; + + using absl::btree_map; using absl::StatusOr; using common::FontData; diff --git a/ift/encoder/dependency_closure.cc b/ift/encoder/dependency_closure.cc index 0cbb6a48..f4b9659e 100644 --- a/ift/encoder/dependency_closure.cc +++ b/ift/encoder/dependency_closure.cc @@ -10,6 +10,9 @@ #include "ift/encoder/requested_segmentation_information.h" #include "ift/encoder/types.h" +using ift::proto::Features; + + using absl::btree_set; using absl::flat_hash_map; using absl::flat_hash_set; diff --git a/ift/encoder/dependency_closure_test.cc b/ift/encoder/dependency_closure_test.cc index 1157f4f6..cf482b53 100644 --- a/ift/encoder/dependency_closure_test.cc +++ b/ift/encoder/dependency_closure_test.cc @@ -15,6 +15,9 @@ #include "util/common.pb.h" #include "ift/encoder/requested_segmentation_information.h" +using ift::proto::PATCH; + + using absl::Status; using common::hb_face_unique_ptr; using common::CodepointSet; diff --git a/ift/encoder/glyph_closure_cache.cc b/ift/encoder/glyph_closure_cache.cc index 154b997c..58320f00 100644 --- a/ift/encoder/glyph_closure_cache.cc +++ b/ift/encoder/glyph_closure_cache.cc @@ -8,6 +8,9 @@ #include "ift/encoder/subset_definition.h" #include "ift/encoder/types.h" +using ift::proto::Glyphs; + + using absl::Status; using absl::StatusOr; using common::GlyphSet; diff --git a/ift/encoder/glyph_closure_cache_test.cc b/ift/encoder/glyph_closure_cache_test.cc index e6e35c9c..d5f29a0a 100644 --- a/ift/encoder/glyph_closure_cache_test.cc +++ b/ift/encoder/glyph_closure_cache_test.cc @@ -8,6 +8,9 @@ #include "ift/encoder/subset_definition.h" #include "ift/freq/probability_bound.h" +using ift::proto::PATCH; + + using absl::StatusOr; using common::CodepointSet; using common::FontData; diff --git a/ift/encoder/glyph_groupings.cc b/ift/encoder/glyph_groupings.cc index 8e2619e6..da37b31b 100644 --- a/ift/encoder/glyph_groupings.cc +++ b/ift/encoder/glyph_groupings.cc @@ -14,6 +14,9 @@ #include "ift/encoder/requested_segmentation_information.h" #include "ift/encoder/types.h" +using ift::proto::FIND_CONDITIONS; + + using absl::btree_map; using absl::btree_set; using absl::flat_hash_map; diff --git a/ift/encoder/glyph_groupings_test.cc b/ift/encoder/glyph_groupings_test.cc index 0a5d9153..8bfd3f74 100644 --- a/ift/encoder/glyph_groupings_test.cc +++ b/ift/encoder/glyph_groupings_test.cc @@ -18,6 +18,10 @@ #include "ift/freq/probability_bound.h" #include "ift/encoder/init_subset_defaults.h" +using ift::proto::PATCH; +using ift::proto::FIND_CONDITIONS; + + namespace ift::encoder { using absl::btree_map; diff --git a/ift/encoder/glyph_segmentation.cc b/ift/encoder/glyph_segmentation.cc index 2ea3bac6..1e16c5a1 100644 --- a/ift/encoder/glyph_segmentation.cc +++ b/ift/encoder/glyph_segmentation.cc @@ -14,6 +14,13 @@ #include "ift/proto/patch_encoding.h" #include "ift/proto/patch_map.h" +using ift::proto::SegmentationPlan; +using ift::proto::SegmentProto; +using ift::proto::Codepoints; +using ift::proto::Features; +using ift::proto::Glyphs; + + using absl::btree_map; using absl::btree_set; using absl::flat_hash_map; diff --git a/ift/encoder/glyph_segmentation.h b/ift/encoder/glyph_segmentation.h index b94c6f0e..4380d17a 100644 --- a/ift/encoder/glyph_segmentation.h +++ b/ift/encoder/glyph_segmentation.h @@ -99,9 +99,9 @@ class GlyphSegmentation { }; static void SubsetDefinitionToSegment(const SubsetDefinition& def, - SegmentProto& segment_proto); + ift::proto::SegmentProto& segment_proto); - SegmentationPlan ToSegmentationPlanProto() const; + ift::proto::SegmentationPlan ToSegmentationPlanProto() const; static absl::Status GroupsToSegmentation( const absl::btree_map& diff --git a/ift/encoder/glyph_segmentation_test.cc b/ift/encoder/glyph_segmentation_test.cc index b9e85ddf..80da2b5b 100644 --- a/ift/encoder/glyph_segmentation_test.cc +++ b/ift/encoder/glyph_segmentation_test.cc @@ -12,6 +12,10 @@ #include "ift/proto/patch_encoding.h" #include "ift/proto/patch_map.h" +using ift::proto::CLOSURE_ONLY; +using ift::proto::PATCH; + + using common::CodepointSet; using common::FontData; using common::hb_face_unique_ptr; diff --git a/ift/encoder/requested_segmentation_information.cc b/ift/encoder/requested_segmentation_information.cc index 1b512893..f7845d69 100644 --- a/ift/encoder/requested_segmentation_information.cc +++ b/ift/encoder/requested_segmentation_information.cc @@ -6,6 +6,9 @@ #include "ift/encoder/segment.h" #include "ift/encoder/subset_definition.h" +using ift::proto::UnmappedGlyphHandling; + + using absl::StatusOr; namespace ift::encoder { diff --git a/ift/encoder/requested_segmentation_information.h b/ift/encoder/requested_segmentation_information.h index 9bbd57e5..08fdee25 100644 --- a/ift/encoder/requested_segmentation_information.h +++ b/ift/encoder/requested_segmentation_information.h @@ -21,12 +21,12 @@ class RequestedSegmentationInformation { static absl::StatusOr> Create( std::vector segments, SubsetDefinition init_font_segment, GlyphClosureCache& closure_cache, - UnmappedGlyphHandling unmapped_glyph_handling); + ift::proto::UnmappedGlyphHandling unmapped_glyph_handling); private: RequestedSegmentationInformation( std::vector segments, SubsetDefinition init_font_segment, - UnmappedGlyphHandling unmapped_glyph_handling); + ift::proto::UnmappedGlyphHandling unmapped_glyph_handling); public: // Merge all of the segments in to_merge into base, assigned it @@ -70,7 +70,7 @@ class RequestedSegmentationInformation { return absl::OkStatus(); } - UnmappedGlyphHandling GetUnmappedGlyphHandling() const { + ift::proto::UnmappedGlyphHandling GetUnmappedGlyphHandling() const { return unmapped_glyph_handling_; } @@ -147,7 +147,7 @@ class RequestedSegmentationInformation { SubsetDefinition full_definition_; common::GlyphSet full_closure_; bool segments_disjoint_; - enum UnmappedGlyphHandling unmapped_glyph_handling_; + enum ift::proto::UnmappedGlyphHandling unmapped_glyph_handling_; }; } // namespace ift::encoder diff --git a/ift/encoder/segmentation_context.cc b/ift/encoder/segmentation_context.cc index 06003a87..70524e8e 100644 --- a/ift/encoder/segmentation_context.cc +++ b/ift/encoder/segmentation_context.cc @@ -11,6 +11,12 @@ #include "ift/encoder/init_subset_defaults.h" #include "ift/encoder/types.h" +using ift::proto::UnmappedGlyphHandling; +using ift::proto::ConditionAnalysisMode; +using ift::proto::CLOSURE_ONLY; +using ift::proto::CLOSURE_AND_VALIDATE_DEP_GRAPH; + + using absl::Status; using absl::StatusOr; using common::GlyphSet; diff --git a/ift/encoder/segmentation_context.h b/ift/encoder/segmentation_context.h index b7a59e15..5bcb2b03 100644 --- a/ift/encoder/segmentation_context.h +++ b/ift/encoder/segmentation_context.h @@ -48,8 +48,8 @@ class SegmentationContext { static absl::StatusOr Create( hb_face_t* face, const SubsetDefinition& initial_segment, const std::vector& segments, - UnmappedGlyphHandling unmapped_glyph_handling, - ConditionAnalysisMode condition_analysis_mode, + ift::proto::UnmappedGlyphHandling unmapped_glyph_handling, + ift::proto::ConditionAnalysisMode condition_analysis_mode, uint32_t brotli_quality, uint32_t init_font_brotli_quality) { // TODO(garretrieger): argument list is getting long, switch to a builder pattern @@ -66,8 +66,8 @@ class SegmentationContext { std::move(segmentation_info) ); - if ((condition_analysis_mode == CLOSURE_AND_DEP_GRAPH) || - (condition_analysis_mode == CLOSURE_AND_VALIDATE_DEP_GRAPH)) { + if ((condition_analysis_mode == ift::proto::CLOSURE_AND_DEP_GRAPH) || + (condition_analysis_mode == ift::proto::CLOSURE_AND_VALIDATE_DEP_GRAPH)) { context.dependency_closure_ = TRY(DependencyClosure::Create(context.segmentation_info_.get(), context.original_face.get())); } return context; @@ -82,16 +82,16 @@ class SegmentationContext { static absl::StatusOr InitializeSegmentationContext( hb_face_t* face, SubsetDefinition initial_segment, std::vector segments, - UnmappedGlyphHandling unmapped_glyph_handling, - ConditionAnalysisMode condition_analysis_mode, + ift::proto::UnmappedGlyphHandling unmapped_glyph_handling, + ift::proto::ConditionAnalysisMode condition_analysis_mode, uint32_t brotli_quality, uint32_t init_font_brotli_quality); private: SegmentationContext( hb_face_t* face, - UnmappedGlyphHandling unmapped_glyph_handling, - ConditionAnalysisMode condition_analysis_mode, + ift::proto::UnmappedGlyphHandling unmapped_glyph_handling, + ift::proto::ConditionAnalysisMode condition_analysis_mode, uint32_t brotli_quality, uint32_t init_font_brotli_quality, std::unique_ptr closure_cache, @@ -151,7 +151,7 @@ class SegmentationContext { return *segmentation_info_; } - ConditionAnalysisMode GetConditionAnalysisMode() const { + ift::proto::ConditionAnalysisMode GetConditionAnalysisMode() const { return condition_analysis_mode_; } @@ -279,7 +279,7 @@ class SegmentationContext { unsigned brotli_quality_; - ConditionAnalysisMode condition_analysis_mode_; + ift::proto::ConditionAnalysisMode condition_analysis_mode_; }; } // namespace ift::encoder diff --git a/ift/feature_registry/feature_registry.h b/ift/feature_registry/feature_registry.h index d92f6255..5f4b49b5 100644 --- a/ift/feature_registry/feature_registry.h +++ b/ift/feature_registry/feature_registry.h @@ -5,7 +5,7 @@ #include "absl/base/no_destructor.h" #include "absl/container/flat_hash_set.h" -namespace ift::featurge_registry { +namespace ift::feature_registry { static const absl::flat_hash_set& DefaultFeatureTags() { static const absl::NoDestructor> kDefaultFeatures((absl::flat_hash_set) { diff --git a/ift/proto/format_2_patch_map_test.cc b/ift/proto/format_2_patch_map_test.cc index 2713b79f..cb34be70 100644 --- a/ift/proto/format_2_patch_map_test.cc +++ b/ift/proto/format_2_patch_map_test.cc @@ -9,6 +9,8 @@ #include "ift/proto/patch_encoding.h" #include "ift/proto/patch_map.h" + + using testing::UnorderedElementsAre; namespace ift::proto { diff --git a/util/auto_segmenter_config.cc b/util/auto_segmenter_config.cc index b17b465a..be7228dd 100644 --- a/util/auto_segmenter_config.cc +++ b/util/auto_segmenter_config.cc @@ -14,6 +14,15 @@ #include "util/load_codepoints.h" #include "util/segmenter_config.pb.h" +using ift::proto::SegmenterConfig; +using ift::proto::CostConfiguration; +using ift::proto::HeuristicConfiguration; +using ift::proto::MergeGroup; +using ift::proto::CLOSURE_AND_DEP_GRAPH; +using ift::proto::MOVE_TO_INIT_FONT; +using ift::proto::FIND_CONDITIONS; + + using absl::btree_set; using absl::flat_hash_map; using absl::flat_hash_set; diff --git a/util/auto_segmenter_config.h b/util/auto_segmenter_config.h index ba00a9a5..bcbf929b 100644 --- a/util/auto_segmenter_config.h +++ b/util/auto_segmenter_config.h @@ -24,7 +24,7 @@ class AutoSegmenterConfig { // quality tradeoff. Lower values have shorter segmenting times, // high values have longer segmenting times but typically results // in better segmentation quality. - static absl::StatusOr GenerateConfig( + static absl::StatusOr GenerateConfig( hb_face_t* face, std::optional primary_script = std::nullopt, std::optional quality_level = std::nullopt); diff --git a/util/auto_segmenter_config_test.cc b/util/auto_segmenter_config_test.cc index 6a11725e..6a5f6731 100644 --- a/util/auto_segmenter_config_test.cc +++ b/util/auto_segmenter_config_test.cc @@ -13,6 +13,12 @@ #include "hb.h" #include "util/load_codepoints.h" +using ift::proto::SegmenterConfig; +using ift::proto::CLOSURE_AND_DEP_GRAPH; +using ift::proto::MOVE_TO_INIT_FONT; +using ift::proto::FIND_CONDITIONS; + + namespace util { namespace { diff --git a/util/closure_glyph_keyed_segmenter_util.cc b/util/closure_glyph_keyed_segmenter_util.cc index 3a26982e..b6eb28e6 100644 --- a/util/closure_glyph_keyed_segmenter_util.cc +++ b/util/closure_glyph_keyed_segmenter_util.cc @@ -30,6 +30,12 @@ #include "util/segmenter_config.pb.h" #include "util/segmenter_config_util.h" +using ift::proto::SegmentationPlan; +using ift::proto::SegmenterConfig; +using ift::proto::CLOSURE_ONLY; +using ift::proto::PATCH; + + /* * Given a code point based segmentation creates an appropriate glyph based * segmentation and associated activation conditions that maintain the "closure diff --git a/util/common.proto b/util/common.proto index 8d2b3552..d249f38d 100644 --- a/util/common.proto +++ b/util/common.proto @@ -1,5 +1,7 @@ edition = "2023"; +package ift.proto; + // A list of unicode code points. message Codepoints { repeated uint32 values = 1; diff --git a/util/convert_iftb.cc b/util/convert_iftb.cc index aa27c60d..cef8b9f9 100644 --- a/util/convert_iftb.cc +++ b/util/convert_iftb.cc @@ -11,6 +11,13 @@ #include "hb.h" #include "util/segmentation_plan.pb.h" +using ift::proto::SegmentationPlan; +using ift::proto::SegmentProto; +using ift::proto::ActivationConditionProto; +using ift::proto::SegmentsProto; +using ift::proto::Glyphs; + + using absl::btree_map; using absl::StatusOr; using absl::string_view; diff --git a/util/convert_iftb.h b/util/convert_iftb.h index da64e223..ada25d3b 100644 --- a/util/convert_iftb.h +++ b/util/convert_iftb.h @@ -8,7 +8,7 @@ namespace util { -absl::StatusOr convert_iftb(absl::string_view iftb_dump, +absl::StatusOr convert_iftb(absl::string_view iftb_dump, hb_face_t* face); } diff --git a/util/font2ift.cc b/util/font2ift.cc index 6c543cb9..043bd308 100644 --- a/util/font2ift.cc +++ b/util/font2ift.cc @@ -30,6 +30,11 @@ #include "util/segmenter_config.pb.h" #include "util/segmenter_config_util.h" +using ift::proto::SegmentationPlan; +using ift::proto::ActivationConditionProto; +using ift::proto::DesignSpace; + + /* * Utility that converts a standard font file into an IFT font file optionally following a * supplied segmentation plan. diff --git a/util/generate_segmenter_config.cc b/util/generate_segmenter_config.cc index 7af04759..22920259 100644 --- a/util/generate_segmenter_config.cc +++ b/util/generate_segmenter_config.cc @@ -14,6 +14,9 @@ #include "util/auto_segmenter_config.h" #include "util/load_codepoints.h" +using ift::proto::SegmenterConfig; + + ABSL_FLAG(std::string, input_font, "in.ttf", "Path to the font file to analyze."); diff --git a/util/generate_table_keyed_config.cc b/util/generate_table_keyed_config.cc index 95c84af3..6bca9034 100644 --- a/util/generate_table_keyed_config.cc +++ b/util/generate_table_keyed_config.cc @@ -12,6 +12,10 @@ #include "util/load_codepoints.h" #include "util/segmentation_plan.pb.h" +using ift::proto::SegmentationPlan; +using ift::proto::Codepoints; + + using absl::StatusOr; using absl::StrCat; using common::CodepointSet; diff --git a/util/load_codepoints.cc b/util/load_codepoints.cc index 61f529f4..768762d1 100644 --- a/util/load_codepoints.cc +++ b/util/load_codepoints.cc @@ -19,6 +19,8 @@ #include "codepoint_count.pb.h" #include "metadata.pb.h" + + using absl::Status; using absl::StatusOr; using absl::StrCat; diff --git a/util/segmentation_plan.proto b/util/segmentation_plan.proto index 8696351d..5e0d7698 100644 --- a/util/segmentation_plan.proto +++ b/util/segmentation_plan.proto @@ -1,5 +1,7 @@ edition = "2023"; +package ift.proto; + import "util/common.proto"; // This message is used to configure an IFT compiler to generate and IFT encoding of some input font. diff --git a/util/segmenter_config.proto b/util/segmenter_config.proto index 475ad98e..c6d3ae7c 100644 --- a/util/segmenter_config.proto +++ b/util/segmenter_config.proto @@ -1,5 +1,7 @@ edition = "2023"; +package ift.proto; + import "util/segmentation_plan.proto"; import "util/common.proto"; diff --git a/util/segmenter_config_util.cc b/util/segmenter_config_util.cc index b3320f21..f5524dc1 100644 --- a/util/segmenter_config_util.cc +++ b/util/segmenter_config_util.cc @@ -13,6 +13,15 @@ #include "ift/freq/unicode_frequencies.h" #include "util/load_codepoints.h" +using ift::proto::SegmentationPlan; +using ift::proto::SegmentProto; +using ift::proto::SegmenterConfig; +using ift::proto::CostConfiguration; +using ift::proto::HeuristicConfiguration; +using ift::proto::MergeGroup; +using ift::proto::SegmentsProto; + + using absl::btree_map; using absl::btree_set; using absl::flat_hash_map; diff --git a/util/segmenter_config_util.h b/util/segmenter_config_util.h index 6c9ce50d..a05c69b5 100644 --- a/util/segmenter_config_util.h +++ b/util/segmenter_config_util.h @@ -15,7 +15,7 @@ namespace util { struct SegmentationResult { ift::encoder::GlyphSegmentation segmentation; - SegmentationPlan plan; + ift::proto::SegmentationPlan plan; absl::btree_map merge_groups; }; @@ -25,14 +25,14 @@ class SegmenterConfigUtil { : config_file_path_(config_file_path) {} absl::StatusOr RunSegmenter( - hb_face_t* face, const SegmenterConfig& config); + hb_face_t* face, const ift::proto::SegmenterConfig& config); ift::encoder::SubsetDefinition SegmentProtoToSubsetDefinition( - const SegmentProto& segment); + const ift::proto::SegmentProto& segment); absl::StatusOr< absl::btree_map> - ConfigToMergeGroups(const SegmenterConfig& config, + ConfigToMergeGroups(const ift::proto::SegmenterConfig& config, const common::CodepointSet& font_codepoints, const absl::btree_set& font_features, std::vector& segments); @@ -53,7 +53,7 @@ class SegmenterConfigUtil { }; std::vector ConfigToSegments( - const SegmenterConfig& config, + const ift::proto::SegmenterConfig& config, const ift::encoder::SubsetDefinition& init_segment, const common::CodepointSet& font_codepoints, const absl::btree_set& font_features, @@ -63,18 +63,18 @@ class SegmenterConfigUtil { const std::string& frequency_data_file_path, bool built_in); absl::StatusOr ProtoToStrategy( - const CostConfiguration& base, const CostConfiguration& config, + const ift::proto::CostConfiguration& base, const ift::proto::CostConfiguration& config, common::CodepointSet& covered_codepoints); absl::StatusOr> ProtoToMergeGroup(const std::vector& segments, const absl::flat_hash_map& id_to_index, - const HeuristicConfiguration& base_heuristic, - const CostConfiguration& base_cost, - const MergeGroup& group); + const ift::proto::HeuristicConfiguration& base_heuristic, + const ift::proto::CostConfiguration& base_cost, + const ift::proto::MergeGroup& group); static common::SegmentSet MapToIndices( - const SegmentsProto& segments, + const ift::proto::SegmentsProto& segments, const absl::flat_hash_map& id_to_index); std::string config_file_path_; diff --git a/util/segmenter_config_util_test.cc b/util/segmenter_config_util_test.cc index f6e4d9c0..2c5477ed 100644 --- a/util/segmenter_config_util_test.cc +++ b/util/segmenter_config_util_test.cc @@ -10,6 +10,10 @@ #include "ift/encoder/subset_definition.h" #include "ift/freq/unicode_frequencies.h" +using ift::proto::SegmenterConfig; +using ift::proto::Features; + + using absl::btree_map; using absl::btree_set; using common::CodepointSet; diff --git a/util/trace_segmentation_plan.cc b/util/trace_segmentation_plan.cc index f7e90e9b..1aa79638 100644 --- a/util/trace_segmentation_plan.cc +++ b/util/trace_segmentation_plan.cc @@ -13,6 +13,11 @@ #include "util/segmentation_plan.pb.h" #include "common/try.h" +using ift::proto::SegmentationPlan; +using ift::proto::SegmentProto; +using ift::proto::ActivationConditionProto; + + using absl::Status; using absl::StatusOr; using common::CodepointSet; From b9fa5aa93af8ebe0242d3606df7016d252aba3a9 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Tue, 10 Mar 2026 00:06:24 +0000 Subject: [PATCH 3/5] Remove convert iftb functionality. This is no longer needed, it existed at the start as a way to get supply font segmentations before we had our own segmenter. --- README.md | 11 +- util/BUILD | 50 --------- util/convert_iftb.cc | 154 -------------------------- util/convert_iftb.h | 16 --- util/convert_iftb_test.cc | 127 --------------------- util/iftb2config.cc | 61 ---------- util/testdata/convert-iftb-sample.txt | 9 -- 7 files changed, 1 insertion(+), 427 deletions(-) delete mode 100644 util/convert_iftb.cc delete mode 100644 util/convert_iftb.h delete mode 100644 util/convert_iftb_test.cc delete mode 100644 util/iftb2config.cc delete mode 100644 util/testdata/convert-iftb-sample.txt diff --git a/README.md b/README.md index bd000300..1d2359cf 100644 --- a/README.md +++ b/README.md @@ -159,7 +159,7 @@ Segmentation plans are in a [textproto format](https://protobuf.dev/reference/pr This repo currently provides a few experimental utilities that can generate segmentation plans for you. It is also possible to write plans by hand, or develop new utilities to generate plans. -In this repo 3 options are currently provided: +In this repo two options are currently provided: 1. [Recommended] `util/closure_glyph_keyed_segmenter_util`: this utility uses a subsetting closure based approach to generate a glyph keyed segmentation plan (extension segments that augment glyph data). It can optionally @@ -188,15 +188,6 @@ In this repo 3 options are currently provided: latin.txt cyrillic.txt greek.txt > table_keyed.txtpb ``` -3. `util/iftb2config`: this utility converts a segmentation obtained from the - [binned incremental font transfer prototype](https://github.com/adobe/binned-ift-reference) - into and equivalent segmentation plan. Example execution: - - ```sh - iftb -VV info my_iftb_font.ttf 2>&1 | \ - bazel run util:iftb2config > segmentation_plan.txtpb - ``` - If separate glyph keyed and table keyed configs were generated using #1 and #2 they can then be combined into one complete plan by concatenating them: diff --git a/util/BUILD b/util/BUILD index cf7afb63..6523b138 100644 --- a/util/BUILD +++ b/util/BUILD @@ -158,23 +158,6 @@ cc_library( ], ) -cc_library( - name = "convert_iftb", - srcs = [ - "convert_iftb.cc", - "convert_iftb.h", - ], - deps = [ - ":segmentation_plan_cc_proto", - "//common", - "@abseil-cpp//absl/container:btree", - "@abseil-cpp//absl/container:flat_hash_map", - "@abseil-cpp//absl/container:flat_hash_set", - "@abseil-cpp//absl/flags:flag", - "@abseil-cpp//absl/flags:parse", - ], -) - cc_library( name = "auto_segmenter_config", srcs = [ @@ -256,22 +239,6 @@ cc_test( ], ) -cc_test( - name = "convert_iftb_test", - size = "small", - srcs = [ - "convert_iftb_test.cc", - ], - data = [ - "testdata/Roboto-Regular.Awesome.ttf", - "testdata/convert-iftb-sample.txt", - ], - deps = [ - ":convert_iftb", - "@googletest//:gtest_main", - ], -) - cc_test( name = "load_codepoints_test", size = "small", @@ -333,23 +300,6 @@ cc_binary( ], ) -cc_binary( - name = "iftb2config", - srcs = [ - "iftb2config.cc", - ], - deps = [ - ":convert_iftb", - ":load_codepoints", - ":segmentation_plan_cc_proto", - "@abseil-cpp//absl/container:flat_hash_set", - "@abseil-cpp//absl/flags:flag", - "@abseil-cpp//absl/flags:parse", - "@abseil-cpp//absl/status:statusor", - "@abseil-cpp//absl/strings", - ], -) - cc_binary( name = "generate_riegeli_test_data", srcs = [ diff --git a/util/convert_iftb.cc b/util/convert_iftb.cc deleted file mode 100644 index cef8b9f9..00000000 --- a/util/convert_iftb.cc +++ /dev/null @@ -1,154 +0,0 @@ -#include "util/convert_iftb.h" - -#include -#include -#include - -#include "absl/container/btree_map.h" -#include "absl/status/statusor.h" -#include "common/font_helper.h" -#include "common/int_set.h" -#include "hb.h" -#include "util/segmentation_plan.pb.h" - -using ift::proto::SegmentationPlan; -using ift::proto::SegmentProto; -using ift::proto::ActivationConditionProto; -using ift::proto::SegmentsProto; -using ift::proto::Glyphs; - - -using absl::btree_map; -using absl::StatusOr; -using absl::string_view; -using common::FontHelper; -using common::IntSet; - -namespace util { - -string_view next_token(string_view line, string_view delim, std::string& out) { - if (line.empty()) { - out = ""; - return line; - } - - size_t index = line.find(delim); - if (index == std::string::npos) { - out = line; - return string_view(); - } - - out = line.substr(0, index); - return line.substr(index + delim.size()); -} - -IntSet load_chunk_set(string_view line) { - IntSet result; - - std::string next; - while (!line.empty()) { - line = next_token(line, ", ", next); - result.insert(std::stoi(next)); - } - - return result; -} - -btree_map load_gid_map(string_view line) { - btree_map result; - - std::string next; - while (!line.empty()) { - line = next_token(line, ", ", next); - - std::string gid; - std::string chunk; - next = next_token(next, ":", gid); - next = next_token(next, ":", chunk); - - result[std::stoi(gid)] = std::stoi(chunk); - } - - return result; -} - -StatusOr create_config( - const btree_map& gid_map, - const IntSet& loaded_chunks, hb_face_t* face) { - auto gid_to_unicode = FontHelper::GidToUnicodeMap(face); - SegmentationPlan config; - // Populate segments in the config. chunks are directly analagous to segments. - auto segments = config.mutable_glyph_patches(); - for (const auto [gid, chunk] : gid_map) { - auto cps = gid_to_unicode.find(gid); - if (cps != gid_to_unicode.end()) { - SegmentProto segment; - auto [it, added] = - config.mutable_segments()->insert(std::pair(chunk, segment)); - for (hb_codepoint_t cp : cps->second) { - it->second.mutable_codepoints()->add_values(cp); - } - } - - Glyphs glyphs; - auto [it, added] = segments->insert(std::pair(chunk, glyphs)); - it->second.add_values(gid); - } - - // Set up the initial subset, which is specified by loaded_chunks - for (auto chunk : loaded_chunks) { - config.mutable_initial_segments()->add_values(chunk); - } - - // Add all non-initial segments to a single non-glyph segment - // TODO(garretrieger): flag to configure having more than one table keyed - // segment. - IntSet non_initial_segments; - for (const auto [gid, chunk] : gid_map) { - if (loaded_chunks.contains(chunk)) { - continue; - } - non_initial_segments.insert(chunk); - } - - SegmentsProto* segments_proto = config.add_non_glyph_segments(); - for (auto chunk : non_initial_segments) { - segments_proto->add_values(chunk); - - ActivationConditionProto* condition = config.add_glyph_patch_conditions(); - condition->set_activated_patch(chunk); - condition->add_required_segments()->add_values(chunk); - } - - return config; -} - -StatusOr convert_iftb(string_view iftb_dump, - hb_face_t* face) { - btree_map gid_map; - IntSet loaded_chunks; - - while (!iftb_dump.empty()) { - std::string line; - iftb_dump = next_token(iftb_dump, "\n", line); - - std::string field; - line = next_token(line, ": ", field); - - fprintf(stderr, ">> %s\n", field.c_str()); - - if (field == "gidMap") { - gid_map = load_gid_map(line); - continue; - } - - if (field == "chunkSet indexes") { - loaded_chunks = load_chunk_set(line); - continue; - } - } - - return create_config(gid_map, loaded_chunks, face); -} - -} // namespace util diff --git a/util/convert_iftb.h b/util/convert_iftb.h deleted file mode 100644 index ada25d3b..00000000 --- a/util/convert_iftb.h +++ /dev/null @@ -1,16 +0,0 @@ -#ifndef UTIL_CONVERT_IFTB_H_ -#define UTIL_CONVERT_IFTB_H_ - -#include "absl/status/statusor.h" -#include "absl/strings/string_view.h" -#include "hb.h" -#include "util/segmentation_plan.pb.h" - -namespace util { - -absl::StatusOr convert_iftb(absl::string_view iftb_dump, - hb_face_t* face); - -} - -#endif // UTIL_CONVERT_IFTB_H_ diff --git a/util/convert_iftb_test.cc b/util/convert_iftb_test.cc deleted file mode 100644 index c78f4c5e..00000000 --- a/util/convert_iftb_test.cc +++ /dev/null @@ -1,127 +0,0 @@ -#include "util/convert_iftb.h" - -#include - -#include - -#include "absl/container/flat_hash_set.h" -#include "common/font_data.h" -#include "common/sparse_bit_set.h" -#include "gmock/gmock.h" -#include "gtest/gtest.h" - -using testing::Eq; - -using absl::flat_hash_set; -using common::FontData; -using common::hb_blob_unique_ptr; -using common::hb_face_unique_ptr; -using common::make_hb_blob; -using common::SparseBitSet; -using google::protobuf::TextFormat; - -namespace util { - -class ConvertIftbTest : public ::testing::Test { - protected: - ConvertIftbTest() : face(common::make_hb_face(nullptr)) { - hb_blob_unique_ptr blob = - common::make_hb_blob(hb_blob_create_from_file_or_fail( - "util/testdata/convert-iftb-sample.txt")); - assert(blob.get()); - - FontData blob_data(blob.get()); - sample_input = blob_data.string(); - - blob = common::make_hb_blob(hb_blob_create_from_file_or_fail( - "util/testdata/Roboto-Regular.Awesome.ttf")); - assert(blob.get()); - blob_data.set(blob.get()); - face = blob_data.face(); - } - - std::string sample_input; - hb_face_unique_ptr face; -}; - -TEST_F(ConvertIftbTest, BasicConversion) { - auto config = convert_iftb(sample_input, face.get()); - ASSERT_TRUE(config.ok()) << config.status(); - - std::string expected_config = - "segments {\n" - " key: 0\n" - " value {\n" - " codepoints {\n" - " values: 115\n" // s - " }\n" - " }\n" - "}\n" - "segments {\n" - " key: 1\n" - " value {\n" - " codepoints {\n" - " values: 65\n" // A - " values: 101\n" // e - " values: 109\n" // m - " }\n" - " }\n" - "}\n" - "segments {\n" - " key: 2\n" - " value {\n" - " codepoints {\n" - " values: 111\n" // o - " values: 119\n" // w - " }\n" - " }\n" - "}\n" - "glyph_patches {\n" - " key: 0\n" - " value {\n" - " values: 0\n" - " values: 5\n" - " }\n" - "}\n" - "glyph_patches {\n" - " key: 1\n" - " value {\n" - " values: 1\n" - " values: 2\n" - " values: 3\n" - " }\n" - "}\n" - "glyph_patches {\n" - " key: 2\n" - " value {\n" - " values: 4\n" - " values: 6\n" - " }\n" - "}\n" - "glyph_patch_conditions {\n" - " required_segments {\n" - " values: 1\n" - " }\n" - " activated_patch: 1\n" - "}\n" - "glyph_patch_conditions {\n" - " required_segments {\n" - " values: 2\n" - " }\n" - " activated_patch: 2\n" - "}\n" - "initial_segments {\n" - " values: 0\n" - "}\n" - "non_glyph_segments {\n" - " values: 1\n" - " values: 2\n" - "}\n"; - - std::string config_string; - TextFormat::PrintToString(*config, &config_string); - - ASSERT_EQ(config_string, expected_config); -} - -} // namespace util diff --git a/util/iftb2config.cc b/util/iftb2config.cc deleted file mode 100644 index eed351fa..00000000 --- a/util/iftb2config.cc +++ /dev/null @@ -1,61 +0,0 @@ -/* - * This utility converts an iftb info dump into the corresponding - * encoding_config.proto config file. - * - * Takes the info dump on stdin and outputs the config on stdout. - */ - -#include - -#include -#include - -#include "absl/flags/flag.h" -#include "absl/flags/parse.h" -#include "absl/status/statusor.h" -#include "absl/strings/str_cat.h" -#include "common/font_data.h" -#include "common/try.h" -#include "util/convert_iftb.h" -#include "util/load_codepoints.h" - -using absl::StatusOr; -using absl::StrCat; -using common::FontData; -using common::hb_blob_unique_ptr; -using common::hb_face_unique_ptr; -using common::make_hb_blob; - -ABSL_FLAG(std::string, font, "font.ttf", - "The font file that corresponds to the IFTB dump."); - -StatusOr load_font(const char* filename) { - return TRY(util::LoadFile(filename)).face(); -} - -int main(int argc, char** argv) { - auto args = absl::ParseCommandLine(argc, argv); - - auto face = load_font(absl::GetFlag(FLAGS_font).c_str()); - if (!face.ok()) { - std::cerr << "Failed to load font " << absl::GetFlag(FLAGS_font) << " " - << face.status() << std::endl; - } - - std::stringstream ss; - ss << std::cin.rdbuf(); - std::string input = ss.str(); - - auto config = util::convert_iftb(input, face->get()); - if (!config.ok()) { - std::cerr << "Failure parsing iftb info dump: " << config.status() - << std::endl; - return -1; - } - - std::string out; - google::protobuf::TextFormat::PrintToString(*config, &out); - - std::cout << out << std::endl; - return 0; -} \ No newline at end of file diff --git a/util/testdata/convert-iftb-sample.txt b/util/testdata/convert-iftb-sample.txt deleted file mode 100644 index 806e87b0..00000000 --- a/util/testdata/convert-iftb-sample.txt +++ /dev/null @@ -1,9 +0,0 @@ -majorVersion: 0 -minorVersion: 1 -ID: bbf038d5 582c7194 b721ed1d e13c8483 -chunkCount: 3 -glyphCount: 7 -chunkSet indexes: 0 -gidMap: 0:0, 1:1, 2:1, 3:1, 4:2, 5:0, 6:2 -filesURI: ./Roboto-Regular_iftb/$3/chunk$3$2$1.br - From 30d673ed27d956b6b3510d385e94ff2fa670c62d Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Tue, 10 Mar 2026 00:25:17 +0000 Subject: [PATCH 4/5] Remove init_subset_defaults.h from public api. --- ift/encoder/BUILD | 6 +----- ift/feature_registry/BUILD | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/ift/encoder/BUILD b/ift/encoder/BUILD index 7bf76c41..435f3b3e 100644 --- a/ift/encoder/BUILD +++ b/ift/encoder/BUILD @@ -67,8 +67,6 @@ cc_library( "merger.cc", "merger.h", "patch_size_cache.h", - "requested_segmentation_information.cc", - "requested_segmentation_information.h", "segmentation_context.cc", "segmentation_context.h", ] + select({ @@ -79,8 +77,6 @@ cc_library( "dependency_closure_disabled.cc", ], }), - visibility = [ - ], deps = [ ":merge_strategy", ":segmentation_info", @@ -109,6 +105,7 @@ cc_library( "requested_segmentation_information.h", "glyph_closure_cache.cc", "glyph_closure_cache.h", + "init_subset_defaults.h", ], visibility = [ "//ift/dep_graph:__pkg__", @@ -144,7 +141,6 @@ cc_library( "subset_definition.cc", ], hdrs = [ - "init_subset_defaults.h", "segment.h", "subset_definition.h", "types.h", diff --git a/ift/feature_registry/BUILD b/ift/feature_registry/BUILD index 59af7880..b70ef365 100644 --- a/ift/feature_registry/BUILD +++ b/ift/feature_registry/BUILD @@ -23,7 +23,7 @@ cc_library( "feature_registry.h", ], visibility = [ - "//visibility:public", + "//ift/encoder:__pkg__", ], deps = [ "@abseil-cpp//absl/base:no_destructor", From 6103869407eedf4dc82b2b3933164be7ddcc7fd7 Mon Sep 17 00:00:00 2001 From: Garret Rieger Date: Tue, 10 Mar 2026 00:28:31 +0000 Subject: [PATCH 5/5] s/check_public_headers.sh/check-public-header.sh/ to be consistent with check-format.sh --- check_public_headers.sh => check-public-headers.sh | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename check_public_headers.sh => check-public-headers.sh (100%) diff --git a/check_public_headers.sh b/check-public-headers.sh similarity index 100% rename from check_public_headers.sh rename to check-public-headers.sh