Closed Bug 1402737 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: me, Assigned: dvander)

References

(Regressed 1 open bug)

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(3 files)

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.
Component: Untriaged → Graphics: Text
Product: Firefox → Core
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.
Flags: needinfo?(me)
Priority: -- → P3
Whiteboard: [gfx-noted]
I’ll do that in a couple of days’ time (I’m away from my low-DPI displays today and tomorrow).
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
Flags: needinfo?(me)
Assignee: nobody → dvander
Has Regression Range: --- → yes
Flags: needinfo?(dvander)
Flags: needinfo?(bas)
Keywords: regression
Priority: P3 → P1
(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.
Flags: needinfo?(dvander)
Chris, if I link you to a build that has logging, would you be able to reproduce the problem and send us the log?
Flags: needinfo?(me)
Sure. I’m also going to attempt to minimise the test case.
Flags: needinfo?(me)
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!)
Attachment #8915483 - Attachment mime type: text/plain → text/html
Thanks so much, Chris - I can indeed reproduce this at normal resolution.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(bas)
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.
This removes the LayerManager feature of disabling component-alpha in intermediate surfaces. Advanced Layers was the only backend to use this.
Attachment #8916880 - Flags: review?(matt.woodrow)
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.
Attachment #8916881 - Flags: review?(matt.woodrow)
Attachment #8916880 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8916881 - Flags: review?(matt.woodrow) → review+
Pushed by danderson@mozilla.com:
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)
I made sure the copy rect was clamped to the source and dest texture sizes.
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).
Flags: needinfo?(me)
https://hg.mozilla.org/mozilla-central/rev/790ef14142e8
https://hg.mozilla.org/mozilla-central/rev/7c7df778de29
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Is this something we should consider uplifting to 57 or can it ride the 58 train?
Flags: needinfo?(dvander)
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]:
Flags: needinfo?(dvander)
Attachment #8916880 - Flags: approval-mozilla-beta?
Comment on attachment 8916880 [details] [diff] [review]
part 1, rm layer manager feature

Recent regression, Beta57+
Attachment #8916880 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Depends on: 1408689
Regressions: 1551211
You need to log in before you can comment on or make changes to this bug.