Skip to content

Implement simple cache for decoded ASTC textures #1339

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

JoseEmilio-ARM
Copy link
Collaborator

Description

Introduce a simple caching mechanism for decoded ASTC textures on platforms that do not support the format. Before decoding, the system will check if there are existing binaries saved locally, and otherwise create them for future runs.

Fixes #1202

General Checklist:

Please ensure the following points are checked:

  • My code follows the coding style
  • I have reviewed file licenses
  • I have commented any added functions (in line with Doxygen)
  • I have commented any code that could be hard to understand
  • My changes do not add any new compiler warnings
  • My changes do not add any new validation layer errors or warnings
  • I have used existing framework/helper functions where possible
  • My changes do not add any regressions
  • I have tested every sample to ensure everything runs correctly
  • This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

  • I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

If this PR contains framework changes:

  • I did a full batch run using the batch command line argument to make sure all samples still work properly

Copy link
Contributor

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, to make your PR work with vkb::scene_graph::components::HPPImage, you would need to add the data_hash to that class as well.
Besides that, as the caching is an ASTC-specific thing, I would have expected that the hash lives in vkb::sg::ASTC.

With this PR, you calculate the hash for each and every image, and you calculate it based on the image data itself. Wouldn't it be more efficient, to just hash the URI of that file? That would probably require to just modify the vkb::GLTFLoader.

@JoseEmilio-ARM
Copy link
Collaborator Author

First of all, to make your PR work with vkb::scene_graph::components::HPPImage, you would need to add the data_hash to that class as well. Besides that, as the caching is an ASTC-specific thing, I would have expected that the hash lives in vkb::sg::ASTC.

With this PR, you calculate the hash for each and every image, and you calculate it based on the image data itself. Wouldn't it be more efficient, to just hash the URI of that file? That would probably require to just modify the vkb::GLTFLoader.

Thanks! Prefer to leave it as is, to support cases where the image itself changes, but not its URI.

@SaschaWillems
Copy link
Collaborator

This PR breaks batch mode for me. When reacing the hlsl_shaders sample (which worked fine last time I did batch with master) it now asserts in create_vk_image.

I'm also getting a lot of (silent?) exceptions when running samples that decode astc now:

image

Not sure if any of these is related, but can't approve this without being able to do a batch run.

@JoseEmilio-ARM
Copy link
Collaborator Author

This PR breaks batch mode for me. When reacing the hlsl_shaders sample (which worked fine last time I did batch with master) it now asserts in create_vk_image.

I can reproduce the issue, and found a workaround in my latest commit, but cannot really explain what is wrong, any ideas @asuessenbach ?

I'm also getting a lot of (silent?) exceptions when running samples that decode astc now:

These are probably the ones I introduced when a problem occurs trying to load an image from the cache. The first time the cache does not exist, so they will be thrown, but in later runs they should not be there anymore

@asuessenbach
Copy link
Contributor

Regarding all those exceptions: I don't think, it's the right approach to use exceptions for non-exceptional cases.
You could instead just use std::filesystem::exists() to check if the cached file exists.

And... having that data_hash in HPPImage as well, at the right position, made the hpp-based samples run correctly. But only because there's no hpp-based sample using an ASTC texture. If we get something like that, we'd need all those hash-related functions from Image there as well. So, would be great if you could add them.

@SaschaWillems
Copy link
Collaborator

Any chance of speeding this up? I'm currently doing large changes to the framework, and testing is extremely tedious as all those high-level framework samples use ASTC and every time it needs to decode them. That makes testing very slow.

@SaschaWillems SaschaWillems self-requested a review July 2, 2025 05:26
Copy link
Collaborator

@SaschaWillems SaschaWillems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏻

A minor suggestion: Add the astc cache folder to the .gitignore

Copy link
Contributor

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could delete the warning in gltf_loader.cpp, line 1489.

Besides that (and the comment below), it looks great now.

{
LOGW("ASTC cache file {} does not exist. ASTC image will be decoded", path.string())
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add some
LOGI("Loading ASTC cache data from file: {}", path.string());

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.

(Meta issue) Improve sample load times
5 participants