1845309 - Google Sheets misrendering due to incorrect built-in WebGL shaders in Firefox

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Hoshino Lina]

Attachment: highp-firefox-fix.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/115.0

Steps to reproduce:

1. Open Google Sheets demo on Firefox on a Mac running Asahi Linux
2. Scroll around

Example URL: https://docs.google.com/spreadsheets/d/1BxiMVs0XRA5nFMdKvBdBZjgmUUqptlbs74OgvE2upms/edit#gid=0

Actual results:

Canvas contents immediately become blurry as they scroll. This happens because Firefox is using incorrect shader precision specifiers in its built-in shaders.

See DrawTargetWebgl::SharedContext::CreateShaders in dom/canvas/DrawTargetWebgl.cpp. The fragment shaders specify precision mediump float; and then don't use overriding precision qualifiers anywhere. This causes the texcoords to use fp16 precision on GPUs that support it, which is not sufficient for correct rendering.

Expected results:

Normal rendering.

The attached patch should fix the problem (I didn't test it in Firefox, but I made the equivalent change using Mesa's shader replacement feature).

Note that this is a common mistake and the Mesa GPU driver we've been developing actually includes a heuristic that fixes the first instance of the problem (where the varying is used as a texcoord directly), but not the second one where there is an intermediate clamp(). It is that second one that is breaking Google Sheets.

This might need a check for GL_FRAGMENT_PRECISION_HIGH to make sure it doesn't break on very old GPUs without support for that.

Comment 1

[Hoshino Lina]

Attachment: highp-firefox-fix.patch

Typo in the first hunk, I had highp varying instead of varying highp. This one should be correct.

Comment 2

[Hoshino Lina]

Also for reference, this is what the shaders look like once they get passed to Mesa (they go through some kind of optimization pass in Firefox): https://gist.github.com/asahilina/f0f81e0043894f0737d7f599d205dfc8

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Graphics: CanvasWebGL' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:jgilbert, could you have a look please?

For more information, please visit BugBot documentation.

Comment 5

[Matt F]

Would love to see this triaged and merged, running FF without hardware acceleration in order to use Google Sheets currently which isn't ideal.

Comment 6

[Ashley Hale [:ahale]]

I feel like we need to more carefully audit the use of mediump qualifiers, they're ignored by ANGLE so this is an invisible issue on Windows but can be very problematic on all other platforms. In my experience the only appropriate place to use mediump is on colors.

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: Bug 1845309 - Use highp for texcoords in DrawTargetWebgl shaders. r?jrmuizel

This issue was originally reported by Asahi Lina, and they also proposed the original
version of these changes. I have reworked them a bit just to clean up how clip TCs
are passed down so that distance AA does not have to be highp.

Their original comment describing the issue were, in part:

"See DrawTargetWebgl::SharedContext::CreateShaders in dom/canvas/DrawTargetWebgl.cpp. The fragment shaders specify precision mediump float; and then don't use overriding precision qualifiers anywhere. This causes the texcoords to use fp16 precision on GPUs that support it, which is not sufficient for correct rendering."

"Note that this is a common mistake and the Mesa GPU driver we've been developing actually includes a heuristic that fixes the first instance of the problem (where the varying is used as a texcoord directly), but not the second one where there is an intermediate clamp(). It is that second one that is breaking Google Sheets."

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bfd7fe0cf994
Use highp for texcoords in DrawTargetWebgl shaders. r=jrmuizel

Comment 9

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/bfd7fe0cf994

Comment 10

[Lee Salzman [:lsalzman]]

Lina, can you confirm if the patch I landed successfully resolves the issue for you once it is included in a nightly build?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Matt, maybe you can confirm that things look better on current Nightly builds?

Comment 12

[Matt F]

Hi Ryan,

I can't replicate this on current Firefox installed for Fedora (like I was able to 9 days ago) so I'm not sure I'd get a good indication on a nightly if this is fixed or not, as I have no 'bad' baseline to compare to. I updated Asahi Linux a few days ago (Tuesday I believe), however there was talk on the forum that Asahi would integrate a fix their end to workaround this https://discussion.fedoraproject.org/t/really-blurry-fonts-in-google-sheets-in-ff-after-scroll/89281 - I guess this got deployed in the meantime. Note: I'm not using ASAHI_MESA_DEBUG=no16.

I'll play some more and if I can replicate it, I'll then test nightly

Comment 13

[Hoshino Lina]

Are there ARM64 nightlies? Last I checked it was only x86-64 on Linux...

We actually disable float16 for all browsers now anyway, to work around bad WebGL apps, so if you use the latest driver/config build it should work either way. You have to edit the driconf (/usr/share/drirc.d/00-mesa-defaults.conf) to comment out the workaround to properly test it.

Comment 14

[Hoshino Lina]

I just got a report via Twitter that the bug is fixed in self-compiled 119.0a1 🎉

https://twitter.com/kjoonlee/status/1703044719897678186

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Now that this has gotten some verification in the wild, is this ready for a Beta approval request?

Comment 16

[Lee Salzman [:lsalzman]]

Comment on attachment 9352060 [details]
Bug 1845309 - Use highp for texcoords in DrawTargetWebgl shaders. r?jrmuizel

Beta/Release Uplift Approval Request

• User impact if declined: Potential Canvaas2D drawing artifacts on mobile GPUs.
• 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): Verified. Shouldn't overtly change the way anything is rendered, but just fixes a potential bug.
• String changes made/needed:
• Is Android affected?: Yes

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9352060 [details]
Bug 1845309 - Use highp for texcoords in DrawTargetWebgl shaders. r?jrmuizel

Approved for landing on mozilla-beta before the merge, will be in the 118.0 release candidate, thanks.

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/3b6b652d2e6a

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9352060 [details]
Bug 1845309 - Use highp for texcoords in DrawTargetWebgl shaders. r?jrmuizel

Approved for 115.4esr.

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/5a104875c516