Refactor common options #817

wasimabbas-arm · 2023-12-09T19:02:22Z

This PR moves thread-count, noSSE and normal-mode from basis OptionsCodec to basis utils and renames OptionsCodec to BasisCodec.

This is required in prepartion for #810 PR, where we are separating basis encode from encode-astc.

If you read the PR by each commit separately it will make more sense.

wasimabbas-arm · 2023-12-09T19:02:37Z

@MarkCallow for your review.

MarkCallow · 2023-12-19T11:28:45Z

@wasimabbas-arm Sorry for the delay. I am working on the 4.3-beta1 release. I haven't reviewed yet but I did notice a couple of things from the description you wrote: thread-count and normal-mode will be needed also by encode-astc, right? When you say rename OptionsCodec to BasisCodec do you mean to OptionsBasisCodec? It should definitely have Options in the name.

Before we proceed any further please create a short document describing your plans: what tools, what options apply to them, will we support normal_mode (the 3 channel to 2 channel conversion anyway) and normalization for uncompressed textures, if so, for what formats,what new options are needed (normalization), etc. Suggest a Google doc. Once done share it with the TSG for feedback.

MarkCallow · 2023-12-19T11:40:14Z

Is PR #810 dependent on this?

tools/ktx/encode_utils.h

tools/ktx/command_create.cpp

tools/ktx/basis_utils.h

wasimabbas-arm · 2023-12-21T12:13:25Z

Before we proceed any further please create a short document describing your plans:

@MarkCallow https://docs.google.com/document/d/1bIsYcornoFjVVLuBGNX6j0cIooYJAwXBPngl0iNmOu4/edit?usp=sharing is a document I created with my proposal. Please have a look. Haven't worked with google docs before so not sure how the permissions work. You probably need to request it.

MarkCallow · 2024-04-17T08:31:28Z

@wasimabbas-arm please rebase this and make sure it follows the proposal you wrote. Also rename compress_utils to deflate_utils. There has been no push back on this proposed named change so let's proceed.

I think these change are orthogonal to the commands presented to the user. It is a good idea to separate the utilities for each type of encoding and deflate from any of the ktx apps. Let's get this done.

wasimabbas-arm · 2024-04-17T22:27:35Z

Looks like there has been a lot of changes since I made this PR. Will take me a while to rebase and make the required edits. On it.

… tools

wasimabbas-arm · 2024-04-19T15:48:08Z

For the failing tests, I have tried to fix those in the CTS but looks like @MarkCallow you have had a go at those already especially for deflate. After pulling the latest I am still getting those tests failing and I don't understand why.

Like the test ktxToolsTest.tools.cli_errors which is looking for golden/tools/cli_errors/missing_command.err.txt

ktx: Missing command.

Usage:
  ktx [--version] [--help] <command> <command-args>

  -h, --help     Print this usage message and exit
  -v, --version  Print the version number of this program and exit
      --testrun  Indicates test run. If enabled the tool will produce deterministic output whenever 
                 possible

Available commands:
  create     Create a KTX2 file from various input files
  extract    Extract selected images from a KTX2 file
  encode     Encode a KTX2 file
  transcode  Transcode a KTX2 file
  info       Print information about a KTX2 file
  validate   Validate a KTX2 file
  help       Display help information about the ktx tool

For detailed usage and description of each subcommand use 'ktx help <command>'
or 'ktx <command> --help'

But I am getting

ktx: Missing command.

Usage:
  ktx [<command>] [OPTION...]

  -h, --help     Print this usage message and exit
  -v, --version  Print the version number of this program and exit
      --testrun  Indicates test run. If enabled the tool will produce deterministic output whenever 
                 possible

Available commands:
  create     Create a KTX2 file from various input files
  deflate    Deflate (supercompress) a KTX2 file
  extract    Extract selected images from a KTX2 file
  encode     Encode a KTX2 file
  transcode  Transcode a KTX2 file
  info       Print information about a KTX2 file
  validate   Validate a KTX2 file
  compare    Compare two KTX2 files
  help       Display help information about the ktx tool

For detailed usage and description of each subcommand use 'ktx help <command>'
or 'ktx <command> --help'

There is deflate that I understand, but the addition of [--version] [--help] shouldn't be introduced by this PR.

Does the CTS tests need fixing for those as well?

MarkCallow · 2024-04-22T01:30:50Z

There is deflate that I understand, but the addition of [--version] [--help] shouldn't be introduced by this PR.

Does the CTS tests need fixing for those as well?

--version and --help appear in both the golden and your output so I do not understand your comment regarding those?

If none of the changes here affect any of the command outputs, you need to

Make sure you are rebased against current main.
In your workarea cd tests/cts; git checkout main having made sure that you have set up the CTS submodule.
cd ..; git commit tests/cts; git push

The last step updates the CTS reference in your branch and the PR to point at what is checked out in tests/cts which will have the tests corresponding to KTX-Software main by virtue of the second step.

If the changes affect any command output, you need to:

Create a branch in tests/cts based on tests/cts's main branch.
Update the relevant tests so everything passes locally
cd tests/cts; git push to push the branch to KTX-Software-CTS.
cd ..; git commit tests/cts; git push

MarkCallow · 2024-04-23T04:54:07Z

@wasimabbas-arm are the tests working for you locally? Did you have to modify any of them?

wasimabbas-arm · 2024-04-23T08:55:19Z

@wasimabbas-arm are the tests working for you locally? Did you have to modify any of them?

@MarkCallow No they don't.

The following tests FAILED:
	611 - ktxToolsTest.create.compare (Failed)
	613 - ktxToolsTest.create.encode_blze_params (Failed)
	618 - ktxToolsTest.create.encode_uastc_params (Failed)
	622 - ktxToolsTest.create.help (Failed)
	651 - ktxToolsTest.encode.compare (Failed)
	653 - ktxToolsTest.encode.encode_blze_params (Failed)
	662 - ktxToolsTest.encode.encode_uastc_params (Failed)
	663 - ktxToolsTest.encode.help (Failed)

This is the same list as what fails in TravisCI

This time once I have pushed the cts commit fewer tests are failing.

--version and --help appear in both the golden and your output so I do not understand your comment regarding those?
The last step updates the CTS reference in your branch and the PR to point at what is checked out in tests/cts which will have the tests corresponding to KTX-Software main by virtue of the second step.

I think I know what was happening before. I was doing this process locally. I will do submodule update on tests/cts and then will go into tests/cts to pull the latest main and then run the tests by running ./ci_scripts/build_macos.sh but looks like that script is resetting the tests submodule to the CTS repo reference that is committed. Not sure why this is happening locally. Otherwise for me to test any changes I have to commit these changes to the CTS repo and then update the reference first?

MarkCallow · 2024-04-23T10:23:22Z

I think I know what was happening before. I was doing this process locally. I will do submodule update on tests/cts and then will go into tests/cts to pull the latest main and then run the tests by running ./ci_scripts/build_macos.sh but looks like that script is resetting the tests submodule to the CTS repo reference that is committed. Not sure why this is happening locally. Otherwise for me to test any changes I have to commit these changes to the CTS repo and then update the reference first?

git submodule update tests/cts will checkout in tests/cts the commit that is referenced in your KTX-Software workarea. When you rebased to current main that commit will be the one referenced by main unless you somehow managed to keep the pre-rebase reference. To actually update the reference, if you need to, you have to checkout the desired commit in the tests/cts directory (that was main in my previous instructions) and then go back to the KTX-Software workarea (cd ../..) and git commit tests/cts.

./ci_scripts/build_macos.sh is not resetting the submodule. It simply calls ctest. Perhaps ctest is but I doubt it as I think it does the same as the RUN_TESTS target in the Xcode project and I know that does not.

There is no need to commit changes to the CTS repo to test them. Nor is there any need to update the reference in your KTX-Software workarea. Simply make the changes in the tests/cts submodule and run the tests. Do NOT do git submodule update. You can also run individual tests directly via clitest.py and avoid cmake and ctest. E.g the following runs the tests in deflate/cli_errors.json.

cd tests/cts/clitests
./clitest.py -e ../../../build/macos-x86_64/Release/ktx -d ../../../build/macos-x86_64/Release/ktxdiff --keep-matching-outputs tests/deflate/cli_errors.json

Check your build directory name. What I wrote in the example is the standard place used by build_macos.sh.

You can use the same command to regenerate the golden files for the test. Simply add -r before the -e.

wasimabbas-arm · 2024-04-23T10:51:24Z

git submodule update tests/cts will checkout in tests/cts the commit that is referenced in your KTX-Software workarea. When you rebased to current main that commit will be the one referenced by main unless you somehow managed to keep the pre-rebase reference. To actually update the reference, if you need to, you have to checkout the desired commit in the tests/cts directory (that was main in my previous instructions) and then go back to the KTX-Software workarea (cd ../..) and git commit tests/cts.

./ci_scripts/build_macos.sh is not resetting the submodule. It simply calls ctest. Perhaps ctest is but I doubt it as I think it does the same as the RUN_TESTS target in the Xcode project and I know that does not.

Here is what I see. I do cd tests/cts in tests/cts I am on a48547df... commit. Since I have now rebased my PR 817 on top of KTX-Software main I also pull the latest tests/cts main and now I am on 7e0154... Add tests for ktx deflate. (#25) in tests/cts. I move out cd ../.. and run ./ci_scripts/build_macos.sh.. It goes and does its thing.

When I go back into tests/cts I am back on a48547df... commit. As you can see I haven't run anything else in between. This is why I was thinking the script is moving me back and I am getting all the errors. (Note: you won't see a48547df... because it was my attempt of fixing the tests changing golden files etc and I pushed that into a local copy of my tests/cts repo. Not sure if that matters)

Anyways since I have pushed in an updated reference to main into PR 817. I think we can ignore that issue but the fact that the few tests are still failing. (Although I am interested to know if the script is doing that, will find out)

You can use the same command to regenerate the golden files for the test. Simply add -r before the -e.

Cool thanks for pointing this one out. I was copy pasting the command directly onto the tool and copying its output and then updating the golden with it.

wasimabbas-arm · 2024-04-23T10:56:54Z

Ah here we go. The script has

if [ "$FEATURE_TOOLS_CTS" = "ON" ]; then
  git submodule update --init --recursive tests/cts
fi

So it is putting you back on the reference of the submodule. I think this is problematic for the reason I mentioned above. You can't work on this repo locally.

MarkCallow · 2024-04-23T11:12:36Z

Ah here we go. The script has

if [ "$FEATURE_TOOLS_CTS" = "ON" ]; then
  git submodule update --init --recursive tests/cts
fi

The script is normally used in CI which starts with a blank slate. Since FEATURE_TOOLS_CTS is not always ON we do not do --recursive on the git clone which would waste bandwidth and money. Instead we do this. It cannot be removed. You will have to add a check so it only does this when running on CI. Travis sets some environment variable you can check, I think. We need to figure out a way to remember that this check will have to be modified when we move to another CI service. If there is a way to check if the submodule is initialized that would be even better.

You can workaround this by committing the new reference (git commit tests/cts) before you run the script.

I almost always generate and use an Xcode project hence this has not been an issue for me.

wasimabbas-arm · 2024-04-23T11:22:19Z

You can workaround this by committing the new reference (git commit tests/cts) before you run the script.

Thats what I have done now. But at least the mystery is solved :)

Now back to the problem. Any thoughts on the following tests failing. I haven't looked deep enough yet but my changes shouldn't introduce anything that results in that. (famous last words)

The following tests FAILED:
	611 - ktxToolsTest.create.compare (Failed)
	613 - ktxToolsTest.create.encode_blze_params (Failed)
	618 - ktxToolsTest.create.encode_uastc_params (Failed)
	622 - ktxToolsTest.create.help (Failed)
	651 - ktxToolsTest.encode.compare (Failed)
	653 - ktxToolsTest.encode.encode_blze_params (Failed)
	662 - ktxToolsTest.encode.encode_uastc_params (Failed)
	663 - ktxToolsTest.encode.help (Failed)

MarkCallow · 2024-04-23T11:33:04Z

Now back to the problem. Any thoughts on the following tests failing.

None. Sorry. Suggest you run them directly with clitest.py. Its output will tell you why they are failing. In the case of output to golden file comparison it will give you the file names it is comparing (the captured output and the golden file) so you can look at the content.

wasimabbas-arm · 2024-04-23T20:58:33Z

Some of the tests I can understand they are related to moving --no-sse around. But some are complaining about KVD size mismatch and have been looking out for where that is done. Since these are comparing binary ktx2 files, its very hard to tell what's different.

MarkCallow · 2024-04-23T23:08:47Z

But some are complaining about KVD size mismatch and have been looking out for where that is done. Since these are comparing binary ktx2 files, its very hard to tell what's different.

Use ktx compare to see how the files are different.

About

if [ "$FEATURE_TOOLS_CTS" = "ON" ]; then
  git submodule update --init --recursive tests/cts
fi

, please remove this from build_{linux,macos},sh and add it to the before_scriptsection of.travis.yml`. It will make your work a little easier and will save having to make another PR to make this change.

EDIT: I decided a separate PR is better. See #899.

wasimabbas-arm · 2024-04-24T09:05:16Z

Use ktx compare to see how the files are different.

Thank you. I have eyes now :)

ktx compare golden/create/compare/output_blze_1_ssim_2d_r8_unorm.ktx2 output/create/compare/output_blze_1_ssim_2d_r8_unorm.ktx2                                                                                                                           

Key/Value Data

-KTXwriterScParams: --clevel 2 --qlevel 16 --threads 1 --no-sse
+KTXwriterScParams: --clevel 2 --qlevel 16 --no-sse --threads 1

Looks like the order of these parameters is changed. I think this will need a PR into the tests/cts repo right?

wasimabbas-arm · 2024-04-24T10:01:00Z

Specifically the following tests are failing, all something to do with --no-sse. In some its the placement in the last one its again the placement but in the help text.

Does all these need changes to the golden files in tests/cts?

ktx-test-fails.txt

tools/ktx/basis_utils.h

MarkCallow · 2024-04-24T10:56:53Z

Yes you will need a PR to KTX-Software-CTS and once the PR is created you will have to push an update to the tests/cts reference to this PR.

I think options are processed in the order they appear when the Combine template is called. That order will directly affect the ordering of the options listed in KTXwriterScParams. The order in the help text depends on the order the @snippet items appear in the command's help text.

Does all these need changes to the golden files in tests/cts?

You either need to change the ordering or generate new golden files.

wasimabbas-arm · 2024-04-24T13:22:41Z

@MarkCallow Most tests are passing now but some linux targets in TravisCI are failing to boot with The command "case "${TRAVIS_OS_NAME:-linux}" in below is more context around the error.

W: https://packages.erlang-solutions.com/ubuntu/dists/focal/Release.gpg: Key is stored in legacy trusted.gpg keyring (/etc/apt/trusted.gpg), see the DEPRECATION section in apt-key(8) for details.
W: GPG error: https://packages.erlang-solutions.com/ubuntu focal Release: The following signatures were invalid: BADSIG D208507CA14F4FCA Erlang Solutions Ltd. <[email protected]>
E: The repository 'https://packages.erlang-solutions.com/ubuntu focal Release' is not signed.
N: Updating from such a repository can't be done securely, and is therefore disabled by default.
N: See apt-secure(8) manpage for repository creation and user configuration details.
W: http://package.perforce.com/apt/ubuntu/dists/jammy/InRelease: Key is stored in legacy trusted.gpg keyring (/etc/apt/trusted.gpg), see the DEPRECATION section in apt-key(8) for details.
The command "case "${TRAVIS_OS_NAME:-linux}" in
  linux)
    if [ "$WASM_BUILD" = "YES" ]; then
      # Need to set uid/gid because, unlike when running docker locally,
      # /src ends up being owned by the uid/gid running this script and
      # the recent fix for CVE-2022-24765 in Git causes Git to error
      # when the repo owner differs from the user. For details see
      # https://github.blog/2022-04-12-git-security-vulnerability-announced/
      docker run -dit --name emscripten --user "$(id -u):$(id -g)" -v $(pwd):/src emscripten/emsdk bash
    elif [ "$CHECK_REUSE" != "ONLY" -a "$CHECK_MKVK" != "ONLY" ]; then
      sudo apt-get update
    fi

Doesn't sound related. Any idea?

MarkCallow · 2024-04-25T07:47:25Z

Doesn't sound related. Any idea?

It is not related. This is happening during apt-get update which updates the packages installed on the Travis Ubuntu runner. It seems there was a problem with the erlang package. I restarted the first failed job. It passed the failing point - it completed the update - so I have also restarted the other failing jobs. I guess Erlang fixed their package.

tools/ktx/basis_utils.h

tools/ktx/command_create.cpp

tools/ktx/command_encode.cpp

tools/ktx/command_create.cpp

wasimabbas-arm · 2024-04-26T13:00:49Z

@MarkCallow If there is no more concerns. this one is ready to be merged.

…header

MarkCallow

Here is the rest of my review.

tools/ktx/command_create.cpp

tools/ktx/basis_utils.h

tools/ktx/encode_utils_basis.h

tools/ktx/encode_utils_common.h

@snippet

Fix various @snippet references that were not updated to reflect file and option name changes in PR #817. Add new encode_utils files to ktxtool target sources to make visible in IDE. Reorganize encoding options and improve explanations in `ktx create` and `ktx encode` man pages. Changes in `ktx encode` documentation are a pre-cursor to adding ASTC support. Full proposed documentation is there but ASTC portions are commented out. This was proposed in a previously distributed e-mail.

wasimabbas-arm force-pushed the refactor_common_options branch from c2d177c to f47584e Compare December 9, 2023 19:37

MarkCallow reviewed Dec 19, 2023

View reviewed changes

tools/ktx/encode_utils.h Show resolved Hide resolved

MarkCallow reviewed Dec 19, 2023

View reviewed changes

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

MarkCallow reviewed Dec 19, 2023

View reviewed changes

tools/ktx/basis_utils.h Show resolved Hide resolved

wasimabbas-arm added 5 commits April 19, 2024 14:39

Abstract encode common options

f557966

Removes OptionsCodec because these are all just basis options

db37f0d

Rename EncodeCodec to BasisCodec in preparation for separating encode…

4dc1223

… tools

Finally rename the utils file to basis_utils.h

f77c969

Updae cts tests part of rebase

ca5113d

wasimabbas-arm force-pushed the refactor_common_options branch from f47584e to ca5113d Compare April 19, 2024 13:54

wasimabbas-arm added 3 commits April 19, 2024 15:01

Rename compress_utils.h to deflate_utils.h

ac7446a

Rename OptionsCodec to OptionsBasis

5a43fdf

Fix bug, now the program will use ktxBasisParam's noSSE directly

870f2f9

Update cts tests

844705e

Rename encode_utils.h to basis_utils.h in doxygen

5de36e7

wasimabbas-arm commented Apr 24, 2024

View reviewed changes

tools/ktx/basis_utils.h Outdated Show resolved Hide resolved

Put noSSE back into common options

f616910

Fill codec common options separately for astc and basis in create

8475985

MarkCallow requested changes Apr 26, 2024

View reviewed changes

tools/ktx/basis_utils.h Outdated Show resolved Hide resolved

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

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

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

Further changes related to compress to deflate

dd12a5e

wasimabbas-arm added 2 commits April 29, 2024 11:38

Move astc options into astc_encode.h

b33f540

Change options_codec to options_codec_basis after changing it in the …

4595078

…header

MarkCallow reviewed Apr 29, 2024

View reviewed changes

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

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

tools/ktx/basis_utils.h Outdated Show resolved Hide resolved

wasimabbas-arm added 3 commits April 29, 2024 17:44

Refactor Option names

3691222

Fix build issues

8c6cb1d

Merge branch 'main' into refactor_common_options

3bc9bfd

MarkCallow reviewed Apr 30, 2024

View reviewed changes

tools/ktx/encode_utils_basis.h Outdated Show resolved Hide resolved

MarkCallow reviewed Apr 30, 2024

View reviewed changes

tools/ktx/encode_utils_common.h Outdated Show resolved Hide resolved

wasimabbas-arm added 3 commits April 30, 2024 10:59

Rename validateEncodeCodec to validateBasisCodec

93f6d1f

Rename captureCodeOption to captureCommonOption

1018e31

Documentation adition for no-sse

cf0617e

MarkCallow approved these changes May 1, 2024

View reviewed changes

MarkCallow merged commit 0a16df8 into KhronosGroup:main May 1, 2024
17 checks passed

MarkCallow mentioned this pull request May 6, 2024

Fix man page issues introduced by PR #817 #903

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor common options #817

Refactor common options #817

wasimabbas-arm commented Dec 9, 2023

wasimabbas-arm commented Dec 9, 2023

MarkCallow commented Dec 19, 2023

MarkCallow commented Dec 19, 2023

wasimabbas-arm commented Dec 21, 2023 •

edited

Loading

MarkCallow commented Apr 17, 2024

wasimabbas-arm commented Apr 17, 2024

wasimabbas-arm commented Apr 19, 2024 •

edited

Loading

MarkCallow commented Apr 22, 2024

MarkCallow commented Apr 23, 2024

wasimabbas-arm commented Apr 23, 2024 •

edited

Loading

MarkCallow commented Apr 23, 2024

wasimabbas-arm commented Apr 23, 2024 •

edited

Loading

wasimabbas-arm commented Apr 23, 2024

MarkCallow commented Apr 23, 2024

wasimabbas-arm commented Apr 23, 2024 •

edited

Loading

MarkCallow commented Apr 23, 2024

wasimabbas-arm commented Apr 23, 2024

MarkCallow commented Apr 23, 2024 •

edited

Loading

wasimabbas-arm commented Apr 24, 2024

wasimabbas-arm commented Apr 24, 2024

MarkCallow commented Apr 24, 2024 •

edited

Loading

wasimabbas-arm commented Apr 24, 2024

MarkCallow commented Apr 25, 2024 •

edited

Loading

wasimabbas-arm commented Apr 26, 2024

MarkCallow left a comment

Refactor common options #817

Refactor common options #817

Conversation

wasimabbas-arm commented Dec 9, 2023

wasimabbas-arm commented Dec 9, 2023

MarkCallow commented Dec 19, 2023

MarkCallow commented Dec 19, 2023

wasimabbas-arm commented Dec 21, 2023 • edited Loading

MarkCallow commented Apr 17, 2024

wasimabbas-arm commented Apr 17, 2024

wasimabbas-arm commented Apr 19, 2024 • edited Loading

MarkCallow commented Apr 22, 2024

MarkCallow commented Apr 23, 2024

wasimabbas-arm commented Apr 23, 2024 • edited Loading

MarkCallow commented Apr 23, 2024

wasimabbas-arm commented Apr 23, 2024 • edited Loading

wasimabbas-arm commented Apr 23, 2024

MarkCallow commented Apr 23, 2024

wasimabbas-arm commented Apr 23, 2024 • edited Loading

MarkCallow commented Apr 23, 2024

wasimabbas-arm commented Apr 23, 2024

MarkCallow commented Apr 23, 2024 • edited Loading

wasimabbas-arm commented Apr 24, 2024

wasimabbas-arm commented Apr 24, 2024

MarkCallow commented Apr 24, 2024 • edited Loading

wasimabbas-arm commented Apr 24, 2024

MarkCallow commented Apr 25, 2024 • edited Loading

wasimabbas-arm commented Apr 26, 2024

MarkCallow left a comment

Choose a reason for hiding this comment

wasimabbas-arm commented Dec 21, 2023 •

edited

Loading

wasimabbas-arm commented Apr 19, 2024 •

edited

Loading

wasimabbas-arm commented Apr 23, 2024 •

edited

Loading

wasimabbas-arm commented Apr 23, 2024 •

edited

Loading

wasimabbas-arm commented Apr 23, 2024 •

edited

Loading

MarkCallow commented Apr 23, 2024 •

edited

Loading

MarkCallow commented Apr 24, 2024 •

edited

Loading

MarkCallow commented Apr 25, 2024 •

edited

Loading