Noise Reduction changes

[Beep6581]

Originally reported on Google Code with ID 1756

1- From the File Browser, right-click on a photo, apply Neutral. Then set the following
Noise Reduction parameters still from the File Browser:
  [Directional Pyramid Denoising]
  Enabled=true
  Luma=70
  Ldetail=70
  Chroma=30
  Method=RGB
  Redchro=0
  Bluechro=0
  Gamma=1.3
2- Open the photo in the Image Editor. Click "1" to zoom to 100%. Take a screenshot
(PNG). Also process the raw, save as TIFF.
3- Disable Noise Reduction.
4- Enable Noise Reduction.
5- Take a screenshot, and again save the raw as TIFF.

The screenshots do not match! The first one has much more noise than the second one.
Same with the TIFF images, they match the preview but not each other. All of these
images should match since you didn't change anything.

Branch: default
Version: 4.0.9.232
Changeset: bc9f15710362
Compiler: gcc 4.6.3
Processor: undefined
System: Linux
Bit depth: 64 bits
Gtkmm: V2.24.2
Build type: release
Build flags:  -march=native -fopenmp -O3 -DNDEBUG
Link flags:   -march=native
OpenMP support: ON
MMAP support: ON

Reported by entertheyoni on 2013-03-04 19:02:20

[Beep6581]

Basically you are suggesting that successive uses of NR enable checkbox lead to a different
amount of NR?
Can you narrow it down-is it luminosity or chroma?

Reported by michaelezra000 on 2013-03-04 21:10:02

[Beep6581]

Yes. The difference appears to be in L only. I'm busy bisecting, turns out it's very
old.

Reported by entertheyoni on 2013-03-04 21:16:41

[Beep6581]

is it always increasing or always decreasing?
If yes, may be a scope of a variable is not set correctly.
If not always, may be it is variable scope relative to the omp block. 

Reported by michaelezra000 on 2013-03-04 21:23:39

[Beep6581]

Noise reduction strength always increases, and it does so only the first time you dis-
and re-enable NR, after that it stays strong.

Seems the bug has always been there and gone unnoticed, present since the first merge
of denoise with default, around 4.0.9.121.

Reported by entertheyoni on 2013-03-05 02:27:29

[Beep6581]

I found that this bug depends on a certain type of image. I tested over 10 different
types of photos, and the ones where I could see this both were underexposed and had
quite a low dynamic range.

Please try this image:
http://rawtherapee.com/shared/test_images/cactuses_noisy_longexp.pef

Reported by entertheyoni on 2013-03-07 17:04:13

[Beep6581]

http://rawtherapee.com/bugs/issue1740/rt_issue1756_denoise_1.jpg
http://rawtherapee.com/bugs/issue1740/rt_issue1756_denoise_2.jpg
No changes at all in RT tool settings. Big difference!

http://rawtherapee.com/bugs/issue1740/rt_issue1756_denoise_difference.jpg
(I boosted curves in the screenshot after taking it)

Reported by entertheyoni on 2013-03-07 17:16:27

[Beep6581]

With your file (pef), I can see the difference.

I'll try to find "why?"

:)

Reported by jdesmis on 2013-03-08 08:20:56

[Beep6581]

Tracked it down to wrong setting of expcomp.

In 'updatePreviewImage' in improccoordinator.cc there is a call to getAutoExp, which
results in setting an excomp-value (though the image doesn't have exposure-compensation),
which then is used in denoising. When you disable and enable, the expcomp-value wents
back to zero (which is correct). Hope that helps a bit.

Reported by heckflosse@i-weyrich.de on 2013-03-10 14:17:03

[Beep6581]

Addition: the expcomp-value is only set internally. You can't see it in the gui, only
by adding some debugging output.

Reported by heckflosse@i-weyrich.de on 2013-03-10 14:53:08

[Beep6581]

The use of the auto-exposure is to approximately center the histogram of the image,
so that the denoising will have a consistent effect on under- or over-exposed images
relative to neutrally exposed images (the assumption being that one will apply exposure
compensation during processing, but that takes place in the pipeline after NR; and
you probably don't want to reverse that ordering, since then every time you touch the
exposure controls or curves you would have to redo NR, and who wants to wait that long
;~)

If the NR tool didn't use auto-exposure on an underexposed raw, the NR would take place
on the uncompensated raw data (just after demosaic); the pixel values will all be in
the shadows, where NR effects are strongest, and the NR will be very heavy handed relative
to an image with the same NR settings but a properly centered histogram.

So if some series of actions in the GUI is resulting in a resetting of the expcomp
parameter, that is a mistake in the code -- in the improccoordinator section, not in
the NR tool.  I don't see why the expcomp parameter set by the getAutoExp method should
be reset by anything, since it should be determined by the raw data and not anything
happening later in the pipeline.

Reported by ejm.60657 on 2013-03-10 18:01:26

[Beep6581]

Maybe I was wrong in saying, what's correct or not. I only wanted to show where the
difference comes from. In fact, the dirpyrDenoise.expcomp is set to zero, when disabling
and enabling NR. 

Reported by heckflosse@i-weyrich.de on 2013-03-10 19:28:13

[Beep6581]

OK, maybe I was a little too fast in saying where the bug is in the code.  I just wanted
to emphasize that the answer is to prevent the expcomp variable used by NR from changing
its value from what it is when applying the autoexposure method to the raw data, rather
than disabling the exposure compensation done by the NR routine.  Could you identify
where the value is getting reset?

Reported by ejm.60657 on 2013-03-10 21:13:45

[Beep6581]

So we agree. I try to find it since some hours, using grep and reading code, but I need
some time. AFAIK, Jacques is also looking at it. But for sure, the error isn't inside
Denoise, because it gets its parameters from outside. 

BTW: wouldn't it be a good solution, to save the value of dirpyrDenoise.expcomp in
the pp3 and only recalculating it, when changing demosaic-method?

Ingo

Reported by heckflosse@i-weyrich.de on 2013-03-10 21:21:30

[Beep6581]

My good instinct says, the value is reset on disabling and enabling, because at enabling
only the stored parts are applied, but thats only good instinct...

Reported by heckflosse@i-weyrich.de on 2013-03-10 21:30:01

[Beep6581]

What's confusing me is that dirpyrdenoise.expcomp should never need to be recalculated.
 It should be calculated once when the file is opened and never again.  If you look
at how this parameter is calculated, it is done using the autoexposure histogram calculated
in rawimagesource.cc, which uses only the raw bayer data (not the demosaiced image
data), by calling the standard autoexposure method in improcfun.cc which uses this
histogram.  So clearly since the input is only the raw bayer data there is no reason
to keep recalculating it when the user touches any controls.

I am puzzled as to why and how it is being recalculated, or if it is not being recalculated,
why and how it is being reset to zero.

Reported by ejm.60657 on 2013-03-10 21:58:08

[Beep6581]

Ok, as it uses only the raw bayer data, there is no need to recalculate, even when demosaic
method changes, another reason to store it in pp3-file.
But there's no entry for dirpyrdenoise.expcomp in pp3, so at least it has to be recalculated
each time, the image is opened in RT, and it seems to be forgotten to recalculate at
one point, the one we search now.

Reported by heckflosse@i-weyrich.de on 2013-03-10 22:24:11

[Beep6581]

Emil please help me understand the logic of autoexposure for NR.

Noise depends on real exposure and raw data before any correction reflect this better
than exposure "corrected". Noise is more at the darks & shadows and decreases at the
more exposed areas.
If we increase exposure by autoexp function then the noisy dark areas became less dark
but the noise (SNR) is the same as before. So a corrected exposure is less indicative
for noise quantity and the related need for denoise.   

Reported by iliasgiarimis on 2013-03-11 02:37:05

[Beep6581]

Yes the noise depends on the actual exposure, and if everything were linear then making
the autoexposure correction would have no effect.  However, NR is carried out on the
image after applying a gamma correction according to the gamma slider.  Gamma is a
nonlinear transformation, and for it to have the intended effect consistently on the
image of stretching shadows and compressing highlights, the histogram should be centered.

Reported by ejm.60657 on 2013-03-11 03:04:16

[Beep6581]

Assigned an initial value in 'procparams.h' to DirPyrDenoiseParams.expcomp to see, where
the wrong value comes from (double expcomp = 0.4711;) (gives a lot of warnings at compile,
but doesn't matter). At testing I saw, that now this value is used after disabling
and enabling Denoise. So it seems, that it's not a reset of the value, but a new instance
is being created somewhere and the initial value is used then. But don't know, where
this occurs.

Ingo

Reported by heckflosse@i-weyrich.de on 2013-03-11 18:48:16

[Beep6581]

Re #16: since it's not a parameter but a computed value, autoExposure parameter's place
would be in the [LiveThumbData] section of the thumbnails's cached data file.

Reported by natureh.510 on 2013-03-11 23:40:49

[Beep6581]

I had the same thought after I wrote #16. But as expcomp is a value, which resides in
procparams.dirpyrDenoise, it's a bit more complicated...

Reported by heckflosse@i-weyrich.de on 2013-03-11 23:45:40

[Beep6581]

It looks like ImProcFunctions::getAutoExp could be a static function, so we could use
it from RawImageSource::load, and then store the dirpyrdenoiseExpComp parameter here,
once for all!? A simple getter would be enough then, used in improccoordinator.

Reported by natureh.510 on 2013-03-12 00:52:24

[Beep6581]

Is dirpyrdenoiseExpComp also necessary for StdImageSource?

Reported by natureh.510 on 2013-03-12 00:56:40

[Beep6581]

Perhaps less so, though you'll have to ask Jacques what he did to implement NR on non-raw
images.  Perhaps also relevant is issue 1746.

Reported by ejm.60657 on 2013-03-12 01:08:11

[Beep6581]

Here's a patch.
Made ImProcFunctions::getAutoExp a static function, but couldn't use it in RawImageSource::load,
because rawData array gets populated later in RawImageSource::copyOriginalPixels, which
is called from RawImageSource::preprocess. So I used it at the end of this function.
Added a new protected member to ImageSource (double dirpyrdenoiseExpComp) with a getter
function getDirPyrDenoiseExpComp ( ), which now is called at all needed places.

Probably not the best solution, but works fine here. I would be glad for a review of
my changes.

Ingo

Reported by heckflosse@i-weyrich.de on 2013-03-12 22:12:16

---

• Attachment: issue1740.patch

[Beep6581]

Emil and Hombre, thank you very much for your comments. They've been a great help, as
I was completely new to this part of RT source.

Ingo

Reported by heckflosse@i-weyrich.de on 2013-03-12 22:22:04

[Beep6581]

Reported by heckflosse@i-weyrich.de on 2013-03-13 12:04:48

[Beep6581]

I didn't tested your patch yet (not enough time), but i've had a look at it. Could you
please remove the expcomp parameter of the DirPyrDenoise class (in procparams) since
it's useless now?

Reported by natureh.510 on 2013-03-13 13:11:19

[Beep6581]

That would require that I

1.) pass expcomp as an additional argument to RGB_denoise (easy modification)
or
2.) find a simple way to call the new getter getDirPyrDenoiseExpComp() from within
RGB_denoise (actually don't know how, but would prefer this one)

Reported by heckflosse@i-weyrich.de on 2013-03-13 13:32:59

[Beep6581]

I don't hink that 2 is possible, so 1 is fine, isn't it?

Reported by natureh.510 on 2013-03-13 13:36:09

[Beep6581]

Ok, will do 1

Reported by heckflosse@i-weyrich.de on 2013-03-13 13:39:57

[Beep6581]

Here's the modified patch with the changes suggested by Hombre. Downside of the solution
is, that now the expcomp value is calculated even if RGBdenoise is deactivated (but
calculation takes less than 200 ms at my machine).

Reported by heckflosse@i-weyrich.de on 2013-03-13 17:33:17

---

• _Attachment: issue1756_2.patch_

[Beep6581]

Yes, but it's computed only once, whatever hiw much you'll play with the Raw tab's settings.

Thanks for the patch, i'll be able to test it in few hours.

Reported by natureh.510 on 2013-03-13 17:39:34

[Beep6581]

I meant downside of the patch, not of your suggestion. First version of the patch has
the same downside.

Unfortunately it's not computed once, as preprocess is called every time when changing
e.g. CA correction in raw tab. Thought about initializing the value to INFINITY and
only recalculate, when it's value is == INFINITY...

Reported by heckflosse@i-weyrich.de on 2013-03-13 18:25:48

[Beep6581]

Here's the INFINITY version

Reported by heckflosse@i-weyrich.de on 2013-03-13 19:12:02

---

• _Attachment: issue1756_3.patch_

[Beep6581]

Works fine, excepted for one thing: the "clip" parameter is used uninitialized in RawImageSource::preprocess
(was the case in improccoordinator.cc before too, and may be the bug we were looking
for).

Emil, what should be its value?

Reported by natureh.510 on 2013-03-13 20:38:57

[Beep6581]

What 'clip' parameter?  Can you point me to a line of code where it is used?

Reported by ejm.60657 on 2013-03-13 20:41:14

[Beep6581]

In   patched RT, line 1082 of rawimagesource.cc.
In unpatched RT, line  196 of improccoordinator.cc.

This parameter is used by getAutoExp.

Reported by natureh.510 on 2013-03-13 20:44:27

[Beep6581]

"clip" is used on line 3015 of improcfun.cc

Reported by natureh.510 on 2013-03-13 20:46:40

[Beep6581]

Line 3015 in improcfun.cc

Reported by heckflosse@i-weyrich.de on 2013-03-13 20:53:27

[Beep6581]

you've been faster

Reported by heckflosse@i-weyrich.de on 2013-03-13 20:53:54

[Beep6581]

I don't think, that's the bug we are looking for, but that doesn't mean, that using
an uninitialized value of clip is correct. It maybe an additional error. But maybe
I'm wrong.

Ingo

Reported by heckflosse@i-weyrich.de on 2013-03-13 21:04:52

[Beep6581]

OK, in unpatched RT it seems there is a bug in line 196 of improccoordinator.cc.

When ipf.autoExp is called in its standard usage in the pipeline on line 280, clip
is set to the user-chosen value in the interface.  This value is used in autoExp to
set a nominal white point as the point in the autoExp histogram that clips 'clip' fraction
of the pixels, ie 'clip' fraction of pixels lie above the nominal white point on the
histogram.

When ipf.autoExp is called on line 200, 'clip' has no set value, so when autoExp is
called we have the possibility of random behavior.  I think line 196 should initialize
'clip' to zero, ie

double clip=0;

and this should lead to more predictable behavior.  The result will be that the autoexposure
histogram will be using the highest pixel value in the raw data as the initial guess
for the white point.

Reported by ejm.60657 on 2013-03-13 21:15:17

[Beep6581]

Here's the modified patch which initializes the value of clip to zero.

Reported by heckflosse@i-weyrich.de on 2013-03-13 22:00:31

---

• _Attachment: issue1756_4.patch_

[Beep6581]

If uninitialized values are so wrong, which I also think is true, I'll post the output
of cppcheck about uninitialized vars in RT in a new Issue.

Ingo

Reported by heckflosse@i-weyrich.de on 2013-03-13 22:04:44

[Beep6581]

"issue1756_4.patch " builds and work fine.

Reported by natureh.510 on 2013-03-13 22:36:33

[Beep6581]

I confirm that the patch compiles and works great in Gentoo x86-64. Both the preview
and the saved image (tested TIFF 16-bit) work fine now. Thank you!

Reported by entertheyoni on 2013-03-13 23:25:11

[Beep6581]

Committed to default

Reported by heckflosse@i-weyrich.de on 2013-03-14 14:15:19

• Status changed: FixedPendingConfirmation

[Beep6581]

Reported by heckflosse@i-weyrich.de on 2013-03-16 13:54:18

• Status changed: Fixed