Close #334: gdColorMatch(): doubtful algorithm #751
Conversation
|
Looks good to me, Thanks @cmb69 ! |
|
I think we need a 2.3.3 release, then most likely a 2.3.4. Once 2.3.3 is out (I will try today or later this week), we can branch GD_2_2 and do only sec fixes there. Master will be 2.4 then and merging this PR as well. Thoughts? |
|
It seems that GD-2.3 has already been branched. Any objections to merge this into master? |
| @@ -4,6 +4,7 @@ | |||
|
|
|||
| #include "gd.h" | |||
| #include "gd_color.h" | |||
| #include <math.h> | |||
|
|
|||
| /** | |||
| * The threshold method works relatively well but it can be improved. | |||
There was a problem hiding this comment.
please update the comment docs to match what the code is doing
There was a problem hiding this comment.
Done. Note though that the mentioned L*A*B and Delta-E variants are orthogonal to my changes. These are about calculating the difference in different color space than (s)RGB (what might indeed be good idea).
My change is only about improving the somewhat arbitrary threshold calculation; 195075 is 3 * 255**2 what would make some sense only if there are no alpha channel differences. Adjusting the old value to 211204 would basically work (identical results for threshold 0 and 100), but has a different distribution (due to sqrt()). For instance:
| col1 | col2 | old | adjusted | new |
|---|---|---|---|---|
| 0x00000000 | 0x00000000 | 0.0 | 0.0 | 0.0 |
| 0x00000000 | 0x20404040 | 6.8 | 6.3 | 25.1 |
| 0x00000000 | 0x40808080 | 27.3 | 25.2 | 50.2 |
| 0x00000000 | 0x60c0c0c0 | 61.4 | 56.7 | 75.4 |
| 0x00000000 | 0x7fffffff | 108.3 | 100.0 | 100.0 |
The new column shows what one would expect (0x40808080 is about half-way off, etc.) The last row shows how bad the old (non-adjusted) calculation was, since it yields a threshold > 100 (although that's not supposed to happen).
We change the algorithm of `gdColorMatch()` to calculate the Euclidean distance of the color component values, what may require to change some thresholds in existing code to match the new algorithm, as can be seen by the need to adapt gdimagecolorreplace.c.
| const float dr = (gdImageRed(im, col1) - gdImageRed(im, col2)) / (float) gdRedMax; | ||
| const float dg = (gdImageGreen(im, col1) - gdImageGreen(im, col2)) / (float) gdGreenMax; | ||
| const float db = (gdImageBlue(im, col1) - gdImageBlue(im, col2)) / (float) gdBlueMax; | ||
| const float da = (gdImageAlpha(im, col1) - gdImageAlpha(im, col2)) / (float) gdAlphaMax; |
There was a problem hiding this comment.
I've just learned that float division might be particularly slow. Should check whether this is an issue (gdColorMatch() is called in a tight loop).
We change the algorithm of
gdColorMatch()to calculate the Euclideandistance of the color component values, what may require to change some
thresholds in existing code to match the new algorithm, as can be seen
by the need to adapt gdimagecolorreplace.c.