Closed Bug 1640523 Opened 1 year ago Closed 1 year ago

Text is Unreadable in Any Webpage with an extremely large Drop Shadow

Categories

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

76 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla79
Tracking Status
firefox-esr68 --- unaffected
firefox76 --- wontfix
firefox77 --- wontfix
firefox78 --- verified
firefox79 --- verified

People

(Reporter: u658110, Assigned: gw)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, reproducible)

Attachments

(2 files)

Attached file Quality Browsing.html

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

Steps to reproduce:

See attached HTML file.
1. The page is relatively long.
2. The style contains a drop shadow.
3. Attempt to read the text half way down the page.
FF Version: 76.0.1, Windows, 64-bit

Actual results:

The text is distorted beyond recognition. The effect occurs strongest at the vertical center of the page. The effect is persistent.

Expected results:

The text should be legible.

Component: Untriaged → Graphics: Text
Keywords: access, reproducible
Product: Firefox → Core
Component: Graphics: Text → Graphics: WebRender
Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(dmalyshau)
Regressed by: 1626827, 1617050

The crash fix could make it blurry only if the WR local-space picture scaling doesn't work correctly. Routing to Bert to check this.

Flags: needinfo?(dmalyshau) → needinfo?(bpeers)

My understanding of the problem:

  • the text surface does not establish a raster root;
  • the text surface is 1494 x 65555; (surface_rect in post_update);
  • in take_context, we ask for a 1494 x 30264 when scrolling halfway;
  • since it's not a raster root, this will crash trying to allocate a giant rendertarget;
  • the crash was fixed by always scaling within limits, even when not a raster root;
  • thus we scale to 25% to fit 30264 < 8192, text is effectively 374 pixels wide and upsampled;

So the code is doing exactly what we need it to, to not crash. It just doesn't look great.
The only fix I see is to avoid asking for a 30264 pixels-high canvas when the screen is only 2K pixels tall, but that's not related to the scaling/crash changes per se.
And even so, you can switch from blur to perspective and get similar problems where there's no clear fix like that (Bug 1634162).

In short I don't think it's a code bug or math oopsie, but a deeper question of how we switched from screen based to surface/RT based rendering? Am I wrong? Thoughts? Thanks :)

Flags: needinfo?(bpeers) → needinfo?(dmalyshau)

PS: that said, it's not normal for the text to get bigger as well - it should be the same size, just blurrier. I'll take a look at that.

Edit: this is perhaps also due to the text not being a raster root, so it has no raster_config, which is how we pass the scale factor to update_font_instance. The crash fix might need an alternative way to route that scale. However cbrewster is also changing how this works with more scaling factors folded directly into the surface.DPI, maybe that could be an alternative?

Blocks: gfx-triage
Severity: -- → S3
Priority: -- → P3
See Also: → 1634162, 1639729

Not really relevant for a11y triage, since this impacts all sighted users with equal severity; it doesn't disproportionately affect low vision users, for example.

Keywords: access
Blocks: wr-78
No longer blocks: gfx-triage

Test case is attached to this bug

Flags: needinfo?(rares.doghi)

I suppose we can also consider switching the local raster root ON for the case where scaling is enforced (and the picture is otherwise not in the local raster root). Glenn, please have a look and tell what you think.

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

It's not totally clear to me why we need to scale this in the case where we don't create a raster root (as the clip-to-screen functionality in this case should result in us not creating a huge target). But I haven't looked in any detail, I'm probably just missing some context - I'll take a look at this today to understand better what's going on.

Assignee: nobody → gwatson
Flags: needinfo?(gwatson)

The problem appears to be in https://searchfox.org/mozilla-central/source/gfx/wr/webrender/src/picture.rs#4826 (which was introduced in https://phabricator.services.mozilla.com/D68230).

In this case, the clip_inflation value is calculated to be ~21,000 is incorrect. This makes WR think the size of the target we need to allocate is much bigger than what we actually need.

There's two potential issues here:

  • The code to calculate clip_inflation appears to have a bug in it.
  • Looking more closely at the code, it's not clear to me why clip_inflation is even needed (although it made sense at the time). It seems like clipped shouldn't need to include any inflation as this is handled later in the specific blur / drop-shadow filter match branches.
Summary: Text is Unreadable in Any Webpage with Drop Shadow → Text is Unreadable in Any Webpage with an extremely large Drop Shadow

The logic for expanding the drop shadow clip rect was calling the
map_vector method in the ScaleOffset struct.

This method was incorrect (it was mapping the value as a point,
including the offset). This is fixed by modifying the map_vector
method to exclude the offset.

However, the text run snapping code was also calling map_vector
but expecting it to treat the value there as a point, so also
introduce a map_point method and switch the text run code
to use that.

Flags: needinfo?(rares.doghi)
Flags: needinfo?(nical.bugzilla)
Pushed by gwatson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f77b09a15a11
Fix render quality on pages with very large drop shadows. r=nical,aosmond
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79

The patch landed in nightly and beta is affected.
:gw, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(gwatson)

Comment on attachment 9152602 [details]
Bug 1640523 - Fix render quality on pages with very large drop shadows.

Beta/Release Uplift Approval Request

  • User impact if declined: Major text quality issues in this specific case (I suspect is very uncommon on real content, but it's hard to know for sure).
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: As per attached test case in bugzilla.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Small patch, easy to back out if needed and only affects a small part of WR.
  • String changes made/needed:
Flags: needinfo?(gwatson)
Attachment #9152602 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9152602 [details]
Bug 1640523 - Fix render quality on pages with very large drop shadows.

Fixes a pretty bad text rendering regression. Approved for 78.0b3.

Attachment #9152602 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

I have managed to reproduce the issue using Fx78.0a1 buildID: 20200524212021.
The issue is verified fixed using Fx79.0a1 and Fx78.0b3 on windows 10, ubuntu 18.04 and macOS 10.13. Now the entire text in the document is readable without issues.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.