Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Compat: sample locations don"t match the spec in OpenGL #4804

Closed
greggman opened this issue Aug 2, 2024 · 7 comments
Closed

Compat: sample locations don"t match the spec in OpenGL #4804

greggman opened this issue Aug 2, 2024 · 7 comments
Labels
api WebGPU API compat WebGPU Compatibility Mode wgsl:builtins
Milestone
Milestone 1

Comments

@greggman
Copy link
Contributor

greggman commented Aug 2, 2024 •
edited
Loading

Running this test, which sets each sample of a 4 count multisample texture to a different color and then uses textureLoad(multisampledTexture, vec2u(0), sample_index) to read each of the samples to a storage buffer in Chrome on Linux AMD OpenGL gets different results than the other APIs

normal results

sample_index: 0  value: 1,0,0,0  si: 0
sample_index: 1  value: 0,1,0,0  si: 1
sample_index: 2  value: 0,0,1,0  si: 2
sample_index: 3  value: 0,0,0,1  si: 3

GL results

sample_index: 0  value: 1,0,0,0  si: 0
sample_index: 1  value: 0,0,0,1  si: 3
sample_index: 2  value: 0,0,1,0  si: 2
sample_index: 3  value: 0,1,0,0  si: 1

I ran into this issue writing the textureLoad WGSL tests

We think maybe swizzling the index in textureLoad on the GL backend is enough to fix this? If not, another solution is to disallow textureLoad on texture_multisample_2d<???> textures in compat. other ideas?
@Kangz
Copy link
Contributor

Kangz commented Aug 27, 2024

We could say that locations may be non-standard in compat, and maybe even expose the sample location info on the adapter?

@Kangz Kangz added the compat WebGPU Compatibility Mode label Aug 27, 2024
@Kangz Kangz added this to the Milestone 1 milestone Aug 27, 2024
@kainino0x
Copy link
Contributor

I don"t know for sure but I suspect standard sample locations are not that critical, and compat should just loosen the guarantee.

@greggman
Copy link
Contributor Author

The workground seems trivial and I think they are critical. Either that, or we should remove

textureLoad(t: texture_depth_multisampled_2d,
                         coords: vec2<C>,
                         sample_index: S)-> f32

from compat

@kdashg
Copy link
Contributor

kdashg commented Nov 20, 2024

Ok so no change needed?

@kainino0x
Copy link
Contributor

The workground seems trivial and I think they are critical.

In that case the workaround sounds great to me.

Ok so no change needed?

We can work around the order of samples, but it"s also possible that the exact positions will be slightly different (i.e. devices where Vulkan would report standardSampleLocations=false). Pretty sure this is less important (and anyway we can"t do anything about it). If this does turn out to happen on devices we target for compat, we will need to loosen the spec, and these tests should fail: https://gpuweb.github.io/cts/standalone/?q=webgpu:api,operation,render_pipeline,sample_mask:* (we may need to split those tests so we test the sample locations in a separate test from the other stuff; we also still need to fix that test for gpuweb/cts#3960)

@kainino0x
Copy link
Contributor

Resolved in meeting that we will workaround, and the number of devices with non standard sample locations is probably vanishingly small (e.g. on vulkan) so we will only revisit loosening the spec if we actually find such devices.

@kainino0x kainino0x closed this as not planned Won't fix, can't repro, duplicate, stale Nov 20, 2024
@Kangz
Copy link
Contributor

Kangz commented Jan 27, 2025

GPU Web WG 2024-11-20 Atlantic-time
  • CW: what"s possible in Compat w.r.t. sample locations?
  • GT: can barely remember :) Can"t look at sample index or sample mask, but can call textureLoad on a multisample texture. So can draw over corner of a texture that"s sample 0 and then textureLoad sample 0 and they"ll be in different orders.
  • GT: think we should polyfill because if you ask for index 1, should get index 1. This is textureLoad, not textureSample. Expect to get what I asked for in that case.
  • JB: in this example, sample index 0 isn"t actually disturbed in OpenGL.
  • GT: use sample 3 then.
  • JB: like your suggestion that since we have a low-cost workaround that works for everybody, Mozilla likes the suggestion. Especially since no change to spec. :)
  • CW: not just that order of samples is shuffled - they might not be in the exact same position.
  • KN: nothing we can do about that. Spec has to loosen that guarantee in Compat. Can say order of quadrants is the same, but samples could be in different places.
  • CW: OpenGL spec doesn"t specify the sample locations.
  • SW: surprising if same device in Vk and GL had different locations.
  • CW: it is configurable on a lot of hardware. :)
  • KN: on Vk, small number of impls that don"t report standard sample locations. On GL I don"t think there"s a way to query it. On Vk , couple of really old NVIDIA devices, and Broadcom D3D?
  • GL: you can query this in GL FWIW.
  • CW: if we can"t change the sample locations, or spec says nothing, people probably won"t realize the sample pattern"s slightly different.
  • SW: I agree. Quadrants are known broken, let"s fix that, and sample locations are unknown how broken they are. Revisit when we have more data.
  • KN: there"s a test for the positions. If they fail, we"ll find out.
  • CW: everyone"s fine remapping quadrants, and is OK not saying anything about non-standard sample locations, and seeing later.
  • KN: yes.
  • Resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
api WebGPU API compat WebGPU Compatibility Mode wgsl:builtins
Projects
None yet
Milestone
Milestone 1
Development

No branches or pull requests
4 participants
@greggman @Kangz @kainino0x @kdashg