Fix loading raw data from .ttc files on macos (servo#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<u8>`
- 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 <[email protected]>

Author: nicoburns

Cargo.lock

@@ -9,6 +9,13 @@ use style::color::AbsoluteColor;
 use crate::backend::Convert;
 use crate::canvas_data::Filter;
 
+impl Convert<peniko::Font> 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<kurbo::Join> for LineJoinStyle {
     fn convert(self) -> kurbo::Join {
         match self {

components/canvas/peniko_conversions.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);

components/canvas/raqote_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();

components/canvas/vello_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();

components/canvas/vello_cpu_backend.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<FontData>,
+    /// 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<FontDataAndIndex>,
 
     shaper: OnceLock<Shaper>,
     cached_shape_data: RwLock<CachedShapeData>,
@@ -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] {

components/fonts/font.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<IpcSharedMemory>);
-
-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<IpcSharedMemory> {
-        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)]

components/fonts/lib.rs

@@ -410,7 +410,7 @@ enum FreeTypeFaceTableProviderData {
 impl FreeTypeFaceTableProviderData {
     fn font_ref(&self) -> Result<FontRef<'_>, 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),
         }
     }

components/fonts/platform/freetype/font.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<Vec<u8>> {
+    pub(crate) fn font_data_and_index(&self) -> Option<FontDataAndIndex> {
         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,
+        })
     }
 }

components/fonts/platform/freetype/mod.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<Vec<u8>> {
-        // 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<FontDataAndIndex> {
         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
+        },
     }
 }