New noise reduction algorithm

[Beep6581]

Originally reported on Google Code with ID 1052

Purpose of code changes on this branch:
To address complaints on the effectiveness of luma NR

When reviewing my code changes, please focus on:
Whether it's better.  A few tweaks are still possible, in particular an attempt was
made to preserve detail; more NR can be had at the cost of softening detail more.

The new code requires a new library to be installed: fftwf3, a fast Fourier transform
utility.  You can obtain it from 

http://www.fftw.org/download.html

For OSX, there is an easy installation via MacPorts.  

Reported by ejm.60657 on 2011-10-16 16:43:38

• Blocking: #1307, #1130

---

• Attachment: FTdenoise-5.patch

[Beep6581]

So all you need is:
1. using Lab->RGB conversion method (any other one?)
2. having access to rgb_cam and cam_rgb in Improcfuncs, or maybe that would not be
necessary if the methods of point #1 above would be accessible from RawImageSource

Reported by natureh.510 on 2011-10-23 12:41:57

[Beep6581]

Just a suggestion:

colorSpaceConversion is the very last operation in RawImageSource::getImage, but it
operates on an converted image, and so will your denoise algorythm, so would it be
possible to transfer colorSpaceConversion to somewhere else or is it so tied to Rraw
files tht it have to be there? I think it would be counter productive to demozaice
the image again only because of the last color space conversion...

Also, wouldn't a newly created "Color" class interesting? This would handle color management/conversion
and be a parent class of RawImageSource and Image8/16. Just a thought.

Reported by natureh.510 on 2011-10-23 13:21:08

[Beep6581]

As I see it there are two options, I'm not sure which is best:

1. Put NR just before colorSpace conversion in RawImageSource::getImage, as outlined
in #51 above.  Image at this point is still in raw colors, so all we have to do is
transform to Lab since NR works on luma and chroma channels, not RGB.  For this, RawImageSource
needs access to methods Lab2XYZ and XZY2Lab in ImProcFunctions (which themselves depend
on the inline function f2xyz in improcfun.h) as well as the NR algo, which should remain
in ImProcFunctions in order to be available to non-raw images (or maybe we make it
its own class?).  RawImageSource also needs to know about the exposure compensation
slider setting so that the image is properly normalized (the sliders relate to the
amount of noise relative to 65535).  Similarly StdImageSource would need the same access,
with the NR again located just before color space conversion in getImage (here of course
it's less crucial since there is no "raw color space", but we need NR to be available
for non-raw images).

2. Keep NR where it is in the pipeline.  Now we need to go back to raw colors, if the
image is a raw image.  For this one needs ImProcFunctions to know about the matrices
cam_xyz and its inverse xyz_cam in order to go back and forth; we want something where
these are NULL if the image is non-raw, and defined and accessible to ImProcFunctions
if the image is raw, so that we can go back to raw colors, and then return to the working
space.  I'm not sure how this version will perform (though my test example worked this
way), at this point in the chain one has already done some nonlinear tonal manipulations
(contrast, saturation, user tone curve etc) and I don't know whether these are better
done before or after NR.  If better done after, we could in this scenario move NR into
ImProcFunctions::rgbProc just after EC and perhaps black point operations.  We probably
also need to copy RawImageSource::inverse33 into ImProcFunctions, or else make it available
from the latter location.

As far as performance goes, demosaic is only performed if M_RAW bit of todo is set;
getImage is performed if M_INIT bit is set, so I think one can distinguish that demosaic
is not necessary if NR is in getImage.

Reported by ejm.60657 on 2011-10-23 14:31:31

[Beep6581]

There are a number of places we do color transformations, at the input stage in RawImageSource
to go to the working space, in ImProcFunctions to go from working space to Lab, and
output stage to go from Lab to monitor space or output space.  There is some entanglement
with rtgui here in that the navigator pane displays info about working space RGB as
well as HSV and Lab values.  Organizing these all into a Color class might be a sensible
refactoring of the code.

Reported by ejm.60657 on 2011-10-23 14:35:23

[Beep6581]

I would definately vote for solution #1. I can work on this now (was quite busy before).

Reported by natureh.510 on 2011-10-27 20:21:46

[Beep6581]

I agree that #1 is probably cleaner from the image processing point of view -- take
the image just after conversion and denoise it, then apply any tonal manipulations
like saturation, contrast, local contrast etc to the result.

So for this we need that RawImageSource can access functions defined in ImProcFunctions.
 Or maybe you see a better way?  Perhaps we can continue the discussion offline via
email.

Reported by ejm.60657 on 2011-10-27 20:49:13

[Beep6581]

> Olli, you had a better luck compiling x64 dependencies before, if you find some 
> time, could you take a look?
Hi Michael, just tried it with various configure options: No luck getting fftw compiled
on 64bit Windows with latest gcc, though I got different err messages.
:-(

Reported by oduis@hotmail.com on 2011-10-30 11:13:07

[Beep6581]

Thanks for trying. I wonder if the x64 dlls provided on that site could be used for
RT compilation directly? 

I created a simple C:\MinGW64\lib\pkgconfig\fftw3f.pc file manually based on lcms2.pc
but that did not help either.

Reported by michaelezra000 on 2011-10-30 14:44:18

[Beep6581]

Some DLLs (when generated by MinGW itself) can be reference directly it seems. Maybe
worth a try, since the official DLLs on their website are built by MinGW64 I think,
so maybe directly linkable.

Reported by oduis@hotmail.com on 2011-10-30 15:22:46

[Beep6581]

I assume you have gone through the documentation on their site for Windows compilation?

Reported by ejm.60657 on 2011-10-30 15:41:48

[Beep6581]

I found that outdated.

Reported by oduis@hotmail.com on 2011-10-30 15:50:50

[Beep6581]

Thinking the other way around worked on Win64: The official DLL is compiled with Mingw64
(however they managed that), so the necessary compile libraries could be retro-generated
from that. Might also be better since we can use the officially supported DLLs.

It compiles and works here, however it’s a bit hacky and not really clean, so if someone
manages to compile it the regular way on MinGW64 it would be nicer.

Reported by oduis@hotmail.com on 2011-10-31 13:00:05

• Labels added: 4.0, Component-Quality

[Beep6581]

Compiles fine on that revision, but can't effectively test it because of purple squares
and black/white bars of noise:
http://i.imgur.com/1k0BI.jpg

Most of the time the luma sliders produce the black/white pattern, while the chroma
slider produces the magenta squares.

Reported by entertheyoni on 2011-11-09 02:09:40

• Status changed: Started

[Beep6581]

DrSlony, can you try to disable OMP for NR quality testing?

Reported by michaelezra000 on 2011-11-09 03:03:26

[Beep6581]

If someone has the same trouble compiling fftw on 64bit for this patch (Michael, did
you manage to compile already?):

1. Download the official fftw64 DLL package, unpack it somewhere you can reach it with
MSYS
2. in MSYS command line, do:
dlltool --def libfftw3f-3.def --dllname libfftw3f-3.dll --output-lib libfftw3f-3.a
3. copy the libfftw3f-3.dll MinGW64/bin and the newly generated .a file to MinGW64/lib,
the .h file to MinGW64/include
4. Create a new textfile MinGW64/lib/pkconfig/fftw3f.pc and enter:
prefix=/gcc/mingw64
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
includedir=${prefix}/include

Name: fftw3f
Description: FFTW3 Float
Version: 3.3
Libs: ${libdir}/libfftw3f-3.a
Cflags: -I${includedir}

Of course change the first line to the full patch of your mingw64 installation.

Reported by oduis@hotmail.com on 2011-11-09 20:56:05

[Beep6581]

Thanks, Olli, I will give this a try!

Reported by michaelezra000 on 2011-11-09 21:33:13

[Beep6581]

Hombre correct, works fine with -DOPTION_OMP=OFF
Functionally it seems to work well too, but I'll leave proper functionality testing
for when openmp support is fixed, as RT is painfully slow to use without openmp :]

(imagine one day when we have GPU processing support, and I'll say RT is painfully
slow with just openmp enabled)

Reported by entertheyoni on 2011-11-09 22:17:57

[Beep6581]

*Michael not Hombre

Reported by entertheyoni on 2011-11-09 22:18:30

[Beep6581]

grr. this patch does not apply cleanly anymore against current source tree.
will look into the OMP bug and try to fix it.

Reported by janrinze on 2011-11-09 22:22:30

[Beep6581]

I'm working on the patch to resynch it (done). Unfortunately, RT crash when i ask for
a 1:1 scale preview.

Reported by natureh.510 on 2011-11-09 22:32:15

[Beep6581]

natureh: can you upload the patch here too?

Reported by janrinze on 2011-11-09 23:20:35

[Beep6581]

First tests with NR before color conversion are encouraging, quality of NR is better,
however current code seems to tilt the WB a little greenish.

At the moment the NR is a hybrid -- luma NR is done by the new method, chroma NR by
the old method.  I will try and see if the new method is an improvement on chroma NR
as well.

Reported by ejm.60657 on 2011-11-09 23:36:21

[Beep6581]

Of course, here is it (it's still not working here, but maybe that the culprit is my
build of the fftw lib). I'd like to work more on it, but i don't have the time right
now. So feel free to work on it if you want.

I've corrected the openmp statments, but it might not be sufficient. Furthermore, i've
create a Slicer class to help in parallelizing the code by providing a list of block.
It may have been simpler to use it, dunno.

The provided patch (almost) disable the enhancement of issue 1103, so if there's bug
left, it shouldn't be there.

Good night.

Reported by natureh.510 on 2011-11-10 00:12:25

---

• Attachment: FTdenoise-16-Hombre.patch

[Beep6581]

Just a very quick 1 minute review. Applies to the latest code and compiles fine. No
artifacts in the preview with openmp enabled! Though I have to manually click on the
preview to update it, as moving the sliders makes the progress bar quickly zoom by
with no visible change to the preview, until I click on it, so it gets reprocessed
from the lowres one again.

ps. I'm using fftw-3.2.2.

Reported by entertheyoni on 2011-11-10 00:46:27

[Beep6581]

patch applies ok, builds and runs too.
The results are somewhat disappointing, the images have a large amount of green cast
after noise reduction.
in 1:1 view the preview does not get updated when settings are changed.
I will have a look ate the code but cannot promise to make any great contributions
soon.

Reported by janrinze on 2011-11-10 19:45:48

[Beep6581]

Found the fix for the greenish tinge, a bug in the function XYZ2Lab in rtengine/color.cc.
 Here is the correct function:

void Color::XYZ2Lab(float X, float Y, float Z, float &L, float &a, float &b) {

    float X1 = X/D50x;
    float Z1 = Z/D50z;

    float fx = (X1<65535.0 ? cachef[X1] : (327.68*exp(log(X1/MAXVAL)/3.0 )));
    float fy = (Y<65535.0 ? cachef[Y] : (327.68*exp(log(Y/MAXVAL)/3.0 )));
    float fz = (Z1<65535.0 ? cachef[Z1] : (327.68*exp(log(Z1/MAXVAL)/3.0 )));

    L = (116.0 * fy - 5242.88); //5242.88=16.0*327.68;
    a = (500.0 * (fx - fy) );
    b = (200.0 * (fy - fz) );

}

Reported by ejm.60657 on 2011-11-11 05:18:22

[Beep6581]

This version adds Emils changes, plus fixes on crash when opening detail window and
a bug in HL reconstruction (hopefully ;-).

Reported by oduis@hotmail.com on 2011-11-11 10:43:35

---

• Attachment: FTdenoise-17-Olli.patch

[Beep6581]

I applied your patch Oliver, but it still crash here when opening a detail window. What
correction did you do to correct it?

Reported by natureh.510 on 2011-11-11 21:01:12

[Beep6581]

It exactly crash on line 312 of FTblockDN.cc :
            fftwf_execute_dft_r2c(plan_forward_blox,Lblox[vblkmod],fLblox[vblkmod]);// FT an
entire row of tiles

Is it okay for you? I have OpenMP enabled but the parallelisation has been removed.
Should i rebuild fftw with multi-threading disabled?

Reported by natureh.510 on 2011-11-11 21:04:39

[Beep6581]

I suspect it is not necessary to enable OpenMP in the building of FFTW itself; the NR
code processes the image in tiles, so we can efficiently multithread with OpenMP pragmas
in the RT code rather than in the execution of the libs.  

Reported by ejm.60657 on 2011-11-11 21:20:51

[Beep6581]

The bug that crashed on me was that there was a pre-allocated lab image used as temp
between the two denoise calls. However the width/height of the RGB and the LAB is a
bit different (dangerous architecture, hard to find). So it went over the array border
and crashed.

The second bug was the i2 injections, it was missing in the ..i2<xx;i2++).

This one seems to be new, doesn't crash here. However I used the offical fftwf DLL,
since I did not manage to built it myself on 64bit.

Reported by oduis@hotmail.com on 2011-11-11 21:26:55

[Beep6581]

#75 #76 no preview update. Looks same as issue 1031.

Reported by GreatBull69 on 2011-11-12 01:56:20

[Beep6581]

Here is the corrected patch: now the sliders take effect correctly. It took me a while
to realize that you voided the dirpyrdenoise method, that's why the preview wasn't
updated responding.

Since you voided this method, i removed the DIRPYRDENOISE enum in refreshmap.h which
is now unused, removed the call to the dirpyrdenoise method too. I also corrected a
memory leak: the temporary LabImage was not freed (dcrop.cc).

I moreover moved the color conversion from the Navigator class to the Color class (missed
it in the previous code refactoring).

The second good news is that it doesn't crash anymore on my system when opening a Detail
window :). I didn't found the culprit, as it was still crashing with the prebuild binaries
converted with Oduis' method.

Regarding the image quality... i'm a bit disappointed of the result with high iso or
underexposed images. I'll upload some critical files to test your algo, with which
i didn't get pleasant result, especially when playing with the Gamma slider!

Reported by natureh.510 on 2011-11-13 13:08:20

---

• Attachment: FTdenoise-18-Hombre.patch

[Beep6581]

I forgot to mention: the parallelisation is still disabled, we'll look at it when the
algorithm will be stabilized.

Reported by natureh.510 on 2011-11-13 13:11:35

[Beep6581]

Hombre, how can I merge in your changes without clobbering everything I've been doing
since I uploaded my last patch?

Reported by ejm.60657 on 2011-11-13 14:29:34

[Beep6581]

You can send me your actual patch (updated to TIP revision) and let me do the merge,
but don't touch it until i send it you back in 1 or 2 hour...

Reported by natureh.510 on 2011-11-13 14:39:28

[Beep6581]

Ok, it's 7 hours... but here is the patch. RT is still crashing here when using fftw,
so i coudn't have test it :(

Emil, just contact me if you want help about the cmakefile problem. An IRC chat would
help.

Reported by natureh.510 on 2011-11-13 21:52:37

---

• Attachment: FTdenoise-20-Hombre.patch

[Beep6581]

Yes, the patch gets bigger and bigger, it's because i've modified identation of code
as usual... You're of course free to trash those modifications but i would appreciate
if you keep it.

Reported by natureh.510 on 2011-11-13 21:55:11

[Beep6581]

How is progress on this?

Reported by entertheyoni on 2011-11-29 20:55:58

[Beep6581]

Sorry, I did not yet get a chance to test this.
Emil, will you be using FFT or EPD going forward?

Reported by michaelezra000 on 2011-11-29 20:59:25

[Beep6581]

Reported by entertheyoni on 2011-11-29 21:02:35

• Blocking: #130

[Beep6581]

It was waiting for EPD to finalize; now it will wait for RT to work for me again.  After
that there is much work to do.

Reported by ejm.60657 on 2011-11-29 21:17:33

[Beep6581]

Michael, it will use both, if it works at all.  EPD will be used to construct a high
pass filter, then FT denoise applied to that.

Reported by ejm.60657 on 2011-11-29 21:21:58

[Beep6581]

Here is a comparison of noise reduction in Lightroom and RT with a crop of a noisy flashless
indoor picture (NEX-5) with the adjustments as screen shots next to the result.
Unfortunately, LR comes out ways better. This ranges from the very first stage without
any reduction (why is that?) to maximal reduction. Smearing seems more obvious in LR,
but does not harm too much. It could be that “color” in LR (does this correspond to
“chrominance” in RT?) is much more efficient.

Reported by Bernhard.Ruthensteiner@gmx.net on 2011-12-08 12:04:15

---

• _Attachment: noisered_LRvsRT.PNG
  _

[Beep6581]

Olli, your instructions were most helpful! I was able to reach 100% in compilation all
the way up to linking:

[100%] Building CXX object rtgui/CMakeFiles/rth.dir/sharpenmicro.cc.obj
Linking CXX executable rawtherapee.exe
..\rtengine\librtengine.a(FTblockDN.cc.obj): /rtengine/FTblockDN.cc:200: undefined
reference to `__imp_fftwf_malloc'

some kind of little thing is missing...

Reported by michaelezra000 on 2011-12-25 01:41:03

[Beep6581]

Morning Michael,
I remember I had problems linking, too. In my case there was trouble with CMAKE which
would not pick up the new linker flags, for whatever reason. I edited the link.txt
(I think it was that one, just search for it in the rtengine directory of CMAKE RELEASE)
manually with the new link setting and it worked.

Reported by oduis@hotmail.com on 2011-12-25 10:19:59

[Beep6581]

Thanks, Olli. 
After running the cmake all link.txt files are re-written. 
So, I ran cmake, then changed the file rtgui/CMakeFiles/rth.dir/link.txt 
and after C:\MinGW64\lib\libtiff.dll.a 
inserted C:\MinGW64\lib\libfftw3f-3.a 

then ran "mingw32-make -j2 install"
and got new rawtherapee.exe !

Probably .pc file needs some adjustment to avoid this workaround.

Reported by michaelezra000 on 2011-12-25 14:11:13

[Beep6581]

found some info here: http://gnuradio.org/redmine/projects/gnuradio/wiki/MingwInstallMain
 (search for FFTW)

Here are the updated instructions that should not require further tweaking (on Winx64):

1. Download the official fftw64 DLL package from http://www.fftw.org/download.html,
   unpack it somewhere you can reach it with MSYS
   (Hint: in MSYS console to change directory to 'C:/DirName' execute 'cd /C/DirName')

2. in MSYS command line, execute:
   dlltool --def libfftw3f-3.def --dllname libfftw3f-3.dll --output-lib libfftw3f-3.a
   dlltool --def libfftw3l-3.def --dllname libfftw3l-3.dll --output-lib libfftw3l-3.a
   dlltool --def libfftw3-3.def --dllname libfftw3-3.dll --output-lib libfftw3-3.a

   This will generate  generate 'libfftw3f-3.a.a' file

3. copy files:
   libfftw3f-3.dll -> MinGW64/bin 
   libfftw3l-3.dll -> MinGW64/bin
   libfftw3-3.dll  -> MinGW64/bin
   libfftw3f-3.a.a -> MinGW64/lib
   fftw3.f.h       -> MinGW64/include

4. Create a new textfile 
   MinGW64/lib/pkgconfig/fftw3f.pc 
   with the following contents:

prefix=/mingw64
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
includedir=${prefix}/include

Name: fftw3f
Description: FFTW3 Float
Version: 3.3
Libs: -L${libdir} -lfftw3f-3 -lm
Cflags: -I${includedir}

Reported by michaelezra000 on 2011-12-25 15:36:14

[Beep6581]

For convenience, download 'fftw-3.3_mingw64_install' from
http://min.us/mbq1sfUTan  unpack into MinGW64 directory

Reported by michaelezra000 on 2011-12-25 15:44:52

[Beep6581]

Hi Emil, for reference, here are some nice illustrations of various aspects of NR (of
"competition"), 
http://www.isl.co.jp/SILKYPIX/english/features/noise.html

Reported by michaelezra000 on 2012-02-01 16:09:25