Munsell correction - Lab adjustements

[Beep6581]

Originally reported on Google Code with ID 1359

This patch adds the correction Munsell, to "Lab Adjustements"

Its action depends on the cursor "saturation", "brightness", "contrast" and the "curves"
luminance.
I changed "avoid color clipping" becomes "avoid color shift". This button enables the
correction Munsell

The software part of "vibrancy" is amended accordingly :)

Reported by jdesmis on 2012-05-09 06:48:04

---

• Attachment: munsell01.patch

[Beep6581]

depass is now only created and used in Debug builds.

Something that i didn't paid attention too is that the new skin control curve in Vibrance
is only used when Vibrance is activated and the Pastel/Saturated slider is != 0.

Would it make sense/is it possible to use it even if Pastel/Saturated are null?

Something that i still need to do: rename some procparams name, converting former "saturation"
parameter to "chromaticity", and "avoirclip" to "avoidcolorshift" at load time.

Curve's colored bar are on still WIP too.

Reported by natureh.510 on 2012-07-09 17:39:00

[Beep6581]

No problem to use "new skin control" with pastel/saturated=0  :)

Thank's Hombre for your good job :)

Reported by jdesmis on 2012-07-09 17:49:12

[Beep6581]

Michael, I will examine what can be done to integrate the "red skin and control" to
the curve. In a previous essay, I got results but with lots of artifacts in transitions.

Reported by jdesmis on 2012-07-09 18:46:10

[Beep6581]

Beware, some users are complaining about RT's complexity. We should really look out
if something is really necessary, and eventually look out for best integration in the
GUI in a KISS point of view (Keep It Stupid Simple) _as much as possible_.

Reported by natureh.510 on 2012-07-09 19:06:10

[Beep6581]

plus one for Hombres view
If there is not a very common use case for it or could be done with other controls,
it is not good to add.

Reported by oduis@hotmail.com on 2012-07-09 19:14:20

[Beep6581]

I think it's can be done with existing controls - not easy but possible ! but I can
examine if this work can be done seamlessly without additional controls (sliders, button
...) but I think also, that there is no fire  :)

Reported by jdesmis on 2012-07-09 19:26:02

[Beep6581]

Yes, "y'a pas l'feu au lac!" ;), but automatic correction based on spreaded around widgets
(i.e. in several tools) must be done we care and has to be very well documented, like
the HLR trick of comment 49.

Reported by natureh.510 on 2012-07-09 20:05:06

[Beep6581]

+1, we simply can add descriptive tooltips

to avoidcolorshift checkbox to cover 
1. what it does
2. relation to HR

to red and skin control
1. what it does
2. relation to chromaticity & CC curves

My point here is that CC and chromaticity change the same thing.
So if there is red and skin control over chromaticity changes it is just logical that
it would apply no matter how the chromaticity is being changed.

Reported by michaelezra000 on 2012-07-09 20:18:26

[Beep6581]

Here is an updated patch, to be applied over Default-TIP as of now.

I've updated French strings, made available the Vibrance curve even with Past/Sat=0,
made the Avoid Color Shift checkbox unsensitive repending on BWToning, but didn't managed
to avoid computing "Avoid Color Shift" if BWToning is enabled. The B&W image also become
colorful if you set a Cc curve (mabe Ch too) while BWToning is enabled. 

Could someone shouls look at this please?

Reported by natureh.510 on 2012-07-10 00:19:04

---

• Attachment: munsell07-10.patch

[Beep6581]

I plan to modify the Red and skin controls's GUI too, to make them more intuitive, by
changing their labels:

Enable Red and Skin Control   >>>   Protect Reds and Skin Tones
Red and Skin Control          >>>   Protection's Strength

(already done in the History strings... :-/)

And i'll inverse the range of the Red and Skin control slider:
0   = No protection = Chromaticity fully applied even on reds and skin tones
100 = Full protection = Chromaticity not applied at all on reds and skin tones

Maybe i could make them unsensitive when Chromaticity = 0, since they are related to
this slider only, like "Shadow recovery" is tied to the "Black" slider.

In fact, to simplify the GUI, i for one would suppress the "Enable Red and Skin Control"
checkbutton, as unchecking it is equal to setting "Red and Skin Control" to 100 actually
(to 0 when the modifications will be done). What's you opinion?

Reported by natureh.510 on 2012-07-10 00:52:20

[Beep6581]

Hi Hombre, 

1. To disable effect of CC curve when bwToning = true:
add if (!bwToning)  on line 665 in improcfunc.cc so you get this:

        if (!bwToning) {
            // I have placed C=f(C) after all C treatments to assure maximum amplitude of "C"
            chroma = sqrt(SQR(atmp)+SQR(btmp)+0.001f);
            chromaCfactor = (satcurve[chroma*adjustr])/(chroma*adjustr);//apply C=f(C)
            atmp *= chromaCfactor;
            btmp *= chromaCfactor;
            // end chroma C=f(C)

            Chprov1 = sqrt(SQR(atmp/327.68f)+SQR(btmp/327.68f));
        }

I can't see clearly where CH curve is applied, but it should be similar logic.

2. Vibrance does not get applied now until HH curve is changed from the diagonal 
value.

3. about unchecking the button - it is an easier shortcut, but it would more user friendly
to not to alter user inputs, so if user selected protect red and then chromaticity
is moved from +10 to 0 then back to +10, user selection is not lost.
you could gray out those controls which have no effect, but separately in rt engine
would implement matching logic of bypassing protection when chromaticity=0.
Then, if red protection would get extended to cc curve also, condition would need to
be extended = no protection when chromaticity=0 and CC curve is unchanged. P.S. sorry
if this is painful:)
As an example, the same logic was implemented for bwToning - when enabled, the Lab
saturation is grayed out but its value is unchanged.

4. Just noticed - the tooltip in BW Toning mentioned Lab saturation, this should be
changed to chromaticity.

5. Suggestion for Tooltip for "avoid color shift" = "Fit colors into gamut of the working
color space and apply Munsell correction"

6. BW toning does not seem to be influenced by avoid color shift - I don't see an issue.

Reported by michaelezra000 on 2012-07-10 01:21:24

[Beep6581]

To stop CH with bwToning enabled: improcfunc.cc line 585 - change to this:

            // using flatcurve from hsv equalizer
            if (chCurveEnabled && !bwToning) {

Reported by michaelezra000 on 2012-07-10 01:27:51

[Beep6581]

Hombre

I agree for comment #60 :
==> Enable Red and Skin Control   >>>   Protect Reds and Skin Tones
==> Red and Skin Control          >>>   Protection's Strength

==> suppress the "Enable Red and Skin Control" 

Reported by jdesmis on 2012-07-10 05:03:14

[Beep6581]

Since it's so silent on this feature I'm looking forward to... all good? Can I help?

Reported by oduis@hotmail.com on 2012-07-16 19:32:56

[Beep6581]

Jacques' algorithm is finished from some days now. It was very hard to make it work,
at least for me, but the GUI part is finished, with a new ColoredBar class that can
handle shades of color or any programmatically rendered shades in background of objects
(throught a newly created BackBuffer class too). I removed the Pixmap code from the
curve widgets since it's deprecated and replaced them by plain Cairo::Surface.

I'll post the patch tonight (21h french time), then i'll be glad for some help for
the set_sensitive thing that doesn't want to work in rtgui/labcurve.cc when called
from LCurve::read.

Reported by natureh.510 on 2012-07-17 15:23:16

• Labels added: Component-UI

[Beep6581]

Here is the latest patch for this issue. Unfortunatly, it's quite unstable for now:
it sometimes crash due to Heap problem that i have no clue on how to solve it, Gdb
is not of any help here :(

Please test it anyway.

I've added ColoredBar to the Vibrance curve (skin tones), TSV, RGB and Lab's CH curve.
More will be done on a second step.

Reported by natureh.510 on 2012-07-17 20:37:48

[Beep6581]

More on the crash: it happens when opening a curve for the first time (a FlatCurve),
or when resizing the right pane while a FlatCurve is opened. It may not be restricted
to the FlatCurves though.

Reported by natureh.510 on 2012-07-17 20:39:28

[Beep6581]

This one will apply correctly over Default.

Reported by natureh.510 on 2012-07-17 20:43:55

[Beep6581]

Thanks for your efforts. I searched a while for the bug, but I couldn't find it unfortunately
:-(   Good night.

Reported by oduis@hotmail.com on 2012-07-17 21:16:32

[Beep6581]

Except from compile problem btw:

C:\GCC\RT\RTSrc\rtengine\color.cc:97:5: error: 'MunsellDebugInfo' does not name a type
C:\GCC\RT\RTSrc\rtengine\color.cc:100:7: error: 'MunsellDebugInfo' has not been declared
C:\GCC\RT\RTSrc\rtengine\color.cc: In function 'void rtengine::reinitValues()':
C:\GCC\RT\RTSrc\rtengine\color.cc:101:6: error: 'maxdhue' was not declared in this
scope
C:\GCC\RT\RTSrc\rtengine\color.cc:102:6: error: 'maxdhuelum' was not declared in this
scope
C:\GCC\RT\RTSrc\rtengine\color.cc:103:6: error: 'depass' was not declared in this scope
C:\GCC\RT\RTSrc\rtengine\color.cc:103:13: error: 'depassLum' was not declared in this
scope

Reported by oduis@hotmail.com on 2012-07-17 21:20:08

[Beep6581]

Sorry, i've only checked the Debug build, i'll correct that for the Release build too.
Good night.

Reported by natureh.510 on 2012-07-17 21:31:31

[Beep6581]

Third and last attempt for today: now build fine for the Release target too.

Reported by natureh.510 on 2012-07-17 22:08:37

[Beep6581]

Hombre & Jacques, great work, and finally the color bars, this is very user-friendly!

More feedback:
1. default language - Typo:
LABCURVE_BWTONING_TIP;With B&T ...
LABCURVE_BWTONING_TIP;With B&W ...

2. labcurve.cc - incorrect default, line 65
patch:     rstprotection = Gtk::manage ( new Adjuster (M("TP_LABCURVE_RSTPROTECTION"),
0, 100, 0.1, 50) );
should be: rstprotection = Gtk::manage ( new Adjuster (M("TP_LABCURVE_RSTPROTECTION"),
0, 100, 0.1, 0) );

2.a might as well fix the default for chromaticity line 39
chromaticity   = Gtk::manage (new Adjuster (M("TP_LABCURVE_CHROMATICITY"), -100, 100,
1, 0));

3. labcurve.cc selecting bwtoning should gray out the chromaticity.
void LCurve::bwtoning_toggled () {
...line 297 add:
chromaticity->set_sensitive(!(bwtoning->get_active ())); //at bwtoning enabled chromaticity
value has no effect

if (listener) {
...
Not sure how to gray out CC and CH curves though. May be
ccshape->set_sensitive(!(bwtoning->get_active ()));
chshape->set_sensitive(!(bwtoning->get_active ()));

4. L curve, parametric - the gradient bar under the curve is blank

5. may be decrease with of the color bars by 2 pixels?

Reported by michaelezra000 on 2012-07-18 01:51:35

[Beep6581]

6. void LCurve::read
Sorry, I did not get a chance to test this, but looks like this should do it:

    bwtconn.block (true);
    bwtoning->set_active (pp->labCurve.bwtoning);
/*add*/    chromaticity->set_sensitive(!(bwtoning->get_active ()));
/*add*/    rstprotection->set_sensitive(!(bwtoning->get_active ()));
/*add*/    ccshape->set_sensitive(!(bwtoning->get_active ()));
/*add*/    chshape->set_sensitive(!(bwtoning->get_active ()));
    bwtconn.block (false);
    acconn.block (true);
    avoidcolorshift->set_active (pp->labCurve.avoidcolorshift);
/*add*/    avoidcolorshift->set_sensitive(!(bwtoning->get_active ()));
    acconn.block (false);

for #3 point in the comment 73 - I think you need to add the same statements as I amrked
here with /*add*/.

Reported by michaelezra000 on 2012-07-18 02:13:48

[Beep6581]

7. I know there was a separate issue on defaults of tools, but since these are such
massive changes, may be worth changing vibrance defaults for Pastel tones and Saturated
tones to 0

Reported by michaelezra000 on 2012-07-18 02:17:43

[Beep6581]

#8, this should be in a separate issue to not to delay this patch.
HL curve for B&W toning!

Reported by michaelezra000 on 2012-07-18 02:21:13

[Beep6581]

After some tests in DEBUG and RELEASE mode, everything seems to function correctly in
the algorithms.
The new color guides are very useful.
Hombre it's a very good job

Michael, HL curve for B&W toning will be done in a separate issue, next month  :)

Reported by jdesmis on 2012-07-18 05:45:17

[Beep6581]

Thanks, Jacques!

#9. Found one more issue: packaged profiles don't reset the 'a' curve to a diagonal.

Reported by michaelezra000 on 2012-07-18 12:42:00

[Beep6581]

I've tested munsell07-17e.patch for about half an hour in Gentoo 64 on a quad-core and
no crashes... now I only need another 12 years to learn to use it :]

The luminance curve histogram is often borked, is this known or should I provide the
Bug Trekker Pack™ (screenshot + raw + pp3)?

Reported by entertheyoni on 2012-07-18 17:35:34

[Beep6581]

No, i'll look at the histogram bug that i've seen too. I also discovered a HUGE memory
leak in Debug mode, the next and hopefully final patch will correct both.

I've made all the requested changes, even sent by mail, excepted 2 points:

73#3 and 74#6:

The "read" method really have to be able to set the widget un/sensitive, doing it in
BWToning_toggled isn't sufficient when used in the batch editing panel, and is useless
if all the code is in "read" and the tool doesn't have an "enabled" boolean.
Unfortunatly, it doesn't work, and i suspect a multithreading problem (Gtk's GUI handling
isn't "multithreaded ready"), so i'll leave the non working code as is in LCurve::read,
and will look out to make it work later when the patch will be committed. It has waited
for already too long now.

78#9:

I can't see any problem in the packaged profiles!? They al set "aCurve=0;", so could
you give me more information on this point?

Reported by natureh.510 on 2012-07-18 19:26:52

[Beep6581]

73#3 and 74#6: I will take a look after commit
78#9: 
a. select Neutral profile
b. change a curve to custom, adjust curve, 
c. then select Neutral profile again. 
result: a-Curve effect is gone, but a-Curve is selected as Custom 
should be - a-Curve is reset to a line diagonal

Reported by michaelezra000 on 2012-07-18 20:05:03

[Beep6581]

Okay, here is the "candidate to commit" one! (just in time before my hollidays :]! )

All reported bugs have been solved, even the set_sensitive handling that should be
(almost ;)) fully functionnal in image editor tabs (i've set everything sensitive in
the batch editor tab).

New feature: I've added a right click handling over the SHCSelector (the range divider
below the parametric curve) to reset it to default values.

RT is now stable on my machine, i guess that the memory leak was the culprit of the
crashes.

There's still one strange problem remaining, that occurs when you press the reset button
of the curve group: the allocated space for the graph seems to be few pixels bigger
than what it can display, until you refresh if by moving the cursor over it... Nothing
to stop a commit i think.

I've also suppressed the Lprov2 and Chprov2 parameters from gamutLchonly because they
are useless -> Lprov1 and Chprov1 are updated to the new values. If you want to keep
the previous one, you have to copy them before calling gamutLchonly.

Finally, i've put the ColoredBar in it's own *.cc/h file.

Reported by natureh.510 on 2012-07-19 00:12:30

[Beep6581]

Hi Hombre, I get this error:
CMake Error at rtgui/CMakeLists.txt:52 (add_executable):
  Cannot find source file:
    coloredbar.cc

hd add this file?

Reported by michaelezra000 on 2012-07-19 01:24:00

[Beep6581]

You're right, sorry.

Reported by natureh.510 on 2012-07-19 06:49:14

[Beep6581]

Hi Hombre, the only small issue I see is in LCurve::read 

This line does not work when chromaticity=0
rstprotection->set_sensitive( !pp->labCurve.bwtoning && !pp->labCurve.chromaticity
);

May be you could add the gray gradient bar to the tone curve also? It looks neat on
the L curve.

BTW, I like the thickness of the bars now - thanks, looks right in the slim UI mode.

Reported by michaelezra000 on 2012-07-19 11:15:32

[Beep6581]

No problem for me !
I tested in "DEBUG" mode,  and the features and algorithms correspond to what is expected.
We may update "default"

Merci Hombre (in french) :)

Reported by jdesmis on 2012-07-19 12:12:29

[Beep6581]

This one should correct the problem from comment #85, as well as the last remaining
problem (comment #82)! :)

If there's no objection, i'll commit tonight (in 4-5h)

@lebedev: that would be a great news! :)

Reported by natureh.510 on 2012-07-19 15:42:27

[Beep6581]

Hombre, I will get home later than that, but trust your changes. Can't wait to have
this merged with new NR!

Reported by michaelezra000 on 2012-07-19 15:46:40

[Beep6581]

Works nice here, too :-)
For my personal taste the black-to-white bars are a bit too poppy, but no postering,
and effective on normal images.
Thanks!

Reported by oduis@hotmail.com on 2012-07-19 16:50:57

[Beep6581]

Small issue:
Keeping vibrance turned off, move the "Skin tones: HH" curve. The status bar shows
"Processing image" and RT grinds for a few seconds (because I have EPD turned on) even
though the preview doesn't change. RT shouldn't do anything because Vibrance is not
enabled.

I got two mallocs within about 15 minutes of using RT. I will run a debug build next
time and report.

Reported by entertheyoni on 2012-07-19 19:48:46

[Beep6581]

Please see http://code.google.com/p/rawtherapee/issues/detail?id=1140#c22 :D

Reported by entertheyoni on 2012-07-19 20:19:19

[Beep6581]

The commit will have to wait a little bite, the bug from comment 66 is still there :(
I'm working on it.

Reported by natureh.510 on 2012-07-19 22:07:21

[Beep6581]

Here is a new patch, i can't guarantee that the bug is solved, but it didn't crashed
since i've built it (5mn test only, i need to go sleeping).

Also, i've inverted the 2 lines of curves of the Lab tool as suggested on IRC: Cc+Ch
on the first line, L+a+b on the second one, to make them all to the (almost) same width.

Reported by natureh.510 on 2012-07-19 23:03:48

[Beep6581]

Yes the buttons do look better now.

RT still crashes when I apply Neutral, open a photo and click on Minima/Maxima Control
Points in the HSV Equalizer panel and either move them into view or have the tool panel
scrolled in such a way that they'd get drawn when I click on them: http://paste2.org/p/2077888

Branch: default
Version: 4.0.9.91
Changeset: 82cfc9ccdc4e
Compiler: gcc 4.5.3
Processor: undefined
System: Linux
Bit depth: 64 bits
Gtkmm: V2.24.2
Build type: debug
Build flags:  -march=native -fopenmp -g
Link flags:   -march=native
OpenMP support: ON
MMAP support: ON

I deleted the cache, restarted RT, etc, clicked on Minima/Maxima Control Points in
the CH drop-down list and RT crashed: http://paste2.org/p/2077892

Deleted cache, etc, clicked on Minima/Maxima Control Points in the HSV Equalizer panel,
now it worked. I moved some points around. Next I clicked on something, I think I tried
collapsing HSV, and it crashed: http://paste2.org/p/2077896

Deleted cache, etc, clicked on Minima/Maxima Control Points in the HSV Equalizer panel
(but first I scrolled down to make room for the curves), the curves appeared but RT
crashed: http://paste2.org/p/2077897

Deleted cache, etc, clicked on Minima/Maxima Control Points in the CH drop-down (but
first I scrolled a bit up so that the curves would be drawn as much off-screen as possible).
RT didn't crash. I scrolled down to see the curves - crash http://i.imgur.com/GKZK2.png
http://paste2.org/p/2077900

The crash appears to happen when the Minima/Maxima Control Points (CH or HSV) are drawn/moved
into view, not just when they're activated by clicking on them (unless clicking means
they'd get drawn).

Reported by entertheyoni on 2012-07-20 00:25:51

[Beep6581]

I just tested for 30 min (Windows64), both HSV equalizer, as CH curve. Move, scroll,
enable them, work with a screen or two, scrolled down to make room for the curves (neutral
or another profile). No crash  :)

Reported by jdesmis on 2012-07-20 05:49:44

[Beep6581]

I did get what appears to be an OMP related crash
(  Fault Module Name:   libgomp_64-1.dll)
when I enabled vibrance, then moved HH skin tones curve up.

Reported by michaelezra000 on 2012-07-20 13:42:03

[Beep6581]

jdesmis the crash is with munsell07-20.patch
munsell07-19c.patch worked fine.

Reported by entertheyoni on 2012-07-20 14:20:40

[Beep6581]

I test with munsell07-20.patch :)

Reported by laurencedesmis on 2012-07-20 14:38:24

[Beep6581]

Error with my mail!
I test with munsell07-20.patch, and no crash :)

Reported by jdesmis on 2012-07-20 14:41:35

[Beep6581]

I just tested again, in "DEBUG" and "RELEASE" (with Munsell07-20.patch), "vibrance"
and curve "HH", HSV equalizer, Lab adjustements and curves CH.

With NEF, CR2, DNG.

Profiles : "neutral", "default" ...

"Working profile" : sRGB Prophoto

I moved the windows, scroll, etc.

No crash :)

Reported by jdesmis on 2012-07-20 15:21:32