Add astc support #433

wasimabbas-arm · 2021-05-19T15:46:25Z

This is the start of astc support. First the cli options required are provided.

Since ktx-software now supports multiple encoders this patch extracts
out the common options into '--encode ' and removes redundant options.
Also add enums for astc compressor into ktx.h.

This is the start of astc support. First cli options are provided. Since ktx-software now supports multiple encoder this patch extracts out the common options into '--encode <encoding>' and removes redundant options. Also adds enums for astc compressor into ktx.h.

wasimabbas-arm · 2021-05-19T15:46:54Z

I am aware of the tabs introduced that needs fixing.

include/ktx.h

utils/scapp.h

Add default options for transfer function and fix typos.

Includes build system changes, command line arguments plumbing support Whitespace changes 'tabs->spaces' astc compression support for 2D images including 'layers, mip-levels, cubefaces' Multi-threaded compresssion support using pthreads and pthreads style api on windows

MarkCallow

Thanks. This is close. A few things need changing. Note my review comments do not include the need to add back the existing options as deprecated since we covered that in email.

CMakeLists.txt

include/ktx.h

tests/toktx-tests

tools/toktx/toktx.cc

include/ktx.h

lib/astc_encode.cpp

wasimabbas-arm

Some comments answered in c01d05d.

lib/astc_encode.cpp

tests/toktx-tests

utils/scapp.h

lib/astc_encode.cpp

utils/scapp.h

wasimabbas-arm

This should to complete answering all the review comments except the following:

create a prototype texture of the correct VkFormat
Re-add all deprecated options

tools/toktx/toktx.cc

utils/scapp.h

tools/toktx/toktx.cc

…texture

…re no more levelIndex calculation is required

MarkCallow

If the wrong indentation is because of the tabs vs spaces you noted, then I apologize for the noise.

I'm having a hard time with GitHub here. It seems difficult to see just the changes since my last review and even some review comments I made before are missing.

utils/scapp.h

include/ktx.h

utils/scapp.h

lib/basis_encode.cpp

tools/toktx/toktx.cc

ci_scripts/build_android.sh

ci_scripts/build_android_debug.sh

CMakeLists.txt

lib/astc_encode.cpp

Add common '--normal_mode', common 'threadCount' option, replaced delete with free

wasimabbas-arm · 2021-05-25T18:36:29Z

"to ASTC, transcodable ETC1S / BasisLZ or high-quality transcodable UASTC format."

EDIT: This is in the usage message for --encode. I guess I didn't select the line when I wrote the comment.

Fixed in 18af498

wasimabbas-arm · 2021-05-25T18:37:25Z

It is still not clear if you can enter a number or only the listed strings.

EDIT: This is in the usage message for astc_quality.

So should we leave it there or you want me to implement my prosal #433 (comment)

wasimabbas-arm · 2021-05-25T18:39:29Z

I have also extracted out the --nomal_mode option that is used by both encoders, just like threads into variable in "options". d440dd1

lib/astc_encode.cpp

include/ktx.h

…brary

wasimabbas-arm · 2021-07-22T00:59:48Z

Regarding #433 (comment) we could do one internal to KTX-Software. Not possible for astc-encoder though. There is strict requirement to be able to build all the different binaries for testing/benchmarking purposes from single configure.

MarkCallow · 2021-07-25T07:33:25Z

Regarding #433 (comment) we could do one internal to KTX-Software. Not possible for astc-encoder though. There is strict requirement to be able to build all the different binaries for testing/benchmarking purposes from single configure.

How is this possible? A CMake configure includes the target processor. Is there some sort of universal binary for ARM? Even if there is, the encoder still needs to support Intel processors.

wasimabbas-arm · 2021-07-26T08:38:13Z

How is this possible?

The encoder cmake script has a for loop to generate different executables based on what ISA options are set from the same configure.

MarkCallow · 2021-07-26T08:53:19Z

The encoder cmake script has a for loop to generate different executables

That loop builds for each possible ISA. What are the global config options that are applied to each of these ISA builds?

What are the "native" and "none" builds?

wasimabbas-arm · 2021-07-26T09:44:15Z

The encoder cmake script has a for loop to generate different executables

That loop builds for each possible ISA. What are the global config options that are applied to each of these ISA builds?

What are the "native" and "none" builds?

So set(CONFIGS ${ISA_NATIVE} ${ISA_NONE} ${ISA_NEON} ${ISA_AVX2} ${ISA_SSE41} ${ISA_SSE2}) and

if(${CONFIG})
     set(ISA_SIMD ${ARTEFACT})
     include(cmake_core.cmake)
endif()

Makes sure it only builds for globally set ISA options.

The native build uses -march=native compiler option which sets "default" options for the compiler. And none is just plan C/C++ code all SIMD code stripped off.

MarkCallow · 2021-07-27T07:54:55Z

So set(CONFIGS ${ISA_NATIVE} ${ISA_NONE} ${ISA_NEON} ${ISA_AVX2} ${ISA_SSE41} ${ISA_SSE2}) and
if(${CONFIG})
     set(ISA_SIMD ${ARTEFACT})
     include(cmake_core.cmake)
endif()
Makes sure it only builds for globally set ISA options.

If the global settings are only which ISAs to build for then why

There is strict requirement to be able to build all the different binaries for testing/benchmarking purposes from single configure.

? Presumably you can configure to build for a single ISA so I still don't understand what this requirement really means. Hopefully astc-encoder will be modified to do compile time detection of the ISA as we've previously discussed. In that case the only configure option should be whether to use SIMD instructions or not and this discussion will be irrevelant. Does -march=native do compile-time detection or which SIMD instructions to use?

wasimabbas-arm · 2021-07-27T09:01:59Z

If the global settings are only which ISAs to build for then why

There is strict requirement to be able to build all the different binaries for testing/benchmarking purposes from single configure.

? Presumably you can configure to build for a single ISA so I still don't understand what this requirement really means.

You can indeed build for a single ISA, which is how KTX-Software is using it. This requirement means the maintainer didn't want to cmake configure multiple times for multiple SIMD and non-SIMD options. Instead do cmake configure -DISA_NEON=ON -DISA_SIMD=ON -DISA_AVX=ON. This was a strict requirement when I was proposing other build system changes to make it work with ktx-software. I also proposed SIMD detection but it doesn't work when you are building multiple variants from the same configure command. Not easily anyways.
The encoder does all of this because it build different variants and runs performance tests on it in CI. Of course it can be done differently like a for loop on all SIMD variants via a shell script in the CI but the maintainer chose to use cmake loop instead.

Does -march=native do compile-time detection or which SIMD instructions to use?

I don't know much about this to be honest. This is just another option that the encoder tests against. IIRC I think clang doesn't support native but I didn't bother testing that. We don't use it in ktx-software. The GCC docs says

‘native’

    This selects the CPU to generate code for at compilation time by determining the processor type of the compiling machine. Using -march=native enables all instruction subsets supported by the local machine (hence the result might not run on different machines). Using -mtune=native produces code optimized for the local machine under the constraints of the selected instruction set.

Also not sure where this conversation is going. We have previously decide to leave this where it is. Unfortunately I don't maintain the encoder and I have same push back when I try to propose changes there. Its something thats used by other clients as well and maintaining backwards compatibility is important. As I have already proposed quite a few changes to the build system to make it work with ktx-software. Some like detect SIMD didn't go well. Either way I think for this PR thats kind of out of scope. We have a way to use its provided options to make it work with ktx-software. I think we should leave it there for the first version.

MarkCallow

Please answer the various questions and make the small changes requested. They're mostly minor but I did find one bug.

Where are the readers for the .hdr and .exr formats? Are they going to be in a subsequent PR? I'm planning to try using Open ImageIO which supports both of these, but I have so much else to do it will be a while before I can get to it.

CMakeLists.txt

utils/scapp.h

lib/basis_encode.cpp

tools/toktx/toktx.cc

CMakeLists.txt

lib/astc_encode.cpp

MarkCallow · 2021-07-27T09:52:34Z

You can indeed build for a single ISA, which is how KTX-Software is using it. This requirement means the maintainer didn't want to cmake configure multiple times for multiple SIMD and non-SIMD options. Instead do cmake configure -DISA_NEON=ON -DISA_SIMD=ON -DISA_AVX=ON.

Got it. Makes perfect sense.

I also proposed SIMD detection but it doesn't work when you are building multiple variants from the same configure command. Not easily anyways.

Why not? The configure command would pick which compilers and targets to use. The SIMD detection can be completely unaware of this. It just looks at whatever pre-defined macros it is presented with.

Does -march=native do compile-time detection or which SIMD instructions to use?

...

‘native’

    This selects the CPU to generate code for at compilation time by determining the processor type of the compiling machine. Using -march=native enables all instruction subsets supported by the local machine (hence the result might not run on different machines). Using -mtune=native produces code optimized for the local machine under the constraints of the selected instruction set.

It sounds like it would be up to the code being compiled to decide whether to invoke SIMD or not.

Also not sure where this conversation is going. We have previously decide to leave this where it is. Unfortunately I don't maintain the encoder and I have same push back when I try to propose changes there. Its something thats used by other clients as well and maintaining backwards compatibility is important. As I have already proposed quite a few changes to the build system to make it work with ktx-software. Some like detect SIMD didn't go well. Either way I think for this PR thats kind of out of scope. We have a way to use its provided options to make it work with ktx-software. I think we should leave it there for the first version.

I agree for the first version we should leave it there. I fully understand the need for compatibility with other encoder clients. I was hoping compile-time detection would simplify things for everybody and we could investigate more deeply for a future version.

This is the start of astc support using `lib/astc-encoder`, a `git subrepo` clone of ARM's ASTC encoder. Since ktx-software now supports multiple encoders this patch extracts out the common options into '--encode <encoding>' and removes redundant options. Also adds enums for astc compressor into ktx.h. Similarly the `--threads` option has been moved from the BasisU options to the common options so it can be used for any encoder. The `--bcmp` option has been renamed to `--etc1s` to reflect its actual use. A common '--normal_mode' option has been added replacing the ETC1S encoder's `--normal_map`. The new option is recognized by the ASTC and ETC1S encoders. A very useful feature of uploading the output files of broken toktx-tests to a cloud storage service has been added to `.travis.yml`.

Add astc cli options

d4ee0ce

This is the start of astc support. First cli options are provided. Since ktx-software now supports multiple encoder this patch extracts out the common options into '--encode <encoding>' and removes redundant options. Also adds enums for astc compressor into ktx.h.

wasimabbas-arm commented May 19, 2021

View reviewed changes

include/ktx.h Outdated Show resolved Hide resolved

utils/scapp.h Outdated Show resolved Hide resolved

wasimabbas-arm commented May 19, 2021

View reviewed changes

utils/scapp.h Show resolved Hide resolved

wasimabbas-arm added 2 commits May 20, 2021 02:03

Add swizzle option, remove redundant astc enum from ktx.h

8781b60

Add default options for transfer function and fix typos.

MarkCallow requested changes May 22, 2021

View reviewed changes

Fix review comments for astc support

c01d05d

wasimabbas-arm commented May 23, 2021

View reviewed changes

wasimabbas-arm added 2 commits May 24, 2021 01:56

Fix review comments for astc support part 2

2bf7bc6

Change bcmp command line options to etc1s in tests

e30fb44

wasimabbas-arm commented May 24, 2021

View reviewed changes

tools/toktx/toktx.cc Outdated Show resolved Hide resolved

utils/scapp.h Outdated Show resolved Hide resolved

utils/scapp.h Show resolved Hide resolved

tools/toktx/toktx.cc Outdated Show resolved Hide resolved

wasimabbas-arm added 6 commits May 24, 2021 10:13

Add back deprecated options --bcmp and --uastc

a806477

Update patch up code in ktxTexture_CompressAstcEx to use a prototype …

d56a6e9

…texture

Remove further redundant code, Since we are now using prototype textu…

99e0a72

…re no more levelIndex calculation is required

Remove redundant loop over the levels of the image

bf19acd

Fix CI by adding ISA_NONE to astcenc options

0ac3ab1

Fix linux ci build

ecee952

MarkCallow reviewed May 25, 2021

View reviewed changes

MarkCallow requested changes May 25, 2021

View reviewed changes

MarkCallow reviewed May 25, 2021

View reviewed changes

lib/astc_encode.cpp Show resolved Hide resolved

MarkCallow reviewed May 25, 2021

View reviewed changes

lib/astc_encode.cpp Outdated Show resolved Hide resolved

wasimabbas-arm added 2 commits May 25, 2021 19:15

Fix review comments

18af498

Add common '--normal_mode', common 'threadCount' option, replaced delete with free

Add combined normal mode

d440dd1

MarkCallow reviewed May 26, 2021

View reviewed changes

lib/astc_encode.cpp Show resolved Hide resolved

MarkCallow reviewed May 26, 2021

View reviewed changes

include/ktx.h Show resolved Hide resolved

Minor typo fixes and ensures numLevels never less than 1

8374a69

wasimabbas-arm added 9 commits July 14, 2021 11:04

Fix travis mac os SSE tests

fa0a663

Bump up iphone SDK version to force 64bit builds only

9e2d753

Add astcenc shared library to install targets when built as static li…

8d0f156

…brary

Reset travis linux build

0ec690d

Remove debug code

0103bb3

Enable cubemap test

572c8df

Reset verbose ctest output

9aafbcd

Merge branch 'master' into astc_support

db2d803

Add reuse exclusion for astc example readme

1d955d5

MarkCallow requested changes Jul 27, 2021

View reviewed changes

wasimabbas-arm added 4 commits July 27, 2021 12:15

Fix indentation

1d75eeb

Docs fix and inputSwizzle size fix

8f42128

Remove ktxint.h dependency from scapp.h

625b5ea

Add VS fix for std::max

e273eff

MarkCallow approved these changes Jul 29, 2021

View reviewed changes

MarkCallow merged commit da435de into KhronosGroup:master Jul 29, 2021

wasimabbas-arm deleted the astc_support branch July 30, 2021 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add astc support #433

Add astc support #433

wasimabbas-arm commented May 19, 2021

wasimabbas-arm commented May 19, 2021

MarkCallow left a comment

wasimabbas-arm left a comment •

edited

Loading

wasimabbas-arm left a comment

MarkCallow left a comment

wasimabbas-arm commented May 25, 2021

wasimabbas-arm commented May 25, 2021

wasimabbas-arm commented May 25, 2021

wasimabbas-arm commented Jul 22, 2021

MarkCallow commented Jul 25, 2021

wasimabbas-arm commented Jul 26, 2021

MarkCallow commented Jul 26, 2021

wasimabbas-arm commented Jul 26, 2021 •

edited

Loading

MarkCallow commented Jul 27, 2021

wasimabbas-arm commented Jul 27, 2021 •

edited

Loading

MarkCallow left a comment

MarkCallow commented Jul 27, 2021

Add astc support #433

Add astc support #433

Conversation

wasimabbas-arm commented May 19, 2021

wasimabbas-arm commented May 19, 2021

MarkCallow left a comment

Choose a reason for hiding this comment

wasimabbas-arm left a comment • edited Loading

Choose a reason for hiding this comment

wasimabbas-arm left a comment

Choose a reason for hiding this comment

MarkCallow left a comment

Choose a reason for hiding this comment

wasimabbas-arm commented May 25, 2021

wasimabbas-arm commented May 25, 2021

wasimabbas-arm commented May 25, 2021

wasimabbas-arm commented Jul 22, 2021

MarkCallow commented Jul 25, 2021

wasimabbas-arm commented Jul 26, 2021

MarkCallow commented Jul 26, 2021

wasimabbas-arm commented Jul 26, 2021 • edited Loading

MarkCallow commented Jul 27, 2021

wasimabbas-arm commented Jul 27, 2021 • edited Loading

MarkCallow left a comment

Choose a reason for hiding this comment

MarkCallow commented Jul 27, 2021

wasimabbas-arm left a comment •

edited

Loading

wasimabbas-arm commented Jul 26, 2021 •

edited

Loading

wasimabbas-arm commented Jul 27, 2021 •

edited

Loading