[naga] Support `textureSampleBaseClampToEdge()` for texture2d #7709

jamienicol · 2025-05-21T13:15:05Z

Adds a new flag to the IR indicating when image sample coordinates are to be clamped. Adds wgsl-in support for parsing and lowering to IR. Validation ensures this flag is only used when sampling a 2D non-arrayed sampled texture, without offset, gather, or depth comparison. This matches the WGSL requirements, with the exception of supporting texture_external textures, which will follow in a later patch.

SPIRV, HLSL, and Metal backends are supported so far, with GLSL left for a follow up. (In GLSL the texture will simply be sampled without the coordinates being clamped.)

It may seem unfortunate to have to handle this separately for each backend, and indeed it would have been possible to implement this simply in the WGSL frontend. However, future patches will add support for using textureSampleBaseClampToEdge() with external textures, which will actually have to be handled by each backend. This patch is laying the groundwork for that.

Connections
Part of #4386
Fixes #7539

Testing
Added snapshot test

Makes these CTS tests pass

Squash or Rebase?

Rebase

Checklist

Run cargo fmt.
Run taplo format.
Run cargo clippy --tests. If applicable, add:
- --target wasm32-unknown-unknown
Run cargo xtask test to run tests.
If this contains user-facing changes, add a CHANGELOG.md entry.

Adds a new flag to the IR indicating when image sample coordinates are to be clamped. Adds wgsl-in support for parsing and lowering to IR. Validation ensures this flag is only used when sampling a 2D non-arrayed sampled texture, without offset, gather, or depth comparison. This matches the WGSL requirements, with the exception of supporting `texture_external` textures, which will follow in a later patch. SPIRV, HLSL, and Metal backends are supported so far, with GLSL left for a follow up. (In GLSL the texture will simply be sampled without the coordinates being clamped.) It may seem unfortunate to have to handle this separately for each backend, and indeed it would have been possible to implement this simply in the WGSL frontend. However, future patches will add support for using textureSampleBaseClampToEdge() with external textures, which will actually have to be handled by each backend. This patch is laying the groundwork for that.

This doesn't do anything yet, but unblocks running some CTS tests.

… execution tests to CTS test list

jimblandy · 2025-06-11T20:11:17Z

@jamienicol This is a bit dumb, but does b71a0e2 look okay to you? The MSL backend has no obligations here, since it's a validation error, but I just felt it would be nicer for the MSL backend to die instead of quietly misbehaving.

jamienicol · 2025-06-11T20:55:01Z

@jimblandy I don't mind it at all. I wouldn't want to go hugely out of the way to repeat validation in the backends, but that is simple enough.

Personally I'd prefer an assertion to returning an error: if we hit that case there's a bug somewhere and I'd rather we know about it via crash reports than users just suffering in silence. But I might be in the minority of preferring crashes to errors.

jamienicol · 2025-06-11T20:55:31Z

Any reason to limit that to MSL rather than doing it for all backends? or was that just as an example?

ErichDonGubler

Looks like @jimblandy is ramping up on review now, so I won't drive a full review myself, but I'll comment on some things I noticed before I left on vacation and now.

ErichDonGubler · 2025-05-21T13:40:27Z

naga/src/back/hlsl/writer.rs

+            Expression::ImageSample {
+                coordinate:
+                    crate::SampleCoordinate {
+                        expr: coordinate,
+                        clamp_to_edge: true,
+                    },
+                image,
+                sampler,
+                ..
+            } => {


thought: It's very unfortunate that we simply don't use the other fields in the case where clamp_to_edge is true. This feels like an appropriate place to use an enum, but I can only imagine this will rewrite quite a bit of code for a questionable benefit.

I agree, this is the same thing I was bringing up here: #7709 (comment)

@ErichDonGubler Jim and I had previously discussed how to enforce valid combinations of ImageSample flags using the type system. (eg using enums). It's a problem that goes beyond just clamp_to_edge. We didn't think it was worth it in the end. I think Jim's suggestion above is a happy middle ground.

ErichDonGubler · 2025-05-21T13:42:50Z

naga/src/back/glsl/mod.rs

@@ -3042,9 +3042,11 @@ impl<'a, W: Write> Writer<'a, W> {
                // The space here isn't required but it helps with readability
                write!(self.out, ", ")?;

+                // TODO: handle coordinate.clamp_to_edge


issue: This shouldn't remain as a TODO item; this should graduate to an issue before getting merged, if it's intended to outlast this PR.

Filed #7791. Linked to it in a comment.

ErichDonGubler · 2025-06-12T04:44:48Z

deno_webgpu/texture.rs

+impl Drop for GPUExternalTexture {
+    fn drop(&mut self) {}
+}
+


nitpick(non-blocking): This Drop implementation shouldn't be necessary at all, AFAIK, with the current behavior you have. I might be wrong in the case of Deno's expected trait implementations, but I don't think so?

If you're going to fill it out later, though, then 🤷🏻‍♂️.

I swear I thought I saw other empty drop impls and just copied that. But looking now I can't find them.

I think long term we would expect GPUExternalTexture to impl Drop. And to call into wgpu's (yet to be added) Global::external_texture_drop(). But actually implementing external texture in deno is something I wasn't planning to do so in the short term.

I will remove the impl for now

Removed in cd4d2d9

ErichDonGubler · 2025-06-12T04:55:43Z

naga/src/back/hlsl/help.rs

+            WrappedImageSample {
+                clamp_to_edge: true,
+            } => {


nitpick: Is there a reason not to destructure and then do if clamp_to_edge?

A subsequent patch will take ImageClass in to account to. So it'll look like:

match sample { WrappedImageSample { clamp_to_edge: true, class: ImageClass::External, } => { ... } WrappedImageSample { clamp_to_edge: true, class: ImageClass::Sampled { kind: ScalarKind::Float, multi: false}, } => { ... } _ => {} }

would you instead propose something like this?

let WrappedImageSample { clamp_to_edge, class } = sample; match (clamp_to_edge, class) { (true, ImageClass::External) => { ... } (true, ImageClass::Sampled { kind: ScalarKind::Float, multi: false}) => { ... } _ => {} }

If so, no complaints from me if you think that's more conventional

Ah, if you're going to add another match arm, I don't think this is a worthy nitpick. I was going to suggest that simply use clamp_to_edge, but if you're adding class to the mix, I think it's more a question of taste than what's more idiomatic.

I think it's easier to read what you've speculated as my proposal, but if the match gets particularly big, it might be nice to have the redundant member names. Up to you!

jimblandy · 2025-06-12T06:40:58Z

Any reason to limit that to MSL rather than doing it for all backends? or was that just as an example?

It'd make sense for all backends.

jimblandy

Thoughts so far:

jimblandy · 2025-06-11T20:27:48Z

naga/src/back/msl/writer.rs

+                    writeln!(self.out, "{l1}{NAMESPACE}::float2 half_texel = 0.5 / {NAMESPACE}::float2(tex.get_width(0u), tex.get_height(0u));")?;
+                    writeln!(
+                        self.out,
+                        "{l1}return tex.sample(samp, {NAMESPACE}::clamp(coords, half_texel, 1.0 - half_texel), 0.0);"


It seems to me this will not always sample the base mip level, as textureSampleBaseClampToEdge requires. Should we also pass {NAMESPACE}::level(0) before the 0.0 offset?

We don't need an offset so the 0.0 is supposed to be the level. But indeed I forgot to wrap it in {NAMESPACE}::level()

Fixed in 2fb382c

jimblandy · 2025-06-12T06:40:43Z

naga/src/back/hlsl/writer.rs

+            Expression::ImageSample {
+                coordinate:
+                    crate::SampleCoordinate {
+                        expr: coordinate,
+                        clamp_to_edge: true,
+                    },
+                image,
+                sampler,
+                ..
+            } => {


I agree, this is the same thing I was bringing up here: #7709 (comment)

jamienicol · 2025-06-12T08:35:56Z

@jamienicol This is a bit dumb, but does b71a0e2 look okay to you? The MSL backend has no obligations here, since it's a validation error, but I just felt it would be nicer for the MSL backend to die instead of quietly misbehaving.

Did the same thing for HLSL in 2fb382c. Slightly different approach for SPIRV as the implementation is different. In theory it should be able to handle other sample flags, except for the level which must be zero. So I've added a check for that alone - I don't think the backends should be in the business of repeating validation in its entirety - rather they should make a reasonable effort to ensure their own constraints are satisfied.

jimblandy · 2025-06-12T15:40:25Z

I don't think the backends should be in the business of repeating validation in its entirety - rather they should make a reasonable effort to ensure their own constraints are satisfied.

Yes, 100%. Backends are allowed to panic and misbehave when validation is buggy. My wanting to tighten up these matches was just opportunistic: it seemed like a small change might save someone some debugging time in he future.

crowlKats

Deno part LGTM

cwfitzgerald assigned jimblandy May 21, 2025

jamienicol force-pushed the texture-sample-base-clamp-to-edge branch from b25453c to 466e9b8 Compare May 27, 2025 08:34

jamienicol requested review from crowlKats and a team as code owners May 27, 2025 08:34

jamienicol added the PR: run CTS label May 27, 2025

jamienicol force-pushed the texture-sample-base-clamp-to-edge branch 2 times, most recently from 133ad63 to b27265b Compare May 27, 2025 14:51

jamienicol force-pushed the texture-sample-base-clamp-to-edge branch 2 times, most recently from 6df8c7c to 9103208 Compare June 4, 2025 15:06

andyleiserson removed the PR: run CTS label Jun 6, 2025

jamienicol force-pushed the texture-sample-base-clamp-to-edge branch from 9103208 to 05da8d1 Compare June 10, 2025 07:29

cwfitzgerald changed the title ~~[naga] Support textureSampleBaseClampToEdge() for texture2d~~ [naga] Support textureSampleBaseClampToEdge() for texture2d Jun 11, 2025

jamienicol and others added 4 commits June 11, 2025 12:55

[deno_webgpu] Add stub GPUExternalTexture implementation

845bd3c

This doesn't do anything yet, but unblocks running some CTS tests.

[cts_runner] Add texture2d<f32> textureSampleBaseClampToEdge() shader…

7c668fa

… execution tests to CTS test list

fixup! [naga] Support textureSampleBaseClampToEdge() for texture2d

b71a0e2

jimblandy force-pushed the texture-sample-base-clamp-to-edge branch from 05da8d1 to b71a0e2 Compare June 11, 2025 20:09

ErichDonGubler reviewed Jun 12, 2025

View reviewed changes

This comment was marked as duplicate.

Sign in to view

jimblandy reviewed Jun 12, 2025

View reviewed changes

fixup! [deno_webgpu] Add stub GPUExternalTexture implementation

cd4d2d9

jamienicol mentioned this pull request Jun 12, 2025

[naga glsl-out] Implement coordinate clamping for textureSampleBaseClampToEdge for GLSL #7791

Open

fixup! [naga] Support textureSampleBaseClampToEdge() for texture2d

2fb382c

crowlKats approved these changes Jun 13, 2025

View reviewed changes

jamienicol mentioned this pull request Jun 13, 2025

[Naga msl-out] Add padding to end of structs. And run some textureSample CTS tests that now pass #7804

Draft

6 tasks

[naga] Support textureSampleBaseClampToEdge() for texture2d #7709

Are you sure you want to change the base?

[naga] Support textureSampleBaseClampToEdge() for texture2d #7709

Uh oh!

Conversation

jamienicol commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimblandy commented Jun 11, 2025

Uh oh!

jamienicol commented Jun 11, 2025

Uh oh!

jamienicol commented Jun 11, 2025

Uh oh!

ErichDonGubler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as duplicate.

Uh oh!

jimblandy commented Jun 12, 2025

Uh oh!

jimblandy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamienicol commented Jun 12, 2025

Uh oh!

jimblandy commented Jun 12, 2025

Uh oh!

crowlKats left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[naga] Support `textureSampleBaseClampToEdge()` for texture2d #7709

[naga] Support `textureSampleBaseClampToEdge()` for texture2d #7709

jamienicol commented May 21, 2025 •

edited

Loading