1270688 - Pipe DWrite rendering param gamma into SkPaint::gamma

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mason Chang [Inactive] [:mchang]]

DWrite fonts are a bit thin with skia content compared to d2d 1.1. This is in large part because the gamma we use is 2.2 in Skia whereas d2d 1.1 queries the primary monitor to determine monitor gamma. This information is passed to DrawTarget::FillGlyphs. We should set the SkPaint gamma to this value so that d2d1.1 fonts are ~= Skia dwrite fonts.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: gamma.patch

This adds another fGamma field to SkPaint[1]. During DrawTargetSkia::FillGlyphs, we read the gamma field from the rendering params, pass it onto the SkPaint object, which passes it to the SkScalerContext. We may want to uplift this API if they want to take it.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/include/core/SkPaint.h?from=SkPaint.h#1066

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: typeface.patch

Attaches the glyph rendering options to the typeface, but fonts on Windows 7 look bigger than d2d instead of thinner. On Windows 10 hidpi, it looks roughly equal.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: typeface.patch

I found out what was going on with the contrast. It looks like D2D actually actively ignores the contrast (and I think gamma too) of the IDWriteRenderingParams whereas Skia does not. I verified this both by an external program [1] where I took the same glyph run and drew the text with two different rendering params. Both look the same when zoomed in with the magnifier. The second verification was that I changed the default cleartype contrast gfxWindowsPlatform creates [2] to 0.5 and compared it to nightly, both running the d2d backend. Text was pixel for pixel exactly the same. I did these tests on both Windows 10 + HiDPI and Windows 7 + Non-HiDPI.

This patch passes the IDWriteRenderingParams through the SkTypeface to properly set just the gamma. I think this touches skia internals less than touching SkPaint, so I went with this. A gamma of 1.8, which is the dwrite default params value, makes skia text rendering look very close to our d2d backend. Right now, this doesn't set the contrast, but we can probably do that too. I think the contrast we're setting in gfxWindowsPlatform is also wrong since we shouldn't set the contrast to a blind 1.0 versus default to use the default rendering params contrast (I'll fix this in another bug). We could then pass this down to skia, but then we'd also still look slightly different than d2d.

Another option is to just use the system default macro to use default rendering params, which means we wouldn't have to touch skia at all. [3]

[1] https://github.com/changm/DWriteFont
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxWindowsPlatform.cpp?case=true&from=gfxWindowsPlatform.cpp#1343
[3] https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/ports/SkTypeface_win_dw.cpp?case=true&from=SkTypeface_win_dw.cpp#268

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: useSystemSettings.patch

Use system default settings for fonts. Then we don't really have to touch skia. This should default to 1.8 gamma and 0.5 contrast. Skia prior was using 2.2 gamma and 0.5 contrast.

Comment 5

[Lee Salzman [:lsalzman]]

Comment on attachment 8753127 [details] [diff] [review]
useSystemSettings.patch

We should file an upstream bug report about the function name issue too.

Comment 6

[Mason Chang [Inactive] [:mchang]]

Try is weird - https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7e52e8dc09c

After talking with Ryan, it seems to be because of our migration to AWS on Windows

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed685a8686a4

Comment 8

[Mason Chang [Inactive] [:mchang]]

(In reply to Lee Salzman [:lsalzman] from comment #5)
> Comment on attachment 8753127 [details] [diff] [review]
> useSystemSettings.patch
> 
> We should file an upstream bug report about the function name issue too.

Filed at https://bugs.chromium.org/p/skia/issues/detail?id=5313

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ed685a8686a4

Comment 10

[Andrei Vaida [:avaida]]

This feature just came up on our radar. Mason, is manual QA required here? If so, could you please expand a bit on how we can help?

Comment 11

[Mason Chang [Inactive] [:mchang]]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #10)
> This feature just came up on our radar. Mason, is manual QA required here?
> If so, could you please expand a bit on how we can help?

Thanks for the offer! The real feature is bug 1248740. The best way to test is to do:

1) go to about:config
2) Change the preference "gfx.content.azure.backends" to "skia".
3) Restart Firefox
4) Verify that the field "AzureContentBackend" is "skia" in about:support
5) Try Firefox and ensure you don't see any rendering artifacts or crashes. Thanks!

Comment 12

[Bogdan Maris, Desktop QA]

We finished testing based on the information received from Mason in comment 11. No new issue was found during our testing. More results can be seen in the gdoc we tracked all our testing (https://goo.gl/Ybk7yU). 

Will this feature be available in Developer Edition 49 as well? I'm asking because we will need to do a feature signoff if yes.

Comment 13

[Mason Chang [Inactive] [:mchang]]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #12)
> We finished testing based on the information received from Mason in comment
> 11. No new issue was found during our testing. More results can be seen in
> the gdoc we tracked all our testing (https://goo.gl/Ybk7yU). 
> 
> Will this feature be available in Developer Edition 49 as well? I'm asking
> because we will need to do a feature signoff if yes.

This feature isn't enabled by default at all yet. You can track it in bug 1248740. This bug was only a subset for bug 1248740. Also, this is Windows only, so you don't have to test Mac or Linux. The Mac version is riding the trains in Gecko 48 in bug 1207332.