Add BCn encoders/decoders with RDO support

[walcht]

As discussed in #1159: ktxTexture2_CompressBCn and ktxTexture2_DecodeBCn are introduced in this PR to allow libktx users/consumers to encode/decode BCn textures from/to raw decompressed formats.

https://github.com/richgel999/bc7enc_rdo does not support BC6HU/BC6HS encoding/decoding and also no BC2 (this format is essentially dead since BC3 replaces it).

Please feel free to give feedback, edit, and nitpick as much as possible.

Some context: I am adding KTX2 support to OIIO (PR: AcademySoftwareFoundation/OpenImageIO#5185) and having libktx encode/decode BCn formats significantly simplifies things (also ETC encoding/decoding which I can also open a PR for - if approved).

I haven't updated the KTX-Software-CTS with the added BCn test files.

Once this is finalized, this will fix #587.

Current TODOs:

• add CTS test files from a primary platform. As far as I understand, I shouldn't create/transcode KTX2 test files on non-primary platfroms (non arm64). I did do the testing on my local machine (see https://github.com/walcht/KTX-Software-CTS). => added CTS tests + golden files (still not on a primary platform).
• add RDO post processing step
  • expose RDO parameters with detailed description (some parameters are still missing - I haven't understood them yet)
  • BC1 mode RDO
  • [x] BC2 mode RDO BC2 is rarely used (I don't see any reason why to use it instead of BC3). Postponed or not planned to be implemented at all.
  • BC3 mode RDO
  • BC4 mode RDO
  • BC5 mode RDO
  • BC6H mode RDO (HDR). I don't know if this is possible. If not, then I will not adjust the ert::reduce_entropy function for the moment. postponed (too much work/overload to include in this PR).
  • BC7 mode RDO
  • ultra smooth mode (see: https://richg42.blogspot.com/2021/02/updated-bc7encrdo-with-improved-smooth.html). => trivial to implement, just have to remove dependency on image_u8
• add multithreading for encoder part (up to 9X decrease in time for my 6-core system)
• add multithreading for RDO (bc7enc_rdo makes use of OpenMP so we need to do our own MT same as in encoder - this is trivial) => 5.50X to 7.00X time decrease compared to single-threaded mode.
• add BCn encoding support for ktx encode command
• add BCn decoding to PNG support for ktx extract command
• add BCn encoding support for ktx create command
• agree on which parameters to expose in ktxBCnParams struct (apparently there are many and I am not qualified to know which subset to expose or to expose them all). For the moment I will just expose them all.
  We agreed on using same parameters as UASTC RDO and adding additional ones if they make sense (I haven't benchmarked skip 0 MSE error option so I might remove it if it useless).
• fix bc7enc_rdo compiler warnings (very simple to do, before merging git diff with original files so that we are 100% that we haven't changed anything).
• finalize test cases (not CLI tests but rather libktx tests - e.g., texturetests, etc.).
• enable SIMD acceleration for BC7 encoder (using ISPC - see https://github.com/ispc/ispc) (this should be straightforward and should be enabled by default via a CMake flag; e.g., BC7_SIMD). => since we are planning to use bc7f we will be planning to use bc7g once it is released (this is the SIMD equivalent of bc7f).
• test LIBKTX_FEATURE_BCN_DECODER CMake flag option
• use bc7f for BC7 encoding instead of bc7enc_rdo's encoder (significantly faster + maintained).
• add single image decoders used by GLUpload/VkUpload (should be exposed in the library API but must be independent of the ktxTexture* classes. Software reading a KTX file incrementally will find them useful).
• add BC6H decoder (copied from MIT Licensed https://github.com/iOrange/bcdec from basisu)
• add BC6H encoder from basist::astc_6x6_hdr::fast_encode_bc6h
  • fix issues with signed formats (same way that UASTC HDR handles it) => if we are given a signed input which the encoder can't handle; asserts fail, etc. (obviously, since BC6HU is the only one supported). Need to do some pre-processing step clamp signed and above-maximal-encodable value...
• add BC2 decoder (unpack BC1 followed by unpack sharp alpha).
• [x] add BC2 encoder postponed/aborted (rarely used).
• when encoding to BC5, we must support --normal-mode so a 3 component normal image can be used as input.
• maybe we should support it when encoding to any BCn format with a real alpha channel?
• update BCn Encoding and Decoding in KTX. Currently it just showcases RDO images which were generated using an erroneous old commit.
• RDO with UNORMS? => RDO expects color data but it could be that what is saved in a UNORM is color data so we leave it as it. Added one liner note in ktx tools for --bcn-rdo param.
• external dependencies should be managed via a subrepo
• add usage documentation example akin to that of ASTC (should be in C)
• add CTS golden files clean-up script or instructions (throughout this PR, tons of golden files were initially added but are no longer needed; these should be cleaned up before merging).
• add python bindings and docs
• add java bindings and docs
• add javascript/WASM bindings and docs

Note1: no LLMs/AI coding tools were used in any capacity whatsoever in writing or aiding in the writing of this PR.
Note2: I am an individual contributor (main reason I am contributing here is to add support for KTX2 in Blender).

Edit: TODO list edits

[walcht]

There are also a lot of compiler warnings from bc7enc_rdo dependency. These should be straightforward to address directly in copied files from bc7enc_rdo.

[CLAassistant]

All committers have signed the CLA.

[MarkCallow]

@walcht, Thank you for this. Can you view the build logs?

One issue I notice immediately is that there are no changes to command_create.cpp. This too needs updating to support BCn encoding.

Regarding RDO and multi-threading, the UASTC encoder also has separate multi-threading options for the encoder and for the RDO step. You can follow the same model.

If we need to fix warnings in the encoder I suggest forking the encoder then incorporating the fork here by way of git subrepo. That will make it easier to potentially contribute fixes back upstream. If you are agreeable I can make the fork and provide instructions for how to incorporate it in your workarea.

Re. SIMD and ISPC, be careful how you support this. We need to support building and running on arm64 processors. Also compile flags to enable SSE or other SIMD options are not compatible with straightforward use of universal build tool chains, which is why this project does not do universal builds. Better is use of compiler pre-defined macros and run-time queries to discover what the software is being compiled for and running on. However since we aren't doing universal builds there is no need to obsess over this last detail.

[walcht]

@MarkCallow - Concerning the command_create.cpp, I am adding BCn encoding for it currently (somehow forgot it) - will also check if I have missed any other commands.

Concerning the build/CI logs: they are mostly failing because of bc7enc_rdo compiler warnings which should be suppressed. They will also fail because I haven't re-generated the golden files yet for ktx CTS (e.g., ktx encode --help output).

If you are agreeable I can make the fork and provide instructions for how to incorporate it in your workarea

Please do so (as far as I understood, this will be forked under the KhronosGroup and any updates here will be pushed there via the git subrepo command). Until that is done, I will keep edit the bc7enc_rdo sub-folder until the CI passes.

Concerning SIMD and ISPC: I think it makes sense to leave this for another PR, do you agree? (reasoning is this: I have to get the basics working properly, add proper testsuite that covers all supported BCn formats, etc. Once that is done, I can open another PR for SIMD performance improvements). Or I will leave this to very end (last in TODO list above).

[walcht]

Updates about CI jobs:

• reuse lint: I don't know why it is not ignoring external/bc7enc_rdo but I have added the SPDX comments in these files.
• MingW CI: failing because of some linkage error with bc7enc_rdo (undefined reference to rgbx::). I have to test this on my Windows machine to see what I am doing wrong/missing.
• MacOS (arm64) compiling bc7enc_rdo fails. I have no idea why (I don't have a MacOS machine so I will be testing this CI on my fork).
• Windows CI: probably some NSIS issue (I don't think I have introduced anything to make it fail)
• Other failures are still due to unused-variables/functions that I don't get locally (have addressed all that were mentioned in the CI logs).

[MarkCallow]

@walcht,

You will need to rebase to or merge current main to get the fix for the NSIS issue on Windows.

I have created a fork of https://github.com/richgel999/bc7enc_rdo. To incorporate it in your add-BCn-decoder branch do the following in the repo root directory:

git subrepo clone https://github.com/KhronosGroup/bc7enc_rdo.git external/bc7enc_rdo -b changes_for_ktx

You can make changes in this subdirectory, as you are now, and when everything is working I can push the changes to the fork.

Concerning SIMD and ISPC: I think it makes sense to leave this for another PR

I agree. I wanted to make you aware that arm64 is a build target.

Re. reuse, you have to add an entry to REUSE.toml to get external/bc7enc_rdo ignored. It is better to do that than add SPDX comments to all the files. The entry in REUSE.toml will have to mention a license. Use the MIT license option.

Here are a few high level points.

• I want to use the bc7f encoder once it is integrated into the bc7enc_rdo repo.
• Re. decoding, currently the ETC and ASTC decoders have different APIs. The former has a non-public unpack function for single blocks. This is called from the GL and Vulkan upload functions, if the device does not support ETC, to convert the data before it is uploaded. The latter has a call that decodes and converts an entire ktxTexture2 object. I want to have both for all decoders: ASTC, BCn and ETC and the transcoders as both have their uses. It was a mistake not to have provided an unpack function and updated the upload functions when the ASTC decoder was added.
• I would like BC6H support to be included in this or a later PR.
• I will be away for 5 days from May 15th and not be able to approve workflow runs during that time. Please plan accordingly.

[MarkCallow]

Re the macOS build failure, because the output from the Xcode build is so voluminous it is run through a script, xcpretty, to prettify it. On a past CI service, without this, the logs exceeded the maximum allowed. Recently, for reasons I have yet to investigate, it has started swallowing compile errors. You can turn it off by editing scripts/install_macos.sh and commenting out the line that installs xcpretty.

[walcht]

@MarkCallow

I want to use the bc7f encoder once it is integrated into the bc7enc_rdo repo.

Agree. I wasn't initially aware of this. Apparently bc7f is significantly faster that the bc7 encoder used here (also the added benefit of being continuously maintained). I will integrate this right now since this seems to be straightforward (bc7enc_rdo also seems a bit not-longer-maintained so l think it's better to just integrate it now rather than waiting for it to be integrated into bc7enc_rdo repo).

Concerning the decoding API: I added BCn decoders for VkUpload/GLUpload as a TODO (will follow same API as in etcunpack). Might also open a PR to add it for ASTC since I have already spent some time getting familiar with this code base.

I would like BC6H support to be included in this or a later PR.

Will add it in this PR. Will add it at the very end though since I have to finalize current formats.

I will be away for 5 days ...

I will try to address CI issues now (It is fine if I incrementally commit here to see if certain jobs pass?).

[walcht]

The integrated basisu_transcoder is a bit outdated (actually, significantly) and doesn't contain bc7f. So for the moment I will just use bc7enc_rdo's BC7 encoder until the fork at (https://github.com/KhronosGroup/basis_universal/tree/fixes_for_ktx_v5_0) is updated to a commit that includes the bc7f namespace.

[MarkCallow]

I was not aware that bc7f is included in basisu_transcoder. By pure coincidence I have just completed integration of Basis Universal release 2.1.0. See the update_basisu_to_2_1_0 branch. We have the v5.0.0 release in flight. I've made a v5.0.0-rc1 release while we wait for some external (to KTX-Software) pieces to be put in place. My plan was to wait until we made the v5.0.0 release before merging this branch. If you retarget this PR and your branch to update_basisu_to_2_1_0 you can start working on bc7f now.

Neither the KTX-Software code nor our golden files required any updates for our extensive test suite to pass with BU 2.1.0. I am therefore amenable to merging it now but will have to discuss within the Khronos WG and can't make any promises.

The single image decoders used by GLUpload/VkUpload should be exposed in the library API but must be independent of the ktxTexture* classes. Software reading a KTX file incrementally will find them useful.

Regarding the current ETC decoder, please note that it does not have a recognized open source license so might not be suitable for your OIIO work.

[walcht]

@MarkCallow - please don't approve the CI workflow yet (it will fail because I haven't updated the golden test files yet).

• I have added initial RDO postprocessing step support for BC1, BC3, and BC7 (only relying on ert.h/cpp without having to use bc7enc_rdo utils.h/.cpp and rdo_bc_encoder.h/cpp because these used OpenMP and it's just better that we only depend on the actual core RDO function in ert.h/cpp).
• Also fixed the encoder in case input texture is not multiple of 4 (still have to absolutely verify this in all cases - this was reading beyond std::vector's size before this commit...).
• No ultrasmooth block support yet (added to TODO list).
• Added initial RDO params for BCnParams in ktx.h (some detailed description for fields I understand).

Will add bc7f once I finish RDO post processing.

[MarkCallow]

I have just merged PR #1170 so you will now find bc7f in main. Please remove external/bc7enc_rdo and adapt this to bc7f. I hope that not too much of your work to date will have been wasted.

One other thing re. bcn_codec.cpp, we need to be able to build with just the decoder when building libktx_read. astc_codec.cpp is one file because of pieces needed by both encoder and decoder so it has ifdefs to allow building only the decoder parts. Unless there is substantial common code you can consider making separate source files for encode and decode. If you do not, then add similar ifdefs.

[walcht]

Update on this:

Before I forget, I want to fix UASTC multi-threaded RDO in the same way you fixed the bc7 RDO. Have you done that yet? If not, it's fine to later. It should be a separate PR from this.

I spent some hours understanding the RDO code and apparently I was initially wrong about this being fixed and I think there is just no way to make multi-threaded RDO deterministic (i.e., running ST vs. MT yields same results).
The fix (which I figured right now that is not actually a fix) just happened to work on the input test images which were very simple, 16x16, grayscale images. That fix just made it less likely for non-determinism to occur but definitely doesn't solve the problem with actual, real-world images.

Why? Because running RDO on the whole image (single-threaded mode) means that each block N depends on previously RDO'ed M blocks (with M influenced by the window/dict size). In MT mode, image is divided into regions (one region per thread) and RDO is ran on these separately.

I'm currently updating my guide, the no-multithreading parameter description, and a bunch of other things to reflect this.

I also see that a lot of improvements can be done on the RDO function but I will postpone these to after when this PR gets merged.

If you want, I can open a PR to add a comment and documentation about this with UASTC RDO (or any other RDO that is used).

[MarkCallow]

Thank you for your study.

Why? Because running RDO on the whole image (single-threaded mode) means that each block N depends on previously RDO'ed M blocks (with M influenced by the window/dict size). In MT mode, image is divided into regions (one region per thread) and RDO is ran on these separately.

The problem we originally observed was that the MT results were non-deterministic between runs. It wasn't investigated; there is still an open issue on the basis_universal repo. Do you think with the changes you have made MT will at least be deterministic between runs?

We can certainly live with ST and MT giving different results provided we explain it in the docs. An obvious question is which produces the result with the least entropy (i.e. which will deflate best with zstd): ST or MT?

Is the dictionary shared between the threads and would that affect the result.

I'm currently updating my guide, the no-multithreading parameter description, and a bunch of other things to reflect this.

I also see that a lot of improvements can be done on the RDO function but I will postpone these to after when this PR gets merged.

Sounds good. When you have time, please give me a summary of the possible improvements.

If you want, I can open a PR to add a comment and documentation about this with UASTC RDO (or any other RDO that is used).

That will be terrific. I'd like to get this in for v5.0.0. It would have to cover BasisLZ, UASTC LDR 4x4 and UASTC HDR 6x6 Intermediate.

[walcht]

Make this an active cross reference to the documentation for the struct.

I just couldn't figure out how to make Doxygen generate a reference inside a C-code comment that is isn't supposed to consume...

[MarkCallow]

Make this an active cross reference to the documentation for the struct.

I just couldn't figure out how to make Doxygen generate a reference inside a C-code comment that is isn't supposed to consume...

This is a markdown page being processed by Doxygen. According to https://www.doxygen.nl/manual/markdown.html to link a documented entity you can use

[The link text](@ref MyClass)

[MarkCallow]

Regarding --normal_mode, one of my Khronos colleagues has said "If anything, BC3 normal maps that use grayscale-alpha encoding should be a thing of a distant past. There's no reason to use those over BC5." Therefore I suggest that --normal_mode should only support BC5.

[walcht]

It is determined by the transfer function in the DFD of the incoming ktx2 file, isn't it? Color model is something different.

I changed it to this:

UNORM vs. SRGB is determined depending on the original BCn VkFormat.

I determine whether the decompressed format is SRGB vs. UNORM depending on the set BCn VkFormat (I don't look at the DFD since I assume if BCn is set to SRGB then so is the DFD's TF => otherwise an invalid KTX2 file, right?).

In normal use a library should not print messages to stderr or stdout because it has no idea if the application is GUI or command line and no idea how the application is using those file descriptors. Use for debugging is fine.

It would be acceptable if we can find a way to provide the warning to the application to let it handle it. Perhaps a return code that says the encoder generated warnings and a query to retrieve the warning message or messages.

I think it makes sense to do this in a separate PR after this gets merged, do you agree?

We should follow typical usage. Allow/require --skip-kvd before the first none option argument. Also print usage for any option arguments that are not skip-kvd. Currently this is assuming a fifth argument is skip_kvd.

I used cxxopts in ktxdiff. It's just better that having to manually parse the CLI arguments/options.

Still a couple of comments that I haven't addressed yet.

CIs will probably fail because of cxxopts dependency setup. Since tests are always added, I will always add it.

[walcht]

We must investigate why. It is intended to be supported but the tolerance is given as a percentage of the minimum of the two values being compared. Could this also be the input being "cleaned" before encoding? Please send me an example and I will investigate.

The two files that causes ktxdiff to fail are:
rgb16_sfloat_files.zip

And this is the output I get:

ktxdiff: Mismatching image data (diff: 6.4373016e-06): level 0, face 0, layer 0, depth 0, pixel 196, component 0 between
          Expected: /home/walcht/KTX-Software/tests/resources/ktx2/rgb16_sfloat_input_for_bc6hu.ktx2 and
          Received: build/Testing/Temporary/GeneratedFilesOnFailure/decoded_rgb16_sfloat_bc6hu.ktx2

The problem is obvious when looking at this code sample in compareSFloat16:

        const auto diff = std::abs(lhs[i] - rhs[i]);
        const auto absMin = std::min(std::abs(lhs[1]), std::abs(rhs[1]));
        if (diff > tolerance * absMin)
            return CompareResult{false, diff, i, i * element_size};

If lhs[i] or rhs[i] is 0, then the if statement will always enter regardless of how high the tolerance is set to. A quick fix is to add an epsilon but a whole different approach to computing the diff for unnormalized values is probably better.

There is also a mistake here at compareSFloat32 (1 should be i):

        const auto absMin = std::min(std::abs(lhs[1]), std::abs(rhs[1]));

Should I fix it here or in another issue + PR?

[walcht]

All review comments are addressed (and some more docs issues are fixed)

The only remaining thing:

[...] There's no reason to use those over BC5." Therefore I suggest that --normal_mode should only support BC5.

So, if I understood correctly and since BC5 encoder (and all other BCn encoders) don't have a normal-map-specific parameters (such as angular error), this will only disallow RDO (i.e., will generate warning when supplied with --bcn-rdo), right?

[MarkCallow]

Starting to review the latest changes now. Sorry for the delay. I've been distracted with other work. As you may have seen I made a v5.0.0-rc2 with all your wonderful fixes.

The only remaining thing:

[...] There's no reason to use those over BC5." Therefore I suggest that --normal_mode should only support BC5.

So, if I understood correctly and since BC5 encoder (and all other BCn encoders) don't have a normal-map-specific parameters (such as angular error), this will only disallow RDO (i.e., will generate warning when supplied with --bcn-rdo), right?

--normal-mode most importantly creates a 2-component normal map. Adjusting encoder parameters is secondary. Yes it is likely a good idea to disable RDO as the "distortion" it uses is based about perception of colors. It would be nice to run some tests though to see if the effects are truly detrimental to normals since RDO can offer a large reduction in the deflated (LZ-compressed) size of the texture. Unfortunately I don't have any tests.

[MarkCallow]

I still have to re-review the stuff in tools/ktx and texturetests.cc. I decided to post my comments so far from the re-review, as I've accumulated a reasonable number, mostly straightforward. This way, you can get started addressing them now, if you have the time.

[MarkCallow]

This completes my re-review after the latest changes.

[MarkCallow]

What is the plan for BC2?

[walcht]

First set of addressed review comments:

What is the plan for BC2?

I want to implement BC2 at the very end when this PR is finished. The BC2 encoder is trivial. I just have to call BC1 on RGB and encode alpha sharply (i.e., into a fixed set of bits).

If you do not use the presets, what is the value range you can use?

The range depends on whether the target is BC1/BC3 or BC7. If BC1/BC3 then the range is (if I remember correctly) [0, 19]. If BC7 then there is no range and the presets map to a set of OR'ed flags. That is the reason why I kept them separately initially. I can revert this or I can document the range + set of OR'ed flags that can be set (this is what I have done in latest commit).

Make indent 4 spaces here and after next if.

I can add a PR that adds CI clang-format (per subdirectory) so that at least ktx tools is consistently formatted.

Why? Is there a problem with imageio's half_to_float? I'd prefer to fix that, if there is.

There is probably a problem with nan/inf values. I need to look into the details but adding those nan/inf checks definitely made some tests pass (that previously didn't).

Did you investigate the "perceptual" parameter?

There are no perceptual (or the like) parameters for BCn encoders.

[MarkCallow]

If you do not use the presets, what is the value range you can use?

The range depends on whether the target is BC1/BC3 or BC7. If BC1/BC3 then the range is (if I remember correctly) [0, 19]. If BC7 then there is no range and the presets map to a set of OR'ed flags. That is the reason why I kept them separately initially. I can revert this or I can document the range + set of OR'ed flags that can be set (this is what I have done in latest commit).

I had not realized BC7 was a set of OR'ed flags. The original version you created defined the enumerand values as numbers like you are now doing in the documentation. Leave the code as is except force ktx_pack_bcn_quality_levels_e to be unsigned (assign one of the enumerands as, e.g. OU). Are the flags exposed in a header visible to a KTX application? If not remove all mention of the flags. If so, document the flags. If you wanted to use the flags you could probably do something gruesome like

    params.bcnCompressionQuality = flagA | flagC; // May need to cast to uint32_t.

Is it really useful to have such fine control over the quality or are the six settings we have sufficient?

Make indent 4 spaces here and after next if.

I can add a PR that adds CI clang-format (per subdirectory) so that at least ktx tools is consistently formatted.

Let's wait on that until after this PR is done and merged.

Why? Is there a problem with imageio's half_to_float? I'd prefer to fix that, if there is.

There is probably a problem with nan/inf values. I need to look into the details but adding those nan/inf checks definitely made some tests pass (that previously didn't).

I see you reverted back to imageio's half_to_float. Is there no need to fix anything in nan/inf related in that code?

Did you investigate the "perceptual" parameter?

There are no perceptual (or the like) parameters for BCn encoders.

For BC7 in the basis_universal repo bc7enc_compress_block_params has an m_perceptual field but I am struggling to find anywhere that sets this from the m_perceptual field of basis_compressor_params.

[walcht]

Are the flags exposed in a header visible to a KTX application?

They are not exposed to libktx users (i.e.,, public header). I removed any mention of these.

Is it really useful to have such fine control over the quality or are the six settings we have sufficient?

From my limited experimentation, the six settings we have are sufficient.

I see you reverted back to imageio's half_to_float. Is there no need to fix anything in nan/inf related in that code?

I think imageio's version is fine. I thought it had problems with returning nan/inf values but apparently there is no issue with it (all CIs pass).

For BC7 in the basis_universal repo bc7enc_compress_block_params has an m_perceptual field but I am struggling to find anywhere that sets this from the m_perceptual field of basis_compressor_params.

These BC7 functions are not used by this PR (and are probably something else entirely). The bc7f encoder we are using is implemented in the file: basisu_transcoder and there is no mention of anything perceptual in any parameters. The only configuration option these functions take is a flags parameter and there is no mention of perceptual (or any other metric) in these flags.

Windows CI will pass if re-run (probably).

[MarkCallow]

Recent changes all look good.

This looks ready except for the BC2 encoder and resolving the handling of perceptual in bc7f. Regarding the former,
do you want to wait until this is merged before adding the BC2 encoder? I am still waiting on the blocker for the v5.0.0 release before which I won't merge this. Regarding the latter, I want to review the code in basisu_transcoder and also look at basis_universal 2.5 which I think has bc7g.

I reran the Windows builds. There is still a test failure: texturetest.ktxTexture2_BCnEncodeDecodeTestRGB16_SFLOAT.encode_rgb16_sfloat_to_bc6hu_then_decode.