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

Fix ktxTexture2_DecodeAstc error returns and document them. #961

Merged
merged 5 commits into from
Dec 22, 2024

Conversation

MarkCallow
Copy link
Collaborator

@MarkCallow MarkCallow commented Nov 29, 2024

This started due to a warning when building documentation that astc_encode.cpp was not being found. After fixing that I noticed issues with the documentation for ktxTexture2_DecodeAstc which I fixed in c16537c. That commit also fixed an issue where the function would fail with KTX_INVALID_OPERATION if a zlib or zstd supercompressed file had been opened without specifying the LOAD_DATA flag by removing the check for KTX_SS_NONE.

Subsequently I realized there still some supercompression scheme situations that need to be guarded against and I realized the function documentation was not listing the possible return values. While fixing these I spotted a write to stdout that this PR comments out. Libraries potentially used by GUI apps should not write to stdout. We could potentially add a flag parameter to request the ouput.

@wasimabbas-arm please review this PR.

Why do decoder failures return KTX_INVALID_OPERATION? Could you analyze the astcenc error and potentially map to other KTX_ errors? Alternatively should we add a new error such as KTX_ASTC_DECODE_FAILED? KTX_INVALID_OPERATION is intended for cases where the application has set things up in way that is invalid for the operation it is requesting.

Adding unit tests for this function to texturetests would be a good thing. Tests of the encode function would be good too, if not already existing.

@wasimabbas-arm
Copy link
Contributor

I have always had problem with the right KTX_errors. For example in astcenc_context_alloc there are quite a lot of different ASTC enoder specific errors returned. But at call site:

    astc_error  = astcenc_context_alloc(&astc_config, threadCount, &astc_context);

    if (astc_error != ASTCENC_SUCCESS)
        return KTX_INVALID_OPERATION;

I can only turn that into KTX_INVALID_OPERATION. So I could never workout how verbose do we have to be about these errors. A rule of thumb could be that if a user can do something about an error it should provide enough context for them to be able to.

In this case astc_context_alloc has many different errors that I have to fix in the KtxTexture2_DecodeAstc but for the user a KTX_INVALID_OPERATION is sufficient. Otherwise we are gonna have to translate all encoder messages into KTX message/errors.

At a minimum creating generic KTX_ENCODE_ERROR and KTX_DECODE_ERROR makes sense. So something like the attached patch would make sense.

diff.patch

@MarkCallow
Copy link
Collaborator Author

Thanks for the diff.

Looking at the list of astcenc_error values it seems to me that some of them may correspond to passing bad parameters to the functions. If so, we should have asserts for those as the libktx code is setting up all the parameters. The one obvious one that isn't is ASTCENC_ERR_OUT_OF_MEM. That should checked for and KTX_OUT_OF_MEMORY returned. Any not fitting into either of these categories we can return as KTX_DECODE_ERROR.

Please list the codes that are for bad parameters vs. those, if any, that are for bad blocks in the input ASTC image.

@abbaswasim
Copy link
Contributor

@MarkCallow not sure what are you proposing. I usually work on PRs that I create. This is your PR. I didn't know if you want me to provide commits to this or create another PR? (This is why I provided that patch instead)

Also not sure what you mean by:

Please list the codes that are for bad parameters vs. those, if any, that are for bad blocks in the input ASTC image.

@MarkCallow
Copy link
Collaborator Author

Sorry for the silence. I've been away at Siggraph Asia.

Also not sure what you mean by:

Please list the codes that are for bad parameters vs. those, if any, that are for bad blocks in the input ASTC image.

I'm want to identify any error codes, if any, that mean bad parameters were passed to astcenc functions. If so, return of such error codes should be checked with asserts. Since KTX-Software is in control of the parameters passed to astc functions and such errors are the fault of KTX-Software.

Other codes, other than ASTCENC_ERR_OUT_OF_MEM, I was assuming are for bad data (i.e. blocks) in the encoded image, hence my choice of language.

I will update this PR once we've decided on how to improve the error handling.

@MarkCallow
Copy link
Collaborator Author

@wasimabbas-arm From studying the source code I have created a mapping between astcenc_error and ktx_error_code_e with asserts for things that are the fault of libktx. Please review.

@wasimabbas-arm
Copy link
Contributor

@MarkCallow LGTM. Thank you for adding these.

@MarkCallow MarkCallow merged commit b619004 into main Dec 22, 2024
18 checks passed
@MarkCallow MarkCallow deleted the decode_astc_error_handling branch December 22, 2024 22:51
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

Successfully merging this pull request may close these issues.

3 participants