Closed Bug 1441323 Opened 4 years ago Closed 3 years ago

Implement font variation support in WebRender windows font backend

Categories

(Core :: Graphics: WebRender, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: jfkthame, Assigned: lsalzman)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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.
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?
Depends on: 1433060
Flags: needinfo?(lsalzman)
What's the status of this bug?
Flags: needinfo?(jfkthame)
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
Blocks: wr-android
No longer blocks: stage-wr-trains
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)
WR PR https://github.com/servo/webrender/pull/3105 will solve this.
Flags: needinfo?(lsalzman)
Attachment #8954192 - Flags: review+
Depends on: 1493496
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/059b3cca8da3
Handle font variation settings in ScaledFontDWrite::GetWRFontInstanceOptions. r=lsalzman
https://hg.mozilla.org/mozilla-central/rev/059b3cca8da3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
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 → ---
(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)
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....
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.
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.
(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...
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
Depends on: 1496670
Depends on: 1497768
No longer depends on: 1496670
This should be fixed now.
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.