[AnyRender Serialize]: Improving Font Serialisation#41
[AnyRender Serialize]: Improving Font Serialisation#41taj-p merged 15 commits intoDioxusLabs:mainfrom
Conversation
nicoburns
left a comment
There was a problem hiding this comment.
This is generally excellent. I'm a little concerned that we're introducing the subsetting transformation using a fairly early-stage crate without much ability to test for regressions (we don't yet have a way to record complex scenes (e.g. webpages using Blitz).
My suggestion would be to mititgate the risk by making the subsetting transformation optional. This could be a write-only optionality (on the read side we just deal with whatever we're given). That way if we run into issues we can easily run it without the option enabled while we figure things out.
While we're at it, perhaps we could also make the woff compression optional? (although this seems less important). Again, I suggest an option on the writing side of things, and reading would just detect whether it's a woff or not, and decompress if it is.
| })?; | ||
| Ok(FontData::new( | ||
| Blob::from(ttf), | ||
| // Since we've extracted all faces into standalone fonts, the index is always 0. |
There was a problem hiding this comment.
Nit: could we move this comment to just above the Ok(.... Then 4 lines can become 1.
There was a problem hiding this comment.
I think this is now overcome by the refactor to use FontWriter with subsetting and woff2 features
| serde_json = "1.0" | ||
| zip = { version = "2.1", default-features = false, features = ["deflate"] } | ||
| sha2 = "0.10" | ||
| klippa = { git = "https://github.com/googlefonts/fontations", rev = "41ec2bdd6dd8589b65eaaaf182b0a2e701d0d826" } |
There was a problem hiding this comment.
klippa not being published is a bit of a problem. That will also prevent publishing of anyrender_serialize. We could ask fontations people to publish. But perhaps we'd be better off publishing our own fork? As I'm anticipating it being difficult to get fontations to publish klippa in lock-step with read-fonts, skrifa, etc.
There was a problem hiding this comment.
I'm happy to publish a klippa fork? Is that the general workaround for things like this?
| &IntSet::empty(), | ||
| &IntSet::empty(), | ||
| &IntSet::empty(), | ||
| &IntSet::empty(), | ||
| &IntSet::empty(), |
There was a problem hiding this comment.
How confident are you these IntSet::empty() parameters are correct? Looks like that will remove all of these items in the font. (I'm not whether we need them, but I'm not confident that we don't).
There was a problem hiding this comment.
I don't think we need them, but let me double check and get back to you
Brilliant comment @nicoburns !!! Thank you! I've feature flagged subsetting and woff2 decompression. My feel is, as you suggest, we start off generating scenes without them and then regression test with them enabled. I'm happy to look into this over the coming week. |
nicoburns
left a comment
There was a problem hiding this comment.
I had in mind runtime feature flags, but this'll do. Approved subject to CI passing.
klippa]`)