vello_sparse_shaders: Avoid using structs for storing data#1619
Conversation
743b8cd to
1cdb7c0
Compare
1cdb7c0 to
d3413f8
Compare
grebmeg
left a comment
There was a problem hiding this comment.
-
Do I understand correctly that the generated GLSL currently does not emit
highpon the relevant struct fields, and that this is what naga generates today? My understanding is that the issue may be deeper, though: even if we emithighpthere, some drivers may still actually demote struct fields tomediump? -
Could you please share which devices you observed the issue on and this change fixes it?
-
I find the proposed approach is hard to read and maintain. I’d prefer to look for another solution:
-
First, I think we may be able to fix precision qualifier emission in naga. From a quick look, write_struct_body does not seem to write each member through
write_typewith a precision prefix, unlike file-level defaults, explicit highp on sampler/image declarations, and sampler function parameters. So I believe this might be fixable in naga for the GLSL backend. -
Second, based on WebGL issue KhronosGroup/WebGL#3351, the problem seems reproducible in fragment shaders specifically. Could we move the struct initialisation/unpacking to the vertex shader instead? I briefly looked at WebRender, and it seems that most “unpack into a struct” logic happens in the vertex shader.
-
Yes, the issue is deeper, as you mentioned. I don't remember off the top of my head what naga currently emits (but I believe it just emits the If you want, you can play around with the following shader on the given phones. If the device work corrects it should return green, otherwise red. I was not able to adjust the shader to use structs on some way without getting rid of the red color on the given phones.
There were a couple, but some for example include Google Pixel 5, Redmi Note 11 and Samsung Galaxy Tab A9. So it seems to affect Adreno GPU's in the low 600 range, maybe also 500 and 400, I haven't checked. But it affects many devices. Also note that if you want to test it you need to do it in conjunction with #1620, just this PR alone might not fix it if I remember correctly.
As mentioned, from my experiments just adding precision qualifiers doesn't help here. You would need some kind of "flatten structs" functionality in naga, and that doesn't seem to exist (and seems very hard, if at all possible, to implement, given that structs can probably be arbitrarily nested?)
From my experiments, even structs that are passed from the vertex shader to the fragment shader are affected by this. This is the whole reason why #1620 is also needed to fix the issue. It seems like you literally cannot use structs in fragment shaders. :/ Feel free to try to double check my findings if you want to, would be good to have confirmation that you also weren't able to find a workaround, but I've spent a lot of time on it, so I'm feeling pretty certain that I didn't miss anything here. |
|
So, I tried it again now. As mentioned, just adding <!doctype html>
<html>
<head>
<style>
html, body, canvas {
width: 100%;
height: 100%;
margin: 0;
display: block;
overflow: hidden;
}
</style>
</head>
<body>
<canvas></canvas>
<script>
const canvas = document.querySelector("canvas");
const gl = canvas.getContext("webgl2");
const vertexSource = `#version 300 es
void main() {
vec2 positions[3] = vec2[3](
vec2(-1.0, -1.0),
vec2( 3.0, -1.0),
vec2(-1.0, 3.0)
);
gl_Position = vec4(positions[gl_VertexID], 0.0, 1.0);
}
`;
const fragmentSource = `#version 300 es
precision highp float;
precision highp int;
struct Pair {
highp uint f0;
};
layout(location = 0) out vec4 out_color;
void main() {
Pair p = Pair(0x00010000u);
uint mode = p.f0 >> 16;
float green = (mode == 1u) ? 1.0 : 0.0;
float red = 1.0 - green;
out_color = vec4(red, green, 0.0, 1.0);
}
`;
function shader(type, source) {
const result = gl.createShader(type);
gl.shaderSource(result, source);
gl.compileShader(result);
return result;
}
const program = gl.createProgram();
gl.attachShader(program, shader(gl.VERTEX_SHADER, vertexSource));
gl.attachShader(program, shader(gl.FRAGMENT_SHADER, fragmentSource));
gl.linkProgram(program);
gl.useProgram(program);
gl.bindVertexArray(gl.createVertexArray());
function draw() {
const scale = devicePixelRatio;
canvas.width = innerWidth * scale;
canvas.height = innerHeight * scale;
gl.viewport(0, 0, canvas.width, canvas.height);
gl.drawArrays(gl.TRIANGLES, 0, 3);
}
addEventListener("resize", draw);
draw();
</script>
</body>
</html>Struct unpacking in the vertex shader also doesn't work, the following still shows up red: <!doctype html>
<html>
<head>
<style>
html, body, canvas {
width: 100%;
height: 100%;
margin: 0;
display: block;
overflow: hidden;
}
</style>
</head>
<body>
<canvas></canvas>
<script>
const canvas = document.querySelector("canvas");
const gl = canvas.getContext("webgl2");
const vertexSource = `#version 300 es
precision highp float;
precision highp int;
struct Pair {
highp uint f0;
};
flat out Pair v_pair;
void main() {
vec2 positions[3] = vec2[3](
vec2(-1.0, -1.0),
vec2( 3.0, -1.0),
vec2(-1.0, 3.0)
);
v_pair = Pair(0x00010000u);
gl_Position = vec4(positions[gl_VertexID], 0.0, 1.0);
}
`;
const fragmentSource = `#version 300 es
precision highp float;
precision highp int;
struct Pair {
highp uint f0;
};
flat in Pair v_pair;
layout(location = 0) out vec4 out_color;
void main() {
uint mode = v_pair.f0 >> 16;
float green = (mode == 1u) ? 1.0 : 0.0;
float red = 1.0 - green;
out_color = vec4(red, green, 0.0, 1.0);
}
`;
function shader(type, source) {
const result = gl.createShader(type);
gl.shaderSource(result, source);
gl.compileShader(result);
return result;
}
const program = gl.createProgram();
gl.attachShader(program, shader(gl.VERTEX_SHADER, vertexSource));
gl.attachShader(program, shader(gl.FRAGMENT_SHADER, fragmentSource));
gl.linkProgram(program);
gl.useProgram(program);
gl.bindVertexArray(gl.createVertexArray());
function draw() {
const scale = devicePixelRatio;
canvas.width = innerWidth * scale;
canvas.height = innerHeight * scale;
gl.viewport(0, 0, canvas.width, canvas.height);
gl.drawArrays(gl.TRIANGLES, 0, 3);
}
addEventListener("resize", draw);
draw();
</script>
</body>
</html>However, removing the wrapper struct and just passing <!doctype html>
<html>
<head>
<style>
html, body, canvas {
width: 100%;
height: 100%;
margin: 0;
display: block;
overflow: hidden;
}
</style>
</head>
<body>
<canvas></canvas>
<script>
const canvas = document.querySelector("canvas");
const gl = canvas.getContext("webgl2");
const vertexSource = `#version 300 es
precision highp float;
precision highp int;
flat out uint v_f0;
void main() {
vec2 positions[3] = vec2[3](
vec2(-1.0, -1.0),
vec2( 3.0, -1.0),
vec2(-1.0, 3.0)
);
v_f0 = 0x00010000u;
gl_Position = vec4(positions[gl_VertexID], 0.0, 1.0);
}
`;
const fragmentSource = `#version 300 es
precision highp float;
precision highp int;
flat in uint v_f0;
layout(location = 0) out vec4 out_color;
void main() {
uint mode = v_f0 >> 16;
float green = (mode == 1u) ? 1.0 : 0.0;
float red = 1.0 - green;
out_color = vec4(red, green, 0.0, 1.0);
}
`;
function shader(type, source) {
const result = gl.createShader(type);
gl.shaderSource(result, source);
gl.compileShader(result);
return result;
}
const program = gl.createProgram();
gl.attachShader(program, shader(gl.VERTEX_SHADER, vertexSource));
gl.attachShader(program, shader(gl.FRAGMENT_SHADER, fragmentSource));
gl.linkProgram(program);
gl.useProgram(program);
gl.bindVertexArray(gl.createVertexArray());
function draw() {
const scale = devicePixelRatio;
canvas.width = innerWidth * scale;
canvas.height = innerHeight * scale;
gl.viewport(0, 0, canvas.width, canvas.height);
gl.drawArrays(gl.TRIANGLES, 0, 3);
}
addEventListener("resize", draw);
draw();
</script>
</body>
</html> |
grebmeg
left a comment
There was a problem hiding this comment.
So, I tried it again now. As mentioned, just adding highp to the struct field doesn't change anything. The following HTML page still shows up red for me:
Feel free to try to double check my findings if you want to, would be good to have confirmation that you also weren't able to find a workaround, but I've spent a lot of time on it, so I'm feeling pretty certain that I didn't miss anything here.
Thanks for trying my suggestion, but you’re right, there doesn’t seem to be a good visible workaround here. I’ve also spent some time looking for alternatives, but unsuccessfully. After reading through the changes one more time, I don’t find them nearly as unclear as I did at first, so I’m in favor of landing this to allow some devices with buggy Adreno GPUs to render properly. Thanks for your work!
# Conflicts: # sparse_strips/vello_sparse_shaders/shaders/render_strips.wgsl
) This is a continuation to linebender#1619 and builds on top of it. With this PR in place, Vello Hybrid runs on Google Pixel 5 and Redmi Note 11 without any problems!
…inebender#1659) Drafted as a follow-up to [this comment](linebender#1619 (comment)). Some old Adreno GPU drivers silently downgrades the precision of struct values in fragment shaders to 16 bits, regardless of the precision the shader requested. The last few commits flattened the existing fragment shaders to dodge the bug; this commit prevents the workaround from silently regressing. `assert_no_structs_in_fragment_shader` parses the WGSL with naga, walks the call graph reachable from the `@fragment` entry point, and rejects any function argument, return value, local variable, `Compose`, or `ZeroValue` of struct type. Struct-typed uniform globals and vertex IO structs are unaffected because their fields are only ever read as scalars/vectors inside the fragment shader. The lint runs both at build time (from `compile_wgsl_shader`, which the build script invokes for every shipped WGSL) and as a unit test (`every_shipped_shader_passes_the_lint`) that runs in CI. See linebender#1604 linebender#1619 linebender#1620 for context. Generated with AI assistance.
This is part 1 of implementing a solution to the problem raised in #1604. Basically, instead of unpacking paint data into dedicated structs, we instead just sample the raw texels whever required and then create dedicated getter methods that extract the field in question from that texel. It does make things a bit less readable, but it's necessary to avoid the precision loss that has been raised in that PR.