bugfixes and color accuracy changes for branch_3.0

[Beep6581]

Originally reported on Google Code with ID 414

Purpose of code changes on this branch:
- correct gamma curve constants for sRGB (in rtengine/curves.h)
- improve code readability by using more brackets in several cases
- change sRGB matrices to Bruce Lindbloom's standards 
  to improve on color accuracy (see http://www.brucelindbloom.com/)
- remove duplicate lookup tables with xcache and zcache.
- trim lookup tables to actual range used.
- fix hsv band selection (value 8092 should be 8192)
- improved highlight correction with better color preservation and
  logarithmic roll off to better match human visual perception.
- improve accuracy of RGB transformations by using floats
- use constant chroma scale to ensure Lab values are accurate.
- fix clipped raw values below rgb max, this fixes color casts in
  highlights that are supposed to be white.
- fix situation of rotated tiff files causing a crash

When reviewing my code changes, please focus on:

- color accuracy and tonal differences when using highlight compression
- please check stability, the changes should not affect stability.
- the use of floats can decrease speed slightly
- HSV equalizer should be a bit more accurate now.
- it may be my imagination but it seems i see reduction
  in noise levels overall
- 100% highlight compression + large exposure compensation now creates
  (i.m.h.o.) a very pleasing HDR effect.

To improve accuracy further i have some interesting things still waiting here like
dithering for 24bit color and full float processing in RGB.

Please feel free to comment on this patch and try it out in combination with branch_3.0
Even though i think the results i get with this patch are really good, there may be
many oversights which i have not been aware of.

Reported by janrinze on 2010-12-16 13:33:55

---

• Attachment: branch30.patch

[Beep6581]

Issue 359 has been merged into this issue.

Reported by oduis@hotmail.com on 2010-12-23 08:53:53

[Beep6581]

I have now myself tested my test image with a floyd-steinberg dithering (implemented
separately from RT, so I have no RT patch), and it does work removing the banding.

Unique color map: http://torger.dyndns.org/temp/fs-map.png
Actual image: http://torger.dyndns.org/temp/fs.png

Didn't handle clipping in my quick implementation so overexposed colors are mishandled.

Note that there are still quite large single color fields, but the transition into
them is really smooth and dithered so it does not seem to be a problem (I'm not on
a calibrated screen for a few weeks though...). My quick hack also handles each R,G,B
channel separately. My goal of this test was just to verify that dithering will work
on a real image rather than making the perfect implementation, and yes it does seem
to work very well.

Previous to this test I thought random dither would be better suited for photos, but
now I think FS is the way to go, in this context it is subtle enough not to give an
"artificial look" like a geometric dither pattern otherwise can do. FS will also work
considerably better than random dither in low res images, so it can be used for thumbnail
display etc.

Reported by torger@ludd.ltu.se on 2010-12-23 14:52:32

[Beep6581]

Here is a patch that includes most of Jan's changes except a bit of the treatment of
the Lab conversion (mostly, restoring D50 illuminant factors to the rgb to xyz conversion).
 Seems to avoid many of Michaels issues in comment #18.  Please test.

Reported by ejm.60657 on 2010-12-24 05:02:40

---

• Attachment: janrinze-mod1.patch

[Beep6581]

my local working changeset for branch 3.0 is 
725:87b060bb962d (Fixed a crash when RT was started with command line and being in
single tab mode; see issue #420)

when compiling with janrinze-mod1.patch I get this error:
[  8%] Building CXX object rtengine/CMakeFiles/rtengine.dir/improcfun.cc.obj
C:\DevTools\SourceCode\rawtherapee_branch_3.0\rtengine\improcfun.cc: In member function
'void rtengine::ImProcFunctions::rgbProc(rtengine::Image16*, rtengine::LabImage*, float*,
fl
oat*, int*, rtengine::SHMap*, int)':
C:\DevTools\SourceCode\rawtherapee_branch_3.0\rtengine\improcfun.cc:307:44: error:
'defmul' was not declared in this scope
mingw32-make[2]: *** [rtengine/CMakeFiles/rtengine.dir/improcfun.cc.obj] Error 1
mingw32-make[1]: *** [rtengine/CMakeFiles/rtengine.dir/all] Error 2
mingw32-make: *** [all] Error 2

should I get the latest branch 3.0 changeset first?

Reported by michaelezra000 on 2010-12-24 16:27:18

[Beep6581]

No that was my mistake.  Here is something that should compile correctly:

Reported by ejm.60657 on 2010-12-24 16:50:51

---

• Attachment: janrinze-mod2.patch

[Beep6581]

janrinze-mod2.patch compiled, but everything  has magenta cast now - thumbnails which
were previously generated and newly opened images.

Reported by michaelezra000 on 2010-12-24 17:10:39

[Beep6581]

The results of highlight.patch are excellent!

Reported by michaelezra000 on 2010-12-24 17:25:29

[Beep6581]

Emil, I agree that the UI control you mentioned for the start of highlight rolloff would
really add to the tool. I could try to add, but not sure if my code base is 100% suitable.

Reported by michaelezra000 on 2010-12-24 17:36:06

[Beep6581]

Here is a modification of the highlight patch incorporating a bugfix, so that it properly
takes into account highlight reconstruction coding.

Michael, I will need to think what is the best way to modify the tone curve Jan implemented
in order to put that in place.

Reported by ejm.60657 on 2010-12-24 17:57:42

---

• Attachment: highlight-2.patch

[Beep6581]

I think the rest of Jan's changes might be better done with his participation, rather
than my trying to figure them out.  Besides, I am toying with the idea of rewriting
the RT pipe to use floating point representation throughout; it's what I would have
preferred from the beginning, and I get annoyed every time I have to work around the
short integer data representation, with its mandatory clipping of data at intermediate
steps and other error propagations.  

Let's think about adding the shoulder point slider and associated code in rgbProc,
then I think I'll probably leave the rest alone.

Reported by ejm.60657 on 2010-12-24 18:09:25

[Beep6581]

Here is a comparison between highlight.patch and highlight-2.patch:
http://www.timelessme.com/temp/postings/RT_Issue-414_02.jpg

Reported by michaelezra000 on 2010-12-24 19:29:43

[Beep6581]

Here is a temporary patch to test the idea of a highlight shoulder slider, suitable
for when the black point is set at zero.  I used the setting on the shadow recovery
slider to adjust the point in 0-255 where the highlight compression starts (0 gives
the original behavior, where compression starts at 0; 100 gives the shoulder at 255,
so squeezes the compression into the white point).

So keep the black point at zero and use the shadow recovery slider in order to set
the point at which highlight compression starts.

Reported by ejm.60657 on 2010-12-24 20:05:16

---

• Attachment: highlight-4shoulder.patch

[Beep6581]

I see the threshold effect from the shadow recovery slider. 
At EC+5, Highligt rec 100 and shadow rec=50 there is inversion of contrast on the streak
of light on the ground left to the horse

Reported by michaelezra000 on 2010-12-24 20:55:13

[Beep6581]

I tested with various images it seems that contrast inversion does not seem to occur
when shadow recovery slider is =<25.

Reported by michaelezra000 on 2010-12-24 22:47:37

[Beep6581]

One way to analyze this would be to use as an input an image with a linear BW gradient,
convert in RT and then measure the brightness profile of the resulting image.

This gives me an idea for new RT feature: to draw a luminance graph vs pixels against
a user inputted line. This would be very helpful for image analysis and also for more
controlled adjustments of parameters,allowing to monitor areas of interest at greater
detail.

Reported by michaelezra000 on 2010-12-24 23:07:22

[Beep6581]

I am finding the test file from Bruce Lindbloom's site that contains all 8-bit colors
to be quite illuminating as to what the controls are doing.  I think I gave the link
earlier.

Reported by ejm.60657 on 2010-12-24 23:24:18

[Beep6581]

Could you show what you mean by the contrast inversion?

Reported by ejm.60657 on 2010-12-25 06:14:53

[Beep6581]

Take a look at this: http://www.timelessme.com/temp/postings/RT_Issue-414_03.jpg

Reported by michaelezra000 on 2010-12-25 16:10:00

[Beep6581]

This may be a bit better:

Reported by ejm.60657 on 2010-12-25 17:45:04

---

• Attachment: highlight-5shoulder.patch

[Beep6581]

This one is excellent. 

I am almost tempted to leave highlight recovery at 100 at all times and control this
setting alone:) I providing both controls in UI would give really great flexibility
for the rolloff. This should be named (and translated) correctly to make sure that
users won't get lost between these two sliders.

Reported by michaelezra000 on 2010-12-25 22:19:57

[Beep6581]

this patch works really well with image I sent earlier: VT_200910_054.NEF
btw, the black slider stopped working

Reported by michaelezra000 on 2010-12-25 22:25:27

[Beep6581]

If we leave 'highlight recovery' as is, I would suggest 'highlight threshold' for the
other.  Or, better yet, 'highlight recovery amount' and 'highlight recovery threshold'.

The black slider stopped working because I disabled the shadow tone controls (black
point and recovery) until the 'highlight recovery threshold' control gets its own slider,
otherwise you are adjusting two things with the same slider and don't know which is
the intended effect.

Reported by ejm.60657 on 2010-12-25 22:51:39

[Beep6581]

I think 'highlight recovery amount' and 'highlight recovery threshold' are perfectly
descriptive.

Reported by michaelezra000 on 2010-12-25 23:05:15

[Beep6581]

Great to see that this has spawned interest!
The highlight roll off might be one of the best and easiest parts to implement.
I will get back here in due time.

Reported by janrinze on 2010-12-25 23:07:46

[Beep6581]

Michael, since you wanted the extra slider for highlight threshold, would you like to
do the UI legwork?

Reported by ejm.60657 on 2011-01-02 08:58:20

[Beep6581]

Hi Emil! Happy New Year! Certainly, I will upload the UI patch here. I will take highlight-5shoulder.patch
as a base, unless there are any further algorithmic changes.

Reported by michaelezra000 on 2011-01-02 15:07:35

[Beep6581]

No changes, I left it alone to go do other things, but this issue should be finalized.

Reported by ejm.60657 on 2011-01-02 15:37:03

[Beep6581]

Here is the patch containing both the highlight-5shoulder.patch and UI changes together.

The new slider needs to be wired to rtengine code and black & shadow recovery restored.

Reported by michaelezra000 on 2011-01-03 02:43:10

---

• Attachment: highlight-6shoulder.patch

[Beep6581]

I just noticed, both contrast sliders also need to be restored, they work as corresponding
brightness adjustments.

Reported by michaelezra000 on 2011-01-03 02:49:57

[Beep6581]

Here is a patch that hooks up the new slider, see if it works for you.  Also fixed the
contrast bug.

Reported by ejm.60657 on 2011-01-03 06:06:52

---

• Attachment: highlight-7shoulder.patch

[Beep6581]

Everything works great! 
In the shot with the horse, the smoothness of tones in the write shirt is very pleasing
to the eye.

Reported by michaelezra000 on 2011-01-03 12:24:57

[Beep6581]

Do we have any outstanding for for this patch? Anything I could help with?

Reported by michaelezra000 on 2011-01-05 17:57:41

[Beep6581]

@Michael: Emil is working on his float branch which will incorporate some of the stuff
from this issue.
Apart from that there may be some bits and pieces that will go into the main branches
in the future.
For now these remain experimental since the real quality jump is not there yet.

Reported by janrinze on 2011-01-05 18:09:26

[Beep6581]

@janrinze: The highlight-7shoulder.patch seems to be self contained. Could this be committed
to branch 3.0 independently? There is a significant improvement in quality of highlight
compression introduced with this patch.

Reported by michaelezra000 on 2011-01-05 18:44:48

[Beep6581]

I agree that this patch is self-contained, has had no complaints for some time now,
and should be committed to default.  As for branch3.0, I personally would like to see
the behavior kept similar between the two, otherwise going forward people will experience
a dramatic change in behavior for the next major release, and thus for this change
to be implemented on 3.0 as well.

The float branch already has the highlight recovery code, but not the shoulder point
slider. 

Reported by ejm.60657 on 2011-01-05 18:50:58

[Beep6581]

I don't see any problem committing the highlight patch to 'default'

@michael: can you commit it yourself?

Reported by janrinze on 2011-01-05 19:52:39

[Beep6581]

janrinze, there may be merging involved during commit and I don't feel completely comfortable,
as may easily make a mistake with this patch. I would try my first commit (just got
the rights a few days ago) on something simpler to begin with.

Reported by michaelezra000 on 2011-01-05 21:22:30

[Beep6581]

no problem.
I will see if i can do the commit somewhere tomorrow

Reported by janrinze on 2011-01-05 21:36:12

[Beep6581]

Since I am familiar with the contents of the patch, I will look at it tonight.

Reported by ejm.60657 on 2011-01-05 22:21:49

[Beep6581]

LibRaw agrees:
http://www.libraw.org/articles/festina-lente.html

Reported by entertheyoni on 2011-01-05 22:46:51

[Beep6581]

I don't agree with the LibRaw article:

http://forums.dpreview.com/forums/read.asp?forum=1018&message=37356164

Reported by ejm.60657 on 2011-01-05 23:18:50

[Beep6581]

URL doesn't work for me. Will try again later.

Reported by entertheyoni on 2011-01-06 18:00:10

[Beep6581]

time to do some cherry picking from my patch

proposed fixes:
- correct gamma curve constants for sRGB (in rtengine/curves.h)
- remove duplicate lookup tables with xcache and zcache.
- trim lookup tables to actual range used.
- fix hsv band selection (value 8092 should be 8192)
- fix situation of rotated tiff files causing a crash

any other suggestions to pick from the patch?

Reported by janrinze on 2011-01-18 16:14:06

[Beep6581]

- fix situation of rotated tiff files causing a crash

has been committed to default and branch_3.0

Reported by janrinze on 2011-01-19 23:42:11

• Status changed: Started

[Beep6581]

Can we close this?

Reported by entertheyoni on 2011-11-06 17:49:51

[Beep6581]

Reported by janrinze on 2011-11-06 19:36:00

• Status changed: Fixed