[read-fonts] hdmx: use explicitly defined record size

read-fonts/generated/generated_hdmx.rs

@@ -50,9 +50,12 @@ impl<'a> FontReadWithArgs<'a> for Hdmx<'a> {
         let mut cursor = data.cursor();
         cursor.advance::<u16>();
         let num_records: u16 = cursor.read()?;
-        cursor.advance::<u32>();
+        let size_device_record: u32 = cursor.read()?;
         let records_byte_len = (num_records as usize)
-            .checked_mul(<DeviceRecord as ComputeSize>::compute_size(&num_glyphs)?)
+            .checked_mul(<DeviceRecord as ComputeSize>::compute_size(&(
+                num_glyphs,
+                size_device_record,
+            ))?)
             .ok_or(ReadError::OutOfBounds)?;
         cursor.advance_by(records_byte_len);
         cursor.finish(HdmxMarker {
@@ -99,7 +102,9 @@ impl<'a> Hdmx<'a> {
     /// Array of device records.
     pub fn records(&self) -> ComputedArray<'a, DeviceRecord<'a>> {
         let range = self.shape.records_byte_range();
-        self.data.read_with_args(range, &self.num_glyphs()).unwrap()
+        self.data
+            .read_with_args(range, &(self.num_glyphs(), self.size_device_record()))
+            .unwrap()
     }
 
     pub(crate) fn num_glyphs(&self) -> u16 {
@@ -137,96 +142,3 @@ impl<'a> std::fmt::Debug for Hdmx<'a> {
         (self as &dyn SomeTable<'a>).fmt(f)
     }
 }
-
-#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
-pub struct DeviceRecord<'a> {
-    /// Pixel size for following widths (as ppem).
-    pub pixel_size: u8,
-    /// Maximum width.
-    pub max_width: u8,
-    /// Array of glyphs (numgGlyphs is from the 'maxp' table).
-    pub widths: &'a [u8],
-}
-
-impl<'a> DeviceRecord<'a> {
-    /// Pixel size for following widths (as ppem).
-    pub fn pixel_size(&self) -> u8 {
-        self.pixel_size
-    }
-
-    /// Maximum width.
-    pub fn max_width(&self) -> u8 {
-        self.max_width
-    }
-
-    /// Array of glyphs (numgGlyphs is from the 'maxp' table).
-    pub fn widths(&self) -> &'a [u8] {
-        self.widths
-    }
-}
-
-impl ReadArgs for DeviceRecord<'_> {
-    type Args = u16;
-}
-
-impl ComputeSize for DeviceRecord<'_> {
-    #[allow(clippy::needless_question_mark)]
-    fn compute_size(args: &u16) -> Result<usize, ReadError> {
-        let num_glyphs = *args;
-        let mut result = 0usize;
-        result = result
-            .checked_add(u8::RAW_BYTE_LEN)
-            .ok_or(ReadError::OutOfBounds)?;
-        result = result
-            .checked_add(u8::RAW_BYTE_LEN)
-            .ok_or(ReadError::OutOfBounds)?;
-        result = result
-            .checked_add(
-                (num_glyphs as usize)
-                    .checked_mul(u8::RAW_BYTE_LEN)
-                    .ok_or(ReadError::OutOfBounds)?,
-            )
-            .ok_or(ReadError::OutOfBounds)?;
-        Ok(result)
-    }
-}
-
-impl<'a> FontReadWithArgs<'a> for DeviceRecord<'a> {
-    fn read_with_args(data: FontData<'a>, args: &u16) -> Result<Self, ReadError> {
-        let mut cursor = data.cursor();
-        let num_glyphs = *args;
-        Ok(Self {
-            pixel_size: cursor.read()?,
-            max_width: cursor.read()?,
-            widths: cursor.read_array(num_glyphs as usize)?,
-        })
-    }
-}
-
-#[allow(clippy::needless_lifetimes)]
-impl<'a> DeviceRecord<'a> {
-    /// A constructor that requires additional arguments.
-    ///
-    /// This type requires some external state in order to be
-    /// parsed.
-    pub fn read(data: FontData<'a>, num_glyphs: u16) -> Result<Self, ReadError> {
-        let args = num_glyphs;
-        Self::read_with_args(data, &args)
-    }
-}
-
-#[cfg(feature = "experimental_traverse")]
-impl<'a> SomeRecord<'a> for DeviceRecord<'a> {
-    fn traverse(self, data: FontData<'a>) -> RecordResolver<'a> {
-        RecordResolver {
-            name: "DeviceRecord",
-            get_field: Box::new(move |idx, _data| match idx {
-                0usize => Some(Field::new("pixel_size", self.pixel_size())),
-                1usize => Some(Field::new("max_width", self.max_width())),
-                2usize => Some(Field::new("widths", self.widths())),
-                _ => None,
-            }),
-            data,
-        }
-    }
-}

read-fonts/src/tables/hdmx.rs

@@ -26,6 +26,89 @@ impl<'a> Hdmx<'a> {
     }
 }
 
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct DeviceRecord<'a> {
+    /// Pixel size for following widths (as ppem).
+    pub pixel_size: u8,
+    /// Maximum width.
+    pub max_width: u8,
+    /// Array of glyphs (numGlyphs is from the 'maxp' table).
+    pub widths: &'a [u8],
+}
+
+impl<'a> DeviceRecord<'a> {
+    /// Pixel size for following widths (as ppem).
+    pub fn pixel_size(&self) -> u8 {
+        self.pixel_size
+    }
+
+    /// Maximum width.
+    pub fn max_width(&self) -> u8 {
+        self.max_width
+    }
+
+    /// Array of widths, indexed by glyph id.
+    pub fn widths(&self) -> &'a [u8] {
+        self.widths
+    }
+}
+
+impl ReadArgs for DeviceRecord<'_> {
+    type Args = (u16, u32);
+}
+
+impl ComputeSize for DeviceRecord<'_> {
+    fn compute_size(args: &(u16, u32)) -> Result<usize, ReadError> {
+        let (_num_glyphs, size_device_record) = *args;
+        // Record size is explicitly defined in the parent hdmx table
+        Ok(size_device_record as usize)
+    }
+}
+
+impl<'a> FontReadWithArgs<'a> for DeviceRecord<'a> {
+    fn read_with_args(data: FontData<'a>, args: &(u16, u32)) -> Result<Self, ReadError> {
+        let mut cursor = data.cursor();
+        let (num_glyphs, _size_device_record) = *args;
+        Ok(Self {
+            pixel_size: cursor.read()?,
+            max_width: cursor.read()?,
+            widths: cursor.read_array(num_glyphs as usize)?,
+        })
+    }
+}
+
+#[allow(clippy::needless_lifetimes)]
+impl<'a> DeviceRecord<'a> {
+    /// A constructor that requires additional arguments.
+    ///
+    /// This type requires some external state in order to be
+    /// parsed.
+    pub fn read(
+        data: FontData<'a>,
+        num_glyphs: u16,
+        size_device_record: u32,
+    ) -> Result<Self, ReadError> {
+        let args = (num_glyphs, size_device_record);
+        Self::read_with_args(data, &args)
+    }
+}
+
+#[cfg(feature = "experimental_traverse")]
+impl<'a> SomeRecord<'a> for DeviceRecord<'a> {
+    fn traverse(self, data: FontData<'a>) -> RecordResolver<'a> {
+        RecordResolver {
+            name: "DeviceRecord",
+            get_field: Box::new(move |idx, _data| match idx {
+                0usize => Some(Field::new("pixel_size", self.pixel_size())),
+                1usize => Some(Field::new("max_width", self.max_width())),
+                2usize => Some(Field::new("widths", self.widths())),
+                _ => None,
+            }),
+            data,
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -34,10 +117,17 @@ mod tests {
     #[test]
     fn read_hdmx() {
         let buf = make_hdmx();
-        let hdmx = Hdmx::read(buf.font_data(), 2).unwrap();
+        let hdmx = Hdmx::read(buf.font_data(), 3).unwrap();
         assert_eq!(hdmx.version(), 0);
         assert_eq!(hdmx.num_records(), 3);
-        assert_eq!(hdmx.size_device_record(), 4);
+        // Note: this table has sizes for 3 glyphs making each device
+        // record 5 bytes in actual length, but each entry in the array
+        // should be aligned to 32-bits so the size is bumped to 8 bytes
+        //
+        // See the HdmxHeader::sizeDeviceRecord field at
+        // <https://learn.microsoft.com/en-us/typography/opentype/spec/hdmx>
+        // "Size of a device record, 32-bit aligned."
+        assert_eq!(hdmx.size_device_record(), 8);
         let records = hdmx
             .records()
             .iter()
@@ -47,18 +137,18 @@ mod tests {
         let expected_records = [
             DeviceRecord {
                 pixel_size: 8,
-                max_width: 12,
-                widths: &[10, 12],
+                max_width: 13,
+                widths: &[10, 12, 13],
             },
             DeviceRecord {
                 pixel_size: 16,
-                max_width: 20,
-                widths: &[18, 20],
+                max_width: 21,
+                widths: &[18, 20, 21],
             },
             DeviceRecord {
                 pixel_size: 32,
-                max_width: 40,
-                widths: &[38, 40],
+                max_width: 52,
+                widths: &[38, 40, 52],
             },
         ];
         assert_eq!(records, expected_records);
@@ -67,7 +157,7 @@ mod tests {
     #[test]
     fn find_by_size() {
         let buf = make_hdmx();
-        let hdmx = Hdmx::read(buf.font_data(), 2).unwrap();
+        let hdmx = Hdmx::read(buf.font_data(), 3).unwrap();
         assert_eq!(hdmx.record_for_size(8).unwrap().pixel_size, 8);
         assert_eq!(hdmx.record_for_size(16).unwrap().pixel_size, 16);
         assert_eq!(hdmx.record_for_size(32).unwrap().pixel_size, 32);
@@ -80,11 +170,11 @@ mod tests {
         be_buffer! {
             0u16,           // version
             3u16,           // num_records
-            4u32,           // size_device_record
-            // 3 records [pixel_size, max_width, width0, width1]
-            [8u8, 12, 10, 12],
-            [16u8, 20, 18, 20],
-            [32u8, 40, 38, 40]
+            8u32,           // size_device_record
+            // 3 records [pixel_size, max_width, width0, width1, ..padding]
+            [8u8, 13, 10, 12, 13, 0, 0, 0],
+            [16u8, 21, 18, 20, 21, 0, 0, 0],
+            [32u8, 52, 38, 40, 52, 0, 0, 0]
         }
     }
 }

resources/codegen_inputs/hdmx.rs

@@ -12,17 +12,6 @@ table Hdmx {
     size_device_record: u32,
     /// Array of device records.
     #[count($num_records)]
-    #[read_with($num_glyphs)]
+    #[read_with($num_glyphs, $size_device_record)]
     records: ComputedArray<DeviceRecord<'a>>,
 }
-
-#[read_args(num_glyphs: u16)]
-record DeviceRecord<'a> {
-    /// Pixel size for following widths (as ppem).
-    pixel_size: u8,
-    /// Maximum width.
-    max_width: u8,
-    /// Array of glyphs (numgGlyphs is from the 'maxp' table).
-    #[count($num_glyphs)]
-    widths: [u8],
-}

write-fonts/generated/generated_hdmx.rs

@@ -83,58 +83,3 @@ impl<'a> FromObjRef<read_fonts::tables::hdmx::Hdmx<'a>> for Hdmx {
 
 #[allow(clippy::needless_lifetimes)]
 impl<'a> FromTableRef<read_fonts::tables::hdmx::Hdmx<'a>> for Hdmx {}
-
-#[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
-#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
-pub struct DeviceRecord {
-    /// Pixel size for following widths (as ppem).
-    pub pixel_size: u8,
-    /// Maximum width.
-    pub max_width: u8,
-    /// Array of glyphs (numgGlyphs is from the 'maxp' table).
-    pub widths: Vec<u8>,
-}
-
-impl DeviceRecord {
-    /// Construct a new `DeviceRecord`
-    pub fn new(pixel_size: u8, max_width: u8, widths: Vec<u8>) -> Self {
-        Self {
-            pixel_size,
-            max_width,
-            widths: widths.into_iter().map(Into::into).collect(),
-        }
-    }
-}
-
-impl FontWrite for DeviceRecord {
-    fn write_into(&self, writer: &mut TableWriter) {
-        self.pixel_size.write_into(writer);
-        self.max_width.write_into(writer);
-        self.widths.write_into(writer);
-    }
-    fn table_type(&self) -> TableType {
-        TableType::Named("DeviceRecord")
-    }
-}
-
-impl Validate for DeviceRecord {
-    fn validate_impl(&self, ctx: &mut ValidationCtx) {
-        ctx.in_table("DeviceRecord", |ctx| {
-            ctx.in_field("widths", |ctx| {
-                if self.widths.len() > (u16::MAX as usize) {
-                    ctx.report("array exceeds max length");
-                }
-            });
-        })
-    }
-}
-
-impl FromObjRef<read_fonts::tables::hdmx::DeviceRecord<'_>> for DeviceRecord {
-    fn from_obj_ref(obj: &read_fonts::tables::hdmx::DeviceRecord, offset_data: FontData) -> Self {
-        DeviceRecord {
-            pixel_size: obj.pixel_size(),
-            max_width: obj.max_width(),
-            widths: obj.widths().to_owned_obj(offset_data),
-        }
-    }
-}