Refactor common options

wasimabbas-arm commented 9 months ago

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 https://github.com/KhronosGroup/KTX-Software/pull/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 commented 9 months ago

@MarkCallow for your review.

MarkCallow commented 8 months ago

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

Is PR #810 dependent on this?

wasimabbas-arm commented 8 months ago

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

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

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.

wasimabbas-arm commented 4 months ago

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

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

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

wasimabbas-arm commented 4 months ago

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

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

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

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

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

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

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

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

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 thebefore_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 commented 4 months ago

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

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

MarkCallow commented 4 months ago

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

@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. <packages@erlang-solutions.com>
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 commented 4 months ago

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.

wasimabbas-arm commented 4 months ago

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

KhronosGroup / KTX-Software

Refactor common options #817