eliott/servo
1
0
Fork 0
mirror of https://github.com/servo/servo synced 2026-04-25 17:15:48 +02:00
Code Issues Packages Projects Releases Wiki Activity

fonts: Refactor the font fallback code a little bit and remove RwLock around FontGroup (#41449)

Browse Source 
This change reworks the logic for finding font fallbacks to make it
simpler. I was involved with writing the code for `FontGroupFamily` and
`FontGroupFamilyMember` and I still struggle a bit with understanding
it, so I've chosen to do this. In addition, the change is in preparation
for more flexible font fallback (#41426).

The main changes here are:
 1. Move the logic for creating a new descriptor with variations into
    the Font constructor.
 2. Add some more general methods to `FontGroupFamily` such as a
    template iterator.
 3. Use `OnceLock` to avoid a convoluted code structure because of
    mutability and also having boolean "loaded" members. This is what
    `OnceLock` and `OnceCell` are for!
 4. Rename `FontGroupFamilyMember` to `FontGroupFamilyTemplate` to
    stress that it is one template of a particular `FontGroupFamily`.

Testing: This should not change behavior so is covered by existing WPT
tests.

---------

Signed-off-by: Martin Robinson <mrobinson@igalia.com>
This commit is contained in:
Martin Robinson
2025-12-22 16:06:25 +01:00
committed by GitHub
parent 1469c5a3a1
commit 55c4f579c9
7 changed files with 156 additions and 170 deletions
Download Patch File Download Diff File Expand all files Collapse all files

225
components/fonts/font.rs
View File

@@ -602,7 +602,7 @@ impl FontGroup {
.font_family
.families
.iter()
.map(FontGroupFamily::new)
.map(FontGroupFamily::local_or_web)
.collect();
FontGroup {
@@ -616,7 +616,7 @@ impl FontGroup {
/// (which will cause a "glyph not found" character to be rendered). If no font at all can be
/// found, returns None.
pub fn find_by_codepoint(
&mut self,
&self,
font_context: &FontContext,
codepoint: char,
next_codepoint: Option<char>,
@@ -661,8 +661,8 @@ impl FontGroup {
if let Some(font) = self.find(
font_context,
char_in_template,
font_has_glyph_and_presentation,
&char_in_template,
&font_has_glyph_and_presentation,
) {
return font_or_synthesized_small_caps(font);
}
@@ -678,8 +678,8 @@ impl FontGroup {
if let Some(font) = self.find_fallback(
font_context,
options.clone(),
char_in_template,
font_has_glyph_and_presentation,
&char_in_template,
&font_has_glyph_and_presentation,
) {
return font_or_synthesized_small_caps(font);
}
@@ -688,7 +688,7 @@ impl FontGroup {
}
/// Find the first available font in the group, or the first available fallback font.
pub fn first(&mut self, font_context: &FontContext) -> Option<FontRef> {
pub fn first(&self, font_context: &FontContext) -> Option<FontRef> {
// From https://drafts.csswg.org/css-fonts/#first-available-font:
// > The first available font, used for example in the definition of font-relative lengths
// > such as ex or in the definition of the line-height property, is defined to be the first
@@ -698,13 +698,13 @@ impl FontGroup {
// > Note: it does not matter whether that font actually has a glyph for the space character.
let space_in_template = |template: FontTemplateRef| template.char_in_unicode_range(' ');
let font_predicate = |_: &FontRef| true;
self.find(font_context, space_in_template, font_predicate)
self.find(font_context, &space_in_template, &font_predicate)
.or_else(|| {
self.find_fallback(
font_context,
FallbackFontSelectionOptions::default(),
space_in_template,
font_predicate,
&space_in_template,
&font_predicate,
)
})
}
@@ -712,42 +712,36 @@ impl FontGroup {
/// Attempts to find a font which matches the given `template_predicate` and `font_predicate`.
/// This method mutates because we may need to load new font data in the process of finding
/// a suitable font.
fn find<TemplatePredicate, FontPredicate>(
&mut self,
fn find(
&self,
font_context: &FontContext,
template_predicate: TemplatePredicate,
font_predicate: FontPredicate,
) -> Option<FontRef>
where
TemplatePredicate: Fn(FontTemplateRef) -> bool,
FontPredicate: Fn(&FontRef) -> bool,
{
let font_descriptor = self.descriptor.clone();
self.families.iter_mut().find_map(|font_group_family| {
font_group_family.find(
&font_descriptor,
font_context,
&template_predicate,
&font_predicate,
)
})
template_predicate: &impl Fn(FontTemplateRef) -> bool,
font_predicate: &impl Fn(&FontRef) -> bool,
) -> Option<FontRef> {
self.families
.iter()
.flat_map(|family| family.templates(font_context, &self.descriptor))
.find_map(|template| {
template.font_if_matches(
font_context,
&self.descriptor,
template_predicate,
font_predicate,
)
})
}
/// Attempts to find a suitable fallback font which matches the given `template_predicate` and
/// `font_predicate`. The default family (i.e. "serif") will be tried first, followed by
/// platform-specific family names. If a `codepoint` is provided, then its Unicode block may be
/// used to refine the list of family names which will be tried.
fn find_fallback<TemplatePredicate, FontPredicate>(
&mut self,
fn find_fallback(
&self,
font_context: &FontContext,
options: FallbackFontSelectionOptions,
template_predicate: TemplatePredicate,
font_predicate: FontPredicate,
) -> Option<FontRef>
where
TemplatePredicate: Fn(FontTemplateRef) -> bool,
FontPredicate: Fn(&FontRef) -> bool,
{
template_predicate: &impl Fn(FontTemplateRef) -> bool,
font_predicate: &impl Fn(&FontRef) -> bool,
) -> Option<FontRef> {
iter::once(FontFamilyDescriptor::default())
.chain(
fallback_font_families(options)
@@ -761,32 +755,67 @@ impl FontGroup {
}),
)
.find_map(|family_descriptor| {
FontGroupFamily {
family_descriptor,
members: None,
}
.find(
&self.descriptor,
font_context,
&template_predicate,
&font_predicate,
)
FontGroupFamily::from(family_descriptor)
.templates(font_context, &self.descriptor)
.find_map(|template| {
template.font_if_matches(
font_context,
&self.descriptor,
template_predicate,
font_predicate,
)
})
})
}
}
/// A [`FontGroupFamily`] can have multiple members if it is a "composite face", meaning
/// that it is defined by multiple `@font-face` declarations which vary only by their
/// `unicode-range` descriptors. In this case, font selection will select a single member
/// that contains the necessary unicode character. Unicode ranges are specified by the
/// [`FontGroupFamilyMember::template`] member.
/// A [`FontGroupFamily`] can have multiple associated `FontTemplate`s if it is a
/// "composite face", meaning that it is defined by multiple `@font-face`
/// declarations which vary only by their `unicode-range` descriptors. In this case,
/// font selection will select a single member that contains the necessary unicode
/// character. Unicode ranges are specified by the [`FontGroupFamilyTemplate::template`]
/// member.
#[derive(MallocSizeOf)]
struct FontGroupFamilyMember {
struct FontGroupFamilyTemplate {
#[ignore_malloc_size_of = "This measured in the FontContext template cache."]
template: FontTemplateRef,
#[ignore_malloc_size_of = "This measured in the FontContext font cache."]
font: Option<FontRef>,
loaded: bool,
font: OnceLock<Option<FontRef>>,
}
impl From<FontTemplateRef> for FontGroupFamilyTemplate {
fn from(template: FontTemplateRef) -> Self {
Self {
template,
font: Default::default(),
}
}
}
impl FontGroupFamilyTemplate {
fn font(
&self,
font_context: &FontContext,
font_descriptor: &FontDescriptor,
) -> Option<FontRef> {
self.font
.get_or_init(|| font_context.font(self.template.clone(), font_descriptor))
.clone()
}
fn font_if_matches(
&self,
font_context: &FontContext,
font_descriptor: &FontDescriptor,
template_predicate: &impl Fn(FontTemplateRef) -> bool,
font_predicate: &impl Fn(&FontRef) -> bool,
) -> Option<FontRef> {
if !template_predicate(self.template.clone()) {
return None;
}
self.font(font_context, font_descriptor)
.filter(font_predicate)
}
}
/// A `FontGroupFamily` is a single font family in a `FontGroup`. It corresponds to one of the
@@ -796,75 +825,37 @@ struct FontGroupFamilyMember {
#[derive(MallocSizeOf)]
struct FontGroupFamily {
family_descriptor: FontFamilyDescriptor,
members: Option<Vec<FontGroupFamilyMember>>,
members: OnceLock<Vec<FontGroupFamilyTemplate>>,
}
impl From<FontFamilyDescriptor> for FontGroupFamily {
fn from(family_descriptor: FontFamilyDescriptor) -> Self {
Self {
family_descriptor,
members: Default::default(),
}
}
}
impl FontGroupFamily {
fn new(family: &SingleFontFamily) -> FontGroupFamily {
FontGroupFamily {
family_descriptor: FontFamilyDescriptor::new(family.clone(), FontSearchScope::Any),
members: None,
}
fn local_or_web(family: &SingleFontFamily) -> FontGroupFamily {
FontFamilyDescriptor::new(family.clone(), FontSearchScope::Any).into()
}
fn find<TemplatePredicate, FontPredicate>(
&mut self,
font_descriptor: &FontDescriptor,
fn templates(
&self,
font_context: &FontContext,
template_predicate: &TemplatePredicate,
font_predicate: &FontPredicate,
) -> Option<FontRef>
where
TemplatePredicate: Fn(FontTemplateRef) -> bool,
FontPredicate: Fn(&FontRef) -> bool,
{
self.members(font_descriptor, font_context)
.find_map(|member| {
if !template_predicate(member.template.clone()) {
return None;
}
if !member.loaded {
if servo_config::pref!(layout_variable_fonts_enabled) {
let variation_settings =
member.template.borrow().compute_variations(font_descriptor);
let descriptor_with_variations =
font_descriptor.with_variation_settings(variation_settings);
member.font =
font_context.font(member.template.clone(), &descriptor_with_variations);
} else {
member.font = font_context.font(member.template.clone(), font_descriptor)
}
member.loaded = true;
}
if matches!(&member.font, Some(font) if font_predicate(font)) {
return member.font.clone();
}
None
font_descriptor: &FontDescriptor,
) -> impl Iterator<Item = &FontGroupFamilyTemplate> {
self.members
.get_or_init(|| {
font_context
.matching_templates(font_descriptor, &self.family_descriptor)
.into_iter()
.map(Into::into)
.collect()
})
}
fn members(
&mut self,
font_descriptor: &FontDescriptor,
font_context: &FontContext,
) -> impl Iterator<Item = &mut FontGroupFamilyMember> {
let family_descriptor = &self.family_descriptor;
let members = self.members.get_or_insert_with(|| {
font_context
.matching_templates(font_descriptor, family_descriptor)
.into_iter()
.map(|template| FontGroupFamilyMember {
template,
loaded: false,
font: None,
})
.collect()
});
members.iter_mut()
.iter()
}
}

29
components/fonts/font_context.rs
View File

@@ -5,7 +5,6 @@
use std::collections::{HashMap, HashSet};
use std::default::Default;
use std::hash::{Hash, Hasher};
use std::ops::Deref;
use std::sync::Arc;
use std::sync::atomic::{AtomicBool, Ordering};
@@ -60,15 +59,7 @@ pub(crate) struct FontParameters {
pub(crate) flags: FontInstanceFlags,
}
#[derive(MallocSizeOf)]
struct FontGroupRef(#[conditional_malloc_size_of] Arc<RwLock<FontGroup>>);
impl Deref for FontGroupRef {
type Target = Arc<RwLock<FontGroup>>;
fn deref(&self) -> &Self::Target {
&self.0
}
}
pub type FontGroupRef = Arc<FontGroup>;
/// The FontContext represents the per-thread/thread state necessary for
/// working with fonts. It is the public API used by the layout and
@@ -91,6 +82,7 @@ pub struct FontContext {
/// A caching map between the specification of a font in CSS style and
/// resolved [`FontGroup`] which contains information about all fonts that
/// can be selected with that style.
#[conditional_malloc_size_of]
resolved_font_groups: RwLock<HashMap<FontGroupCacheKey, FontGroupRef>>,
web_fonts: CrossThreadFontStore,
@@ -175,7 +167,7 @@ impl FontContext {
/// Returns a `FontGroup` representing fonts which can be used for layout, given the `style`.
/// Font groups are cached, so subsequent calls with the same `style` will return a reference
/// to an existing `FontGroup`.
pub fn font_group(&self, style: ServoArc<FontStyleStruct>) -> Arc<RwLock<FontGroup>> {
pub fn font_group(&self, style: ServoArc<FontStyleStruct>) -> FontGroupRef {
let font_size = style.font_size.computed_size().into();
self.font_group_with_size(style, font_size)
}
@@ -186,19 +178,19 @@ impl FontContext {
&self,
style: ServoArc<FontStyleStruct>,
size: Au,
) -> Arc<RwLock<FontGroup>> {
) -> Arc<FontGroup> {
let cache_key = FontGroupCacheKey { size, style };
if let Some(font_group) = self.resolved_font_groups.read().get(&cache_key) {
return font_group.0.clone();
return font_group.clone();
}
let mut descriptor = FontDescriptor::from(&*cache_key.style);
descriptor.pt_size = size;
let font_group = Arc::new(RwLock::new(FontGroup::new(&cache_key.style, descriptor)));
let font_group = Arc::new(FontGroup::new(&cache_key.style, descriptor));
self.resolved_font_groups
.write()
.insert(cache_key, FontGroupRef(font_group.clone()));
.insert(cache_key, font_group.clone());
font_group
}
@@ -209,6 +201,13 @@ impl FontContext {
font_template: FontTemplateRef,
font_descriptor: &FontDescriptor,
) -> Option<FontRef> {
let font_descriptor = if servo_config::pref!(layout_variable_fonts_enabled) {
let variation_settings = font_template.borrow().compute_variations(font_descriptor);
&font_descriptor.with_variation_settings(variation_settings)
} else {
font_descriptor
};
self.get_font_maybe_synthesizing_small_caps(
font_template,
font_descriptor,

25
components/fonts/tests/font_context.rs
View File

@@ -238,28 +238,16 @@ mod font_context {
assert!(
std::ptr::eq(
&*context
.context
.font_group(ServoArc::new(style1.clone()))
.read(),
&*context
.context
.font_group(ServoArc::new(style1.clone()))
.read()
&*context.context.font_group(ServoArc::new(style1.clone())),
&*context.context.font_group(ServoArc::new(style1.clone())),
),
"the same font group should be returned for two styles with the same hash"
);
assert!(
!std::ptr::eq(
&*context
.context
.font_group(ServoArc::new(style1.clone()))
.read(),
&*context
.context
.font_group(ServoArc::new(style2.clone()))
.read()
&*context.context.font_group(ServoArc::new(style1.clone())),
&*context.context.font_group(ServoArc::new(style2.clone())),
),
"different font groups should be returned for two styles with different hashes"
)
@@ -275,7 +263,6 @@ mod font_context {
let group = context.context.font_group(ServoArc::new(style));
let font = group
.write()
.find_by_codepoint(&mut context.context, 'a', None, None, None)
.unwrap();
assert_eq!(&font_face_name(&font.identifier()), "csstest-ascii");
@@ -289,7 +276,6 @@ mod font_context {
);
let font = group
.write()
.find_by_codepoint(&mut context.context, 'a', None, None, None)
.unwrap();
assert_eq!(&font_face_name(&font.identifier()), "csstest-ascii");
@@ -303,7 +289,6 @@ mod font_context {
);
let font = group
.write()
.find_by_codepoint(&mut context.context, 'á', None, None, None)
.unwrap();
assert_eq!(&font_face_name(&font.identifier()), "csstest-basic-regular");
@@ -327,7 +312,6 @@ mod font_context {
let group = context.context.font_group(ServoArc::new(style));
let font = group
.write()
.find_by_codepoint(&mut context.context, 'a', None, None, None)
.unwrap();
assert_eq!(
@@ -337,7 +321,6 @@ mod font_context {
);
let font = group
.write()
.find_by_codepoint(&mut context.context, 'á', None, None, None)
.unwrap();
assert_eq!(

12
components/layout/flow/inline/text_run.rs
View File

@@ -480,7 +480,7 @@ impl TextRun {
let lang = parent_style.get_font()._x_lang.clone();
let Some(font) = font_group.write().find_by_codepoint(
let Some(font) = font_group.find_by_codepoint(
&layout_context.font_context,
character,
next_character,
@@ -516,12 +516,9 @@ impl TextRun {
// Either we have a current segment or we only had control character and whitespace. In both
// of those cases, just use the first font.
if current.is_none() {
current = font_group
.write()
.first(&layout_context.font_context)
.map(|font| {
TextRunSegment::new(font, Script::Common, Level::ltr(), self.text_range.start)
})
current = font_group.first(&layout_context.font_context).map(|font| {
TextRunSegment::new(font, Script::Common, Level::ltr(), self.text_range.start)
})
}
// Extend the last segment to the end of the string and add it to the results.
@@ -583,7 +580,6 @@ pub(super) fn get_font_for_first_font_for_style(
) -> Option<FontRef> {
let font = font_context
.font_group(style.clone_font())
.write()
.first(font_context);
if font.is_none() {
warn!("Could not find font for style: {:?}", style.clone_font());

3
components/layout/layout_impl.rs
View File

@@ -1534,7 +1534,6 @@ impl FontMetricsProvider for LayoutFontMetricsProvider {
.font_group_with_size(ServoArc::new(font.clone()), base_size.into());
let Some(first_font_metrics) = font_group
.write()
.first(font_context)
.map(|font| font.metrics.clone())
else {
@@ -1551,7 +1550,6 @@ impl FontMetricsProvider for LayoutFontMetricsProvider {
.zero_horizontal_advance
.or_else(|| {
font_group
.write()
.find_by_codepoint(font_context, '0', None, None, None)?
.metrics
.zero_horizontal_advance
@@ -1562,7 +1560,6 @@ impl FontMetricsProvider for LayoutFontMetricsProvider {
.ic_horizontal_advance
.or_else(|| {
font_group
.write()
.find_by_codepoint(font_context, '\u{6C34}', None, None, None)?
.metrics
.ic_horizontal_advance

24
components/malloc_size_of/lib.rs
View File

@@ -51,7 +51,7 @@ use std::collections::BinaryHeap;
use std::hash::{BuildHasher, Hash};
use std::ops::Range;
use std::rc::Rc;
use std::sync::Arc;
use std::sync::{Arc, OnceLock};
use resvg::usvg::fontdb::Source;
use style::properties::ComputedValues;
@@ -580,6 +580,22 @@ impl<T: MallocConditionalSizeOf> MallocConditionalSizeOf for OnceCell<T> {
}
}
impl<T: MallocSizeOf> MallocSizeOf for OnceLock<T> {
fn size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
self.get()
.map(|interior| interior.size_of(ops))
.unwrap_or_default()
}
}
impl<T: MallocConditionalSizeOf> MallocConditionalSizeOf for OnceLock<T> {
fn conditional_size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
self.get()
.map(|interior| interior.conditional_size_of(ops))
.unwrap_or_default()
}
}
// See https://github.com/rust-lang/rust/issues/68318:
// We don't want MallocSizeOf to be defined for Rc and Arc. If negative trait bounds are
// ever allowed, this code should be uncommented. Instead, there is a compile-fail test for
@@ -697,6 +713,12 @@ impl<T: MallocSizeOf> MallocSizeOf for parking_lot::RwLock<T> {
}
}
impl<T: MallocConditionalSizeOf> MallocConditionalSizeOf for parking_lot::RwLock<T> {
fn conditional_size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
(*self.read()).conditional_size_of(ops)
}
}
impl<T: MallocSizeOf, Unit> MallocSizeOf for euclid::Length<T, Unit> {
fn size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
self.0.size_of(ops)

8
components/script/dom/canvas/2d/canvas_state.rs
View File

@@ -1495,11 +1495,10 @@ impl CanvasState {
let font_style = self.font_style();
let font_group = font_context.font_group(font_style.clone());
let mut font_group = font_group.write();
let font = font_group.first(font_context).expect("couldn't find font");
let ascent = font.metrics.ascent.to_f64_px();
let descent = font.metrics.descent.to_f64_px();
let runs = self.build_unshaped_text_runs(font_context, &text, &mut font_group);
let runs = self.build_unshaped_text_runs(font_context, &text, &font_group);
let mut total_advance = 0.0;
let shaped_runs: Vec<_> = runs
@@ -2273,13 +2272,12 @@ impl CanvasState {
// > attribute.
let font_style = self.font_style();
let font_group = font_context.font_group_with_size(font_style, Au::from_f64_px(size));
let mut font_group = font_group.write();
let Some(first_font) = font_group.first(font_context) else {
warn!("Could not render canvas text, because there was no first font.");
return None;
};
let runs = self.build_unshaped_text_runs(font_context, &text, &mut font_group);
let runs = self.build_unshaped_text_runs(font_context, &text, &font_group);
// TODO: This doesn't do any kind of line layout at all. In particular, there needs
// to be some alignment along a baseline and also support for bidi text.
@@ -2340,7 +2338,7 @@ impl CanvasState {
&self,
font_context: &FontContext,
text: &'text str,
font_group: &mut FontGroup,
font_group: &FontGroup,
) -> Vec<UnshapedTextRun<'text>> {
let mut runs = Vec::new();
let mut current_text_run = UnshapedTextRun::default();