KhronosGroup / KTX-Software

KTX (Khronos Texture) Library and Tools
Other
875 stars 230 forks source link

WIP: Encode astc support #810

Closed wasimabbas-arm closed 4 days ago

wasimabbas-arm commented 11 months ago

This PR has:

aqnuep commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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.

MarkCallow commented 1 week ago

@wasimabbas-arm are you making progress? I like this finished by the end of this month (Oct) so we can build the next release and move on to HDR support. I have one item I am working on too that I must finish before the release will be complete.

abbaswasim commented 1 week ago

@wasimabbas-arm are you making progress? I like this finished by the end of this month (Oct) so we can build the next release and move on to HDR support. I have one item I am working on too that I must finish before the release will be complete.

No sorry. I had a brief look at this a while ago after the other PR was merged. Since we agreed we don't want a separate tool, I wasn't sure if it's worth continuing this PR or create a new one. I will have a look this weekend.

wasimabbas-arm commented 1 week ago

@MarkCallow This is now replaced by https://github.com/KhronosGroup/KTX-Software/pull/952 please close this.