-
Notifications
You must be signed in to change notification settings - Fork 226
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
WIP: Encode astc support #810
base: main
Are you sure you want to change the base?
Conversation
|
||
if (texture->supercompressionScheme != KTX_SS_NONE) | ||
fatal(rc::INVALID_FILE, "Cannot encode KTX2 file with {} supercompression.", | ||
toString(ktxSupercmpScheme(texture->supercompressionScheme))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Also glad your reviewed this, does it mean the other checks in executeEncodeAstc
makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -8,6 +8,34 @@ | |||
|
|||
namespace ktx { | |||
|
|||
/** | |||
//! [command options_astc] | |||
<dl> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the <dl> should be here. Is it in the other snippets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In encode_utils.h it has, and from the source c4cc963#diff-8ca48860d1bd768774f38f0f522ba91ef7dbb887e660917c74b7088da850752aL604 where I brought this from, it had it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@MarkCallow, @lexaknyazev, I believe that the consensus was that 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. |
I agree with the consensus that encode is limited to BasisU. I have no issue with changing the name of this tool from 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 |
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 @lexaknyazev seems to have a vision for the long-term structure of the new CLI tools, so I would really interested in his suggestions. |
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 think its not just the change of What's the issue with calling this 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. |
Yes.
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.
+1. |
Yes, they are. But they aren't formats known to the GPU. Therefore the file's vkFormat field is set to |
lib/astc_encode.cpp
Outdated
@@ -360,7 +360,7 @@ astcSwizzle(const ktxAstcParams ¶ms) { | |||
|
|||
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have another tabs v. spaces thing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a925f3f
lib/astc_encode.cpp
Outdated
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a925f3f
} | ||
|
||
if (!This->pData) { | ||
if (ktxTexture_isActiveStream((ktxTexture*)This)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lib/astc_encode.cpp
Outdated
|
||
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another tab vs space thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lib/astc_encode.cpp
Outdated
} | ||
} | ||
|
||
// We are done with astcencoder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decoder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a925f3f
tools/ktx/command_create.cpp
Outdated
@@ -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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/The/VkFormat enum that specifies the/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a925f3f
tools/ktx/command_create.cpp
Outdated
@@ -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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/input files/texture object created as input to the encoding step/
For this and the previous comment, also change the Doxygen docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a925f3f
tools/ktx/command_encode_astc.cpp
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tab vs space?
There are lots more after this both in this and following files. I gave up flagging them after this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all tabs changed to spaces in a925f3f
tools/ktx/metrics_utils.h
Outdated
ec = ktxTexture2_TranscodeBasis(texture, KTX_TTF_RGBA32, 0); | ||
if (isFormatAstc((VkFormat)texture->vkFormat)) | ||
{ | ||
ec = ktxTexture2_DecodeAstc(texture, VK_FORMAT_R8G8B8A8_UNORM); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
*/ | ||
// will update "This" with the uncompressed copy | ||
KTX_API KTX_error_code KTX_APIENTRY | ||
ktxTexture2_DecodeAstc(ktxTexture2 *This, ktx_uint32_t vkformat) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Leave it in the same file. Let's rename the file to astc_codec.cpp
.
This PR has:
encode-astc
tool