Add GPUTextureDescriptor option for srgb reinterpretation #2336
Conversation
| : <dfn>viewFormats</dfn> | ||
| :: | ||
| Specifies what view {{GPUTextureViewDescriptor/format}} values will be allowed when calling | ||
| {{GPUTexture/createView()}} on this texture. |
There was a problem hiding this comment.
should we also say that copying to/from these formats is allowed?
There was a problem hiding this comment.
Copying between srgb and non-srgb is always allowed (#2329), so we don't need anything more than that until viewFormats can express something beyond that (#842 (comment)).
| Specifies what view {{GPUTextureViewDescriptor/format}} values will be allowed when calling | ||
| {{GPUTexture/createView()}} on this texture. | ||
|
|
||
| <!-- This could be extended in the future with either more enum options |
There was a problem hiding this comment.
Why not go for it from the start? Currently, the PR defines this new layer of abstraction by introducing the ways of "modifying" formats, like adding/removing sRGB. So users have to think which exact formats they can use, anyway. They might as well list these formats. If they are wrong, they'll get an error earlier.
There was a problem hiding this comment.
Because we don't have consensus on whether to do general reinterpretation with a flag or a list, but we had tentative consensus on adding something for srgb reinterpretation specifically.
There was a problem hiding this comment.
Ok, I see. On one hand, we have to have spec text similar to this describing which classes of reinterpretations are going to be allowed, regardless of how the API is actually shaped at view creation. So just making this description of classes to be the API makes sense.
But on the other hand, it makes less sense. As a user of WebGPU, I know a texture format, and I know which one I'll need to reinterpret with. So instead of just telling it, I have to go through this list of classes and see which one includes my format conversion. Then the implementation has to potentially do the opposite on Vulkan.
Speaking of which, would the implementation pessimistically specify all of the possible formats (allowed by selected classes) to VK_KHR_image_format_list? Do we have an idea if this will make it less efficient than telling Vulkan the exact format we want to use?
There was a problem hiding this comment.
With VK_KHR_image_format_list I'm not sure. In the srgb case it's better than a general 'reinterpret' flag since we only have to list the srgb formats when using that extension on the backend. But if we had a general 'reinterpret' flag that was based on the intersection of Vulkan/D3D12/Metal compatibility classes then I don't know.
There was a problem hiding this comment.
Right, the "srgb" is a simpler case because it only has 2 formats in a class. So an implementation on VK_KHR_image_format_list would know exactly what to specify, and it would be no worse than the user telling us the list of formats.
However, this GPUTextureReinterpretationOption enum is meant to grow into other things, which are going to be more wide. So it's not clear to me how it's going to scale, given the concerns here. Perhaps, it would be productive to come up with at least one more value for it, so that we can see it more generally?
There was a problem hiding this comment.
The other option I have in mind would be a predefined list depending on the texture format, according to the investigation #744 (comment). So essentially everything that's green on the D3D12 tab of that spreadsheet as it's the most restrictive. It could potentially be called something like "same-layout" (D3D12 requires the components sit in the same bits of the texel, but allows reinterpreting between int/unorm/float/etc).
However if we ended up agreeing on a format list then perhaps we wouldn't bother with that.
There was a problem hiding this comment.
So there can be:
- srgb (current)
- same layout (the green boxes in D3D12 table), which also includes srgb
- 32bit-uint (for the yellow box in D3D12 table)
There was a problem hiding this comment.
The yellow box is a bit complex because it only works for UAVs. I'm not sure how we would want to expose that.
|
Tweaked the enum value name for further bikeshedding, and added support to GPUCanvasConfiguration and address #1231 (comment). |
|
To roundup my concern, it's mostly that merging the castable sets into groups would end up with some backends going a less efficient path for no good reason. For example, if we expose the "same-layout" option for casting, then "r32-float" would be castable to "r32-uint". Looking at the RADV code, the DCC (color compression) will have to be disabled if both formats aren't float or non-float at the same time: (type1 == dcc_channel_float) != (type2 == dcc_channel_float)Therefore, even if the user only wanted to cast "r32-sint" to "r32-uint", their program will run slower on AMD than necessary. Instead, we can ask users to just give us a list of formats. We'd still have to define the classes of reinterpretation in the spec (of what's allowed), but our implementation would in general run faster this way. Arguably, it's also easier for users to just give us the format list. |
|
I don't disagree with you. I'm going to post some more thoughts on #168. If we did have a list, this particular preset isn't that useful (otherwise you can just specify |
|
Meeting: Group accepted this, modulo bikeshedding, punting on whether we do a view format list or additional "tiers" of format reinterpretation but the people in that meeting leaned toward tiers if we can come up with reasonable ones. |
kainino0x
added
copyediting
|
Closing in favor of a full list of formats. I'll open that as a new PR. |
Here's a hopefully-future-proof proposal for how to expose just srgb reinterpretation for now until we figure out how we will expose more flexible texture reinterpretation.
Issue: #168
Related: #744, #811
Preview | Diff