-
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 all commits
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 |
---|---|---|
|
@@ -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, | ||
} | ||
} | ||
|
||
|
@@ -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.
What the repercussions of no longer having this font data?
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.
Ha, good catch;
Font
implements Deref to [u8] which will panic if there's no data associated: https://github.com/servo/font-kit/blob/master/src/loaders/core_text.rs#L784Granted, this can still happen on master if a font has no path.
Also,
Font::copy_font_data
will return None instead of font contents, but that at least does not panic in surprising ways. I'd say that if the panic and implicit deref to [u8] poses a problem, one can always turn to usingFont::copy_font_data
, which gives a chance to avoid a panic.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.
On the other hand, this PR is already Semver-breaking with a change to
Loader::from_native_font
, so we might as well get rid of the Deref implementation?