Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DirectWrite font rendering for Windows #549

Merged
merged 4 commits into from
Nov 10, 2016
Merged

Conversation

vvuk
Copy link
Contributor

@vvuk vvuk commented Nov 9, 2016

This PR adds DirectWrite font rendering for Windows, using a thin native-rust dwrite wrapper library. There's a corresponding PR for Servo that adds support for DirectWrite to their font backend as well.

Things work, though right now there is no support for web fonts. This isn't hard to add and should be straightforward; @metajack already has rust code for implementing the required COM interfaces to do so, and this just needs to be done in https://github.com/vvuk/dwrote-rs.


This change is Reviewable

Copy link
Member

@glennw glennw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Just a few minor issues.

@@ -95,9 +101,12 @@ extern crate core_graphics;
#[cfg(target_os="macos")]
extern crate core_text;

#[cfg(not(target_os="macos"))]
#[cfg(any(target_os="linux", target_os="android"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be all(unix, not(target_os = "macos")) I think, since WR builds and runs on *BSD now.

use dwrote;

pub struct FontContext {
// FIXME do I need to protect this in a mutex?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - FontContexts are created per-thread and stored in TLS.

return
}

panic!("DWrite can't do raw fonts yet");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this break a lot of websites? Should we consider making it non-fatal for now?

@@ -371,10 +372,13 @@ pub enum MixBlendMode {
pub type NativeFontHandle = CGFont;

/// Native fonts are not used on Linux; all fonts are raw.
#[cfg(not(target_os = "macos"))]
#[cfg_attr(not(target_os = "macos"), derive(Clone, Serialize, Deserialize))]
#[cfg(any(target_os = "linux", target_os = "android"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here as the above comment about *BSD.


// don't rasterize blank glyphs
if width_u == 0 || height_u == 0 {
return (Some(dims), Some(RasterizedGlyph {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WR expects the font platform to return None when the glyph has zero dimensions.

@@ -371,10 +372,13 @@ pub enum MixBlendMode {
pub type NativeFontHandle = CGFont;

/// Native fonts are not used on Linux; all fonts are raw.
#[cfg(not(target_os = "macos"))]
#[cfg_attr(not(target_os = "macos"), derive(Clone, Serialize, Deserialize))]
#[cfg(all(unix, not(target_os="macos")))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vvuk This is resulting in Clone not being derived on Linux, causing a compile error. I'm not sure why - it looks correct to me. I guess we could try make it not(windows or mac) or something like that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what that change is supposed to do, but please do try not(windows or mac).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(@nox - @glennw left out a second added line which was the cfg_attr with the same conditional as the cfg)

Yeah, this is definitely a serde bug. In the generated types.rs, this is expanding out to:

#[cfg(target_os = "macos")]
pub type NativeFontHandle = CGFont;
#[allow(non_upper_case_globals, unused_attributes, unused_qualifications)]
#[cfg(all(unix, not(target_os = "macos")))]
const _IMPL_DESERIALIZE_FOR_NativeFontHandle: () =
...
#[cfg(target_os = "macos")]
pub type NativeFontHandle = CGFont;
#[allow(non_upper_case_globals, unused_attributes, unused_qualifications)]
#[cfg(all(unix, not(target_os = "macos")))]
const _IMPL_DESERIALIZE_FOR_NativeFontHandle: () =
    {
...
    };
/// Native fonts are not used on Linux; all fonts are raw.
#[cfg(all(unix, not(target_os = "macos")))]
pub struct NativeFontHandle;

Note that all mention of derive(Clone) was lost. Even if I add a second cfg_attr with just derive(Clone) it gets eaten as well. Changing the conditional does not change the behaviour. Forcing serde to 0.8.16 doesn't help. Hrm.

@vvuk
Copy link
Contributor Author

vvuk commented Nov 10, 2016

I pushed a new commit by itself that should fix this... hopefully someone from serde can look at it and see what's up with the other version.

@glennw
Copy link
Member

glennw commented Nov 10, 2016

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 2ace4b4 has been approved by glennw

@bors-servo
Copy link
Contributor

⚡ Test exempted - status

@bors-servo bors-servo merged commit 2ace4b4 into servo:master Nov 10, 2016
bors-servo pushed a commit that referenced this pull request Nov 10, 2016
DirectWrite font rendering for Windows

This PR adds DirectWrite font rendering for Windows, using a thin native-rust dwrite wrapper library.  There's a corresponding PR for Servo that adds support for DirectWrite to their font backend as well.

Things work, though right now there is no support for web fonts.  This isn't hard to add and should be straightforward; @metajack already has rust code for implementing the required COM interfaces to do so, and this just needs to be done in https://github.com/vvuk/dwrote-rs.

<!-- Reviewable:start -->

---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/549)
<!-- Reviewable:end -->
@vvuk vvuk deleted the dw-fonts branch December 6, 2016 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants