From 7264030a2883559fae4a7c560f44b09d683a01cc Mon Sep 17 00:00:00 2001 From: Wez Furlong Date: Sun, 20 Aug 2023 14:23:17 -0700 Subject: [PATCH] fonts: adopt fixed crate for freetype fixed-point math This found at least one bug and should prevent future bugs. --- Cargo.lock | 19 ++++++ deps/freetype/Cargo.toml | 1 + deps/freetype/regenerate.sh | 18 ++++++ deps/freetype/src/fixed_point.rs | 84 +++++++++++++++++++++++++ deps/freetype/src/lib.rs | 27 ++++++-- deps/freetype/src/types.rs | 8 +++ wezterm-font/src/ftwrap.rs | 48 +++++--------- wezterm-font/src/rasterizer/freetype.rs | 66 ++++++++++--------- wezterm-font/src/shaper/harfbuzz.rs | 8 +-- wezterm-gui/src/termwindow/palette.rs | 2 +- 10 files changed, 207 insertions(+), 74 deletions(-) create mode 100644 deps/freetype/src/fixed_point.rs create mode 100644 deps/freetype/src/types.rs diff --git a/Cargo.lock b/Cargo.lock index 891ffd055..d40e117fb 100644 --- a/Cargo.lock +++ b/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]] diff --git a/deps/freetype/Cargo.toml b/deps/freetype/Cargo.toml index 0b07cf149..41ffbaad9 100644 --- a/deps/freetype/Cargo.toml +++ b/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"]} diff --git a/deps/freetype/regenerate.sh b/deps/freetype/regenerate.sh index b0ffe2a03..c6106ad8f 100755 --- a/deps/freetype/regenerate.sh +++ b/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 diff --git a/deps/freetype/src/fixed_point.rs b/deps/freetype/src/fixed_point.rs new file mode 100644 index 000000000..149e93f18 --- /dev/null +++ b/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 { + type Storage; +} + +impl SelectFixedStorage for i8 { + type Storage = fixed::FixedI8; +} +impl SelectFixedStorage for i16 { + type Storage = fixed::FixedI16; +} +impl SelectFixedStorage for i32 { + type Storage = fixed::FixedI32; +} +impl SelectFixedStorage for i64 { + type Storage = fixed::FixedI64; +} + +pub type FT_F2Dot14 = >::Storage; +pub type FT_F26Dot6 = >::Storage; +pub type FT_Fixed = >::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) -> >::Storage { + >::Storage::from_bits(self.0) + } + + /// Extract the F26Dot6 equivalent value + pub fn f26d6(self) -> >::Storage { + >::Storage::from_bits(self.0) + } +} + +impl From for FT_Pos { + fn from(src: FT_F26Dot6) -> FT_Pos { + FT_Pos(src.to_bits()) + } +} + +impl From for FT_Pos { + fn from(src: FT_Fixed) -> FT_Pos { + FT_Pos(src.to_bits()) + } +} diff --git a/deps/freetype/src/lib.rs b/deps/freetype/src/lib.rs index a96f0f1c8..476412d88 100644 --- a/deps/freetype/src/lib.rs +++ b/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 ::std::cmp::Eq for __BindgenUnionField {} 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; diff --git a/deps/freetype/src/types.rs b/deps/freetype/src/types.rs new file mode 100644 index 000000000..f9d8f2bda --- /dev/null +++ b/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; diff --git a/wezterm-font/src/ftwrap.rs b/wezterm-font/src/ftwrap.rs index af94b4403..c7dc53f87 100644 --- a/wezterm-font/src/ftwrap.rs +++ b/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::(); + let default_value = axis.def.to_num::(); 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::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) +} diff --git a/wezterm-font/src/rasterizer/freetype.rs b/wezterm-font/src/rasterizer/freetype.rs index 6f195daac..b9d948704 100644 --- a/wezterm-font/src/rasterizer/freetype.rs +++ b/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::()); 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 { - 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(), ) } diff --git a/wezterm-font/src/shaper/harfbuzz.rs b/wezterm-font/src/shaper/harfbuzz.rs index 4394bf88c..6d234d76c 100644 --- a/wezterm-font/src/shaper/harfbuzz.rs +++ b/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::() }; 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., ), diff --git a/wezterm-gui/src/termwindow/palette.rs b/wezterm-gui/src/termwindow/palette.rs index 5a2867dbe..c3e6ce4c9 100644 --- a/wezterm-gui/src/termwindow/palette.rs +++ b/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, },