When using a blurring filter on a layer with transparency in 32 bit float mode, the FFT convolution puts random pixels into the fully transparent parts (alpha=0) of the image. I'm not sure what is the mathematically correct solution, but this patch fixes the problem. The alphaValue can be 0 here, and the original code is dividing with this value. But I don't understand why this happens only in the float32 mode.
Details
Details
Diff Detail
Diff Detail
- Repository
- R37 Krita
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
I made a new version of this diff. After some investigations I think if alphaValue is zero, it means alphaValueInv is Inf. Then, when you multiply with this value, the result is either NaN or Inf, depending on what value you multiply with (0 * Inf = NaN). I think the visible pixels appear when the result is NaN so the comparisons against the min and max values fails. Then the NaN goes into the layer pixel data. I modified the code a bit: it tests alphaValueInv instead of alphaValue, because in a division, not only the exact 0.0 can cause an invalid result. And I also fill the destination channels with zero values when the wrong case occurs. If you know a better way for this (without the double conversion) feel free to change it.
Comment Actions
Hi, @fazek!
Thank you for a nice catch! I have only one comment:
Probably, it is better to check for
if (alphaValue == 0) { ...}or
if (alphaValue < epsilon) { ...}or
if (alphaValue < std::numeric_limits<qreal>::epsilon()) { ...}The problem is that the behavior of std::isfinite() depends significantly on the compiler, CPU and compilation options, so it might cause troubles in the future. Basically, why not just skip this division if alpha is zero? Working with NaN makes the FPU work in a special mode, so it might also cause significant performance issues in the end.
Comment Actions
First I used the (alphaValue > 0) version but then I was curious what happens here. But the epsilon version is better. We could add extra safety with tricky comparisons as well (and maybe it's worth to search for other occurences in the code):
As far as I know, a comparison of two floats is always false if at least one of them is NaN. So instead of
inline void limitValue(qreal *value, qreal lowBound, qreal highBound) {
if (*value > highBound) {
*value = highBound;
} else if (*value < lowBound){
*value = lowBound;
}
}we could use something like this (and hoping the compiler knows it right and not optimizes the trick out):
inline void limitValue(qreal *value, qreal lowBound, qreal highBound) {
if (*value <= highBound) {
if (*value < lowBound) {
*value = lowBound;
}
} else {
*value = highBound; // NaN falls here
}
}Comment Actions
I made some more tests. GNU c++ knows the difference between (a < b) and (!(a >= b)) so I added an extra modification in the limitValue() to filter out any remaining NaNs. Interesting that this modification alone solves the original problem. But since I'm not sure what happens in other compilers, it's safer to keep the original alphaValue test solution too, just I changed it to the epsilon version.
Comment Actions
Hi, @fazek!
The patch looks nice now! Please push it!
Could you also elaborate a bit about the difference between '<' and '!<='? NaNs are not equal to anything. Could you add a comment about it before pushing?