After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 728607 - Patch to make divide blend mode work at 32-bit floating point
Patch to make divide blend mode work at 32-bit floating point
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: General
git master
Other All
: Normal normal
: 2.10
Assigned To: GIMP Bugs
GIMP Bugs
Depends on:
Blocks:
 
 
Reported: 2014-04-20 13:23 UTC by Elle Stone
Modified: 2017-01-15 14:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch to make divide blend mode work at 32-bit floating point (1.02 KB, patch)
2014-04-20 13:23 UTC, Elle Stone
committed Details | Review
xcf file showing issues with divide and unbounded mode sRGB (731.46 KB, image/x-gimpfile)
2014-04-20 14:06 UTC, Elle Stone
  Details

Description Elle Stone 2014-04-20 13:23:03 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.
Comment 1 Elle Stone 2014-04-20 14:06:51 UTC
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.
Comment 2 Michael Natterer 2014-04-29 22:19:54 UTC
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(-)
Comment 3 Mike Henning (drawoc) 2014-04-30 01:44:29 UTC
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?
Comment 4 Ville Sokk 2014-04-30 06:05:44 UTC
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.
Comment 5 Michael Natterer 2014-04-30 09:58:31 UTC
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.
Comment 6 Elle Stone 2014-04-30 10:46:05 UTC
(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.
Comment 7 Michael Natterer 2014-05-21 22:26:35 UTC
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.
Comment 8 Mike Henning (drawoc) 2014-05-21 22:56:58 UTC
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?
Comment 9 Michael Natterer 2014-05-21 23:48:55 UTC
I would not trust that this behavior is guaranteed on all implementations.
Comment 10 Øyvind Kolås (pippin) 2017-01-15 14:37:00 UTC
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