[write-fonts] Use short offsets in gvar where appropriate

read-fonts/src/tables/gvar.rs

@@ -38,7 +38,8 @@ impl FontReadWithArgs<'_> for U16Or32 {
 }
 
 impl U16Or32 {
-    fn get(self) -> u32 {
+    #[inline]
+    pub fn get(self) -> u32 {
         self.0
     }
 }

write-fonts/src/tables/gvar.rs

@@ -123,8 +123,16 @@ impl Gvar {
     }
 
     fn compute_flags(&self) -> GvarFlags {
-        //TODO: use short offsets sometimes
-        GvarFlags::LONG_OFFSETS
+        let max_offset = self
+            .glyph_variation_data_offsets
+            .iter()
+            .fold(0, |acc, val| acc + val.length + val.length % 2);
+
+        if max_offset / 2 <= (u16::MAX as u32) {
+            GvarFlags::default()
+        } else {
+            GvarFlags::LONG_OFFSETS
+        }
     }
 
     fn compute_glyph_count(&self) -> u16 {
@@ -254,11 +262,15 @@ impl GlyphVariations {
             .map(|tup| tup.build(shared_tuple_map, shared_points.as_ref()))
             .unzip();
 
-        GlyphVariationData {
+        let mut temp = GlyphVariationData {
             tuple_variation_headers: tuple_headers,
             shared_point_numbers: shared_points,
             per_tuple_data: tuple_data,
-        }
+            length: 0,
+        };
+
+        temp.length = temp.compute_size();
+        temp
     }
 }
 
@@ -417,6 +429,11 @@ pub struct GlyphVariationData {
     // optional; present if multiple variations have the same point numbers
     shared_point_numbers: Option<PackedPointNumbers>,
     per_tuple_data: Vec<GlyphTupleVariationData>,
+    /// calculated length required to store this data
+    ///
+    /// we compute this once up front because we need to know it in a bunch
+    /// of different places (u32 because offsets are max u32)
+    length: u32,
 }
 
 /// The serializable representation of a single glyph tuple variation data
@@ -457,19 +474,37 @@ struct GlyphDataWriter<'a> {
 
 impl FontWrite for GlyphDataWriter<'_> {
     fn write_into(&self, writer: &mut TableWriter) {
-        assert!(self.long_offsets, "short offset logic not implemented");
-        let mut last = 0u32;
-        last.write_into(writer);
+        if self.long_offsets {
+            let mut last = 0u32;
+            last.write_into(writer);
 
-        // write all the offsets
-        for glyph in self.data {
-            last += glyph.compute_size();
+            // write all the offsets
+            for glyph in self.data {
+                last += glyph.compute_size();
+                last.write_into(writer);
+            }
+        } else {
+            // for short offsets we divide the real offset by two; this means
+            // we will have to add padding if necessary
+            let mut last = 0u16;
             last.write_into(writer);
+
+            // write all the offsets
+            for glyph in self.data {
+                let size = glyph.compute_size();
+                // ensure we're always rounding up to the next 2
+                let short_size = (size / 2) + size % 2;
+                last += short_size as u16;
+                last.write_into(writer);
+            }
         }
         // then write the actual data
         for glyph in self.data {
             if !glyph.is_empty() {
                 glyph.write_into(writer);
+                if !self.long_offsets {
+                    writer.pad_to_2byte_aligned();
+                }
             }
         }
     }
@@ -934,4 +969,107 @@ mod tests {
             Some(PackedPointNumbers::Some(vec![1, 3]))
         );
     }
+
+    // when using short offsets we store (real offset / 2), so all offsets must
+    // be even, which means when we have an odd number of bytes we have to pad.
+    fn make_31_bytes_of_variation_data() -> Vec<GlyphDeltas> {
+        vec![
+            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::required(1, 0),
+                ],
+                None,
+            ),
+        ]
+    }
+
+    // sanity checking my input data for two subsequent tests
+    #[test]
+    fn who_tests_the_testers() {
+        let variations = GlyphVariations::new(GlyphId::NOTDEF, make_31_bytes_of_variation_data());
+        let mut tupl_map = HashMap::new();
+
+        // without shared tuples this would be 39 bytes
+        assert_eq!(variations.clone().build(&tupl_map).length, 39);
+
+        // to get our real size we need to mock up the shared tuples:
+        tupl_map.insert(&variations.variations[0].peak_tuple, 1);
+        tupl_map.insert(&variations.variations[1].peak_tuple, 2);
+
+        let built = variations.clone().build(&tupl_map);
+        // we need an odd number to test impact of padding
+        assert_eq!(built.length, 31);
+    }
+
+    fn assert_test_offset_packing(n_glyphs: u16, should_be_short: bool) {
+        let (offset_len, data_len, expected_flags) = if should_be_short {
+            // when using short offset we need to pad data to ensure offset is even
+            (u16::RAW_BYTE_LEN, 32, GvarFlags::empty())
+        } else {
+            (u32::RAW_BYTE_LEN, 31, GvarFlags::LONG_OFFSETS)
+        };
+
+        let test_data = make_31_bytes_of_variation_data();
+        let a_small_number_of_variations = (0..n_glyphs)
+            .map(|i| GlyphVariations::new(GlyphId::new(i), test_data.clone()))
+            .collect();
+
+        let gvar = Gvar::new(a_small_number_of_variations).unwrap();
+        assert_eq!(gvar.compute_flags(), expected_flags);
+
+        let writer = gvar.compile_variation_data();
+        let mut sink = TableWriter::default();
+        writer.write_into(&mut sink);
+
+        let bytes = sink.into_data().bytes;
+        let expected_len = (n_glyphs + 1) as usize * offset_len // offsets
+                             + data_len * n_glyphs as usize; // rounded size of each glyph
+        assert_eq!(bytes.len(), expected_len);
+
+        let dumped = crate::dump_table(&gvar).unwrap();
+        let loaded = read_fonts::tables::gvar::Gvar::read(FontData::new(&dumped)).unwrap();
+
+        assert_eq!(loaded.glyph_count(), n_glyphs);
+        assert_eq!(loaded.flags(), expected_flags);
+        assert!(loaded
+            .glyph_variation_data_offsets()
+            .iter()
+            .map(|off| off.unwrap().get())
+            .enumerate()
+            .all(|(i, off)| off as usize == i * data_len));
+    }
+
+    #[test]
+    fn prefer_short_offsets() {
+        let _ = env_logger::builder().is_test(true).try_init();
+        assert_test_offset_packing(5, true);
+    }
+
+    #[test]
+    fn use_long_offsets_when_necessary() {
+        // 2**16 * 2 / (31 + 1 padding) (bytes per tuple) = 4096 should be the first
+        // overflow
+        let _ = env_logger::builder().is_test(true).try_init();
+        assert_test_offset_packing(4095, true);
+        assert_test_offset_packing(4096, false);
+        assert_test_offset_packing(4097, false);
+    }
 }