1334761 - Font spacing occasionally inconsistent in Aurora

Status: VERIFIED FIXED

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

Description

[Ray Satiro]

Attachment: 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.

Comment 1

[Jonathan Kew [:jfkthame]]

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.

Comment 2

[Ray Satiro]

Attachment: 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.

Comment 3

[Jonathan Kew [:jfkthame]]

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/

Comment 4

[Ray Satiro]

Attachment: 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.

Comment 5

[Ray Satiro]

Attachment: 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

Comment 6

[Jonathan Kew [:jfkthame]]

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.

Comment 7

[Bas Schouten (:bas.schouten)]

(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.

Comment 8

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1334761: UpdateClearTypeUsage on startup.

Review commit: https://reviewboard.mozilla.org/r/108236/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108236/

Comment 9

[Jonathan Kew [:jfkthame]]

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.

Comment 10

[Jonathan Kew [:jfkthame]]

: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.

Comment 11

[Bas Schouten (:bas.schouten)]

(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.

Comment 12

[Jonathan Kew [:jfkthame]]

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.

Comment 13

[Ray Satiro]

Attachment: 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.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4832416f0a86

Comment 17

[Ray Satiro]

Thanks guys. I just started using Nightly 20171113220112 and can confirm this issue appears to be fixed.

Comment 18

[Ray Satiro]

Attachment: 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).

Comment 19

[Ray Satiro]

Attachment: Screenshot of Firefox 77 tabs

Can someone reopen this, because it's still an issue. I observe it in both Firefox 77 and Thunderbird 68.7.

In the attached screenshot of my Firefox tabs you can see that 'n' and/or 't' in the 'Enter A Bug' tab has incorrect spacing; and 'a' and/or 'r' in the 'Clipboard.png' tab has incorrect spacing.

Comment 20

[Lee Salzman [:lsalzman]]

Ray, can you please file a new bug with your specific case? That would be better than reopening this bug.

Comment 21

[Ray Satiro]

(In reply to Lee Salzman [:lsalzman] from comment #20)

Ray, can you please file a new bug with your specific case? That would be better than reopening this bug.

Filed as bug 1631674.