GNOME Bugzilla – Bug 644032
Color banding in Hue-Saturation with overlap
Last modified: 2016-11-13 09:59:09 UTC
I just ran into this bug the other day with the Hue-Saturation tool. Steps to reproduce: 1 - Paint a simple gradient from red (RGB 255,0,0) to magenta (RGB 255,0,255). 2 - Access the Hue-Saturation tool, select the Magenta channel, and adjust its hue clockwise by a decent amount (+30, 60, or even 180º will do). Everything looks fine so far, right? Now start increasing the overlap from 0 to 100% and watch as rainbow color bands appear across the gradient. This is unexpected, to say the least. 3 - Now, leaving the overlap at 100%, reset the Magenta channel and start slowing increasing the hue. Watch how rainbow color bands start appearing out of the blue (no pun intended) almost right away, and the peculiar way they spread across the gradient as the hue shift is increased further. This can occur with other color combinations too. For example, using a green-cyan gradient, and adjusting the hue of the Cyan channel (with 100% overlap), color bands start to appear at approximately -125º, and again note the peculiar way they spread across the gradient as you increase the shift in hue.
Created attachment 215132 [details] Screenshot of colorbands produced by (Magenta channel +45º, overlap 50%) Here's an illustration for that first example - note that the adjustments made are not particularly significant - the behavior is very easy to reproduce. This can occur on almost any color channel in the Hue-Saturation tool: - Red channel: (Unable to duplicate) - Yellow channel: any Hue < 0 - Green: Hue < -60 - Cyan channel: Hue < -120 or Hue > 120 - Blue channel: Hue > 60 - Magenta channel: any Hue > 0 Of note is that this only occurs when the adjustment calculations cross a red/magenta boundary.
Confirmed on gimp-2.8.2 But the problem doesn't occur on master.
Created attachment 237096 [details] Demonstration of behavior (expected vs actual) Really; if somebody's finally fixed it for good then cool, but as of (my) 2.8.4 it's still quite alive and well. Attached is a demonstration of an actual usecase (similar in colors to the one I originally discovered it with); I've also posted an XCF version of it to the mailing list for anyone interested in verifying it on their GIMP.
You can compare the math in: 2.8: app/base/hue-saturation.c master: app/operations/gimpoperationhuesaturation.c I did the port without making an effort to actually understand the math, so I didn't "fix" anything, it just fixed itself by using floating point. Your best chance is probably to look for overflowing 8 bit color values, or similar integer problems.
I've browsed the online repo before ... in master, the math is pretty straightforward, almost* foolproof: http://git.gnome.org/browse/gimp/tree/app/operations/gimpoperationhuesaturation.c ----------------------- mapped_primary_hue = map_hue (config, hue, hsl.h); mapped_secondary_hue = map_hue (config, secondary_hue, hsl.h); /* Find nearest hue on the circle between primary and * secondary hue */ diff = mapped_primary_hue - mapped_secondary_hue; if (diff < -0.5) { mapped_secondary_hue -= 1.0; } else if (diff >= 0.5) { mapped_secondary_hue += 1.0; } hsl.h = (mapped_primary_hue * primary_intensity + mapped_secondary_hue * secondary_intensity); ----------------------- While the gimp-2.8 branch reads like this: http://git.gnome.org/browse/gimp/tree/app/base/hue-saturation.c?h=gimp-2-8 ----------------------- if (diff < -127 || diff >= 128) r = (gint) (hs->hue_transfer[hue][r] * primary_intensity + (hs->hue_transfer[secondary_hue][r] + 255) * secondary_intensity) % 255; else r = hs->hue_transfer[hue][r] * primary_intensity + hs->hue_transfer[secondary_hue][r] * secondary_intensity; ----------------------- Um, yeah, I see it. Notice that in 2.8 secondary_hue is always adjusted PLUS one full circle, not plus OR minus as it is in master. (And why does it use 255 instead of 256? This creates an output range of [0,254] when the output range should be [0,255]. ) Hmm.... I think the best way to fix the hue adjustment is, instead of calculating both channels independently and then interpolating the results, we should work some algebra and make a better equation entirely: // The current behavior is basically this: result_hue = mapped_primary_hue * primary_intensity + mapped_secondary_hue * secondary_intensity // Given: mapped_*_hue = original_hue + *_hue_adjustment result_hue = (original_hue + primary_hue_adjustment) * primary_intensity + (original_hue + secondary_hue_adjustment) * secondary_intensity // (aka) result_hue = original_hue * primary_intensity + original_hue * secondary_intensity + primary_hue_adjustment * primary_intensity + secondary_hue_adjustment * secondary_intensity // (aka) result_hue = original_hue * (primary_intensity + secondary_intensity) + (primary_hue_adjustment * primary_intensity) + (secondary_hue_adjustment * secondary_intensity) // Given: primary_intensity + secondary_intensity = 1 result_hue = original_hue * 1 + (primary_hue_adjustment * primary_intensity) + (secondary_hue_adjustment * secondary_intensity) // (aka) result_hue = original_hue + (primary_hue_adjustment * primary_intensity) + (secondary_hue_adjustment * secondary_intensity) (...always enjoyed a good algebra exercise...) And since primary_intensity and secondary_intensity are calculated directly from original_hue (and other stable values), the result can probably also be cached in an integer lookup array for faster subsequent access. Heeh... long comment. Maybe I really should think about installing yet another compiler so I can try a few GIMP code experiments firsthand... could have saved a lot of time compared to typing this up. (It's not that I type slow; I just edit a lot) (* - ps: there are a few cases where even master can get the hue-saturation result wrong, but those are extremely rare, and generally bizarre enough to question the user's sanity anyway)
Looks like I did understand the math after all when porting :) Please turn your findings into a patch.
strata_ranger, what about a patch?
Busy. (job, the usual distractions...) More tangibly, I do have a text editor with code formatting but to actually test a patch I'd definitely need to set up a compiler. Which I'm increasingly tempted to do the more I hang around the bugtracker/mailing list (but that's another topic).
I would take an untested patch too :)
strata_ranger, ping?
Created attachment 303113 [details] [review] Hue overlap regions should interpolate hue adjustments then map, not map then interpolate Took me some time to find it (wow, 2012? old), and while I don't know if it's in entirely the correct format, this is the general idea. I typed it up relative to the floating point calculations in master (I notice 2.8 uses integer maths).
Sorry for being such a lame reviewer... I don't understand, I thought the GEGL version worked just fine, but this patch is against the GEGL operation? What do I miss?
If it helps, I haven't been actively monitoring this either.... 1) In the 2.8 branch the bounds check performed on a huesat adjustment with overlap increments one of the mappings in only a single direction, so in cases where a red/magenta boundary is crossed, sometimes it's incrementing the wrong channel in the wrong direction. Examples: (magenta+60, red+0, yellow-60) with 100% overlap. Expected result for any input hue between magenta-red-yellow: red. Actual result: a full rainbow of hues. 2) The general order of operations for overlap hue calculations is just conceptually wrong to begin with -- mapping the hue by both channels and then interpolating the result leaves specific cases where the red/magenta bounds checking logic actually produces the wrong result. Rewriting the calculation to interpolate the adjustment first then map it once eliminates the matter entirely (also, gets the job done slightly faster: 1 interpolation + 1 hue mapping vs. 2 hue mappings + 1 interpolation). Example: (yellow+120, green-120, input pixel=#80ff00) with any (non-zero) overlap. Expected result: hue+0 (color #80ff00). Actual result: hue+180 (color #8000ff). (Because the difference between adjacent channels exceeds 180 degrees, GIMP assumed a red/magenta boundary must have been crossed ... when that is actually not the case!)
Yes the rationale of merging the hue values *then* mapping is clear. I was just under the impression that the GEGL code is correct (see comment 2). So to be clear: we fix the GEGL code to be really correct, and leave the old 2.8 code to rot as it should, right?
It would be kind of nice to fix the first case in the (non-GEGL) 2.8 code since that one is so easy to generate. Let's see, according to github the file is app/base/huesaturation.c, around line 175 : -------------- gint diff; diff = hs->hue_transfer[hue][r] - hs->hue_transfer[secondary_hue][r]; if (diff < -127 || diff >= 128) r = (gint) (hs->hue_transfer[hue][r] * primary_intensity + (hs->hue_transfer[secondary_hue][r] + 255) * secondary_intensity) % 255; else r = hs->hue_transfer[hue][r] * primary_intensity + hs->hue_transfer[secondary_hue][r] * secondary_intensity; -------------- Note that if (diff < -127), incrementing secondary_hue by +255 actually magnifies the diff instead of reducing it. Instead it should be: -------------- gint diff; diff = hs->hue_transfer[hue][r] - hs->hue_transfer[secondary_hue][r]; if (diff >= 128) r = (gint) (hs->hue_transfer[hue][r] * primary_intensity + (hs->hue_transfer[secondary_hue][r] + 255) * secondary_intensity) % 255; else if (diff < -127) r = (gint) ((hs->hue_transfer[hue][r] + 255) * primary_intensity + hs->hue_transfer[secondary_hue][r] * secondary_intensity) % 255; else r = hs->hue_transfer[hue][r] * primary_intensity + hs->hue_transfer[secondary_hue][r] * secondary_intensity; --------------
Fixed in master, finally: commit b27cdfa99a5c12ce151e8c826d3648cc320f245d Author: Richard Gitschlag <strata_ranger@hotmail.com> Date: Sun Nov 13 10:56:15 2016 +0100 Bug 644032 - Color banding in Hue-Saturation with overlap When calculating an overlap between two ranges, interpolate the hue adjustment from config->hue[primary_range] and config->hue[secondary_range] BEFORE mapping it to the input value. This fixes odd edge cases where only one of the ranges crosses the red/magenta wraparound, or if adjustments to different channels yield more than 180 degree difference from each other. app/operations/gimpoperationhuesaturation.c | 56 ++++++++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 22 deletions(-)