Repository navigation
[read-fonts] a new Font API - #1852
Conversation
Where we open the door to a more ergonomic API that supports sanitization, caching, sourcing data from platform native font handles and nicer interop for skrifa/harfrust.
| fn try_load<T>( | ||
| &self, | ||
| sanitize_fn: impl Fn(&'a [u8]) -> Result<T, ReadError>, | ||
| load_fn: impl Fn(&'a [u8]) -> Result<T, ReadError>, | ||
| ) -> Result<T, ReadError> { | ||
| match self.flag.load(Ordering::Acquire) { | ||
| Self::UNCHECKED => { | ||
| if let Ok(table) = sanitize_fn(self.data) { | ||
| self.flag.store(Self::VALID, Ordering::Release); | ||
| Ok(table) | ||
| } else { | ||
| self.flag.store(Self::INVALID, Ordering::Release); | ||
| Err(ReadError::ValidationError) | ||
| } | ||
| } | ||
| Self::VALID => load_fn(self.data), | ||
| // Assume any other bit pattern is invalid | ||
| _ => Err(ReadError::ValidationError), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the hook for tracking sanitize state.
| fn load_table_with_tag<'a, T: FontRead<'a>>( | ||
| &'a self, | ||
| state: Option<TableState<'a>>, | ||
| tag: Tag, | ||
| ) -> Result<T, ReadError> { | ||
| let state = state.ok_or(ReadError::TableIsMissing(tag))?; | ||
| state.try_load( | ||
| |data| FontRead::read(data.into()), | ||
| |data| FontRead::read(data.into()), | ||
| ) | ||
| } |
There was a problem hiding this comment.
And this is where try_load is called. Since this isn't built on the sanitize branch, we currently just pass FontRead::read for both the sanitize and load fns.
| /// | ||
| /// The field contains the number of available fonts. This is always 1 for | ||
| /// .ttf and .otf files and usually greater than 1 for .ttc and .otc files. | ||
| OpenType(u32), |
There was a problem hiding this comment.
Nit. AAT uses the same SFNT container but is not technically OpenType.
There was a problem hiding this comment.
This was originally Sfnt (here and in other places) but that seemed too obtuse. Any suggestions for a better name?
edit: I should say that I think sfnt is the most accurate name for this but felt that others might find it too obtuse so changed it preemptively.
There was a problem hiding this comment.
Nothing other than Sfnt comes to mind. But maybe TrueType as the ancestor of all works better? Donno. Fine with OpenType.
| } | ||
|
|
||
| fn sbix(&self) -> Result<tables::sbix::Sbix<'a>, ReadError> { | ||
| // should we make the user pass this in? |
There was a problem hiding this comment.
I believe this comment was copied from the first impl of TableProvider but I agree. We should probably remove the is_long parameter from loca() as well since I don’t think anyone uses it.
| #[derive(Clone)] | ||
| pub enum FontBlob { | ||
| /// A borrowed static reference to a slice of bytes. | ||
| Static(&'static [u8]), |
There was a problem hiding this comment.
What is the motivation for the static variant here? Const construction of FontBlobs? Cheaper cloning of static FontBlobs?
(because &'static [u8] wrapped in Arc is a valid Arc<dyn AsRef<[u8]> + Send + Sync>, and this enum is introducing a branch in accessing the font data)
I guess perhaps accessing the font data pointer isn't a on a hot path as this design as you end up with cached pointers into specific parts of the data? But it occurs to me that it ought to be possible with some unsafe code to remove the branch required when accessing the data here (branch would still be required for Clone and Drop).
Might be that you have to require Deref or even StableDeref rather AsRef for that to be sound though.
There was a problem hiding this comment.
The motivation was mostly unsafe FFIs where we can potentially avoid an unnecessary allocation and (as you pointed out) indirection. This is probably premature optimization and we can likely omit it when we stabilize the API.
We might indeed start using unsafe at some point to cache preparsed tables (or groups of tables!) internally. Another option is to make FontTables a wrapper type with a lifetime that resolves all the indirections on construction so we can avoid them during bulk table lookup although the compiler possibly does this for us.
and make `Font::tables()` infallible by using empty `FontTables` type with `TableSource::None` for non-SFNT fonts
- and some additional tests - fix typos
- move FontSource to its own module - add FontKindRepr and change FontKind to take references; this will support future support for pure CFF - feature gate whole API
to satisfy clippy
|
This is now ready for review. As discussed elsewhere, this is now gated on a new The new API basically looks like this: // Create a font, shareable among multiple instances
let font = Font::new(font_data, 1).unwrap();
// Want to access some font tables?
let tables = font.tables();
let glyf = tables.glyf().unwrap();
// We have an SFNT at index 1
assert!(matches!(font.kind(), FontKind::Sfnt(_, 1)));
// Create an instance at 16px, wght=450, wdth=75
let instance = FontInstance::builder(&font).size(Some(16.0)).variations([("wght", 450.0), ("wdth", 75.0)]).build();
// Check some basic properties
assert_eq!(instance.size(), Some(16.0));
assert!(!instance.normalized_coords().is_empty());
// Shaping with HarfRust
let glyph_buffer = harfrust::shape(&instance, unicode_buffer, options);
// Perhaps in the future...
instance.glyphs().outline(gid).unwrap().draw(pen);
instance.glyphs().color(gid).unwrap().paint(painter);We want the font types to remain immutable and thread safe but also want to lazily compute some data. See liberal use of onces and atomics with more to come. See Pure CFF is still TODO. We need some "accelerators" to make this work nicely. |
cmyr
left a comment
There was a problem hiding this comment.
leaving some comments for potential future discussion, but i'm happy to see this merged and we can iterate on it as needed.
| /// | ||
| /// Calling this directly will make your `Font` **UNUSABLE** with | ||
| /// HarfRust. |
| &self.0.kind | ||
| } | ||
|
|
||
| /// If this is a table based (i.e. OpenType) font, then returns an object |
Where we open the door to a more ergonomic API that supports sanitization, caching, sourcing data from platform native font handles and nicer interop for skrifa/harfrust.
Rationale at googlefonts/oxidize#77