bytesPerBlock refactors; generalize validation,image_copy,buffer_related over aspects #3490
Conversation
- Test a few depth/stencil formats that weren't in kSizedTextureFormats (depth24plus-stencil8 stencil, depth32float-stencil8 depth+stencil) - Parameterize over aspect (depth-only/stencil-only/all) - Reduce some overparameterization to make up for it (net ~3x faster) Removes a use of the deprecated .bytesPerBlock.
|
I've been sending all these reviews to @lokokung, but at this point I can rebalance them. @shrekshao could you PTAL? |
There was a problem hiding this comment.
LGTM with comments
For RROR: Tests missing from listing_meta.json. Please add the new tests (See docs/adding_timing_metadata.md):, you may add it manually, or run cts with dawn-node which will automatically generate for you (Ben's slides can be found at https://sites.google.com/corp/google.com/webgpu/projects/cts?authuser=0)
| (!!info.depth || !!info.stencil) && p.copyHeightInBlocks !== p._textureHeightInBlocks | ||
| ); | ||
| }) | ||
| .unless(p => p._aspectKey !== 'color' && p.copyHeightInBlocks !== p._textureHeightInBlocks) |
There was a problem hiding this comment.
The only p.copyHeightInBlocks !== p._textureHeightInBlocks case is p.copyHeightInBlocks = 0, p._textureHeightInBlocks = 1 which can be a no-op. I think it's better expand _textureHeightInBlocks with [p.copyHeightInBlocks, pp.copyHeightInBlocks + 1] for > 0 cases?
| _aspectKey: 'color' | 'depth' | 'stencil'; | ||
| }> { | ||
| const info = kTextureFormatInfo[format]; | ||
| if (info.color?.bytes) { |
There was a problem hiding this comment.
Question: why is here info.color?.bytes instead of info.color
| } from '../../../format_info.js'; | ||
| import { align } from '../../../util/math.js'; | ||
| import { ImageCopyType } from '../../../util/texture/layout.js'; | ||
| import { ValidationTest } from '../validation_test.js'; | ||
|
|
||
| export const kReducedFormatsList = [ | ||
| 'r8unorm', |
There was a problem hiding this comment.
For testing compat copyT2B workaround impl purporse I'd like to also include ['r8snorm', 'rg8snorm', 'bgra8unorm']
Removes two uses of the deprecated .bytesPerBlock.
(depth24plus-stencil8 stencil, depth32float-stencil8 depth+stencil)
Issue: Part 7 of #2495
Requirements for PR author:
.unimplemented()./** documented */and new helper files are found inhelper_index.txt.Requirements for reviewer sign-off:
When landing this PR, be sure to make any necessary issue status updates.