[skrifa] glyf: use gvar for advance delta when HVAR is missing #1312
Conversation
We were using gvar to compute deltas for left side-bearings but not for advance widths when the HVAR table is missing. Fixes #1311
| // Modify only LSB | ||
| AvailableVarMetrics::MissingSideBearings => points.len() - 3, | ||
| // Modify nothing | ||
| AvailableVarMetrics::Present => points.len() - 4, |
There was a problem hiding this comment.
we know we've always got at least the four phantom points I suppose?
There was a problem hiding this comment.
yeah, we ensure at least 4 points at entry to the function:
There was a problem hiding this comment.
Why isn't it written as len - PHANTOM_POINT_COUNT?
| MissingSideBearings, | ||
| /// No variable metrics table. | ||
| Missing, | ||
| } |
There was a problem hiding this comment.
I think I would expect these to be named for what is available not what isn't? - e.g. All, Advances, None as opposed to Present/MissingSide/Missing
| } | ||
| } | ||
| None => AvailableVarMetrics::Missing, | ||
| }; |
There was a problem hiding this comment.
IMHO this might still read better (closer to left-to-right top to bottom) as map? - something like glyph_metrics.hvar.map(|hvar| hvar.lsb_mapping.map(present).unwrap_or(missing sides)).unwrap_or(missing everything)
| } | ||
| AvailableVarMetrics::MissingSideBearings => { | ||
| self.phantom[0].x += deltas[count - 4].x; | ||
| } |
There was a problem hiding this comment.
Reading deltas at count - magic # is recurring, I wonder if there is an opportunity to make a helper fn or newtype with a nicely named (and probably always inlined) accessor?
|
@rsheeter sure, will do a follow up with some of these changes |
Additional code cleanup suggested after merging #1312
Fixes issue with advance width computation for variable fonts that lack an HVAR table, for details see [1]. Updated crates: * skrifa: 0.26.3 => 0.26.4 * [email protected] vetted for does-not-implement-crypto, safe-to-deploy, ub-risk-0 [1] googlefonts/fontations#1312 Fixed: chromium:385924890 Change-Id: I294d3286071fbf25d416a3225489ec9618125f15 Cq-Include-Trybots: chromium/try:android-rust-arm32-rel Cq-Include-Trybots: chromium/try:android-rust-arm64-dbg Cq-Include-Trybots: chromium/try:android-rust-arm64-rel Cq-Include-Trybots: chromium/try:linux-rust-x64-dbg Cq-Include-Trybots: chromium/try:linux-rust-x64-rel Cq-Include-Trybots: chromium/try:win-rust-x64-dbg Cq-Include-Trybots: chromium/try:win-rust-x64-rel Disable-Rts: True Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6163787 Commit-Queue: Dominik Röttsches <[email protected]> Reviewed-by: Łukasz Anforowicz <[email protected]> Cr-Commit-Position: refs/heads/main@{#1404768}
We were using gvar to compute deltas for left side-bearings but not for advance widths when the HVAR table is missing.
This only affects the TrueType interpreter (for Skia's usage) and hinted VFs are rare so it took a while to catch this one.
Fixes #1311