Add texture_1d support to textureSampleLevel by beaufortfrancois · Pull Request #4406 · gpuweb/cts

beaufortfrancois · 2025-06-17T12:27:28Z

As discussed in gpuweb/gpuweb#5001 (comment), this PR adds tests in the CTS for texture_1d support to textureSampleLevel. This is to make sure this actually works as intended as we're implementing in Tint with https://issues.chromium.org/382514673

Requirements for PR author:

All missing test coverage is tracked with "TODO" or .unimplemented().
New helpers are /** documented */ and new helper files are found in helper_index.txt.
Test behaves as expected in a WebGPU implementation. (If not passing, explain above.) It works with Chromium patched with https://dawn-review.googlesource.com/c/dawn/+/247034
Test have be tested with compatibility mode validation enabled and behave as expected. (If not passing, explain above.)

Requirements for reviewer sign-off:

Tests are properly located in the test tree.
Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

beaufortfrancois · 2025-06-17T12:28:57Z

+ * t  The sampled or depth texture to sample.
+ * s  The sampler type.
+ * coords The texture coordinates used for sampling.
+ * level The mip level, clamped to 0.


Will clamped to 0 be part of the spec? Do I need to do something about it in the CTS and Tint implementation?

FYI Here's my naive thoughts regarding the spec: https://github.com/gpuweb/gpuweb/compare/main...beaufortfrancois:gpuweb:textureSampleLevel1d?expand=1

I think "clamped to zero" would fall out of the generic mip level selection rules that apply to all textures. That said, I'm struggling to find exactly where that is written down in either the WGSL or API specs.

As for Tint, I don't think you need to do anything special, as I believe the backend APIs already do The Right Thing (i.e. clamp).

Ideally CTS would test out-of-bounds mip levels to ensure this is the case, but if we're not already doing this for other textures then that would be a separate issue.

This test tests out-of-bounds mip levels. I'm not sure the comment is needed. Whatever number of mips the texture is, the test will test out of bounds mip levels.

I've updated it and used same prose as the other tests for level

jrprice

Looks OK to me but I'm not familiar with the texture execution testing framework.

@greggman could you please take a quick look and see if this looks sound?

jrprice · 2025-06-17T18:28:50Z

+ * t  The sampled or depth texture to sample.
+ * s  The sampler type.
+ * coords The texture coordinates used for sampling.
+ * level The mip level, clamped to 0.


I think "clamped to zero" would fall out of the generic mip level selection rules that apply to all textures. That said, I'm struggling to find exactly where that is written down in either the WGSL or API specs.

As for Tint, I don't think you need to do anything special, as I believe the backend APIs already do The Right Thing (i.e. clamp).

Ideally CTS would test out-of-bounds mip levels to ensure this is the case, but if we're not already doing this for other textures then that would be a separate issue.

jrprice · 2025-06-17T18:31:28Z

+    t.skipIfTextureFormatAndDimensionNotCompatible(format, '1d');
+    skipIfTextureFormatNotSupportedOrNeedsFilteringAndIsUnfilterable(t, minFilter, format);
+
+    // We want at least 4 blocks or something wide enough for 3 mip levels.


This comment seems inaccurate since we're only testing a single level here?

maybe leave it? If 1d textures ever support mip levels nothing needs to change here. We'd just change chooseTextureSize. It's not technically inaccurate. We're choosing a size that could support at least 3 mip levels. We're not actually making 3 mip levels.

I'll leave it then.

greggman · 2025-06-17T18:56:18Z

+    t.skipIfTextureFormatAndDimensionNotCompatible(format, '1d');
+    skipIfTextureFormatNotSupportedOrNeedsFilteringAndIsUnfilterable(t, minFilter, format);
+
+    // We want at least 4 blocks or something wide enough for 3 mip levels.


maybe leave it? If 1d textures ever support mip levels nothing needs to change here. We'd just change chooseTextureSize. It's not technically inaccurate. We're choosing a size that could support at least 3 mip levels. We're not actually making 3 mip levels.

greggman · 2025-06-17T18:57:14Z

+ * t  The sampled or depth texture to sample.
+ * s  The sampler type.
+ * coords The texture coordinates used for sampling.
+ * level The mip level, clamped to 0.


This test tests out-of-bounds mip levels. I'm not sure the comment is needed. Whatever number of mips the texture is, the test will test out of bounds mip levels.

greggman · 2025-06-17T19:07:57Z

+      mipmapFilter: minFilter,
+    };
+
+    const calls: TextureCall<vec1>[] = generateTextureBuiltinInputs1D(50, {


I think this code needs the mipLevel code to match the 2d code. Both in the call to genearteTextureBuiltinInputs1D and in the return code. That's what makes it test out of bounds mip levels. It doesn't exist in textureSample test because texture sample test chooses mip levels by derivatives.

Done. Thanks for catching this!

beaufortfrancois requested a review from jrprice June 17, 2025 12:27

beaufortfrancois commented Jun 17, 2025

View reviewed changes

Add texture_1d support to textureSampleLevel

5814f41

beaufortfrancois force-pushed the textureSampleLevel-1d branch from c969e78 to 5814f41 Compare June 17, 2025 12:47

jrprice reviewed Jun 17, 2025

View reviewed changes

jrprice requested a review from greggman June 17, 2025 18:33

greggman requested changes Jun 17, 2025

View reviewed changes

beaufortfrancois added 2 commits June 18, 2025 09:21

Address feedback from @greggman

200f68e

Merge branch 'main' of github.com:gpuweb/cts into textureSampleLevel-1d

ce42687

greggman approved these changes Jun 20, 2025

View reviewed changes

beaufortfrancois merged commit 2a8d4a8 into main Jun 20, 2025
1 check passed

beaufortfrancois deleted the textureSampleLevel-1d branch June 20, 2025 04:51

Conversation

beaufortfrancois commented Jun 17, 2025

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!

jrprice 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants