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.

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

Static Samplers #284

Closed
austinEng opened this issue Mar 14, 2024 · 7 comments
Closed

Static Samplers #284

austinEng opened this issue Mar 14, 2024 · 7 comments
Labels
non-breaking Does not require a breaking change (that would block V1.0)

Comments

@austinEng
Copy link
Collaborator

Chrome is interested in adding static samplers to Dawn. Filing this issue because I think there's a possibility it could eventually be a core webgpu concept. Great to add it to the header if that's the case.

Proposal:

+ // Can be chained in WGPUSamplerBindingLayout
+ typedef struct WGPUSamplerBindingLayoutStatic {
+   WGPUChainedStruct chain;
+   WGPUSampler sampler;
+ } WGPUSamplerBindingLayoutStatic WGPU_STRUCTURE_ATTRIBUTE;

typedef enum WGPUFeatureName {
 ...
+  WGPUFeatureName_StaticSamplers,
 ...
}

OR

typedef struct WGPUSamplerBindingLayout {
  WGPUChainedStruct const * nextInChain;
  WGPUSamplerBindingType type;
+ WGPUSampler staticSampler;
 } WGPUSamplerBindingLayout WGPU_STRUCTURE_ATTRIBUTE;

typedef enum WGPUFeatureName {
 ...
+  WGPUFeatureName_StaticSamplers,
 ...
}
  • This doesn't exist in the Web API right now, but it's useful in native, especially for YCbCr sampling (more stuff needed for full YCbCr). We could have an experimental WGPUFeatureName_StaticSamplers, and if it makes it to the Web and all implementations add it, remove the feature? It can be emulated by the implementation when the backend doesn't actually support it.
  • Vulkan, it would map to pImmutableSamplers on VkDescriptorSetLayoutBinding
  • D3D12, it would map to D3D12_STATIC_SAMPLER_DESC in the root signature
  • Metal, it would map to constexpr sampler injected into the MSL shader.
  • In the first option, it's invalid to have non-null staticSampler if WGPUFeatureName_StaticSamplers is not enabled.
    This is how Vulkan does it (no chained struct)
  • In the second option, it's invalid to chain WGPUSamplerBindingLayoutStatic if WGPUFeatureName_StaticSamplers is not enabled.

The first option is attractive because I expect this could become core, so it would be nifty to put it in the base sampler binding layout struct before we cut the webgpu.h 1.0 header. On the other hand, if it never becomes core, it's awkward that it's in the base struct, but you still need to explicitly enable the feature to use it.

@Kangz
Copy link
Collaborator

Kangz commented Mar 15, 2024

I'm not sure that it will become a core WebGPU feature soon, because it seems a bit niche and polyfillable excpet for YCbCr sampling, or if in the future we have some constexpr sampler similarly to Metal that can be used for implicit layouts. +1 to everything else though!

@austinEng
Copy link
Collaborator Author

I was sorta hoping that even if it doesn't become core WebGPU soon, we could add it in native and then drop all WGPUFeatureName gating it at that point. That could be too much future-predicting probably, and if it doesn't happen, webgpu.h will be in a bad position where there's an optional member that isn't valid except for some specific native implementations.

@kainino0x
Copy link
Collaborator

kainino0x commented Mar 15, 2024

On the other hand, if it never becomes core, it's awkward that it's in the base struct, but you still need to explicitly enable the feature to use it.

This would be fine, we also have unclippedDepth (that is in the JS spec but it is an optional feature).

EDIT: here's the issue where we're merging it to the core struct #212 (comment)

@kainino0x
Copy link
Collaborator

webgpu.h will be in a bad position where there's an optional member that isn't valid except for some specific native implementations.

but agree this wouldn't be great

@austinEng
Copy link
Collaborator Author

Thinking more, I guess policy wise, we should probably be conservative, and probably shouldn't add this until it is in the JS spec first. That means that even if we're very confident it can eventually be core webgpu, it should still be a chained struct. Because even if it does become core, who knows what the name will be - will it be "immutable" or "static" or something else?
Also, what happens if/when we have sampler arrays?

What do you think?

@kainino0x
Copy link
Collaborator

Yeah, that sounds right to me.

@kainino0x kainino0x added the non-breaking Does not require a breaking change (that would block V1.0) label Mar 16, 2024
@austinEng
Copy link
Collaborator Author

Closing this since it's Dawn-internal for now. Can reopen if we want to make it common.

@austinEng austinEng closed this as not planned Won't fix, can't repro, duplicate, stale Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-breaking Does not require a breaking change (that would block V1.0)
Projects
None yet
Development

No branches or pull requests

3 participants