1779800 - WebGL2 instanced rendering fails on Firefox 102 on Mac OS 12.2

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect, P2)

Description

[Karl]

Attachment: This test should show 10 colored triangles, but just shows one.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Firefox/102.0

Steps to reproduce:

Use the built-in variable gl_InstanceID in a WebGL2 vertex shader.

Actual results:

The value of gl_InstanceID is not correct. It seems to always be zero.
Note that this bug is not present and the test provided works fine on Firefox 102 on Linux, and it also works fine on Safari on Mac, but it fails on Firefox 102 on Mac.

Expected results:

The value of gl_InstanceID should be set properly so it can be accessed by the vertex shader.

Comment 1

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

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I can repro on macOS 13 beta.

Comment 3

[Haik Aftandilian [:haik]]

I can reproduce this on macOS 12.4 on an Intel MacBook Pro, but can not reproduce it on an Apple Silicon Mac running macOS 13 Beta (22A5295i).

On Intel, mozgression didn't help because I couldn't find a good build. Going back to builds before August 2020 the test reports "Your browser does not support WebGL 2" and just renders a black square.

On any build I tested after August 2020, the test fails. I ran mozregression with mach (below) and via the pip package and both seemed to work for this range.

$ ./mach mozregression --bad 2022-07-25 --good 2020-07-01 -a "https://bug1779800.bmoattachments.org/attachment.cgi?id=9285664"

Comment 4

[Karl]

It seems this bug does not occur if you use a vertex array buffer to provide the 3 triangle coordinates instead of calculating them in the vertex shader. So perhaps the absence of a VAO is somehow disabling the correct setting of gl_InstanceID in the vertex shader?

Comment 5

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

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

For more information, please visit auto_nag documentation.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: WebGL2-instance-test.html

Looks like Mac needs fake-vertex-attrib for an instance attrib. :S

Comment 7

[Kelsey Gilbert [:jgilbert]]

Minimal need seems to be that there needs to be a vertex attrib array enabled, with a non-zero instancing divisor, and with a vertex buffer of at least one byte attached.
I think this is a buggy optimization where the driver thinks it can truncate instanceCount to 1 if there's no instance attrib buffer.
Fortunately, it seems like we just need to do minimal setup for this case, and probably only if there's static use of gl_InstanceID but no instance buffer.
Honestly I don't think this optimization the driver is doing is valuable, so we might as well no check for gl_InstanceID at all.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Actually it looks like it doesn't even need any vertexAttribDivisor set at all. It's enough to have literally any vertexAttribArray enabled. Maybe this is an old fastpath for all-disabled vertexAttribArray drawing?
This means we can leverage our fake-vertex-attrib-0 then.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: WebGL2-instance-test.html

Comment 10

[Kelsey Gilbert [:jgilbert]]

//TEST.enableAnyAttribArray = true; // Uncomment to fix.

[...]

  if (TEST.enableAnyAttribArray) {
    gl.enableVertexAttribArray(0);
  }
  gl.vertexAttribPointer(0, 1, gl.FLOAT, false, 0, 0);

Comment 11

[Kelsey Gilbert [:jgilbert]]

Attachment: WebGL2-instance-test.html

Comment 12

[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 auto_nag documentation.

Comment 13

[Kelsey Gilbert [:jgilbert]]

MacOS GL driver really just has some serious bugs with gl_VertexID and gl_InstanceID:
https://github.com/KhronosGroup/WebGL/pull/3484

We just need workarounds for these.

Comment 14

[Kelsey Gilbert [:jgilbert]]

The main workaround for website authors is the same as we will be integrating into Firefox:
Polyfill to replace gl_VertexID and gl_InstanceID with our own IDs. E.g.:

let vdata = new Int32Array(Math.max(MAX_USED_VERTEX_ID, MAX_USED_INSTANCE_ID));
vdata = vdata.map((v,i) => i);
const vbuf = gl.createBuffer();
gl.bindBuffer(gl.ARRAY_BUFFER, vbuf);
gl.bufferData(gl.ARRAY_BUFFER, vdata, gl.STATIC_DRAW);

let loc = gl.getAttribLocation(prog, "user_VertexID");
if (loc != -1) {
  gl.enableVertexAttribArray(loc);
  gl.bindBuffer(gl.ARRAY_BUFFER, vbuf);
  gl.vertexAttribIPointer(loc, 1, gl.INT, 0, 0);
}

loc = gl.getAttribLocation(prog, "user_InstanceID");
if (loc != -1) {
  gl.enableVertexAttribArray(loc);
  gl.bindBuffer(gl.ARRAY_BUFFER, vbuf);
  gl.vertexAttribIPointer(loc, 1, gl.INT, 0, 0);
  gl.vertexAttribDivisor(loc, 1);
}

Comment 15

[Kelsey Gilbert [:jgilbert]]

And in the vertex shader:

in highp int user_VertexID;
in highp int user_InstanceID;

Comment 16

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1779800 - Fix WebGL instancing in some cases on Mac.

This reverts the code that would collapse DrawElements to DrawArrays in some cases,
because it was a correct transform in some cases.

Instead, we need to always keep around a cpu copy of the index buffer,
so that we can figure out how many fakeAttrib0 verts we need.

Finally, in order to correct for the bug where Mac offsets the base instance by first in DrawArrays,
we need to (temporarily) patch vertexAttribPointer offsets for any call with non-zero first.
(This can be disabled via MOZ_WEBGL_WORKAROUND_FIRST_AFFECTS_INSTANCE_ID=0)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47db0dc58cf1
Fix WebGL instancing in some cases on Mac. r=gfx-reviewers,lsalzman

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/47db0dc58cf1

Comment 20

[Kelsey Gilbert [:jgilbert]]

This is reasonably-likely to cause a minor perf regression on affected systems, so we might see Talos alert here.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9294347 [details]
Bug 1779800 - Fix WebGL instancing in some cases on Mac.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This fixes correctness for a category of real webgl content, and we don't want to wait on the next whole esr cycle for it.
• User impact if declined: Certain misbehaving pages due to webgl correctness issues.
• Fix Landed on Version: 106
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's on Release106 already, and we do have pretty good tests in this area, that visibly got less-fail-y after fixing this.

Comment 23

[Donal Meehan [:dmeehan]]

Kelsey, this patch doesn't graft cleanly to esr102. Could you please attach a patch based on esr102?

Comment 24

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

Set release status flags based on info from the regressing bug 1476327

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Hi Kelsey, it appears that the rebased ESR102 patch doesn't build.
https://treeherder.mozilla.org/jobs?repo=try&revision=95d63a0eba91abd5db4c68b7e5af4251652d4f39

Comment 26

[Kelsey Gilbert [:jgilbert]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #25)

Hi Kelsey, it appears that the rebased ESR102 patch doesn't build.
https://treeherder.mozilla.org/jobs?repo=try&revision=95d63a0eba91abd5db4c68b7e5af4251652d4f39

Yes I'm working on it. I just want to show progress.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

(In reply to Kelsey Gilbert [:jgilbert] from comment #26)

Yes I'm working on it. I just want to show progress.

Ah, thanks!

Comment 28

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1779800 - [esr102] Fix WebGL instancing in some cases on Mac.

This reverts the code that would collapse DrawElements to DrawArrays in some cases,
because it was not a correct transform in some cases.

Instead, we need to always keep around a cpu copy of the index buffer,
so that we can figure out how many fakeAttrib0 verts we need.

Finally, in order to correct for the bug where Mac offsets the base instance by first in DrawArrays,
we need to (temporarily) patch vertexAttribPointer offsets for any call with non-zero first.
(This can be disabled via MOZ_WEBGL_WORKAROUND_FIRST_AFFECTS_INSTANCE_ID=0)

Comment 29

[Kelsey Gilbert [:jgilbert]]

Ok this builds locally.
Try: https://treeherder.mozilla.org/jobs?repo=try&revision=60f8c144ef883b2079a8a41918bf43f0be157afe

Comment 30

[Kelsey Gilbert [:jgilbert]]

Looks likely green!

Comment 31

[Ryan VanderMeulen [:RyanVM]]

I'm seeing test_2_conformance2__rendering__builtin-vert-attribs.html timeouts across the board?

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Nevermind, I see there was an updated push.
https://treeherder.mozilla.org/jobs?repo=try&revision=88556bcd10da78fcc979a9253e703a6249776303

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9306124 [details]
Bug 1779800 - [esr102] Fix WebGL instancing in some cases on Mac.

Approved for 102.6esr, thanks for the rebased patch!

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/bfcd0ce4da09

Comment 35

[Anca Soncutean, Desktop QA]

Reproduced with Fx 102 on macOS 12.
Verified fixed with Fx 109.0a1 (2022-12-06), Fx 108.0 and Fx 102.6.0esr on macOS 12.