Ciecam02 artifacts

[Beep6581]

Originally reported on Google Code with ID 2819

With the raw and pp3 from here: https://drive.google.com/folderview?id=0B8sFrdsLj4-nflZ0ZGM1UkMxeS1OVnRSX0d6Znp2cHNhc0xsajJmN1Z2Z0R0cng3MzZUX3c&usp=sharing

artifacts are reproducible. I tracked down that the reason is only in ciecam02, not
in wavelet, nr or vibrance (which are also enabled in the pp3)

I already found and fixed 3 bugs, found (but not cleanly fixed) a 4th bug and now I'm
searching the 5th (and hopefully last) bug.

I'll post a patch when all 5 bugs are fixed.

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-20 22:49:59

[Beep6581]

I decided to make a series of patches. One for each bugfix.

testimage: http://www.rawtherapee.com/shared/test_images/saturated_blues_led.pef

Apply Default profile, enable ciecam02. Don't change any setting in ciecam02.
Expected result: Image should stay nearly the same as without ciecam02
Result: a lot of dark artifacts in lower region of image

Fixed with this patch

Reported by heckflosse@i-weyrich.de on 2015-06-22 16:34:26

---

• _Attachment: issue2819_00.patch_

[Beep6581]

Before I post the next patch I would like to get some opinions.

testimage: http://www.rawtherapee.com/shared/test_images/renon_earth_pyramids.pef

Apply default profile, enable ciecam02, set Algorithm to Brightness + Colourfullness,
then set Brightness(Q) to -57
Result: the image gets darker, but saturation increases. This behaviour is also one
reason for artifacts with saturated blues and ciecam02.
The patch is almost ready, but of course it would break compatibility. 

I don't think that this increase of saturation is intended, but I'm not sure.

Michael, what do you think?

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-22 21:26:41

[Beep6581]

I forgot to mention that I also set Exposure compensation to 0.31
The increased saturation is better visible with that value

Reported by heckflosse@i-weyrich.de on 2015-06-22 21:37:40

[Beep6581]

Hi Ingo, I tried patch 00 with saturated_blues_led.pef and it did not make any difference
in the same test. However, what I noticed is that if working profile is switched to
sRGB the black artifacts disappear.

Unfortunately I cannot comment on Brightness/Saturation relationship with CIECAM02,
I will try to research, and may be Jacques could comment.

Reported by michaelezra000 on 2015-06-22 22:15:14

[Beep6581]

Hi Michael, strange that it does not make any difference on your side. Does that mean,
that the black artifacts are still there with patch 00 (when ciecam02 sliders are at
0) ?

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-22 22:29:42

[Beep6581]

Hi Ingo,  patch 00 fixes black tones for navigator and thumbnail, but it is the same
in the preview and output render.

Reported by michaelezra000 on 2015-06-22 22:59:35

[Beep6581]

Michael, do you have in Preferences/Colour Management/Ciecam02/Use float instead...
disabled???

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-22 23:05:24

[Beep6581]

yes!:)  when enabled, patch is working:)

Reported by michaelezra000 on 2015-06-22 23:11:49

[Beep6581]

:) I try not to touch the 'reference' double precision code

Reported by heckflosse@i-weyrich.de on 2015-06-22 23:12:17

[Beep6581]

This patch allows to compare new and old algorithm. I only changed Algorithm 'Brightness
+ Colourfulness (QM)'.

Raw file from here: https://drive.google.com/folderview?id=0B8sFrdsLj4-nflZ0ZGM1UkMxeS1OVnRSX0d6Znp2cHNhc0xsajJmN1Z2Z0R0cng3MzZUX3c&usp=sharing

Apply Default profile, enable ciecam02, set Algorithm to 'All', Brightness to -30 and
Colourfulness to 40. You'll see a lot of bright Artifacts in dark saturated blue region.

Now change Algorithm to 'Brightness + Colourfulness (QM)' (the one I modified). Artifacts
are gone.

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-23 11:08:25

• Status changed: PatchSubmitted

---

• _Attachment: issue2819_01.patch_

[Beep6581]

Hmm, my patch 01 is more or less a combination of Lightness and Colourfulness.....

Reported by heckflosse@i-weyrich.de on 2015-06-23 12:57:45

[Beep6581]

Ok, forget patch 01 from #10

Additionally to patch 00 this patch fixes the following bugs:

testimage: http://www.rawtherapee.com/shared/test_images/saturated_blues_led.pef

first bug:

Apply Default profile, enable ciecam02, set Algorithm to 'All'
Set Chroma slider to -0.1 gives wrong result. If you decrease further (e.g. -0.2) it
will be correct again. Same for Saturation and Colourfulness slider
Fixed

Second bug:

Apply Default profile, enable ciecam02, set Algorithm to 'All'
Set Hue to 35
Switch to Algorithm 'Lightness + Chroma' or 'Brightness + Colourfulness'
Though the hue slider is not available when choosing one of this algorithms, its value
is applied.
Fixed

Third bug:

Apply Default profile, enable ciecam02, set Algorithm to 'All'
set saturation to a value > 0
=> dark artifacts in saturated blue regions
Fixed

I also cleaned the code a bit to prepare further optimizations.

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-24 16:51:08

---

• _Attachment: issue2819_02.patch_

[Beep6581]

Hi Ingo, I can confirm that 02-patch solves the three bugs listed above. However, when
I set hue to 35 as in the second case the following happens in pathed RT:
http://johanthor.smugmug.com/Other/RawTherapee/i-dBfZvhq

In the unpatched version this is what I see, that is no white spots:
http://johanthor.smugmug.com/Other/RawTherapee/i-5mXbFWS

I have this in my preferences:
http://johanthor.smugmug.com/Other/RawTherapee/i-QMzWL8X

Reported by johan@birkagatan.com on 2015-06-24 17:54:21

[Beep6581]

The first and second link in my post above should change places. Don't know how I managed
to mix them :-)

Reported by johan@birkagatan.com on 2015-06-24 17:57:04

[Beep6581]

Hi Johan, thanks for testing. I confirm your issue. Will have a look now.

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-24 18:59:20

[Beep6581]

Johan, re #13. In deed that's an old bug which now becomes visible also with sliders
(except hue) set to 0.

To verify try this with unpatched RT:

testimage: http://www.rawtherapee.com/shared/test_images/saturated_blues_led.pef

Apply Default profile, enable ciecam02, set Algorithm to 'All'
Set all ciecam02 sliders except hue to -0.2. Then set hue to 35

Actually I don't know how to fix this bug, but I think it's better now (more predictable)
than unpatched.

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-24 21:12:37

[Beep6581]

Additional thoughts:

Ciecam02 allows to modify 6 layers

Lightness (J)
Brightness (Q)
Chroma (C)
Saturation (S)
Colourfulness (M)
Hue (h)

These layers are not independent. Actually problematic (when used with saturated blue)
are

Brightness (Q) with negative values
Colourfulness (M) with positive values
Hue (h) with values in the range of 33 to 78

I'll investigate further.

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-24 21:44:43

[Beep6581]

With revision fe25e7e3f86d I also fixed 'First' and 'Second' from #12. Also included
a fix for a nan issue.
Issue stays open.

Reported by heckflosse@i-weyrich.de on 2015-06-25 20:26:55

[Beep6581]

Forgot to mention: 'Third' from #12 and patch 00 are not fixed with latest commit

Reported by heckflosse@i-weyrich.de on 2015-06-25 20:31:42

[Beep6581]

Here's a new patch which applies to tip and includes patch 00 and 'third' from #12.

It's quite large because I moved all the ciecam02 stuff from colortemp.cc(h) to ciecam02.cc(h).
By doing this I also detected some unnecessary source dependencies and fixed them too
(which makes the patch larger). I also fixed some compiler warnings (which makes the
patch larger).

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-26 22:09:10

---

• _Attachment: issue2819_03.patch_

[Beep6581]

Addition to #20:

In case the changes (patch 00 and 'third' from #12) won't be accepted (which I could
understand), I will commit this patch without this changes to get the code cleanup
into the repository.

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-26 23:06:12

[Beep6581]

I found the reason for the bright green artifacts in saturated blue regions when using
ciecam02 (Calculation of a and b changes sign at some conditions).

I'll try to post a patch tomorrow.

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-27 21:02:08

[Beep6581]

Here's a patch which avoids a lot of the bright green and magenta artifacts in saturated
blue regions.

testimage from here: https://drive.google.com/folderview?id=0B8sFrdsLj4-nflZ0ZGM1UkMxeS1OVnRSX0d6Znp2cHNhc0xsajJmN1Z2Z0R0cng3MzZUX3c&usp=sharing

Apply default profile + ciecam02, set Algorithm to 'Brightness and Colourfulness (QM)',
set 'Brightness (Q)' to -21 and 'Colourfulness (M)' to 40. Disable 'Gamut Control'.

This produces a lot of magenta and green artifacts in saturated blue regions.

I tracked the problem to the function calculate_abfloat. I placed some comments in
that function that describe the problem and also changed the function, which fixes
most of the artifacts (but not all, there seems to a be a thing that I miss...)

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-28 20:46:29

---

• _Attachment: issue2819_04.patch_

[Beep6581]

I confirm that patch-04 solves the problem we noted in "saturated_blues..." and your
test case in #23! :-) Really good work, Ingo!
I haven't noticed any other issues, but will continue testing.

Reported by johan@birkagatan.com on 2015-06-29 08:32:10

[Beep6581]

Johan, thanks for testing. Patch 04 still produced bright artifacts in saturated blue
regions with the raw and pp3 from #0. Fixed with this patch :-)
Changes between patch 04 and 05 are in function calculate_abfloat

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-29 19:10:42

---

• _Attachment: issue2819_05.patch_

[Beep6581]

patch 05 still needs some fine tuning in ciecam02.cc line 549 to 556. I'll wait until
Jacques is back. Maybe he has some good ideas.

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-29 21:08:33

[Beep6581]

Ingo
I'm temporarily back to my home, but with little time to spare.
I leave for 3 days ... and would be available from Friday.

I just load the patch ...

Ingo you did a very good job:
1) separating the "CIECAM02" code that of "colortemp" is more readable

2) finding many "small" bugs ... In the original code CIECAM, there were plenty of
"small" or "big" bugs ... Gradually the code should become more breast.

3) the last change you propose had escaped the designer CIECAM and myself. Indeed many
of which function together sines and cosines used, have a "strange" behavior when PI
modulo or 180 modulo.
The solution you propose solves many problems, the more important....

4) probably we still find other abnormalities of the original code CIECAM

I think you can commit this patch.

However, as with CBDL or "Tone-mapping", work space and CIECAM02 data type are different
from Lab, significantly smaller. If we wish to significantly improve the combined use
of "wavelet" and "CIECAM" it will be desirable to write a version of Wavelet with "J
C h Q M..." rather than "Lab".
But that is another matter and much work. To do ... only if many bugs appear.

Thank you again for this very good job.
:)

Reported by jdesmis on 2015-06-30 06:41:29

[Beep6581]

Jacques, thanks for testing and reviewing my changes :-)

I committed patch 05 to revision 6463150c2b39
Issue stays open for further fixes.

Ingo

Reported by heckflosse@i-weyrich.de on 2015-06-30 12:10:59

[Beep6581]

I started another round of optimizing ciecam02 for speed. I think, about double speed
is possible.

Ingo

Reported by heckflosse@i-weyrich.de on 2015-07-03 22:59:40

[Beep6581]

ciecam02 is not easy to vectorize (using SSE) completely. But there are some parts (conversion
from lab to ciecam02 and backwards) which can be vectorized even if the steps in between
are not vectorized (using temporary line buffers).

I just analyzed the lab->ciecam02 and ciecam02->lab conversion. Here's the first result:

operation       addition/subtraction  multiplication  division  square root  pow  atan2
 sincos
------------------------------------------------------------------------------------------------
lab->ciecam02          46                  85            20        2          5   
 1      1
ciecam02->lab          54                  76            25        1          5   
 0      2

These operations can be divided by 4 using SSE. Of course there will be some overhead
for the line buffers but I expect a reasonable speedup nevertheless.

I'll start tomorrow implementing this stuff.

Ingo

Reported by heckflosse@i-weyrich.de on 2015-07-03 23:14:36

[Beep6581]

This patch vectorizes the conversion from lab to ciecam02. Vectorization from ciecam02
to lab follows with next patch. I measured with my usual D800 file and the attached
pp3. I detected no differences in output.

before patch : 4264 ms
after patch  : 2890 ms

Ingo

Reported by heckflosse@i-weyrich.de on 2015-07-05 13:36:12

---

• _Attachment: ciecamspeedup_00.patch_
• Attachment: ciecam.pp3

[Beep6581]

Vanilla on the left, ciecamspeedup_00 on the right.
The first section is when I open a folder with some images using CIECAM02.
Zoom = zoom in to 100%.
Out = zoom to fit
http://i.imgur.com/utTIOXW.png

Reported by entertheyoni on 2015-07-05 15:05:34

• Labels added: Type-Enhancement
• Labels removed: Type-Defect

[Beep6581]

This patch adds vectorization of conversion from ciecam02 to xyz. 

before patch : 4264 ms
patch 00     : 2890 ms
patch 01     : 1617 ms

Ingo

Reported by heckflosse@i-weyrich.de on 2015-07-05 16:31:57

---

• _Attachment: ciecamspeedup_01.patch_

[Beep6581]

Hi Ingo, this is a great speedup!:)
I compared before/after with use of colorfullness, Contrast and Lightness curve.
There is a difference in saturation, it is higher after the patch.

Reported by michaelezra000 on 2015-07-05 17:55:08

[Beep6581]

Michael, can you post the pp3 you used for your test please?

Reported by heckflosse@i-weyrich.de on 2015-07-05 18:39:42

[Beep6581]

Reported by michaelezra000 on 2015-07-05 19:08:36

---

• _Attachment: _MEZ8097.NEF.pp3_

[Beep6581]

Michael, I comapared before/after using your pp3 on my test image. No differences. Can
you please post a complete example (pp3 + raw file)?

Ingo

Reported by heckflosse@i-weyrich.de on 2015-07-05 19:15:52

[Beep6581]

Hi Ingo, please download the raw here: http://michaelezra.com/Temp/_MEZ8097.zip

Reported by michaelezra000 on 2015-07-06 03:20:14

[Beep6581]

Michael, I processed your file without patch and with patch 01 to tiff 16. Again no
differences in output even when calculating absolute difference enhanced with an extreme
brightness curve. 

Can you retest please.

Reported by heckflosse@i-weyrich.de on 2015-07-06 10:03:26

[Beep6581]

Hi Ingo, my "before-patch" RT version was RT 4.2.231, which was prior  Revision 6463150c2b39
(Ciecam02 artifacts, Issue 2819). There are saturation differences with that version.
However you are right, when I simply removed the patch and tested against 4.2.235 /
Revision 0731183ceb76, there are no differences.

Reported by michaelezra000 on 2015-07-06 13:13:15

[Beep6581]

Hi Michael, I had a look about the saturation differences. They are caused by the following
change in Revision 6463150c2b39:

Before that revision, even when Saturation wasn't changed (Algorithm QM has no saturation
slider), the unchanged Saturation values of the image have been passed to the function
curvecolorfloat, which cut them.
With the patch from Revision 6463150c2b39 they stay untouched in this case.

Ingo

Reported by heckflosse@i-weyrich.de on 2015-07-06 13:39:05

[Beep6581]

Here's a new patch. I made some code cleanups. Speed is about the same as last patch
(a bit faster but nothing to write home about). I would like to commit this patch before
next Friday (I'm away for about three weeks then)

Ingo

btw: I don't know why the size of the patch increased so much since I applied it to
latest revision. I didn't change line endings......

Ingo

Reported by heckflosse@i-weyrich.de on 2015-07-06 22:42:37

---

• _Attachment: ciecamspeedup_02.patch_

[Beep6581]

The size increase is because sleefsseavx.c was included in patch 2, something caused
its line endings to be changed. Cleaned patch attached.

Reported by entertheyoni on 2015-07-07 07:24:30

---

• _Attachment: ciecamspeedup_02.clean.patch_

[Beep6581]

DrSlony, thank you for cleaning the patch.

Reported by heckflosse@i-weyrich.de on 2015-07-07 10:04:24

[Beep6581]

Fixed cleaned patch

Reported by heckflosse@i-weyrich.de on 2015-07-07 10:27:14

---

• _Attachment: ciecamspeedup_03.patch_

[Beep6581]

Speedup committed to revision 34668edfc729

Reported by heckflosse@i-weyrich.de on 2015-07-07 14:35:29

• Status changed: FixedPendingConfirmation

[Beep6581]

#41 about these saturation differences - are you sure the behaviour is correct now?
I am not talking about general increase of saturation, which would be easy to correct,
but color-specific differences. Now saturation for green is boosted much stronger,
whereas the differences for red or blue seem to be small. An example: Before I used
CIECAM saturation +18, now I have to use +7 to get the same saturation increase for
green colors, but at this +7 setting blue and red are now much less saturated than
at +18. So green seems to get an additional +11 saturation boost compared to before,
which results in a somewhat weird color saturation balance.

For reference, before this patch, the saturation increase and resulting color balance
with LAB chromaticity compared to CIECAM (chromaticity or saturation, I didn't test
colorfulness) was nearly identical. Now the result is very different, green is boosted
much more with CIECAM compared to LAB.

I can upload Raw files if necessary, but the effect should be easily visible with any
Raw file that contains green grass or trees etc. 

Reported by stefan.ittner on 2015-07-08 09:13:27

[Beep6581]

One more observation: Color saturation is now increased significantly only by activating
the CIECAM module, even when saturation, colorfulness and chroma sliders (and all other
controls) are set to zero. This does not seem right...

Reported by stefan.ittner on 2015-07-12 12:55:03

[Beep6581]

additional information: the saturation increase simply by activating CIECAM seems to
be limited to green, other colors seem unaffected. 
I tested with Raw files from Pentax K-5 II and Canon S110, both show the same problem.

Reported by stefan.ittner on 2015-07-12 14:03:11

[Beep6581]

Full set of examples (RT 4.2.222 vs. 4.2.238, Win64 builds from homepage) here: http://filebin.net/0pn4g4jis8

Observation: Green gets boosted just by activating CIECAM in 4.2.238, looks pretty
"toxic" to me. This was not the case in 4.2.222. Rendering with CIECAM off or LAB tools
is identical for both versions, so culprit is most likely CIECAM...

Reported by stefan.ittner on 2015-07-12 19:20:57