Skip to content

Commit

Permalink
[read-fonts] hdmx: use explicitly defined record size (#1293)
Browse files Browse the repository at this point in the history 
We were not accounting for the 32-bit alignment requirement.
  • Loading branch information
@dfrg
dfrg authored Dec 19, 2024
1 parent 0c9fec3 commit a65d7fd
Show file tree
Hide file tree
Showing 4 changed files with 113 additions and 177 deletions.
104 changes: 8 additions & 96 deletions read-fonts/generated/generated_hdmx.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
}
}
}
118 changes: 104 additions & 14 deletions read-fonts/src/tables/hdmx.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand All @@ -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()
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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]
}
}
}
13 changes: 1 addition & 12 deletions resources/codegen_inputs/hdmx.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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],
}
55 changes: 0 additions & 55 deletions write-fonts/generated/generated_hdmx.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
}
}

0 comments on commit a65d7fd

Please sign in to comment.