1402737 - Arial font rendering changes from hinted to subpixel when I interact with the document in many ways

Status: RESOLVED FIXED

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

Description

[Chris Morgan]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20170924100550

Steps to reproduce:

I have been noticing this in LiquidPlanner (which I can’t give access to) for the last couple of weeks or so.

The page in question has a panel that pops up (a child of the body, `position: fixed`, with a `position: relative` child with a `position: absolute; overflow: auto` child that contains all the content except for the panel title, which doesn’t change as described below). This panel contains lots of things including various text (set in Arial) and form controls.

When the panel opens, the font rendering is crisp. (Hinted rendering, I suppose.) Or sometimes it’s fuzzy, but within a second becomes crisp. (There is an opacity transition in play which may affect things, though normally the transition finishes before the text appears.)

When I interact with the panel in various ways (normally clicking anywhere seems to do it, scrolling definitely does it), the font rendering for the entire panel changes to fuzzy. (Non-hinted, subpixel rendering, I presume.)

When I switch away from the tab and immediately return, the text rendering is still fuzzy; if I do other things before returning it normally seems to have returned to crisp rendering.

I have a 2dppx primary display and two external 1dppx displays; I can only reproduce this on the 1dppx displays; the high-DPI display doesn’t use hinted rendering.

The transition from hinted to subpixel rendering is jarring (the fonts become heavier).

If necessary, I can try to craft a new document that reproduces the issue.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Chris, any chance you could use mozregression (http://mozilla.github.io/mozregression/quickstart.html) to pinpoint when this started happening?  Default options when running that tool should be fine.

Comment 2

[Chris Morgan]

I’ll do that in a couple of days’ time (I’m away from my low-DPI displays today and tomorrow).

Comment 3

[Chris Morgan]

Turns out it started earlier than I thought, almost three months ago: mozilla-central 2017-07-10 was good, 2017-07-11 bad. There may have been other confounding factors that prevented it from cropping up (I observed during this process that if the screen was too wide it didn’t demonstrate the error).

Here’s the end of the bisect log:

2017-10-03T10:33:55: INFO : Narrowed inbound regression window from [26b15288, a625a2e9] (3 builds) to [26b15288, 392ed89e] (2 builds) (~1 steps left)
2017-10-03T10:33:55: DEBUG : Starting merge handling...
2017-10-03T10:33:55: DEBUG : Using url: https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=392ed89ec2730a48d10b1cec741e86a242d28aa3&full=1
2017-10-03T10:33:59: DEBUG : Found commit message:
Enable Advanced Layers on Nightly Windows 8+ builds. (bug 1375743, r=milan)

2017-10-03T10:33:59: INFO : The bisection is done.
2017-10-03T10:33:59: INFO : Stopped

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Chris Morgan from comment #0)

If you can get us STR or a test case that would help a lot. AL did change how we do subpixel AA in very complex cases, but we haven't yet seen it cause problems and we haven't been able to craft a test case that has problems either.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Chris, if I link you to a build that has logging, would you be able to reproduce the problem and send us the log?

Comment 6

[Chris Morgan]

Sure. I’m also going to attempt to minimise the test case.

Comment 7

[Chris Morgan]

Attachment: Minimal demonstration of the bug

Attached is a minimal test case. Removing anything causes the rendering to switch to either hinted or antialiased rendering, depending on what you remove. (There is thus still inconsistency which isn’t ideal, but it’s not as disconcerting as having it switch techniques based purely on clicking on it!)

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Thanks so much, Chris - I can indeed reproduce this at normal resolution.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

The bug is that Advanced Layers doesn't support component-alpha in intermediate surfaces. It's extremely rare, but when it does happen in an animation/transition, it is visible enough to be jarring. I guess we'll have to bite the bullet and implement it.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, rm layer manager feature

This removes the LayerManager feature of disabling component-alpha in intermediate surfaces. Advanced Layers was the only backend to use this.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, AL support

This adds support for copying the backdrop before rendering intermediate surfaces that have component alpha.

If an intermediate surface requires a backdrop copy, it skips rendering - though it still renders child views. The entire surface is treated as invalid, and it is not cleared.

When we go to actually composite the intermediate surface into its parent layer, we first copy the backdrop into the temp surface, and then render all children. Then we can proceed as normal.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Chris could you try either of these builds to see if it fixes your problem?

64-bit: https://queue.taskcluster.net/v1/task/GbTPWvVkR--ooS9YPfm6Pw/runs/0/artifacts/public/build/setup.exe
32-bit: https://queue.taskcluster.net/v1/task/K1ekR2l0SYOt2_OFrWtGMg/runs/0/artifacts/public/build/setup.exe

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8916881 [details] [diff] [review]
part 2, AL support

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

::: gfx/layers/mlgpu/RenderPassMLGPU.cpp
@@ +1024,5 @@
> +  visible += IntPoint::Truncate(transform._41, transform._42);
> +  visible -= mParentView->GetTargetOffset();
> +
> +  MLGRenderTarget* target = mAssignedLayer->GetRenderTarget();
> +  mDevice->CopyTexture(

Are we guaranteed that the destination has pixels that cover everything in the containers visible region?

Just want to make sure we don't hit an error for weird corner cases where we didn't have a minimal visible region.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/790ef14142e8
Remove the ability for LayerManagers to disable complex component alpha cases. (bug 1402737 part 1, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c7df778de29
Support component-alpha for intermediate surfaces in Advanced Layers. (bug 1402737 part 2, r=mattwoodrow)

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

I made sure the copy rect was clamped to the source and dest texture sizes.

Comment 17

[Chris Morgan]

I tested the try build 632db96125dd0a55a8e1f26de33730b41cb8f7db and it looks like it’s fixed, but I’m away from my low-DPI displays today and so shan’t confirm it absolutely until later (it’s much easier to see the difference on low-DPI displays).

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/790ef14142e8
https://hg.mozilla.org/mozilla-central/rev/7c7df778de29

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to 57 or can it ride the 58 train?

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8916880 [details] [diff] [review]
part 1, rm layer manager feature

I'd like to uplift, since even though it's rare it's quite noticeable when it happens.

Approval Request Comment
[Feature/Bug causing the regression]: Advanced Layers
[User impact if declined]: poor subpixel font rendering in some animations
[Is this code covered by automated tests?]:
[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]: Both patches in this bug
[Is the change risky?]: No
[Why is the change risky/not risky?]: The incorrect rendering was a result of an optimization; this patch reverts the algorithm back to the one used in Firefox 56. The biggest risk is that we will instead render fonts incorrectly in a slightly different way.
[String changes made/needed]:

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8916880 [details] [diff] [review]
part 1, rm layer manager feature

Recent regression, Beta57+

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e316a95d1d60
https://hg.mozilla.org/releases/mozilla-beta/rev/8a55ee69a86b

Comment 23

[Chris Morgan]

It’s nice having this fixed (it’s definitely fixed on the current Nightly). I love filing bugs with Mozilla, because they’re pretty much always fixed quickly and effectively. Thanks, David.