Text does not render with a skia backend on windows

RESOLVED FIXED in Firefox 44

Status

()

Core
Graphics: Text
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

Trunk
mozilla44
x86_64
Windows
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
When setting the content backend to skia, no text renders. The main reason is that we use DirectWrite fonts, which are not supported in skia at the moment. However, even using GDI fonts, all text is rendered in 1 position and characters are never advanced.
(Assignee)

Comment 1

3 years ago
Created attachment 8668201 [details] [diff] [review]
Fallback to GDI fonts if skia is the preferred backend

This patch enables us to fallback to GDI fonts if Skia is the preferred content backend. It also, I think fixes just a minor bug. The code at [1] was always failing because we'd fail to receive a DC at [2], hence the advance position during text rendering would always be 0, so we'd render all the text in the same position. I think DCFromDrawTarget was meant to return the screen DC with non-cairo DrawTargets.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxGDIFont.cpp?case=true&from=gfxGDIFont.cpp&offset=0#521
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxWindowsPlatform.cpp?case=true&from=gfxWindowsPlatform.cpp#88
Attachment #8668201 - Flags: review?(jwatt)
Cool, this will at least make it easier to switch to and from Skia as a backend - it's a pain to change the pref back once you can't see any of the text in about:support :)
Comment on attachment 8668201 [details] [diff] [review]
Fallback to GDI fonts if skia is the preferred backend

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

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +106,5 @@
> +  // Gets the whole screen DC
> +  if (!mDC) {
> +	  mDC = GetDC(nullptr);
> +	  SetGraphicsMode(mDC, GM_ADVANCED);
> +	  mNeedsRelease = true;

You've got tab characters in here which is messing up the indentation.

Replace them with spaces. While you're here I'd suggest putting the comment inside the |if| block and maybe phrasing it as "Get the whole screen DC:". Up to you on that one though.
Attachment #8668201 - Flags: review?(jwatt) → review+

Comment 4

3 years ago
(In reply to Mason Chang PTO [:mchang] from comment #0)
> When setting the content backend to skia, no text renders. The main reason
> is that we use DirectWrite fonts, which are not supported in skia at the
> moment. However, even using GDI fonts, all text is rendered in 1 position
> and characters are never advanced.

Is this really true? There's clearly code in Skia for supporting DirectWrite fonts. Does it not work for some reason? Is this possibly related to bug 1149318?

Updated

3 years ago
Flags: needinfo?(mchang)
(Assignee)

Comment 6

3 years ago
(In reply to John Daggett (:jtd) from comment #4)
> (In reply to Mason Chang PTO [:mchang] from comment #0)
> > When setting the content backend to skia, no text renders. The main reason
> > is that we use DirectWrite fonts, which are not supported in skia at the
> > moment. However, even using GDI fonts, all text is rendered in 1 position
> > and characters are never advanced.
> 
> Is this really true? There's clearly code in Skia for supporting DirectWrite
> fonts. Does it not work for some reason? Is this possibly related to bug
> 1149318?

At least in the version that we have, when forcing DirectWrite fonts, Skia will error out saying "DirectWrite and Skia do not mix" [1]. We still have to build out support to properly use DirectWrite with skia, which is in bug 842894. 

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/2d/ScaledFontDWrite.h?case=true&from=ScaledFontDWrite.h#41
Flags: needinfo?(mchang)
(Assignee)

Comment 8

3 years ago
Had to rebase off bug 1208638.
Flags: needinfo?(mchang)
https://hg.mozilla.org/mozilla-central/rev/8d342ef310db
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.