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

Support horizontally flipped TGA images #68

Merged
merged 8 commits into from
Jul 21, 2024
Merged

Conversation

illwieckz
Copy link
Member

@illwieckz illwieckz commented Jul 11, 2024

Support horizontally flipped TGA images.

The TGA format is a bottom-left format by default: the first byte of the first column is expected to be displayed at the bottom-left of the screen, this behavior is inherited from devices painting the screen from the bottom left to the top right.

The TGA format provides two bits in the image descriptor byte to paint the data from other sides. The 4th bit tells the device to paint the screen from right-to left and the 5th bit tells the device to paint the screen from top to bottom.

So basically:

  • 00: from bottom-left to top-right
  • 01: from top-left to bottom-right
  • 10: from bottom-right to top-left
  • 11: from top-right to bottom-left

Previously stb_image only read the 5th bit and then only supported the loading of vertically flipped images, stb_image was ignoring the 4th bit coding the horizontal flip. Now both flipping directions are supported for both raw and RLE storage.

The RLE TGA images were converted from raw TGA ones using GIMP and
some tweaks as the GIMP exporter only support bottom-left and top-left:

- Left ones are unmodified GIMP export.
- Right ones are in-GIMP horizontally flipped images exported as left
  ones then the TGA X flip bit was modified by hand in a hex editor.
The TGA format is a bottom-left format by default: the first byte
of the first column is expected to be displayed at the bottom-left
of the screen, this behavior is inherited from devices painting the
screen from the bottom left to the top right.

The TGA format provides two bits in the image descriptor byte to paint
the data from other sides. The 4th bit tells the device to paint the
screen from right-to left and the 5th bit tells the device to paint
the screen from top to bottom.

So basically:

- 00: from bottom-left to top-right
- 01: from top-left to bottom-right
- 10: from bottom-right to top-left
- 11: from top-right to bottom-left

Previously stb_image only read the 5th bit and then only supported the
loading of vertically flipped images, stb_image was ignoring the 4th bit
coding the horizontal flip. Now both flipping directions are supported
for both raw and RLE storage.
@illwieckz illwieckz added the enhancement New feature or request label Jul 11, 2024
@illwieckz
Copy link
Member Author

illwieckz commented Jul 11, 2024

This is tested against existing raw TGA test files already provided in the repository, and additional RLE TGA test files provided with the PR.

The raw TGA ones are from https://gitlab.com/illwieckz/investigating-tga-orientation-in-imagemagick

They are generated with a script written for the only purpose of generating those reference images for debugging image libraries and tools.

The RLE TGA images were converted from the raw TGA ones using GIMP and some tweaks, as the GIMP exporter only supports bottom-left and top-left:

  • Left ones are unmodified GIMP export.
  • Right ones are in-GIMP horizontally flipped images exported as left ones then the TGA X flip bit was modified by hand in a hex editor.

@illwieckz
Copy link
Member Author

illwieckz commented Jul 11, 2024

We are in need for a reliable tool we can recommend that is able to convert TGA to PNG, as TGA is the de-facto legacy idTech3 lossless image format and we prefer to store PNG or WebP images in repository (and cwebp expects PNG as input), so we need to reliably convert TGA files to PNG.

ImageMagick convert is not reliable, it is known to be buggy for years, it is also well known that there is no way to know if a given ImageMagick convert version is affected or not:

The bug is considered to not be a bug by the maintainer despite countless reports and a complete study having been published with an alternate from-scratch implementation written against the specifications to compare:

For reference, here are the specifications:

The crunch repository being a required dependency of the engine because of the inc/crn_decomp.h transcoder header library, the CRN format being the recommended image formats for the Unvanquished releases, and the Dæmon crunch tool being the recommended tool to be used in the Unvanquished production pipeline, the crunch tool is the best candidate for a recommended tool for converting TGA files to PNG, as we already maintain it and make sure it works and is available to contributors.

@illwieckz
Copy link
Member Author

I plan to submit the stb_image changes to upstream.

@illwieckz illwieckz force-pushed the illwieckz/tga-x-flip branch 7 times, most recently from 71e63b3 to a5680a3 Compare July 11, 2024 06:08
@illwieckz
Copy link
Member Author

illwieckz commented Jul 11, 2024

I added some checks of what crunch produces, it confirmed i686 without SSE2 doesn't produce same file than all other tested platforms, as discussed there:

When passing the -msse2 flag the checksum is the same as the one computed on other platforms.

@illwieckz illwieckz force-pushed the illwieckz/tga-x-flip branch 10 times, most recently from 9e879a6 to 4385e86 Compare July 11, 2024 09:07
@illwieckz illwieckz force-pushed the illwieckz/tga-x-flip branch 3 times, most recently from f74086e to 504b4aa Compare July 16, 2024 05:58
@illwieckz
Copy link
Member Author

illwieckz commented Jul 16, 2024

So, I implemented the idea of dumping a checksum file and check against it.

It happened that by doing so I was able to test the checksum for all the files, and then I discovered that “jpeg to dds” is not reproducible. When running a GCC build on Ubuntu 24.04 or on 22.04 chroot on 24.04 on my own Zen 2 amd64, I don't get the same result for jpg to dds that I get with GCC on Ubuntu 22.04 on Azure amd64… I don't know why yet but I give up on that for now.

So for now the exhaustive file check is disabled, what is still done is the clone check: for every command that should produce the same file (like png to crn and bmp to crn, or bottom-left tga to crn and top-right tga to crn), it is checked that all the clones produce the same checksum. This is enough to verify that image orientation is properly implemented.

@illwieckz illwieckz force-pushed the illwieckz/tga-x-flip branch 4 times, most recently from 525a02b to 4f3de1c Compare July 16, 2024 19:46
@illwieckz
Copy link
Member Author

illwieckz commented Jul 16, 2024

So I discovered that GCC defaults on some -std=gnu*, and when not using -std=c*, GCC uses -ffp-contract=fast even when -ffast-math is disabled, so GCC uses -ffp-contract=fast by default… This is what made the generated files not reproducible.

Now CMake also disables FP contraction whith USE_FAST_MATH=OFF.

I enabled the file checksuming, it now works everywhere.

If needed (for example when committing a change changing some format on purpose), the checksum database can be re-generated this way:

test/test.py --record

@illwieckz
Copy link
Member Author

Hmm, I still have a mistmatch on jpg-to-dds, converting unvanquished_64.jpg to unvanquished_64.dds produces this on all CI (Azure/Appveyor/MSVC/GCC/MinGW/Clang/AppleClang/Windows/Linux/macOS/amd64/arm64/i686…):

  • c1b3b785d107ba2f18097fb1cf009f598dbda9e797741ed66bd6cf9b1c376447f0990bb3890af12e3c73c4e02410adc0b5be6f386867d8ffc6aded53adb5f309

But on my end I got this whatever the compiler:

  • e331afe1f1bc6d48b95afcc031e3b13d067ac093a60031631643961903d26d385093cc312f9a7f6903303fbc7c5d4852389d05725cf8529c8957ebb64367d8fb

What's weird is that it's always one on CI, and always another one on my end.

@illwieckz
Copy link
Member Author

illwieckz commented Jul 16, 2024

The only 8 bytes differences in a 2872 bytes file:

20240717-003242-000 vbindiff-jpg-to-dds

@illwieckz
Copy link
Member Author

illwieckz commented Jul 16, 2024

So I disabled exhaustive file checking again, I only left clone checking enabled.

More research may be done in another time in another PR, there is no need to postpone merging what is already working.

@illwieckz illwieckz force-pushed the illwieckz/tga-x-flip branch 5 times, most recently from 25cc8c7 to 711a7be Compare July 17, 2024 02:03
- appveyor: do not build the shared library (it is unusable yet),
- azure-pipelines: make all binaries use the same library,
- codeql: use the shared library.
@illwieckz
Copy link
Member Author

illwieckz commented Jul 17, 2024

I finally reproduced the jpg-to-dds mismatch!

No  c1b3b785d1 build/test/jpg-to-all/unvanquished_64.dds

I thought that such mismatch was insane:

Host System Compiler Architecture blake2 sum
🌍️ Appveyor Windows MSVC 16 amd64 ✅️ c1b3b785d1
🌍️ Appveyor Windows MSVC 16 i686 ✅️ c1b3b785d1
🌍️ Azure Ubuntu 20.04 GCC 8 amd64 ✅️ c1b3b785d1
🌍️ Azure Ubuntu 20.04 Clang 11 amd4 ✅️ c1b3b785d1
🌍️ Azure Ubuntu 20.04 GCC 9 i686 ✅️ c1b3b785d1
🌍️ Azure Ubuntu 20.04 GCC 9 arm64 ✅️ c1b3b785d1
🌍️ Azure Ubuntu 22.04 MinGW 10 amd64 ✅️ c1b3b785d1
🌍️ Azure Ubuntu 22.04 MinGW 10 i686 ✅️ c1b3b785d1
🌍️ Azure macOS 12 AppleClang 14 amd64 ✅️ c1b3b785d1
🏡️ Home Ubuntu 24.04 GCC 13 amd64 ❌️ e331afe1f1
🏡️ Home Ubuntu 24.04 Clang 16 amd64 ❌️ e331afe1f1
🏡️ Home Ubuntu 24.04 Clang 18 amd64 ❌️ e331afe1f1
🏡️ Home Ubuntu 20.04 GCC 9 arm64 ❌️ e331afe1f1
🏡️ Home Ubuntu 20.04 GCC 9 armhf ❌️ e331afe1f1
🏡️ Home Ubuntu 24.04 MinGW 13 amd64 ❌️ e331afe1f1
🏡️ Home macOS 10 AppleClang 10 amd64 ❌️ e331afe1f1
🏡️ Home FreeBSD 13.3 Clang 17 amd64 ❌️ e331afe1f1

The only difference I could identify was that it was running on my end, and the hypothesis of my home being subject to a spell was not realistic, so I started to check the differences between the CI servers and my own system, looking at environment variables, etc.

Then I found the difference… My computer has 32 threads. Those CI systems provide no more than 4 threads (If I'm right: 2 on AppVeyor and 4 on Azure).

As soon as I do crunch -helperThreads 3 I reproduce the same checksum as the CI on my end!

@illwieckz illwieckz force-pushed the illwieckz/tga-x-flip branch 2 times, most recently from fc21439 to 0974162 Compare July 17, 2024 04:10
@illwieckz
Copy link
Member Author

illwieckz commented Jul 17, 2024

So, it looks like all the options to make image conversion reproducible in a CI are there, I then enabled the exhaustive file check.

@slipher
Copy link
Member

slipher commented Jul 20, 2024

LGTM

@illwieckz illwieckz merged commit 6f59cc5 into master Jul 21, 2024
14 checks passed
@illwieckz illwieckz deleted the illwieckz/tga-x-flip branch July 21, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants