Rawtherapee: HL reconstruction sometimes not enabled

[Floessie]

Hi fellows,

Today I had a RT session with 156cb762ec08bb2af8840d7034bd541bc8a32526. Using the default profile, for some images the HL reconstruction wasn't active when first opening them, and I had to activate it manually.

Do you experience the same? Could have slipped in lately...

Best,
Flössie

[heckflosse]

@Floessie That's normal behaviour.
https://github.com/Beep6581/RawTherapee/blob/dev/rtengine/improccoordinator.cc#L277

[Floessie]

Thanks for the hint, Ingo.

Hm, then the decision whether to show it or not is broken. The last image I worked with had a completly burnt sky showing a magenta cast. I wish I could provide you with this one, but it's a portrait and not taken by me...

Until today, HLR was always enabled, I really can't recall a shot where it wasn't. Not that I always overexpose (rather the contrary), but there seem to be spots that clip all the time.

Best,
Flössie

PS: Next time I encounter an innocuous shot showing this behavior, I'll post it instantly. 😄

[heckflosse]

@Floessie As a workaround just enable hl reconstruction for your default profile ;-)

[heckflosse]

@Floessie After looking at the code I think you found a real piece of bullshit in the code, which absolutely makes no sense and is faulty too.

I will take a closer look now and write down my findings in a following post.

[heckflosse]

@Floessie

Here my findings:

1. the function HLReconstructionNecessary not only returns true if highlights are clipped. It also returns true when shadows are clipped. Why?

2. why is this function in procparams.cc? That does not make sense at all.

3. the function returns true if 50 pixels of a channel are clipped. Why 50? Why independant on image size? Makes no sense.

We really should review this part of the code...

[Floessie]

Ingo, thanks for your investigation and the Torvalds-like tone. 😆

Either the code should do what it promises, or it should be flushed down the toilet. procparams.cc may have been a good idea once, nowadays I'd expect it to reside in rtgui next to the place of its usage...

Glad you found it (and I wasn't completely wrong when reporting)!

Best,
Flössie

[heckflosse]

@Floessie

I'd expect it to reside in rtgui

I disagree, because it's also used in simpleprocess which is also used in rt-cli, which has no gui ;-)

[heckflosse]

@Floessie This patch removes the shitty stuff

diff --git a/rtengine/improccoordinator.cc b/rtengine/improccoordinator.cc
index 765c4b70..87a039a7 100644
--- a/rtengine/improccoordinator.cc
+++ b/rtengine/improccoordinator.cc
@@ -199,11 +199,7 @@ void ImProcCoordinator::updatePreviewImage (int todo, Crop* cropCall)
         imgsrc->preprocess ( rp, params.lensProf, params.coarse );
         imgsrc->getRAWHistogram ( histRedRaw, histGreenRaw, histBlueRaw );

-        if (highDetailNeeded) {
-            highDetailPreprocessComputed = true;
-        } else {
-            highDetailPreprocessComputed = false;
-        }
+        highDetailPreprocessComputed = highDetailNeeded;
     }

     /*
@@ -269,21 +265,6 @@ void ImProcCoordinator::updatePreviewImage (int todo, Crop* cropCall)
         }
     }

-
-    // Updating toneCurve.hrenabled if necessary
-    // It has to be done there, because the next 'if' statement will use the value computed here
-    if (todo & M_AUTOEXP) {
-        if (params.toneCurve.autoexp) {// this enabled HLRecovery
-            if (ToneCurveParams::HLReconstructionNecessary (histRedRaw, histGreenRaw, histBlueRaw) && !params.toneCurve.hrenabled) {
-                // switching params.toneCurve.hrenabled to true -> shouting in listener's ears!
-                params.toneCurve.hrenabled = true;
-
-                // forcing INIT to be done, to reconstruct HL again
-                todo |= M_INIT;
-            }
-        }
-    }
-
     if (todo & (M_INIT | M_LINDENOISE | M_HDR)) {
         MyMutex::MyLock initLock (minit); // Also used in crop window

diff --git a/rtengine/procparams.cc b/rtengine/procparams.cc
index 80041e70..e1d62bb9 100644
--- a/rtengine/procparams.cc
+++ b/rtengine/procparams.cc
@@ -357,22 +357,6 @@ bool ToneCurveParams::operator !=(const ToneCurveParams& other) const
     return !(*this == other);
 }

-bool ToneCurveParams::HLReconstructionNecessary(const LUTu& histRedRaw, const LUTu& histGreenRaw, const LUTu& histBlueRaw)
-{
-    if (options.rtSettings.verbose) {
-        printf("histRedRaw[  0]=%07d, histGreenRaw[  0]=%07d, histBlueRaw[  0]=%07d\nhistRedRaw[255]=%07d, histGreenRaw[255]=%07d, histBlueRaw[255]=%07d\n",
-                histRedRaw[0], histGreenRaw[0], histBlueRaw[0], histRedRaw[255], histGreenRaw[255], histBlueRaw[255]);
-    }
-
-    return
-        histRedRaw[255] > 50
-        || histGreenRaw[255] > 50
-        || histBlueRaw[255] > 50
-        || histRedRaw[0] > 50
-        || histGreenRaw[0] > 50
-        || histBlueRaw[0] > 50;
-}
-
 RetinexParams::RetinexParams() :
     enabled(false),
     cdcurve{
diff --git a/rtengine/procparams.h b/rtengine/procparams.h
index 7cc80b31..b3f13d00 100644
--- a/rtengine/procparams.h
+++ b/rtengine/procparams.h
@@ -286,7 +286,6 @@ struct ToneCurveParams {
     bool operator ==(const ToneCurveParams& other) const;
     bool operator !=(const ToneCurveParams& other) const;

-    static bool HLReconstructionNecessary(const LUTu& histRedRaw, const LUTu& histGreenRaw, const LUTu& histBlueRaw);
 };

 /**
diff --git a/rtengine/simpleprocess.cc b/rtengine/simpleprocess.cc
index 02760030..ebb59159 100644
--- a/rtengine/simpleprocess.cc
+++ b/rtengine/simpleprocess.cc
@@ -200,16 +200,6 @@ private:
         imgsrc->setCurrentFrame (params.raw.bayersensor.imageNum);
         imgsrc->preprocess ( params.raw, params.lensProf, params.coarse, params.dirpyrDenoise.enabled);

-        if (params.toneCurve.autoexp) {// this enabled HLRecovery
-            LUTu histRedRaw (256), histGreenRaw (256), histBlueRaw (256);
-            imgsrc->getRAWHistogram (histRedRaw, histGreenRaw, histBlueRaw);
-
-            if (ToneCurveParams::HLReconstructionNecessary (histRedRaw, histGreenRaw, histBlueRaw) && !params.toneCurve.hrenabled) {
-                params.toneCurve.hrenabled = true;
-                // WARNING: Highlight Reconstruction is being forced 'on', should we force a method here too?
-            }
-        }
-
         if (pl) {
             pl->setProgress (0.20);
         }

[Floessie]

@heckflosse What a relief! 😂 👍