Add minmax-sampling feature #1283

kvark · 2020-12-08T17:16:54Z

Closes #1267

This is useful for building hierarchical depth bounds, as one example.

Hardware support:

Vulkan: VK_EXT_sampler_filter_minmax is present on around 50% desktops and 25% Android devices. There is a set of formats supporting min/max sampling, enabled by filterMinmaxSingleComponentFormats property. It's hard to grasp the support surface for the others.
D3D12: apparently gated by TILED_RESOURCES_TIER_2, but I don't know how to find the stats on how widely it's supported, and whether or not all the formats support it (@RafaelCintron ?)
D3D11: MinMaxFiltering flag, nice documentation in https://docs.microsoft.com/en-us/windows/win32/direct3d11/tiled-resources-texture-sampling-features
Metal: not seeing anything for this

Note: min/max sampling shouldn't be considered "filtering" for the purpose of validation.

TODO:

figure out which formats support this, potentially add a column to our format table.
determine if it's a filter mode, or something entirely different. Can a sampler be linearly magnifying but minmaxing the minification? In D3D12 it can, it seems, but maybe we don't need that.
determine if a separate sampler binding type is needed

Preview | Diff

github-actions · 2020-12-08T17:18:58Z

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
_a979cf8

spec/index.bs

Kangz · 2020-12-08T17:23:34Z

spec/index.bs

@@ -1375,6 +1375,7 @@ enum GPUFeatureName {
    "pipeline-statistics-query",
    "texture-compression-bc",
    "timestamp-query",
+    "minmax-sampling",


Is minmax sampling allowed for uint, int textures, but also for comparison samplers?

The minmax feature in vulkan doesn't require support for any integer formats, so I assume we wouldn't support any. It also can't be used with comparison samplers apparently (kinda weird).

If the format is a depth/stencil format, this bit only specifies that the depth aspect (not the stencil aspect) of an image of this format supports min/max filtering, and that min/max filtering of the depth aspect is supported when depth compare is disabled in the sampler.

It could be, but getting the stats for this is hard :(
For now the spec only allows it on a few selected formats (see last commit).

Wow, that's not a lot of formats.

spec/index.bs

kainino0x · 2020-12-08T17:41:19Z

spec/index.bs

@@ -7239,6 +7257,22 @@ The following enums are supported if and only if the {{GPUFeatureName/"timestamp
        * {{GPUQueryType/"timestamp"}}
 </dl>

+## <dfn dfn-type=enum-value dfn-for=GPUFeatureName>minmax-sampling</dfn> ## {#minmax-sampling}


Suggested change

## <dfn dfn-type=enum-value dfn-for=GPUFeatureName>minmax-sampling</dfn> ## {#minmax-sampling}

## <dfn enum-value for=GPUFeatureName>minmax-sampling</dfn> ## {#minmax-sampling}

We should change this across all features, consistently (as a follow-up)

github-actions · 2020-12-08T18:09:35Z

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
_2c2a89f

spec/index.bs

kainino0x · 2020-12-08T18:21:28Z

spec/index.bs

@@ -1375,6 +1375,7 @@ enum GPUFeatureName {
    "pipeline-statistics-query",
    "texture-compression-bc",
    "timestamp-query",
+    "minmax-sampling",


Wow, that's not a lot of formats.

github-actions · 2020-12-08T18:46:29Z

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
_53bd4d1

github-actions · 2020-12-08T18:50:52Z

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
_eb1c48d

kvark · 2020-12-08T18:51:02Z

Thank you for the quick and detailed reviews! We have now gone through a few iterations, and the spec is not quite the same as it was in the first draft.
There appear to be some interesting interactions between the filtering and reduction modes (which is a new enum added in the last commit). In Vulkan:

The reduction mode is orthogonal to the minification and magnification filter parameters. The filter parameters are used to identify the set of texels used to produce a final filtered value; the reduction mode identifies how these texels are combined.

So it sounds like a format still has to be filterable, even if the min/max reduction mode is used. This mostly affects "r32float", which I tentatively marked as enabled for min/maxing, even though one technically can do it today, since filtering is not supported (i.e. they get a WebGPU validation error because of filtering requirements).

kainino0x

OK; as I said on the issue I wasn't sure whether it was tied to filterability or not.

I should have noticed that this should have defined a reduction mode instead of a filter mode, whoops.

spec/index.bs

kainino0x · 2020-12-09T23:47:00Z

spec/index.bs

+            |s|.{{GPUSampler/[[descriptor]]}}.{{GPUSamplerDescriptor/compare}} is `null` or undefined.
+            Otherwise, set it to `true`.
+        1. Set |s|.{{GPUSampler/[[isMinMax]]}} to `false` if
+            |s|.{{GPUSampler/[[descriptor]]}}.{{GPUSamplerDescriptor/reduction}} is {{GPUReductionMode/"weighted-average"}}.


Seems kind of backward, I think it's easier to read if it says set it to true if it's "minimum" or "maximum"

kainino0x · 2020-12-09T23:48:18Z

spec/index.bs

        1. Set |s|.{{GPUSampler/[[isFiltering]]}} to `false` if none of {{GPUSamplerDescriptor/minFilter}},
            {{GPUSamplerDescriptor/magFilter}}, or {{GPUSamplerDescriptor/mipmapFilter}} has the value of
            {{GPUFilterMode/"linear"}}. Otherwise, set it to `true`.
+        1. Set |s|.{{GPUSampler/[[isComparison]]}} to `false` if
+            |s|.{{GPUSampler/[[descriptor]]}}.{{GPUSamplerDescriptor/compare}} is `null` or undefined.
+            Otherwise, set it to `true`.


You can just say undefined; JS null and undefined both get converted to IDL undefined
https://heycam.github.io/webidl/#es-dictionary

I would probably flip this to say "true if compare is defined" or "is specified" or "is not undefined".

Wish we could come up with a better phrasing than "otherwise set it to true". Like "set isComparison to (s.descriptor.compare ≠ undefined)" maybe?

kainino0x · 2020-12-09T23:53:55Z

spec/index.bs

@@ -3555,7 +3573,7 @@ A {{GPUProgrammableStageDescriptor}} describes the entry point in the user-provi
                        : {{GPUSamplerBindingType/"comparison"}}
                        :: the |binding| is a comparison sampler
                        : {{GPUSamplerBindingType/"minmax"}}
-                        :: the |binding| is a min/max sampler
+                        :: the |binding| is a non-comparison sampler


Maybe have "filtering or non-filtering or minmax" as one case, or just "otherwise".

kainino0x · 2020-12-10T00:14:36Z

spec/index.bs

@@ -2349,7 +2355,7 @@ match one texel.
 <script type=idl>
 enum GPUFilterMode {
    "nearest",
-    "linear"
+    "linear",


Briefly considering if we try to come up with a word other than linear, since this also describes minmax filters which aren't exactly "linear"? I'm not sure what it would be; "nearest"/"linear"(/"cubic") is well understood terminology. I think they're clearest.

Vulkan has an extension to use min/max with "cubic", too, fwiw: https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.html#textures-texel-cubic-filtering

Kangz · 2020-12-10T13:00:48Z

nit: are we sure we want to have this in the spec given it seems a very niche or advanced feature? I'm worried you both have spend significant time on this when it is nowhere close to the critical path to getting WebGPU out :)

kvark · 2020-12-10T15:35:22Z

I just needed a warm-up before starting #1293 :)
Seriously though, I don't yet know if it's needed for MVP.
We don't have it implemented, although it's not going to be too hard to add.
Ideally, we'd reach out to ISVs and ask them about a number of features that we are considering for MVP, including this one, and make a decision based on this feedback.

…1417) This PR adds unimplmented stubs for the read-write-modify atomic operations. * `atomicAdd` * `atomicAnd` * `atomicCompareExchangeWeak` * `atomicExchange` * `atomicMax` * `atomicMin` * `atomicOr` * `atomicSub` * `atomicXor` Issue gpuweb#1275, gpuweb#1276, gpuweb#1277, gpuweb#1278, gpuweb#1279, gpuweb#1280, gpuweb#1281, gpuweb#1282, gpuweb#1283

kvark added the feature request A request for a new GPU feature exposed in the API label Dec 8, 2020

kvark mentioned this pull request Dec 8, 2020

Feature for Min/Max filter modes gfx-rs/wgpu#1065

Open

Kangz reviewed Dec 8, 2020

View reviewed changes

kainino0x approved these changes Dec 8, 2020

View reviewed changes

kvark marked this pull request as draft December 8, 2020 18:27

kvark force-pushed the minmax branch from 53bd4d1 to eb1c48d Compare December 8, 2020 18:48

kvark marked this pull request as ready for review December 8, 2020 18:51

kainino0x reviewed Dec 8, 2020

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

kvark added 4 commits December 8, 2020 17:54

Add minmax-sampling feature

867e5d5

Add min/max capability to the format table

fcda953

Add GPUReductionMode

8fdb231

Add "isMinMax" internal slot to GPUSampler

e396b48

kvark force-pushed the minmax branch from eb1c48d to e396b48 Compare December 8, 2020 23:00

kvark mentioned this pull request Dec 9, 2020

Integer Sampling & Sampler Reduction Modes #1267

Open

kainino0x approved these changes Dec 10, 2020

View reviewed changes

kvark mentioned this pull request Dec 10, 2020

Sampler reduction modes gfx-rs/gfx#3523

Closed

kvark mentioned this pull request Jan 13, 2021

Add image features for transfers and min/max sampling gfx-rs/gfx#3576

Merged

3 tasks

kvark added this to the post-V1 milestone Nov 22, 2021

kainino0x marked this pull request as draft August 25, 2022 01:35

kainino0x added the api WebGPU API label May 21, 2024

	## <dfn dfn-type=enum-value dfn-for=GPUFeatureName>minmax-sampling</dfn> ## {#minmax-sampling}
	## <dfn enum-value for=GPUFeatureName>minmax-sampling</dfn> ## {#minmax-sampling}

Add minmax-sampling feature #1283

Are you sure you want to change the base?

Add minmax-sampling feature #1283

Uh oh!

Conversation

kvark commented Dec 8, 2020 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 8, 2020

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 8, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 8, 2020

Uh oh!

github-actions bot commented Dec 8, 2020

Uh oh!

kvark commented Dec 8, 2020

Uh oh!

kainino0x left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Kangz commented Dec 10, 2020

Uh oh!

kvark commented Dec 10, 2020

Uh oh!

Uh oh!

kvark commented Dec 8, 2020 •

edited by pr-preview bot

Loading