Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[skrifa] autohint bug fixes to match FT #1145

Merged
merged 4 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions fauntlet/src/compare_glyphs.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use crate::Hinting;

use super::{FreeTypeInstance, InstanceOptions, RegularizingPen, SkrifaInstance};
use skrifa::{outline::pen::PathElement, GlyphId};
use std::{io::Write, path::Path};
Expand All @@ -13,6 +15,21 @@ pub fn compare_glyphs(
// Don't run on bitmap fonts (yet)
return true;
}
if let Some(Hinting::Auto(_)) = options.hinting {
// This font is a pathological case for autohinting due to the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add an issue reference?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. I want to get some actual metrics from this font so we can potentially add limits to prevent this performance cliff. Beyond a certain level of geometric complexity, hinting is likely to produce useless results anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, similarly we might want to disable it for COLRv1 fonts, but I can do that on the client side.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sgtm.

Filed #1147 and added a link in the comment

// extreme number of generated segments and edges. To be
// precise, it takes longer to test this single font than the
// entire remainder of the Google Fonts corpus so we skip it
// here.
// Discussion at <https://github.com/googlefonts/fontations/issues/1147>
if ft_instance
.family_name()
.unwrap_or_default()
.contains("Handjet")
{
return true;
}
}
let glyph_count = skrifa_instance.glyph_count();
let is_scaled = options.ppem != 0;

Expand Down Expand Up @@ -86,6 +103,7 @@ pub fn compare_glyphs(
if exit_on_fail {
std::process::exit(1);
}
break;
}
}
ok
Expand Down
24 changes: 7 additions & 17 deletions fauntlet/src/font/freetype.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,12 @@ use freetype::{
ffi::{FT_Long, FT_Vector},
Face, Library,
};
use skrifa::{
outline::{HintingMode, LcdLayout, OutlinePen},
GlyphId,
};
use skrifa::{outline::OutlinePen, GlyphId};

use std::ffi::{c_int, c_void};

use super::{InstanceOptions, SharedFontData};

fn load_flags_from_hinting(mode: HintingMode) -> LoadFlag {
match mode {
HintingMode::Strong => LoadFlag::TARGET_MONO,
HintingMode::Smooth { lcd_subpixel, .. } => match lcd_subpixel {
Some(LcdLayout::Horizontal) => LoadFlag::TARGET_LCD,
Some(LcdLayout::Vertical) => LoadFlag::TARGET_LCD_V,
None => LoadFlag::TARGET_NORMAL,
},
}
}

pub struct FreeTypeInstance {
face: Face<SharedFontData>,
load_flags: LoadFlag,
Expand All @@ -37,10 +23,10 @@ impl FreeTypeInstance {
let mut face = library
.new_memory_face2(data.clone(), options.index as isize)
.ok()?;
let mut load_flags = LoadFlag::NO_AUTOHINT | LoadFlag::NO_BITMAP;
let mut load_flags = LoadFlag::NO_BITMAP;
match options.hinting {
None => load_flags |= LoadFlag::NO_HINTING,
Some(hinting) => load_flags |= load_flags_from_hinting(hinting),
Some(hinting) => load_flags |= hinting.freetype_load_flags(),
};
if options.ppem != 0 {
face.set_pixel_sizes(options.ppem, options.ppem).ok()?;
Expand Down Expand Up @@ -76,6 +62,10 @@ impl FreeTypeInstance {
Some(Self { face, load_flags })
}

pub fn family_name(&self) -> Option<String> {
self.face.family_name()
}

pub fn is_scalable(&self) -> bool {
self.face.is_scalable()
}
Expand Down
73 changes: 64 additions & 9 deletions fauntlet/src/font/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ use std::{
sync::Arc,
};

use ::freetype::Library;
use ::freetype::{face::LoadFlag, Library};
use ::skrifa::{
outline::HintingMode,
outline::{HintingOptions, SmoothMode, Target},
raw::{types::F2Dot14, FileRef, FontRef, TableProvider},
};

Expand All @@ -16,21 +16,76 @@ mod skrifa;
pub use freetype::FreeTypeInstance;
pub use skrifa::SkrifaInstance;

#[derive(Copy, Clone, PartialEq, Eq, Default, Debug)]
pub enum HintingTarget {
#[default]
Normal,
Light,
Lcd,
VerticalLcd,
Mono,
}

impl HintingTarget {
pub fn to_skrifa_target(self) -> Target {
match self {
Self::Normal => SmoothMode::Normal.into(),
Self::Light => SmoothMode::Light.into(),
Self::Lcd => SmoothMode::Lcd.into(),
Self::VerticalLcd => SmoothMode::VerticalLcd.into(),
Self::Mono => Target::Mono,
}
}

pub fn to_freetype_load_flags(self) -> LoadFlag {
match self {
Self::Normal => LoadFlag::TARGET_NORMAL,
Self::Light => LoadFlag::TARGET_LIGHT,
Self::Lcd => LoadFlag::TARGET_LCD,
Self::VerticalLcd => LoadFlag::TARGET_LCD_V,
Self::Mono => LoadFlag::TARGET_MONO,
}
}
}

#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum Hinting {
Interpreter(HintingTarget),
Auto(HintingTarget),
}

impl Hinting {
pub fn skrifa_options(self) -> HintingOptions {
match self {
Self::Interpreter(target) => HintingOptions {
engine: ::skrifa::outline::Engine::Interpreter,
target: target.to_skrifa_target(),
},
Self::Auto(target) => HintingOptions {
engine: ::skrifa::outline::Engine::Auto(None),
target: target.to_skrifa_target(),
},
}
}

pub fn freetype_load_flags(self) -> LoadFlag {
match self {
Self::Interpreter(target) => LoadFlag::NO_AUTOHINT | target.to_freetype_load_flags(),
Self::Auto(target) => LoadFlag::FORCE_AUTOHINT | target.to_freetype_load_flags(),
}
}
}

#[derive(Copy, Clone, Debug)]
pub struct InstanceOptions<'a> {
pub index: usize,
pub ppem: u32,
pub coords: &'a [F2Dot14],
pub hinting: Option<HintingMode>,
pub hinting: Option<Hinting>,
}

impl<'a> InstanceOptions<'a> {
pub fn new(
index: usize,
ppem: u32,
coords: &'a [F2Dot14],
hinting: Option<HintingMode>,
) -> Self {
pub fn new(index: usize, ppem: u32, coords: &'a [F2Dot14], hinting: Option<Hinting>) -> Self {
Self {
index,
ppem,
Expand Down
9 changes: 7 additions & 2 deletions fauntlet/src/font/skrifa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,13 @@ impl<'a> SkrifaInstance<'a> {
let outlines = font.outline_glyphs();
let hinter = if options.ppem != 0 && options.hinting.is_some() {
Some(
HintingInstance::new(&outlines, size, options.coords, options.hinting.unwrap())
.ok()?,
HintingInstance::new(
&outlines,
size,
options.coords,
options.hinting.unwrap().skrifa_options(),
)
.ok()?,
)
} else {
None
Expand Down
4 changes: 3 additions & 1 deletion fauntlet/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,7 @@ mod font;
mod pen;

pub use compare_glyphs::compare_glyphs;
pub use font::{Font, FreeTypeInstance, InstanceOptions, SharedFontData, SkrifaInstance};
pub use font::{
Font, FreeTypeInstance, Hinting, HintingTarget, InstanceOptions, SharedFontData, SkrifaInstance,
};
pub use pen::RegularizingPen;
Loading