Beep6581 / RawTherapee

A powerful cross-platform raw photo processing program
https://rawtherapee.com
GNU General Public License v3.0
2.9k stars 323 forks source link

Tone Mapping (CIECAM02) unusable - whites never white since issue 1766 commit #1811

Closed Beep6581 closed 9 years ago

Beep6581 commented 9 years ago

Originally reported on Google Code with ID 1827

Since rev 5578e4820ec9 solving issue 1766, Tone Mapping (CIECAM02) produces washed out
colors in highlights, because the highlights are squashed up at 75% in the histogram
and taper off since there. The screenshot makes this clear.

Adjusting any curve in RT, whether RGB/L tone curve or CIECAM02 Brightness/Lightness
curves, makes no difference, the histogram still shows highlights squashed at 75%.

Latest on the left, 4.0.10.30 2207:6dfd0cc40aa6 on the right.
http://i.imgur.com/mqmfnH3.jpg

Reported by entertheyoni on 2013-04-05 12:32:28

Beep6581 commented 9 years ago

Reported by entertheyoni on 2013-04-05 12:32:54

Beep6581 commented 9 years ago
Sample photo perfect for testing tone mapping uploaded:
http://rawtherapee.com/shared/test_images/dobbiaco_bar.pef

ps. HRA/HRT/HR don't change the 75% squash either.

Reported by entertheyoni on 2013-04-05 12:35:47

Beep6581 commented 9 years ago
When you hover over an area that should be clipped white (L=100), with TM (CIECAM02)
turned on it shows L=75.

I found that changing "Adaptation scene luminosity" from the default of 2000 to 200
gets L to equal 100 again. So some questions arise:
1- Can this be considered a proper solution, or just a temporary workaround?
2- Is this proper use of the slider, or is it taking advantage of a quirk?
3- Should the hard-coded default be changed to 200?

Reported by entertheyoni on 2013-04-05 12:47:52

Beep6581 commented 9 years ago
DrSlony

The cursor "Adaptation scene luminosity" CIECAM02 is used to adjust depending on the
absolute luminosity of the shooting.

2000 cd/m2 corresponds substantially to exposure to sun "light" or shade.

In this case we are "inside" and the value should probably be between 20 and 300 cd/m2...perhaps
less or more.
Setting to 200 you propose seems to solve the problem.

But by default I chose 2000 cd/m2 which corresponds to a majority of cases...

There is an explanation in the manual, but can be should be more specific?

Maybe also should create specific pp3 ??

:)

Reported by jdesmis on 2013-04-05 15:22:18

Beep6581 commented 9 years ago
On the opposite end - tonemapping without CIECAM leads to clipped whites (there is a
GC issue on that)

Reported by michaelezra000 on 2013-04-05 15:23:07

Beep6581 commented 9 years ago
Jacques thank you for the explanation.

There remains the issue that when a user enables tone mapping it should "just work".
Currently, it doesn't. I had no idea I'd need to lower that slider, and in fact before
that issue 1750 patch I did not have to. Why should I do it now? The luminosity of
the shooting did not change. It's the same photo.

Me creating PP3 files won't solve this usability problem.

The easiest and worst solution would be to have RT set that value to 200 automatically
when the user enables Tone Mapping and CIECAM02.
The most difficult and best solution would be for it to "just work".

Reported by entertheyoni on 2013-04-05 15:45:59

Beep6581 commented 9 years ago
Before issue167, the curves were false and produced artifacts in the highlights.
Now the curves are correct.

Of course you can put 200 as the default, but it will produce false (slighty) color/brightness
in a majority of cases...

CIECAM is complex, I greatly simplified yet (should have at least 1 or 2 more sliders...)
but it will require learning.

But I have nothing against "200" by default.

:)

Reported by jdesmis on 2013-04-05 16:20:40

Beep6581 commented 9 years ago
An automatic EV detection would be nice.

Most times the metered by camera ExposureValue exists in exif and if not one can calculate
it from exposure duration - aperture - ISO ... 

Reported by iliasgiarimis on 2013-04-05 19:32:03

Beep6581 commented 9 years ago
iliasgiarimis EV does not affect this issue.

Reported by entertheyoni on 2013-04-06 01:00:27

Beep6581 commented 9 years ago
I though it might be possible to have RT automatically lower ASL until whites are white
based on the histogram, but that's not so simple. If you look at the histogram in the
screenshot, the spike corresponds to the overexposed window frame area which should
be white but isn't. The problem is that after the spike follow some brighter values
- these are e.g. saturated reds on the lamp. So while at ASL=250 some parts of the
lamp have V=100% and L=99, the window frame that most clearly should be white is at
V=98.4 L=97. Such an automated algorithm would need to be not fooled by these high
values on the lamp, while the clipped area remains not white. Maybe i you could limit
the area of the image that gets sampled to only clipped pixels then it could automatically
set the correct ASL.

When tested on a photo without any clipped pixels, lowering ASL until the histogram
touches the right end seems to work quite well.

Any chance you could look into automating this?

Reported by entertheyoni on 2013-04-06 11:16:48

Beep6581 commented 9 years ago
@ entertheyoni #9

Isn't metered EV (or LV) a clear index for the scene illumination level ..??.

What we really need is "Light Value" (=scene luminance) which may exist in exif or
can be easily calculated from Exposure Value. http://en.wikipedia.org/wiki/Exposure_value

Reported by iliasgiarimis on 2013-04-06 12:09:24

Beep6581 commented 9 years ago
http://www.pentaxforums.com/forums/photography-articles/88197-excel-2003-lv-light-value-calculator.html

Reported by iliasgiarimis on 2013-04-06 12:45:44

Beep6581 commented 9 years ago
Jacques, this is such a find - thanks for pointing out that adaptation scene luminosity
causes a dramatic impact when using tonemapping!

I am curious why does it have almost no visible effect without using CIECAM tonemapping?

Reported by michaelezra000 on 2013-04-06 13:34:38

Beep6581 commented 9 years ago
@DrSLONY, Ilias
Effectively, EV can be calculated according to the values ​​exif. Of course this assumes
that the exposure is correct.
In RT is not simple to interface GUI ... but I'll try.

@Michael
CIECAM requires new learning

@all
However, for "tone-mapping" this aspect is "the tree that hides the forest". "Tone-mapping"
has since the beginning,  defects especially in the treatment of highlights. Enough
compression seems to work well for low light, but I think failing to highlights.

A year ago, I proposed (issue 1222) a change that was not unanimously (that is to say
the least draws)!

CIECAM02 works around the problem, but do not solve completely.

Ben said he would not make changes to the code for "tone-mapping".

If you want, I can try again with different algorithms (to create!) to improve the
rendering of "tone mapping" with highlights

But I would not start this work, until there is not a unanimous request !

:)

Reported by jdesmis on 2013-04-06 15:53:29

Beep6581 commented 9 years ago
Could you outline what this change would do? If it's the same as comment 0 in issue
1222, could you elaborate on 2) ?

In the meanwhile, what do we do here?
I can change 2000 to 200 which works well with TM+CIECAM02 in all cases I tried (keeps
whites white). No visible difference between 2000 and 200 when TM is off. You ok'd
this in comment #7.

Reported by entertheyoni on 2013-04-06 16:11:18

Beep6581 commented 9 years ago
Jacques, I think tonemapping should 
1. lift the shadows 
2. AND compress the highlights 
3. AND prevent both shadows and highlights from being clipped

I think currently tonemapping (without CIECAM) does mostly #1.
So I think improvements would be welcome:)

Reported by michaelezra000 on 2013-04-06 16:13:44

Beep6581 commented 9 years ago
I agree with #13. Now that I know what the slider does my vote is to add a tool tip
and manual entry to say it is used to control the histogram and set the 'greyness'
of tonemapping. The tech explanation may be correct, but is confusing to users who
haven't been exposed to radiometry. To most of us a standard candle is the size you
find in the  biggest bin at Walmart  

Reported by scribble1@charter.net on 2013-04-06 16:18:36

Beep6581 commented 9 years ago
I do not know if I understand everything perfectly (my bad english .., why everyone
does not speak French!).

I do not know today  how to approach the problem of "tone-mapping", but it must be
different from issue 1222.

"In the meanwhile, what do we do here?" :==> it is my question : "what do you want
I do ?"

Change 2000 to 200 takes 5 minutes. If you want you can do it!

But we can do better, calculating EV (ie Adaptation scene luminosity cd/m2) with Exif,
it will take more than 5 minutes!

Now I would not change the "tone-mapping code" only if you (not you DrSlony in particular),
but you (general demand) ask me!

:)

Reported by jdesmis on 2013-04-06 16:27:46

Beep6581 commented 9 years ago
Works fine here

Reported by entertheyoni on 2013-04-06 21:32:08


Beep6581 commented 9 years ago
Same patch with PP3 version bumped from 306 to 307

Reported by entertheyoni on 2013-04-06 21:34:04


Beep6581 commented 9 years ago
jdes

Is changing the default to 200 the end of this? I have a tutorial in draft where I
brought up this issue and changed the value several times to find the visual effect
I was after. 

When CIECAM02 was first written as an experimental system, knowing the cd/m2  value
was important. But as it is used now in RT, it is another slider like exposure and
contrast that is user adjustable. So I still think it need a tool tip explaining that
use.

scribble

Reported by scribble1@charter.net on 2013-04-07 05:00:22

Beep6581 commented 9 years ago
@DrSlony
when I saw you change all pp3 ... I said, "there's something else."

I looked at the code CIECAM, and there is an error in the return TM ==> CIECAM (calculating
J from Q)
Thus the values ​​returned with La=2000 correspond to the ones you have with your patch
and La=200

I apologize for this error.

@scribble 
I would add a tooltip

Reported by jdesmis on 2013-04-07 05:22:02


Beep6581 commented 9 years ago
After this patch this slider behaves more consistently w/without tonemapping and global
brightness with CIECAM tonemapping seems correct.

Reported by michaelezra000 on 2013-04-07 22:04:25

Beep6581 commented 9 years ago
I confirm that issue1827_TM.patch solves the issue. Thank you!

Reported by entertheyoni on 2013-04-07 22:57:35

Beep6581 commented 9 years ago
Thank you for yours tests.

I Changed also the "double" version of CIECAM

Commit to default :)

Reported by jdesmis on 2013-04-08 04:54:25

Beep6581 commented 9 years ago
I'll work on the slider "Adaptation scen luminosity" ==> make it sensitive to the values
​​EV (exif) and add a tooltip...in the next days 

Reported by jdesmis on 2013-04-08 05:01:43

Beep6581 commented 9 years ago
I'll close this so it no longer blocks 1818, and the slider enhancement can go into
an issue of its own.

Reported by entertheyoni on 2013-04-08 12:01:44

Beep6581 commented 9 years ago
OK

Reported by jdesmis on 2013-04-08 15:05:14

Beep6581 commented 9 years ago
Thanks Jacques:)!

Reported by michaelezra000 on 2013-04-08 22:04:13