Skip to content

Commit 635bf10

Browse files
garretriegergarretrieger
committed
Table keyed diff fixes:
- Set patch type for new tables to replacement. - Don't include patches for tables that are unchanged.
1 parent 12ad6bd commit 635bf10

File tree

2 files changed

+111
-48
lines changed

2 files changed

+111
-48
lines changed

‎ift/table_keyed_diff.cc‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "hb.h"
1010

1111
using absl::btree_set;
12+
using absl::flat_hash_map;
1213
using absl::flat_hash_set;
1314
using absl::Status;
1415
using common::FontData;
@@ -26,7 +27,9 @@ Status TableKeyedDiff::Diff(const FontData& font_base,
2627
auto derived_tags = FontHelper::GetTags(face_derived);
2728
auto diff_tags = TagsToDiff(base_tags, derived_tags);
2829

29-
absl::flat_hash_map<std::string, std::pair<uint32_t, FontData>> patches;
30+
flat_hash_map<std::string, std::pair<uint32_t, FontData>> patches;
31+
flat_hash_set<hb_tag_t> new_tables;
32+
flat_hash_set<hb_tag_t> unchanged_tables;
3033

3134
for (std::string tag : diff_tags) {
3235
hb_tag_t t = HB_TAG(tag[0], tag[1], tag[2], tag[3]);
@@ -39,10 +42,20 @@ Status TableKeyedDiff::Diff(const FontData& font_base,
3942

4043
FontData base_table;
4144
if (!replaced_tags_.contains(tag)) {
42-
base_table = FontHelper::TableData(face_base, t);
45+
if (in_base) {
46+
base_table = FontHelper::TableData(face_base, t);
47+
} else {
48+
new_tables.insert(t);
49+
}
4350
}
4451

4552
FontData derived_table = FontHelper::TableData(face_derived, t);
53+
if (base_table == derived_table) {
54+
// If table is unchanged then no diff is needed.
55+
unchanged_tables.insert(t);
56+
continue;
57+
}
58+
4659
FontData table_patch;
4760
auto sc = binary_diff_.Diff(base_table, derived_table, &table_patch);
4861
if (!sc.ok()) {
@@ -57,6 +70,14 @@ Status TableKeyedDiff::Diff(const FontData& font_base,
5770
hb_face_destroy(face_base);
5871
hb_face_destroy(face_derived);
5972

73+
for (hb_tag_t t : unchanged_tables) {
74+
std::string tag = FontHelper::ToString(t);
75+
auto it = diff_tags.find(tag);
76+
if (it != diff_tags.end()) {
77+
diff_tags.erase(it);
78+
}
79+
}
80+
6081
// Serialize to the binary format
6182
std::string data;
6283
FontHelper::WriteUInt32(HB_TAG('i', 'f', 't', 'k'), data);
@@ -86,6 +107,7 @@ Status TableKeyedDiff::Diff(const FontData& font_base,
86107
// Write out table patches
87108
for (std::string tag : diff_tags) {
88109
hb_tag_t t = HB_TAG(tag[0], tag[1], tag[2], tag[3]);
110+
89111
FontHelper::WriteUInt32(t, data);
90112

91113
auto it = patches.find(tag);
@@ -98,7 +120,7 @@ Status TableKeyedDiff::Diff(const FontData& font_base,
98120

99121
FontData& patch_data = it->second.second;
100122

101-
if (replaced_tags_.contains(tag)) {
123+
if (replaced_tags_.contains(tag) || new_tables.contains(t)) {
102124
WRITE_UINT8(0b00000001, data, "");
103125
} else {
104126
WRITE_UINT8(0b00000000, data, "");

‎ift/table_keyed_diff_test.cc‎

Lines changed: 86 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,19 @@ class TableKeyedDiffTest : public ::testing::Test {
3232
std::string tag1_str = FontHelper::ToString(tag1);
3333
std::string tag2_str = FontHelper::ToString(tag2);
3434
std::string tag3_str = FontHelper::ToString(tag3);
35+
36+
std::string header{
37+
'i', 'f', 't', 'k', 0, 0, 0, 0, // reserved
38+
39+
0, 0, 0, 1, 0, 0, 0, 2, 0, 0, 0, 3, 0, 0, 0, 4, // compat id
40+
};
3541
};
3642

3743
StatusOr<std::string> diff_table(std::string before, std::string after) {
3844
FontData base, derived;
3945
base.copy(before);
4046
derived.copy(after);
4147

42-
;
4348
BrotliBinaryDiff differ;
4449
FontData patch;
4550
auto sc = differ.Diff(base, derived, &patch);
@@ -53,32 +58,25 @@ StatusOr<std::string> diff_table(std::string before, std::string after) {
5358
TEST_F(TableKeyedDiffTest, BasicDiff) {
5459
char second_offset = 38 + 9 + diff_table("foo", "fooo")->length();
5560
char third_offset = second_offset + 9 + diff_table("bar", "baar")->length();
56-
std::string expected =
57-
std::string{
58-
'i', 'f', 't', 'k',
59-
0, 0, 0, 0, // reserved
60-
61-
0, 0, 0, 1,
62-
0, 0, 0, 2,
63-
0, 0, 0, 3,
64-
0, 0, 0, 4, // compat id
65-
66-
0, 2, // patches count
67-
0, 0, 0, 38, // patches offset[0]
68-
0, 0, 0, second_offset, // patches offset[1]
69-
0, 0, 0, third_offset, // patches offset[2]
70-
71-
't', 'a', 'g', '1',
72-
0, // flags
73-
0, 0, 0, 4, // uncompressed size
74-
} +
75-
*diff_table("foo", "fooo") +
76-
std::string{
77-
't', 'a', 'g', '2',
78-
0, // flags
79-
0, 0, 0, 4, // uncompressed size
80-
} +
81-
*diff_table("bar", "baar");
61+
std::string expected = header +
62+
std::string{
63+
0, 2, // patches count
64+
0, 0, 0, 38, // patches offset[0]
65+
0, 0, 0, second_offset, // patches offset[1]
66+
0, 0, 0, third_offset, // patches offset[2]
67+
} +
68+
std::string{
69+
't', 'a', 'g', '1',
70+
0, // flags
71+
0, 0, 0, 4, // uncompressed size
72+
} +
73+
*diff_table("foo", "fooo") +
74+
std::string{
75+
't', 'a', 'g', '2',
76+
0, // flags
77+
0, 0, 0, 4, // uncompressed size
78+
} +
79+
*diff_table("bar", "baar");
8280

8381
FontData before = FontHelper::BuildFont({
8482
{tag1, "foo"},
@@ -97,6 +95,38 @@ TEST_F(TableKeyedDiffTest, BasicDiff) {
9795
ASSERT_EQ(patch.string(), expected);
9896
}
9997

98+
TEST_F(TableKeyedDiffTest, BasicDiff_IgnoreUnchanged) {
99+
char second_offset = 34 + 9 + diff_table("bar", "baar")->length();
100+
std::string expected = header +
101+
std::string{
102+
0, 1, // patches count
103+
0, 0, 0, 34, // patches offset[0]
104+
0, 0, 0, second_offset, // patches offset[1]
105+
} +
106+
std::string{
107+
't', 'a', 'g', '2',
108+
0, // flags
109+
0, 0, 0, 4, // uncompressed size
110+
} +
111+
*diff_table("bar", "baar");
112+
113+
FontData before = FontHelper::BuildFont({
114+
{tag1, "foo"},
115+
{tag2, "bar"},
116+
});
117+
118+
FontData after = FontHelper::BuildFont({
119+
{tag1, "foo"},
120+
{tag2, "baar"},
121+
});
122+
123+
TableKeyedDiff differ(CompatId(1, 2, 3, 4));
124+
FontData patch;
125+
auto sc = differ.Diff(before, after, &patch);
126+
ASSERT_TRUE(sc.ok()) << sc;
127+
ASSERT_EQ(patch.string(), expected);
128+
}
129+
100130
/*
101131
TODO reimplement these against the new format.
102132
TEST_F(TableKeyedDiffTest, ReplacementDiff) {
@@ -158,37 +188,48 @@ TEST_F(TableKeyedDiffTest, RemoveTable) {
158188
ASSERT_TRUE(new_table.ok()) << new_table.status();
159189
ASSERT_EQ(*new_table, "foo");
160190
}
191+
*/
161192

162193
TEST_F(TableKeyedDiffTest, AddTable) {
194+
char second_offset = 38 + 9 + diff_table("foo", "fooo")->length();
195+
char third_offset = second_offset + 9 + diff_table("bar", "baar")->length();
196+
std::string expected = header +
197+
std::string{
198+
0, 2, // patches count
199+
0, 0, 0, 38, // patches offset[0]
200+
0, 0, 0, second_offset, // patches offset[1]
201+
0, 0, 0, third_offset, // patches offset[2]
202+
} +
203+
std::string{
204+
't', 'a', 'g', '1',
205+
0b00000001, // flags
206+
0, 0, 0, 4, // uncompressed size
207+
} +
208+
*diff_table("foo", "fooo") +
209+
std::string{
210+
't', 'a', 'g', '2',
211+
0, // flags
212+
0, 0, 0, 4, // uncompressed size
213+
} +
214+
*diff_table("bar", "baar");
215+
163216
FontData before = FontHelper::BuildFont({
164-
{tag1, "foo"},
217+
{tag2, "bar"},
165218
});
166219

167220
FontData after = FontHelper::BuildFont({
168-
{tag1, "foo"},
169-
{tag2, "bar"},
221+
{tag1, "fooo"},
222+
{tag2, "baar"},
170223
});
171224

172-
TableKeyedDiff differ;
225+
TableKeyedDiff differ(CompatId(1, 2, 3, 4));
173226
FontData patch;
174227
auto sc = differ.Diff(before, after, &patch);
175228
ASSERT_TRUE(sc.ok()) << sc;
176-
177-
PerTablePatch patch_proto;
178-
ASSERT_TRUE(patch_proto.ParseFromArray(patch.data(), patch.size()));
179-
ASSERT_TRUE(patch_proto.removed_tables().empty());
180-
181-
ASSERT_EQ(patch_proto.table_patches_size(), 2);
182-
183-
auto new_table = patch_table("foo", patch_proto.table_patches().at(tag1_str));
184-
ASSERT_TRUE(new_table.ok()) << new_table.status();
185-
ASSERT_EQ(*new_table, "foo");
186-
187-
new_table = patch_table("", patch_proto.table_patches().at(tag2_str));
188-
ASSERT_TRUE(new_table.ok()) << new_table.status();
189-
ASSERT_EQ(*new_table, "bar");
229+
ASSERT_EQ(patch.string(), expected);
190230
}
191231

232+
/*
192233
TEST_F(TableKeyedDiffTest, FilteredDiff) {
193234
FontData before = FontHelper::BuildFont({
194235
{tag1, "foo"},

0 commit comments

Comments
 (0)