Bug in MSRLocal()

[heckflosse]

I found a bug in MSRLocal() here: https://github.com/Beep6581/RawTherapee/blob/dev/rtengine/ipretinex.cc#L1367

Have a look:

            float kval = 1.f;
#ifdef _OPENMP
            #pragma omp parallel for
#endif

            for (int y = 0; y < H_L; ++y) {
                for (int x = 0; x < W_L; ++x) {
                    float threslow = threslum * 163.f;

                    if (src[y][x] < threslow) {
                        kval = src[y][x] / threslow;
                    }
                }
            }

First bug is the parallelization races, so let's remove it:

            float kval = 1.f;
            for (int y = 0; y < H_L; ++y) {
                for (int x = 0; x < W_L; ++x) {
                    float threslow = threslum * 163.f;

                    if (src[y][x] < threslow) {
                        kval = src[y][x] / threslow;
                    }
                }
            }

What does this loop now? It sets kval to the value of the last pixel which is less then threslow to the value of the pixel divided by threslow If this is the intended behaviour (which I doubt), we can simply rewrite it this way, which is way faster:

        float kval = 1.f;
        bool found = false;
        const float threslow = threslum * 163.f;
        for (int y = H_L - 1; y >= 0 && !found; --y) {
            for (int x = W_L - 1; x >= 0 ; --x) {
                if (src[y][x] < threslow) {
                    kval = src[y][x] / threslow;
                    found = true;
                    break;
                }
            }
        }

But, as I doubt, that this is the intended behaviour, I want to know, what the intended behaviour is. @Desmis Jacques, can you enlighten me?

[Floessie]

BTW, what does "MSR" stand for here? This or this? :wink:

[heckflosse]

Multi Scale Retinex ?

[Floessie]

We can't know for sure. And that's the problem with TLAs (and too short abbreviations basically). :wink:

[Desmis]

MSR is Multi Scale Retinex...use since 5 or 6 years... or more ?? This name is not from me, but an "usual name" for "retinex"...to differentiate from "Original retinex" which is for "dodge and burn" (this is not the term used by Ipol) "retinex"==>retine==>eye perception

@heckflosse Ingo

You are true, I made another bullshit :) "kval" must be an array y, x

Thank you for your job :)

jacques

[heckflosse]

Here's a fix:

diff --git a/rtengine/ipretinex.cc b/rtengine/ipretinex.cc
index db01985b6..d65bf1cad 100644
--- a/rtengine/ipretinex.cc
+++ b/rtengine/ipretinex.cc
@@ -1354,32 +1354,19 @@ void ImProcFunctions::MSRLocal(int call, int sp, bool fftw, int lum, float** red
     }

     if (scal == 1) {//only if user select scal = 1
+        const float threslow = threslum * 163.f;

-        float kval = 1.f;
 #ifdef _OPENMP
         #pragma omp parallel for
 #endif

         for (int y = 0; y < H_L; ++y) {
             for (int x = 0; x < W_L; ++x) {
-                float threslow = threslum * 163.f;
-
-                if (src[y][x] < threslow) {
-                    kval = src[y][x] / threslow;
-                }
-            }
-        }
-
-
-#ifdef _OPENMP
-        #pragma omp parallel for
-#endif
-
-        for (int y = 0; y < H_L; ++y) {
-            for (int x = 0; x < W_L; ++x) {
-                float buf = (src[y][x] - out[y][x]) * value_1;
+                const float srcVal = src[y][x];
+                const float kval = rtengine::min(srcVal / threslow, 1.f);
+                float buf = (srcVal - out[y][x]) * value_1;
                 buf *= (buf > 0.f) ? lig : dar;
-                luminance[y][x] = LIM(src[y][x] + (1.f + kval) * buf, -32768.f, 32768.f);
+                luminance[y][x] = LIM(srcVal + (1.f + kval) * buf, -32768.f, 32768.f);
             }
         }

@Desmis Jacques, is this the intended behaviour now?

[Desmis]

@heckflosse Ingo

Yes it is :)

jacques

[heckflosse]

I will push the fix to the cppcheck_cleanups branch I'm on atm.