GNOME Bugzilla – Bug 728607
Patch to make divide blend mode work at 32-bit floating point
Last modified: 2017-01-15 14:37:00 UTC
Created attachment 274758 [details] [review] patch to make divide blend mode work at 32-bit floating point The current divide blend mode doesn't work at 32-bit floating point: http://ninedegreesbelow.com/gimpgit/unbounded-srgb-divide-blend-mode.html#divide-code The current code uses a workaround to deal with the problem of dividing by zero: gfloat comp = (256.0 / 255.0 * in[b]) / (1.0 / 255.0 + layer[b]); That workaround is 8-bit specific. The following code is 32-bit floating point specific: gfloat comp = ( 4294967296.0/ 4294967295.0 * in[b]) / (1.0 / 4294967295.0 + layer[b]); 4294967296 is 2^32. Perhaps 2^64 might be needed for 64-bit data. So perhaps a precision-aware switch needs to be added. I suspect odd things might happen when dealing with numbers as small as 1/(2^32). But I don't know enough about floating point math to say what or why or even if. Divide blend mode doesn't work in unbounded mode sRGB and also doesn't work if the results aren't clipped. Divide mode assumes bounded data. You can divide unbounded data but the results aren't useful for technical uses of divide and quite suspect for artistic uses. The solution might be to make a linear gamma color space from the chromaticities of the source color space. The problem of dividing by 0 remains. As the denominator approaches zero, the result is larger and larger numbers (clipped to 1.0 or not clipped, depending on whether you leave the clipping code in). But at exactly zero the result is black. This is consistent with what PhotoShop does, at least according to an online complaint about same. It seems to me that adding a line of code that reads "if denominator = 0, result = 1" (or positive infinity if one really does want unclipped results) might be the thing to do.
Created attachment 274761 [details] xcf file showing issues with divide and unbounded mode sRGB The attached xcf file has been converted from the source color space to unbounded mode sRGB. The layers show what happens when trying to divide by sRGB red and also the original source color space red after converting to unbounded mode sRGB.
Thanks, fixed in master: commit 3915ac01b4be62dec26c3e19c18c60f23ebd724d Author: Elle Stone <ellestone@ninedegreesbelow.com> Date: Sun Apr 20 08:39:23 2014 -0400 Bug 728607 - Patch to make divide blend mode work at 32-bit floating point Properly port divide mode to 32-bit float. app/operations/gimpoperationdividemode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
This code isn't much better. To begin with, the value 4294967296.0 has absolutely no relevance to 32-bit floating point values. You might as well have chosen any random number that came to your head. Single precision floating point values have one sign bit, eight exponent bits, and a 23-bit significand, which means that they can contain values greater than 4294967296, just with a difference in precision. Actually, at 4294967296, 1 ULP is already greater than 1.0, which means that if you weren't casting to doubles implicitly, you'd actually be discarding all of your image data to rounding errors, and simply setting all of your image to 1.0. For some input values, you're probably already causing a loss of precision right now. I'd suggest that the line be changed, quite simply, to: gfloat comp = in[b] / layer[b]; When layer[b] is positive zero, this should produce the correct result. IEEE 745 defines that (some number) / +0.0 should equal +inf, which I expect CLAMP to handle correctly. That being said, if layer[b] is negative zero, (some number) / -0.0 will be -inf, which clamps to 0.0, which might make divide behave unpredictably. I'm not really sure what to do about that, but I think that values of negative zero should be relatively rare, and for that matter, any negative value will clamp to zero anyway, so I'm not sure it's inconsistent. So, I think the most sane thing to do here is to simply let the divide by zero happen. But then, aren't these supposedly compat layer modes anyway?
Before commiting it, compatibility with GIMP 2.8 should have been checked. As I've said multiple times before, the code is like that to be similar to what GIMP does currently. You requested this, Mitch.
I reviewed that patch together with pippin and the conclusion was that it merely changes both values by a very tiny number. In 8-bit land this was 1.0 / 255.0, now it's something much smaller, which seemed good enough. Any idea for a better patch? The old code is clearly an 8-bit artifact.
(In reply to comment #3) > This code isn't much better. > > To begin with, the value 4294967296.0 has absolutely no relevance to 32-bit > floating point values. You might as well have chosen any random number that > came to your head. The value wasn't chosen exactly randomly. I followed the pattern of the existing code's avoidance of dividing by zero, but looked for a number that was large enough such that when dividing an image by itself, all resulting RGB values were equal to 1.000000, except of course where the RGB values were exactly 0.0, the image in question having a full range of RGB values from 0.0 to 1.0. > > I'd suggest that the line be changed, quite simply, to: > gfloat comp = in[b] / layer[b]; I don't claim to understand 32-bit floating point precision code, but if division by 0 really isn't a problem, then the workaround to avoid division by 0 isn't needed, in which case gfloat comp = in[b] / layer[b]; would seem to be much better.
I still don't see what is wrong with this patch. Allowing division by zero is clearly not going to happen, the assumption that you can somehow continue to work with the returned NaN is wrong.
1.0f / 0.0f is infinity, not NaN. The infinity is clamped to 1.0 later on. (assuming IEEE Floating point) Why not allow division by zero?
I would not trust that this behavior is guaranteed on all implementations.
commit 60847cce734d175b70e4ac3041d459e0051709a7 Author: Øyvind Kolås <pippin@gimp.org> Date: Sun Jan 15 01:28:29 2017 +0100 app: divide blend remove odd ratio scaling, replace with range check for nan