Closed Bug 1600472 Opened 3 months ago Closed 1 month ago

WebRender breaks font subpixel AA on many sites on Nightly

Categories

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

72 Branch
Desktop
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 --- unaffected
firefox72 --- wontfix
firefox73 --- fixed
firefox74 --- fixed

People

(Reporter: hwti, Assigned: gw, NeedInfo)

References

(Depends on 1 open bug, Regression)

Details

(Keywords: access, nightly-community, regression)

Attachments

(4 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

Load www.reddit.com, www.google.com or mail.google.com (with an account), and look at font rendering.

Actual results:

Those sites are using grayscale rendering, unless I disable WebRender.
This seems to be a regression, since Firefox 70.0.1 (with WebDriver enabled) doesn't have the issue (most of the text is using subpixel AA).

Expected results:

Subpixel AA (ClearType) should be used, as with WebRender disabled, especially since there is no strong hinting to compensate.

Is it possible for you to get a regression window using https://mozilla.github.io/mozregression/?

Flags: needinfo?(hwti)

Bug 1590284 - Construct picture cache slices for scroll roots. r=kvark

With this patch, picture cache slices are constructed each time
a new scroll root is established. This reduces rasterization
cost on pages that have large fixed position elements, and pages
that contain multiple scroll roots.

Differential Revision: https://phabricator.services.mozilla.com/D50026

In the removed text "However, it does mean that we reduce number of tiles / GPU memory, and keep subpixel AA".
So I guess losing subpixel AA was intended, but the issue IHMO is it's probably mixed with subpixel layout and/or lack of hinting (but I don't know if it's still possible to do proper strong hinting with DirectWrite and current Windows fonts, let alone web fonts).

Flags: needinfo?(hwti)
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → Windows 10
Regressed by: 1590284
Hardware: Unspecified → Desktop
Flags: needinfo?(gwatson)

Loïc, do you see Subpixel AA on the same sites in Chrome?

Flags: needinfo?(hwti)

In Chrome, on Windows :

  • www.google.com : subpixel AA, except the google applications menu which is sometimes using grayscale AA
  • mail.google.com : mostly grayscale AA
  • www.reddit.com : grayscale AA

But there are opposite cases : on this page, Chrome uses grayscale AA while Firefox uses subpixel AA.
Btw, it seems the FiraGO font used here lacks vertical hinting, such as for the horizontal line in "A", or the additional half-width gray line on top of "R" : this is present in both browsers, but less visible in Chrome since the whole rendering is more blurry (probably less hinting, and grayscale AA).

Chrome text rendering on Windows is different from Firefox, often much more blurry (probably different hinting parameters, and perhaps even subpixel layout differences). So for me most of the time grayscale AA looks better in Chrome, but subpixel AA looks better in Firefox (but it might depend on the font).

But on Linux I use grayscale AA with full hinting enabled at system level (including FREETYPE_PROPERTIES=truetype:interpreter-version=35), and in this configuration I prefer Chrome rendering to Firefox one.

Flags: needinfo?(hwti)

We are expecting to lose subpixel AA on some pages where Gecko previously has subpixel AA. This is related to the recent work to integrate WR with native compositor interfaces. The trade off is an expected significant performance and battery life win. Our aim is to still have subpixel AA anywhere that Chrome manages to implement subpixel AA.

Given that, and the comment above - it sounds like we are matching what Chrome does on mail.google.com and also on www.reddit.com, but we are not getting subpixel AA on the google home page, whereas Chrome is - is that right? If so, I'll start looking at this bug by trying to repro on the google home page.

Flags: needinfo?(gwatson) → needinfo?(hwti)

I seem to be getting subpixel AA on the google.com home page and also search results (tested on Windows + Linux). Is there a specific URL where you don't see subpixel AA on that domain (where Chrome does have subpixel AA)?

For the google home page (doesn't matter if logged in or not), I see grayscale AA in Firefox Nightly for :

  • search field
  • search buttons
  • Gmail, Images, Sign in on the top right corner
  • Google applications menu

The bar at the bottom has subpixel AA.
The search results page has subpixel AA too.

Flags: needinfo?(hwti)

The grayscale AA rendering of Firefox doesn't look good IHMO.
Chrome rendering is more blurry, which sometimes looks better with grayscale AA (Reddit homepage for example), but :

  • sometimes it results in extreme blurriness : Chrome devtools on Windows (grayscale AA) look really bad compared to the same on Linux with full hinting
  • Firefox rendering is usually better when comparing both with subpixel AA on Windows (especially on older fonts like Arial)

Could the new behavior be configurable, if it isn't too much of a burden to support different code paths ?

If this new behavior isn't toggled dynamically on a page (depending on animations for example), could the layout and hinting be adjusted accordingly, since there wouldn't be any need to match the subpixel AA sizes ?
That would be text positioned on integer coordinates, probably rounded to integer size, strong (GDI classic ?) hinting, and real grayscale AA rendering (not ClearType-like rendering converted back to grayscale, not sure if that is currently the case).
That's what Chrome does on Linux when fontconfig is configured to grayscale AA with full hinting, and FREETYPE_PROPERTIES is set to "truetype:interpreter-version=35" (else the hinting gets lowered). But that's a gobal setting, ie not depending on the page content, so it already knows how it will be rendered during the layout phase.

OK, I was able to reproduce that now, thanks. It looks like an issue caused by the fixed position footer content. I'll take a look and see how we can handle this better.

Making the setting configurable (allowing subpixel AA in more cases at the cost of some performance / battery) is something we've discussed, and may be able to implement in future (not sure on the priority though).

Unfortunately, the behavior is dependent on page content, which will make it difficult to adjust layout parameters, I think (although I'm no expert at all in those areas). We only disable subpixel AA if we can't establish that there is an opaque background rectangle behind the text runs in question. At the moment, WR is quite conservative in this regard (since it's a really noticeable artifact if you render subpixel AA on a transparent surface). WR should get better at handling more of these cases in future, which will allow subpixel AA in more cases than we enable it now.

The glyphs should be getting rasterized in this case in real grayscale AA, I believe (will need to confirm this though).

There's a potentially simple fix for this, which I have prototyped locally.

We only want to create slices where there's a performance win from having extra cache slices (to reduce rasterization during scrolling). The performance win from this generally scales linearly with the number of pixels in the slice.

We can get a reasonable estimate of this by calculating the area of the fixed position cluster as a percentage of the area of the viewport rectangle for this scroll root. By doing this, we can say it's only worth creating a slice if the fixed position element is a significant portion of the viewport rectangle.

I tested this and it works well. My concern is that a heuristic like that is prone to weird edge cases. For example:

  • If the fixed position element gets animated with a weird scaling transform, our estimates will be incorrect.
  • If a display list happens to animate the size of the fixed position element around that threshold value, we might be enabling / disabling subpixel AA causing artifacts.

I want to avoid heuristics like this where possible, since we don't want to end up having to constantly tweak these depending on various pages and other heuristics. However, in this case it might make sense and be quite a reasonable compromise. I'll ponder it a bit more.

Assignee: nobody → gwatson

(In reply to Glenn Watson [:gw] from comment #11)

  • If a display list happens to animate the size of the fixed position element around that threshold value, we might be enabling / disabling subpixel AA causing artifacts.

This is the aspect that concerns me the most. Also, from a user's perspective, it would be strange to get subpixel AA sometimes but not all the time.

I think in Loïc's configuration we should just never create transparent slices for the purposes of scrolling. That means we will do GPU work on every scroll ("total overdraw of the affected area" times "size of the affected area"), but that's not too different from what we were doing pre-WebRender: Our non-WebRender compositor would use a component alpha layer for this case, which has a compositing cost equivalent to 2x overdraw, so non-WebRender would pay a cost of 3 * size of the affected area (1 for the opaque layer in the background + 2 for the component alpha layer), which in many cases will have been even more than the equivalent WebRender rasterization work.

Here's a proposal:

  • There's a boolean allow_sacrificing_subpixel_aa_for_scrolling setting, per window, set by Gecko based on platform and display DPI, and possibly also based on hardware/software WR in the future.
  • For the tab content area, there's a number_of_slices_created_for_scrolling count which takes slice culling into account.
  • The idea is to limit number_of_slices_for_scrolling to 2 or 3.
  • Implementation: Only allow creating slices for scrolling purposes if
    • doing so doesn't increase number_of_slices_for_scrolling past the limit, and
    • the slice is opaque, or doesn't contain text, or allow_sacrificing_subpixel_aa_for_scrolling is true.

How do you feel about this proposal?

In general, this sounds like a great idea to me.

We don't know with certainty (though we can make a reasonable guess) that a slice is fully opaque when we cut the scene up into slices. Things like the presence of a complex clip or animated transform mean that we only make the exact opacity determination during frame building, rather than scene building.

Is the slice opacity check vital to what you described above? It sounds like the other conditions and options would still be suitable to achieve what we want here?

It probably makes sense to also expose allow_sacrificing_subpixel_aa_for_scrolling as a user preference that can be overridden by the user if they want to?

Flags: needinfo?(mstange)

(In reply to Glenn Watson [:gw] from comment #13)

Is the slice opacity check vital to what you described above? It sounds like the other conditions and options would still be suitable to achieve what we want here?

Just to be clear: You're proposing a behavior such that, if allow_sacrificing_subpixel_aa_for_scrolling is false, no slice would be created for the fixed element any time it contains text, even if such a slice could have been opaque? That sounds ok to me, sure.

Flags: needinfo?(mstange)

Oh and yes, I think having an about:config pref for this is a good idea. Let's not expose it in the regular preferences UI though.

Depends on: 1599606
Pushed by gwatson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa07114fb54a
Part 1 - Add API for configuring performance vs. quality. r=mstange
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

Reopened since there are more patches to land before this is fixed.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Target Milestone: mozilla73 → ---

I would like to chime in

(In reply to Glenn Watson [:gw] from comment #6)

We are expecting to lose subpixel AA on some pages where Gecko previously has subpixel AA.

This is completely unacceptable from "access" point of view for users with disabilities and diseases. For users with unhealthy eyes, it can cause fatigue, asthenopia and headaches, because of unsharpy and blurry text rendering. I'm hoping this will be reevaluated and reconsidered.

(In reply to Glenn Watson [:gw] from comment #6)

The trade off is an expected significant performance and battery life win.

Is performance win visible and perceptible in real usage or is it just only measurable in synthetic test?
For battery life win, I have created long time ago some forgotten bug #1351755, which will save battery life much more.

(In reply to Glenn Watson [:gw] from comment #6)

Our aim is to still have subpixel AA anywhere that Chrome manages to implement subpixel AA.

Firefox have to be better than Chrome. Firefox font rendering is still superior to Chrome one. Let's not downgrade it.

Keywords: access
Version: Trunk → 72 Branch

(In reply to Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #20)

I would like to chime in

(In reply to Glenn Watson [:gw] from comment #6)

We are expecting to lose subpixel AA on some pages where Gecko previously has subpixel AA.

This is completely unacceptable from "access" point of view for users with disabilities and diseases. For users with unhealthy eyes, it can cause fatigue, asthenopia and headaches, because of unsharpy and blurry text rendering. I'm hoping this will be reevaluated and reconsidered.

In https://phabricator.services.mozilla.com/D56316, we have landed a configuration setting that tells WR to force subpixel AA even if there is a performance cost. This will be set under the following conditions:

  • Resolution is <= 1920 x 1080 (at these resolutions, subpixel AA is most useful). At higher resolutions such as 4k, the performance costs and reduced usefulness of subpixel AA mean we can default to favoring the performance setting.
  • There will be an about:config configuration value that users who really want subpixel AA forced even at 4k resolution can enable it.
  • We might also default to performance option in some other configurations (e.g. if the GPU driver is broken and we fall back to software rendering).

(In reply to Glenn Watson [:gw] from comment #6)

The trade off is an expected significant performance and battery life win.

Is performance win visible and perceptible in real usage or is it just only measurable in synthetic test?
For battery life win, I have created long time ago some forgotten bug #1351755, which will save battery life much more.

It's a really significant battery and performance win at higher resolutions on pages that are affected by this setting. It can result in almost halving the total memory bandwidth per frame.

(In reply to Glenn Watson [:gw] from comment #6)

Our aim is to still have subpixel AA anywhere that Chrome manages to implement subpixel AA.

Firefox have to be better than Chrome. Firefox font rendering is still superior to Chrome one. Let's not downgrade it.

Does the above cover your concerns?

(In reply to Glenn Watson [:gw] from comment #22)

It's a really significant battery and performance win at higher resolutions on pages that are affected by this setting. It can result in almost halving the total memory bandwidth per frame.

So I guess this is more for IGP on laptops, especially with an HiDPI display.
On desktop, with a dedicated GPU (so lots of memory bandwidth), this could perhaps default to subpixel AA even at higher resolutions like 2560x1440 (or just rely to the DPI settings).

(In reply to Loïc Yhuel from comment #23)

(In reply to Glenn Watson [:gw] from comment #22)

It's a really significant battery and performance win at higher resolutions on pages that are affected by this setting. It can result in almost halving the total memory bandwidth per frame.

So I guess this is more for IGP on laptops, especially with an HiDPI display.
On desktop, with a dedicated GPU (so lots of memory bandwidth), this could perhaps default to subpixel AA even at higher resolutions like 2560x1440 (or just rely to the DPI settings).

Yes, that's right. We'll probably tweak the exact defaults - as you mentioned, it's reasonable on (most) dedicated GPUs to default to subpixel AA.

The priority flag is not set for this bug.
:jbonisteel, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jbonisteel)
Blocks: wr-72
Flags: needinfo?(jbonisteel)
Priority: -- → P3

This patch only allows sacrificing subpixel anti-aliasing when the
screen size is larger than WUXGA, and when the force disable pref is not
set. In the future, we may also add disable this for high end GPUs.

This also consolidates the WebRender debug flags to use the same
signaling infrastructure to avoid needing to store the debug flag state
and check on each transaction. Instead it now applies the debug flag
updates when the gfxVar changes.

Blocks: wr-73
No longer blocks: wr-72

Hello! What do you mean, "firefox72 --- fix-optional"? Can you tell me how to enable subpixel AA for Firefox 72.0?
On my 24 "1920x1080 monitor, some fonts began to look not very nice. (
Thank you.

We (incorrectly) didn't realize that this affected 72, so the patch didn't land in time. As a temporary step to work around this, you can disable WR by setting the gfx.webrender.force-disabled flag in about:config (a restart will be required). I'll post further updates here once we have more information on landing the fix.

Pushed by aosmond@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec0e9a72aebb
Disable allowing sacrificing subpixel anti-aliasing for small screens. r=jrmuizel

Assuming this also occurs on firefox74 based on the regression range.

add missing header include which apparently Linux didn't need, try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b2cd0f7ee6a562ac25402183a1ea8c732482105

Flags: needinfo?(aosmond)
Pushed by aosmond@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eac17a862c29
Disable allowing sacrificing subpixel anti-aliasing for small screens. r=jrmuizel
Status: ASSIGNED → RESOLVED
Closed: 3 months ago1 month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

Did you want to nominate this for Beta73 uplift? It's early in the cycle, so I think it's reasonable to do so if the patch isn't particularly scary.

Flags: needinfo?(gwatson)

I think it's reasonable to uplift this, yep. Jeff, does that sound good to you?

Flags: needinfo?(gwatson) → needinfo?(jmuizelaar)
Regressions: 1609268

Andrew, can you merge the piece from 1609268 and this change and request uplift to Beta/

Flags: needinfo?(jmuizelaar) → needinfo?(aosmond)

Comment on attachment 9121316 [details]
[beta] Bug 1600472 - Disable allowing sacrificing subpixel anti-aliasing for small screens.

Beta/Release Uplift Approval Request

  • User impact if declined: May have poor font rendering with smaller screens, when we don't have a performance reason to do so.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This just touches some WebRender debug prefs and adds the subpixel anti-aliasing flag. Before the user had no control over the setting, now they have a pref that can toggle it as well.
  • String changes made/needed:
Attachment #9121316 - Flags: approval-mozilla-beta?
Comment on attachment 9121316 [details]
[beta] Bug 1600472 - Disable allowing sacrificing subpixel anti-aliasing for small screens.

Improves font rendering for WR users on smaller screens. Approved for 73.0b6.
Attachment #9121316 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Backed out for failures on position-sticky-scroll-with-clip-and-abspos.html

backout: https://hg.mozilla.org/releases/mozilla-beta/rev/d238440ed92954087986bfe2bbdfd773f37135b4

push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&selectedJob=285265684&revision=e42d01b594e86c09d0c1e4afd4f92c756535dc33&searchStr=%28wr1%29&group_state=expanded

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285265684&repo=mozilla-beta&lineNumber=33804

[task 2020-01-16T22:49:01.875Z] 22:49:01 INFO - TEST-START | /css/css-position/position-sticky-scroll-with-clip-and-abspos.html
[task 2020-01-16T22:49:01.875Z] 22:49:01 INFO - PID 1908 | 1579214941872 Marionette INFO Testing http://web-platform.test:8000/css/css-position/position-sticky-scroll-with-clip-and-abspos.html == http://web-platform.test:8000/css/css-position/position-sticky-scroll-with-clip-and-abspos-ref.html
[task 2020-01-16T22:49:01.894Z] 22:49:01 INFO - PID 1908 | [Parent 4496, Main Thread] WARNING: we only accept nsIURI interface type, patch welcome: file z:/build/build/src/dom/ipc/PropertyBagUtils.cpp, line 112
[task 2020-01-16T22:49:01.894Z] 22:49:01 INFO - PID 1908 | [Child 9116, Main Thread] WARNING: Trying to request nsIHttpChannel from DocumentChannelChild, this is likely broken: file z:/build/build/src/netwerk/ipc/DocumentChannelChild.cpp, line 63
[task 2020-01-16T22:49:01.973Z] 22:49:01 INFO - PID 1908 | 1579214941966 Marionette INFO [31] Emitted TestRendered event
[task 2020-01-16T22:49:02.038Z] 22:49:02 INFO - PID 1908 | 1579214942032 Marionette WARN [31] http://web-platform.test:8000/css/css-position/position-sticky-scroll-with-clip-and-abspos-ref.html overflows viewport (width: 783, height: 2216)
[task 2020-01-16T22:49:02.059Z] 22:49:02 INFO - PID 1908 | [Parent 4496, Main Thread] WARNING: we only accept nsIURI interface type, patch welcome: file z:/build/build/src/dom/ipc/PropertyBagUtils.cpp, line 112
[task 2020-01-16T22:49:02.060Z] 22:49:02 INFO - PID 1908 | [Child 9116, Main Thread] WARNING: Trying to request nsIHttpChannel from DocumentChannelChild, this is likely broken: file z:/build/build/src/netwerk/ipc/DocumentChannelChild.cpp, line 63
[task 2020-01-16T22:49:02.140Z] 22:49:02 INFO - PID 1908 | 1579214942132 Marionette INFO [31] Emitted TestRendered event
[task 2020-01-16T22:49:02.186Z] 22:49:02 INFO - PID 1908 | 1579214942182 Marionette WARN [31] http://web-platform.test:8000/css/css-position/position-sticky-scroll-with-clip-and-abspos.html overflows viewport (width: 783, height: 2216)
[task 2020-01-16T22:49:02.205Z] 22:49:02 INFO - PID 1908 | 1579214942194 Marionette INFO Allowed 1787-1787 pixels different, maximum difference per channel 92-92
[task 2020-01-16T22:49:02.281Z] 22:49:02 INFO - TEST-UNEXPECTED-FAIL | /css/css-position/position-sticky-scroll-with-clip-and-abspos.html | Testing http://web-platform.test:8000/css/css-position/position-sticky-scroll-with-clip-and-abspos.html == http://web-platform.test:8000/css/css-position/position-sticky-scroll-with-clip-and-abspos-ref.html

Flags: needinfo?(gwatson)

Andrew, any ideas why this would have caused those issues in beta?

Flags: needinfo?(gwatson) → needinfo?(aosmond)

Looks like some reftest fuzzing was removed in nightly by bug 1608885, but not in beta, and these patches were related to fixing it. I've included the necessary fuzz removal in this patch (hopefully).

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb6f917934fc9508eaaa38f8757667c2bfaf332a

I'll re-request uplift assuming it is green.

Attachment #9121316 - Attachment is obsolete: true
Flags: needinfo?(aosmond)
Attachment #9122127 - Flags: review+
Comment on attachment 9122127 [details]
[beta] Bug 1600472 - Disable allowing sacrificing subpixel anti-aliasing for small screens. v2

Moving the approval over to the new patch. This should be landing for the 73.0b8 build happening today.
Attachment #9122127 - Flags: approval-mozilla-beta+
Attachment #9121316 - Flags: approval-mozilla-beta+

(In reply to Ryan VanderMeulen [:RyanVM] from comment #45)

Comment on attachment 9122127 [details]
[beta] Bug 1600472 - Disable allowing sacrificing subpixel anti-aliasing for
small screens. v2

Moving the approval over to the new patch. This should be landing for the
73.0b8 build happening today.

Thanks. Looks like the try is fine (I often see that wrench failure with OSX, unrelated).

== Change summary for alert #24806 (as of Mon, 27 Jan 2020 13:40:11 GMT) ==

Regressions:

1% raptor-tp6-reddit-firefox-cold fcp linux64-shippable-qr opt 279.96 -> 283.17

Improvements:

3% raptor-tp6-reddit-firefox-cold fcp linux64-shippable-qr opt 280.18 -> 272.58

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=24806

Attached image example1.png

How to make fonts with (gfx.webrender.force-disabled = false (default)) be the same as with (gfx.webrender.force-disabled = true)
Now they are different. By default, they have become some kind of thin.

(In reply to alexandrv1k from comment #49)

Created attachment 9125838 [details]
example1.png

How to make fonts with (gfx.webrender.force-disabled = false (default)) be the same as with (gfx.webrender.force-disabled = true)
Now they are different. By default, they have become some kind of thin.

Up

Please don't change bug flags.

Flags: needinfo?(gwatson)

Lee might have some ideas on the text differences?

Flags: needinfo?(gwatson) → needinfo?(lsalzman)
You need to log in before you can comment on or make changes to this bug.