Conversation
Add limited support for input and output for the KTX2 format. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
|
Update: I'm working on integrating BCn decoders/encoders directly within libktx so that this PR gets significantly simpler (e.g., from 12_000 lines most of which are from dependencies to circa 2_000). |
*Significantly simplify KTX support by relying on libktx to decode/encode GPU-compressed formats such as BCn, ASTC, and ETC. Consequently, BCn and ETC dependencies are removed. All future required encoders/decoders should be implemented in libktx and not here to keep this is as simple, maintainable, and short as possible. *Remove ETC sub-dependency from local libktx build. *Update ktx README.md documentation to reflect that only libktx dependency is and should be used (i.e., no additional dependencies should be added). Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
|
Update2: significantly simplified the whole PR. Now only dependency is libktx (since I offloaded most work to libktx via a PR that will be probably merged soon). Can you please not approve workflows unless I explicitly request a workflow run? (to avoid filling the actions log with CIs that I know will certainly fail). |
*IntelLLVM is simply not supported by libktx. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Sure, in this case I can do so, but only because you are a a first-time contributor to this project, so admins have to approve workflow runs on the project's main account. Once you have any PR accepted and merged, the workflows will run automatically. Here is the general solution that you or others can do, in this situation and after you no longer need admin approval: Let's say you have pushed to your fork's branch "mypr" and submitted a PR to the project from that branch. (Aside: I strongly recommend working in topic branches, and never submitting a PR from your "main".) After submitting the PR, you realize you need to iterate (you want to add things, or CI is not passing and you need to make repairs, or you have review comments to address), but you don't want to do a series of many many pushes as you go, each of which will trigger a CI run on the main branch as well as sending email alerts to everybody "watching" this project, since pushing to your branch is technically a PR update. Instead, push to a different branch! This pushes your fork's "mypr" branch to your fork in a "test" branch. This will run the CI on your fork in your account (allowing you to see if CI passes for the changes you've made), but since your "test" branch is not associated with the PR, it will not send the rest of us alerts and will not trigger a CI run on the project's main repo account. Then when you finally have it right and are ready for the rest of us to see it, you can |
*Add to_native* conversion to KTX2 output similar to how JPEG and PNG OIIO outputs are written. *Apply clang-format-17 as opposed to the previously applied clang-format v18 which caused the CI to fail. *Update CMAKE_VERSION in CIs to minimum version required by libktx (i.e., 3.23). *Fix uninitialized structs passed to libktx. *Other misc clean-ups and minor refactoring. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
|
Question about older CMake version that is failing some CIs and icpx:
All CI issues are now solved. Will push updates here when sufficient testing is done. |
…actoring *KTX does not support compilation with Intel's C++ compiler ICX hence why an override option is provided and is used in ICX CI. *Do all decompression/decoding/deflation in open() rather than in seek_subimage() Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
*Due to missing 'alpha' support (which obviously exists in libktx), unit testing was failing (due to unexpected number of channels). *Misc cleanup; removal of ktxTexture* member field and only using ktxTexture2*. *Remove MSVC/GNU/Clang checks on libktx dependency in externalpackages. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
*Previously, a buffer allocated via a malloc by libktx was freed using RAII via std::unique_ptr's delete[] which doesn't match the allocator. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
*libktx > 4.3.2 requires CMake >= 3.22. The code changes required for this will also be applied accordingly (e.g., older libktx versions don't support direct BCn encoding/decoding). Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
*Previously, libktx version was hardcoded to v5.0.0 now it is based on the CMake-set variable Ktx_VERSION. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
*malloc has to be matched by a call to free not a call to delete. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
*Attempting to address old CMake version with 'oldest' CIs. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
*Point to not-yet-merged PR in libktx only for Intel-based MacOS runner. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
This is added so that CIs on older runners/compilers/cmake can pass. Newer version of libktx require CMake >= 3.22 which these runners cannot install. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
ktxBasisParams is vastly different starting from libktx >= 5.0.0. Use smart pointers to handle ktxTexture resources and clean them up. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
To be reverted once the linkage issue for ASTCENC is solved. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
* Removed because std::from_char on MacOS still doesn't support parsing of floating point values. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
* CIs with old gcc versions ("oldest" CIs) use an older version of CMake
than that is required by libktx 5.0.0. This commit attempts to disable
ktx plugin testing on such old configurations by setting ENABLE_KTX to
0.
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
* libktx v4.3.2 is removed from build_Ktx.cmake and, from now on, only libktx v5.0.0 or newer is supported. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
* Add initial documentation about ktx plugin and its exposed attributes * Add more extensive testing * Misc cleanups Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
|
PR is now ready for initial review. There are still some missing tests but at this point I will wait for initial review then complete missing tests as necessary (there are also a lot of introduced |
* libktx's ktxTexture_GetRowPitch is wrongly padding the actual correct computation to be a multiple of 4. This is only valid for KTXv1 but we only use/support KTXv2 and ktxTexture_GetRowPitch simply does not distinguish between the two. See issue: KhronosGroup/KTX-Software#1228 * Add HDR support for ktxinput alongside UASTC-HDR/uncompressed HDR testcases. * Add support for HDR ASTC vkFormats (ASTC_WxH_SFLOAT_BLOCK). * Clean-up metadata parsing (KTXScWriterParams was wrongly serialized as uint8 array rather than a string). Remove unnecessary/unused metadata. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
lgritz
left a comment
There was a problem hiding this comment.
I like the general gist of this and I would definitely like to get ktx support in as a 3.2 feature.
Took a preliminary pass over this and commented on an array of topics, not intended to be comprehensive, but enough for you to get started on the obvious things.
|
Thank you for the initial review. CIs are failing because I was tracking main from KTX-Software. A couple of days ago, libktx 5.0.0-rc2 was finally released which fixes tons of very relevant issues and we can finally stabilize on it. Some noteworthy notes before any further reviews:
|
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
* libktx now points to the recently released 5.0.0-rc2 tag with all the relevant fixes. * make use of OIIO's Strutils instead of own iequals and std::stof and the likes. * use Strutil::icontains instead of std::regex for literal strings. * add missing license header in ktx_pvt.h. * remove doxygen-style comments. * use span-based alternatives when possible (still not applicable to main OIIO write/read scanlines due to missing z parameter). * remove CMake >= 3.22 check. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
* KTXwriterScParams was previously copied as is from input KTX2 file to output KTX2 file's KVD. This is not ideal because each writer (in this case OIIO) should write its own set params. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
* Prior to this commit, metadata KVD keys were wrongly constructed via a string_view; the `keylen` includes the NUL termination char which should not be supplied to the string_view. * KTXorientation metadata is now parsed and mapped to the corresponding OIIO Orientation value. A test case with `--reorient` is included. * Cubemaps are now properly supported. Cubemap faces are exposed via the `subimage` parameter. * clang-format using latest clang-format so that the CI shuts up. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
* Using the attribute "ktx:1d" and given a height <= 1, a 1D texture can be written. * The attribute "ktx::supercompressionscheme" is now fixed to only store the strings "zstd" or "zip" (i.e., zlib). * Misc clean-up and dead code/comments removal. Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
* span-based methods (both for the reader and the writer) now hold the main implementations and the to-be-deprecated and less-safer point-based methods simply call span-based methods with inferred data buffer length. * add proper support for writing cubemap textures using write_tiles. * fix reading of cubemap textures to now use read_native_tiles instead of the previous erroneous each-slice-is-a-subimage approach. * fix reading of volume textures by 'simulatin' them as tile-based textures with tile size: WIDTHx1x1. This, however, weirdly causes slowdowns because of repetitive calls to read_native_volumetric_tiles (either OIIO poorly implements this or we are doing something wrong in this KTX2 plugin). Signed-off-by: Walid Chtioui <walid.chtioui.main@gmail.com>
Description
Fixes #2329. Add initial KTX2 format support (closest similar plugin/format in OIIO is DDS).
This KTX plugin support obviously nullifies the benefits of using KTX in the
first place. That being said, this plugin is still useful so that end users
don't have to convert back and forth between KTX <-> supported format (e.g., PNG).
It is also useful to convert to and from KTX2 format using OIIO.
An example usecase would be Blender and its glTf import/export plugin.
(see KhronosGroup/glTF-Blender-IO#1896).
Ideally, at some point in the future, OIIO may introduce a new API to accomodate
texture formats that are mainly used for fast texture uploads to GPUs.
For details on KTX2 and what this PR supports and what it doesn't (alongside a
description of current limitations), see
src/ktx.imageio/README.md(copied partsof it at the end of this).
Tests:
Added initial testsuite but only for the input (i.e., info_command) part of the plugin.
Ideally, after fixing CI issues, the tests will be adjusted for ktxouput.
Checklist:
=> Haven't used any AI coding assistant tools in any capacity whatsoever.
behavior.
PR, by pushing the changes to my fork and seeing that the automated CI
passed there.
fixed any problems reported by the clang-format CI test.
corresponding Python bindings. If altering ImageBufAlgo functions, I also
exposed the new functionality as oiiotool options.
=> No new API is introduced. This is just a plugin.
The sections below are copied from
src/ktx.imageio/README.md. These will be updatedas this PR progresses.
Supported Encoders/Decoders
Supported/Tested texture kinds:
SINGLE_TEXTURE_1DSINGLE_TEXTURE_2DSINGLE_TEXTURE_3DCUBEMAP_TEXTUREARRAY_TEXTURE_1DARRAY_TEXTURE_2DARRAY_TEXTURE_CUBEMAP[ ](not planned)ARRAY_TEXTURE_3DSupported/Tested raw VkFormats (decoder + encoder):
VK_FORMAT_R8_UNORMVK_FORMAT_R8G8_[SRGB|UNORM]VK_FORMAT_R8G8B8_[SRGB|UNORM]VK_FORMAT_R8G8B8A8_[SRGB|UNORM]VK_FORMAT_R16G16B16_SFLOATVK_FORMAT_ASTC_NxM_SRGB_BLOCK(all block dimensions)VK_FORMAT_ASTC_NxM_SFLOAT_BLOCK(all block dimensions)Block-compressed formats (decoder + encoder):
[ ] BCn (waiting on libktx BCn support PR merge here: Add BCn encoders/decoders with RDO support KhronosGroup/KTX-Software#1167)=> add BCn support in a separate PR whenever libktx 5.0.0 is officially released and my BCn PR is merged.Basis Universal schemes (encoder + decoder):
UASTCETC1SSupercompression schemes (decompressor + compressor):
ZLIBZSTDDependencies (only one - libktx)
libktx: for general KTX@ format support (loading of KTX2 files, transcoding
support, supercompression decompression support, etc.).
OpenImageIO/src/cmake/build_Ktx.cmakelib/etcdec.cxx's license (ETC is not used in PR).This PR should only be merged after the release of libktx 5.0.0.
Resources