Closed Bug 1321901 Opened 3 years ago Closed 3 years ago

Use IDWriteFontFace::GetRecommendedRenderingMode for font rendering mode in Skia

Categories

(Core :: Graphics: Text, defect, P3, major)

52 Branch
x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files, 2 obsolete files)

Some fonts still look off compared to d2d when using Skia. I was testing a couple of sites and noticed a difference on specific fonts at specific sizes. After looking around, it was because Skia was using CLEARTYPE_NATURAL_SYMMETRIC mode [1] by default. Gecko uses DWRITE_RENDERING_MODE_DEFAULT [2], which at least from what I can tell, uses the same mode as GetRecommendedRenderingMode. If I change Skia to use GetRecommendedRenderingMode depending on the typeface, we get pretty much the exact same fonts as D2D, fixing the lightness issue.

I was able to reproduce this outside of Gecko as well.

[1] http://searchfox.org/mozilla-central/source/gfx/skia/skia/src/ports/SkScalerContext_win_dw.cpp#321
[2] http://searchfox.org/mozilla-central/source/gfx/thebes/gfxWindowsPlatform.cpp#1265
Compare this on non-hidpi on d2d vs Skia. The underside of the U is lighter and there is fuzz above and below the D.
Will file upstream bug as well.
Attachment #8816606 - Flags: review?(lsalzman)
Attachment #8816606 - Flags: review?(lsalzman)
Found some text that is still lighter than d2d. The first patch fixes the original test case.
Comment on attachment 8816606 [details] [diff] [review]
Use Recommended RenderingMode depending on typeface

Still trying to fix the lightness issue on some fonts, but this patch fixes a few other weird looking text too. Will fixup the lightness issue in another patch.
Attachment #8816606 - Flags: review?(lsalzman)
Comment on attachment 8816606 [details] [diff] [review]
Use Recommended RenderingMode depending on typeface

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

r=lsalzman yes from work week
Attachment #8816606 - Flags: review?(lsalzman) → review+
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e0dfe88d42
Use IDWriteFontFace::GetRecommendedRenderingMode for font rendering mode in Skia. r=lsalzman
Flags: needinfo?(mchang)
For the light test case, I figured out it was because the default contrast we use with d2d is 1.0, but it is 0.5 for Skia. However, setting contrast to 1.0 in Skia makes other fonts too thick.
Looks like fonts where we force GDI mode use a contrast of 0.5, but non GDI fonts use 1.0. Even though in gfxWindowsPlatform, both are set to use 1.0 contrast. Note taking for myself here.
From playing with GDI fonts outside of gecko, it looks like a contrast with the LUT of ~0.7-0.75 is ~= D2D with GDI Dwrite params which contain a contrast of 1.0.
Attached patch Use Recommended rendering mode (obsolete) — Splinter Review
Some of the test cases were failing because they would use DWRITE_RENDERING_MODE_OUTLINE, which isn't really supported. Cairo would fallback to DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL_SYMMETRIC[1], which is what Skia would do previously as well. Updated to copy this behavior. Some of the reftests needed extra fuzzing too.

[1] http://searchfox.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-d2d-surface.cpp#3767
Attachment #8816606 - Attachment is obsolete: true
Attachment #8818681 - Flags: review?(lsalzman)
Comment on attachment 8818681 [details] [diff] [review]
Use Recommended rendering mode

Do we need the include "nsDebug.h"? It doesn't look like it.
Attachment #8818681 - Attachment is obsolete: true
Attachment #8818681 - Flags: review?(lsalzman)
Attachment #8818924 - Flags: review?(lsalzman)
Attachment #8818924 - Flags: review?(lsalzman) → review+
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29a19471a5aa
Use IDWriteFontFace::GetRecommendedRenderingMode for font rendering mode in Skia. r=lsalzman
https://hg.mozilla.org/mozilla-central/rev/29a19471a5aa
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8818924 [details] [diff] [review]
Use Recommended rendering mode

Approval Request Comment
[Feature/Bug causing the regression]: Skia on windows, bug 1007702
[User impact if declined]: Some fonts will look slightly different for users.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: Will probably also need bug 1323587.
[Is the change risky?]: No
[Why is the change risky/not risky?]: It just sets a different rendering mode flag depending on the font.
[String changes made/needed]: None
Attachment #8818924 - Flags: approval-mozilla-aurora?
Comment on attachment 8818924 [details] [diff] [review]
Use Recommended rendering mode

change font rendering mode with skia on windows, for aurora52
Attachment #8818924 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.