fonts: Add font variations support for Windows#38831
Conversation
nicoburns
left a comment
nicoburns
left a comment
There was a problem hiding this comment.
LGTM. A few minor comments which I would not consider blocking.
| let variations: Vec<_> = variations | ||
| .into_iter() | ||
| .map(|variation| DWRITE_FONT_AXIS_VALUE { | ||
| axisTag: variation.tag.swap_bytes(), |
There was a problem hiding this comment.
How was the need for swap_bytes determined?
There was a problem hiding this comment.
The table tags that DirectWrite uses have their bytes swapped in relation to the ones that Servo and HarfBuzz uses. See https://learn.microsoft.com/en-us/windows/win32/api/dwrite/nf-dwrite-dwrite_make_opentype_tag.
There was a problem hiding this comment.
I've copy-and-pasted a comment about this here as well.
| let variations = font_face.variations().unwrap_or_default(); | ||
| let variations = variations.iter().map(|dwrote_variation| { | ||
| FontVariation { | ||
| tag: dwrote_variation.axisTag.swap_bytes(), | ||
| value: dwrote_variation.value, | ||
| } | ||
| }).collect(); |
There was a problem hiding this comment.
This could just be variations.to_vec() where variations is the function parameter?
There was a problem hiding this comment.
Hrm. I couldn't figure out how to use to_vec() here. We need to convert from DWRITE_FONT_AXIS_VALUE to FontVariation (from WebRender), so I think a map is required.
| // On FreeType and CoreText platforms, the platform layer is able to read the minimum, maxmimum, | ||
| // and default values of each axis. This doesn't seem possible here and it seems that Gecko | ||
| // also just sets the value of the axis based on the values from the style as well. |
There was a problem hiding this comment.
It seems that direct write provides API to extract range, but probably dwrote wrapper must be updated to support variation limits extraction on Windows.
https://docs.rs/dwrote/latest/dwrote/index.html
https://learn.microsoft.com/en-us/windows/win32/api/dwrite_3/ne-dwrite_3-dwrite_font_axis_attributes
https://learn.microsoft.com/en-us/windows/win32/api/dwrite_3/ns-dwrite_3-dwrite_font_axis_range
Update:
I see that you have already added it to dwrote!
There was a problem hiding this comment.
Oh, interesting. It does seem like we can also extract the ranges and defaults, but we don't need to either. From https://learn.microsoft.com/en-us/windows/win32/api/dwrite_3/nf-dwrite_3-idwritefontresource-createfontface:
The axis values that you provide are permitted to be a subset or superset of all the ones actually supported by the font. Any unspecified axes use their default values: values beyond the ranges are clamped, and any non-varying axes have no effect.
It seems like just reading the values from the font is a bit more efficient since they are normalized upon creation anyway.
There was a problem hiding this comment.
I'm not sure about DirectWrite, but looking at skrifa the coordinates are also normalized on creation there, but they are normalized into 16-bit (14.2) fixed-point format. So converting that back to f32 is an additional step (it looks to me like the FreeType backend is also converting back from 16.16 to f32).
I was wondering whether we needed to normalize at all, and whether we could just store the unormalized values we get from style directly and rely on anything using them to handle normalization.
mrobinson
force-pushed
the
variations-windows
branch
2 times, most recently
from
f2c29a2 to
0a2d1b4
Compare
Unlike other platforms where we read the default axis values and combine it with variations from style to make the font face, we set the variations from the style when creating the font face and then read the final variations from the face. It seems that DirectWrite does the normalization of variation values internally. This depends on servo/dwrote-rs#68. Signed-off-by: Martin Robinson <[email protected]>
mrobinson
force-pushed
the
variations-windows
branch
from
0a2d1b4 to
58bae5c
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
I get lots of deprecated warning. |
Addressed in #38856. |
4 participants
Unlike other platforms where we read the default axis values and combine
it with variations from style to make the font face, we set the
variations from the style when creating the font face and then read the
final variations from the face. It seems that DirectWrite does the
normalization of variation values internally.
This depends on servo/dwrote-rs#68.
Testing: We currently don't have tests for Windows, but variation support is
covered by the WPT tests.
Fixes: This is part of #38800.