KhronosGroup / KTX-Software

KTX (Khronos Texture) Library and Tools
Other
887 stars 233 forks source link

ktxTexture2_DecodeAstc does not check its vkformat parameter #960

Open MarkCallow opened 4 days ago

MarkCallow commented 4 days ago

The function needs to check that the provided format is supported by the decoder for the input ASTC format. E.g. an 8-bit UNORM format for UNORM ASTC formats, an 8-bit SRGB format for SRGB ASTC formats and a FLOAT format for ASTC HDR input. Function should return KTX_INVALID_OPERATION for a mismatched VkFormat.

Actually is there even a point to the vkformat parameter? Can the underlying decoder do any conversions?

@wasimabbas-arm please fix this. I can't find a way to add you to the Assignees list.

wasimabbas-arm commented 3 days ago

@MarkCallow

Woops. Looks like at call site I have been using

ec = ktxTexture2_DecodeAstc(texture, VK_FORMAT_R8G8B8A8_UNORM);

I had some TODOs, in the decoding bit some for HDR that I won't resolve now but other for using this to make the right profile and not hard code it to astcenc_profile profile{ASTCENC_PRF_LDR_SRGB}; would still be useful but read next.

This means the PSNR tests usings those specific values for those specific golden files won't be correct either? Having a look where I am using this method in the metrics.

ec = ktxTexture2_DecodeAstc(texture, VK_FORMAT_R8G8B8A8_UNORM);
ec = ktxTexture2_TranscodeBasis(texture, KTX_TTF_RGBA32, 0);

Both Basis and ASTC are using hardcoded RGBA format. So I guess that cancells out when it comes to PSNR.

If metrics can be calculated in any specific format then ktxTexture2_DecodeAstc should deal with the format correctly. If its always going to be hardcoded RGBA8 then the format doesn't need to be provided in to ktxTexture2_DecodeAstc. What's the deal with ktxTexture2_TranscodeBasis? should that also be transcoding to the right format? Does it support enough uncompressed formats that maps to what we would need?

Actually is there even a point to the vkformat parameter? Can the underlying decoder do any conversions?

Yes the decoder can decode to the right format. From sRGB astc to sRGB image and RGB astc into RGB image etc, including HDR formats (16 and 32bit).

MarkCallow commented 2 days ago

It is fine, until we add HDR support, for metrics to hardcode passing VK_FORMAT_R8G8B8A8_UNORM to ktxTexture2_DecodeAstc since you say it will convert sRGB. The vkformat parameter of ktxTexture2_DecodeAstc must be correctly mapped to the astcenc format.

ktxTexture2_TranscodeBasis can transcode to RGBA32, R5G6B5 and RGB4444. The first is the equivalent of VK_FORMAT_R8G8B8A8_UNORM.

Since ktxTexture2_DecodeAstc is a public function it must reject formats not supported by the decoder in astcenc which includes, obviously, any ASTC format other than that of the input texture. Should the same format be a nop or should it raise an error? It also includes any other block compressed format. At present there are no checks whatever. Please add them.

I should have spotted this when reviewing the original PR. My bad.