Support DirectWrite using the Skia backend

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: claritise, Assigned: mchang)

Tracking

21 Branch
mozilla46
x86
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.2; rv:21.0) Gecko/20130219 Firefox/21.0
Build ID: 20130219031055
Looks like Skia supports DirectWrite, but doesn't expose any API to create a SkTypeface from an existing IDWriteFontFace*.

There's an internal function for this though, so I don't see why we couldn't expose that and use it.

We also need the IDWriteFont* and IDWriteFontFamily* objects, I *think* we can get these through the gfxFont, but I'm not entirely sure.

Updated

6 years ago
Component: Untriaged → Graphics
Product: Firefox → Core
CC'ing people who know more about windows fonts.

1) Is it possible to get the IDWriteFont* and IDWriteFontFamily* from a gfxDWriteFont*? (I know we can get the IDWriteFontFace*).

2) Does it make sense to expose the SkCreateTypefaceFromDWriteFont API (http://mxr.mozilla.org/mozilla-central/source/gfx/skia/src/ports/SkFontHost_win_dw.cpp#685) to create Skia font objects for DWrite.

It's not obvious to me if there is any reason that this should only be called internally, but I'm happy to look into this more myself if need be.
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> 2) Does it make sense to expose the SkCreateTypefaceFromDWriteFont API
> (http://mxr.mozilla.org/mozilla-central/source/gfx/skia/src/ports/
> SkFontHost_win_dw.cpp#685) to create Skia font objects for DWrite.

I have no objection to this. It's the plan I've got for supporting Cairo fonts etc.
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)

> 1) Is it possible to get the IDWriteFont* and IDWriteFontFamily* from a
> gfxDWriteFont*? (I know we can get the IDWriteFontFace*).

Why do you need the IDWriteFontFamily*?
(In reply to John Daggett (:jtd) from comment #4)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> 
> > 1) Is it possible to get the IDWriteFont* and IDWriteFontFamily* from a
> > gfxDWriteFont*? (I know we can get the IDWriteFontFace*).
> 
> Why do you need the IDWriteFontFamily*?
Flags: needinfo?(matt.woodrow)
George probably knows more about this now.

Skia previously only supported using a single font backend at compile, but they had plans to support dynamic switches between backends.

We need this to support both GDI and DWrite with a single binary.
Flags: needinfo?(matt.woodrow) → needinfo?(gwright)
Vlad looked into this most recently. I will also investigate.
Flags: needinfo?(vladimir)
Skia supports 1 default backend, but it can compile multiple backends in (and for windows, we do end up with both GDI and DirectWrite backends compiled in).  We just need to be explicit about which one gets used.
Flags: needinfo?(vladimir)
Thinking about this further, it should be pretty reasonable to do this. On Android we currently use two FontHost backends (one for Cairo fonts, and one that wraps a TrueType file with discovery code for the Android file layout). We just need to ensure that the SkTypeface caches don't clash. We can do something similar for Windows. The only real issue would be the various factory methods can only be supplied by one of the fonthosts, but as we don't use any of them anyway it becomes a non-issue.
Flags: needinfo?(gwright)
Assignee

Updated

4 years ago
Duplicate of this bug: 999169
Assignee

Updated

4 years ago
Assignee: nobody → mchang
Posted patch Support DWrite fonts in skia (obsolete) — Splinter Review
This patch carries the information about a gfxDWriteFont, most importantly the IDWriteFont and IDWriteFontFamily, which is required by Skia. When we create the ScaledFontDWRite from the gfxDWrite, we keep references to the IDWrite* members, then in DrawTargetSkia, we can finally query the ScaledFontDWrite to get the SkTypeface required for skia. I browsed a few sites and everything seemed to work.

Two things that are somewhat confusing though.
1) Sometimes, the gfxDWriteFontEntry doesn't actually have a font, only the FontFamily. [A]. In those cases, I just default back to the DefaultFont(), but is there supposed to be a font? How does d2d get a font just from the font family? I didn't see any APIs on MSDN.
2) Sometimes, the font is blurry compared to using the DWrite fonts with d2d. Any hints on why?

[A] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxDWriteFonts.cpp?from=gfxDWriteFonts.cpp#84
Attachment #8679708 - Flags: review?(bas)
Err sorry, error on question 1. How can we get a IDWriteFont just from the IWriteFontFace?
Comment on attachment 8679708 [details] [diff] [review]
Support DWrite fonts in skia

Review of attachment 8679708 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/ScaledFontDWrite.cpp
@@ +294,5 @@
>  ScaledFontDWrite::ScaledFontDWrite(uint8_t *aData, uint32_t aSize,
>                                     uint32_t aIndex, Float aGlyphSize)
> +  : mFont(nullptr)
> +  , mFontFamily(nullptr)
> +  , ScaledFontBase(aGlyphSize)

Please fix order, initialize base class before members.

::: gfx/2d/ScaledFontDWrite.h
@@ +25,1 @@
>      , ScaledFontBase(aSize)

idem
Attachment #8679708 - Flags: review?(bas) → review+
After talking with Lee, I backed this out until we can decide on how we want to upstream / manage our own skia changesets. Also, the latest version of Skia changes the DirectWrite implementation a bit, so we'll land this once we update our skia implementation.

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
rebased on master.
Attachment #8679708 - Attachment is obsolete: true

Comment 20

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cf4d7af79300
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46

Updated

3 years ago
Depends on: 1307220
Comment on attachment 8704768 [details] [diff] [review]
Support dwrite fonts in skia

Review of attachment 8704768 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxDWriteFonts.cpp
@@ +110,5 @@
> +            static_cast<gfxDWriteFontFamily*>(fontList->GetDefaultFont(aFontStyle));
> +
> +        mFont = defaultFontFamily->GetDefaultFont();
> +        NS_WARNING("Using default font");
> +    }

I don't see how this can be OK.

It's expected that a gfxDWriteFontEntry may not hold an IDWriteFont reference; in the case of an @font-face resource, it will *instead* have an IDWriteFontFile. See https://dxr.mozilla.org/mozilla-central/rev/7ae377917236b7e6111146aa9fb4c073c0efc7f4/gfx/thebes/gfxDWriteFontList.h#190-195.

But if it's a font entry for a downloaded resource (and therefore fe->GetFont() returns null), it doesn't make sense to just substitute some default font instead. I don't know what this IDWriteFont ends up getting used for in the Skia backend -- maybe nothing, if we're lucky?! -- but if we're supposed to be using the downloaded font, we can't just start using the default font instead for any meaningful purpose.

So... why do we need to pass this IDWriteFont to Skia, and what does it get used for? Identify where it's used, and that will most likely point to a scenario where downloadable fonts don't work correctly.
Flags: needinfo?(mchang)
Assignee

Updated

3 years ago
Depends on: 1309917
(In reply to Jonathan Kew (:jfkthame) from comment #21)
> Comment on attachment 8704768 [details] [diff] [review]
> Support dwrite fonts in skia
> 
> Review of attachment 8704768 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxDWriteFonts.cpp
> @@ +110,5 @@
> > +            static_cast<gfxDWriteFontFamily*>(fontList->GetDefaultFont(aFontStyle));
> > +
> > +        mFont = defaultFontFamily->GetDefaultFont();
> > +        NS_WARNING("Using default font");
> > +    }
> 
> I don't see how this can be OK.
> 
> It's expected that a gfxDWriteFontEntry may not hold an IDWriteFont
> reference; in the case of an @font-face resource, it will *instead* have an
> IDWriteFontFile. See
> https://dxr.mozilla.org/mozilla-central/rev/
> 7ae377917236b7e6111146aa9fb4c073c0efc7f4/gfx/thebes/gfxDWriteFontList.h#190-
> 195.
> 
> But if it's a font entry for a downloaded resource (and therefore
> fe->GetFont() returns null), it doesn't make sense to just substitute some
> default font instead. I don't know what this IDWriteFont ends up getting
> used for in the Skia backend -- maybe nothing, if we're lucky?! -- but if
> we're supposed to be using the downloaded font, we can't just start using
> the default font instead for any meaningful purpose.
> 
> So... why do we need to pass this IDWriteFont to Skia, and what does it get
> used for? Identify where it's used, and that will most likely point to a
> scenario where downloadable fonts don't work correctly.

Thanks for finding this! Filed as bug 1309917.
Flags: needinfo?(mchang)
You need to log in before you can comment on or make changes to this bug.