Font spacing occasionally inconsistent in Aurora

VERIFIED FIXED in Firefox 57

Status

()

Core
Graphics: Text
P3
normal
VERIFIED FIXED
a year ago
2 months ago

People

(Reporter: Ray Satiro, Unassigned)

Tracking

({fonts})

53 Branch
mozilla57
x86_64
Windows 7
fonts
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Created attachment 8831406 [details]
Clipboard.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170126004004

Steps to reproduce:

About a week ago the font rendering in Aurora changed ever so slightly, and occasionally I will notice some words do not have letters that are properly spaced. Although I can 100% reproduce this issue with some private websites I do not have a public page that can be used to reproduce.


Actual results:

In the attached screenshot you can see the 'sh' in shipping is too close together, basically the h is moved over a pixel column to the left. Other samples I've captured show the same thing, there is a letter or two that is erroneously one column to the left.

I bisected but the window is very wide, it's thousands of commits.

 5:35.94 INFO: application_buildid: 20170122071525
 5:35.94 INFO: application_changeset: 24a81d93e07cc96300f8e1f5c69034dd4743bd63
 5:35.94 INFO: application_name: Firefox
 5:35.94 INFO: application_repository: https://hg.mozilla.org/releases/mozilla-aurora
 5:35.94 INFO: application_version: 52.0a2
Was this inbound build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry' or 'exit' and press Enter): good
 6:08.44 INFO: Narrowed inbound regression window from [b58eb6e9, dca7b42e] (3 revisions) to [24a81d93, dca7b42e] (2 revisions) (~1 steps left)
 6:08.44 INFO: Oh noes, no (more) inbound revisions :(
 6:08.44 INFO: Last good revision: 24a81d93e07cc96300f8e1f5c69034dd4743bd63
 6:08.44 INFO: First bad revision: dca7b42e6c67219398e6419293c075ef829adb29
 6:08.44 INFO: Pushlog:
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=24a81d93e07cc96300f8e1f5c69034dd4743bd63&tochange=dca7b42e6c67219398e6419293c075ef829adb29




Expected results:

The letters should be properly spaced.
(Reporter)

Updated

a year ago
Has Regression Range: --- → yes
Keywords: fonts
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64

Updated

a year ago
Component: Untriaged → Graphics: Text
Product: Firefox → Core
Could you please attach your about:support data, to see more details of the exact configuration involved?

Do you see the same behavior in Nightly? If so, a Nightly (rather than Aurora) regression window would be much more precise.
Flags: needinfo?(raysatiro)
(Reporter)

Comment 2

a year ago
Created attachment 8831460 [details]
about_support_aurora_53.0a2_20170126004004.txt

(In reply to Jonathan Kew (:jfkthame) from comment #1)
> Could you please attach your about:support data, to see more details of the
> exact configuration involved?

Ok it's attached.

> Do you see the same behavior in Nightly? If so, a Nightly (rather than
> Aurora) regression window would be much more precise.

I don't see the same behavior in nightly. Tested 54.0a1 20170127030206.
Flags: needinfo?(raysatiro)
Did it occur with Nightly in the 53.0a1 timeframe, before 53 moved to the Aurora channel? (I.e. I'm wondering if something has recently been fixed on Nightly.)

E.g. a build from https://download-origin.cdn.mozilla.net/pub/firefox/nightly/2017/01/2017-01-23-03-02-11-mozilla-central/
(Reporter)

Comment 4

a year ago
Created attachment 8831464 [details]
Firefox Trunk 20170127030206 - HWA Comparison.png

(In reply to Jonathan Kew (:jfkthame) from comment #3)
> Did it occur with Nightly in the 53.0a1 timeframe, before 53 moved to the
> Aurora channel? (I.e. I'm wondering if something has recently been fixed on
> Nightly.)

It appears there's a similar issue in Nightly but only if I disable hardware acceleration. See this attachment for an example. I will try to bisect it.
Priority: -- → P3
Whiteboard: [gfx-noted]
(Reporter)

Comment 5

a year ago
Created attachment 8831485 [details]
Firefox mozilla-inbound 20161222030652 - 1st and 2nd Run.png

Disregard HWA setting, that doesn't have anything to do with it. For some reason the bug isn't present in a first run in Nightly like it is in Aurora. When on a hunch I had disabled HWA and restarted, the bug appeared just because it wasn't the first run, not because HWA was disabled. I don't know why it takes a restart in Nightly to reproduce but not Aurora.


Bisect was successful:

12:49.27 INFO: Narrowed inbound regression window from [3fe0ff6b, 96cf05fa] (4 revisions) to [66b31b9d, 96cf05fa] (2 revisions) (~1 steps left)
12:49.27 INFO: Oh noes, no (more) inbound revisions :(
12:49.27 INFO: Last good revision: 66b31b9d9a1a636ef8855e578867bdb34c435546
12:49.27 INFO: First bad revision: 96cf05faacfe6ba130553323bde9f0bb80298c8a
12:49.27 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=66b31b9d9a1a636ef8855e578867bdb34c435546&tochange=96cf05faacfe6ba130553323bde9f0bb80298c8a

12:49.89 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1325199


Steps to reproduce:

mozregression --launch 96cf05faacfe6ba130553323bde9f0bb80298c8a
- Go to about:support in first tab; observe font spacing good; leave tab open
- Open another tab and install Restart Firefox extension:
https://addons.mozilla.org/en-US/firefox/addon/restartfox
- Click the toolbar button to restart.
- Observer first tab about:support font spacing bad
Attachment #8831464 - Attachment is obsolete: true
Flags: needinfo?(bas)
Looks to me like only calling gfxDWriteFont::UpdateClearTypeUsage() on Paint events may not be sufficient, as it means we may be constructing textruns using the wrong setting (and then updating it before painting them). Specifically, in this case I suspect we construct a textrun (which includes getting glyph advances and setting positions) with mUseClearType=true, as that's its initial value, but then at Paint-event time we discover that the user has font smoothing disabled and so we set it to false. Then we render strongly-hinted pixel-grid-snapped monochrome glyphs using fractional-pixel positions that were computed for CT-antialiased ones, and the result is poor spacing.

Maybe we could set mUseClearType during initialization of the gfxDWriteFont, instead of on Paint events. That would guarantee it is set appropriately before we do any textrun construction using the font.
(In reply to Jonathan Kew (:jfkthame) from comment #6)
> Looks to me like only calling gfxDWriteFont::UpdateClearTypeUsage() on Paint
> events may not be sufficient, as it means we may be constructing textruns
> using the wrong setting (and then updating it before painting them).
> Specifically, in this case I suspect we construct a textrun (which includes
> getting glyph advances and setting positions) with mUseClearType=true, as
> that's its initial value, but then at Paint-event time we discover that the
> user has font smoothing disabled and so we set it to false. Then we render
> strongly-hinted pixel-grid-snapped monochrome glyphs using fractional-pixel
> positions that were computed for CT-antialiased ones, and the result is poor
> spacing.
> 
> Maybe we could set mUseClearType during initialization of the gfxDWriteFont,
> instead of on Paint events. That would guarantee it is set appropriately
> before we do any textrun construction using the font.

Presumably gfxDWriteFont initialization is expensive enough that the high cost of updating mUseClearType at that time is acceptable? Note that we also need to update in on the drawing backend, there was a bug on my initial patch that makes stuff disappear if there's an inconsistency. This is probably exceedingly rare though, maybe, since this is a static anyway, we can just do this once on gfxPlatform initialization? Let me suggest a patch and we can see if it works.
Flags: needinfo?(bas)
Comment hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
Comment on attachment 8831680 [details]
Bug 1334761: UpdateClearTypeUsage on startup.

https://reviewboard.mozilla.org/r/108236/#review109252

This would resolve things for the vast majority of the time, but it's not 100% reliable because the system setting can change while the browser is running; if that happens, we're back to the situation where we might instantiate a gfxDWriteFont with the no-longer-correct value, use it to shape some text runs, and then when we go to paint the text we'll update the cleartype setting, so we'll paint with glyphs that don't correspond to the metrics used for shaping.

Given that it's pretty rare for people to change their font smoothing settings, maybe we don't care that we could mis-render text immediately after such a change? Seems unfortunate, though, to leave a known case where we'll have this inconsistency.

How expensive is the Update call? Instantiating a gfxDWriteFont is going to be a relatively heavyweight operation already, so I'd be surprised if adding this to it made any detectable difference.
:bas - ping for followup here; as I don't think the proposed patch will solve the issue completely, I'd prefer to include an Update call in the gfxDWriteFont initialization, unless you think that is too expensive.
Flags: needinfo?(bas)
(In reply to Jonathan Kew (:jfkthame) from comment #10)
> :bas - ping for followup here; as I don't think the proposed patch will
> solve the issue completely, I'd prefer to include an Update call in the
> gfxDWriteFont initialization, unless you think that is too expensive.

Yeah, it's a tricky consideration, it's pretty expensive, but like you said, this is a pretty heavy weight operation anyway. It's important to note as far as I can tell, as you already suggested, this only affects a single frame of new gfxDWriteFront stuff written. It seemed wasteful to me to waste cycles on gfxDWriteFont::gfxDWriteFont for a situation that extremely rare.
Flags: needinfo?(bas)
It'd be a rare situation, but if it happens, it would persist longer than a single frame, I think: the issue would occur when a textrun has been constructed using one setting of mUseClearType, but then gets rendered with a different setting. The poor drawing would then persist as long as the affected textrun(s) live.

Or could we detect when mUseClearType actually changes, and force textruns to be flushed and reconstructed at that point? That would clear up any mismatch.
(Reporter)

Comment 13

4 months ago
Created attachment 8907381 [details]
Firefox Trunk 20170908220146.PNG

I just switched back to nightly after I recent hiatus and I still see occasional inconsistent font spacing in the tabs and bookmark bar. A screenshot is attached.
Created attachment 8908011 [details] [diff] [review]
Update ClearType usage at startup, and flush caches and reflow everything if it changes during the session

Here's a patch that checks the setting at startup, but also handles the (rare) case of dynamic changes by flushing our caches and ensuring we rebuild all textruns if the setting has changed, so that we don't risk rendering text with stale glyph spacing. There should be no perf impact on the usual case where the cleartype setting remains constant.
Attachment #8908011 - Flags: review?(bas)
Attachment #8908011 - Flags: review?(bas) → review+
Attachment #8831680 - Flags: review?(jfkthame)

Comment 15

4 months ago
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4832416f0a86
Update ClearType usage at startup, and flush caches and reflow everything if it changes during the session. r=bas
https://hg.mozilla.org/mozilla-central/rev/4832416f0a86
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Reporter)

Comment 17

2 months ago
Thanks guys. I just started using Nightly 20171113220112 and can confirm this issue appears to be fixed.
Status: RESOLVED → VERIFIED
(Reporter)

Comment 18

2 months ago
Created attachment 8931108 [details]
Capture.PNG

Actually it looks like this may still be an issue. Look at the i and the l letters in the attached screenshot from 59.0a1 (2017-11-21) (64-bit).
You need to log in before you can comment on or make changes to this bug.