From 320b90f9126bdc0ea07010724a4208e8db37c898 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Fri, 22 Sep 2023 15:12:58 -0400 Subject: [PATCH 1/4] [write-fonts] Use shared points in GlyphVariations This is a basic initial implementation, that needs a bit of fixing up; in particular we should look into whether or not it is worth sharing the 'all' value (the zero byte) which would only be saving a byte per tuple, and would only make sense if it is common for fonts to often have lots of tuples that use all points? but hey, a byte is a byte. Also I have an idea for a fancy but slow algorithm to compute more efficient share points, and that could be fun to look into. --- write-fonts/src/tables/gvar.rs | 176 ++++++++++++++++++++++++++------- 1 file changed, 140 insertions(+), 36 deletions(-) diff --git a/write-fonts/src/tables/gvar.rs b/write-fonts/src/tables/gvar.rs index d86328520..c2d0693cf 100644 --- a/write-fonts/src/tables/gvar.rs +++ b/write-fonts/src/tables/gvar.rs @@ -27,6 +27,7 @@ pub struct GlyphDeltas { intermediate_region: Option<(Tuple, Tuple)>, // (x, y) deltas or None for do not encode. One entry per point in the glyph. deltas: Vec, + point_numbers: PackedPointNumbers, } /// A delta for a single value in a glyph. @@ -197,19 +198,69 @@ impl GlyphVariations { } } + /// Determine if we should use 'shared point numbers' + /// + /// If multiple tuple variations for a given glyph use the same point numbers, + /// it is possible to store this in the glyph table, avoiding duplicating + /// data. + /// + /// This implementation is currently based on the one in fonttools, where it + /// is part of the compileTupleVariationStore method: + /// + /// + /// # Note + /// + /// There is likely room for some optimization here, depending on the + /// structure of the point numbers. If it is common for point numbers to only + /// vary by an item or two, it may be worth picking a set of shared points + /// that is a subset of multiple different tuples; this would mean you could + /// make some tuples include deltas that they might otherwise omit, but let + /// them omit their explicit point numbers. + /// + /// For fonts with a large number of variations, this could produce reasonable + /// savings, at the cost of a significantly more complicated algorithm. + fn compute_shared_points(&self) -> Option { + let mut point_number_counts = HashMap::new(); + // count how often each set of numbers occurs + for deltas in &self.variations { + // FIXME: should we bother sharing if 'all'? it saves.. one byte per + // region? + if deltas.point_numbers == PackedPointNumbers::All { + continue; + } + // for each set points, get compiled size + number of occurances + let (_, count) = point_number_counts + .entry(&deltas.point_numbers) + .or_insert_with(|| { + let size = deltas.point_numbers.compute_size(); + (size as usize, 0usize) + }); + *count += 1; + } + + // find the one that saves the most bytes + let (pts, (size, count)) = point_number_counts + .into_iter() + .filter(|(_, (_, count))| *count > 1) + .max_by_key(|(_, (size, count))| *count * *size)?; + log::trace!("max shared pts ({size}B, {count}n), {pts:?}"); + + // no use sharing points if they only occur once + (count > 1).then(|| pts.to_owned()) + } + fn build(self, shared_tuple_map: &HashMap<&Tuple, u16>) -> GlyphVariationData { - //FIXME: for now we are not doing fancy efficient point encodings, - //and all tuples contain all points (and so all are stored) - let shared_points = PackedPointNumbers::All; + let shared_points = self.compute_shared_points(); + let (tuple_headers, tuple_data): (Vec<_>, Vec<_>) = self .variations .into_iter() - .map(|tup| tup.build(shared_tuple_map, &shared_points)) + .map(|tup| tup.build(shared_tuple_map, shared_points.as_ref())) .unzip(); GlyphVariationData { tuple_variation_headers: tuple_headers, - shared_point_numbers: Some(shared_points), + shared_point_numbers: shared_points, per_tuple_data: tuple_data, } } @@ -248,44 +299,51 @@ impl GlyphDeltas { "all tuples must have equal length" ); } + let point_numbers = if deltas.iter().all(|d| d.required) { + PackedPointNumbers::All + } else { + PackedPointNumbers::Some( + deltas + .iter() + .enumerate() + .filter_map(|(i, d)| d.required.then_some(i as u16)) + .collect(), + ) + }; GlyphDeltas { peak_tuple, intermediate_region, deltas, + point_numbers, } } fn build( self, shared_tuple_map: &HashMap<&Tuple, u16>, - _shared_points: &PackedPointNumbers, + shared_points: Option<&PackedPointNumbers>, ) -> (TupleVariationHeader, GlyphTupleVariationData) { // build variation data for all points, even if they could be interp'd fn build_non_sparse(deltas: &[GlyphDelta]) -> GlyphTupleVariationData { let (x_deltas, y_deltas) = deltas.iter().map(|delta| (delta.x, delta.y)).unzip(); GlyphTupleVariationData { - private_point_numbers: None, + private_point_numbers: Some(PackedPointNumbers::All), x_deltas: PackedDeltas::new(x_deltas), y_deltas: PackedDeltas::new(y_deltas), } } // build sparse variation data, omitting interpolatable values - fn build_sparse(deltas: &[GlyphDelta]) -> GlyphTupleVariationData { - let mut x_deltas = Vec::with_capacity(deltas.len()); - let mut y_deltas = Vec::with_capacity(deltas.len()); - let mut points = Vec::with_capacity(deltas.len()); - for (i, (x, y)) in deltas + fn build_sparse( + deltas: &[GlyphDelta], + private_point_numbers: Option, + ) -> GlyphTupleVariationData { + let (x_deltas, y_deltas) = deltas .iter() - .enumerate() - .filter_map(|(i, delta)| delta.required.then_some((i, (delta.x, delta.y)))) - { - x_deltas.push(x); - y_deltas.push(y); - points.push(i as u16); - } + .filter_map(|delta| delta.required.then_some((delta.x, delta.y))) + .unzip(); GlyphTupleVariationData { - private_point_numbers: Some(PackedPointNumbers::Some(points)), + private_point_numbers, x_deltas: PackedDeltas::new(x_deltas), y_deltas: PackedDeltas::new(y_deltas), } @@ -295,6 +353,7 @@ impl GlyphDeltas { peak_tuple, intermediate_region, deltas, + point_numbers, } = self; let (idx, peak_tuple) = match shared_tuple_map.get(&peak_tuple) { @@ -302,25 +361,33 @@ impl GlyphDeltas { None => (None, Some(peak_tuple)), }; + let use_shared_points = Some(&point_numbers) == shared_points; + let has_sparse_points = point_numbers != PackedPointNumbers::All; + + // - if we use shared points, we don't bother building the dense repr + // - if we use all points, there's no point in building sparse + let dense_data = (!use_shared_points).then(|| build_non_sparse(&deltas)); + let sparse_data = has_sparse_points.then(|| { + let private_point_numbers = (!use_shared_points).then_some(point_numbers); + build_sparse(&deltas, private_point_numbers) + }); + // just because some points may be interpolatable does not mean that the // sparse representation is more efficient, since it requires us to // also explicitly include point numbers; so we try both packings and // pick the best. - - let dense_data = build_non_sparse(&deltas); - let dense_size = dense_data.compute_size(); - let has_interpolatable_points = deltas.iter().any(|d| !d.required); - let (data, data_size, has_private_points) = if has_interpolatable_points { - let sparse_data = build_sparse(&deltas); - let sparse_size = sparse_data.compute_size(); - log::trace!("gvar tuple variation size (dense, sparse): ({dense_size}, {sparse_size})"); - if sparse_size < dense_size { - (sparse_data, sparse_size, true) - } else { - (dense_data, dense_size, false) + let (data_size, data) = match (dense_data, sparse_data) { + (Some(data), None) | (None, Some(data)) => (data.compute_size(), data), + (Some(dense), Some(sparse)) => { + let dense_size = dense.compute_size(); + let sparse_size = sparse.compute_size(); + if sparse_size < dense_size { + (sparse_size, sparse) + } else { + (dense_size, dense) + } } - } else { - (dense_data, dense_size, false) + (None, None) => unreachable!("we always build at least one"), }; let header = TupleVariationHeader::new( @@ -328,7 +395,7 @@ impl GlyphDeltas { idx, peak_tuple, intermediate_region, - has_private_points, + !use_shared_points, ); (header, data) @@ -524,7 +591,8 @@ mod tests { use super::*; #[test] - fn smoke_test() { + fn gvar_smoke_test() { + let _ = env_logger::builder().is_test(true).try_init(); let table = Gvar::new(vec![ GlyphVariations::new(GlyphId::new(0), vec![]), GlyphVariations::new( @@ -688,4 +756,40 @@ mod tests { .collect(); assert_eq!(points, vec![(1, 2), (3, 4), (5, 6), (5, 6), (7, 8)]); } + + #[test] + fn share_points() { + let variations = GlyphVariations::new( + GlyphId::new(0), + vec![ + GlyphDeltas::new( + Tuple::new(vec![F2Dot14::from_f32(1.0), F2Dot14::from_f32(1.0)]), + vec![ + GlyphDelta::required(1, 2), + GlyphDelta::required(3, 4), + GlyphDelta::required(5, 6), + GlyphDelta::optional(5, 6), + GlyphDelta::required(7, 8), + ], + None, + ), + GlyphDeltas::new( + Tuple::new(vec![F2Dot14::from_f32(-1.0), F2Dot14::from_f32(-1.0)]), + vec![ + GlyphDelta::required(10, 20), + GlyphDelta::required(30, 40), + GlyphDelta::required(50, 60), + GlyphDelta::optional(50, 60), + GlyphDelta::required(70, 80), + ], + None, + ), + ], + ); + + assert_eq!( + variations.compute_shared_points(), + Some(PackedPointNumbers::Some(vec![0, 1, 2, 4])) + ) + } } From 709abb1890744f0e48e3db8ef4199e47b9e14ed0 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Fri, 22 Sep 2023 16:11:50 -0400 Subject: [PATCH 2/4] [write-fonts] Allow 'all' packedpoints to be shared --- write-fonts/src/tables/gvar.rs | 51 ++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/write-fonts/src/tables/gvar.rs b/write-fonts/src/tables/gvar.rs index c2d0693cf..b1a9ce65d 100644 --- a/write-fonts/src/tables/gvar.rs +++ b/write-fonts/src/tables/gvar.rs @@ -223,11 +223,6 @@ impl GlyphVariations { let mut point_number_counts = HashMap::new(); // count how often each set of numbers occurs for deltas in &self.variations { - // FIXME: should we bother sharing if 'all'? it saves.. one byte per - // region? - if deltas.point_numbers == PackedPointNumbers::All { - continue; - } // for each set points, get compiled size + number of occurances let (_, count) = point_number_counts .entry(&deltas.point_numbers) @@ -324,10 +319,10 @@ impl GlyphDeltas { shared_points: Option<&PackedPointNumbers>, ) -> (TupleVariationHeader, GlyphTupleVariationData) { // build variation data for all points, even if they could be interp'd - fn build_non_sparse(deltas: &[GlyphDelta]) -> GlyphTupleVariationData { + fn build_non_sparse(deltas: &[GlyphDelta], shared_points: bool) -> GlyphTupleVariationData { let (x_deltas, y_deltas) = deltas.iter().map(|delta| (delta.x, delta.y)).unzip(); GlyphTupleVariationData { - private_point_numbers: Some(PackedPointNumbers::All), + private_point_numbers: (!shared_points).then_some(PackedPointNumbers::All), x_deltas: PackedDeltas::new(x_deltas), y_deltas: PackedDeltas::new(y_deltas), } @@ -362,12 +357,13 @@ impl GlyphDeltas { }; let use_shared_points = Some(&point_numbers) == shared_points; - let has_sparse_points = point_numbers != PackedPointNumbers::All; + // only build sparse if we actually have sparse points + let try_sparse = point_numbers != PackedPointNumbers::All; + // only build dense if no shared points, unless they are non-sparse + let try_dense = !use_shared_points || !try_sparse; - // - if we use shared points, we don't bother building the dense repr - // - if we use all points, there's no point in building sparse - let dense_data = (!use_shared_points).then(|| build_non_sparse(&deltas)); - let sparse_data = has_sparse_points.then(|| { + let dense_data = try_dense.then(|| build_non_sparse(&deltas, use_shared_points)); + let sparse_data = try_sparse.then(|| { let private_point_numbers = (!use_shared_points).then_some(point_numbers); build_sparse(&deltas, private_point_numbers) }); @@ -792,4 +788,35 @@ mod tests { Some(PackedPointNumbers::Some(vec![0, 1, 2, 4])) ) } + + #[test] + fn share_all_points() { + let variations = GlyphVariations::new( + GlyphId::new(0), + vec![ + GlyphDeltas::new( + Tuple::new(vec![F2Dot14::from_f32(1.0), F2Dot14::from_f32(1.0)]), + vec![ + GlyphDelta::required(1, 2), + GlyphDelta::required(3, 4), + GlyphDelta::required(5, 6), + ], + None, + ), + GlyphDeltas::new( + Tuple::new(vec![F2Dot14::from_f32(-1.0), F2Dot14::from_f32(-1.0)]), + vec![ + GlyphDelta::required(2, 4), + GlyphDelta::required(6, 8), + GlyphDelta::required(7, 9), + ], + None, + ), + ], + ); + + let shared_tups = HashMap::new(); + let built = variations.build(&shared_tups); + assert_eq!(built.shared_point_numbers, Some(PackedPointNumbers::All)) + } } From 754d31d9e81b318a5d7895064293dd0992fd3a54 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Tue, 26 Sep 2023 16:03:58 -0400 Subject: [PATCH 3/4] [write-fonts] Slight rework of shared point picking Our first impl had a little wart, where if a given tuple had any points which would be omitted, we would only consider the sparse representation when choosing a set of packed points to share, which was suboptimal. The solution involves fully building and measuring the tuple at construction time, and deciding which repr is best then. --- write-fonts/src/tables/gvar.rs | 247 +++++++++++++++++++++++---------- 1 file changed, 171 insertions(+), 76 deletions(-) diff --git a/write-fonts/src/tables/gvar.rs b/write-fonts/src/tables/gvar.rs index b1a9ce65d..1297c2473 100644 --- a/write-fonts/src/tables/gvar.rs +++ b/write-fonts/src/tables/gvar.rs @@ -27,7 +27,7 @@ pub struct GlyphDeltas { intermediate_region: Option<(Tuple, Tuple)>, // (x, y) deltas or None for do not encode. One entry per point in the glyph. deltas: Vec, - point_numbers: PackedPointNumbers, + best_point_packing: PackedPointNumbers, } /// A delta for a single value in a glyph. @@ -225,20 +225,19 @@ impl GlyphVariations { for deltas in &self.variations { // for each set points, get compiled size + number of occurances let (_, count) = point_number_counts - .entry(&deltas.point_numbers) + .entry(&deltas.best_point_packing) .or_insert_with(|| { - let size = deltas.point_numbers.compute_size(); + let size = deltas.best_point_packing.compute_size(); (size as usize, 0usize) }); *count += 1; } // find the one that saves the most bytes - let (pts, (size, count)) = point_number_counts + let (pts, (_, count)) = point_number_counts .into_iter() .filter(|(_, (_, count))| *count > 1) .max_by_key(|(_, (size, count))| *count * *size)?; - log::trace!("max shared pts ({size}B, {count}n), {pts:?}"); // no use sharing points if they only occur once (count > 1).then(|| pts.to_owned()) @@ -276,6 +275,10 @@ impl GlyphDelta { pub fn optional(x: i16, y: i16) -> Self { Self::new(x, y, false) } + + fn to_tuple(self) -> (i16, i16) { + (self.x, self.y) + } } impl GlyphDeltas { @@ -294,61 +297,80 @@ impl GlyphDeltas { "all tuples must have equal length" ); } - let point_numbers = if deltas.iter().all(|d| d.required) { - PackedPointNumbers::All - } else { - PackedPointNumbers::Some( - deltas - .iter() - .enumerate() - .filter_map(|(i, d)| d.required.then_some(i as u16)) - .collect(), - ) - }; + // at construction time we build both iup optimized & not versions + // of ourselves, to determine what representation is most efficient; + // the caller will look at the generated packed points to decide which + // set should be shared. + let best_point_packing = Self::pick_best_point_number_repr(&deltas); GlyphDeltas { peak_tuple, intermediate_region, deltas, - point_numbers, + best_point_packing, } } + // this is a type method just to expose it for testing, we call it before + // we finish instantiating self. + // + // NOTE: we do a lot of duplicate work here with creating & throwing away + // buffers, and that can be improved at the cost of a bit more complexity + fn pick_best_point_number_repr(deltas: &[GlyphDelta]) -> PackedPointNumbers { + if deltas.iter().all(|d| d.required) { + return PackedPointNumbers::All; + } + + let dense = Self::build_non_sparse_data(deltas); + let sparse = Self::build_sparse_data(deltas); + let dense_size = dense.compute_size(); + let sparse_size = sparse.compute_size(); + log::trace!("dense {dense_size}, sparse {sparse_size}"); + if sparse_size < dense_size { + sparse.private_point_numbers.unwrap() + } else { + PackedPointNumbers::All + } + } + + fn build_non_sparse_data(deltas: &[GlyphDelta]) -> GlyphTupleVariationData { + let (x_deltas, y_deltas) = deltas.iter().map(|delta| (delta.x, delta.y)).unzip(); + GlyphTupleVariationData { + private_point_numbers: Some(PackedPointNumbers::All), + x_deltas: PackedDeltas::new(x_deltas), + y_deltas: PackedDeltas::new(y_deltas), + } + } + + fn build_sparse_data(deltas: &[GlyphDelta]) -> GlyphTupleVariationData { + let (x_deltas, y_deltas) = deltas + .iter() + .filter_map(|delta| delta.required.then_some((delta.x, delta.y))) + .unzip(); + let point_numbers = deltas + .iter() + .enumerate() + .filter_map(|(i, delta)| delta.required.then_some(i as u16)) + .collect(); + GlyphTupleVariationData { + private_point_numbers: Some(PackedPointNumbers::Some(point_numbers)), + x_deltas: PackedDeltas::new(x_deltas), + y_deltas: PackedDeltas::new(y_deltas), + } + } + + // shared points is just "whatever points, if any, are shared." We are + // responsible for seeing if these are actually our points, in which case + // we are using shared points. fn build( self, shared_tuple_map: &HashMap<&Tuple, u16>, shared_points: Option<&PackedPointNumbers>, ) -> (TupleVariationHeader, GlyphTupleVariationData) { - // build variation data for all points, even if they could be interp'd - fn build_non_sparse(deltas: &[GlyphDelta], shared_points: bool) -> GlyphTupleVariationData { - let (x_deltas, y_deltas) = deltas.iter().map(|delta| (delta.x, delta.y)).unzip(); - GlyphTupleVariationData { - private_point_numbers: (!shared_points).then_some(PackedPointNumbers::All), - x_deltas: PackedDeltas::new(x_deltas), - y_deltas: PackedDeltas::new(y_deltas), - } - } - - // build sparse variation data, omitting interpolatable values - fn build_sparse( - deltas: &[GlyphDelta], - private_point_numbers: Option, - ) -> GlyphTupleVariationData { - let (x_deltas, y_deltas) = deltas - .iter() - .filter_map(|delta| delta.required.then_some((delta.x, delta.y))) - .unzip(); - GlyphTupleVariationData { - private_point_numbers, - x_deltas: PackedDeltas::new(x_deltas), - y_deltas: PackedDeltas::new(y_deltas), - } - } - let GlyphDeltas { peak_tuple, intermediate_region, deltas, - point_numbers, + best_point_packing: point_numbers, } = self; let (idx, peak_tuple) = match shared_tuple_map.get(&peak_tuple) { @@ -356,42 +378,28 @@ impl GlyphDeltas { None => (None, Some(peak_tuple)), }; - let use_shared_points = Some(&point_numbers) == shared_points; - // only build sparse if we actually have sparse points - let try_sparse = point_numbers != PackedPointNumbers::All; - // only build dense if no shared points, unless they are non-sparse - let try_dense = !use_shared_points || !try_sparse; - - let dense_data = try_dense.then(|| build_non_sparse(&deltas, use_shared_points)); - let sparse_data = try_sparse.then(|| { - let private_point_numbers = (!use_shared_points).then_some(point_numbers); - build_sparse(&deltas, private_point_numbers) - }); - - // just because some points may be interpolatable does not mean that the - // sparse representation is more efficient, since it requires us to - // also explicitly include point numbers; so we try both packings and - // pick the best. - let (data_size, data) = match (dense_data, sparse_data) { - (Some(data), None) | (None, Some(data)) => (data.compute_size(), data), - (Some(dense), Some(sparse)) => { - let dense_size = dense.compute_size(); - let sparse_size = sparse.compute_size(); - if sparse_size < dense_size { - (sparse_size, sparse) - } else { - (dense_size, dense) - } - } - (None, None) => unreachable!("we always build at least one"), + let has_private_points = Some(&point_numbers) != shared_points; + let (x_deltas, y_deltas) = match &point_numbers { + PackedPointNumbers::All => deltas.iter().map(|d| (d.x, d.y)).unzip(), + PackedPointNumbers::Some(pts) => pts + .iter() + .map(|idx| deltas[*idx as usize].to_tuple()) + .unzip(), }; + let data = GlyphTupleVariationData { + private_point_numbers: has_private_points.then_some(point_numbers), + x_deltas: PackedDeltas::new(x_deltas), + y_deltas: PackedDeltas::new(y_deltas), + }; + let data_size = data.compute_size(); + let header = TupleVariationHeader::new( data_size, idx, peak_tuple, intermediate_region, - !use_shared_points, + has_private_points, ); (header, data) @@ -755,6 +763,7 @@ mod tests { #[test] fn share_points() { + let _ = env_logger::builder().is_test(true).try_init(); let variations = GlyphVariations::new( GlyphId::new(0), vec![ @@ -762,10 +771,11 @@ mod tests { Tuple::new(vec![F2Dot14::from_f32(1.0), F2Dot14::from_f32(1.0)]), vec![ GlyphDelta::required(1, 2), - GlyphDelta::required(3, 4), + GlyphDelta::optional(3, 4), GlyphDelta::required(5, 6), GlyphDelta::optional(5, 6), GlyphDelta::required(7, 8), + GlyphDelta::optional(7, 8), ], None, ), @@ -773,10 +783,11 @@ mod tests { Tuple::new(vec![F2Dot14::from_f32(-1.0), F2Dot14::from_f32(-1.0)]), vec![ GlyphDelta::required(10, 20), - GlyphDelta::required(30, 40), + GlyphDelta::optional(30, 40), GlyphDelta::required(50, 60), GlyphDelta::optional(50, 60), GlyphDelta::required(70, 80), + GlyphDelta::optional(70, 80), ], None, ), @@ -785,7 +796,7 @@ mod tests { assert_eq!( variations.compute_shared_points(), - Some(PackedPointNumbers::Some(vec![0, 1, 2, 4])) + Some(PackedPointNumbers::Some(vec![0, 2, 4])) ) } @@ -819,4 +830,88 @@ mod tests { let built = variations.build(&shared_tups); assert_eq!(built.shared_point_numbers, Some(PackedPointNumbers::All)) } + + // comparing our behaviour against what we know fonttools does. + #[test] + #[allow(non_snake_case)] + fn oswald_Lcaron() { + let _ = env_logger::builder().is_test(true).try_init(); + let d1 = GlyphDeltas::new( + Tuple::new(vec![F2Dot14::from_f32(-1.0), F2Dot14::from_f32(-1.0)]), + vec![ + GlyphDelta::optional(0, 0), + GlyphDelta::required(35, 0), + GlyphDelta::optional(0, 0), + GlyphDelta::required(-24, 0), + GlyphDelta::optional(0, 0), + GlyphDelta::optional(0, 0), + ], + None, + ); + + let d1_sparse = GlyphDeltas::build_sparse_data(&d1.deltas); + + assert_eq!( + d1_sparse + .private_point_numbers + .clone() + .unwrap() + .compute_size(), + 4 + ); + assert_eq!(d1_sparse.x_deltas.compute_size(), 3); + assert_eq!(d1_sparse.y_deltas.compute_size(), 1); + + let d1_dense = GlyphDeltas::build_non_sparse_data(&d1.deltas); + + assert_eq!(d1_dense.x_deltas.compute_size(), 6); + assert_eq!(d1_dense.y_deltas.compute_size(), 1); + + assert_eq!(d1_sparse.compute_size(), d1_dense.compute_size()); + + let d2 = GlyphDeltas::new( + Tuple::new(vec![F2Dot14::from_f32(1.0), F2Dot14::from_f32(1.0)]), + vec![ + GlyphDelta::optional(0, 0), + GlyphDelta::required(26, 15), + GlyphDelta::optional(0, 0), + GlyphDelta::required(46, 0), + GlyphDelta::optional(0, 0), + GlyphDelta::optional(0, 0), + ], + None, + ); + let d2_sparse = GlyphDeltas::build_sparse_data(&d2.deltas); + + assert_eq!( + d2_sparse + .private_point_numbers + .as_ref() + .unwrap() + .compute_size(), + 4 + ); + assert_eq!(d2_sparse.x_deltas.compute_size(), 3); + assert_eq!(d2_sparse.y_deltas.compute_size(), 3,); + + let d2_dense = GlyphDeltas::build_non_sparse_data(&d2.deltas); + + assert_eq!(d2_dense.x_deltas.compute_size(), 6); + assert_eq!(d2_dense.y_deltas.compute_size(), 4); + + assert!(d2_sparse.compute_size() < d2_dense.compute_size()); + + let tups = HashMap::new(); + let variations = GlyphVariations::new(GlyphId::new(0), vec![d1, d2]); + assert!(variations.compute_shared_points().is_none()); + let built = variations.build(&tups); + assert_eq!( + built.per_tuple_data[0].private_point_numbers, + Some(PackedPointNumbers::All) + ); + assert_eq!( + built.per_tuple_data[1].private_point_numbers, + Some(PackedPointNumbers::Some(vec![1, 3])) + ); + } } From caec769f3126eb6a9e450f0295150c4fb2c46dd6 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Wed, 27 Sep 2023 12:06:12 -0400 Subject: [PATCH 4/4] Address review feedback (PR #628) - fixup how we determine the size savings of shared points - add a test for *not* sharing points when all are unique - cleanup our test of the Lcaron glyph in Oswald - add issue links to notes --- write-fonts/src/tables/gvar.rs | 156 +++++++++++++++++++-------------- 1 file changed, 88 insertions(+), 68 deletions(-) diff --git a/write-fonts/src/tables/gvar.rs b/write-fonts/src/tables/gvar.rs index 1297c2473..b5f544933 100644 --- a/write-fonts/src/tables/gvar.rs +++ b/write-fonts/src/tables/gvar.rs @@ -219,6 +219,8 @@ impl GlyphVariations { /// /// For fonts with a large number of variations, this could produce reasonable /// savings, at the cost of a significantly more complicated algorithm. + /// + /// (issue ) fn compute_shared_points(&self) -> Option { let mut point_number_counts = HashMap::new(); // count how often each set of numbers occurs @@ -234,13 +236,13 @@ impl GlyphVariations { } // find the one that saves the most bytes - let (pts, (_, count)) = point_number_counts + let (pts, _) = point_number_counts .into_iter() + // no use sharing points if they only occur once .filter(|(_, (_, count))| *count > 1) - .max_by_key(|(_, (size, count))| *count * *size)?; + .max_by_key(|(_, (size, count))| (*count - 1) * *size)?; - // no use sharing points if they only occur once - (count > 1).then(|| pts.to_owned()) + Some(pts.to_owned()) } fn build(self, shared_tuple_map: &HashMap<&Tuple, u16>) -> GlyphVariationData { @@ -313,8 +315,9 @@ impl GlyphDeltas { // this is a type method just to expose it for testing, we call it before // we finish instantiating self. // - // NOTE: we do a lot of duplicate work here with creating & throwing away + // we do a lot of duplicate work here with creating & throwing away // buffers, and that can be improved at the cost of a bit more complexity + // fn pick_best_point_number_repr(deltas: &[GlyphDelta]) -> PackedPointNumbers { if deltas.iter().all(|d| d.required) { return PackedPointNumbers::All; @@ -831,79 +834,96 @@ mod tests { assert_eq!(built.shared_point_numbers, Some(PackedPointNumbers::All)) } - // comparing our behaviour against what we know fonttools does. + // three tuples with three different packedpoint representations means + // that we should have no shared points #[test] - #[allow(non_snake_case)] - fn oswald_Lcaron() { - let _ = env_logger::builder().is_test(true).try_init(); - let d1 = GlyphDeltas::new( - Tuple::new(vec![F2Dot14::from_f32(-1.0), F2Dot14::from_f32(-1.0)]), + fn dont_share_unique_points() { + let variations = GlyphVariations::new( + GlyphId::new(0), vec![ - GlyphDelta::optional(0, 0), - GlyphDelta::required(35, 0), - GlyphDelta::optional(0, 0), - GlyphDelta::required(-24, 0), - GlyphDelta::optional(0, 0), - GlyphDelta::optional(0, 0), + GlyphDeltas::new( + Tuple::new(vec![F2Dot14::from_f32(1.0), F2Dot14::from_f32(1.0)]), + vec![ + GlyphDelta::required(1, 2), + GlyphDelta::optional(3, 4), + GlyphDelta::required(5, 6), + GlyphDelta::optional(5, 6), + GlyphDelta::required(7, 8), + GlyphDelta::optional(7, 8), + ], + None, + ), + GlyphDeltas::new( + Tuple::new(vec![F2Dot14::from_f32(-1.0), F2Dot14::from_f32(-1.0)]), + vec![ + GlyphDelta::required(10, 20), + GlyphDelta::required(35, 40), + GlyphDelta::required(50, 60), + GlyphDelta::optional(50, 60), + GlyphDelta::required(70, 80), + GlyphDelta::optional(70, 80), + ], + None, + ), + GlyphDeltas::new( + Tuple::new(vec![F2Dot14::from_f32(0.5), F2Dot14::from_f32(1.0)]), + vec![ + GlyphDelta::required(1, 2), + GlyphDelta::optional(3, 4), + GlyphDelta::required(5, 6), + GlyphDelta::optional(5, 6), + GlyphDelta::optional(7, 8), + GlyphDelta::optional(7, 8), + ], + None, + ), ], - None, - ); - - let d1_sparse = GlyphDeltas::build_sparse_data(&d1.deltas); - - assert_eq!( - d1_sparse - .private_point_numbers - .clone() - .unwrap() - .compute_size(), - 4 ); - assert_eq!(d1_sparse.x_deltas.compute_size(), 3); - assert_eq!(d1_sparse.y_deltas.compute_size(), 1); - - let d1_dense = GlyphDeltas::build_non_sparse_data(&d1.deltas); - - assert_eq!(d1_dense.x_deltas.compute_size(), 6); - assert_eq!(d1_dense.y_deltas.compute_size(), 1); - assert_eq!(d1_sparse.compute_size(), d1_dense.compute_size()); + let shared_tups = HashMap::new(); + let built = variations.build(&shared_tups); + assert!(built.shared_point_numbers.is_none()); + } - let d2 = GlyphDeltas::new( - Tuple::new(vec![F2Dot14::from_f32(1.0), F2Dot14::from_f32(1.0)]), + // comparing our behaviour against what we know fonttools does. + #[test] + #[allow(non_snake_case)] + fn oswald_Lcaron() { + let _ = env_logger::builder().is_test(true).try_init(); + // in this glyph, it is more efficient to encode all points for the first + // tuple, but sparse points for the second (the single y delta in the + // second tuple means you can't encode the y-deltas as 'all zero') + let variations = GlyphVariations::new( + GlyphId::new(0), vec![ - GlyphDelta::optional(0, 0), - GlyphDelta::required(26, 15), - GlyphDelta::optional(0, 0), - GlyphDelta::required(46, 0), - GlyphDelta::optional(0, 0), - GlyphDelta::optional(0, 0), + GlyphDeltas::new( + Tuple::new(vec![F2Dot14::from_f32(-1.0), F2Dot14::from_f32(-1.0)]), + vec![ + GlyphDelta::optional(0, 0), + GlyphDelta::required(35, 0), + GlyphDelta::optional(0, 0), + GlyphDelta::required(-24, 0), + GlyphDelta::optional(0, 0), + GlyphDelta::optional(0, 0), + ], + None, + ), + GlyphDeltas::new( + Tuple::new(vec![F2Dot14::from_f32(1.0), F2Dot14::from_f32(1.0)]), + vec![ + GlyphDelta::optional(0, 0), + GlyphDelta::required(26, 15), + GlyphDelta::optional(0, 0), + GlyphDelta::required(46, 0), + GlyphDelta::optional(0, 0), + GlyphDelta::optional(0, 0), + ], + None, + ), ], - None, ); - let d2_sparse = GlyphDeltas::build_sparse_data(&d2.deltas); - - assert_eq!( - d2_sparse - .private_point_numbers - .as_ref() - .unwrap() - .compute_size(), - 4 - ); - assert_eq!(d2_sparse.x_deltas.compute_size(), 3); - assert_eq!(d2_sparse.y_deltas.compute_size(), 3,); - - let d2_dense = GlyphDeltas::build_non_sparse_data(&d2.deltas); - - assert_eq!(d2_dense.x_deltas.compute_size(), 6); - assert_eq!(d2_dense.y_deltas.compute_size(), 4); - - assert!(d2_sparse.compute_size() < d2_dense.compute_size()); - - let tups = HashMap::new(); - let variations = GlyphVariations::new(GlyphId::new(0), vec![d1, d2]); assert!(variations.compute_shared_points().is_none()); + let tups = HashMap::new(); let built = variations.build(&tups); assert_eq!( built.per_tuple_data[0].private_point_numbers,