diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1c9bfe6e..108f1fc6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,6 +22,10 @@ jobs: run: | "${GITHUB_WORKSPACE}/bin/bazel" test -c opt ... --test_output=all + - name: Test (Default Build, No Dep Graph) + run: | + "${GITHUB_WORKSPACE}/bin/bazel" test --//:harfbuzz_dep_graph=False ... --test_output=all + check_tests_osx: name: Check Tests (Mac OS) runs-on: macos-latest diff --git a/BUILD b/BUILD index e69de29b..67588f95 100644 --- a/BUILD +++ b/BUILD @@ -0,0 +1,15 @@ +load("@bazel_skylib//rules:common_settings.bzl", "bool_flag") + +bool_flag( + name = "harfbuzz_dep_graph", + build_setting_default = True, + visibility = ["//visibility:public"], +) + +config_setting( + name = "use_harfbuzz_dep_graph", + flag_values = { + ":harfbuzz_dep_graph": "True", + }, + visibility = ["//visibility:public"], +) diff --git a/MODULE.bazel b/MODULE.bazel index eb729eef..cc36d725 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -16,6 +16,7 @@ bazel_dep(name = "platforms", version = "1.0.0") bazel_dep(name = "rules_rust", version = "0.68.1") bazel_dep(name = "glib", version = "2.82.2.bcr.7") bazel_dep(name = "brotli", version = "1.2.0") +bazel_dep(name = "bazel_skylib", version = "1.9.0") # Frequency Data bazel_dep(name = "ift_encoder_data", version = "git") diff --git a/README.md b/README.md index fdf9b84e..d861e63d 100644 --- a/README.md +++ b/README.md @@ -5,11 +5,11 @@ This code repository contains an implementation of an ## Status -This implementation is still in the early stages and as at the moment is work in progress. +This implementation is still in the early stages and as at the moment is work in progress. We aim to keep it updated and consistent with the current IFT specification working draft found [here](https://w3c.github.io/IFT/Overview.html). -The current implementation is capable of producing a spec-compliant encoding, but does not +The current implementation is capable of producing a spec-compliant encoding, but does not yet fully support all aspects of the specification. Notably: * Format 1 patch maps are not generated. @@ -33,6 +33,19 @@ and run all of the tests: bazel test ... ``` +### Building without Dependency Graph Support + +By default this depends on the experimental harfbuzz dependency graph API which isn't yet in mainline harfbuzz. +The dependency graph functionality can be disabled at compile time using the `harfbuzz_dep_graph` build flag. +For example: + +```sh +bazel build --//:harfbuzz_dep_graph=False ... +bazel test --//:harfbuzz_dep_graph=False ... +``` + +Disabling the harfbuzz dependency graph api will cause segmenter runs using the `CLOSURE_AND_DEP_GRAPH` and `CLOSURE_AND_VALIDATE_DEP_GRAPH` condition analysis modes to fail. + ## Code Style The code follows the [Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html). Formatting is enforced by an automated check for new commits to this repo. You can auto-correct formatting for all files using the check-format.sh diff --git a/common/BUILD b/common/BUILD index 7f0e7862..8be01e9b 100644 --- a/common/BUILD +++ b/common/BUILD @@ -40,9 +40,6 @@ cc_library( "try.h", "woff2.h", ], - copts = [ - "-DHB_EXPERIMENTAL_API", - ], visibility = [ "//visibility:public", ], diff --git a/ift/BUILD b/ift/BUILD index 22f09e22..983bdd58 100644 --- a/ift/BUILD +++ b/ift/BUILD @@ -57,9 +57,6 @@ cc_test( "table_keyed_diff_test.cc", "url_template_test.cc", ], - copts = [ - "-DHB_EXPERIMENTAL_API", - ], data = [ "//common:testdata", "//ift:testdata", @@ -80,9 +77,6 @@ cc_test( srcs = [ "integration_test.cc", ], - copts = [ - "-DHB_EXPERIMENTAL_API", - ], data = [ "//common:testdata", "//ift:testdata", diff --git a/ift/dep_graph/BUILD b/ift/dep_graph/BUILD index dcc31780..95bcf599 100644 --- a/ift/dep_graph/BUILD +++ b/ift/dep_graph/BUILD @@ -1,18 +1,18 @@ load("@rules_cc//cc:cc_library.bzl", "cc_library") load("@rules_cc//cc:cc_test.bzl", "cc_test") + cc_library( name = "dep_graph", - srcs = [ - "dependency_graph.h", - "dependency_graph.cc", - "node.h", - "traversal.h", - ], - copts = [ - "-DHB_EXPERIMENTAL_API", - "-DHB_DEPEND_API", - ], + srcs = select({ + "//:use_harfbuzz_dep_graph": [ + "dependency_graph.h", + "dependency_graph.cc", + "node.h", + "traversal.h", + ], + "//conditions:default": [], + }), visibility = [ "//ift/encoder:__pkg__", ], @@ -31,13 +31,12 @@ cc_library( cc_test( name = "dependency_graph_test", size = "small", - srcs = [ - "dependency_graph_test.cc", - ], - copts = [ - "-DHB_EXPERIMENTAL_API", - "-DHB_DEPEND_API", - ], + srcs = select({ + "//:use_harfbuzz_dep_graph": [ + "dependency_graph_test.cc", + ], + "//conditions:default": [], + }), data = [ "//common:testdata", ], diff --git a/ift/dep_graph/dependency_graph.h b/ift/dep_graph/dependency_graph.h index 003626ed..1eaf3a38 100644 --- a/ift/dep_graph/dependency_graph.h +++ b/ift/dep_graph/dependency_graph.h @@ -27,9 +27,9 @@ namespace ift::dep_graph { class TraversalContext; /* - * Wrapper around harfbuzz's glyph depedency graph API. + * Wrapper around harfbuzz's glyph dependency graph API. * - * Allows exploring glyph depedencies within a font. + * Allows exploring glyph dependencies within a font. */ class DependencyGraph { public: diff --git a/ift/dep_graph/dependency_graph_test.cc b/ift/dep_graph/dependency_graph_test.cc index d7c09d50..5267d3f1 100644 --- a/ift/dep_graph/dependency_graph_test.cc +++ b/ift/dep_graph/dependency_graph_test.cc @@ -318,8 +318,8 @@ TEST_F(DependencyGraphTest, ImpliedFeatureEdge_Liga) { // TODO(garretrieger): // - basic math, CFF, and COLR tests. -// TODO(garretrieger) we currently only have a few specialized tests, relyng primarily on DepedencyClosureTest -// for coverage of DepedencyGraph functionality. We should add some basic tests here that test DepedencyGraph +// TODO(garretrieger) we currently only have a few specialized tests, relyng primarily on DependencyClosureTest +// for coverage of DependencyGraph functionality. We should add some basic tests here that test DepedencyGraph // core features in isolation. } // namespace ift::dep_graph \ No newline at end of file diff --git a/ift/dep_graph/node.h b/ift/dep_graph/node.h index 39140c2b..943f9d2c 100644 --- a/ift/dep_graph/node.h +++ b/ift/dep_graph/node.h @@ -10,7 +10,7 @@ namespace ift::dep_graph { -// A single node in a fonts glyph depedency graph. +// A single node in a fonts glyph dependency graph. class Node { public: enum NodeType { diff --git a/ift/dep_graph/traversal.h b/ift/dep_graph/traversal.h index 18e50157..07a0472b 100644 --- a/ift/dep_graph/traversal.h +++ b/ift/dep_graph/traversal.h @@ -9,7 +9,7 @@ namespace ift::dep_graph { -// A single node in a fonts glyph depedency graph. +// A single node in a fonts glyph dependency graph. class Traversal { public: void SetPendingEdges() { diff --git a/ift/encoder/BUILD b/ift/encoder/BUILD index b02930ff..6fb3e398 100644 --- a/ift/encoder/BUILD +++ b/ift/encoder/BUILD @@ -9,10 +9,6 @@ cc_library( "compiler.cc", "compiler.h", ], - copts = [ - "-DHB_EXPERIMENTAL_API", - "-DHB_DEPEND_API", - ], visibility = [ "//ift:__subpackages__", "//ift/client:__pkg__", @@ -60,7 +56,6 @@ cc_library( "candidate_merge.h", "complex_condition_finder.cc", "complex_condition_finder.h", - "dependency_closure.cc", "dependency_closure.h", "estimated_patch_size_cache.cc", "estimated_patch_size_cache.h", @@ -78,11 +73,14 @@ cc_library( "requested_segmentation_information.h", "segmentation_context.cc", "segmentation_context.h", - ], - copts = [ - "-DHB_EXPERIMENTAL_API", - "-DHB_DEPEND_API", - ], + ] + select({ + "//:use_harfbuzz_dep_graph": [ + "dependency_closure.cc", + ], + "//conditions:default": [ + "dependency_closure_disabled.cc", + ], + }), visibility = [ ], deps = [ @@ -145,9 +143,6 @@ cc_library( "subset_definition.h", "types.h", ], - copts = [ - "-DHB_EXPERIMENTAL_API", - ], visibility = [ "//visibility:public", ], @@ -203,10 +198,6 @@ cc_test( srcs = [ "glyph_segmentation_test.cc", ], - copts = [ - "-DHB_EXPERIMENTAL_API", - "-DHB_DEPEND_API", - ], data = [ "//common:testdata", "//ift:testdata", @@ -225,10 +216,6 @@ cc_test( srcs = [ "complex_condition_finder_test.cc", ], - copts = [ - "-DHB_EXPERIMENTAL_API", - "-DHB_DEPEND_API", - ], data = [ "//common:testdata", "//ift:testdata", @@ -247,10 +234,6 @@ cc_test( srcs = [ "closure_glyph_segmenter_test.cc", ], - copts = [ - "-DHB_EXPERIMENTAL_API", - "-DHB_DEPEND_API", - ], data = [ "//common:testdata", "//ift:testdata", @@ -268,10 +251,6 @@ cc_test( srcs = [ "glyph_closure_cache_test.cc", ], - copts = [ - "-DHB_EXPERIMENTAL_API", - "-DHB_DEPEND_API", - ], data = [ "//common:testdata", ], @@ -289,10 +268,6 @@ cc_test( data = [ "//common:testdata", ], - copts = [ - "-DHB_EXPERIMENTAL_API", - "-DHB_DEPEND_API", - ], deps = [ ":segmentation_context", "//common", @@ -307,10 +282,6 @@ cc_test( "candidate_merge_test.cc", "mock_patch_size_cache.h", ], - copts = [ - "-DHB_EXPERIMENTAL_API", - "-DHB_DEPEND_API", - ], data = [ "//common:testdata", ], @@ -339,13 +310,12 @@ cc_test( cc_test( name = "dependency_closure_test", size = "small", - srcs = [ - "dependency_closure_test.cc", - ], - copts = [ - "-DHB_EXPERIMENTAL_API", - "-DHB_DEPEND_API", - ], + srcs = select({ + "//:use_harfbuzz_dep_graph": [ + "dependency_closure_test.cc", + ], + "//conditions:default": [], + }), data = [ "//common:testdata", ], diff --git a/ift/encoder/closure_glyph_segmenter_test.cc b/ift/encoder/closure_glyph_segmenter_test.cc index a4b27a3c..62d2f74f 100644 --- a/ift/encoder/closure_glyph_segmenter_test.cc +++ b/ift/encoder/closure_glyph_segmenter_test.cc @@ -36,9 +36,16 @@ class ClosureGlyphSegmenterTest : public ::testing::Test { segmenter_find_conditions(8, 8, FIND_CONDITIONS, CLOSURE_ONLY), segmenter_move_to_init_font(8, 8, MOVE_TO_INIT_FONT, CLOSURE_ONLY), +#ifdef HB_DEPEND_API segmenter_dep_graph(8, 8, PATCH, CLOSURE_AND_VALIDATE_DEP_GRAPH), segmenter_find_conditions_dep_graph(8, 8, FIND_CONDITIONS, CLOSURE_AND_VALIDATE_DEP_GRAPH), - segmenter_move_to_init_font_dep_graph(8, 8, MOVE_TO_INIT_FONT, CLOSURE_AND_VALIDATE_DEP_GRAPH) { + segmenter_move_to_init_font_dep_graph(8, 8, MOVE_TO_INIT_FONT, CLOSURE_AND_VALIDATE_DEP_GRAPH) +#else + segmenter_dep_graph(8, 8, PATCH, CLOSURE_ONLY), + segmenter_find_conditions_dep_graph(8, 8, FIND_CONDITIONS, CLOSURE_ONLY), + segmenter_move_to_init_font_dep_graph(8, 8, MOVE_TO_INIT_FONT, CLOSURE_ONLY) +#endif + { roboto = from_file("common/testdata/Roboto-Regular.ttf"); noto_nastaliq_urdu = from_file("common/testdata/NotoNastaliqUrdu.subset.ttf"); diff --git a/ift/encoder/dependency_closure.cc b/ift/encoder/dependency_closure.cc index aa46c0a9..0cbb6a48 100644 --- a/ift/encoder/dependency_closure.cc +++ b/ift/encoder/dependency_closure.cc @@ -28,7 +28,7 @@ using ift::dep_graph::Traversal; namespace ift::encoder { DependencyClosure::AnalysisAccuracy DependencyClosure::TraversalAccuracy(const Traversal& traversal) const { - // TODO(garretrieger): there's several types of depedencies that we do not handle yet and as a result + // TODO(garretrieger): there's several types of dependencies that we do not handle yet and as a result // consider inaccurate. Adding support for these will allow the dep graph to be used more widely: // - UVS edges: more complex case is generating conjunctive conditions from them. // - Ligatures: at least in simple non-nested cases we should be able to generate the corresponding conditions. diff --git a/ift/encoder/dependency_closure.h b/ift/encoder/dependency_closure.h index 6a05b17d..dd0718ab 100644 --- a/ift/encoder/dependency_closure.h +++ b/ift/encoder/dependency_closure.h @@ -11,15 +11,18 @@ #include "common/int_set.h" #include "common/try.h" #include "hb.h" -#include "ift/dep_graph/dependency_graph.h" #include "ift/encoder/types.h" +#ifdef HB_DEPEND_API +#include "ift/dep_graph/dependency_graph.h" +#endif + namespace ift::encoder { class RequestedSegmentationInformation; /* - * Performs closure analysis (like GlyphClosureCache) using a depedency graph + * Performs closure analysis (like GlyphClosureCache) using a dependency graph * instead of closure. The dependency graph is not always accurate (overestimating * the true closure in some cases) so this returns a signal on the accuracy of * the analysis. @@ -29,10 +32,14 @@ class DependencyClosure { static absl::StatusOr> Create( const RequestedSegmentationInformation* segmentation_info, hb_face_t* face) { +#ifndef HB_DEPEND_API + return std::unique_ptr(new DependencyClosure()); +#else dep_graph::DependencyGraph graph = TRY(dep_graph::DependencyGraph::Create(segmentation_info, face)); auto result = std::unique_ptr(new DependencyClosure(std::move(graph), segmentation_info, face)); TRYV(result->SegmentsChanged(true, common::SegmentSet::all())); return result; +#endif } enum AnalysisAccuracy { @@ -79,7 +86,7 @@ class DependencyClosure { // Finds the complete set of segments that may have some interaction on the presence of glyphs // in the glyph closure. // - // Utilizes the depedency graph to make the determination, so it's possible that the result + // Utilizes the dependency graph to make the determination, so it's possible that the result // may be overestimated. absl::StatusOr SegmentsThatInteractWith(const common::GlyphSet& glyphs); @@ -90,6 +97,9 @@ class DependencyClosure { private: + #ifndef HB_DEPEND_API + DependencyClosure() {} + #else DependencyClosure( dep_graph::DependencyGraph&& graph, const RequestedSegmentationInformation* segmentation_info, @@ -166,6 +176,7 @@ class DependencyClosure { absl::flat_hash_map segment_context_glyphs_; absl::flat_hash_map segments_that_have_context_feature_; absl::flat_hash_map> segment_context_features_; + #endif uint64_t accurate_results_ = 0; uint64_t inaccurate_results_ = 0; diff --git a/ift/encoder/dependency_closure_disabled.cc b/ift/encoder/dependency_closure_disabled.cc new file mode 100644 index 00000000..7ae40564 --- /dev/null +++ b/ift/encoder/dependency_closure_disabled.cc @@ -0,0 +1,31 @@ +#include "ift/encoder/dependency_closure.h" + +#include "common/int_set.h" + +using absl::Status; +using absl::StatusOr; +using common::GlyphSet; +using common::IntSet; +using common::SegmentSet; + +namespace ift::encoder { + +Status DependencyClosure::SegmentsChanged(bool init_font_change, const SegmentSet& segments) { + return absl::UnimplementedError("Depdency graph functionality was disabled during compilation and is unvailable"); +} + +StatusOr DependencyClosure::AnalyzeSegment( + const common::SegmentSet& segments, GlyphSet& and_gids, GlyphSet& or_gids, + GlyphSet& exclusive_gids) { + return absl::UnimplementedError("Depdency graph functionality was disabled during compilation and is unvailable"); +} + +StatusOr DependencyClosure::SegmentsThatInteractWith(const GlyphSet& glyphs) { + return absl::UnimplementedError("Depdency graph functionality was disabled during compilation and is unvailable"); +} + +StatusOr DependencyClosure::SegmentInteractionGroup(const SegmentSet& segments) { + return absl::UnimplementedError("Depdency graph functionality was disabled during compilation and is unvailable"); +} + +} // namespace ift::encoder \ No newline at end of file diff --git a/ift/encoder/glyph_groupings.h b/ift/encoder/glyph_groupings.h index 0c09f04d..d6a90c24 100644 --- a/ift/encoder/glyph_groupings.h +++ b/ift/encoder/glyph_groupings.h @@ -126,7 +126,7 @@ class GlyphGroupings { absl::Status GroupGlyphs( const RequestedSegmentationInformation& segmentation_info, const GlyphConditionSet& glyph_condition_set, - GlyphClosureCache& closure_cache, std::optional depedency_closure, + GlyphClosureCache& closure_cache, std::optional dependency_closure, common::GlyphSet glyphs, const common::SegmentSet& modified_segments); // Converts this grouping into a finalized GlyphSegmentation. @@ -155,7 +155,7 @@ class GlyphGroupings { const GlyphConditionSet& glyph_condition_set, const common::SegmentSet& inscope_segments, GlyphClosureCache& closure_cache, - std::optional depedency_closure + std::optional dependency_closure ); // Removes all stored grouping information related to glyph with the specified diff --git a/ift/encoder/glyph_groupings_test.cc b/ift/encoder/glyph_groupings_test.cc index 612fde6f..0a5d9153 100644 --- a/ift/encoder/glyph_groupings_test.cc +++ b/ift/encoder/glyph_groupings_test.cc @@ -634,7 +634,8 @@ TEST_F(GlyphGroupingsTest, ComplexConditionFinding_Basic) { ASSERT_TRUE(glyph_groupings_complex_.UnmappedGlyphs().empty()); } -TEST_F(GlyphGroupingsTest, ComplexConditionFinding_Basic_WithDepedencyGraph) { +#ifdef HB_DEPEND_API +TEST_F(GlyphGroupingsTest, ComplexConditionFinding_Basic_WithDependencyGraph) { std::unique_ptr dep_closure = *DependencyClosure::Create( requested_segmentation_info_complex_.get(), roboto_.get()); @@ -669,6 +670,7 @@ TEST_F(GlyphGroupingsTest, ComplexConditionFinding_Basic_WithDepedencyGraph) { ASSERT_EQ(expected, glyph_groupings_complex_.ConditionsAndGlyphs()); ASSERT_TRUE(glyph_groupings_complex_.UnmappedGlyphs().empty()); } +#endif TEST_F(GlyphGroupingsTest, ComplexConditionFinding_IncrementalUnchanged) { auto sc = glyph_groupings_complex_.GroupGlyphs( diff --git a/ift/encoder/segmentation_context.cc b/ift/encoder/segmentation_context.cc index b4a08084..0404c53c 100644 --- a/ift/encoder/segmentation_context.cc +++ b/ift/encoder/segmentation_context.cc @@ -197,7 +197,7 @@ Status SegmentationContext::AnalyzeSegment(const SegmentSet& segment_ids, if (and_gids != dep_and_gids || or_gids != dep_or_gids || exclusive_gids != dep_exclusive_gids) { - LOG(ERROR) << "Mismatch between closure and depedency analysis conditions for segments " << segment_ids.ToString(); + LOG(ERROR) << "Mismatch between closure and dependency analysis conditions for segments " << segment_ids.ToString(); for (segment_index_t s : segment_ids) { LOG(ERROR) << "segment[" << s << "].codepoints = " << segmentation_info_->Segments().at(s).Definition().codepoints.ToString(); LOG(ERROR) << "segment[" << s << "].features.size() = " << segmentation_info_->Segments().at(s).Definition().feature_tags.size(); @@ -207,7 +207,7 @@ Status SegmentationContext::AnalyzeSegment(const SegmentSet& segment_ids, PrintDiff("EXC", exclusive_gids, dep_exclusive_gids); LOG(ERROR) << "init codepoints = " << segmentation_info_->InitFontSegment().codepoints.ToString(); LOG(ERROR) << "init glyphs = " << segmentation_info_->InitFontGlyphs().ToString(); - return absl::InternalError("Depedency graph conditions does not match the closure analysis conditions"); + return absl::InternalError("Dependency graph conditions does not match the closure analysis conditions"); } } diff --git a/third_party/harfbuzz.BUILD b/third_party/harfbuzz.BUILD index e17d7e14..81b0c30c 100644 --- a/third_party/harfbuzz.BUILD +++ b/third_party/harfbuzz.BUILD @@ -110,13 +110,19 @@ cc_library( ], copts = [ "-DHAVE_CONFIG_H", - "-DHB_EXPERIMENTAL_API", - "-DHB_DEPEND_API", "-Iexternal/w3c_patch_subset_incxfer/third_party/harfbuzz", ] + select({ ":macos": ["-DHAVE_XLOCALE_H=1"], "//conditions:default": [], }), + defines = [ + "HB_EXPERIMENTAL_API", + ] + select({ + "@ift_encoder//:use_harfbuzz_dep_graph": [ + "HB_DEPEND_API", + ], + "//conditions:default": [], + }), includes = [ "src", "src/hb-ucdn", diff --git a/util/BUILD b/util/BUILD index dce062d7..6248f0c8 100644 --- a/util/BUILD +++ b/util/BUILD @@ -99,10 +99,6 @@ cc_binary( srcs = [ "closure_glyph_keyed_segmenter_util.cc", ], - copts = [ - "-DHB_EXPERIMENTAL_API", - "-DHB_DEPEND_API", - ], data = [ "@ift_encoder_data//:freq_data", ], diff --git a/util/segmenter_config.proto b/util/segmenter_config.proto index 5f627f71..475ad98e 100644 --- a/util/segmenter_config.proto +++ b/util/segmenter_config.proto @@ -7,17 +7,17 @@ enum ConditionAnalysisMode { // Analyzes glyph conditions using closure analysis only. CLOSURE_ONLY = 0; - // Attempts to analyze glyph conditions using a depedency + // Attempts to analyze glyph conditions using a dependency // graph where possible. When not possible it falls back // to closure analysis. This is for many cases much faster - // then pure closure based analysis. However, the depedency + // then pure closure based analysis. However, the dependency // graph approach is still experimental and may (rarely) produce // different results than the pure closure analysis approach. CLOSURE_AND_DEP_GRAPH = 1; - // Always runs both closure and depedency graph analysis, the + // Always runs both closure and dependency graph analysis, the // results are compared and an error is raised if they differ. - // Useful primarily for testing the accuracy of the depedency + // Useful primarily for testing the accuracy of the dependency // graph approach. CLOSURE_AND_VALIDATE_DEP_GRAPH = 2; }