Potential issues in the shader

[theagentd]

Hello!

I've noticed a number of peculiar things in the fragment shader code, which I believe are bugs:

1. float invSigmaQx2PI = INV_PI * invSigmaQx2; // 1.0 / (sqrt(PI) * sigma) This line is not equal to 1.0 / (sqrt(PI) * sigma), which seems to simply be a comment error. However, this variable is also not used anywhere in the code, see point 3.

2. vec2 d = vec2(x,y)/size; Dividing d by the texture size implies that the standard deviation (sigma) is in normalized texture coordinates. In fact, dot(d, d) is so close to zero that the resulting denoised image looks identical if it is simply replaced with 0.0. I believe the correct value should be vec2(x, y);, without the division.

3. float blurFactor = exp( -dot(d , d) * invSigmaQx2 ) * invSigmaQx2; The multiplication by invSigmaQx2 seems incorrect and should be invSigmaQx2PI.

Thanks for the nice and simple denoising filter!

Edit: Here's my hopefully fixed ShaderToy code, also switched to using texelFetch() instead of texture(). https://pastebin.com/FUScJxgK

Edit2: Forcing float blurFactor = 1.0; gives visually indistuingishable results to the original shader, and most likely behaves more like a smart box filter than a smart gaussian blur. It does however still look good, and is presumably a decent bit cheaper to evaluate.

[BrutPitt]

First, thanks for reporting this, and thanks also for the detailed annotation.

float invSigmaQx2PI = INV_PI invSigmaQx2; // 1.0 / (sqrt(PI) sigma) This line is not equal to 1.0 / (sqrt(PI) * sigma), which seems to simply be a comment error. However, this variable is also not used anywhere in the code, see point 3.

float blurFactor = exp( -dot(d , d) invSigmaQx2 ) invSigmaQx2; The multiplication by invSigmaQx2 seems incorrect and should be invSigmaQx2PI.

Yes, you got right: "mathematically" I write it correctly: // fXY = exp( -dot(k,k) * invSigmaQx2) * invSigmaQx2PI - Line #147 (or #129 in shaderToy example)

but in the code I used it incorrectly (thanks!)

Dividing d by the texture size implies that the standard deviation (sigma) is in normalized texture coordinates. In fact, dot(d, d) is so close to zero that the resulting denoised image looks identical if it is simply replaced with 0.0. I believe the correct value should be vec2(x, y);, without the division.

And, yes, d not need to be divided by size, but only: vec4 walkPx = texture(tex,uv+d/size);

Here's my hopefully fixed Shadertoy code, also switched to using texelFetch() instead of texture(). https://pastebin.com/FUScJxgK

In the original version, used in glChAoS.P, I also use texelFetch in the bilateralSmartSmooth function. The trouble has been that, when I changed it in texture, for this repo (just because is more flexible to use the normalized size), I have divided wrongly also the dot product in exp of blurFactor.

P.S. I changed also the source in Shadertoy, and added a comment to thank you, and a link to this page

[BrutPitt]

Forcing float blurFactor = 1.0; gives visually indistuingishable results to the original shader, and most likely behaves more like a smart box filter than a smart gaussian blur. It does however still look good, and is presumably a decent bit cheaper to evaluate.

If you use low sigma values, you can note better visually the differences (in right side):

float blurFactor = exp( -dot(d , d) * invSigmaQx2 ) * invSigmaQx2PI;

float blurFactor = 1.0

[theagentd]

Thanks for the quick response!

I too noticed that setting blurFactor to 1.0 did improve quality a bit, since all samples contribute equally. In the end, it works like a box filter (or rather, a pixelated circle?) so it's expected that it eliminates more noise than a gaussian blur. However, in that case it makes more sense to just define a radius in pixels than sigma and ksigma.

[BrutPitt]

I too noticed that setting blurFactor to 1.0 did improve quality a bit, since all samples contribute equally.

It depends from distance of the central pixel (more it is distant and lower is the value: it influences less the central pixel), areas (uniforms vs edges), and threshold value... in uniform colors (like sky) it appears better, because you accentuate the smooth (from far pixels) Anyway nothing forbids to use a magnitude factor to accentuate it, or a personal fixed/variable value (also to add).

In the end, it works like a box filter (or rather, a pixelated circle?) so it's expected that it eliminates more noise than a gaussian blur.

The filter is based on a gaussian blur, the second part takes into account the pixels differences (for edges), so it, in total, loses in the overall smooth (compared to traditional blur on uniform areas) maintaining however detailed edges. Indeed, in glChAoS.P, I use also a combination of denoise+pure gaussian, to get more smooth effect. (like star-dust, when in blending mode) Would make sense to pass more then once with more and more small threshold values, to obtain better results (mostly on uniform areas). Anyway it's not intended to be used with rendering techniques (like Montecarlo approach): there are better filters, with interframe weighted values.

However, in that case it makes more sense to just define a radius in pixels than sigma and ksigma.

Definitely float radius = round(kSigma*sigma);, so you can substitute directly the number of pixels... and after obtain sigma to use in the equation. Or assign sigma = radius, assuming kSigma = 1 Many real-time filters define a fixed rectangular kernel (in pixels), or fixed/variable Sigma with fixed K = 2 (which would be more than enough), but for values kSigma < 3 the edges of "kernel" area are less and less nuanced as kSigma decreases. (comparating with the same radius, namely, same pixel area) Perhaps non visually perceptible in images, but when you apply it on a dot with a black background, it can also be seen visually and not only numerically. It follows the Gauss curve, and the curve is "cut" from your kernel box at k*Sigma: with same trend of gaussian curve, of course. This is valid conceptually, practically, on images, also a fixed kernel, with 1.0<k<2.0 is acceptable.

[theagentd]

The old source code is still listed in readme.me.