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

Implement texelData for rgb9e5ufloat and add texelData unit tests #352

Merged
merged 4 commits into from
Nov 18, 2020

Conversation

austinEng
Copy link
Collaborator

  • Adds dataType and componentType to the texture format table for encodable texture formats. dataType is the format's data representation whereas componentType is the GPUTextureComponentType - the type you get in the shader.
  • Implements texelData helpers for packed format rgb9e5ufloat
  • Adds unit tests for upload texel data as dataType, loading the componentType in the shader, and writing it out.

@austinEng austinEng marked this pull request as draft November 13, 2020 18:32
@austinEng
Copy link
Collaborator Author

Running the tests in https://chromium-review.googlesource.com/c/chromium/src/+/2536758
it looks okay modulo precision..

Going to need to have some way to have a tolerance that varies based on ULPs, the expected value, and the component index (since some formats have different precision for each component).

This test uploads a single texel to encodable texture formats
and then reads it back in a shader with textureLoad to check that
the values are as expected.
@austinEng austinEng marked this pull request as ready for review November 14, 2020 00:27
@austinEng austinEng requested a review from kainino0x November 14, 2020 00:28

const float10 = {
decode: identity,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, this wouldn't actually be the identity function but would cut the precision of the input value down so that it could be represented by a float10

I'm not sure if that will actually help the precision issues though or simply need a better way to compute tolerance.

.map((C, i) => {
return `[[offset(${i * 4})]] result${C} : ${shaderType};`;
})
.join('\n')}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could just return a vec4<shaderType>. Then you can just output.result = textureLoad(...) I think. In the result you can just leave the unused components untested so we're not testing that behavior here (should be a separate test).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but then I can't just use t.expectContents of the whole thing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Couldn't you still make this a simple vec4 and write into it selectively in main()? (Or can you not put swizzles on the left of = in wgsl?) Anyway, either is okay.

} & TextureFormatInfo;
} = /* prettier-ignore */ {
'depth32float': { renderable: true, color: false, depth: true, stencil: false, storage: false, copySrc: true, copyDst: false, bytesPerBlock: 4, blockWidth: 1, blockHeight: 1 },
'depth32float': { renderable: true, color: false, depth: true, stencil: false, storage: false, copySrc: true, copyDst: false, bytesPerBlock: 4, blockWidth: 1, blockHeight: 1, dataType: 'float', componentType: 'float' },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmh can't this also be "depth-comparison"? How are we going to handle formats that support multiple components types? (or is this just for sampling in a texture to read the value and nothing else?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably going to need to make this an array or we'll have to add per-aspect information at some point.. but for now it's just for the sampling part.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given I think we're changing 'depth-comparison' to 'depth-stencil' upstream, maybe we don't even need to support 'float' component type for depth formats?

Copy link
Collaborator Author

@austinEng austinEng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL again

} & TextureFormatInfo;
} = /* prettier-ignore */ {
'depth32float': { renderable: true, color: false, depth: true, stencil: false, storage: false, copySrc: true, copyDst: false, bytesPerBlock: 4, blockWidth: 1, blockHeight: 1 },
'depth32float': { renderable: true, color: false, depth: true, stencil: false, storage: false, copySrc: true, copyDst: false, bytesPerBlock: 4, blockWidth: 1, blockHeight: 1, dataType: 'float', componentType: 'float' },
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably going to need to make this an array or we'll have to add per-aspect information at some point.. but for now it's just for the sampling part.

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

.map((C, i) => {
return `[[offset(${i * 4})]] result${C} : ${shaderType};`;
})
.join('\n')}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Couldn't you still make this a simple vec4 and write into it selectively in main()? (Or can you not put swizzles on the left of = in wgsl?) Anyway, either is okay.

@austinEng austinEng merged commit e0b76d2 into gpuweb:main Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants