Implement font variation support in WebRender windows font backend

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P1
normal
RESOLVED FIXED
Last year
8 months ago

People

(Reporter: jfkthame, Assigned: lsalzman)

Tracking

(Blocks 2 bugs)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

Last year
AFAICS, the Windows font backend in webrender does not yet support font variations.

Variation support in the Gecko DirectWrite font code was added in bug 1430632 (only when running on the Win10 Fall Creators Update), but when webrender is enabled, the variations are not handled.

This will require added support in ScaledFontDWrite::GetWRFontInstanceOptions, but AFAICS that will not be sufficient by itself; support is also needed on the WR side.
Reporter

Comment 1

Last year
The first part here is trivial (building on top of the helper provided in bug 1433060); this should add the variation settings to the WR instance options. But on the webrender side, the dwrite font support doesn't do anything with the variations, so they're just ignored. Lee, any chance you could take this and fill in what's missing?
Reporter

Updated

Last year
Depends on: 1433060
Flags: needinfo?(lsalzman)
What's the status of this bug?
Flags: needinfo?(jfkthame)
Reporter

Comment 3

Last year
Lee would be the best person to answer that. AIUI it requires adding support for the variations APIs in dwrote, then hooking that up to the variation info that "pt 1" above would send in the font instance options.
Flags: needinfo?(jfkthame)
Assignee: nobody → lsalzman
Reporter

Comment 4

10 months ago
Jeff, I don't understand these dependency changes:

> Blocks: 1485449
> No longer blocks: 1386669

This is very much a Windows bug, and has nothing to do with Android.... was this an accidental update that should be reverted?
Flags: needinfo?(jmuizelaar)
Yep. That was an accidental update. Thanks for catching it.
Blocks: stage-wr-trains
No longer blocks: wr-android
Flags: needinfo?(jmuizelaar)
We can't release this to the field, but we can let this ride to beta.  Obviously I'd love this solved early in Beta (or earlier) if we can.
Priority: P1 → P2
I think we should probably block beta on this. It would be a confusing bug to run into as web developer because font variations would just be broken.
Flags: needinfo?(mreavy)
I admit -- I'd really love it for Beta.  Lee -- do you think you can get this done in the next 2 weeks?
Flags: needinfo?(mreavy)
Priority: P2 → P1
Lee is looking at a skia update. Jeff do you know who else could help here?
Flags: needinfo?(jmuizelaar)
We can probably wait for the skia update, provided it doesn't take too long.
Flags: needinfo?(jmuizelaar)
Assignee

Comment 11

9 months ago
WR PR https://github.com/servo/webrender/pull/3105 will solve this.
Flags: needinfo?(lsalzman)
Assignee

Updated

9 months ago
Attachment #8954192 - Flags: review+

Comment 12

9 months ago
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/059b3cca8da3
Handle font variation settings in ScaledFontDWrite::GetWRFontInstanceOptions. r=lsalzman

Comment 13

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/059b3cca8da3
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Reporter

Comment 14

8 months ago
Lee, I was just trying current Nightly (2018-10-09) on Win10, and this doesn't appear to be working: looked at several demo sites (axis-praxis.org, v-fonts.com, curtismann.org/variablefonts), and in all cases when WR is enabled, I'm seeing the glyph positions move in response to changing variation values, but the actual glyph shapes don't change. Did something about the fix here not quite work out right?
Status: RESOLVED → REOPENED
Flags: needinfo?(lsalzman)
Resolution: FIXED → ---
Assignee

Comment 15

8 months ago
(In reply to Jonathan Kew (:jfkthame) from comment #14)
> Lee, I was just trying current Nightly (2018-10-09) on Win10, and this
> doesn't appear to be working: looked at several demo sites (axis-praxis.org,
> v-fonts.com, curtismann.org/variablefonts), and in all cases when WR is
> enabled, I'm seeing the glyph positions move in response to changing
> variation values, but the actual glyph shapes don't change. Did something
> about the fix here not quite work out right?

I unfortunately don't have a machine available on which to test that, so you'd have to see what's going on in the Windows backend... The place to look would be here: https://dxr.mozilla.org/mozilla-central/source/gfx/webrender/src/platform/windows/font.rs#183

We'd want to verify that the right values are getting passed in and that create_font_face_with_variations is actually succeeding.
Flags: needinfo?(lsalzman)
Reporter

Comment 16

8 months ago
Ugh... ok, I'll try to bring up a debug build and see what I can find out. It may take a while, though, my Windows builds aren't the fastest in the world....
Reporter

Comment 17

8 months ago
Initial indications seem to be that the right variation values (or at least something that looks plausible are being passed in, but create_font_face_with_variations may be failing. Might need a non-optimized build to get a better idea of just where, though.
Reporter

Comment 18

8 months ago
So AFAICS (though the VS debugger seems to struggle to follow the rust code...), the call to face5.GetFontResource at https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/third_party/rust/dwrote/src/font_face.rs#307 appears to be failing, and so we never succeed in creating a font instance with the required variations.
Reporter

Comment 19

8 months ago
(In reply to Jonathan Kew (:jfkthame) from comment #18)
> So AFAICS (though the VS debugger seems to struggle to follow the rust
> code...), the call to face5.GetFontResource at
> https://searchfox.org/mozilla-central/rev/
> 80ac71c1c54af788b32e851192dfd2de2ec18e18/third_party/rust/dwrote/src/
> font_face.rs#307 appears to be failing, and so we never succeed in creating
> a font instance with the required variations.

Retract that: after adding some tracing statements, it seems that create_font_face_with_variations _does_ successfully return something. Trying to dig further...
Assignee

Comment 20

8 months ago
We were supplying font variation tags to DWrite with the wrong endianness. This is fixed by WR PR https://github.com/servo/webrender/pull/3179
Assignee

Updated

8 months ago
Depends on: 1496670
No longer depends on: 1496670
This should be fixed now.
Status: REOPENED → RESOLVED
Closed: 9 months ago8 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.