[glsl-in] Consider supporting combined image/samplers

[magcius]

My use case for Naga is converting from GLSL ES to WGSL at runtime. GLSL ES only supports combined image/samplers, and it would be quite annoying for me to change my code to do something else. Many typical approaches to this problem are limited by the restrictions of GLSL.

For instance, my current policy is to have texture bindings and samplers take consecutive bindings.

I would be fine if the GLSL is parsed into an IR that can't be output by the WGSL backend, and I would have to convert it to the proper policy myself.

[JCapucho]

Can you post a small piece of code of what you mean?

[magcius]

We talked about this a bit in Matrix here: https://matrix.to/#/!fjjkgHFcwtkREywzfk:matrix.org/$K2DR_lrip11nvTM7r3rnk-dPRe5C56SjJ1RSTusnTv0?via=matrix.org&via=mozilla.org&via=sorunome.de

The idea is that I could write code like:

uniform sampler2D u_Texture;

And then be able to turn that into, e.g.:

[[group(0), binding(0)]] var T_u_Texture : texture_2d<f32>;
[[group(0), binding(1)]] var S_u_Texture : sampler;

[JCapucho]

Hmm how would that interact with vulkans combined image samplers? https://vulkan-tutorial.com/Texture_mapping/Combined_image_sampler

[magcius]

Similarly, or the same, actually. The issue is more that WebGPU/WGSL, and also D3D12/HLSL both don't support combined image samplers, so you need to figure out what to do with it on output.

[kvark]

Adding full support for combined stuff into IR is a biiig issue. I'm not excited about this, and I hope we'll not need it any time soon. In particular because WebGPU doesn't have combined image-samplers at all. So perhaps this can be helped on glsl-in side?

[magcius]

I don't need full support, I'm happy to write the translation code. I think it's going to be a big sticking point for those migrating from WebGL, and those interested in having engines that support both WebGL and WebGPU.

[glfmn]

While the kinks of getting combined image/samplers working, the error message that naga provides when using an unsupported combined sampler could be improved. Currently, it simply says:

error: Not implemented: variable qualifier
  ┌─ glsl:7:29
  │
7 │ layout(binding = 1) uniform sampler2D image;
  │                             ^^^^^^^^^

However, if the user looks up any documentation or tutorials online for GLSL, they will see countless examples using sampler2D as above, even for vulkan applications. Some sort of additional note or different text to indicate somehow that the proper form is:

layout(set = 1, binding = 0) uniform texture2D u_texture;
layout(set = 1, binding = 1) uniform sampler u_sampler;

and to use the function sampler2D(u_texture, u_sampler) to make the sampler would be much more helpful. At the very least, something like Not implemented: combined image samplers are unsupported.

[chyyran]

I encountered this issue and filed gfx-rs/naga#2100 in error because of the confusing error message, but let me give my 2 cents while I'm here.

My use case is implementing RetroArch's shader pipeline as a dynamic library in Rust, which involves compiling shader files that I don't have control over here: https://github.com/libretro/slang-shaders.

Many of these use the uniform sampler2D u_Texture; syntax. I had originally thought that I could work around this by round-tripping from the shaderc crate to obtain SPIR-V bytecode, then parsing with spv-in, but this gives me an InvalidId error when parsing the SPIR-V. Even if there was a transformation in glsl-in, it doesn't seem like it would match up with whatever SPIR-V glslang output when dealing with combined image samplers.

For example, this shader that does consecutive bindings for combined image samplers

layout(set = 0, binding = 2) uniform sampler2D Source;
layout(set = 0, binding = 3) uniform sampler2D OriginalHistory1;
layout(set = 0, binding = 4) uniform sampler2D OriginalHistory2;
layout(set = 0, binding = 5) uniform sampler2D OriginalHistory3;
layout(set = 0, binding = 6) uniform sampler2D OriginalHistory4;
layout(set = 0, binding = 7) uniform sampler2D OriginalHistory5;
layout(set = 0, binding = 8) uniform sampler2D OriginalHistory6;
layout(set = 0, binding = 9) uniform sampler2D OriginalHistory7;

A source transformation here would need to take into account mapping bindings between the images and the samplers

[chyyran]

I don't think this is a good way of going about it, but as a proof of concept I was able to get naga to accept SPIR-V (generated by SPIRV-Cross) that contains a combined image sampler without IR changes.

The output of this however doesn't look very correct, see particularly the wgsl example here https://gist.github.com/chyyran/14e3e17fe5d8ed56a685eee71db32947

    let _e10: vec4<f32> = textureSample(Source, Source, _e9); // (???)

[Nazariglez]

I am having the same issue trying to integrate naga with https://github.com/Nazariglez/notan/pull/200, We're using glsl-to-spirvand shaderc under feature flags, so keeping the compatibility between the shaders and the code was my top priority while integrating naga, but then I run into this error, and I am not sure how to proceed from here.

Is there something that we can/help with to allow the use of layout(binding = 0) uniform sampler2D u_texture; as input and output? (in our case we need it to convert glsl gfx-rs/naga#450 to 330 and 300 ES for WebGL and OpenGL 3.3).

Thanks

[kvark]

Thanks to all the participants for input! I think it's clear that, if naga wants to replace glsl-to-spirv, it needs to support combined image-samplers. So we'll need a capability flag for these, and to represent them in IR in some way. Strawman proposal is:

• add COMBINED_IMAGE_SAMPLER to valid::Capabilities flags. This will never be enabled in WebGPU stack.
• add includes_sampler flag to the ImageClass::Sampler
• allow Expression::ImageSample to have both image and sampler global var handles being the same, if pointing to a combined image-sampler

Next step is - contributions wanted!

[Nazariglez]

I am happy to help with it. Do we have any other PR with a "similar" feature or changes that I can use as a reference? This is the first time I will contribute to naga (and on any other transpiler) and I am not sure how to start. Thanks!

[kvark]

I suppose gfx-rs/naga#153 could be considered an example. The case for combined image/samplers should be simpler though, since it's not affecting WGSL or MSL.

[chyyran]

Implemented some preliminary work on the IR side guided by kvark's suggestions in https://github.com/chyyran/naga/tree/feat-combined-image-samplers

Remaining work

• Allow Expression::ImageSample to validate when image and sampler handles are the same
• [glsl-in] Implement parsing samplerXD to fix parsing variable qualifiers error
• [spv-in] Deal with including the sampler in an existing image when parsing OpTypeSampledImage (do we just need to locate the existing Handle<Expression> and set the includes_sampler bool?

Not sure if glsl-out and spv-out will 'just work' with these changes, but hlsl-out and wgsl-out will definitely require a lowering pass. SPIRV-Cross does this by generating a SamplerState _Texture_sample with the sampler register having the same index as the texture register for HLSL, translating texture(Texture, TexCoord) calls to Texture.Sample(_Texture_sampler, TexCoord). Perhaps we could do something similar for WGSL (possibly MSL?) as well.

Also, should this issue be combined with https://github.com/gfx-rs/wgpu/issues/4323?

[Nazariglez]

Hey @chyyran, is there something that I can help you with? I forked naga yesterday to start looking into this and just saw that you already begin. Thanks

Let me know if you need anything.

[chyyran]

@Nazariglez You can go ahead and fork my branch at https://github.com/chyyran/naga/tree/feat-combined-image-samplers

The remaining work items are as I spelled out. Big one is parsing variable quantifiers samplerXD so naga can accept GLSL syntax like uniform sampler2D MyTexture. I probably won't have time to look at this too deeply for now so please don't wait for me haha.

[chyyran]

@Nazariglez Seems like samplerND types need to be added in https://github.com/gfx-rs/naga/blob/master/src/front/glsl/types.rs#L112

[chyyran]

@Nazariglez I ended up adding enough of combined image sampler support for Naga to parse .glsl shaders with such types properly in https://github.com/chyyran/naga/tree/feat-combined-image-samplers

Tested with the following input

Input

#version 450

layout(set = 0, binding = 0, std140) uniform UBO
{
   mat4 MVP;
};

layout(push_constant) uniform Push {
   float ColorMod;
} params;

layout(location = 0) in vec2 vTexCoord;
layout(location = 0) out vec4 FragColor;
layout(binding = 1) uniform sampler2D Source;
void main()
{
   FragColor = texture(Source, vTexCoord) * params.ColorMod;
}

Output

#version 330 core
#extension GL_EXT_texture_shadow_lod : require
struct UBO {
    mat4x4 MVP;
};
struct Push {
    float ColorMod;
};
struct type_46 {
    vec4 FragColor;
};
uniform Push params;

vec2 vTexCoord_1 = vec2(0.0);

vec4 FragColor = vec4(0.0);

uniform highp sampler2D _group_0_binding_1_fs;

smooth in vec2 _vs2fs_location0;
layout(location = 0) out vec4 _fs2p_location0;

void main_1() {
    vec2 unnamed = vTexCoord_1;
    vec2 _e9 = vTexCoord_1;
    vec4 _e10 = texture(_group_0_binding_1_fs, vec2(_e9));
    Push _e11 = params;
    FragColor = (_e10 * _e11.ColorMod);
    return;
}

void main() {
    vec2 vTexCoord = _vs2fs_location0;
    vTexCoord_1 = vTexCoord;
    main_1();
    vec4 _e14 = FragColor;
    type_46 _tmp_return = type_46(_e14);
    _fs2p_location0 = _tmp_return.FragColor;
    return;
}

It seems to validate to my surprise, could you check if this ends up working for you?

[chyyran]

Whatever work I did ended up dumping a bunch of unused types into the module, and caused existing texture(sampler2D(texture, sampler), coord) syntax to fail with Too many arguments in constructor, not sure how to resolve that.