[skrifa] glyph metrics fixes#692
Conversation
A few changes to fix correctness and/or match FreeType: 1. Make unscaled case pass-through. Avoids overflow on large source values. 2. Cast advance width to `u16` before scaling to match FT 3. Don't reject empty `glyf` glyphs when reading metric deltas from `gvar`.
| // This is an identity scale for the pattern | ||
| // `mul_div(value, scale, 64)` | ||
| Fixed::from_bits(0x10000 * 64) | ||
| // Just pass through in unscaled case. Avoids potential overflow |
There was a problem hiding this comment.
Not quite clear from the comment how returning None here is a pass through? Is that handled in the caller?
There was a problem hiding this comment.
Yes, the comment is a bit imprecise. The assumption is that the caller will not apply a scale if this returns None.
There was a problem hiding this comment.
I’ll update the comment to reflect this.
| advance += self.metric_deltas_from_gvar(glyph_id)[1]; | ||
| } | ||
| Some(self.fixed_scale.apply(advance)) | ||
| // FT casts advance to FT_UShort after applying variation delta and |
There was a problem hiding this comment.
For which cases is this FT emulation needed, and do we still provide correct behavior? Does it lead to an intentional truncation?
Background: We are just discussing a new COLRv1 lack of precision issues due to FT's limited internal representation around variation deltas:
https://gitlab.freedesktop.org/freetype/freetype/-/issues/1257
See also
https://gitlab.freedesktop.org/freetype/freetype/-/commit/1178227b39a4eeecbefad226d4a753b345a16eb1
There was a problem hiding this comment.
This was actually triggered by a case where FT wraps a negative value. This one is difficult to accommodate in testing because the scale is applied after the fact.
And yes, I’ve been tracking that issue and discussed it with Rod yesterday. We agreed that COLR seems like a good place to experiment with breaking compatibility for more accurate results so we’d be fine with making those changes here.
There was a problem hiding this comment.
The issue I was quoting from FT originates from COLRv1 but applies to all variation values (MVAR, HVAR, etc., listed in the issue) that use tt_var_get_item_delta - is that where the truncations happens?
What kind of value was that that was wrapped? This really sounds like a bug in FT - I lean towards not emulating that.
There was a problem hiding this comment.
Further thinking: If that issue in FT was fixed (for example running FT with @tursunova's patch (which still needs a bit of work, but should cover most cases)) - would then the fauntlet runs and FT match with regards to this line?
There was a problem hiding this comment.
That patch doesn’t seem to touch the unsigned short cast so we’d still have a diff. This case might be a buggy font because it generates a negative advance width. I think those are generally unexpected in text layout.
On the broader question, we do FT style truncation for deltas at the call site rather than in the IVS code so we can adjust as needed. We do also have the rounding though, which I ported from your 64 bit accumulation patch.
There was a problem hiding this comment.
which I ported from your 64 bit accumulation patch.
Which one do you mean, or which point in the code in FT is that?
On the broader question, we do FT style truncation for deltas at the call site rather than in the IVS code so we can adjust as needed.
I have trouble understanding that sentence. What do you mean by "adjust as needed" in this context? And can you elaborate on the distinction between "call site" and "IVS code"?
There was a problem hiding this comment.
That cast occurs in tt_face_get_metrics which returns the advance through a pointer to FT_UShort.
There was a problem hiding this comment.
I meant the rounding in FT_MulAddFix here: https://gitlab.freedesktop.org/freetype/freetype/-/blob/master/src/base/ftcalc.c?ref_type=heads#L1083
wrt call site vs IVS, we don’t have any generic “item delta” function that does truncation. Wherever we are applying a delta we truncate explicitly, usually with a comment that this is done to match FT. For example:
fontations/skrifa/src/metrics.rs
Line 304 in d4651c5
edit: by “adjust accordingly” I mean that we can return higher precision for say, advance widths, without requiring sweeping changes.
| @@ -54,20 +54,20 @@ impl Size { | |||
| } | |||
There was a problem hiding this comment.
Which tests are covering these behavior changes? Fauntlet runs?
There was a problem hiding this comment.
Yes, with these changes, we fully match FT advances for fauntlet runs on all of gfonts+noto.
My thinking is that we match first and then decide to break compatibility in well understood ways.
|
With this applied, when comparing advances of FT for the COLRv1 test font, in the Fontations: FreeType: In |
|
Hrm, yes, this patch matches linearHoriAdvance. I’m guessing Skia is pulling from the glyph slot advance field which is calculated differently and rounded somewhere along the way. My intention was to provide this value in ScalerMetrics::adjusted_advance but I haven’t hooked that up yet. |
Yes, looks like it in
SGTM, I will update then to retrieve it from that field. |
|
By “haven’t hooked it up” I mean that field is an option and is not populated yet :( |
Not to rush, but any updates on that? I was feeling like I needed a break from bitmap glyphs for a day and came back to this. |
|
Yes, this is computed now for TrueType outlines. It will still be |
Thanks for the update. Do you mean in shipping/merged code? Is this in |
|
It’s in |
|
Closing this for now to clean things up but keeping the branch around in case we find a need to match linearHoriAdvance. |
2 participants
A few changes to fix correctness and/or match FreeType:
u16before scaling to match FTglyfglyphs when reading metric deltas fromgvar