Add validation steps for createTexture and createTextureView. #1237
Conversation
Forbid zero-sized textures and views.
Kangz
left a comment
Kangz
left a comment
There was a problem hiding this comment.
I assume we'll want much more validation for texture creation but LGTM as an initial batch.
|
Any reason this isn't based on #799? |
kvark
left a comment
kvark
left a comment
There was a problem hiding this comment.
Thank you for writing this down! Needs some work before landing
I didn't realize that was written! |
kvark
left a comment
kvark
left a comment
There was a problem hiding this comment.
Looks good for the most part, thank you for updating!
| - 1 ≤ {{GPUTextureDescriptor/size}}.height | ||
| - 1 ≤ {{GPUTextureDescriptor/size}}.depth | ||
| - Let |maxDim| = {{GPUTextureDescriptor/size}}.width | ||
| - if {{GPUTextureDescriptor/dimension}} == {{GPUTextureDimension/'2d'}}: |
There was a problem hiding this comment.
to editors: we'll probably turn this into a switch
| <div class=validusage> | ||
| - 0 ≤ {{GPUTextureViewDescriptor/baseMipLevel}} < {{GPUTextureDescriptor/mipLevelCount}} | ||
| - 1 ≤ {{GPUTextureViewDescriptor/mipLevelCount}} ≤ {{GPUTextureDescriptor/mipLevelCount}} - {{GPUTextureViewDescriptor/baseMipLevel}} | ||
| - 0 ≤ {{GPUTextureViewDescriptor/baseArrayLayer}} < {{GPUTextureDescriptor/size}}.depth |
There was a problem hiding this comment.
the depth should only be related to the array layers for non-3D textures
| - {{GPUTextureDescriptor/size}}.width == {{GPUTextureDescriptor/size}}.height | ||
| - {{GPUTextureViewDescriptor/arrayLayerCount}} % 6 == 0 | ||
|
|
||
| - if |dimension| is {{GPUTextureViewDimension/"3d"}}: |
There was a problem hiding this comment.
to editors: similarly, we'll want to turn this into a switch
| - 1 ≤ {{GPUTextureDescriptor/size}}.depth | ||
| - Let |maxDim| = {{GPUTextureDescriptor/size}}.width | ||
| - if {{GPUTextureDescriptor/dimension}} == {{GPUTextureDimension/'2d'}}: | ||
| |maxDim| = max(|maxDim|, {{GPUTextureDescriptor/size}}.height) |
There was a problem hiding this comment.
Maybe something like:
| |maxDim| = max(|maxDim|, {{GPUTextureDescriptor/size}}.height) | |
| Set |maxDim| = max(|maxDim|, {{GPUTextureDescriptor/size}}.height) |
mainly because right now it kind of looks like a boolean condition
| - 1 ≤ {{GPUTextureDescriptor/size}}.width | ||
| - 1 ≤ {{GPUTextureDescriptor/size}}.height | ||
| - 1 ≤ {{GPUTextureDescriptor/size}}.depth | ||
| - Let |maxDim| = {{GPUTextureDescriptor/size}}.width |
There was a problem hiding this comment.
aside: One reason I've been contemplating using "must" on every line is that it fixes the awkwardness of mixing boolean expressions with imperative statements like "Let x = ...". I don't know what the right style is though.
- 1 must ≤ size.width
- 1 ≤ size.width must be true
- ?
| - 1 ≤ {{GPUTextureDescriptor/size}}.height | ||
| - 1 ≤ {{GPUTextureDescriptor/size}}.depth | ||
| - Let |maxDim| = {{GPUTextureDescriptor/size}}.width | ||
| - if {{GPUTextureDescriptor/dimension}} == {{GPUTextureDimension/'2d'}}: |
There was a problem hiding this comment.
nit: capitalize "If"
nit: slight preference for "is" over "=="
| error and return an [=invalid=] {{GPUTextureView}}. | ||
| <div class=validusage> | ||
| - 0 ≤ {{GPUTextureViewDescriptor/baseMipLevel}} < {{GPUTextureDescriptor/mipLevelCount}} | ||
| - 1 ≤ {{GPUTextureViewDescriptor/mipLevelCount}} ≤ {{GPUTextureDescriptor/mipLevelCount}} - {{GPUTextureViewDescriptor/baseMipLevel}} |
There was a problem hiding this comment.
GPUTextureViewDescriptor/mipLevelCount and GPUTextureDescriptor/mipLevelCount are indistinguishable in the rendered text. Needs to be |something|.{{GPUTextureViewDescriptor/mipLevelCount}} and so on.
| error and return an [=invalid=] {{GPUTextureView}}. | ||
| <div class=validusage> | ||
| - 0 ≤ {{GPUTextureViewDescriptor/baseMipLevel}} < {{GPUTextureDescriptor/mipLevelCount}} | ||
| - 1 ≤ {{GPUTextureViewDescriptor/mipLevelCount}} ≤ {{GPUTextureDescriptor/mipLevelCount}} - {{GPUTextureViewDescriptor/baseMipLevel}} |
There was a problem hiding this comment.
[1] IMO clearer as
- view.baseMipLevel ≥ 0
- view.mipLevelCount ≥ 1
- view.baseMipLevel + view.mipLevelCount ≤ texture.mipLevelCount
(and note the third rule makes "view.baseMipLevel < texture.mipLevelCount" redundant)
We've done this in other places, with the assumption that numbers in the spec operate in the space of mathematical real numbers (or integers) and have no overflow behaviors.
| - 0 ≤ {{GPUTextureViewDescriptor/baseMipLevel}} < {{GPUTextureDescriptor/mipLevelCount}} | ||
| - 1 ≤ {{GPUTextureViewDescriptor/mipLevelCount}} ≤ {{GPUTextureDescriptor/mipLevelCount}} - {{GPUTextureViewDescriptor/baseMipLevel}} | ||
| - 0 ≤ {{GPUTextureViewDescriptor/baseArrayLayer}} < {{GPUTextureDescriptor/size}}.depth | ||
| - 1 ≤ {{GPUTextureViewDescriptor/arrayLayerCount}} ≤ {{GPUTextureDescriptor/size}}.depth - {{GPUTextureViewDescriptor/baseArrayLayer}} |
There was a problem hiding this comment.
similar comment here to [1]
| - if {{GPUTextureViewDescriptor/aspect)) == {{GPUTextureAspect/"depth-only"}} | ||
| - {{GPUTextureViewDescriptor/format}} must have a depth component | ||
| - if {{GPUTextureViewDescriptor/aspect)) == {{GPUTextureAspect/"stencil-only"}} | ||
| - {{GPUTextureViewDescriptor/format}} must have a stencil component |
There was a problem hiding this comment.
| - {{GPUTextureViewDescriptor/format}} must have a stencil component | |
| - {{GPUTextureViewDescriptor/format}} must have a stencil component | |
| Issue: create and link to algorithms for "have a {depth,stencil} component". |
|
I opened #1397 not realizing this PR was still in flight. Can we move forward on it? Missing texture / view creation validation is a big hole. |
|
createView algorithm landed with #1406. |
|
I'll close this, we should do any remaining work from this PR in new PR(s). |
This CL adds unimplemented stubs for the `sinh` builtin. Issue: gpuweb#1237
4 participants
Forbid zero-sized textures and views.
Resolves #1200.
Preview | Diff