Increase speed of noise reduction preview when Auto Chroma is selected, and fix some bugs in noise reduction

[Beep6581]

Originally reported on Google Code with ID 2557

Noise reduction preview rarely uses all available cores because the tile size is very
large. For example on my screen in full screen preview it uses only 3 of 8 cores. I
don't want to change the tile size for several reasons but made some first tries using
nested parallel regions. This way we can increase the number of cores used in NR preview
and reduce processing time. I'll post a first patch when Issue 2495 is committed.

Ingo

Reported by heckflosse@i-weyrich.de on 2014-11-05 14:07:50

[Beep6581]

I decided to cover some Auto chroma method 'Automatic global' issues first.
When 'Automatic Global' is activated, it gets called each time I pan around or open
a new detail window. That means each pan has an additional delay of about 300 ms (for
a D800 NEF) at my system, which isn't the slowest one. It should only get called, when
something was changed, which has influence on these values (raw preprocessing, demosaic
method, maybe some other things)

First patch follows soon.

Ingo

Reported by heckflosse@i-weyrich.de on 2014-11-05 18:58:36

[Beep6581]

Here's the first patch. Before patch auto chroma 'preview mode' was executed not only
for the main preview but also for the before preview in before/after mode and for each
detail window. Fixed. Additionally there was a bug in history which caused that before/after
didn't work correctly with denoise. Fixed.
Next patch will include the changes for the auto chroma 'Automatic' mode. I'll move
that code from dcrop.cc to improccoordinator.cc.

Ingo

Reported by heckflosse@i-weyrich.de on 2014-11-09 17:37:45

• Status changed: PatchSubmitted

---

• _Attachment: issue2557_00.patch_

[Beep6581]

Another thing I don't understand: In line 768 of patched dcrop.cc (may be different
line in unpatched) the decision whether single or double precision ciecam02 shall be
used depends on the new 'expert mode'. Was that intended, Jacques?

Ingo

Reported by heckflosse@i-weyrich.de on 2014-11-09 20:40:36

[Beep6581]

I changed the thing I mentioned in #3. I didn't move the Auto Chroma global and auto
chroma multizone from dcrop.cc to improccoordinator.cc yet, but will do that with one
of the next patches. But I did the changes mentioned in #2 for this modes too. Also
included another correction for history. There a still a lot of bugs to be fixed.

Ingo

Reported by heckflosse@i-weyrich.de on 2014-11-09 23:03:07

[Beep6581]

There is a bug in last patch. I'll post a new patch tomorrow...

Reported by heckflosse@i-weyrich.de on 2014-11-10 00:29:02

[Beep6581]

Hello Ingo

for #3, it is a bug (for me...another reason not to drink Guiness !!)

You must replace (please do it with your patch) :
            if(settings->leveldnautsimpl==1) {
by
            if(settings->ciecamfloat) {

:)

Reported by jdesmis on 2014-11-10 06:26:15

[Beep6581]

https://www.youtube.com/watch?v=TOnOamDXasE

Reported by entertheyoni on 2014-11-10 08:52:13

[Beep6581]

Jacques: I'll correct that with my next patch ;-)

Reported by heckflosse@i-weyrich.de on 2014-11-10 18:18:37

[Beep6581]

Here's the next patch. I corrected #6, solved a race condition in dcrop.cc and added
a missing history message. Jacques, can you have a look at the things I changed to
solve the race condition? 
dcrop.cc 

line 412
line 443 to 445
line 450
line 503

Reported by heckflosse@i-weyrich.de on 2014-11-11 16:52:09

---

• _Attachment: issue2557_02.patch_

[Beep6581]

The same race condition is in simpleprocess.cc
I'll fix it when I have your OK to the solution of last patch, Jacques

Reported by heckflosse@i-weyrich.de on 2014-11-11 17:38:30

[Beep6581]

Jacques, another question:

Was there a special reason for choosing 3x3 tiling in 'Automatic global'?

9 tiles don't scale good on modern architectures. Here's a table which compares 3x3
to 9x9 tiling performance-wise:
Values in table are % processing time compared to one core (I assume tiles are not
overlapping in 'Automatic global')

cores      1       2       4       8      12       16
tiling
--------------------------------------------------------
3x3       100    55,55   33,33   22,22   11,11   11,11
9x9       100    50,61   25,92   13,58    8,64    7,40

Ingo

Reported by heckflosse@i-weyrich.de on 2014-11-11 18:33:59

[Beep6581]

Ingo

I just tested...No problem for me :)

Why 9 for "automatic global"...why not ?

If you want to replace 3x3 by 9x9 no problem... be carefull with small images to overlap
with big size of "cells"

Thank you for this enhancement
:)

Reported by jdesmis on 2014-11-12 07:10:22

[Beep6581]

Here's the next patch. Following things are fixed.

1.) memory leak when "Luminance" and "Chrominance - Master" were set to 0 and median
was disabled
2.) memory leak in simpleprocess.cc when using 'auto chroma multi zones'
3.) a crash which can be reproduced in vanilla by this steps:
    open image, apply neutral, zoom to 100%, enable nr, set chroma curve to linear,
set luma mode to slider
4.) when "Luminance" and "Chrominance - Master" were set to 0 and median was disabled,
NR was skipped in past, even when curves have been set to non linear.
5.) some divisions by zero

There are still some things to fix but I would like to commit this patch first, because
it fixes some memory leaks and a crash.

Ingo

Reported by heckflosse@i-weyrich.de on 2014-11-12 19:33:55

---

• _Attachment: issue2257_03.patch_

[Beep6581]

I changed the summary because the original issue mutated to a bug fix issue

Reported by heckflosse@i-weyrich.de on 2014-11-12 19:58:23

[Beep6581]

I confirm #13.3
I cannot reproduce #13.4, as when I set all sliders to 0 and all curves to max, then
patched vs unpatched does not differ (unpatched is tip + 2555local5). In both cases
I see luma noise has been reduced and chroma noise not.

Reported by entertheyoni on 2014-11-12 21:20:59

[Beep6581]

Hi Ingo, on 13.4: The chroma curve originally meant to modulate the action of Chroma
sliders. May be Jacques could add if it would make sense to allow the curve to act
independently

Reported by michaelezra000 on 2014-11-12 21:45:40

[Beep6581]

Isn't modulating a value of zero also modulating?

Reported by heckflosse@i-weyrich.de on 2014-11-12 21:51:02

[Beep6581]

:)

Reported by michaelezra000 on 2014-11-12 22:02:12

[Beep6581]

This is patch 3 with fixes for language strings in default.

Michael: as I came to realize, the luma curve should be independent because it is more
predictable to use just the curve while leaving the slider at 0, whereas in the case
of chroma it is not so, because "auto chroma" controls just the sliders (in a very
good way), in which case it is convenient to modulate them using the curve (in a multiplicative
way, not additive). If chrominance master is close to 0, changing the curve should
have no effect. That is the way it currently is, and I think it works well.

Reported by entertheyoni on 2014-11-12 22:45:41

---

• Attachment: 2557nr4.txt

[Beep6581]

But actually it's an additive way... And I won't change that

Reported by heckflosse@i-weyrich.de on 2014-11-12 22:51:54

[Beep6581]

Hello Ingo
Thank you for this work, that solve many problems...No crash

Michael
We cannot easily modified Chroma curve, and I think it work well as that.

Ingo
I think you can commit this change :)

Reported by jdesmis on 2014-11-13 06:23:07

[Beep6581]

I added #10 and committed to revision 3194b730cacd

Reported by heckflosse@i-weyrich.de on 2014-11-13 11:25:49

[Beep6581]

Here's the next patch.

1.) solves a bug which can be reproduced before patch as follows:
    Start RT
    Apply Default Iso High to an image
    Send the image twice to the queue
    Start the queue
    As you can see, the auto chroma values 'SIMPL...' in verbose output are not the
same
    (Hint for Jacques: The bug was caused by declaring bool perf in improcfun.h)
2.) I merged the two classes NoisCurve and NoisCCcurve into one class NoiseCurve and
made some changes to this class to get rid off all the 'ccdenoiseutili' and 'lldenoiseutili'
stuff.
3.) made a very small speedup (boxblur.h)
4.) removed some unused variables etc.

Jacques, please review the changes :-)
Again I would like to commit this patch before I proceed, because it fixes a bug.

Ingo

Reported by heckflosse@i-weyrich.de on 2014-11-14 22:48:43

---

• _Attachment: issue2557_05.patch_

[Beep6581]

Ingo, I am curious if #1 also solves the bug that when auto chroma is selected and use
would reset the chroma slider value repeatedly, it would get auto re-calculated to
different values. On a side note, actually points to one more bug, but in GUI that
sliders are sometimes enabled and sometimes not when auto chroma is selected.

Reported by michaelezra000 on 2014-11-14 23:18:46

[Beep6581]

Michael, I thought my English is not so bad, but this time I don't understand what you
mean...

Reported by heckflosse@i-weyrich.de on 2014-11-14 23:29:32

[Beep6581]

Perhaps a more detailed instruction how to reproduce the behaviour you mentioned, could
help me to understand ;-)

Reported by heckflosse@i-weyrich.de on 2014-11-14 23:36:34

[Beep6581]

I recall, that yesterday in IRC DrSlony asked me, why I didn't mention that the disabling
of the sliders doesn't work before commit in #23, but works after commit. I told him,
I don't know, but I tested before and after patch and before it didn't work and after
it did work. But I actually don't know why...

Reported by heckflosse@i-weyrich.de on 2014-11-14 23:40:15

[Beep6581]

Hi Ingo, sorry there are a few typos in my comment. Here are the steps:
1. Open image with neutral profile, zoom to 100%
2. Enable NR
3. Set chroma NR to Auto
4. Reset the chroma master slider clicking on a small arrow next to it or move the
slider manually.
5. Since Auto chroma NR is enabled, the slider is moved back. But as step 4 is repeated,
the chroma NR value indicated on this slider is different (almost) every time.

My impression was that calculation of the NR parameter values passed from engine to
GUI has this bug. May be it is related to OMP and declaration of some variables in
an incorrect place, so they get randomized on occasion.

Reported by michaelezra000 on 2014-11-15 00:15:10

[Beep6581]

Hi Michael, no problem

1.) and 2.) work fine
but 3.) doesn't work because it's already at 'Automatic global' after 1.) and 2.)
4.) also doesn't work because it's inactive

Which revision do you use?

Reported by heckflosse@i-weyrich.de on 2014-11-15 00:27:37

[Beep6581]

Version: 4.2.22
Changeset: e360ea52f473

Reported by michaelezra000 on 2014-11-15 00:37:40

[Beep6581]

Ok, so it's better to continue discussion when you upgraded to vanilla + issue2557_05.patch
;-)

Reported by heckflosse@i-weyrich.de on 2014-11-15 00:40:51

[Beep6581]

I agree, it works great after the patch. I also tested by switching several times from
Preview to Automatic global and the greyed-out slider values were restoring reliably.
Thanks, Ingo:)

Reported by michaelezra000 on 2014-11-15 03:45:57

[Beep6581]

All works fine ("perf" is the same error as before !!)
Again, thank you Ingo, for this good work 

You can commit this change :)

Reported by jdesmis on 2014-11-15 07:21:43

[Beep6581]

Issue 2573 has been merged into this issue.

Reported by heckflosse@i-weyrich.de on 2014-11-15 11:14:36

[Beep6581]

re #35, Michael: I guess it's caused by this cheat in line 574 of dcrop.cc:

if(params.dirpyrDenoise.Lmethod=="CUR") params.dirpyrDenoise.luma=0.5f;//very small
value to init process - select curve or slider

Reported by heckflosse@i-weyrich.de on 2014-11-15 11:44:57

[Beep6581]

Hi Ingo, thanks for finding it. I discovered this at 300% zoom when tiny white speckles
would disappear from the image with only chroma  NR was set in the GUI.

Reported by michaelezra000 on 2014-11-15 13:00:42

[Beep6581]

Jacques, I've a question about the intention of the cheats from #36 (and also in improccordinator.cc
and simpleprocess.cc):
Were they needed to don't skip NR in case a Luminance Curve was used but Slider was
set to zero? In this case we could replace them now by cheating with 0.f.
Or where they needed because in line 812 of FTblockDN.cc impulse_nr is only called
when dnparams.luma>0.01? In this case we should stay with 0.5f.
But in any case, when Method is set to 'Curve' and Curve is Linear, we should set 0.f
instead of 0.5f

I included the last thing into the attached patch. Additionally, the cheat now is not
visible in gui.

Try this with 'before patch':

Open Image, Enable NR, Set method to slider and slider to 0.00, Set method to 'Curve',
Close Image, Open Image, Set method to Slider and its value is 0.5

Ingo

Reported by heckflosse@i-weyrich.de on 2014-11-15 13:01:44

---

• _Attachment: issue2557_06.patch_

[Beep6581]

Ingo

One again, you are true? and also thanks to Michael to have found this anomaly.
As there is a special "menu" for "impulse noise reduction", we can without problem,
use the solution you have proposed in patch _06. (and it works)

Thank you again

Jacques

Reported by jdesmis on 2014-11-15 13:22:25

[Beep6581]

Jacques, patch 06 as posted or with additional changes (replacing 0.5f with 0.f even
when curve is not linear)?

Reported by heckflosse@i-weyrich.de on 2014-11-15 13:44:09

[Beep6581]

We also should try to get a consistent behaviour. Actually when curve is used the internal
call of impulse_nr is skipped (when we set to 0.f) or executed with the constant value
of 0.5f if we set this value.
But when slider is used, internal impulse_nr is called wit the value of the slider
(upper limited by 50.f).

Should we eventually remove the internal impulse_nr call inside RGB_denoise completely?

Ingo

Reported by heckflosse@i-weyrich.de on 2014-11-15 13:57:58

[Beep6581]

In french we say "choix Cornelien"...because there are advantages and disadvantages

I proposed :
* as patch06
* and also : remove the internal impulse_nr call inside RGB_denoise, it is redundant

Reported by jdesmis on 2014-11-15 14:07:02

[Beep6581]

ok, thank you :-)

Reported by heckflosse@i-weyrich.de on 2014-11-15 14:14:01

[Beep6581]

Jacques, in 'Standard mode' setting to 0.f instead of 0.5f only works because there
is a typo in this line in FTblockDN.cc:

if(settings->leveldnautsimpl==0 && dnparams.C2method!="MAN") execwavelet=true;

If we correct the line to 

if(settings->leveldnautsimpl==0 && dnparams.C2method!="MANU") execwavelet=true;

it doesn't work ;-) So I'll stay with 0.5f when luma curve is used and not linear and
0.f if luma curve is used but linear.
Setting to 0.f would skip the wavelet in cases we don't want it to be skipped, I guess
(for example, using luma curve and median, but chroma master manually set to 0).
I'll remove the internal impulse_nr calls too and post a patch later in the evening.

Ingo

Reported by heckflosse@i-weyrich.de on 2014-11-15 15:40:41

[Beep6581]

Ok

I spent a lot of time to evaluate each boundary conditions ... but some are passed
through
Thank you again

Jacques

Reported by jdesmis on 2014-11-15 15:45:16

[Beep6581]

Here's the patch with the things mentioned above

Reported by heckflosse@i-weyrich.de on 2014-11-15 16:24:23

---

• _Attachment: issue2557_07.patch_

[Beep6581]

Testing patch 7.
I haven't noticed any quirks in the interface though I only tested briefly.

No differences when saving the same image via queue 4 times:
http://i.imgur.com/gxWzdNG.png

Comparison of unpatched vs patched
Default ISO Medium + "Quality: High" http://i.imgur.com/KluxqZB.jpg
Default ISO Medium + "Quality: Medium" http://i.imgur.com/Wcy66oG.jpg
No eyeball differences.

Reported by entertheyoni on 2014-11-15 22:54:06

[Beep6581]

Ingo, thanks for the fix!

I found one more issue.
1. open image with neutral profile, enable NR, zoom >=100%
2. Switch Chroma method to manual, Set Chorma master = 0
3. Observe that "Preview Noise: Mean=0 High=0"
4. When Chroma NR method is switched to either Preview of Auto the label reporting
the amount of noise in the preview area still is as in step 3.

Seems that setting chroma master to 0 disables the noise estimation of the preview
area.

Reported by michaelezra000 on 2014-11-16 04:22:53

[Beep6581]

Michael

I try to reproduce #48

To have "Mean=0 High=0", you must also set the "Chrominance curve" to zero, and also
luminance slider to zero. In other case, "mean" and "high" are different from 0

In this case, wavelet is not enabled...and no noise estimation is done...Noise estimation
is done by wavelet...

Ingo, I try patch _7, it is OK for me !

:)

Reported by jdesmis on 2014-11-16 10:28:13

[Beep6581]

I included a fix for #48 and committed to revision 984d9cf50d7d

Reported by heckflosse@i-weyrich.de on 2014-11-16 14:32:25

[Beep6581]

Thanks, Ingo! I uploaded this build to the main site for Winx64.

Reported by michaelezra000 on 2014-11-16 16:49:21