From 195e55291e403a1a6c05877b945c654783765993 Mon Sep 17 00:00:00 2001 From: Chad Brokaw Date: Thu, 10 Oct 2024 12:27:50 -0400 Subject: [PATCH] [skrifa] cff: allow empty private dict and fix scaling We were erroneously rejecting a Private DICT with length 0. Also, the value `Fixed::ONE` was used as a sentinel for unscaled drawing but this value can be produced when upem is 512 and requested ppem is 8, resulting in incorrect scaling. Replaced the scale factor with an option to correct this. --- font-test-data/src/lib.rs | 3 + font-test-data/test_data/README.md | 10 + .../test_data/ttf/material_icons_subset.ttf | Bin 0 -> 1272 bytes .../test_data/ttx/material_icons_subset.ttx | 256 ++++++++++++++++++ skrifa/src/outline/cff/mod.rs | 68 +++-- 5 files changed, 319 insertions(+), 18 deletions(-) create mode 100644 font-test-data/test_data/ttf/material_icons_subset.ttf create mode 100644 font-test-data/test_data/ttx/material_icons_subset.ttx diff --git a/font-test-data/src/lib.rs b/font-test-data/src/lib.rs index 407ecff28..0b6242363 100644 --- a/font-test-data/src/lib.rs +++ b/font-test-data/src/lib.rs @@ -87,6 +87,9 @@ pub static AHEM: &[u8] = include_bytes!("../test_data/ttf/ahem.ttf"); pub static AVAR2_CHECKER: &[u8] = include_bytes!("../test_data/ttf/avar2checker.ttf"); +pub static MATERIAL_ICONS_SUBSET: &[u8] = + include_bytes!("../test_data/ttf/material_icons_subset.ttf"); + pub mod varc { pub static CJK_6868: &[u8] = include_bytes!("../test_data/ttf/varc-6868.ttf"); pub static CONDITIONALS: &[u8] = include_bytes!("../test_data/ttf/varc-ac01-conditional.ttf"); diff --git a/font-test-data/test_data/README.md b/font-test-data/test_data/README.md index 3a9431bdc..59b391006 100644 --- a/font-test-data/test_data/README.md +++ b/font-test-data/test_data/README.md @@ -73,6 +73,16 @@ Describes the provenance, usage and generation procedures for font data used for pyftsubset notoserif-regular.ttf --layout-features+=c2sc --no-hinting --text=Hfix ``` +* _material_icons_subset_ + * font: Google Material Icons Regular + * source: https://fonts.googleapis.com/icon?family=Material+Icons + * license: [Apache 2][Apache2] + * usage: testing empty Private DICT and scaling with upem=512, ppem=8 + * subset: just a single glyph to check scaling + ```shell + pyftsubset material_icons.otf --gids=2 + ``` + * _avar2checker_ * font: avar2 checker * source: https://github.com/Lorp/fencer/tree/main/src/fonts diff --git a/font-test-data/test_data/ttf/material_icons_subset.ttf b/font-test-data/test_data/ttf/material_icons_subset.ttf new file mode 100644 index 0000000000000000000000000000000000000000..ab473c789a5804ff03c42dedcd39bb49f8e98556 GIT binary patch literal 1272 zcmbtU-%C?b9RHrXyIqShOSbAkwmlSKHQVM|WQCZeNQ$XTs6FUKryK4UyEE34)0e=# zL~Z&KOISVGgHVAWjNa`j`UA=!81-n}!JzdycT1;W6rIcYp3mp|>wNF`oO_3Sz9BkF zY0{Csr?=Ps`s30|B6@>a?Hj#&W8>?_2O{x3__?9ci|z}zu3LzNQ{WSm5jjrA(hJyA z;GSSWo@(FTi4!p%?+rpB>I4S91AaOfNu~q<@b3_Yh@6U(aEB{iaUPW;fwR`G734=g z_A?$+lBf$qgadc~nanOQN=un?uAJ4Y#^#Lrv_)+`P6A_k`?$+F`=^Z9_!0LM=gV1BIa_Wk`z)2B zK}n?)Tcs!|`BcHG77c}bzF<>}QX!RBtQE6Z2~~<}|LXZQ)jak|EqbMHzh!f_VqV*n z*8S@l|JLT#bY|MGub8h|-WgW1xnZyhttFD#^Om!x!VVa1yJW?8#EyJax( z259r`y=SfC&DVZ3>1tiwe(sWCp)ZcblhrC(#WcN+_l%GFqQ%xQm(?mxh-YjZXt6-- zG3gtXhvQ=nrY;OhmxfEjdwVpPIkanBqWd^qCLRF`V%9LDb6_5!bD~bKwYqJQMT)atfaN;|EE`PTU>>;_>}7iTqJSjX*y`bI{7L|9xDoZZ~;Q zx0AeD^&av8U#3pHPY0wE)^^xB@ODm2&p+kgUm5Sx|Lk_K2YVfG9OwbR0Q`FJuiMN| zPg_NF4Ieum@X|29!(5(NuisDYUjUE82G3%`-^{2v2se8Nd}IpC$tbl@)9)ajmFO3I CvGD`| literal 0 HcmV?d00001 diff --git a/font-test-data/test_data/ttx/material_icons_subset.ttx b/font-test-data/test_data/ttx/material_icons_subset.ttx new file mode 100644 index 000000000..5bf1a20df --- /dev/null +++ b/font-test-data/test_data/ttx/material_icons_subset.ttx @@ -0,0 +1,256 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Copyright 2019-2023 Google LLC. All Rights Reserved. + + + Google Material Icons + + + Regular + + + Google Material Icons 2024-06-24T07:13:04.639094 + + + Google Material Icons Regular + + + 2024-06-24T07:13:04.639094 + + + GoogleMaterialIcons-Regular + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + 512 endchar + + + 512 405 448 rmoveto + -298 hlineto + -24 -19 -19 -24 hvcurveto + -298 vlineto + -24 19 -19 24 vhcurveto + 298 hlineto + 24 19 19 24 hvcurveto + 298 vlineto + 24 -19 19 -24 vhcurveto + -128 vmoveto + -213 -298 181 21 -96 32 128 -53 85 298 -85 vlineto + -74 -48 rmoveto + 48 -32 -128 32 48 vlineto + 37 -48 37 0 -48 64 48 64 -37 0 -37 -48 rlineto + -128 -80 rmoveto + 53 hlineto + 12 9 10 11 hvcurveto + 86 vlineto + 11 -9 10 -12 vhcurveto + -53 hlineto + -12 -10 -10 -11 hvcurveto + -86 vlineto + -11 10 -10 12 vhcurveto + 10 96 rmoveto + 32 -64 -32 64 hlineto + endchar + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/skrifa/src/outline/cff/mod.rs b/skrifa/src/outline/cff/mod.rs index 52dfc0caa..02c812c88 100644 --- a/skrifa/src/outline/cff/mod.rs +++ b/skrifa/src/outline/cff/mod.rs @@ -169,13 +169,16 @@ impl<'a> Outlines<'a> { Some(ppem) if self.units_per_em > 0 => { // Note: we do an intermediate scale to 26.6 to ensure we // match FreeType - Fixed::from_bits((ppem * 64.) as i32) / Fixed::from_bits(self.units_per_em as i32) + Some( + Fixed::from_bits((ppem * 64.) as i32) + / Fixed::from_bits(self.units_per_em as i32), + ) } - _ => Fixed::ONE, + _ => None, }; // When hinting, use a modified scale factor // - let hint_scale = Fixed::from_bits((scale.to_bits() + 32) / 64); + let hint_scale = Fixed::from_bits((scale.unwrap_or(Fixed::ONE).to_bits() + 32) / 64); let hint_state = HintState::new(&hint_params, hint_scale); Ok(Subfont { is_cff2: self.is_cff2(), @@ -210,7 +213,8 @@ impl<'a> Outlines<'a> { let blend_state = subfont.blend_state(self, coords)?; let mut pen_sink = PenSink::new(pen); let mut simplifying_adapter = NopFilteringSink::new(&mut pen_sink); - if hint { + // Only apply hinting if we have a scale + if hint && subfont.scale.is_some() { let mut hinting_adapter = HintingSink::new(&subfont.hint_state, &mut simplifying_adapter); charstring::evaluate( @@ -250,14 +254,12 @@ impl<'a> Outlines<'a> { } range } else { - // Last chance, use the private dict range from the top dict if - // available. + // Use the private dict range from the top dict. + // Note: "A Private DICT is required but may be specified as having + // a length of 0 if there are no non-default values to be stored." + // let range = self.top_dict.private_dict_range.clone(); - if !range.is_empty() { - Some(range.start as usize..range.end as usize) - } else { - None - } + Some(range.start as usize..range.end as usize) } .ok_or(Error::MissingPrivateDict) } @@ -273,7 +275,7 @@ impl<'a> Outlines<'a> { #[derive(Clone)] pub(crate) struct Subfont { is_cff2: bool, - scale: Fixed, + scale: Option, subrs_offset: Option, pub(crate) hint_state: HintState, store_index: u16, @@ -396,11 +398,11 @@ where /// scaling process. struct ScalingSink26Dot6<'a, S> { inner: &'a mut S, - scale: Fixed, + scale: Option, } impl<'a, S> ScalingSink26Dot6<'a, S> { - fn new(sink: &'a mut S, scale: Fixed) -> Self { + fn new(sink: &'a mut S, scale: Option) -> Self { Self { scale, inner: sink } } @@ -419,11 +421,11 @@ impl<'a, S> ScalingSink26Dot6<'a, S> { // converts to font units. // let b = Fixed::from_bits(a.to_bits() >> 10); - if self.scale != Fixed::ONE { + if let Some(scale) = self.scale { // Scaled case: // 3. Multiply by the original scale factor (to 26.6) // - let c = b * self.scale; + let c = b * scale; // 4. Convert from 26.6 to 16.16 Fixed::from_bits(c.to_bits() << 10) } else { @@ -591,7 +593,7 @@ mod tests { #[test] fn unscaled_scaling_sink_produces_integers() { let nothing = &mut (); - let sink = ScalingSink26Dot6::new(nothing, Fixed::ONE); + let sink = ScalingSink26Dot6::new(nothing, None); for coord in [50.0, 50.1, 50.125, 50.5, 50.9] { assert_eq!(sink.scale(Fixed::from_f64(coord)).to_f32(), 50.0); } @@ -604,7 +606,7 @@ mod tests { // match FreeType scaling with intermediate conversion to 26.6 let scale = Fixed::from_bits((ppem * 64.) as i32) / Fixed::from_bits(upem as i32); let nothing = &mut (); - let sink = ScalingSink26Dot6::new(nothing, scale); + let sink = ScalingSink26Dot6::new(nothing, Some(scale)); let inputs = [ // input coord, expected scaled output (0.0, 0.0), @@ -707,6 +709,36 @@ mod tests { assert!(svg.to_string().ends_with('Z')); } + /// Ensure we don't reject an empty Private DICT + #[test] + fn empty_private_dict() { + let font = FontRef::new(font_test_data::MATERIAL_ICONS_SUBSET).unwrap(); + let common = OutlinesCommon::new(&font).unwrap(); + let outlines = super::Outlines::new(&common).unwrap(); + assert!(outlines.top_dict.private_dict_range.is_empty()); + assert!(outlines.private_dict_range(0).unwrap().is_empty()); + } + + /// Actually apply a scale when the computed scale factor is + /// equal to Fixed::ONE. + /// + /// Specifically, when upem = 512 and ppem = 8, this results in + /// a scale factor of 65536 which was being interpreted as an + /// unscaled draw request. + #[test] + fn proper_scaling_when_factor_equals_fixed_one() { + let font = FontRef::new(font_test_data::MATERIAL_ICONS_SUBSET).unwrap(); + assert_eq!(font.head().unwrap().units_per_em(), 512); + let glyphs = font.outline_glyphs(); + let glyph = glyphs.get(GlyphId::new(1)).unwrap(); + let mut svg = SvgPen::with_precision(6); + glyph + .draw((Size::new(8.0), LocationRef::default()), &mut svg) + .unwrap(); + // This was initially producing unscaled values like M405.000... + assert!(svg.starts_with("M6.328125,7.000000 L1.671875,7.000000")); + } + /// For the given font data and extracted outlines, parse the extracted /// outline data into a set of expected values and compare these with the /// results generated by the scaler.