-
Notifications
You must be signed in to change notification settings - Fork 105
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
CoreText: Faster OTC font loading #232
base: main
Are you sure you want to change the base?
Changes from 1 commit
263918a
aa44d72
8cb2f41
cef0442
c957266
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -144,5 +144,5 @@ pub mod source; | |
#[cfg(feature = "source")] | ||
pub mod sources; | ||
|
||
mod matching; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this change be split out into a different PR? |
||
pub mod matching; | ||
mod utils; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,7 +35,7 @@ use std::path::Path; | |
/// fonts. | ||
pub trait Loader: Clone + Sized { | ||
/// The handle that the API natively uses to represent a font. | ||
type NativeFont; | ||
type NativeFont: 'static; | ||
|
||
/// Loads a font from raw font data (the contents of a `.ttf`/`.otf`/etc. file). | ||
/// | ||
|
@@ -63,22 +63,23 @@ pub trait Loader: Clone + Sized { | |
} | ||
|
||
/// Creates a font from a native API handle. | ||
unsafe fn from_native_font(native_font: Self::NativeFont) -> Self; | ||
unsafe fn from_native_font(native_font: &Self::NativeFont) -> Self; | ||
|
||
/// Loads the font pointed to by a handle. | ||
fn from_handle(handle: &Handle) -> Result<Self, FontLoadingError> { | ||
match *handle { | ||
Handle::Memory { | ||
ref bytes, | ||
font_index, | ||
} => Self::from_bytes((*bytes).clone(), font_index), | ||
match handle { | ||
Handle::Memory { bytes, font_index } => Self::from_bytes((*bytes).clone(), *font_index), | ||
#[cfg(not(target_arch = "wasm32"))] | ||
Handle::Path { | ||
ref path, | ||
font_index, | ||
} => Self::from_path(path, font_index), | ||
Handle::Path { path, font_index } => Self::from_path(path, *font_index), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mind removing unrelated reformatting? |
||
#[cfg(target_arch = "wasm32")] | ||
Handle::Path { .. } => Err(FontLoadingError::NoFilesystem), | ||
Handle::Native { .. } => { | ||
if let Some(native) = handle.native_as::<Self::NativeFont>() { | ||
unsafe { Ok(Self::from_native_font(native)) } | ||
} else { | ||
Err(FontLoadingError::UnknownFormat) | ||
} | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,29 +122,14 @@ impl Font { | |
} | ||
|
||
/// Creates a font from a native API handle. | ||
pub unsafe fn from_native_font(core_text_font: NativeFont) -> Font { | ||
Font::from_core_text_font(core_text_font) | ||
} | ||
|
||
unsafe fn from_core_text_font(core_text_font: NativeFont) -> Font { | ||
let mut font_data = FontData::Unavailable; | ||
match core_text_font.url() { | ||
None => warn!("No URL found for Core Text font!"), | ||
Some(url) => match url.to_path() { | ||
Some(path) => match File::open(path) { | ||
Ok(ref mut file) => match utils::slurp_file(file) { | ||
Ok(data) => font_data = FontData::Memory(Arc::new(data)), | ||
Err(_) => warn!("Couldn't read file data for Core Text font!"), | ||
}, | ||
Err(_) => warn!("Could not open file for Core Text font!"), | ||
}, | ||
None => warn!("Could not convert URL from Core Text font to path!"), | ||
}, | ||
} | ||
|
||
pub unsafe fn from_native_font(core_text_font: &NativeFont) -> Font { | ||
Font::from_core_text_font_no_path(core_text_font.clone()) | ||
} | ||
/// Creates a font from a native API handle, without performing a lookup on the disk. | ||
pub unsafe fn from_core_text_font_no_path(core_text_font: NativeFont) -> Font { | ||
Font { | ||
core_text_font, | ||
font_data, | ||
font_data: FontData::Unavailable, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What the repercussions of no longer having this font data? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ha, good catch; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the other hand, this PR is already Semver-breaking with a change to |
||
} | ||
} | ||
|
||
|
@@ -153,7 +138,10 @@ impl Font { | |
/// This function is only available on the Core Text backend. | ||
pub fn from_core_graphics_font(core_graphics_font: CGFont) -> Font { | ||
unsafe { | ||
Font::from_core_text_font(core_text::font::new_from_CGFont(&core_graphics_font, 16.0)) | ||
Font::from_core_text_font_no_path(core_text::font::new_from_CGFont( | ||
&core_graphics_font, | ||
16.0, | ||
)) | ||
Comment on lines
+141
to
+144
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change seems unrelated? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, it kinda is; |
||
} | ||
} | ||
|
||
|
@@ -628,7 +616,7 @@ impl Loader for Font { | |
} | ||
|
||
#[inline] | ||
unsafe fn from_native_font(native_font: Self::NativeFont) -> Self { | ||
unsafe fn from_native_font(native_font: &Self::NativeFont) -> Self { | ||
Font::from_native_font(native_font) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,6 +61,7 @@ const OPENTYPE_TABLE_TAG_HEAD: u32 = 0x68656164; | |
|
||
/// DirectWrite's representation of a font. | ||
#[allow(missing_debug_implementations)] | ||
#[derive(Clone)] | ||
pub struct NativeFont { | ||
/// The native DirectWrite font object. | ||
pub dwrite_font: DWriteFont, | ||
|
@@ -160,7 +161,8 @@ impl Font { | |
|
||
/// Creates a font from a native API handle. | ||
#[inline] | ||
pub unsafe fn from_native_font(native_font: NativeFont) -> Font { | ||
pub unsafe fn from_native_font(native_font: &NativeFont) -> Font { | ||
let native_font = native_font.clone(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this cloning the font data or just the handle here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one is a bit more involved as we're going through COM (https://github.com/servo/dwrote-rs/blob/master/src/font.rs#L44, https://github.com/servo/dwrote-rs/blob/master/src/font_face.rs#L31), but I'd also say that this is bumping just the refcounts. So we should be good. |
||
Font { | ||
dwrite_font: native_font.dwrite_font, | ||
dwrite_font_face: native_font.dwrite_font_face, | ||
|
@@ -747,7 +749,7 @@ impl Loader for Font { | |
} | ||
|
||
#[inline] | ||
unsafe fn from_native_font(native_font: Self::NativeFont) -> Self { | ||
unsafe fn from_native_font(native_font: &Self::NativeFont) -> Self { | ||
Font::from_native_font(native_font) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -197,9 +197,10 @@ impl Font { | |
} | ||
|
||
/// Creates a font from a native API handle. | ||
pub unsafe fn from_native_font(freetype_face: NativeFont) -> Font { | ||
pub unsafe fn from_native_font(freetype_face: &NativeFont) -> Font { | ||
// We make an in-memory copy of the underlying font data. This is because the native font | ||
// does not necessarily hold a strong reference to the memory backing it. | ||
let freetype_face = *freetype_face; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question effectively here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case we're copying a pointer, so it's just a handle. |
||
const CHUNK_SIZE: usize = 4096; | ||
let mut font_data = vec![]; | ||
loop { | ||
|
@@ -1036,7 +1037,7 @@ impl Loader for Font { | |
} | ||
|
||
#[inline] | ||
unsafe fn from_native_font(native_font: Self::NativeFont) -> Self { | ||
unsafe fn from_native_font(native_font: &Self::NativeFont) -> Self { | ||
Font::from_native_font(native_font) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,7 @@ macro_rules! match_handle { | |
font_index, $index | ||
); | ||
} | ||
Handle::Native { .. } => {} | ||
} | ||
}; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -622,18 +622,6 @@ pub fn get_font_properties() { | |
assert_eq!(properties.stretch, Stretch(1.0)); | ||
} | ||
|
||
#[cfg(feature = "source")] | ||
#[test] | ||
pub fn get_font_data() { | ||
Comment on lines
-625
to
-627
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this test being removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What OS are you on? It fails for me on Mac, iirc due to https://github.com/servo/font-kit/pull/232/files#diff-6ba3732b9cf93e63d757c9bbbe03032cb17ed4525b45153ec0217b7df271ebf1R132 which leads to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Im on Linux There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it's unwise to remove the test. At the very least, we can cfg-gate it. |
||
let font = SystemSource::new() | ||
.select_best_match(&[FamilyName::SansSerif], &Properties::new()) | ||
.unwrap() | ||
.load() | ||
.unwrap(); | ||
let data = font.copy_font_data().unwrap(); | ||
debug_assert!(SFNT_VERSIONS.iter().any(|version| data[0..4] == *version)); | ||
} | ||
|
||
#[cfg(feature = "source")] | ||
#[test] | ||
pub fn load_font_table() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm. Any reason this isn't called
as_native
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, no. There's no particular reason for that, I'll change it. Funnily enough, in doc comments I've referred to it as
as_native
. Oh well. :p