Skip to content

Commit 0b72634

Browse files
garretriegergarretrieger
committed
Add separate brotli quality setting for the init font merging phase.
1 parent 03d900a commit 0b72634

10 files changed

+113
-92
lines changed

‎ift/encoder/candidate_merge.cc‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,8 @@ StatusOr<std::pair<double, GlyphSet>> CandidateMerge::ComputeInitFontCostDelta(
356356
} else {
357357
double before = existing_init_font_size;
358358
double after =
359-
TRY(merger.Context().patch_size_cache->GetPatchSize(new_glyph_closure));
359+
TRY(merger.Context().patch_size_cache_for_init_font->GetPatchSize(
360+
new_glyph_closure));
360361
if (after > before) {
361362
// case where after is < before happen occasionally as the result of
362363
// running with lower brotli compression quality. Ignores these in order
@@ -382,7 +383,8 @@ StatusOr<std::pair<double, GlyphSet>> CandidateMerge::ComputeInitFontCostDelta(
382383
condition.Probability(merger.Context().SegmentationInfo().Segments(),
383384
*merger.Strategy().ProbabilityCalculator()));
384385
double patch_size_before =
385-
TRY(merger.Context().patch_size_cache->GetPatchSize(glyphs)) +
386+
TRY(merger.Context().patch_size_cache_for_init_font->GetPatchSize(
387+
glyphs)) +
386388
per_request_overhead;
387389

388390
GlyphSet new_glyphs = glyphs;
@@ -395,7 +397,8 @@ StatusOr<std::pair<double, GlyphSet>> CandidateMerge::ComputeInitFontCostDelta(
395397

396398
if (!new_glyphs.empty()) {
397399
double patch_size_after =
398-
TRY(merger.Context().patch_size_cache->GetPatchSize(new_glyphs)) +
400+
TRY(merger.Context().patch_size_cache_for_init_font->GetPatchSize(
401+
new_glyphs)) +
399402
per_request_overhead;
400403
double cost_after = patch_probability * patch_size_after;
401404

‎ift/encoder/candidate_merge_test.cc‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ TEST_F(CandidateMergeTest, AssessMerge_CostDeltas) {
121121
auto probability_calculator =
122122
std::make_unique<freq::MockProbabilityCalculator>(segments_with_merges);
123123

124-
ClosureGlyphSegmenter segmenter;
124+
ClosureGlyphSegmenter segmenter(8, 8);
125125
auto context =
126126
segmenter.InitializeSegmentationContext(roboto.get(), {}, segments);
127127
ASSERT_TRUE(context.ok()) << context.status();
@@ -187,7 +187,7 @@ TEST_F(CandidateMergeTest, AssessMerge_WithBestCandidate) {
187187
auto probability_calculator =
188188
std::make_unique<freq::MockProbabilityCalculator>(segments_with_merges);
189189

190-
ClosureGlyphSegmenter segmenter;
190+
ClosureGlyphSegmenter segmenter(8, 8);
191191
auto context =
192192
segmenter.InitializeSegmentationContext(roboto.get(), {}, segments);
193193
ASSERT_TRUE(context.ok()) << context.status();
@@ -252,7 +252,7 @@ TEST_F(CandidateMergeTest, AssessMerge_CostDeltas_Complex) {
252252
{{'i'}, {0.95, 0.95}},
253253
};
254254

255-
ClosureGlyphSegmenter segmenter;
255+
ClosureGlyphSegmenter segmenter(8, 8);
256256
auto context =
257257
segmenter.InitializeSegmentationContext(roboto.get(), {}, segments);
258258
ASSERT_TRUE(context.ok()) << context.status();
@@ -304,7 +304,7 @@ TEST_F(CandidateMergeTest, AssessMerge_CostDeltas_Complex_ModifiedConditions) {
304304
freq::UnicodeFrequencies frequencies{
305305
{{' ', ' '}, 100}, {{'a', 'a'}, 50}, {{'f', 'f'}, 75}, {{'i', 'i'}, 95}};
306306

307-
ClosureGlyphSegmenter segmenter;
307+
ClosureGlyphSegmenter segmenter(8, 8);
308308
auto context =
309309
segmenter.InitializeSegmentationContext(roboto.get(), {}, segments);
310310
ASSERT_TRUE(context.ok()) << context.status();
@@ -392,7 +392,7 @@ TEST_F(CandidateMergeTest, AssessPatchMerge) {
392392
auto probability_calculator =
393393
std::make_unique<freq::MockProbabilityCalculator>(segments_with_merges);
394394

395-
ClosureGlyphSegmenter segmenter;
395+
ClosureGlyphSegmenter segmenter(8, 8);
396396
auto context =
397397
segmenter.InitializeSegmentationContext(roboto.get(), {}, segments);
398398
ASSERT_TRUE(context.ok()) << context.status();
@@ -443,7 +443,7 @@ TEST_F(CandidateMergeTest, AssessPatchMerge_RequiresPatches) {
443443
auto probability_calculator =
444444
std::make_unique<freq::MockProbabilityCalculator>(segments_with_merges);
445445

446-
ClosureGlyphSegmenter segmenter;
446+
ClosureGlyphSegmenter segmenter(8, 8);
447447
auto context =
448448
segmenter.InitializeSegmentationContext(roboto.get(), {}, segments);
449449
ASSERT_TRUE(context.ok()) << context.status();

‎ift/encoder/closure_glyph_segmenter.cc‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ Status ValidateIncrementalGroupings(hb_face_t* face,
8888
const SegmentationContext& context) {
8989
SegmentationContext non_incremental_context(
9090
face, context.SegmentationInfo().InitFontSegment(),
91-
context.SegmentationInfo().Segments(), 1);
91+
context.SegmentationInfo().Segments(), 1, 1);
9292

9393
// Compute the glyph groupings/conditions from scratch to compare against the
9494
// incrementall produced ones.
@@ -304,7 +304,7 @@ static StatusOr<std::vector<Segment>> ToOrderedSegments(
304304
StatusOr<GlyphSegmentation> ClosureGlyphSegmenter::CodepointToGlyphSegments(
305305
hb_face_t* face, SubsetDefinition initial_segment,
306306
const std::vector<SubsetDefinition>& subset_definitions,
307-
std::optional<MergeStrategy> strategy, uint32_t brotli_quality) const {
307+
std::optional<MergeStrategy> strategy) const {
308308
btree_map<SegmentSet, MergeStrategy> merge_groups;
309309
if (!subset_definitions.empty() && strategy.has_value()) {
310310
SegmentSet all;
@@ -313,7 +313,7 @@ StatusOr<GlyphSegmentation> ClosureGlyphSegmenter::CodepointToGlyphSegments(
313313
}
314314

315315
return CodepointToGlyphSegments(face, initial_segment, subset_definitions,
316-
merge_groups, brotli_quality, false);
316+
merge_groups, false);
317317
}
318318

319319
StatusOr<std::vector<Merger>> ToMergers(
@@ -331,7 +331,7 @@ StatusOr<std::vector<Merger>> ToMergers(
331331
StatusOr<GlyphSegmentation> ClosureGlyphSegmenter::CodepointToGlyphSegments(
332332
hb_face_t* face, SubsetDefinition initial_segment,
333333
const std::vector<SubsetDefinition>& subset_definitions,
334-
btree_map<SegmentSet, MergeStrategy> merge_groups, uint32_t brotli_quality,
334+
btree_map<SegmentSet, MergeStrategy> merge_groups,
335335
bool place_fallback_in_init) const {
336336
for (const auto& [segments, strategy] : merge_groups) {
337337
if (strategy.UseCosts()) {
@@ -343,7 +343,7 @@ StatusOr<GlyphSegmentation> ClosureGlyphSegmenter::CodepointToGlyphSegments(
343343
std::vector<Segment> segments =
344344
TRY(ToOrderedSegments(subset_definitions, merge_groups, with_shared));
345345
SegmentationContext context = TRY(InitializeSegmentationContext(
346-
face, initial_segment, std::move(segments), brotli_quality));
346+
face, initial_segment, std::move(segments)));
347347

348348
std::vector<Merger> mergers =
349349
TRY(ToMergers(context, with_shared, merge_groups));
@@ -440,7 +440,7 @@ StatusOr<GlyphSegmentation> ClosureGlyphSegmenter::CodepointToGlyphSegments(
440440
StatusOr<SegmentationContext>
441441
ClosureGlyphSegmenter::InitializeSegmentationContext(
442442
hb_face_t* face, SubsetDefinition initial_segment,
443-
std::vector<Segment> segments, uint32_t brotli_quality) const {
443+
std::vector<Segment> segments) const {
444444
uint32_t glyph_count = hb_face_get_glyph_count(face);
445445
if (!glyph_count) {
446446
return absl::InvalidArgumentError("Provided font has no glyphs.");
@@ -451,7 +451,8 @@ ClosureGlyphSegmenter::InitializeSegmentationContext(
451451
AddInitSubsetDefaults(initial_segment);
452452

453453
// No merging is done during init.
454-
SegmentationContext context(face, initial_segment, segments, brotli_quality);
454+
SegmentationContext context(face, initial_segment, segments, brotli_quality_,
455+
init_font_merging_brotli_quality_);
455456

456457
// ### Generate the initial conditions and groupings by processing all
457458
// segments and glyphs. ###

‎ift/encoder/closure_glyph_segmenter.h‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ struct SegmentationCost {
3333
*/
3434
class ClosureGlyphSegmenter {
3535
public:
36+
ClosureGlyphSegmenter(uint32_t brotli_quality,
37+
uint32_t init_font_merging_brotli_quality)
38+
: brotli_quality_(brotli_quality),
39+
init_font_merging_brotli_quality_(init_font_merging_brotli_quality) {}
40+
3641
/*
3742
* Analyzes a set of codepoint segments using a subsetter closure and computes
3843
* a GlyphSegmentation which will satisfy the "glyph closure requirement" for
@@ -44,14 +49,13 @@ class ClosureGlyphSegmenter {
4449
absl::StatusOr<GlyphSegmentation> CodepointToGlyphSegments(
4550
hb_face_t* face, SubsetDefinition initial_segment,
4651
const std::vector<SubsetDefinition>& subset_definitions,
47-
std::optional<MergeStrategy> strategy = std::nullopt,
48-
uint32_t brotli_quality = 8) const;
52+
std::optional<MergeStrategy> strategy = std::nullopt) const;
4953

5054
absl::StatusOr<GlyphSegmentation> CodepointToGlyphSegments(
5155
hb_face_t* face, SubsetDefinition initial_segment,
5256
const std::vector<SubsetDefinition>& subset_definitions,
5357
absl::btree_map<common::SegmentSet, MergeStrategy> merge_groups,
54-
uint32_t brotli_quality, bool place_fallback_in_init) const;
58+
bool place_fallback_in_init) const;
5559

5660
/*
5761
* Generates a segmentation context for the provided segmentation input.
@@ -61,7 +65,7 @@ class ClosureGlyphSegmenter {
6165
*/
6266
absl::StatusOr<SegmentationContext> InitializeSegmentationContext(
6367
hb_face_t* face, SubsetDefinition initial_segment,
64-
std::vector<Segment> segments, uint32_t brotli_quality = 8) const;
68+
std::vector<Segment> segments) const;
6569

6670
/*
6771
* Computes the total cost (expected number of bytes transferred) for a given
@@ -70,6 +74,10 @@ class ClosureGlyphSegmenter {
7074
absl::StatusOr<SegmentationCost> TotalCost(
7175
hb_face_t* original_face, const GlyphSegmentation& segmentation,
7276
const freq::ProbabilityCalculator& probability_calculator) const;
77+
78+
private:
79+
uint32_t brotli_quality_;
80+
uint32_t init_font_merging_brotli_quality_;
7381
};
7482

7583
} // namespace ift::encoder

‎ift/encoder/closure_glyph_segmenter_test.cc‎

Lines changed: 55 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@ class ClosureGlyphSegmenterTest : public ::testing::Test {
2727
protected:
2828
ClosureGlyphSegmenterTest()
2929
: roboto(make_hb_face(nullptr)),
30-
noto_nastaliq_urdu(make_hb_face(nullptr)) {
30+
noto_nastaliq_urdu(make_hb_face(nullptr)),
31+
segmenter(8, 8) {
3132
roboto = from_file("common/testdata/Roboto-Regular.ttf");
3233
noto_nastaliq_urdu =
3334
from_file("common/testdata/NotoNastaliqUrdu.subset.ttf");
@@ -402,7 +403,7 @@ TEST_F(ClosureGlyphSegmenterTest,
402403
UnmappedGlyphs_FallbackSegmentMovedToInitFont) {
403404
auto segmentation = segmenter.CodepointToGlyphSegments(
404405
noto_nastaliq_urdu.get(), {}, {{0x62a}, {0x62b}, {0x62c}, {0x62d}},
405-
btree_map<SegmentSet, MergeStrategy>{}, 8, true);
406+
btree_map<SegmentSet, MergeStrategy>{}, true);
406407
ASSERT_TRUE(segmentation.ok()) << segmentation.status();
407408

408409
ASSERT_EQ(segmentation->UnmappedGlyphs().size(), 0);
@@ -789,7 +790,7 @@ TEST_F(ClosureGlyphSegmenterTest, TotalCost) {
789790
GlyphSegmentation::GroupsToSegmentation({}, {}, {}, {}, segmentation1);
790791
ASSERT_TRUE(sc.ok()) << sc;
791792

792-
ClosureGlyphSegmenter segmenter;
793+
ClosureGlyphSegmenter segmenter(8, 8);
793794
SegmentationCost base_cost =
794795
*segmenter.TotalCost(roboto.get(), segmentation1, calculator);
795796
ASSERT_GT(base_cost.total_cost, 1000);
@@ -1071,24 +1072,23 @@ TEST_F(ClosureGlyphSegmenterTest, MultipleMergeGroups) {
10711072
*MergeStrategy::CostBased(std::move(group2_freq), 75, 2)},
10721073
};
10731074

1074-
auto segmentation =
1075-
segmenter.CodepointToGlyphSegments(roboto.get(), {},
1076-
{{'a'},
1077-
{'b'},
1078-
{'c'},
1079-
{'d'},
1080-
{'e'},
1081-
{'f'},
1082-
{'g'},
1083-
{'h'},
1084-
{'i'},
1085-
{'j'},
1086-
{'k'},
1087-
{'l'},
1088-
{'m'},
1089-
{'n'},
1090-
{'o'}},
1091-
merge_groups, 8, false);
1075+
auto segmentation = segmenter.CodepointToGlyphSegments(roboto.get(), {},
1076+
{{'a'},
1077+
{'b'},
1078+
{'c'},
1079+
{'d'},
1080+
{'e'},
1081+
{'f'},
1082+
{'g'},
1083+
{'h'},
1084+
{'i'},
1085+
{'j'},
1086+
{'k'},
1087+
{'l'},
1088+
{'m'},
1089+
{'n'},
1090+
{'o'}},
1091+
merge_groups, false);
10921092
ASSERT_TRUE(segmentation.ok()) << segmentation.status();
10931093

10941094
std::vector<SubsetDefinition> expected_segments = {
@@ -1168,24 +1168,23 @@ TEST_F(ClosureGlyphSegmenterTest, MultipleMergeGroups_InitFontMove) {
11681168
{{0, 1, 7, 8, 9, 10, 11, 12, 13, 14}, s2},
11691169
};
11701170

1171-
auto segmentation =
1172-
segmenter.CodepointToGlyphSegments(roboto.get(), {},
1173-
{{'a'},
1174-
{'b'},
1175-
{'c'},
1176-
{'d'},
1177-
{'e'},
1178-
{'f'},
1179-
{'g'},
1180-
{'h'},
1181-
{'i'},
1182-
{'j'},
1183-
{'k'},
1184-
{'l'},
1185-
{'m'},
1186-
{'n'},
1187-
{'o'}},
1188-
merge_groups, 8, false);
1171+
auto segmentation = segmenter.CodepointToGlyphSegments(roboto.get(), {},
1172+
{{'a'},
1173+
{'b'},
1174+
{'c'},
1175+
{'d'},
1176+
{'e'},
1177+
{'f'},
1178+
{'g'},
1179+
{'h'},
1180+
{'i'},
1181+
{'j'},
1182+
{'k'},
1183+
{'l'},
1184+
{'m'},
1185+
{'n'},
1186+
{'o'}},
1187+
merge_groups, false);
11891188
ASSERT_TRUE(segmentation.ok()) << segmentation.status();
11901189

11911190
// Only segments from the groups that set a threshold are eligible to be moved
@@ -1255,15 +1254,14 @@ TEST_F(ClosureGlyphSegmenterTest, MultipleMergeGroups_CompositesRespectGroups) {
12551254
{{2, 3}, *MergeStrategy::CostBased(std::move(group2_freq), 75, 1)},
12561255
};
12571256

1258-
auto segmentation =
1259-
segmenter.CodepointToGlyphSegments(roboto.get(), {},
1260-
{
1261-
{'f'},
1262-
{'g'},
1263-
{'i'},
1264-
{'j'},
1265-
},
1266-
merge_groups, 8, false);
1257+
auto segmentation = segmenter.CodepointToGlyphSegments(roboto.get(), {},
1258+
{
1259+
{'f'},
1260+
{'g'},
1261+
{'i'},
1262+
{'j'},
1263+
},
1264+
merge_groups, false);
12671265
ASSERT_TRUE(segmentation.ok()) << segmentation.status();
12681266

12691267
// f + i would normally be a good merge, but here it's skipped since it
@@ -1297,15 +1295,14 @@ TEST_F(ClosureGlyphSegmenterTest, MultipleMergeGroups_Heuristic) {
12971295
{{2, 3}, MergeStrategy::Heuristic(10000)},
12981296
};
12991297

1300-
auto segmentation =
1301-
segmenter.CodepointToGlyphSegments(roboto.get(), {},
1302-
{
1303-
{'f'},
1304-
{'g'},
1305-
{'i'},
1306-
{'j'},
1307-
},
1308-
merge_groups, 8, false);
1298+
auto segmentation = segmenter.CodepointToGlyphSegments(roboto.get(), {},
1299+
{
1300+
{'f'},
1301+
{'g'},
1302+
{'i'},
1303+
{'j'},
1304+
},
1305+
merge_groups, false);
13091306
ASSERT_TRUE(segmentation.ok()) << segmentation.status();
13101307

13111308
// f + i would normally be a good merge, but here it's skipped since it
@@ -1416,7 +1413,7 @@ if (s2) then p2
14161413
{{0, 1}, *MergeStrategy::CostBased(std::move(frequencies), 75, 3)},
14171414
{{2}, MergeStrategy::None()},
14181415
},
1419-
8, false);
1416+
false);
14201417
ASSERT_TRUE(segmentation.ok()) << segmentation.status();
14211418

14221419
ASSERT_EQ(segmentation->ToString(),

0 commit comments

Comments
 (0)