Skip to content

Commit

Permalink
review feedback
Browse files Browse the repository at this point in the history 
* collapse test metrics asserts to single expression
* reduce CBDT test case size
* remove useless comments
* add links to issue for improving codegen to relevant places
  • Loading branch information
@dfrg
dfrg committed Oct 10, 2023
1 parent d4c4505 commit 05d977f
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 188 deletions.
4 changes: 3 additions & 1 deletion font-codegen/src/fields.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,8 @@ fn traversal_arm_for_field(
let data = fld.offset_getter_data_src();
quote!(Field::new(#name_str, traversal::FieldType::var_array(#typ_str, self.#name()#maybe_unwrap, #data)))
}
// HACK: who wouldn't want to hard-code ValueRecord handling
// See if there are better ways to handle these hardcoded types
// <https://github.com/googlefonts/fontations/issues/659>
FieldType::Struct { typ } if typ == "ValueRecord" || typ == "SbitLineMetrics" => {
let offset_data = pass_data
.cloned()
Expand Down Expand Up @@ -479,6 +480,7 @@ impl Field {
// catch `ValueRecord` so use this attribute to ignore it.
// Fields that require args for reading can't be read "zerocopy"
// anyway.
// <https://github.com/googlefonts/fontations/issues/659>
self.attrs.read_with_args.is_none()
&& matches!(
self.typ,
Expand Down
2 changes: 2 additions & 0 deletions font-codegen/src/record.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ pub(crate) fn generate(item: &Record, all_items: &Items) -> syn::Result<TokenStr
let mut maybe_extra_traits = item
.gets_extra_traits(all_items)
.then(|| quote!(PartialEq, Eq, PartialOrd, Ord, Hash));
// Just make all records Copy?
// <https://github.com/googlefonts/fontations/issues/659>
if item.name == "SbitLineMetrics" {
use quote::TokenStreamExt;
let mut copy_trait = quote!(Copy,);
Expand Down
Binary file modified font-test-data/test_data/ttf/embedded_bitmaps.ttf
View file
Binary file not shown.
93 changes: 13 additions & 80 deletions font-test-data/test_data/ttx/embedded_bitmaps.ttx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@
<GlyphOrder>
<!-- The 'id' attribute is only for humans; it is ignored when parsed. -->
<GlyphID id="0" name=".notdef"/>
<GlyphID id="1" name="g1"/>
<GlyphID id="2" name="g2"/>
<GlyphID id="3" name="g3"/>
<GlyphID id="4" name="numbersign"/>
<GlyphID id="1" name="eblc_3_ebdt_2"/>
<GlyphID id="2" name="eblc_3_ebdt_2_glyph2"/>
<GlyphID id="3" name="eblc_2_ebdt_5"/>
<GlyphID id="4" name="noto_emoji_style_ebdt"/>
</GlyphOrder>

<maxp>
Expand All @@ -38,7 +38,7 @@
<EBDT>
<header version="2.0"/>
<strikedata index="0">
<ebdt_bitmap_format_2 name="g1">
<ebdt_bitmap_format_2 name="eblc_3_ebdt_2">
<SmallGlyphMetrics>
<height value="8"/>
<width value="3"/>
Expand All @@ -50,7 +50,7 @@
eeaeea
</rawimagedata>
</ebdt_bitmap_format_2>
<ebdt_bitmap_format_2 name="g2">
<ebdt_bitmap_format_2 name="eblc_3_ebdt_2_glyph2">
<SmallGlyphMetrics>
<height value="8"/>
<width value="4"/>
Expand All @@ -61,22 +61,10 @@
<rawimagedata>
f0f0f0f0
</rawimagedata>
</ebdt_bitmap_format_2>
<ebdt_bitmap_format_2 name="g3">
<SmallGlyphMetrics>
<height value="8"/>
<width value="4"/>
<BearingX value="0"/>
<BearingY value="6"/>
<Advance value="4"/>
</SmallGlyphMetrics>
<rawimagedata>
abababab
</rawimagedata>
</ebdt_bitmap_format_2>
</strikedata>
<strikedata index="1">
<ebdt_bitmap_format_5 name="g3">
<ebdt_bitmap_format_5 name="eblc_2_ebdt_5">
<rawimagedata>
aabbccdd 00112233 ffee1234 424242aa
88990011
Expand Down Expand Up @@ -128,8 +116,8 @@
<!-- GlyphIds are written but not read. The firstGlyphIndex and
lastGlyphIndex values will be recalculated by the compiler. -->
<eblc_index_sub_table_3 imageFormat="2" firstGlyphIndex="1" lastGlyphIndex="2">
<glyphLoc id="1" name="g1"/>
<glyphLoc id="2" name="g2"/>
<glyphLoc id="1" name="eblc_3_ebdt_2"/>
<glyphLoc id="2" name="eblc_3_ebdt_2_glyph2"/>
</eblc_index_sub_table_3>
</strike>
<strike index="1">
Expand Down Expand Up @@ -184,15 +172,15 @@
<vertBearingY value="-9"/>
<vertAdvance value="0"/>
</BigGlyphMetrics>
<glyphLoc id="3" name="g3"/>
<glyphLoc id="3" name="eblc_2_ebdt_5"/>
</eblc_index_sub_table_2>
</strike>
</EBLC>

<CBDT>
<header version="3.0"/>
<strikedata index="0">
<cbdt_bitmap_format_17 name="numbersign">
<cbdt_bitmap_format_17 name="noto_emoji_style_ebdt">
<SmallGlyphMetrics>
<height value="128"/>
<width value="136"/>
Expand All @@ -201,62 +189,7 @@
<Advance value="136"/>
</SmallGlyphMetrics>
<rawimagedata>
89504e47 0d0a1a0a 0000000d 49484452
00000088 00000080 08030000 00e737d1
0d000000 8a504c54 4547704c 5c5c5c75
75756d6d 6d727272 7474746a 69696c6c
6c696969 6f6f6f6b 6b6b6968 68737272
56555570 70706d6d 6d6b6b6b 61606068
68686666 66646363 64636354 53535b59
59616060 52515154 5353605f 5f5e5e5e
5d5d5d5b 5b5b5150 505a5959 51505052
51515756 56515050 51505052 51515453
534f4e4e 4f4e4e51 50504f4e 4e4f4e4e
4f4e4eb5 c8e4e900 00002e74 524e5300
208040eb ff511070 a3c38fff 30ffffff
80ffffea ff60bfff af9fffff ffff70ff
10cfff40 ef8fff80 bfff50ff df66dbac
80000002 6b494441 547801ed d8878eea
400c85e1 13c2d27b 872dd909 1d26efff
78b76107 4b9adb22 e2a0953f b5e8df36
5e4c8579 06c61863 8c31c618 13d57e89
21c5b718 414f547f b96988d8 a0566f42
4deb85b4 45ec708c a1a6db23 3511eb1c
63a8e9f4 4803b966 8f414f8f 45c8c5dc
3a50d31f 30116b94 7a6da869 0c881cbe
cdb10635 b5e1cda0 2bf76648 34777548
1a220e07 149b5033 1cddc8e1 e311a943
4d73c444 6c71eb40 4d1cfa9b 5d8e63a8
194f485b ee0dc706 d4b42744 0e3f6111
d474a624 46ae3f65 d033654d e41adc34
77754a66 726f3876 a1a63527 72f80eb5
e9026aba 73227775 c631869a e59cc817
23730635 fdd57c75 b346aeb5 620b94a8
31beebce 56643e5f 72dcac72 f30db5b8
8473bc0a 6f6ff7eb 557ec157 f2ba8147
1bbf1532 fec20779 2fa48483 7c14f285
0f927c16 f2f883b8 e56701a9 4389a274
cb960e64 b7cf63e2 a064b765 1172074a
9f29a07e 903de441 c8016a92 23598ab8
e4b8809a cd919c44 dc73dc41 cd39307c
7464d073 617de476 dc52a8e9 5f988827
6e1ba8b9 7a22875f 733c41cd c913397c
ca517357 3d4944f4 cc41cd3e 30fc8edb
1e6a5c68 f884db19 4ac4f099 88872a77
550e7fd6 3f88cb3c eb83f5bd ce4db338
df653eb7 3f337f97 715b9470 0e5fc8a2
84ad28e4 6407b183 a81f24f1 85247834
b7f1059c 1d4a9478 9645206e edd9c641
4b1af8c7 f79feec5 48f6ac2f 46ecc5c8
156a36a1 e13d8ba0 260d0cdf f70c5ac2
c35f2bd9 5516da9b 03d42cfe b8ab09d4
1c42c367 4ff26ed3 79862779 804fa126
fae3aeae 2bfe6464 53e503fc bae24f46
d6cff100 1fbea3f6 b9ed2b7e 80df693e
c0b373e0 6f467b8a 0be871c9 e9a7c441
e89f7e59 c018638c 31c61863 4cf5be03
d8291f21 ceb2e953 00000000 49454e44
ae426082
89504e47 0d0a1a0a
</rawimagedata>
</cbdt_bitmap_format_17>
</strikedata>
Expand Down Expand Up @@ -305,7 +238,7 @@
<!-- GlyphIds are written but not read. The firstGlyphIndex and
lastGlyphIndex values will be recalculated by the compiler. -->
<eblc_index_sub_table_1 imageFormat="17" firstGlyphIndex="4" lastGlyphIndex="4">
<glyphLoc id="4" name="numbersign"/>
<glyphLoc id="4" name="noto_emoji_style_ebdt"/>
</eblc_index_sub_table_1>
</strike>
</CBLC>
Expand Down
93 changes: 14 additions & 79 deletions read-fonts/src/tables/cbdt.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,77 +22,22 @@ mod tests {
let cbdt = font.cbdt().unwrap();
let size = &cblc.bitmap_sizes()[0];
// Metrics for size at index 0
assert_eq!(size.hori.ascender(), 101);
assert_eq!(size.hori.descender(), -27);
assert_eq!(size.hori.width_max(), 136);
assert_eq!(size.vert.ascender(), 101);
assert_eq!(size.vert.descender(), -27);
assert_eq!(size.vert.width_max(), 136);
assert_eq!(size.start_glyph_index(), GlyphId::new(4));
assert_eq!(size.end_glyph_index(), GlyphId::new(4));
assert_eq!(size.ppem_x(), 109);
assert_eq!(size.ppem_y(), 109);
assert_eq!(size.bit_depth(), 32);
assert!(
size.hori.ascender() == 101
&& size.hori.descender() == -27
&& size.hori.width_max() == 136
&& size.vert.ascender() == 101
&& size.vert.descender() == -27
&& size.vert.width_max() == 136
&& size.start_glyph_index() == GlyphId::new(4)
&& size.end_glyph_index() == GlyphId::new(4)
&& size.ppem_x() == 109
&& size.ppem_y() == 109
&& size.bit_depth() == 32
);
let expected: &[(GlyphId, &[u8], SmallGlyphMetrics)] = &[(
GlyphId::new(4),
&raw_image_data(
r#"89504e47 0d0a1a0a 0000000d 49484452
00000088 00000080 08030000 00e737d1
0d000000 8a504c54 4547704c 5c5c5c75
75756d6d 6d727272 7474746a 69696c6c
6c696969 6f6f6f6b 6b6b6968 68737272
56555570 70706d6d 6d6b6b6b 61606068
68686666 66646363 64636354 53535b59
59616060 52515154 5353605f 5f5e5e5e
5d5d5d5b 5b5b5150 505a5959 51505052
51515756 56515050 51505052 51515453
534f4e4e 4f4e4e51 50504f4e 4e4f4e4e
4f4e4eb5 c8e4e900 00002e74 524e5300
208040eb ff511070 a3c38fff 30ffffff
80ffffea ff60bfff af9fffff ffff70ff
10cfff40 ef8fff80 bfff50ff df66dbac
80000002 6b494441 547801ed d8878eea
400c85e1 13c2d27b 872dd909 1d26efff
78b76107 4b9adb22 e2a0953f b5e8df36
5e4c8579 06c61863 8c31c618 13d57e89
21c5b718 414f547f b96988d8 a0566f42
4deb85b4 45ec708c a1a6db23 3511eb1c
63a8e9f4 4803b966 8f414f8f 45c8c5dc
3a50d31f 30116b94 7a6da869 0c881cbe
cdb10635 b5e1cda0 2bf76648 34777548
1a220e07 149b5033 1cddc8e1 e311a943
4d73c444 6c71eb40 4d1cfa9b 5d8e63a8
194f485b ee0dc706 d4b42744 0e3f6111
d474a624 46ae3f65 d033654d e41adc34
77754a66 726f3876 a1a63527 72f80eb5
e9026aba 73227775 c631869a e59cc817
23730635 fdd57c75 b346aeb5 620b94a8
31beebce 56643e5f 72dcac72 f30db5b8
8473bc0a 6f6ff7eb 557ec157 f2ba8147
1bbf1532 fec20779 2fa48483 7c14f285
0f927c16 f2f883b8 e56701a9 4389a274
cb960e64 b7cf63e2 a064b765 1172074a
9f29a07e 903de441 c8016a92 23598ab8
e4b8809a cd919c44 dc73dc41 cd39307c
7464d073 617de476 dc52a8e9 5f988827
6e1ba8b9 7a22875f 733c41cd c913397c
ca517357 3d4944f4 cc41cd3e 30fc8edb
1e6a5c68 f884db19 4ac4f099 88872a77
550e7fd6 3f88cb3c eb83f5bd ce4db338
df653eb7 3f337f97 715b9470 0e5fc8a2
84ad28e4 6407b183 a81f24f1 85247834
b7f1059c 1d4a9478 9645206e edd9c641
4b1af8c7 f79feec5 48f6ac2f 46ecc5c8
156a36a1 e13d8ba0 260d0cdf f70c5ac2
c35f2bd9 5516da9b 03d42cfe b8ab09d4
1c42c367 4ff26ed3 79862779 804fa126
fae3aeae 2bfe6464 53e503fc bae24f46
d6cff100 1fbea3f6 b9ed2b7e 80df693e
c0b373e0 6f467b8a 0be871c9 e9a7c441
e89f7e59 c018638c 31c61863 4cf5be03
d8291f21 ceb2e953 00000000 49454e44
ae426082"#,
),
&[0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a],
SmallGlyphMetrics {
height: 128,
width: 136,
Expand All @@ -103,22 +48,12 @@ mod tests {
)];
for (gid, data, metrics) in expected {
let location = size.location(cblc.offset_data(), *gid).unwrap();
// all glyphs have data format == 17
assert_eq!(location.format, 17);
let bitmap_data = cbdt.data(&location).unwrap();
let (img_fmt, img_data) = bitmap_data.content.extract_data();
// all glyphs are PNG
assert_eq!(img_fmt, BitmapDataFormat::Png);
assert_eq!(img_data, *data);
assert_eq!(bitmap_data.extract_small_metrics(), metrics);
}
}

fn raw_image_data(raw: &str) -> Vec<u8> {
raw.replace([' ', '\n', '\r'], "")
.as_bytes()
.chunks(2)
.map(|str_hex| u8::from_str_radix(std::str::from_utf8(str_hex).unwrap(), 16).unwrap())
.collect()
}
}
56 changes: 28 additions & 28 deletions read-fonts/src/tables/ebdt.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,20 @@ mod tests {
let ebdt = font.ebdt().unwrap();
let size = &eblc.bitmap_sizes()[0];
// Metrics for size at index 0
assert_eq!(size.hori.ascender(), 6);
assert_eq!(size.hori.descender(), 2);
assert_eq!(size.hori.width_max(), 4);
assert_eq!(size.hori.max_before_bl(), 6);
assert_eq!(size.hori.min_after_bl(), -2);
assert_eq!(size.vert.ascender(), 6);
assert_eq!(size.vert.descender(), 2);
assert_eq!(size.start_glyph_index(), GlyphId::new(1));
assert_eq!(size.end_glyph_index(), GlyphId::new(2));
assert_eq!(size.ppem_x(), 7);
assert_eq!(size.ppem_y(), 7);
assert_eq!(size.bit_depth(), 1);
assert!(
size.hori.ascender() == 6
&& size.hori.descender() == 2
&& size.hori.width_max() == 4
&& size.hori.max_before_bl() == 6
&& size.hori.min_after_bl() == -2
&& size.vert.ascender() == 6
&& size.vert.descender() == 2
&& size.start_glyph_index() == GlyphId::new(1)
&& size.end_glyph_index() == GlyphId::new(2)
&& size.ppem_x() == 7
&& size.ppem_y() == 7
&& size.bit_depth() == 1
);
// Bit aligned formats in this strike:
let expected: &[(GlyphId, &[u8], SmallGlyphMetrics)] = &[
(
Expand Down Expand Up @@ -82,11 +84,9 @@ mod tests {
];
for (gid, data, metrics) in expected {
let location = size.location(eblc.offset_data(), *gid).unwrap();
// all glyphs have data format == 2
assert_eq!(location.format, 2);
let bitmap_data = ebdt.data(&location).unwrap();
let (img_fmt, img_data) = bitmap_data.content.extract_data();
// all glyphs are bit aligned
assert_eq!(img_fmt, BitmapDataFormat::BitAligned);
assert_eq!(img_data, *data);
assert_eq!(bitmap_data.extract_small_metrics(), metrics);
Expand All @@ -100,18 +100,20 @@ mod tests {
let ebdt = font.ebdt().unwrap();
let size = &eblc.bitmap_sizes()[1];
// Metrics for size at index 1
assert_eq!(size.hori.ascender(), 12);
assert_eq!(size.hori.descender(), 5);
assert_eq!(size.hori.width_max(), 9);
assert_eq!(size.hori.max_before_bl(), 12);
assert_eq!(size.hori.min_after_bl(), -5);
assert_eq!(size.vert.ascender(), 12);
assert_eq!(size.vert.descender(), 5);
assert_eq!(size.start_glyph_index(), GlyphId::new(3));
assert_eq!(size.end_glyph_index(), GlyphId::new(3));
assert_eq!(size.ppem_x(), 15);
assert_eq!(size.ppem_y(), 15);
assert_eq!(size.bit_depth(), 1);
assert!(
size.hori.ascender() == 12
&& size.hori.descender() == 5
&& size.hori.width_max() == 9
&& size.hori.max_before_bl() == 12
&& size.hori.min_after_bl() == -5
&& size.vert.ascender() == 12
&& size.vert.descender() == 5
&& size.start_glyph_index() == GlyphId::new(3)
&& size.end_glyph_index() == GlyphId::new(3)
&& size.ppem_x() == 15
&& size.ppem_y() == 15
&& size.bit_depth() == 1
);
let expected: &[(GlyphId, &[u8])] = &[(
GlyphId::new(3),
&[
Expand All @@ -135,11 +137,9 @@ mod tests {
vert_advance: 0,
})
);
// all glyphs have data format == 5
assert_eq!(location.format, 5);
let bitmap_data = ebdt.data(&location).unwrap();
let (img_fmt, img_data) = bitmap_data.content.extract_data();
// all glyphs are bit aligned
assert_eq!(img_fmt, BitmapDataFormat::BitAligned);
assert_eq!(img_data, *data);
}
Expand Down

0 comments on commit 05d977f

Please sign in to comment.