1932416 - Some images have started rendering black [Adreno 3xx]

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Kestrel]

Attachment: Background image renders black

Samsung Galaxy Tab 4 (SM-T530)
LineageOS 18.1 (Android 11)
Qualcomm Adreno (TM) 305
OpenGL ES 3.0 V@140.0

STR:

1. Visit https://www.w3schools.com/cssref/playdemo.php?filename=playcss_background-position.

Expected:
Image of pink tree with owl.

Actual:
Image of black tree silhouette.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8c38504b6e67fe72df6cff3cc2e3344742117707&tochange=46c23625da22e4d899c421fcb57cde9b1e0d201b

Regressed by Bug 1913568.

Comment 1

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

:jnicol, since you are the author of the regressor, bug 1913568, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Jamie Nicol [:jnicol]]

Interesting. This makes no sense whatsover. Definitely a driver bug. But I can indeed reproduce on an Adreno 306 device.

The bug occurs when rendering with the brush_image_ALPHA_PASS_REPETITION shader. setting background-repeat: no-repeat; avoids the issue, as we no longer use the REPETITION shader variant.

The regressor made a tiny change to the brush_image shader: scaling the UV coordinates by the texture size if the NORMALIZED_UVS flag is set. This flag isn't even set in the test case (it's only used for video).

I suspect the shader is miscompiling due to be too complex. There are a lot of branches in the repetition code. Perhaps this additional one is one branch too many. Replacing it with a mix() appears to avoid the bug.

Comment 3

[Jamie Nicol [:jnicol]]

Attachment: Bug 1932416 - Avoid shader miscompile due to if statement on Adreno devices. r?#gfx-reviewers

The brush_image shader handles normalized input UV coordinates by
first unnormalizing them if the flag is set, allowing the same logic
to be used subsequently regardless of whether the flag is
set. Unfortunately this appears to cause a miscompilation in the
REPETITION variant of the shader on some Adreno devices. Removing the
additional branch by replacing the if statement with a mix() avoids
the issue.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d36e9e1ce0a
Avoid shader miscompile due to if statement on Adreno devices. r=gfx-reviewers,nical

Comment 5

[amarc]

https://hg.mozilla.org/mozilla-central/rev/0d36e9e1ce0a

Comment 6

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

The patch landed in nightly and beta is affected.
:jnicol, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox133 to wontfix.

For more information, please visit BugBot documentation.

Comment 7

[Donal Meehan [:dmeehan]]

Slight correction on Comment 6, Fx133 is now in RC and beta is closed.
We could review a release uplift request on this :jnicol if you think it's suitable.
It's not new in Fx133 but we could consider it in the planned dot release.

Comment 8

[Jamie Nicol [:jnicol]]

Given this only affects a relatively small number of users, and the bug's existence in the first place shows how risky changing shader code can be, I'm not convinced we want to uplift it straight to release.