Skip to content
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

Improve output determinism and add internal ktxdiff tool for comparing test outputs #745

Merged
merged 5 commits into from
Jul 31, 2023

Conversation

VaderDev
Copy link
Contributor

@VaderDev VaderDev commented Jul 26, 2023

  • cmake: Set FMT_SYSTEM_HEADERS in cmake to suppress some warnings in fmt headers. (Compile error "possibly dangling reference to a temporary" #737)
  • cmake: Set CMAKE_LIBRARY_OUTPUT_DIRECTORY on MacOS to address .dylib lookup issues.
  • cmake: Add /fp:precise, -ffp-contract=off and -ffp-model=precise compiler options to improve output determinism
  • create: Fix documentation typo (Help ktx_create (small typo in the documentation) #744)
  • create: Fix color space conversion over and underflow
  • ktxdiff: Add ktxdiff as an internal testing tool
  • cts: Integrate ktxdiff with the outputTolerance test property
  • cts: Remove allowMismatchOnMSVC property in favor of outputTolerance
  • cts: Introduce the concept of a --primary platform (replacing the --msvc flag). On primary platforms output tolerance is ignored and CTS requires exact matching of the outputs. On non-primary platforms if the outputTolerance is enabled the golden file regeneration is disabled. The primary platform is currently set to GCC as this is the only platform that produces deterministic output on every system we have tested it.
  • cts: Switch to FindPython3

ktxdiff: This is a new test only tool with limited capability which aims to address our testing needs.
This is not a user facing tool as that would requires comprehensive and complete feature sets, documentation and test coverage which are outside of its scope. The tool separately compares the metadata and the uncompressed (zlib/zstd) "untranscoded" (uastc, basis-lz) decoded (astc) image data by either memcmp or by the given tolerance value.
Usage: ktxdiff <expected-filepath> <received-filepath> <tolerance-float>. ktxdiff exits with 0 on match, 1 on mismatch, 2 on error and writes information about mismatch to stdout and information about any error to stderr.

Copy link
Collaborator

@MarkCallow MarkCallow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this because I wanted to see what you had done with the FP options. A few changes are needed to get the same settings that the ASTC dev says makes their invariance tests pass on all the compilers they support.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
tools/ktx/image.hpp Outdated Show resolved Hide resolved
tests/ktxdiff/ktxdiff_main.cpp Show resolved Hide resolved
tests/ktxdiff/ktxdiff_main.cpp Show resolved Hide resolved
@VaderDev
Copy link
Contributor Author

Thank you for the review.
I am still trying to find the reason for last failure with that MSVC build. On my system with MSVC it is successful, on our MSVC CI its successful, and even other MSVC jobs are passing, so another curios one.

@MarkCallow
Copy link
Collaborator

even other MSVC jobs are passing, so another curios one.

Those other MSVC jobs are not running the tests. They are principally for checking builds are successful with various options.

@VaderDev
Copy link
Contributor Author

Those other MSVC jobs are not running the tests.

windows (windows-2019, v142, x64, ...) does run the CTS

@MarkCallow
Copy link
Collaborator

windows (windows-2019, v142, x64, ...) does run the CTS

That's VS2019. The one that's failing is VS2022 cl.exe.

@VaderDev
Copy link
Contributor Author

Okey, the results are in. The issue was too strict output tolerance.

@VaderDev VaderDev marked this pull request as ready for review July 27, 2023 11:53
@aqnuep aqnuep marked this pull request as draft July 27, 2023 12:07
@VaderDev VaderDev marked this pull request as ready for review July 27, 2023 14:54
@MarkCallow
Copy link
Collaborator

Do you have any idea why tests were passing with VS2019 cl.exe ( < v19.30) and failing with VS2022 cl.exe (v19.30), both on x64, before you adjusted the tolerance?

Would you please modify tests/toktx-tests.cmake to use ktxdiff instead of diff. I'd like include it with this PR.

@VaderDev
Copy link
Contributor Author

Do you have any idea why tests were passing with VS2019 cl.exe ( < v19.30) and failing with VS2022 cl.exe (v19.30), both on x64, before you adjusted the tolerance?

I am pretty sure they changed their std library implementation between VS2019 and VS2022 causing basis-lz to produce different artifacts. I tried to keep the tolerance values reasonable low, but some artifacts can cause really high differences if you look at the difference between each color value. If you are unfortunate enough a 'negative' artifact (an artifact that causes a color value to be lower) can be replaced with a 'positive' one causing the difference to double.

Would you please modify tests/toktx-tests.cmake to use ktxdiff instead of diff. I'd like include it with this PR.

Not as part of this PR. This PR was already a big enough headache as it tightened every platforms conformance.

Tests in tests/toktx-tests.cmake and in texturetests.cc require additional work as this assert
Assertion failed: pos == private->_levelIndex[level].byteOffset + baseOffset, file D:/Project/KTX-Software/lib/writer2.c, line 530
is failing on multiple platforms (Most likely a StreambufStream bug).

@MarkCallow
Copy link
Collaborator

I am pretty sure they changed their std library implementation between VS2019 and VS2022 causing basis-lz to produce different artifacts.

So it wasn't anything to do with floating point. I'm glad to know that because we are speficying the same type of FP behavior to both compilers.

Tests in tests/toktx-tests.cmake and in texturetests.cc require additional work as this assert ... is failing on multiple platforms

I'll look into it but it won't be until some time after Siggraph. What are the circumstances needed to trigger the assert? I don't see it currently. Does it happen when you change a test to use ktxdiff instead of diff?

@VaderDev
Copy link
Contributor Author

So it wasn't anything to do with floating point.

No its not. Its just a consequence that we are now doing a meaningful test on every platform.

What are the circumstances needed to trigger the assert?

(I misspoke, the assert is only fires on MinGW, the other platforms have different issues)
Run all tests on
MinGW Debug (Win11 GCC 13.1.0): 68 failure, most of them are the assert
MSVC Debug (Win11 VS2022): 1 failure that only comes up on my dev machine: ktx2check-test-all: ERROR: Size of image data in file does not match size calculated from levelIndex.
MacOS Release: 119 failure, most of them are file loading failures (not a ktx2 file, failed to load png file or Binary files differ)

And just to clarify, these failing tests are only affecting the legacy tools. Every new tool and new test passes on every platform that we are testing.

@MarkCallow
Copy link
Collaborator

What are the circumstances needed to trigger the assert?

(I misspoke, the assert is only fires on MinGW, the other platforms have different issues)
Run all tests on MinGW Debug (Win11 GCC 13.1.0): 68 failure, most of them are the assert
MSVC Debug (Win11 VS2022): 1 failure that only comes up on my dev machine: ktx2check-test-all: ERROR: Size of image data in file does not match size calculated from levelIndex.
MacOS Release: 119 failure, most of them are file loading failures (not a ktx2 file, failed to load png file or Binary files differ)

And just to clarify, these failing tests are only affecting the legacy tools. Every new tool and new test passes on every platform that we are testing.

I haven't been running the tests on Debug configurations in CI because they take so damn long. I don't understand the MacOS failures. They aren't showing up in CI nor when I run them on my MacBook. What is different for you?

The ktx2check-test-all failure sounds very much like the failure I saw in ktx2check-test-pipe-read which is due to a bug in pipes in Git for Windows 2.41.0's bash/MinGW. See git-for-windows/git#4464. That test is no longer run on WIN32. However ktx2check-test-all does not use pipes so it must be a different problem.

I am happy to fix StringbufStream as we'll continue to support that. I don't want to put too much time into the legacy tools though.

I have to finish work on issue #727 before I can look into these issues.

@aqnuep
Copy link
Collaborator

aqnuep commented Jul 28, 2023

@MarkCallow, we run our tests on a Mac Mini M2 running macOS 13.4.1 (build 22F82).

@MarkCallow
Copy link
Collaborator

@MarkCallow, we run our tests on a Mac Mini M2 running macOS 13.4.1 (build 22F82).

I just ran the tests on an M2 running 13.4.1 (c) without this PR. All tests are passing except the small list I sent you before, that fail due to precision issues, etc. The same tests were passing on 13.4.1 before that. Are these failures new with this PR? Presumably they occur only on Apple Silicon as the tests are passing in CI on x64.

@VaderDev
Copy link
Contributor Author

VaderDev commented Jul 28, 2023

All of these failures are really old, they are failing on commits even before the first ktxtools PR was merged. (I think the windows ones were not failing at the start of the project, but one windows update around 2023.mar. introduced some failures, similarly as it did for the CI)

@VaderDev
Copy link
Contributor Author

VaderDev commented Jul 28, 2023

Additional important information: With Apple Silicon on MacOS most of the tests are not failing on Debug build, they only fail on Release build.

@MarkCallow
Copy link
Collaborator

MarkCallow commented Jul 28, 2023

All of these failures are really old, they are failing on commits even before the first ktxtools PR was merged

Which failures, the Debug build failures on MinGW and Windows or the Release failures on macOS? I can understand not noticing the former as CI does not run the tests on Debug builds. I can't understand not noticing the latter as they are run in CI on x64 and I am always running them in Release mode on my M2 MacBook before I commit anything.

Additional important information: With Apple Silicon on MacOS most of the tests are not failing on Debug build, they only fail on Release build.

I am completely confused as to why you are seeing these failures and I am not.

Are you running in Xcode or from the command line? If from Xcode have you built the package target before you run the RUN_TESTS target? It is not possible in CMake to create dependencies for RUN_TESTS that will build the programs to be tested. Building package is the simplest way to get everything. Until now. Likely there is no install (rightly so) for ktxdiff so it will not get built by the package target. Probably need to build that target as well before running RUN_TESTS.

@VaderDev
Copy link
Contributor Author

All of these failures are really old, they are failing on commits even before the first ktxtools PR was merged

Which failures, the Debug build failures on MinGW and Windows or the Release failures on macOS?

Both of them are old.
MacOS: We did not see notice these before because M2 MacOS was a recent addition to our CI (we integrated it during the development of this PR) and on the github CI they are running fine. Did not investigated the reasons.
MinGW: I noticed it recently and checked older commits and found that it was failing even before the ktxtools branch.

Are you running in Xcode or from the command line?

Command line with calling cmake and ctest

It is not possible in CMake to create dependencies for RUN_TESTS that will build the programs to be tested.

Yes it is possible.

But, we got sidetracked in this conversation. This PR should not cause or affect these failures. I will gather every failure and send you an email with the list and details.

@MarkCallow
Copy link
Collaborator

But, we got sidetracked in this conversation.

I am awaiting @aqnuep's review.

Copy link
Collaborator

@aqnuep aqnuep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@MarkCallow MarkCallow merged commit 7f67af7 into KhronosGroup:main Jul 31, 2023
13 checks passed
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
…g test outputs (KhronosGroup#745)

* Implement internal ktxdiff tool for comparing test output files.
* Fix color conversion overflow.
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
…g test outputs (KhronosGroup#745)

* Implement internal ktxdiff tool for comparing test output files.
* Fix color conversion overflow.
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
…g test outputs (KhronosGroup#745)

* Implement internal ktxdiff tool for comparing test output files.
* Fix color conversion overflow.
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
…g test outputs (KhronosGroup#745)

* Implement internal ktxdiff tool for comparing test output files.
* Fix color conversion overflow.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants