WIP: Encode astc support #810

wasimabbas-arm · 2023-12-01T17:04:41Z

This PR has:

Commit to move astc options out of command_create.cpp into astc_utils.h
Commit to add options for encode-astc tool
Minor fixes into build_macos.sh file

ci_scripts/build_macos.sh

MarkCallow · 2023-12-02T09:00:44Z

tools/ktx/command_encode_astc.cpp

+
+    if (texture->supercompressionScheme != KTX_SS_NONE)
+        fatal(rc::INVALID_FILE, "Cannot encode KTX2 file with {} supercompression.",
+            toString(ktxSupercmpScheme(texture->supercompressionScheme)));


This is too blunt a hammer. Only BasisLZ supercompressed images should be rejected. There is no reason to reject uncompressed images supercompressed with zstd or zlib.

And as is being done by the switch below this comment UASTC compressed images and any *_BLOCK VkFormats should be rejected as any such encode-decode sequence will add noise plus we don't have decoders for many of the formats.

I must say I haven't started looking into these options in detail yet. As you might know this is a straight copy from command_encode.cpp. Does this option not make sense there

KTX-Software/tools/ktx/command_encode.cpp

Line 156 in 97feeae

if (texture->supercompressionScheme != KTX_SS_NONE)

either?

Also glad your reviewed this, does it mean the other checks in executeEncodeAstc makes sense?

Assuming this also requires an inflat from whatever supercompression is provided before it can be sent to the encoder? At least astc encoder doesn't support suprecompressed input.

Inflate is done automatically by libktx when a ktxTexture2 is created from a file.

command_encode.cpp should be refusing to do anything with a file already supercompressed with BasisLZ. For other supercompression schemes it should open the file to check the format and refuse if it is UASTC, any block-compressed GPU format or if it is not one of the small number of suitable input formats. The last 2 checks apply too when the input is not supercompressed.

Inflate is done automatically by libktx when a ktxTexture2 is created from a file.

Let me get that right. If I load a ktx2 file in encode-astc that was supercompressed. It will be inflated but texture->supercompressionScheme will still tell me what supercompression it was?

I think I am getting confused again with the "supercompression" types we are referring to in here.
The only things that talk about "Supercompress" in the help doc for create and encode is in relation to zstd and zlib so I don't know what does the following mean?

a file already supercompressed with BasisLZ

and

For other supercompression schemes it should open the file to check the format and refuse if it is UASTC

Let me get that right. If I load a ktx2 file in encode-astc that was supercompressed. It will be inflated but texture->supercompressionScheme will still tell me what supercompression it was?

Yes. When the data is loaded it is inflated. The supercompressionScheme will have been changed to KTX_SS_NONE. To check the original scheme value you need to load the texture without setting KTX_TEXTURE_CREATE_LOAD_IMAGE_DATA_BIT in the createFlags. When you eventually load the data it will be inflated. For this case you can just load the texture with the flag set and afterwards check the vkFormat field to see if it is something you can deal with.

I think I am getting confused again with the "supercompression" types we are referring to in here.

There are 3 types, zstd, zlib and basislz. The last combines encoding to ETC1S with a tailored supercompression scheme.

tools/ktx/command_encode_astc.cpp

tools/ktx/command_create.cpp

MarkCallow · 2023-12-04T03:37:54Z

tools/ktx/astc_utils.h

@@ -8,6 +8,34 @@

 namespace ktx {

+/**
+//! [command options_astc]
+<dl>


I'm not sure the <dl> should be here. Is it in the other snippets?

In encode_utils.h it has, and from the source c4cc963#diff-8ca48860d1bd768774f38f0f522ba91ef7dbb887e660917c74b7088da850752aL604 where I brought this from, it had it too.

Okay. When this PR is further along we can build the docs and check what it looks like. I'll leave this conversation open to remind us.

aqnuep · 2023-12-04T09:06:46Z

@MarkCallow, @lexaknyazev, I believe that the consensus was that ktx encode is not supposed to deal with GPU BC compression, but only with actual supercompressions. I know this PR proposes to introduce ASTC encoding as a separate ktx encode-astc subcommand, but I think we would better off with a dedicated BC compression command, or something.

In any case, I think there should be some design work internally before committing to supporting a new subcommand in the new tool, and any new subcommand will have to come with a comprehensive, dedicated test set like the one we have for all other new CLI tools.

MarkCallow · 2023-12-04T09:28:31Z

I agree with the consensus that encode is limited to BasisU. I have no issue with changing the name of this tool from encode-astc. I have not been able to think of a good name so am open to suggestions. bc doesn't feel descriptive enough and block-compress feels too long for frequent typing. bcompress? blockcmp? Also I suspect many users will not be familiar with the meaning of "block compression".

This PR is a way for @wasimabbas-arm to share his design for review and discussion. That is one reason it is labelled WIP. Since it uses the same CLI options as ASTC compression in ktx create not much discussion about functionality is needed. Absolutely it must and will have CTS tests.

aqnuep · 2023-12-04T10:01:18Z

Thanks for the clarifications, @MarkCallow. This just caught me by surprise as I accidentally stumbled upon it.

I don't have a good name for this either, but since this changes the underlying vkFormat of the image, I'm tempted to say it should be named something like convert or reformat, but the former is too broad, and the latter is also weird.

@lexaknyazev seems to have a vision for the long-term structure of the new CLI tools, so I would really interested in his suggestions.

wasimabbas-arm · 2023-12-04T10:14:31Z

@aqnuep

I believe that the consensus was that ktx encode is not supposed to deal with GPU BC compression, but only with actual supercompressions.

Not sure I understand this fully. First of all I assume BC here means block compressed in general and not the BCx flavours of block compressed texture formats. Thats simple. But aren't BasisLZ/UASC block compressed formats? Or just not sure what you mean by "deal with".

I don't have a good name for this either, but since this changes the underlying vkFormat of the image, I'm tempted to say it should be named something like convert or reformat, but the former is too broad, and the latter is also weird.

I think its not just the change of vkFormat that is important from this tools' point of view but that it compresses the image with astc encoding.

What's the issue with calling this encode-astc if we are happy to have multiple tools? IIRC we discussed probably better to have encode-astc, encode-basis and encode-anything-else comes in the future. Are you proposing to have everything in encode itself?

Design wise, it does need some work because there will be duplication but not much else work is required. What do you have in mind.

MarkCallow · 2023-12-04T12:17:01Z

First of all I assume BC here means block compressed in general

Yes.

if we are happy to have multiple tools?

Shortly after I wrote my response to @aqnuep I realized supporting all block compression types in a single tool is likely not a good idea. It may be okay of we're only going to support ASTC and BC7 but any more and each should probably have its own tool.

@lexaknyazev seems to have a vision for the long-term structure of the new CLI tools, so I would really interested in his suggestions.

+1.

MarkCallow · 2023-12-04T12:22:08Z

But aren't BasisLZ/UASC block compressed formats? Or just not sure what you mean by "deal with".

Yes, they are. But they aren't formats known to the GPU. Therefore the file's vkFormat field is set to VK_FORMAT_UNDEFINED. For me that is the key difference and what makes folding encoding/compression of both into one tool problematic.

…rics

MarkCallow · 2023-12-05T02:26:09Z

lib/astc_encode.cpp

@@ -360,7 +360,7 @@ astcSwizzle(const ktxAstcParams &params) {

 static void
 astcBlockDimensions(ktx_uint32_t block_size,
-               uint32_t& block_x, uint32_t& block_y, uint32_t& block_z) {
+					uint32_t& block_x, uint32_t& block_y, uint32_t& block_z) {


I think we have another tabs v. spaces thing here.

Fixed in a925f3f

MarkCallow · 2023-12-05T02:28:24Z

lib/astc_encode.cpp

@@ -391,6 +391,87 @@ astcBlockDimensions(ktx_uint32_t block_size,
    }
 }

+static void
+astcBlockDimensions(VkFormat format,
+					uint32_t &x, uint32_t &y, uint32_t &z) noexcept {


Fixed in a925f3f

MarkCallow · 2023-12-05T02:44:08Z

lib/astc_encode.cpp

+    }
+
+    if (!This->pData) {
+        if (ktxTexture_isActiveStream((ktxTexture*)This)) {


I think you can just call ktxTexture2_LoadImageData within the if (!This->pData) block. If there is no stream it will return KTX_INVALID_OPERATION. If the result is != KTX_SUCCESS then destroy the prototype and return the result.

MarkCallow · 2023-12-05T02:47:50Z

lib/astc_encode.cpp

+
+    // if(params->perceptual) flags |= ASTCENC_FLG_USE_PERCEPTUAL;
+
+	uint32_t threadCount{1}; // Decompression isn't the bottlneck and only used when checking for psnr and ssim


Another tab vs space thing?

This is very annoying. I have https://www.emacswiki.org/emacs/SmartTabs on, as we spoke about it before as well. I still think this is the best way to indent code but it needs to be done for the whole project. We spoke about this before doing such indentation via clang-format. I created #545 PR. It was closed without any conlcusion though.

For now I have added a pre-commit git-hook for myself. Maybe if the repo won't accept commits a githook should be installed for users.

#!/bin/sh # Reject commits that adds tabs if git diff --cached | egrep '^\+.*\t'>/dev/null; then echo "Can't commit due to introduction of tabs in the following lines" git diff --cached | egrep '^\+.*\t' exit 1 fi

Replying just to this for now. Will look over the other changes later.

I closed #545 because there had been many changes to the code you'd opened. Since then Khronos has contracted with RasterGrid to set up something clang-format based to ensure uniform code style. That work has not yet started.

Until then the pre-commit hook seems like a good idea.

MarkCallow · 2023-12-05T02:48:09Z

lib/astc_encode.cpp

+		}
+	}
+
+    // We are done with astcencoder


Fixed in a925f3f

MarkCallow · 2023-12-05T02:50:55Z

tools/ktx/command_create.cpp

@@ -95,7 +95,7 @@ struct OptionsCreate {

    void init(cxxopts::Options& opts) {
        opts.add_options()
-                (kFormat, "KTX format enum that specifies the image data format."
+                (kFormat, "The data format of the images in the output KTX file."


s/The/VkFormat enum that specifies the/

Fixed in a925f3f

MarkCallow · 2023-12-05T02:52:23Z

tools/ktx/command_create.cpp

@@ -95,7 +95,7 @@ struct OptionsCreate {

    void init(cxxopts::Options& opts) {
        opts.add_options()
-                (kFormat, "KTX format enum that specifies the image data format."
+                (kFormat, "The data format of the images in the output KTX file."
                    " The enum names are matching the VkFormats without the VK_FORMAT_ prefix."
                    " The VK_FORMAT_ prefix is ignored if present."
                    "\nWhen used with --encode it specifies the format of the input files before the encoding step."


s/input files/texture object created as input to the encoding step/

For this and the previous comment, also change the Doxygen docs.

Fixed in a925f3f

MarkCallow · 2023-12-05T02:53:50Z

tools/ktx/command_encode_astc.cpp

@@ -163,9 +157,12 @@ void CommandEncodeAstc::executeEncodeAstc() {

    MetricsCalculator metrics;
    metrics.saveReferenceImages(texture, options, *this);
+
+	options.mode = KTX_PACK_ASTC_ENCODER_MODE_LDR; // TODO: Fix me for HDR textures


tab vs space?

There are lots more after this both in this and following files. I gave up flagging them after this point.

all tabs changed to spaces in a925f3f

MarkCallow · 2023-12-05T03:12:28Z

tools/ktx/metrics_utils.h

-        ec = ktxTexture2_TranscodeBasis(texture, KTX_TTF_RGBA32, 0);
+		if (isFormatAstc((VkFormat)texture->vkFormat))
+		{
+			ec = ktxTexture2_DecodeAstc(texture, VK_FORMAT_R8G8B8A8_UNORM);


Please update the documentation for --compare-ssim and --compare-psnr. I failed to notice before that they are currently only valid for BasisU compression. The documentation still says that. I knew we had ASTC decode working but I see now that it is only in ktx extract.

…stc decoding

wasimabbas-arm · 2023-12-09T13:37:28Z

lib/astc_encode.cpp

+ */
+// will update "This" with the uncompressed copy
+KTX_API KTX_error_code KTX_APIENTRY
+ktxTexture2_DecodeAstc(ktxTexture2 *This, ktx_uint32_t vkformat) {


@MarkCallow In-case it didn't register, as you can see I have introduced this "Decode" method inside ktx.h and astc_encode.cpp this might sound a bit weird but I didn't think creating a seprate astc_decode.cpp file was worth it. If you don't agree please shout.

It did register but thanks for the added notice. Does the decoder share much code with what is already in astc_encode.cpp? If it does not it is probably better to create astc_decode.cpp. Maybe we can change command_extract.cpp to use it. On second thoughts maybe not. It only needs to decode the image(s) being extracted not the whole texture.

It does reuse bits from astc_encode.cpp thats mainly why I put it there and some more decode specific bits I added too but more importantly it will be using bits from astc_encode.cpp for HDR stuff as well going forward.

Okay. Leave it in the same file. Let's rename the file to astc_codec.cpp.

wasimabbas-arm added 3 commits December 1, 2023 16:42

Add missing backslahes to build_macos.sh

74a8199

Move astc options out of create

9000e0c

Add encode-astc support

4263d61

MarkCallow reviewed Dec 2, 2023

View reviewed changes

ci_scripts/build_macos.sh Outdated Show resolved Hide resolved

MarkCallow requested changes Dec 2, 2023

View reviewed changes

MarkCallow reviewed Dec 2, 2023

View reviewed changes

tools/ktx/command_encode_astc.cpp Outdated Show resolved Hide resolved

Shuffle around doxygen snippet and remove CMAKE_EXPORT_COMPILE_COMMANDS

c4cc963

MarkCallow reviewed Dec 4, 2023

View reviewed changes

tools/ktx/command_create.cpp Outdated Show resolved Hide resolved

MarkCallow reviewed Dec 4, 2023

View reviewed changes

Add decode method for astc textures that is used by psnr and ssim met…

24d0fee

…rics

MarkCallow reviewed Dec 5, 2023

View reviewed changes

Remove tabs, replacing with 4 spaces each, fix bug with vkformat in a…

a925f3f

…stc decoding

wasimabbas-arm commented Dec 9, 2023

View reviewed changes

Minor typo fix

b8ccb41

wasimabbas-arm mentioned this pull request Dec 9, 2023

Refactor common options #817

Merged


		// if(params->perceptual) flags \|= ASTCENC_FLG_USE_PERCEPTUAL;

		uint32_t threadCount{1}; // Decompression isn't the bottlneck and only used when checking for psnr and ssim

WIP: Encode astc support #810

Are you sure you want to change the base?

WIP: Encode astc support #810

Conversation

wasimabbas-arm commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkCallow Dec 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkCallow Dec 4, 2023 • edited

Choose a reason for hiding this comment

MarkCallow Dec 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqnuep commented Dec 4, 2023

MarkCallow commented Dec 4, 2023

aqnuep commented Dec 4, 2023

wasimabbas-arm commented Dec 4, 2023 • edited

MarkCallow commented Dec 4, 2023

MarkCallow commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wasimabbas-arm Dec 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkCallow Dec 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkCallow Dec 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkCallow Dec 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkCallow Dec 4, 2023 •

edited

MarkCallow Dec 4, 2023 •

edited

MarkCallow Dec 4, 2023 •

edited

wasimabbas-arm commented Dec 4, 2023 •

edited

wasimabbas-arm Dec 9, 2023 •

edited

MarkCallow Dec 5, 2023 •

edited

MarkCallow Dec 5, 2023 •

edited

MarkCallow Dec 5, 2023 •

edited