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

ktxTexture2_DecodeAstc does not check its vkformat parameter #960

Open
MarkCallow opened this issue Nov 29, 2024 · 2 comments
Open

ktxTexture2_DecodeAstc does not check its vkformat parameter #960

MarkCallow opened this issue Nov 29, 2024 · 2 comments

Comments

@MarkCallow
Copy link
Collaborator

MarkCallow commented Nov 29, 2024

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
Copy link
Contributor

wasimabbas-arm commented Nov 29, 2024

@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
Copy link
Collaborator Author

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.

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

No branches or pull requests

2 participants