Fix glsl shader for Android Mali-GXXx GPUs and Vulkan API 1.3.xxx

[Alex2782]

Fixes #86621

Issue #73384 maybe only partially, not 3D on PowerVR Rogue GPUs comment

---

Test on Firebase

---

Test Project: GlowTest.zip

• Glow on/off
• DOF Blur on/off
• rendering/scaling_3d/scale = 0.5

web-build_20240606_x988_panther-33-en_US-portrait_video.mp4

• Glow
• DOF Blur
• Rendering Scale 3D

[clayjohn]

I have taken a close look at this and have reached out to ARM engineers for assistance. This is clearly an ARM driver bug that only impacts newer Mali devices.

To fix #86621, we only have to make the change in tonemap.glsl (and the other files that have exactly the same vertex shader), we don't need to remove all array accesses.

It seems the problem actually comes from the clamp() function call that clamps the UV coordinates. The UV coordinates should be (0,0), (0,2), (2,0). But on the broken devices the coordinates are (-2,-2), (-2,2), (2,-2). This is consistent with the clamp failing to clamp to (0,0). In fact, if you change the line to uv_interp = clamp(gl_Position.xy, vec2(-1.0, -1.0), vec2(1.0, 1.0)) * 2.0; // saturate(x) * 2.0, then desktop behaves the same as the problematic Mali devices.

ARM is taking a look as it seems this is reproducible in newer driver versions (I confirmed in driver version 46.0.0). They may be able to help us identify a more elegant workaround.

I think we should wait to hear back from them as the current workaround will have a performance impact as it emits many more instructions including branches. Ideally, we should be able to workaround the bug with something much less intrusive.

[Alex2782]

This is a strange UV bug, without if / else if I couldn't find another solution.

some GLSL tests

---

void main() {
	vec2 base_arr[3] = vec2[](vec2(-1.0, -1.0), vec2(-1.0, 3.0), vec2(3.0, -1.0));
	//gl_Position = vec4(base_arr[gl_VertexIndex], 0.0, 1.0);
	
	//BUG
	//vec2 base = gl_Position.xy;
	//uv_interp = clamp(base, vec2(0.0, 0.0), vec2(1.0, 1.0)) * 2.0; // saturate(x) * 2.0
	
	//BUG
	//uv_interp.x = clamp(gl_Position.x, 0.0, 1.0) * 2.0;
	//uv_interp.y = clamp(gl_Position.y, 0.0, 1.0) * 2.0;

	//BUG
	// GLSL Docs: clamp returns the value of x constrained to the range minVal to maxVal. 
	// The returned value is computed as min(max(x, minVal), maxVal).
	//uv_interp = min(max(gl_Position.xy, vec2(0.0, 0.0)), vec2(1.0, 1.0)) * 2.0;

	
	int vertex_index = gl_VertexIndex;
	vec2 vertex_base = base_arr[vertex_index];


	//OK
	/*
	if (vertex_index == 0) {
		vertex_base = base_arr[0];
	} else if (vertex_index == 1) {
		vertex_base = base_arr[1];
	} else {
		vertex_base = base_arr[2];
	}
	*/

	//BUG
	/*
	if (vertex_index == 0) {
		vertex_base = base_arr[gl_VertexIndex];
	} else if (vertex_index == 1) {
		vertex_base = base_arr[gl_VertexIndex];
	} else {
		vertex_base = base_arr[gl_VertexIndex];
	}
	*/	

	//OK
	/*
	if (vertex_index == 0) {
		vertex_base = base_arr[vertex_index];
	} else if (vertex_index == 1) {
		vertex_base = base_arr[vertex_index];
	} else {
		vertex_base = base_arr[vertex_index];
	}
	*/
	
	//BUG
	/*
	for (int i=0; i < 100; i++) {
		vertex_base = base_arr[vertex_index];
	}
	*/

	//BUG
	for (int i=0; i < 3; i++) {
		if (vertex_index == 0 || vertex_index == 1 || vertex_index != 0) {
			vertex_base = base_arr[vertex_index];
		}
	}
	
	gl_Position = vec4(vertex_base, 0.0, 1.0);
	uv_interp = clamp(vertex_base, vec2(0.0, 0.0), vec2(1.0, 1.0)) * 2.0; // saturate(x) * 2.0

}

Fragment: UV debug

	if (uv_interp.x < -0.5 && uv_interp.y < -0.5) {
		color += vec4(0.2, 0.0, 0.0, 1.0);
	}
	else if (uv_interp.x < -0.2 && uv_interp.y < -0.2) {
		color += vec4(0.0, 0.2, 0.0, 1.0);
	}
	else if (uv_interp.x < 0.0 && uv_interp.y < 0.0) {
		color += vec4(0.0, 0.0, 0.2, 1.0);
	}
	else if (uv_interp.x > 0.5 && uv_interp.y > 0.5) {
		color += vec4(0.0, 0.2, 0.2, 1.0);
	}
	else if (uv_interp.x > 0.2 && uv_interp.y > 0.2) {
		color += vec4(0.2, 0.0, 0.2, 1.0);
	}	
	else if (uv_interp.x > 0.0 && uv_interp.y > 0.0) {
		color += vec4(0.2, 0.2, 0.0, 1.0);
	}

	frag_color = color;
}

UV-BUG (negative values)	UV-OK (positive values)

[clayjohn]

An update on this. ARM has confirmed that the internal shader compiler is replacing the clamp() function with a single min() operation. For some reason the compiler doesn't make this mistake when using the branches. However, we should avoid using branches for performance reasons.

I have asked if they know of a better workaround

[Alex2782]

the internal shader compiler is replacing the clamp() function with a single min() operation.

A few days ago I tried it without the clamp function, same issue.

	//BUG
	// GLSL Docs: clamp returns the value of x constrained to the range minVal to maxVal. 
	// The returned value is computed as min(max(x, minVal), maxVal).
	uv_interp = min(max(gl_Position.xy, vec2(0.0, 0.0)), vec2(1.0, 1.0)) * 2.0;

[clayjohn]

A few days ago I tried it without the clamp function, same issue.

I tried the same. ARM confirmed it results in the same code as clamp()

[Alex2782]

Should we wait longer?

I think we should wait to hear back from them as the current workaround will have a performance impact as it emits many more instructions including branches. Ideally, we should be able to workaround the bug with something much less intrusive.

However, we should avoid using branches for performance reasons.

I could also solve without #include and only in GLSL files for Glow, DOF Blur and Rendering Scale 3D.

Bunnymark on Adreno 650 (3y old device), GLES3 commits canvas.glsl if / else if was more performant: #87352 (comment) as an array with 6 vec2 values.

Details

---

• Samsung Tab S7 (Android 13)
• Debug App
• 2k resolution (no stretch)

I ran 2 tests for each:

• benchmark output: 10960 and 10820 with if / else if
• benchmark output: 10900 and 10840 with is_x_set / is_y_set
• benchmark output: 10660 and 10760 with vertex_base_arr[6] and gl_VertexID % 6
• benchmark output: 10860 and 10700 with vertex_base_arr[4] and gl_VertexID % 4

---

I think that the performance improvements or deteriorations will not be measurable because Glow etc. is only executed once per frame.

[akien-mga]

I tested this PR (rebased on latest master) on a Google Pixel 7a (Mali-G710) and I confirm this fixes the issue for me.

[clayjohn]

I could also solve without #include and only in GLSL files for Glow, DOF Blur and Rendering Scale 3D.

Let's do this for now. I suspect this will slightly harm performance of those shaders on modern devices. However, as you pointed out, each of those shaders run at most a few times per frame, so the overall performance impact would be minimal.

I would like to find a more elegant workaround in the future. But we should solve this for 4.3.

So please go ahead and apply your fix directly to Glow, DoF Blur, and Rendering Scale 3D and I think Tonemap.glsl needs it as well

Also, for context, we are still waiting to hear back from ARM. But we can't hold off the release while we wait, so it is best to go forward with a known workaround in the meantime

[Alex2782]

• tonemap.glsl for Glow and Rendering Scale 3D
• and bokeh_dof_raster.glsl for DOF Blur

Test on Pixel 7a

[clayjohn]

Looks good! Thank you.

Let's go with this workaround for 4.3 and then hopefully do something else once we have a different workaround

[akien-mga]

Thanks!

[clayjohn]

For posterity, ARM has confirmed that this is a driver bug (now fixed in r51) and using branches as we did is the best workaround. The alternative is to use a uniform buffer to store the position array