fonts: adopt fixed crate for freetype fixed-point math

This found at least one bug and should prevent future bugs.

Cargo.lock

@@ -343,6 +343,12 @@ version = "1.1.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "d468802bab17cbc0cc575e9b053f41e72aa36bfa6b7f55e3529ffa43161b97fa"
 
+[[package]]
+name = "az"
+version = "1.2.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "7b7e4c2464d97fe331d41de9d5db0def0a96f4d823b8b32a2efd503578988973"
+
 [[package]]
 name = "backtrace"
 version = "0.3.68"
@@ -1671,6 +1677,18 @@ version = "1.2.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "8fcfdc7a0362c9f4444381a9e697c79d435fe65b52a37466fc2c1184cee9edc6"
 
+[[package]]
+name = "fixed"
+version = "1.23.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "79386fdcec5e0fde91b1a6a5bcd89677d1f9304f7f986b154a1b9109038854d9"
+dependencies = [
+ "az",
+ "bytemuck",
+ "half 2.2.1",
+ "typenum",
+]
+
 [[package]]
 name = "fixedbitset"
 version = "0.4.2"
@@ -1789,6 +1807,7 @@ name = "freetype"
 version = "0.1.0"
 dependencies = [
  "cc",
+ "fixed",
 ]
 
 [[package]]

deps/freetype/Cargo.toml

@@ -7,6 +7,7 @@ links = "freetype"
 build = "build.rs"
 
 [dependencies]
+fixed = "1.23"
 
 [build-dependencies]
 cc = {version="1.0", features = ["parallel"]}

deps/freetype/regenerate.sh

@@ -1,5 +1,15 @@
 #!/bin/bash
 
+bindgen bindings.h -o src/types.rs \
+  --no-layout-tests \
+  --no-doc-comments \
+  --blocklist-type "FT_(Int16|UInt16|Int32|UInt32|Int16|Int64|UInt64)" \
+  --raw-line "#![allow(non_camel_case_types)]" \
+  --default-enum-style rust \
+  --generate=types \
+  --allowlist-type="FT_(Fixed|Pos|F\\d+Dot\\d+)" \
+  -- -Ifreetype2/include
+
 bindgen bindings.h -o src/lib.rs \
   --no-layout-tests \
   --no-doc-comments \
@@ -9,6 +19,9 @@ bindgen bindings.h -o src/lib.rs \
   --raw-line "#![allow(non_upper_case_globals)]" \
   --raw-line "#![allow(clippy::unreadable_literal)]" \
   --raw-line "#![allow(clippy::upper_case_acronyms)]" \
+  --raw-line "mod types;" \
+  --raw-line "mod fixed_point;" \
+  --raw-line "pub use fixed_point::*;" \
   --raw-line "pub type FT_Int16 = i16;" \
   --raw-line "pub type FT_UInt16 = u16;" \
   --raw-line "pub type FT_Int32 = i32;" \
@@ -21,3 +34,8 @@ bindgen bindings.h -o src/lib.rs \
   --allowlist-type="(SVG|[FT]T)_.*" \
   --allowlist-var="(SVG|[FT]T)_.*" \
   -- -Ifreetype2/include
+
+perl -i -pe 's,^pub type FT_Fixed =,//$&,' src/lib.rs
+perl -i -pe 's,^pub type FT_F26Dot6 =,//$&,' src/lib.rs
+perl -i -pe 's,^pub type FT_F2Dot14 =,//$&,' src/lib.rs
+perl -i -pe 's,^pub type FT_Pos =,//$&,' src/lib.rs

deps/freetype/src/fixed_point.rs

@@ -0,0 +1,84 @@
+//! Freetype and fonts in general make use of fixed point types
+//! to represent fractional numbers without using floating point
+//! types.
+//! Since those types are expressed in C as integers, it can be
+//! easy to misue the values without scaling/adapting them appropriately.
+//! This module adopts the fixed crate to manage that more robustly.
+//!
+//! In order to drop those types in to the bindgen-generated bindings
+//! that comprise most of this crate, we separately run bindgen to
+//! extract the underlying types and alias them in here as various
+//! XXXStorage types.
+//!
+//! Now, since those types are based on things like `c_long` and
+//! `c_short`, we don't necessarily know whether those are `i32` or `i64`
+//! we need to use a helper trait to allow the compiler to resolve
+//! the FixedXX variant that matches the storage size.
+use crate::types::{
+    FT_F26Dot6 as F26Dot6Storage, FT_F2Dot14 as F2Dot14Storage, FT_Fixed as FixedStorage,
+    FT_Pos as PosStorage,
+};
+use fixed::types::extra::{U14, U16, U6};
+
+/// Helper trait to resolve eg: `c_long` to the `fixed::FixedIXX`
+/// type that occupies the same size
+pub trait SelectFixedStorage<T> {
+    type Storage;
+}
+
+impl<T> SelectFixedStorage<T> for i8 {
+    type Storage = fixed::FixedI8<T>;
+}
+impl<T> SelectFixedStorage<T> for i16 {
+    type Storage = fixed::FixedI16<T>;
+}
+impl<T> SelectFixedStorage<T> for i32 {
+    type Storage = fixed::FixedI32<T>;
+}
+impl<T> SelectFixedStorage<T> for i64 {
+    type Storage = fixed::FixedI64<T>;
+}
+
+pub type FT_F2Dot14 = <F2Dot14Storage as SelectFixedStorage<U14>>::Storage;
+pub type FT_F26Dot6 = <F26Dot6Storage as SelectFixedStorage<U6>>::Storage;
+pub type FT_Fixed = <FixedStorage as SelectFixedStorage<U16>>::Storage;
+
+/// FT_Pos is used to store vectorial coordinates. Depending on the context, these can
+/// represent distances in integer font units, or 16.16, or 26.6 fixed-point pixel coordinates.
+#[repr(transparent)]
+#[derive(Copy, Clone, Debug)]
+pub struct FT_Pos(PosStorage);
+
+impl FT_Pos {
+    /// Return the value expressed in font-units
+    pub fn font_units(self) -> PosStorage {
+        self.0
+    }
+
+    /// Construct a pos expressed in font-units
+    pub fn from_font_units(v: PosStorage) -> Self {
+        Self(v)
+    }
+
+    /// Extract the FT_Fixed/F16Dot16 equivalent value
+    pub fn f16d16(self) -> <PosStorage as SelectFixedStorage<U16>>::Storage {
+        <PosStorage as SelectFixedStorage<U16>>::Storage::from_bits(self.0)
+    }
+
+    /// Extract the F26Dot6 equivalent value
+    pub fn f26d6(self) -> <PosStorage as SelectFixedStorage<U6>>::Storage {
+        <PosStorage as SelectFixedStorage<U6>>::Storage::from_bits(self.0)
+    }
+}
+
+impl From<FT_F26Dot6> for FT_Pos {
+    fn from(src: FT_F26Dot6) -> FT_Pos {
+        FT_Pos(src.to_bits())
+    }
+}
+
+impl From<FT_Fixed> for FT_Pos {
+    fn from(src: FT_Fixed) -> FT_Pos {
+        FT_Pos(src.to_bits())
+    }
+}

deps/freetype/src/lib.rs

@@ -5,6 +5,9 @@
 #![allow(non_upper_case_globals)]
 #![allow(clippy::unreadable_literal)]
 #![allow(clippy::upper_case_acronyms)]
+mod fixed_point;
+mod types;
+pub use fixed_point::*;
 pub type FT_Int16 = i16;
 pub type FT_UInt16 = u16;
 pub type FT_Int32 = i32;
@@ -58,6 +61,22 @@ impl<T> ::std::cmp::Eq for __BindgenUnionField<T> {}
 pub const FT_RENDER_POOL_SIZE: u32 = 16384;
 pub const FT_MAX_MODULES: u32 = 32;
 pub const TT_CONFIG_OPTION_MAX_RUNNABLE_OPCODES: u32 = 1000000;
+pub const FT_CHAR_BIT: u32 = 8;
+pub const FT_USHORT_MAX: u32 = 65535;
+pub const FT_INT_MAX: u32 = 2147483647;
+pub const FT_INT_MIN: i32 = -2147483648;
+pub const FT_UINT_MAX: u32 = 4294967295;
+pub const FT_LONG_MIN: i64 = -9223372036854775808;
+pub const FT_LONG_MAX: u64 = 9223372036854775807;
+pub const FT_ULONG_MAX: i32 = -1;
+pub const FT_LLONG_MAX: u64 = 9223372036854775807;
+pub const FT_LLONG_MIN: i64 = -9223372036854775808;
+pub const FT_ULLONG_MAX: i32 = -1;
+pub const FT_SIZEOF_INT: u32 = 4;
+pub const FT_SIZEOF_LONG: u32 = 8;
+pub const FT_SIZEOF_LONG_LONG: u32 = 8;
+pub const FT_OUTLINE_CONTOURS_MAX: u32 = 32767;
+pub const FT_OUTLINE_POINTS_MAX: u32 = 32767;
 pub const FT_OUTLINE_NONE: u32 = 0;
 pub const FT_OUTLINE_OWNER: u32 = 1;
 pub const FT_OUTLINE_EVEN_ODD_FILL: u32 = 2;
@@ -849,7 +868,7 @@ pub struct FT_StreamRec_ {
     pub limit: *mut ::std::os::raw::c_uchar,
 }
 pub type FT_StreamRec = FT_StreamRec_;
-pub type FT_Pos = ::std::os::raw::c_long;
+//pub type FT_Pos = ::std::os::raw::c_long;
 #[repr(C)]
 #[derive(Debug, Copy, Clone)]
 pub struct FT_Vector_ {
@@ -1055,9 +1074,9 @@ pub type FT_Int = ::std::os::raw::c_int;
 pub type FT_UInt = ::std::os::raw::c_uint;
 pub type FT_Long = ::std::os::raw::c_long;
 pub type FT_ULong = ::std::os::raw::c_ulong;
-pub type FT_F2Dot14 = ::std::os::raw::c_short;
-pub type FT_F26Dot6 = ::std::os::raw::c_long;
-pub type FT_Fixed = ::std::os::raw::c_long;
+//pub type FT_F2Dot14 = ::std::os::raw::c_short;
+//pub type FT_F26Dot6 = ::std::os::raw::c_long;
+//pub type FT_Fixed = ::std::os::raw::c_long;
 pub type FT_Error = ::std::os::raw::c_int;
 pub type FT_Pointer = *mut ::std::os::raw::c_void;
 pub type FT_Offset = usize;

deps/freetype/src/types.rs

@@ -0,0 +1,8 @@
+/* automatically generated by rust-bindgen 0.66.1 */
+
+#![allow(non_camel_case_types)]
+
+pub type FT_Pos = ::std::os::raw::c_long;
+pub type FT_F2Dot14 = ::std::os::raw::c_short;
+pub type FT_F26Dot6 = ::std::os::raw::c_long;
+pub type FT_Fixed = ::std::os::raw::c_long;

wezterm-font/src/ftwrap.rs

@@ -346,18 +346,11 @@ impl Face {
                     let instance = &styles[vidx];
                     let axes = std::slice::from_raw_parts(mm.axis, mm.num_axis as usize);
 
-                    fn ft_make_tag(a: u8, b: u8, c: u8, d: u8) -> FT_ULong {
-                        (a as FT_ULong) << 24
-                            | (b as FT_ULong) << 16
-                            | (c as FT_ULong) << 8
-                            | (d as FT_ULong)
-                    }
-
                     for (i, axis) in axes.iter().enumerate() {
                         let coords =
                             std::slice::from_raw_parts(instance.coords, mm.num_axis as usize);
-                        let value = coords[i] as f64 / (1 << 16) as f64;
-                        let default_value = axis.def as f64 / (1 << 16) as f64;
+                        let value = coords[i].to_num::<f64>();
+                        let default_value = axis.def.to_num::<f64>();
                         let scale = if default_value != 0. {
                             value / default_value
                         } else {
@@ -473,7 +466,7 @@ impl Face {
 
         // Scaling before truncating to integer minimizes the chances of hitting
         // the fallback code for set_pixel_sizes below.
-        let size = (point_size * 64.0) as FT_F26Dot6;
+        let size = FT_F26Dot6::from_num(point_size);
 
         let selected_size = match self.set_char_size(size, size, dpi, dpi) {
             Ok(_) => {
@@ -853,7 +846,7 @@ impl Face {
                 conic_to: Some(conic_to),
                 cubic_to: Some(cubic_to),
                 shift: 16, // match the same coordinate space as transforms
-                delta: 0,
+                delta: FT_Pos::from_font_units(0),
             };
 
             let mut ops = vec![];
@@ -1082,8 +1075,7 @@ impl Face {
     fn cell_metrics(&mut self) -> ComputedCellMetrics {
         unsafe {
             let metrics = &(*(*self.face).size).metrics;
-            let height = (metrics.y_scale as f64 * f64::from((*self.face).height))
-                / (f64::from(0x1_0000) * 64.0);
+            let height = metrics.y_scale.to_num::<f64>() * f64::from((*self.face).height) / 64.0;
 
             let mut width = 0.0;
             let mut num_examined = 0;
@@ -1096,8 +1088,8 @@ impl Face {
                 if succeeded(res) {
                     num_examined += 1;
                     let glyph = &(*(*self.face).glyph);
-                    if glyph.metrics.horiAdvance as f64 > width {
-                        width = glyph.metrics.horiAdvance as f64;
+                    if glyph.metrics.horiAdvance.font_units() as f64 > width {
+                        width = glyph.metrics.horiAdvance.font_units() as f64;
                     }
                 }
             }
@@ -1109,8 +1101,8 @@ impl Face {
                     if succeeded(res) {
                         num_examined += 1;
                         let glyph = &(*(*self.face).glyph);
-                        if glyph.metrics.horiAdvance as f64 > width {
-                            width = glyph.metrics.horiAdvance as f64;
+                        if glyph.metrics.horiAdvance.font_units() as f64 > width {
+                            width = glyph.metrics.horiAdvance.font_units() as f64;
                         }
                     }
                 }
@@ -1472,23 +1464,7 @@ pub struct NameRecord {
 }
 
 pub fn vector_x_y(vector: &FT_Vector) -> (f32, f32) {
-    let x = fixed_to_f32(vector.x);
-    let y = fixed_to_f32(vector.y);
-    (x, y)
-}
-
-pub fn two_dot_14_to_f64(f: FT_F2Dot14) -> f64 {
-    f as f64 / (1 << 14) as f64
-}
-
-/// Fixed-point 16.16 to float.
-///
-pub fn fixed_to_f32(f: FT_Fixed) -> f32 {
-    f as f32 / (1 << 16) as f32
-}
-
-pub fn fixed_to_f64(f: FT_Fixed) -> f64 {
-    f as f64 / (1 << 16) as f64
+    (vector.x.f16d16().to_num(), vector.y.f16d16().to_num())
 }
 
 pub fn composite_mode_to_operator(mode: FT_Composite_Mode) -> cairo::Operator {
@@ -1526,3 +1502,7 @@ pub fn composite_mode_to_operator(mode: FT_Composite_Mode) -> cairo::Operator {
         _ => unreachable!(),
     }
 }
+
+fn ft_make_tag(a: u8, b: u8, c: u8, d: u8) -> FT_ULong {
+    (a as FT_ULong) << 24 | (b as FT_ULong) << 16 | (c as FT_ULong) << 8 | (d as FT_ULong)
+}

wezterm-font/src/rasterizer/freetype.rs

@@ -1,7 +1,6 @@
 use crate::ftwrap::{
-    composite_mode_to_operator, fixed_to_f32, fixed_to_f64, two_dot_14_to_f64, vector_x_y,
-    FT_Affine23, FT_ColorIndex, FT_ColorLine, FT_ColorStop, FT_Get_Colorline_Stops, FT_Int32,
-    FT_PaintExtend, IsSvg, FT_LOAD_NO_HINTING,
+    composite_mode_to_operator, vector_x_y, FT_Affine23, FT_ColorIndex, FT_ColorLine, FT_ColorStop,
+    FT_Fixed, FT_Get_Colorline_Stops, FT_Int32, FT_PaintExtend, IsSvg, FT_LOAD_NO_HINTING,
 };
 use crate::hbwrap::DrawOp;
 use crate::parser::ParsedFont;
@@ -280,10 +279,10 @@ impl FreeTypeRasterizer {
 
         if parsed.synthesize_italic {
             face.set_transform(Some(FT_Matrix {
-                xx: 1 * 65536,                         // scale x
-                yy: 1 * 65536,                         // scale y
-                xy: (FAKE_ITALIC_SKEW * 65536.0) as _, // skew x
-                yx: 0 * 65536,                         // skew y
+                xx: FT_Fixed::from_num(1),                // scale x
+                yy: FT_Fixed::from_num(1),                // scale y
+                xy: FT_Fixed::from_num(FAKE_ITALIC_SKEW), // skew x
+                yx: FT_Fixed::from_num(0),                // skew y
             }));
         }
 
@@ -434,8 +433,8 @@ impl<'a> Walker<'a> {
                         y0,
                         x1,
                         y1,
-                        r0: grad.r0 as f32,
-                        r1: grad.r1 as f32,
+                        r0: grad.r0.font_units() as f32,
+                        r1: grad.r1.font_units() as f32,
                         color_line: self.decode_color_line(&grad.colorline)?,
                     };
                     self.ops.push(paint);
@@ -444,8 +443,8 @@ impl<'a> Walker<'a> {
                     let grad = paint.u.sweep_gradient.as_ref();
                     log::info!("{level:>3} {grad:?}");
                     let (x0, y0) = vector_x_y(&grad.center);
-                    let start_angle = fixed_to_f32(grad.start_angle);
-                    let end_angle = fixed_to_f32(grad.end_angle);
+                    let start_angle = grad.start_angle.to_num();
+                    let end_angle = grad.end_angle.to_num();
 
                     let paint = PaintOp::PaintSweepGradient {
                         x0,
@@ -500,7 +499,7 @@ impl<'a> Walker<'a> {
                     log::info!("{level:>3} {t:?}");
 
                     let mut matrix = Matrix::identity();
-                    matrix.translate(fixed_to_f64(t.dx), fixed_to_f64(t.dy));
+                    matrix.translate(t.dx.to_num(), t.dy.to_num());
                     self.ops.push(PaintOp::PushTransform(matrix));
                     self.walk_paint(t.paint, level + 1)?;
                     self.ops.push(PaintOp::PopTransform);
@@ -510,14 +509,14 @@ impl<'a> Walker<'a> {
                     log::info!("{level:>3} {scale:?}");
 
                     // Scaling around a center coordinate
-                    let center_x = fixed_to_f64(scale.center_x);
-                    let center_y = fixed_to_f64(scale.center_x);
+                    let center_x = scale.center_x.to_num();
+                    let center_y = scale.center_x.to_num();
 
                     let mut p1 = Matrix::identity();
                     p1.translate(center_x, center_y);
 
                     let mut p2 = Matrix::identity();
-                    p2.scale(fixed_to_f64(scale.scale_x), fixed_to_f64(scale.scale_y));
+                    p2.scale(scale.scale_x.to_num(), scale.scale_y.to_num());
 
                     let mut p3 = Matrix::identity();
                     p3.translate(-center_x, -center_y);
@@ -535,14 +534,14 @@ impl<'a> Walker<'a> {
                     log::info!("{level:>3} {rot:?}");
 
                     // Rotating around a center coordinate
-                    let center_x = fixed_to_f64(rot.center_x);
-                    let center_y = fixed_to_f64(rot.center_x);
+                    let center_x = rot.center_x.to_num();
+                    let center_y = rot.center_x.to_num();
 
                     let mut p1 = Matrix::identity();
                     p1.translate(center_x, center_y);
 
                     let mut p2 = Matrix::identity();
-                    p2.rotate(PI * fixed_to_f64(rot.angle));
+                    p2.rotate(PI * rot.angle.to_num::<f64>());
 
                     let mut p3 = Matrix::identity();
                     p3.translate(-center_x, -center_y);
@@ -560,14 +559,14 @@ impl<'a> Walker<'a> {
                     log::info!("{level:>3} {skew:?}");
 
                     // Skewing around a center coordinate
-                    let center_x = fixed_to_f64(skew.center_x);
-                    let center_y = fixed_to_f64(skew.center_x);
+                    let center_x = skew.center_x.to_num();
+                    let center_y = skew.center_x.to_num();
 
                     let mut p1 = Matrix::identity();
                     p1.translate(center_x, center_y);
 
-                    let x_skew_angle = fixed_to_f64(skew.x_skew_angle);
-                    let y_skew_angle = fixed_to_f64(skew.y_skew_angle);
+                    let x_skew_angle: f64 = skew.x_skew_angle.to_num();
+                    let y_skew_angle: f64 = skew.y_skew_angle.to_num();
                     let x = (PI * -x_skew_angle).tan();
                     let y = (PI * y_skew_angle).tan();
 
@@ -605,7 +604,7 @@ impl<'a> Walker<'a> {
     }
 
     fn decode_color_index(&mut self, c: &FT_ColorIndex) -> anyhow::Result<SrgbaPixel> {
-        let alpha = two_dot_14_to_f64(c.alpha);
+        let alpha: f64 = c.alpha.to_num();
         let (r, g, b, a) = if c.palette_index == 0xffff {
             // Foreground color.
             // We use white here because the rendering stage will
@@ -613,7 +612,12 @@ impl<'a> Walker<'a> {
             (0xff, 0xff, 0xff, 1.0)
         } else {
             let color = self.face.get_palette_entry(c.palette_index as _)?;
-            (color.red, color.green, color.blue, c.alpha as f64 / 255.)
+            (
+                color.red,
+                color.green,
+                color.blue,
+                color.alpha as f64 / 255.,
+            )
         };
 
         let alpha = (a * alpha * 255.) as u8;
@@ -634,7 +638,7 @@ impl<'a> Walker<'a> {
             let stop = unsafe { stop.assume_init() };
 
             color_stops.push(ColorStop {
-                offset: stop.stop_offset as f32 / 65535.,
+                offset: stop.stop_offset.to_num(),
                 color: self.decode_color_index(&stop.color)?,
             });
         }
@@ -700,12 +704,12 @@ pub enum PaintOp {
 
 fn affine2x3_to_matrix(t: FT_Affine23) -> Matrix {
     Matrix::new(
-        fixed_to_f64(t.xx),
-        fixed_to_f64(t.yx),
-        fixed_to_f64(t.xy),
-        fixed_to_f64(t.yy),
-        fixed_to_f64(t.dy),
-        fixed_to_f64(t.dx),
+        t.xx.to_num(),
+        t.yx.to_num(),
+        t.xy.to_num(),
+        t.yy.to_num(),
+        t.dy.to_num(),
+        t.dx.to_num(),
     )
 }
 

wezterm-font/src/shaper/harfbuzz.rs

@@ -722,15 +722,15 @@ impl FontShaper for HarfbuzzShaper {
         let scale = self.handles[font_idx].scale.unwrap_or(1.);
 
         let selected_size = pair.face.set_font_size(size * scale, dpi)?;
-        let y_scale = unsafe { (*(*pair.face.face).size).metrics.y_scale as f64 / 65536.0 };
+        let y_scale = unsafe { (*(*pair.face.face).size).metrics.y_scale.to_num::<f64>() };
         let mut metrics = FontMetrics {
             cell_height: PixelLength::new(selected_size.height),
             cell_width: PixelLength::new(selected_size.width),
             // Note: face.face.descender is useless, we have to go through
             // face.face.size.metrics to get to the real descender!
-            descender: PixelLength::new(
-                unsafe { (*(*pair.face.face).size).metrics.descender as f64 } / 64.0,
-            ),
+            descender: PixelLength::new(unsafe {
+                (*(*pair.face.face).size).metrics.descender.f26d6().to_num()
+            }),
             underline_thickness: PixelLength::new(
                 unsafe { (*pair.face.face).underline_thickness as f64 } * y_scale / 64.,
             ),

wezterm-gui/src/termwindow/palette.rs

@@ -151,7 +151,7 @@ fn build_commands(
         match (scores.get(&*a.brief), scores.get(&*b.brief)) {
             // Want descending frecency score, so swap a<->b
             // for the compare here
-            (Some(a), Some(b)) => match b.partial_cmp(&a) {
+            (Some(a), Some(b)) => match b.partial_cmp(a) {
                 Some(Ordering::Equal) | None => {}
                 Some(ordering) => return ordering,
             },