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

Simplify rgb to hsv conversion #8432

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

Conversation

Yay295
Copy link
Contributor

@Yay295 Yay295 commented Oct 1, 2024

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

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.
@Yay295
Copy link
Contributor Author

Yay295 commented Oct 1, 2024

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
Copy link
Contributor Author

Yay295 commented Oct 1, 2024

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.

@Yay295 Yay295 changed the title Handle negative intermediate value in rgb to hsv conversion Simplify rgb to hsv conversion Oct 1, 2024
@radarhere
Copy link
Member

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.
    frompalette(Imaging imOut, Imaging imIn, const char *mode) {
    ImagingSectionCookie cookie;
    int alpha;
    int y;
    void (*convert)(UINT8 *, const UINT8 *, int, ImagingPalette);
  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
Copy link
Member

hugovk commented Oct 1, 2024

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
Copy link
Contributor Author

Yay295 commented Oct 1, 2024

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.

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.

3 participants