From 39629560c80edf0c3ee91e50202a4b8c633614fe Mon Sep 17 00:00:00 2001 From: Nico Burns Date: Tue, 19 Aug 2025 12:57:48 +0100 Subject: [PATCH] Fix loading raw data from `.ttc` files on macos (#38753) # Objective Ensure that functionality which uses the raw font data (such as rendering text to canvas) works correctly on macOS when the specified font is a system font that lives in an OpenType Collection (`.ttc`) file. ## Changes made - The `read_data_from_file` in each backend now returns a `index: u32` in addition to `data: Vec` - The `data` field on the `Font` type has been renamed to `raw` and the `data` method on the `Font` type has been renamed to `raw_font`. This allows the index to be cached as computing is moderately expensive on macOS (on the order of 100 microseconds). - Both of the above now store/return a `struct RawFont` instead of a `FontData` where `RawFont` is defined as `struct RawFont { data: FontData, index: u32 }`. - The users of the `data` method have been updated to use the cached index from `data` rather than calling `.index()` each time. --------- Signed-off-by: Nico Burns --- Cargo.lock | 1 + components/canvas/peniko_conversions.rs | 7 ++ components/canvas/raqote_backend.rs | 7 +- components/canvas/vello_backend.rs | 12 ++-- components/canvas/vello_cpu_backend.rs | 12 ++-- components/fonts/font.rs | 38 +++++----- components/fonts/lib.rs | 27 +------- components/fonts/platform/freetype/font.rs | 2 +- components/fonts/platform/freetype/mod.rs | 11 ++- components/fonts/platform/macos/font_list.rs | 69 ++++++++++++++++--- .../fonts/platform/windows/font_list.rs | 13 ++-- components/shared/fonts/Cargo.toml | 1 + components/shared/fonts/lib.rs | 40 +++++++++++ 13 files changed, 164 insertions(+), 76 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 224c6b78777..8cf016e53f8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2661,6 +2661,7 @@ dependencies = [ name = "fonts_traits" version = "0.0.1" dependencies = [ + "ipc-channel", "malloc_size_of_derive", "range", "serde", diff --git a/components/canvas/peniko_conversions.rs b/components/canvas/peniko_conversions.rs index 61c94bd0adc..607ea942918 100644 --- a/components/canvas/peniko_conversions.rs +++ b/components/canvas/peniko_conversions.rs @@ -9,6 +9,13 @@ use style::color::AbsoluteColor; use crate::backend::Convert; use crate::canvas_data::Filter; +impl Convert for fonts::FontDataAndIndex { + fn convert(self) -> peniko::Font { + use std::sync::Arc; + peniko::Font::new(peniko::Blob::new(Arc::new(self.data)), self.index) + } +} + impl Convert for LineJoinStyle { fn convert(self) -> kurbo::Join { match self { diff --git a/components/canvas/raqote_backend.rs b/components/canvas/raqote_backend.rs index c55767ff7a0..87ed0e3ff8b 100644 --- a/components/canvas/raqote_backend.rs +++ b/components/canvas/raqote_backend.rs @@ -333,8 +333,11 @@ impl GenericDrawTarget for raqote::DrawTarget { SHARED_FONT_CACHE.with(|font_cache| { let identifier = template.identifier(); if !font_cache.borrow().contains_key(&identifier) { - let data = std::sync::Arc::new(run.font.data().as_ref().to_vec()); - let Ok(font) = Font::from_bytes(data, identifier.index()) else { + let Ok(font_data_and_index) = run.font.font_data_and_index() else { + return; + }; + let data = std::sync::Arc::new(font_data_and_index.data.as_ref().to_vec()); + let Ok(font) = Font::from_bytes(data, font_data_and_index.index) else { return; }; font_cache.borrow_mut().insert(identifier.clone(), font); diff --git a/components/canvas/vello_backend.rs b/components/canvas/vello_backend.rs index c766fe51087..7ef8572c422 100644 --- a/components/canvas/vello_backend.rs +++ b/components/canvas/vello_backend.rs @@ -402,13 +402,11 @@ impl GenericDrawTarget for VelloDrawTarget { SHARED_FONT_CACHE.with(|font_cache| { let identifier = template.identifier(); if !font_cache.borrow().contains_key(&identifier) { - font_cache.borrow_mut().insert( - identifier.clone(), - peniko::Font::new( - peniko::Blob::from(run.font.data().as_ref().to_vec()), - identifier.index(), - ), - ); + let Ok(font) = run.font.font_data_and_index() else { + return; + }; + let font = font.clone().convert(); + font_cache.borrow_mut().insert(identifier.clone(), font); } let font_cache = font_cache.borrow(); diff --git a/components/canvas/vello_cpu_backend.rs b/components/canvas/vello_cpu_backend.rs index 7b63bc64641..bde2e03aa88 100644 --- a/components/canvas/vello_cpu_backend.rs +++ b/components/canvas/vello_cpu_backend.rs @@ -305,13 +305,11 @@ impl GenericDrawTarget for VelloCPUDrawTarget { SHARED_FONT_CACHE.with(|font_cache| { let identifier = template.identifier(); if !font_cache.borrow().contains_key(&identifier) { - font_cache.borrow_mut().insert( - identifier.clone(), - peniko::Font::new( - peniko::Blob::from(run.font.data().as_ref().to_vec()), - identifier.index(), - ), - ); + let Ok(font) = run.font.font_data_and_index() else { + return; + }; + let font = font.clone().convert(); + font_cache.borrow_mut().insert(identifier.clone(), font); } let font_cache = font_cache.borrow(); diff --git a/components/fonts/font.rs b/components/fonts/font.rs index 751aeecdba7..03c58e425ec 100644 --- a/components/fonts/font.rs +++ b/components/fonts/font.rs @@ -33,8 +33,8 @@ use crate::platform::font::{FontTable, PlatformFont}; pub use crate::platform::font_list::fallback_font_families; use crate::{ ByteIndex, EmojiPresentationPreference, FallbackFontSelectionOptions, FontContext, FontData, - FontIdentifier, FontTemplateDescriptor, FontTemplateRef, FontTemplateRefMethods, GlyphData, - GlyphId, GlyphStore, LocalFontIdentifier, Shaper, + FontDataAndIndex, FontDataError, FontIdentifier, FontTemplateDescriptor, FontTemplateRef, + FontTemplateRefMethods, GlyphData, GlyphId, GlyphStore, LocalFontIdentifier, Shaper, }; #[macro_export] @@ -254,8 +254,9 @@ pub struct Font { pub metrics: FontMetrics, pub descriptor: FontDescriptor, - /// The data for this font. This might be uninitialized for system fonts. - data: OnceLock, + /// The data for this font. And the index of the font within the data (in case it's a TTC) + /// This might be uninitialized for system fonts. + data_and_index: OnceLock, shaper: OnceLock, cached_shape_data: RwLock, @@ -313,7 +314,9 @@ impl Font { template, metrics, descriptor, - data: data.map(OnceLock::from).unwrap_or_default(), + data_and_index: data + .map(|data| OnceLock::from(FontDataAndIndex { data, index: 0 })) + .unwrap_or_default(), shaper: OnceLock::new(), cached_shape_data: Default::default(), font_instance_key: Default::default(), @@ -348,17 +351,20 @@ impl Font { /// Return the data for this `Font`. Note that this is currently highly inefficient for system /// fonts and should not be used except in legacy canvas code. - pub fn data(&self) -> &FontData { - self.data.get_or_init(|| { - let FontIdentifier::Local(local_font_identifier) = self.identifier() else { - unreachable!("All web fonts should already have initialized data"); - }; - FontData::from_bytes( - &local_font_identifier - .read_data_from_file() - .unwrap_or_default(), - ) - }) + pub fn font_data_and_index(&self) -> Result<&FontDataAndIndex, FontDataError> { + if let Some(data_and_index) = self.data_and_index.get() { + return Ok(data_and_index); + } + + let FontIdentifier::Local(local_font_identifier) = self.identifier() else { + unreachable!("All web fonts should already have initialized data"); + }; + let Some(data_and_index) = local_font_identifier.font_data_and_index() else { + return Err(FontDataError::FailedToLoad); + }; + + let data_and_index = self.data_and_index.get_or_init(move || data_and_index); + Ok(data_and_index) } pub fn variations(&self) -> &[FontVariation] { diff --git a/components/fonts/lib.rs b/components/fonts/lib.rs index 37b1da91724..43804a49a2a 100644 --- a/components/fonts/lib.rs +++ b/components/fonts/lib.rs @@ -14,42 +14,17 @@ pub mod platform; mod shaper; mod system_font_service; -use std::sync::Arc; - pub use font::*; pub use font_context::*; pub use font_store::*; pub use font_template::*; +pub use fonts_traits::*; pub use glyph::*; -use ipc_channel::ipc::IpcSharedMemory; -use malloc_size_of_derive::MallocSizeOf; pub use platform::LocalFontIdentifier; pub use shaper::*; pub use system_font_service::*; use unicode_properties::{EmojiStatus, UnicodeEmoji, emoji}; -/// A data structure to store data for fonts. Data is stored internally in an -/// [`IpcSharedMemory`] handle, so that it can be send without serialization -/// across IPC channels. -#[derive(Clone, MallocSizeOf)] -pub struct FontData(#[conditional_malloc_size_of] pub(crate) Arc); - -impl FontData { - pub fn from_bytes(bytes: &[u8]) -> Self { - Self(Arc::new(IpcSharedMemory::from_bytes(bytes))) - } - - pub(crate) fn as_ipc_shared_memory(&self) -> Arc { - self.0.clone() - } -} - -impl AsRef<[u8]> for FontData { - fn as_ref(&self) -> &[u8] { - &self.0 - } -} - /// Whether or not font fallback selection prefers the emoji or text representation /// of a character. If `None` then either presentation is acceptable. #[derive(Clone, Copy, Debug, PartialEq)] diff --git a/components/fonts/platform/freetype/font.rs b/components/fonts/platform/freetype/font.rs index 617c488df33..8d95cc9e3d3 100644 --- a/components/fonts/platform/freetype/font.rs +++ b/components/fonts/platform/freetype/font.rs @@ -410,7 +410,7 @@ enum FreeTypeFaceTableProviderData { impl FreeTypeFaceTableProviderData { fn font_ref(&self) -> Result, ReadError> { match self { - Self::Web(ipc_shared_memory) => FontRef::new(&ipc_shared_memory.0), + Self::Web(ipc_shared_memory) => FontRef::new(ipc_shared_memory.as_ref()), Self::Local(mmap, index) => FontRef::from_index(mmap, *index), } } diff --git a/components/fonts/platform/freetype/mod.rs b/components/fonts/platform/freetype/mod.rs index feea468008e..09bad600c95 100644 --- a/components/fonts/platform/freetype/mod.rs +++ b/components/fonts/platform/freetype/mod.rs @@ -13,6 +13,8 @@ use serde::{Deserialize, Serialize}; use style::Atom; use webrender_api::NativeFontHandle; +use crate::{FontData, FontDataAndIndex}; + pub mod font; mod freetype_face; @@ -57,9 +59,14 @@ impl LocalFontIdentifier { } } - pub(crate) fn read_data_from_file(&self) -> Option> { + pub(crate) fn font_data_and_index(&self) -> Option { let file = File::open(Path::new(&*self.path)).ok()?; let mmap = unsafe { Mmap::map(&file).ok()? }; - Some(mmap[..].to_vec()) + let data = FontData::from_bytes(&mmap); + + Some(FontDataAndIndex { + data, + index: self.variation_index as u32, + }) } } diff --git a/components/fonts/platform/macos/font_list.rs b/components/fonts/platform/macos/font_list.rs index c7864084de6..922b449737b 100644 --- a/components/fonts/platform/macos/font_list.rs +++ b/components/fonts/platform/macos/font_list.rs @@ -6,9 +6,11 @@ use std::fs::File; use std::path::Path; use base::text::{UnicodeBlock, UnicodeBlockMethod, unicode_plane}; -use log::debug; +use log::{debug, warn}; use malloc_size_of_derive::MallocSizeOf; use memmap2::Mmap; +use read_fonts::types::NameId; +use read_fonts::{FileRef, TableProvider as _}; use serde::{Deserialize, Serialize}; use style::Atom; use style::values::computed::font::GenericFontFamily; @@ -18,8 +20,8 @@ use webrender_api::NativeFontHandle; use crate::platform::add_noto_fallback_families; use crate::platform::font::CoreTextFontTraitsMapping; use crate::{ - EmojiPresentationPreference, FallbackFontSelectionOptions, FontIdentifier, FontTemplate, - FontTemplateDescriptor, LowercaseFontFamilyName, + EmojiPresentationPreference, FallbackFontSelectionOptions, FontData, FontDataAndIndex, + FontIdentifier, FontTemplate, FontTemplateDescriptor, LowercaseFontFamilyName, }; /// An identifier for a local font on a MacOS system. These values comes from the CoreText @@ -43,16 +45,61 @@ impl LocalFontIdentifier { 0 } - pub(crate) fn read_data_from_file(&self) -> Option> { - // TODO: This is incorrect, if the font file is a TTC (collection) with more than - // one font. In that case we either need to reconstruct the pertinent tables into - // a bundle of font data (expensive) or make sure that the value returned by - // `index()` above is correct. The latter is potentially tricky as macOS might not - // do an accurate mapping between the PostScript name that it gives us and what is - // listed in the font. + pub(crate) fn font_data_and_index(&self) -> Option { let file = File::open(Path::new(&*self.path)).ok()?; let mmap = unsafe { Mmap::map(&file).ok()? }; - Some(mmap[..].to_vec()) + + // Determine index + let file_ref = FileRef::new(mmap.as_ref()).ok()?; + let index = ttc_index_from_postscript_name(file_ref, &self.postscript_name); + + Some(FontDataAndIndex { + data: FontData::from_bytes(&mmap), + index, + }) + } +} + +/// CoreText font enumeration gives us a Postscript name rather than an index. +/// This functions maps from a Postscript name to an index. +/// +/// This mapping works for single-font files and for simple TTC files, but may not work in all cases. +/// We are not 100% sure which cases (if any) will not work. But we suspect that variable fonts may cause +/// issues due to the Postscript names corresponding to instances not being straightforward, and the possibility +/// that CoreText may return a non-standard in that scenerio. +fn ttc_index_from_postscript_name(font_file: FileRef<'_>, postscript_name: &str) -> u32 { + match font_file { + // File only contains one font: simply return 0 + FileRef::Font(_) => 0, + // File is a collection: iterate through each font in the collection and check + // whether the name matches + FileRef::Collection(collection) => { + for i in 0..collection.len() { + let font = collection.get(i).unwrap(); + let name_table = font.name().unwrap(); + if name_table + .name_record() + .iter() + .filter(|record| record.name_id() == NameId::POSTSCRIPT_NAME) + .any(|record| { + record + .string(name_table.string_data()) + .unwrap() + .chars() + .eq(postscript_name.chars()) + }) + { + return i; + } + } + + // If we fail to find a font, just use the first font in the file. + warn!( + "Font with postscript_name {} not found in collection", + postscript_name + ); + 0 + }, } } diff --git a/components/fonts/platform/windows/font_list.rs b/components/fonts/platform/windows/font_list.rs index e9cdaac8562..6fdc2eac7c8 100644 --- a/components/fonts/platform/windows/font_list.rs +++ b/components/fonts/platform/windows/font_list.rs @@ -15,8 +15,8 @@ use style::values::specified::font::FontStretchKeyword; use webrender_api::NativeFontHandle; use crate::{ - EmojiPresentationPreference, FallbackFontSelectionOptions, FontIdentifier, FontTemplate, - FontTemplateDescriptor, LowercaseFontFamilyName, + EmojiPresentationPreference, FallbackFontSelectionOptions, FontData, FontDataAndIndex, + FontIdentifier, FontTemplate, FontTemplateDescriptor, LowercaseFontFamilyName, }; pub fn for_each_available_family(mut callback: F) @@ -67,14 +67,19 @@ impl LocalFontIdentifier { } } - pub(crate) fn read_data_from_file(&self) -> Option> { + pub(crate) fn font_data_and_index(&self) -> Option { let font = FontCollection::system() .font_from_descriptor(&self.font_descriptor) .ok()??; let face = font.create_font_face(); + let index = face.get_index(); let files = face.get_files(); assert!(!files.is_empty()); - Some(files[0].get_font_file_bytes()) + + let data = files[0].get_font_file_bytes(); + let data = FontData::from_bytes(&data); + + Some(FontDataAndIndex { data, index }) } } diff --git a/components/shared/fonts/Cargo.toml b/components/shared/fonts/Cargo.toml index 25eb260b525..d2fda9441da 100644 --- a/components/shared/fonts/Cargo.toml +++ b/components/shared/fonts/Cargo.toml @@ -12,6 +12,7 @@ name = "fonts_traits" path = "lib.rs" [dependencies] +ipc-channel = { workspace = true } malloc_size_of = { workspace = true } malloc_size_of_derive = { workspace = true } range = { path = "../../range" } diff --git a/components/shared/fonts/lib.rs b/components/shared/fonts/lib.rs index fa55e40f655..d56c63dbde1 100644 --- a/components/shared/fonts/lib.rs +++ b/components/shared/fonts/lib.rs @@ -6,6 +6,7 @@ use std::sync::Arc; +use ipc_channel::ipc::IpcSharedMemory; use malloc_size_of_derive::MallocSizeOf; use range::{RangeIndex, int_range_index}; use serde::{Deserialize, Serialize}; @@ -18,3 +19,42 @@ int_range_index! { } pub type StylesheetWebFontLoadFinishedCallback = Arc; + +/// A data structure to store data for fonts. Data is stored internally in an +/// [`IpcSharedMemory`] handle, so that it can be sent without serialization +/// across IPC channels. +#[derive(Clone, MallocSizeOf)] +pub struct FontData(#[conditional_malloc_size_of] pub(crate) Arc); + +impl FontData { + pub fn from_bytes(bytes: &[u8]) -> Self { + Self(Arc::new(IpcSharedMemory::from_bytes(bytes))) + } + + pub fn as_ipc_shared_memory(&self) -> Arc { + self.0.clone() + } +} + +impl AsRef<[u8]> for FontData { + fn as_ref(&self) -> &[u8] { + &self.0 + } +} + +/// Raw font data and an index +/// +/// If the font data is of a TTC (TrueType collection) file, then the index of a specific font within +/// the collection. If the font data is for is single font then the index will always be 0. +#[derive(Clone)] +pub struct FontDataAndIndex { + /// The raw font file data (.ttf, .otf, .ttc, etc) + pub data: FontData, + /// The index of the font within the file (0 if the file is not a ttc) + pub index: u32, +} + +#[derive(Copy, Clone, PartialEq)] +pub enum FontDataError { + FailedToLoad, +}