Closed Bug 1695912 Opened 3 years ago Closed 3 years ago

"Failed to link shader program: brush_blend" when browsing bbc.com on Adreno 330

Categories

(Core :: Graphics: WebRender, defect)

Product:
Core
Component:
Graphics: WebRender
Platform:
ARM
Android
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
88 Branch
Tracking Flags:
Tracking Status
firefox88 --- fixed

People

(Reporter: h.winnemoeller, Assigned: jnicol)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

webgl-varyings.html
16.20 KB, text/html
Details
Bug 1695912 - Ensure shaders use 16 or fewer varying vectors even if driver does not pack them. r?jrmuizel
48 bytes, text/x-phabricator-request
Details | Review

Steps to reproduce

  1. Set gfx.webrender.all to true and gfx.webrender.allow-partial-present-buffer-age to false (see Bug 1695771) and restart Fenix.
  2. Open bbc.com and start opening some articles, I did try https://www.bbc.com/news/world-europe-56242617 and https://www.bbc.com/news/explainers-52380823 in private browsing mode.

Expected results
Website is displayed normally.

Actual results
Sometimes, Fenix force closes. If it does not force close, at some point, Fenix will fall back to OpenGl compositing and display the following failure log:

Failure Log
(#0) Error: Failed to load a program object with a program binary: cs_scale renderer Adreno (TM) 330 Invalid binary image passed to glProgramBinaryOES.
(#15) Error: shader-cache: Failed program_binary
(#16) Error: Failed to load a program object with a program binary: brush_image renderer Adreno (TM) 330 Invalid binary image passed to glProgramBinaryOES.
(#17) Error: shader-cache: Failed program_binary
(#18) Error: Failed to load a program object with a program binary: ps_text_run renderer Adreno (TM) 330 Invalid binary image passed to glProgramBinaryOES.
(#19) Error: shader-cache: Failed program_binary
(#20) Error: Failed to load a program object with a program binary: brush_opacity renderer Adreno (TM) 330 Invalid binary image passed to glProgramBinaryOES.
(#21) Error: shader-cache: Failed program_binary
(#22) Error: Failed to load a program object with a program binary: brush_blend renderer Adreno (TM) 330 Invalid binary image passed to glProgramBinaryOES.
(#23) Error: shader-cache: Failed program_binary
(#24) Error: Failed to link shader program: brush_blend --From Vertex Shader: Error: varying variables do not fit in 16 vectors. --From Fragment Shader: Error: varying variables do not fit in 16 vectors.
(#25) Error: wr_renderer_render: Shader(Link("brush_blend", "--From Vertex Shader:\nError: varying variables do not fit in 16 vectors.\n--From Fragment Shader:\nError: varying variables do not fit in 16 vectors.\n"))
(#26) Error: Fallback (SW-)WR to Basic
(#27) Error: Failed to link shader program: brush_blend
(#28) Error: wr_renderer_render: Shader(Link("brush_blend", ""))
(#29) Error: Compositors might be mixed (5,2)

Device Info
Vendor and Model: Sony Xperia Z2
OS version: Android 6.0.1
GPU model: Adreno 330 (WebRender Compositing)
Number of cores: 4 (Snapdragon 801)

Fenix version
Nightly 210301 17:01 (Build #2015796169)
AC: 74.0.20210301143120, 8a55899f2
GV: 88.0a1-20210301093612
AS: 72.1.0

Blocks: wr-adreno3xx

I cannot reproduce this on either of those URLs, and neither of them cause brush_blend to be compiled for me. However, if I enable gfx.webrender.precache-shaders then I can indeed reproduce. We seem to hit this problem in brush_blend_ALPHA_PASS, cs_svg_filter, and brush_yuv_image_ALPHA_PASS_TEXTURE_2D_YUV.

The driver advertises itself as having 16 varying vectors available. It appers to be up to drivers precisely how to pack varyings in to those vectors but the spec does require a minimum required algorithm. Angle has an implementation of this algorithm to test whether packing should succeed, and it says that the shaders in question should indeed fit in to 16 vecs.

So it appears the driver is not spec compliant.

Working around the driver bug is trivial: we can manually pack a few varyings in to vecs. But the part I'm struggling with is how to test this and ensure we don't regress it in the future, given that this driver is not tested on CI. I need to try to reverse engineer what's going on in the driver, and then probably add some rust bindings to angle and test this in angle_shader_validation.

My best guess at what's happening in the driver is that it doesn't do any packing whatsoever. If we sum up the number of rows for each varying without any packing, and also remove unused varyings from the count then the numbers nearly look correct: For cs_svg_image after I've manually packed some varyings so that it successfully compiles, this logic gives me 17 vecs in the vertex shader and 16 in the fragment shader. The vertex shader includes gl_Position but the fragment shader does not reference gl_FragCoord. The spec states that:

If referenced in the fragment shader (after preprocessing), the built-in special variables (gl_FragCoord, gl_FrontFacing and gl_PointCoord) are included when calculating the storage requirements of fragment inputs.

It doesn't say anything about gl_Position in the vertex shader, so maybe it doesn't count. So I think we want to sum up the number of rows used by each varying without any packing, only counting used varyings, and discounting gl_Position. Using that logic the numbers match whether or not this driver will successfully compile the shaders, for these shaders at least.

It seems like it should be possible to create webgl test case that would show this and fail in Chrome and Firefox right?

Attached file webgl-varyings.html —

This tries to compile the brush_yuv_image_ALPHA_PASS_TEXTURE_2D_YUV shader. It reproduces the bug on firefox, but on chrome webgl fails to initialize. Perhaps they have blocklisted webgl on this GPU.

By manually packing some varyings I've found a case where it takes up 17 vecs if the driver weren't doing any additional packing, and that doesn't include gl_Position or gl_FragCoord, yet it compiles successfully on the device. So maybe the driver is doing some very basic packing. In any case it seems sensible to assume the worst case and ensure our varyings fit in 16 vecs without the driver doing any additional packing

What does chrome://gpu/ say?

Flags: needinfo?(jnicol)

WebGL2: Unavailable

MSAA and depth texture buggy on Adreno 3xx prior to Android 9, also disable WebGL2: 449116, 698197, 1042214

Flags: needinfo?(jnicol)

On some Adreno 3xx devices we have observed that the driver does not
pack varyings in to vectors as efficiently as the spec mandates. This
results in some of our shaders using a greater number of varying
vectors than GL_MAX_VARYING_VECTORS (which 16 on this device), leading
to shader compilation errors at run time.

Work around this by manually packing our varyings in to fewer
vectors. Additionally, add a test to ensure that we never use more
than 16 vectors even if the driver were to perform no additional
packing.

Assignee: nobody → jnicol
Status: NEW → ASSIGNED
Pushed by jnicol@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2171d982acdf
Ensure shaders use 16 or fewer varying vectors even if driver does not pack them. r=jrmuizel

Ah, it seems that by dynamically indexing a ivec4 rather than an int[4] the loop no longer gets unrolled, and dynamically indexing the vector seems to be buggy on Adreno. If I first copy the vectors components in to a temporary array that would avoid the driver bug, but actually the loop once again gets unrolled and the copy gets optimized out, so it's identical to before.

Flags: needinfo?(jnicol)
Pushed by jnicol@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/44347d27b4bb
Ensure shaders use 16 or fewer varying vectors even if driver does not pack them. r=jrmuizel

https://hg.mozilla.org/mozilla-central/rev/44347d27b4bb

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox88: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
Regressions: 1708937
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: