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

Test image stats of all image modes #8393

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Yay295
Copy link
Contributor

@Yay295 Yay295 commented Sep 18, 2024

Test the image stats of all image modes. This should give an indicator if a code change changes the image data anywhere.

@Yay295
Copy link
Contributor Author

Yay295 commented Sep 18, 2024

and somehow specific tests in specific environments are failing

specifically:
"sum" with mode "LAB" on "amazon-2-amd64"
"sum" with mode "HSV" on "debian-12-bookworm-x86"

@Yay295
Copy link
Contributor Author

Yay295 commented Sep 19, 2024

The image stats are all based on Image.histogram(), so I've created #8394.

@radarhere
Copy link
Member

radarhere commented Sep 21, 2024

#8394 (comment)

This makes me think there's actually something going on in the conversion from "RGB" to "LAB" and "HSV" that is somehow not the same in every environment.

From what I can see, our 32-bit Docker job is giving different results from RGB to HSV conversion at

h = fmod((h / 6.0 + 1.0), 1.0);

Perhaps this is to be expected when you are checking the output with a high level of precision?

As for the LAB check, it is happening when using lcms2 2.6. This is not surprising given it is noticeably older than lcms2 2.12, otherwise the earliest version used in our Docker jobs.

@Yay295
Copy link
Contributor Author

Yay295 commented Sep 22, 2024

I tried changing the RGB to HSV conversion to use double instead of float, and I tried implementing fmod() directly, but neither of those changes (nor both together) resolved the issue. It did cause the max value to be correct, but the hash was still wrong for just that environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants