diff --git a/crates/fonts/src/fontdb.rs b/crates/fonts/src/fontdb.rs index f0a06efe..426b95c9 100644 --- a/crates/fonts/src/fontdb.rs +++ b/crates/fonts/src/fontdb.rs @@ -245,6 +245,11 @@ pub struct FontDb { /// System fallback state: enabled, and characters no system font covers. #[cfg(not(target_arch = "wasm32"))] sys: Mutex, + /// Held while installed fonts are loaded, so threads that need the same family wait for one + /// load instead of racing it: what a lookup finds doesn't depend on which thread got there + /// first. + #[cfg(not(target_arch = "wasm32"))] + loading: Mutex<()>, } #[cfg(not(target_arch = "wasm32"))] @@ -535,6 +540,8 @@ impl FontDb { cataloged: std::sync::OnceLock::new(), #[cfg(not(target_arch = "wasm32"))] sys: Mutex::new(SysFallback { enabled: true, ..Default::default() }), + #[cfg(not(target_arch = "wasm32"))] + loading: Mutex::new(()), } } @@ -600,14 +607,33 @@ impl FontDb { /// Add a user font (TTF/OTF/TTC bytes). Returns the number of faces added (0 if unparseable or /// every face was already present). pub fn add_font(&self, bytes: Vec) -> usize { - let data = Arc::new(bytes); - let mut added = 0; - for (i, family, style, coords) in enumerate_faces(&data) { - if self.read_faces().iter().any(|f| f.family.eq_ignore_ascii_case(&family) && f.style.eq_ignore_ascii_case(&style)) { - continue; + self.add_fonts(vec![bytes]) + } + + /// Add font files together: lookups see all of their faces or none, never part of a family. + /// Returns the number of faces added. + fn add_fonts(&self, files: Vec>) -> usize { + let present = |faces: &[Arc], family: &str, style: &str| { + faces.iter().any(|f| f.family.eq_ignore_ascii_case(family) && f.style.eq_ignore_ascii_case(style)) + }; + let mut new = Vec::new(); + for bytes in files { + let data = Arc::new(bytes); + for (i, family, style, coords) in enumerate_faces(&data) { + if present(&self.read_faces(), &family, &style) { + continue; + } + if let Some(f) = make_face(FontBytes::Owned(data.clone()), i, family, style, coords) { + new.push(Arc::new(f)); + } } - if let Some(f) = make_face(FontBytes::Owned(data.clone()), i, family, style, coords) { - self.faces.write().unwrap_or_else(|e| e.into_inner()).push(Arc::new(f)); + } + let mut faces = self.faces.write().unwrap_or_else(|e| e.into_inner()); + let mut added = 0; + for f in new { + // Checked again under the lock: another thread may have added the same face. + if !present(&faces, &f.family, &f.style) { + faces.push(f); added += 1; } } @@ -677,9 +703,21 @@ impl FontDb { n } - /// Load the files of the installed `family`. Returns whether any face was added. + /// Load the files of the installed `family` unless it is loaded. Returns whether it is loaded + /// (by this call or any other). #[cfg(not(target_arch = "wasm32"))] fn load_cataloged(&self, family: &str) -> bool { + let loading = self.loading.lock().unwrap_or_else(|e| e.into_inner()); + self.load_family(family, &loading) + } + + /// [`FontDb::load_cataloged`] for a caller already holding `loading`. + #[cfg(not(target_arch = "wasm32"))] + fn load_family(&self, family: &str, _loading: &std::sync::MutexGuard<'_, ()>) -> bool { + // Another thread may have loaded it while this one waited for the lock. + if self.is_loaded(family) { + return true; + } let paths: Vec = { let cat = self.read_catalog(); let mut p: Vec<_> = cat.iter().filter(|c| c.family.eq_ignore_ascii_case(family)).map(|c| c.path.clone()).collect(); @@ -687,13 +725,8 @@ impl FontDb { p.dedup(); p }; - let mut any = false; - for p in paths { - if let Ok(data) = std::fs::read(&p) { - any |= self.add_font(data) > 0; - } - } - any + self.add_fonts(paths.iter().filter_map(|p| std::fs::read(p).ok()).collect()); + self.is_loaded(family) } /// Resolve a family + style to a face, falling back to the closest style of the family, then to @@ -846,6 +879,7 @@ impl FontDb { if c.is_control() || c.is_whitespace() { return false; } + let seen = self.read_faces().len(); { let sys = self.sys.lock().unwrap_or_else(|e| e.into_inner()); if !sys.enabled || sys.misses.contains(&c) { @@ -854,9 +888,18 @@ impl FontDb { } let covered = |db: &FontDb| db.read_faces().iter().any(|f| f.covers(c)); let cataloged: Vec = self.read_catalog().iter().map(|e| e.family.clone()).collect(); + let loading = self.loading.lock().unwrap_or_else(|e| e.into_inner()); + // Another thread may have loaded a face for `c` (faces are only ever added), or found + // there is none, while this one waited. + if self.read_faces().len() != seen && covered(self) { + return true; + } + if self.sys.lock().unwrap_or_else(|e| e.into_inner()).misses.contains(&c) { + return false; + } for fam in SYSTEM_FALLBACKS { if !self.is_loaded(fam) && cataloged.iter().any(|f| f.eq_ignore_ascii_case(fam)) { - self.load_cataloged(fam); + self.load_family(fam, &loading); if covered(self) { return true; } diff --git a/crates/fonts/src/lib.rs b/crates/fonts/src/lib.rs index 94e8081f..2587e7b0 100644 --- a/crates/fonts/src/lib.rs +++ b/crates/fonts/src/lib.rs @@ -152,6 +152,34 @@ mod tests { assert!(!shape(&f, "Hello", &[], |c| c).is_empty()); } + #[test] + fn concurrent_lookups_all_find_an_installed_family() { + // Threads resolving an installed family nobody had loaded yet used to fall back to + // another face when a different thread finished loading it first, so the same text was + // laid out in different fonts depending on timing. + let dir = std::env::temp_dir().join(format!("deckcraft-fonts-concurrent-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("Hack-Regular.ttf"), epaint_default_fonts::HACK_REGULAR).unwrap(); + for _ in 0..20 { + let db = FontDb::with_font_dirs(vec![dir.clone()]); + db.set_system_fallback(false); + let start = std::sync::Barrier::new(8); + let got: Vec = std::thread::scope(|s| { + let threads: Vec<_> = (0..8) + .map(|_| { + s.spawn(|| { + start.wait(); + db.face("Hack", "Regular").family.clone() + }) + }) + .collect(); + threads.into_iter().map(|t| t.join().unwrap()).collect() + }); + assert!(got.iter().all(|f| f == "Hack"), "{got:?}"); + } + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn office_fonts_have_substitutes() { assert!(substitutes("Calibri").contains(&"Carlito"));