[naga hlsl-out] HLSL implemention of external textures #7826

jamienicol · 2025-06-18T16:30:46Z

Connections
Depends on #7822
Part of #4386

Description
HLSL implementation of external textures

Testing
Snapshot tests. Manual testing.

Squash or Rebase?
Rebase after manually squashing any fixup commits that arise from review

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.

Still not changelog worthy but getting close!

jamienicol · 2025-06-27T14:11:29Z

Rebased and updated textureLoad implementation to use load_transform parameter, as described here

jimblandy · 2025-07-22T22:27:46Z

Rebased on trunk since #7822 is landed.

jimblandy · 2025-07-22T22:59:26Z

Rebased on trunk since #7823 has landed, and regenerated snapshot output.

jimblandy · 2025-07-23T01:55:47Z

Not a request for a change, just some thoughts:

Does NagaExternalTextureParams really need to be in the IR? It's pretty strange to have a type in there that the code never uses, and that is used only by some backends and must be ignored by others. This seems like something that's going to break easily or confuse people, because it certainly isn't a typical use of IR types. For example, it might seem reasonable to someone that compaction should not treat special types as axiomatically rooted - but changing compaction to remove apparently unused special types would break whoever's using external_texture_params.

In the long term, I'm wondering if Naga doesn't need two IRs, one designed around front-end needs, and a second around backend needs, with a lowering pass between them. The lowering code could be shared by backends, but it would take parameters indicating which transformations the backends needed. One such transformation might be, "lower external textures to ordinary textures and parameter blocks". Others might be the HLSL access rewriting, or the unrestricted pointer parameters rewrites.

jimblandy · 2025-07-23T01:59:45Z

@jamienicol Do the doc changes in ~~7741fba~~ 611c3c9 look correct?

naga/src/back/wgsl/writer.rs

jimblandy

The first commit, "[naga] Generate special type for external texture params buffer", looks good to me.

jimblandy · 2025-07-23T05:16:28Z

@jamienicol In 17f8bf8 I was trying to reduce the repetition in Namer::reset, but I'm not sure I succeeded. If it doesn't seem like a clear improvement to you, we can drop that fixup!.

jimblandy

The second commit, "[naga] Reserve names for each plane and params buffer of external texture", looks good to me.

jimblandy

The log message for the third commit - the long one - is superb, but it belongs in the code, not in the commit message.

jimblandy · 2025-07-23T05:28:04Z

Sorry - still reviewing that final commit.

jamienicol · 2025-07-23T07:56:46Z

@jamienicol Do the doc changes in ~~7741fba~~ 611c3c9 look correct?

They look great, modulo a typo which I've fixed in 5f90c8e

jamienicol · 2025-07-23T08:18:09Z

The log message for the third commit - the long one - is superb, but it belongs in the code, not in the commit message.

I believe the important contents of it is all in the code, spread around various places as appropriate. But a single high-level overview of how external textures are implemented for HLSL in the code somewhere doesn't sound like a bad thing. Any suggestions for where to put this?

jamienicol · 2025-07-23T08:26:37Z

Does NagaExternalTextureParams really need to be in the IR? It's pretty strange to have a type in there that the code never uses, and that is used only by some backends and must be ignored by others. This seems like something that's going to break easily or confuse people, because it certainly isn't a typical use of IR types. For example, it might seem reasonable to someone that compaction should not treat special types as axiomatically rooted - but changing compaction to remove apparently unused special types would break whoever's using external_texture_params.

No, it doesn't need to be in the IR. The benefit of it being there is that we don't have to duplicate its declaration in each backend, and it takes care of fiddly bits such as adding padding, packing a mat3x2 as 3x vec2 in HLSL, etc, for us. When I stumbled across the existing special_types for ray queries etc it seemed conceptually similar-ish to me.

(You're right to point out a confusing difference in how the code as-is handles my params struct vs existing special types, which I've replied to above.)

I'm afraid I don't know what "axiomatically rooted" means. But FWIW our tests would explode if somebody made that change to compaction and it would be quickly caught in CI.

None of this is required, and if you think the weirdness outweighs the benefits I am more than happy to emit the declarations in the backends instead.

jamienicol · 2025-07-23T08:40:28Z

Also pushed 84d4f0e which makes your as_deref() change for NameKey::ExternalTextureFunctionArgument to NameKey::ExternalTextureGlobalVariable as well.

jamienicol · 2025-07-23T16:05:00Z

Pushed 678de10 to add a comment saying why we don't want to compact away the params special type

And d5a4687 adding a module-level comment

jimblandy · 2025-07-30T08:08:39Z

Hmm. This doesn't properly ensure that NagaExternalDimensions2D is a unique name. But this is a pre-existing issue for all query functions. Filed as #8025.

jimblandy

It seems to me that sample_transform must be a transform that takes the (0,0) - (1,1) square to an axis-aligned rectangle that lies within (0,0) - (1,1), because otherwise the use of min and max to compute bounds in nagaTextureSampleBaseClampToEdge doesn't work.

If that's correct, could we get that commented? Perhaps the doc comment for SpecialTypes::external_texture_params would be a good place.

jimblandy · 2025-07-30T08:56:34Z

Is it really the case that nagaTextureSampleBaseClampToEdge doesn't need to take params.size into account, but nagaTextureLoadExternal does? If so, that deserves a comment.

jimblandy · 2025-07-30T09:35:07Z

@jamienicol I think Naga does support TypeInner::BindingArrays of images, but this PR's approach doesn't support a BindingArray of External images. The validator should forbid that.

I should have caught this in the review for #7822, sorry about that.

jimblandy

All right! Just some minor comments.

naga/src/back/hlsl/help.rs

naga/src/back/wgsl/writer.rs

jamienicol · 2025-07-30T13:16:15Z

It seems to me that sample_transform must be a transform that takes the (0,0) - (1,1) square to an axis-aligned rectangle that lies within (0,0) - (1,1), because otherwise the use of min and max to compute bounds in nagaTextureSampleBaseClampToEdge doesn't work.

If that's correct, could we get that commented? Perhaps the doc comment for SpecialTypes::external_texture_params would be a good place.

Yes this is correct. I think wgpu_core::device::resource::ExternalTextureParams would be the better place to document this, and I'll link to there from SpecialTypes::external_texture_params. Unfortunately it can't be a real link because they're in different crates, but I still think that's the most sensible place, as if a field is added/removed/changed in that struct the documentation is right there.

Is it really the case that nagaTextureSampleBaseClampToEdge doesn't need to take params.size into account, but nagaTextureLoadExternal does? If so, that deserves a comment.

This is really the case. textureSampleBaseClampToEdge() works in normalized coordinates so the texture size doesn't matter, we just need to clamp to 0..1. (with transform and half-texel adjustment). textureLoad works with unnormalized coordinates so we need to clamp to the size the external texture claims to be. (not the actual texture size, as there may be a crop rect)

Perhaps we could do the equivalent of what we do in the sample case, eg transform (0,0)..(texture_size - 1) and then clamp within that rect. But that's more instructions, and I seem to recall there was an issue with that approach anyway although unfortunately I can no longer remember the specifics

@jamienicol I think Naga does support TypeInner::BindingArrays of images, but this PR's approach doesn't support a BindingArray of External images. The validator should forbid that.

I should have caught this in the review for #7822, sorry about that.

I'll see if I can fix this

jamienicol · 2025-07-30T13:28:52Z

I filed #8027 for actually implementing binding array support

jimblandy · 2025-07-30T19:18:02Z

@jamienicol Okay, I pushed one more doc tweak, but this looks good to go. The binding array check looked right to me.

Thanks!!

During wgsl lowering, if we encounter an external texture type then generate the `ExternalTextureParams` struct. This will be required by most Naga backends to implement external textures. This type is not actually used by wgsl-in or the IR. However, generating it in Naga IR ensures tricky details such as member alignment are handled for us. wgsl-out must ensure it does *not* generate code for this type, as it handles external textures natively.

…ture Adds new `NameKey` variants `ExternalTextureGlobalVariable` and `ExternalTextureFunctionArgument`, like their non-external-texture cousins but additionally keyed by either being a specific plane index or params buffer. For each external texture global variable or function argument reserve additional names for 3 planes and the params buffer. For Naga backends which must represent external textures as multiple variables/arguments, this will allow them to uniquely name each one.

This adds HLSL backend support for `ImageClass::External` (ie WGSL's `external_texture` texture type). For each external texture global variable in the IR, we declare 3 `Texture2D` globals as well as a `cbuffer` for the params. The additional bindings required by these are found in the newly added `external_texture_binding_map`. Unique names for each can be obtained using `NameKey::ExternalTextureGlobalVariable`. For functions that contain ImageQuery::Size, ImageLoad, or ImageSample expressions for external textures, ensure we have generated wrapper functions for those expressions. When emitting code for the expressions themselves, simply insert a call to the wrapper function. For size queries, we return the value provided in the params struct. If that value is [0, 0] then we query the size of the plane 0 texture and return that. For load and sample, we sample the textures based on the number of planes specified in the params struct. If there is more than one plane we additionally perform YUV to RGB conversion using the provided matrix. Unfortunately HLSL does not allow structs to contain textures, meaning we are unable to wrap the 3 textures and params struct variables in a single variable that can be passed around. For our wrapper functions we therefore ensure they take the three textures and the params as consecutive arguments. Likewise, when declaring user-defined functions with external texture arguments, we expand the single external texture argument into 4 consecutive arguments. (Using NameKey::ExternalTextureFunctionArgument to ensure unique names for each.) Thankfully external textures can only be used as either global variables or function arguments. This means we only have to handle the `Expression::GlobalVariable` and `Expression::FunctionArgument` cases of `write_expr()`. Since in both cases we know the external texture can only be an argument to either a user-defined function or one of our wrapper functions, we can simply emit the names of the variables for each three textures and the params struct in a comma-separated list.

…ternal textures For simplicity's sake our initial implementation of external textures will not support binding arrays of external textures. We should therefore reject any shaders which use them during validation. Their implementation will be tracked in gfx-rs#8027. naga/src/valid/type.rs JJ: JJ: Lines starting with "JJ:" (like this one) will be removed.

This adds MSL backend support for `ImageClass::External`. (ie WGSL's `external_texture` texture type). This is implemented very similarily to the HLSL implementation in gfx-rs#7826. Each external texture global variable is lowered to 3 `texture2d`s and a buffer of type NagaExternalTextureParams. As usual in Naga's MSL backend, these are passed as arguments to the entry point. The bindings for each of these arguments are provided via the usual binding map, using a new `BindExternalTextureTarget` variant of `BindTarget`. Unlike HLSL, MSL allows textures to be used as fields in structs. We therefore immediately wrap these variables in a `NagaExternalTextureWrapper` struct. This wrapper can then conveniently be passed to either user-defined functions or builtin implementations that accept an external texture. The WGSL builtins `textureDimensions()`, `textureLoad()`, and `textureSampleBaseClampToEdge()` are implemented using wrapper functions using the regular `write_wrapped_functions()` machinery.

This adds MSL backend support for `ImageClass::External`. (ie WGSL's `external_texture` texture type). This is implemented very similarily to the HLSL implementation in #7826. Each external texture global variable is lowered to 3 `texture2d`s and a buffer of type NagaExternalTextureParams. As usual in Naga's MSL backend, these are passed as arguments to the entry point. The bindings for each of these arguments are provided via the usual binding map, using a new `BindExternalTextureTarget` variant of `BindTarget`. Unlike HLSL, MSL allows textures to be used as fields in structs. We therefore immediately wrap these variables in a `NagaExternalTextureWrapper` struct. This wrapper can then conveniently be passed to either user-defined functions or builtin implementations that accept an external texture. The WGSL builtins `textureDimensions()`, `textureLoad()`, and `textureSampleBaseClampToEdge()` are implemented using wrapper functions using the regular `write_wrapped_functions()` machinery.

jamienicol requested a review from a team as a code owner June 18, 2025 16:30

jamienicol force-pushed the hlsl-texture-external branch 4 times, most recently from 636c40c to 78d8574 Compare June 23, 2025 12:23

cwfitzgerald assigned jimblandy Jun 25, 2025

jamienicol mentioned this pull request Jun 27, 2025

wgpu-core implementation for external textures #7823

Merged

6 tasks

jamienicol force-pushed the hlsl-texture-external branch from 78d8574 to 681f342 Compare June 27, 2025 13:16

jamienicol force-pushed the hlsl-texture-external branch from 681f342 to bb06952 Compare July 18, 2025 09:10

jimblandy force-pushed the hlsl-texture-external branch from bb06952 to f9cbe75 Compare July 22, 2025 22:26

jimblandy force-pushed the hlsl-texture-external branch from f9cbe75 to 2084c01 Compare July 22, 2025 22:59

jimblandy reviewed Jul 23, 2025

View reviewed changes

naga/src/back/wgsl/writer.rs Show resolved Hide resolved

jimblandy force-pushed the hlsl-texture-external branch from 7741fba to 611c3c9 Compare July 23, 2025 02:12

jimblandy reviewed Jul 23, 2025

View reviewed changes

jimblandy requested changes Jul 23, 2025

View reviewed changes

jamienicol force-pushed the hlsl-texture-external branch from 16c971e to d5a4687 Compare July 23, 2025 16:10

jamienicol requested a review from jimblandy July 29, 2025 15:45

jimblandy force-pushed the hlsl-texture-external branch from 0eee109 to 2c5b18a Compare July 30, 2025 08:12

jimblandy reviewed Jul 30, 2025

View reviewed changes

jimblandy mentioned this pull request Jul 30, 2025

Naga's WGSL backend should avoid emitting definitions for special types #8026

Open

jimblandy approved these changes Jul 30, 2025

View reviewed changes

naga/src/back/hlsl/help.rs Outdated Show resolved Hide resolved

naga/src/back/hlsl/help.rs Outdated Show resolved Hide resolved

naga/src/back/wgsl/writer.rs Show resolved Hide resolved

jamienicol force-pushed the hlsl-texture-external branch from 65e338e to 2527383 Compare July 30, 2025 13:37

jamienicol mentioned this pull request Jul 30, 2025

[naga hlsl-out] Handle external texture color space conversion #8028

Merged

5 tasks

jimblandy force-pushed the hlsl-texture-external branch from 89b337c to c3aa17e Compare July 30, 2025 20:10

jamienicol added 4 commits July 30, 2025 13:27

jimblandy force-pushed the hlsl-texture-external branch from c3aa17e to bb7a4e0 Compare July 30, 2025 20:27

jimblandy merged commit 59f815a into gfx-rs:trunk Jul 30, 2025
40 checks passed

jamienicol deleted the hlsl-texture-external branch July 31, 2025 08:07

jamienicol mentioned this pull request Aug 18, 2025

Metal / MSL external texture implementation #8117

Merged

6 tasks

[naga hlsl-out] HLSL implemention of external textures #7826

[naga hlsl-out] HLSL implemention of external textures #7826

Uh oh!

Conversation

jamienicol commented Jun 18, 2025

Uh oh!

jamienicol commented Jun 27, 2025

Uh oh!

jimblandy commented Jul 22, 2025

Uh oh!

jimblandy commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimblandy commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimblandy commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jimblandy left a comment

Choose a reason for hiding this comment

Uh oh!

jimblandy commented Jul 23, 2025

Uh oh!

jimblandy left a comment

Choose a reason for hiding this comment

Uh oh!

jimblandy left a comment

Choose a reason for hiding this comment

Uh oh!

jimblandy commented Jul 23, 2025

Uh oh!

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

Uh oh!

Uh oh!

jamienicol commented Jul 23, 2025

Uh oh!

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

Uh oh!

Uh oh!

jamienicol commented Jul 23, 2025

Uh oh!

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

Uh oh!

Uh oh!

jimblandy commented Jul 30, 2025

Uh oh!

jimblandy left a comment

Choose a reason for hiding this comment

Uh oh!

jimblandy commented Jul 30, 2025

Uh oh!

jimblandy commented Jul 30, 2025

Uh oh!

jimblandy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jamienicol commented Jul 30, 2025

Uh oh!

jamienicol commented Jul 30, 2025

Uh oh!

jimblandy commented Jul 30, 2025

Uh oh!

Uh oh!

Uh oh!

jimblandy commented Jul 22, 2025 •

edited

Loading

jimblandy commented Jul 23, 2025 •

edited

Loading

jimblandy commented Jul 23, 2025 •

edited

Loading

jamienicol commented Jul 23, 2025 •

edited

Loading

jamienicol commented Jul 23, 2025 •

edited

Loading

jamienicol commented Jul 23, 2025 •

edited

Loading