Make FontResolver a trait and use to provide access to a fontdb

[dhardy]

Summary

usvg::FontResolver is changed from a struct to a trait, with a default implementation DefaultFontResolver.

usvg::Options and usvg::Tree no longer contain fontdb; instead then FontResolver provides access as required.

Motivation

The existing model:

• Requires that the fontdb be shared under an Arc, which shouldn't be necessary
• Clones the fontdb before modifying
• Makes the modified fontdb accessible through usvg::Tree, which doesn't appear to be the right place for it

Instead, the user is expected to provide a &dyn FontResolver instance in order to process any SVG involving text. As a consequence:

• The user may use an existing fontdb instance (without cloning) and may even insert additional fonts directly into this DB (via RefCell, Mutex or similar)
• The user may retrieve the original fontdb instance used without Arc
• The minimal set-up with working text processing is slightly more complex (and unfortunately runs into a borrow-checker error if font_resolver is not declared before usvg::Options).

[RazrFalcon]

@LaurenzV

[RazrFalcon]

Do I understand correctly that your approach doesn't allow sharing fontdb between threads? That's one of the requirements.

[LaurenzV]

I'll delegate to @laurmaedje. :p

[dhardy]

Do I understand correctly that your approach doesn't allow sharing fontdb between threads? That's one of the requirements.

I think it should but haven't tested. Specifically, there is a Sync bound on FontResolver (to make&dyn FontResolver: Send) and the user may implement any type of lock they wish.

DefaultFontResolver is not really suitable for this, but it doesn't need to be.

[RazrFalcon]

DefaultFontResolver is not really suitable for this, but it doesn't need to be.

Both the current and the old implementations allowed sharing fontdb between threads. But in this patch you're simply cloning fontdb in crates/resvg/tests/integration/main.rs This is far from ideal.

[dhardy]

Fixed: there's no longer a need for DefaultFontResolver since we can just make Database implement FontResolver (avoiding the need for a variant over a &Database).

I think the previous code would have cloned the DB anyway when calling Options::fontdb_mut in order to call the resolvers (which needed &mut Database passed even when they only used &Database)?

Certainly now it shouldn't be cloning the Database.

[LaurenzV]

Just so you know I briefly talked to @laurmaedje and he said he would comment why he implemented it the way he did, although I'm not sure when he will get to it. But might be good to wait a bit before merging.

[RazrFalcon]

Ok, will wait then. I do not want to break typst again.

[laurmaedje]

For context, the current implementation comes from https://github.com/RazrFalcon/resvg/pull/769. This implementation was preceded by the earlier PR https://github.com/RazrFalcon/resvg/pull/754.

The design in this PR is closer to #754 than #769. The discussion of that PR provides some insights into problems, but I'll try to summarize things here.

The main conceptual problem, from my understanding, is how support for fonts in embedded in SVGs would work. In the current model, since usvg manages the database, it can mutate it itself. The Arc optimizes for the common case, but conceptually each SVG has its own database and that's also why it ends up in the tree.

On another note, if I'm not mistaken the implementation in this PR doesn't really allow for lazy font loading. Since both select_font and select_fallback are &self, we'd have to resort to interior mutability on the Database, but that's incompatible with the method fontdb(&self) -> &Database. There is some discussion about this problem in #754, but all workarounds I found are ugly.

If I may ask: Which concrete use case is the motivation for changing things again? The Arc is very cheap and the deep-clone of the Database only happens if you both provide fonts up-front and on-demand. And even then, cloning a Database isn't that costly since the font data itself is reference counted internally or memory-mapped on-demand. Moreover, while having the fontdb in the usvg::Tree seems a bit weird perhaps, it makes the user-facing API much simpler and harder to use in the wrong way.

The one somewhat costly thing I can see is that if you parse a bunch of SVGs and lazy load, the fontdb is populated over and over again. But you can cache things in the background and keep that relatively cheap (in the grand scheme of what usvg does I would be surprised if this was a large factor). Moreover, it helps a bit with correctness because in the current design it's harder to mess things up in a way where parsing of one SVG affects font selection of another SVG (because of an already populated database).

[RazrFalcon]

Moreover, while having the fontdb in the usvg::Tree seems a bit weird perhaps, it makes the user-facing API much simpler and harder to use in the wrong way.

Another reason for this is that in the future we will support fonts embedded in CSS, which must not be added to the "global" fontdb. That's why we need an ability to deeply clone the database on-demand.

[dhardy]

On another note, if I'm not mistaken the implementation in this PR doesn't really allow for lazy font loading. Since both select_font and select_fallback are &self, we'd have to resort to interior mutability on the Database,

The intention is that a wrapper type be used to manage that lock... which also requires an associated type (something I forgot to add previously)...

pub trait FontResolver: Debug + Sync {
    type DbGuard<'a>: std::ops::Deref<Target = Database>;
    fn fontdb(&self) -> Self::DbGuard<'_>;
    // ...
}

#[derive(Debug)]
struct LockingFontResolver {
    db: std::sync::Mutex<Database>,
}
impl FontResolver for LockingFontResolver {
    type DbGuard<'a> = std::sync::MutexGuard<'a, Database>;
    fn fontdb(&self) -> Self::DbGuard<'_> {
        self.db.lock().unwrap()
    }
}

... and thus usage of dyn FontResolver becomes problematic. (We probably don't want to add generic parameters to all usages.)

A better option might be to do this:

pub trait FontResolver: Debug + Sync {
    fn with_fontdb_dyn(&self, f: Box<dyn FnOnce(&Database) + '_>);
    // ...
}

pub trait FontResolverExt: FontResolver {
    fn with_fontdb<R>(&self, f: impl (FnOnce(&Database) -> R)) -> Option<R> {
        let mut result = None;
        let out = &mut result;
        self.with_fontdb_dyn(Box::new(|db| *out = Some(f(db))));
        result
    }
}
impl<FR: FontResolver + ?Sized> FontResolverExt for FR {}

Then we can still use dyn FontResolver:

pub(crate) fn convert(text: &mut Text, resolver: &dyn FontResolver) -> Option<()> {
    let (text_fragments, bbox) = layout::layout_text(text, resolver)?;
    text.layouted = text_fragments;
    text.bounding_box = bbox.to_rect();
    text.abs_bounding_box = bbox.transform(text.abs_transform)?.to_rect();

    let (group, stroke_bbox) = resolver.with_fontdb(|fontdb| flatten::flatten(text, fontdb))??;
    text.flattened = Box::new(group);
    text.stroke_bounding_box = stroke_bbox.to_rect();
    text.abs_stroke_bounding_box = stroke_bbox.transform(text.abs_transform)?.to_rect();

    Some(())
}

[dhardy]

Another reason for this is that in the future we will support fonts embedded in CSS, which must not be added to the "global" fontdb. That's why we need an ability to deeply clone the database on-demand.

Admittedly, this is a big motivation to keep the existing design (passing Arc<Database> in). Especially if you have another mechanism for caching loaded fonts.

[laurmaedje]

The intention is that a wrapper type be used to manage that lock... which also requires an associated type (something I forgot to add previously)...

... and thus usage of dyn FontResolver becomes problematic. (We probably don't want to add generic parameters to all usages.)

I went down the same path of thoughts during #754 and ended up with a very similar approach here, but I think it's far from pretty.