Skip to content

Conversation

@Daniil-SV
Copy link

I'm currently working on KTX support in OpenImageIO (OIIO further), and I've encountered some issues along the way.
Builds on some of my MacOS-based CIs were failing due to errors related to missing zstd headers. For some reason, the compiler simply couldn't find them.
I don't think the problem lies with OIIO, as dependencies are compiled in a separate process during cmake configuration stage.
After a little investigation, I concluded that dependencies are working a bit messy here. I suspect that in my case, the compiler was failing because some zstd headers were used directly from basisu while others contained the repository itself.
So I decided to try to "fix"/"rework" it.

Implementation

I decided to use FetchContent, because it is a modern approach to dealing with dependencies, and it gives quite a lot of opportunities for developers. Under the hood, it uses a git clone and all dependencies are collected in one centralized folder, so there shouldn't be such problems with dependency files. In my opinion, this also solves the dependency maintainability issue, you just need to change the git tag inside cmake file to update dependency version. Also, FetchContent calls find_package under the hood, which allows you to use system packages to speed up the build or use custom packages, which is I think will be very useful, at least for OIIO.

Testing

I tested this in OIIO CI and KTX-Software CI. The build works fine in both cases, but some ktxdiff tests fail. I checked each failing test and concluded that the updated zstd version (more details in the notes) changed the compressed data, causing the test to fail when comparing them, even though the actual pixel data is essentially the same. I updated the test files locally and all tests pass successfully on my Windows machine, so if necessary, I can send additional PR to the CTS repository.

Notes

  • I also have changes for Basisu, but I'm a bit confused by the fact that it uses a fork with custom changes. Currently, the fetch comes from my own fork here, based on the original repository, where I made dependency changes and changes based on KhronosGroup fork. But I'm not sure where I should submit a pull request: to the official repository or should i replicate my changes to the KhronosGroup fork.
  • I updated ZSTD from 1.4.9 to 1.5.7. When using FetchContent, it adds and configures the project as if it were a add_subdirectory, but 1.4.9 is too old, so using it causes CMake version conflicts.
  • I assume that FetchContent will replace the current implementation with subrepo for those dependencies for which I wrote a cmake file, but if this is not accepted, then I am ready to rewrite it at least as an option, just tell me.
  • The current version of Basisu has some warnings, so I disabled global werror for a successful build (most likely temporarily)

@CLAassistant
Copy link

CLAassistant commented Dec 18, 2025

CLA assistant check
All committers have signed the CLA.

@MarkCallow
Copy link
Collaborator

@Daniil-SV I am delighted to hear you are working on KTX support in OpenImageIO. Thank you for your efforts.

I am new to FetchContent so it may take me a while to fully understand the proposed changes. Please bear with me.

@kring (primarily) and @dg0yt are also working on and have interest in improving the dependency handling, in their case for vcpkg. Via this comment, as I can find no way to officially add them as reviewers, I invite them to review this PR. I need to ensure it contains nothing that conflicts with what is needed for vcpkg or will make that work harder.

In a quick skim over the changes I noticed that it no longer adds dependent libraries' objects into a static libktx. Why did you make this change? Making a single library is intended to simplify linking for those not using CMake though I have no idea how large is the group of non-CMake users.

The golden files for the failing zstd tests should be ideally be updated using GCC. Use of ktxdiff on these tests is because we have previously encountered non-deterministic behaviour across compilers or platforms. Using a particular compiler helps limit the changes in the golden files to those warranted by changes in the code generating them.

Addressing your notes in the same order.

  • We use a custom fork of basis_universal because (a) we wanted to use it as a subproject and felt such use might not work with the original because it uses the CMAKE_{C,CXX} global variables to set compile options and (b) the original offers no way to stop it building everything and we only want to build the basisu_encoder target. The fork also has fixes for reuse needed because we use it as a subproject. I have submitted a PR with the changes to the upstream basis_universal repo and I am hopeful it will be accepted in January.
  • I don't see a problem with using ZSTD 1.5.7. The reason for using the one included in basis_universal is to save space. It seems unnecessary to download an install ZSTD when we already have it in the source.
  • I'll get back to you on this when I understand FetchContent better.
  • I suspect the warnings are coming from the basis_universal application builds which, as I mentioned, can't be disabled in their current CMakeLists.txt. Binomial accepted and have merged all the fixes I provided for the warnings in basisu_encoder that aren't ignored either by options set in CMakeLists.txt or by pragmas in the code. basisu_encoder builds for me without warnings on Android (GCC) iOS (Clang), Linux (GCC), macOS (Clang), MinGW (GCC) and Windows (Clang and MSVC). I will not merge a PR that turns off KTX_WERROR. Without it, the number if warnings builds rapidly usually without being noticed as they are always on "some other platform".

@Daniil-SV
Copy link
Author

In a quick skim over the changes I noticed that it no longer adds dependent libraries' objects into a static libktx. Why did you make this change?

Right, I forgot about that, thanks for pointing it out. I originally removed it because I was getting a lot of warnings due to this optimization, but I think I can now revert it back and implement it without errors.

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