Simplify RGB to HSV conversion

[Yay295]

An incorrect hue was being calculated when "h/6 is negative" because Python's % and C's fmod use a slightly different strategy. So to match Python, we need to implement it ourselves. It can also be simplified some due to the second parameter being 1.

The starting hue calculation has also been simplified by inlining some values. ex.:

h = bc - gc;
h = (((float)(maxc - b)) / cr) - (((float)(maxc - g)) / cr);
h = ((maxc - b) - (maxc - g)) / cr;
h = (maxc - b - maxc + g) / cr;
h = (g - b) / cr

[Yay295]

For the fmod simplification, fmod looks like this (if it wasn't implemented in assembly):

double fmod(double x, double y) {
    return x - y * trunc(x / y);
}

Since y is 1.0 in our case, this becomes just x - trunc(x). And then replace trunc with floor to match Python.

[Yay295]

I've been trying to tweak this code for long enough that I forgot the + 1.0 in the original code (fmod((h / 6.0 + 1.0), 1.0)) already handles the negative values. So this change doesn't fix anything; it's just optimizations.

[radarhere]

From what I can see, there are four basic changes here.

1. You've rearranged the declarations. I don't think there's a need to change them being grouped at the start. It is a pattern used elsewhere, e.g. https://github.com/python-pillow/Pillow/blob/f614580a2de93235d9fe5c802f3d54f881a65c5e/src/libImaging/Convert.c#L1263-L1267
2. You've rearranged math operations to divide once rather than twice.
3. You've replaced fmod with other code. From the back and forth comments, I'm a bit confused about why you've done that?
4. You've removed CLIP8. Could you explain why?

[hugovk]

re: 1., I think declarations at the start of a block was required by the C89 standard, but is no longer by modern compilers. And I find declaring on first use more readable, rather than have to refer back.

[Yay295]

You've replaced fmod with other code. From the back and forth comments, I'm a bit confused about why you've done that?

Originally I did it because the existing comment confused me, thinking negative values weren't being handled correctly. But the new code does replace fmod with a simple floor, so it should be slightly faster.

You've removed CLIP8. Could you explain why?

Because of the fmod, h at that point will be between 0 and 1, so there's no need to clip it after multiplying by 255 because it will already be between 0 and 255.

s in the original code is 255.0 * (float)(maxc - minc) / (float)(maxc). (float)(maxc - minc) / (float)(maxc) will be between 0 and 1, so again the result will already be between 0 and 255. This calculation can also be done without converting anything to floating-point, so I did that too.

[radarhere]

So this change doesn't fix anything; it's just optimizations.

To be clear, you're saying that this PR should not adjust the output data?

[Yay295]

Correct

[radarhere]

To see if the output stayed the same, I converted hopper to HSV, and saved the output to a file for comparison - https://github.com/radarhere/Pillow/commit/8811b00940a0a851a2f9f3adf67bc516d72d4182 / https://github.com/radarhere/Pillow/actions/runs/11159666541

When I introduced this change, the test started failing - meaning that this does change the output.

[hugovk]

See also https://github.com/python/cpython/pull/124880 for a similar CPython change to colorsys.rgb_to_hsv(), which the Pillow function was originally based on.

[radarhere]

Given this PR is motivated purely by performance, but changes the output as a side effect, I'm going to close this. I doubt this function is a bottleneck, and given that we receive issues even when PNG bytes change in saved images, I'm reluctant to alter output.