Beep6581 / RawTherapee

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

Precision loss in LCMS when icc has table-based gamma #2065

Open Beep6581 opened 9 years ago

Beep6581 commented 9 years ago

Originally reported on Google Code with ID 2081

When converting from working space to output space a LCMS 16 bit conversion is made.
From a linear space if I understand correctly so we lose precision in shadows directly,
and if we convert from a large to a small space (say Prophoto to sRGB) there may be
some further precision lost. Not a huge thing, but we do have floting point internals,
so why waste that.

The attached patch instead makes a float->16 bit conversion and interleaves lab->xyz
with LCMS conversion, so we gain a little performance too despite floating point math.

Old 16->16 took 1400 ms and new float->16 1200 ms with a test image on my box.

Reported by torger@ludd.ltu.se on 2013-11-23 09:57:20


Beep6581 commented 9 years ago

Reported by torger@ludd.ltu.se on 2013-11-23 09:58:39

Beep6581 commented 9 years ago

Reported by torger@ludd.ltu.se on 2013-11-23 10:00:06

Beep6581 commented 9 years ago
I can confirm a very small speedup from 2511 ms to 2480 ms for a D800 NEF using LCMS
2.3

Reported by heckflosse@i-weyrich.de on 2013-11-23 11:35:14

Beep6581 commented 9 years ago
I tried to measure precision gain by converting image with extreme shadow detail with/without
the patch, but don't see differences for TIF-16 output using ProPhoto.

Difference visualization method - in Photoshop paste one image on top of another.
Execute the attached action.

Reported by michaelezra000 on 2013-11-23 12:45:09


Beep6581 commented 9 years ago
Thanks for testing. I'll test too, haven't done that yet. To detect differences you'd
need ProPhoto working space and sRGB output space, and 16 bit format. And LCMS would
have to do a proper conversion which also is an unknown.

Reported by torger@ludd.ltu.se on 2013-11-23 15:42:40

Beep6581 commented 9 years ago
Done some tests with a histogram counter inside so I can see how many unique input values
and output values there are.

Seems to be that LCMS throws away bits even if float in and float out (tried to alter
the code to that). Without LCMS (no output profile, only matrixing to sRGB) there's
about as many unique input as output values, but with LCMS doing the conversion to
sRGB there's a drop of unique values from about 55000 to 33000 in the green channel
in my test image.

I have version lcms 2.2 on my box.

Reported by torger@ludd.ltu.se on 2013-11-23 16:32:52

Beep6581 commented 9 years ago
Looked around in the LCMS code and it's full of function pointers so it's not too easy
to follow. There are some shortcuts through integer math here and there so I guess
we hit some of that.

What we probably should do is to wrap LCMS and detect if it's a matrix only conversion
(nearly always) and if so do that ourselves, both faster and better precision. If it's
a LUT-based transform we are probably at a later stage where one can sacrifice some
resolution.

Reported by torger@ludd.ltu.se on 2013-11-23 16:53:12

Beep6581 commented 9 years ago
Anders, was this distinct-levels decrease with RTsRGB as output profile ?

Reported by iliasgiarimis on 2013-11-23 17:07:59

Beep6581 commented 9 years ago
yes

Reported by torger@ludd.ltu.se on 2013-11-23 17:16:37

Beep6581 commented 9 years ago
ahh... maybe it's the gamma curve... I'll check.

Reported by torger@ludd.ltu.se on 2013-11-23 17:17:13

Beep6581 commented 9 years ago
Yes, it's very likely poor gamma curve handling in LCMS. When there's a fixed gamma
constant, like in AdobeRGB.cc, there's no loss in values, but if I use RT_Medium_gsRGB
(same size of color space, but gamma specified as a 4096 point curve) I get a loss.

However, I think the solution to this problem is to make own conversion instead of
using LCMS if the following is satisfied:
 - output profile has no LUT (ie only matrices)
 - rendering intent is relative (ie no gamut mapping tricks, just clip out of gamut

   colors)
 - there's only one gamma curve

ie 99% of the cases.

Reported by torger@ludd.ltu.se on 2013-11-23 17:32:35

Beep6581 commented 9 years ago
... and/or report upstream so that the problem can be fixed in LCMS. Note that I haven't
tested with the bleeding edge LCMS.

Reported by torger@ludd.ltu.se on 2013-11-23 17:33:11

Beep6581 commented 9 years ago
tested with lcms 2.5, same issue there.

Reported by torger@ludd.ltu.se on 2013-11-23 18:28:46

Beep6581 commented 9 years ago
Argrghh this is the second time I'm inside lcms2 searching for bugs. Function pointers
everywhere! Impossible to navigate.

Reported by torger@ludd.ltu.se on 2013-11-23 18:43:18

Beep6581 commented 9 years ago

Reported by torger@ludd.ltu.se on 2013-11-23 22:03:32

Beep6581 commented 9 years ago

Reported by torger@ludd.ltu.se on 2013-11-23 22:04:01

Beep6581 commented 9 years ago
I think I've found the bug in lcms2. It's in the function "cmsEvalToneCurveFloat" which
despite it's name make a direct call to the 16 bit tone curve function if the curve
is table based. Ie the intermediate values are interpolated in 16 bit space, not floating
point.

I've reported it on lcms list, but I suspect the answer will be that "don't use legacy
table based curves" and that it's not a bug but design choice. We'll see.

Reported by torger@ludd.ltu.se on 2013-11-23 23:17:28

Beep6581 commented 9 years ago
I got response from the LCMS author and it was as I expected, he has no intention to
fix the bug, as he don't think it is a bug. He thinks that as the curve is specified
with integers the output should be integer too. He thinks we should use v4 profiles.

I don't agree, legacy profiles are everywhere and there's no reason a CM lib should
punish use of them. But it's not much we can do about it. LCMS is as it is.

One way to solve it is to remove the tables from RT_sRGB etc and instead use single
parameter gamma. For Prophoto and gamma 1.0 we can surely do it, for sRGB if we want
bu then the gamma won't be true sRGB.

Another way to do it is to implement conversion of these profiles natively in RT as
it's quite simple. I think tables should be removed from the profiles where they describe
a pure single value parametric curve anyway though.

Reported by torger@ludd.ltu.se on 2013-11-24 07:53:12

Beep6581 commented 9 years ago
Prophoto uses complex curve also. Linear with a slope of 16 up to Et = 16^(1.8/(1-1.8))
= 0.001953 .. Is this linear part optional ?.

http://en.wikipedia.org/wiki/ProPhoto_RGB_color_space

Reported by iliasgiarimis on 2013-11-24 15:10:03

Beep6581 commented 9 years ago
Yes, I'm quite sure it is. If you bring up an old prophoto.icc and an AdobeRGB.icc both
have their curves defined as a single exponential, ProPhoto as 461/256 ~ 1.8 and AdobeRGB
as 563/256 ~ 2.19.

Internally in RT for curves etc we use a fixed gamma which is 569/256 ~ 2.22 which
is an approximation of sRGB gamma. If you bring up a legacy srgb.icc it will not have
a single exponential curve though, it will have a 256 point array describing a curve
with a linear start.

Reported by torger@ludd.ltu.se on 2013-11-24 15:21:52

Beep6581 commented 9 years ago
I'm postponing this (won't commit a patch now), as it's not many practical use cases
where it has any meaning. As output is put into 16 bit anyway it's hard to see any
reduction in precision even if it's there.

However I really hate throwing away any single bit so it's likely I'll make that matrix
shaper implementation anyway, even if most users won't have any practical use concerning
precision (at least until we support floating point tiff output) performance will be
better.

Reported by torger@ludd.ltu.se on 2013-11-24 16:26:04

Beep6581 commented 9 years ago
Shall add that it's a bit hard to analyze precision issues by just looking at unique
value counts as I've done. For example with 16 bit input with 6700 values I got 20000
unique 16 bit output values, ie new values created by the conversion, which can be
normal due to rounding stuff in interpolation I guess.

Knowing that precision *is* lost is quite easy though, it's just math. Truncating down
to 16 bit linear space and then convert to 16 bit gamma space will be at cost of precision
compared to converting from float and map to 16 bit in the end. So we'll see if I manage
to keep my paws away or get into this again.

As an author of an HDR software I *really* don't like to throw away bits, just knowing
that it happens make me sleep worse :-)

Reported by torger@ludd.ltu.se on 2013-11-24 16:41:15

Beep6581 commented 9 years ago
According to Marti it's a comptability thing which makes it necessary to make those
calculations in integer mode, so we can't expect to get that from LCMS.

Reported by torger@ludd.ltu.se on 2013-11-24 18:07:06

Beep6581 commented 9 years ago

Reported by torger@ludd.ltu.se on 2013-11-25 15:56:34

Beep6581 commented 9 years ago
With Rt 4.0.12.70 - Revision: cdeb7d15824b 

Discovered that on the synthetic DNG gray scale
http://cl.ly/3F2v0T0j2x3P/RGB_255to0_52steps_Bayer.dng.zip
http://translate.google.com/translate?sl=ru&tl=en&js=n&prev=_t&hl=el&ie=UTF-8&u=http%3A%2F%2Fsail2ithaki.livejournal.com%2F161042.html

Some greys are still not totally grey. This with almost all output profiles except
"no_icm:sRGB output"

The most significant deviations are at the dark tones when using linear gamma . Blacks
look to get crashed sometimes ..
(I like to use linear output many times when I downsample because we get better definition
this way.. http://www.4p8.com/eric.brasseur/gamma.html) 

I disabled "avoid color shift" and used working space: sRGB - output: RT_sRGB_g10
expected = raw/65535
  raw   expected    RT
    0    0.00%     0.0%
   12    0.02%     0.0%
   53    0.08%     0.0%
  129    0.20%     0.0%R, 0.4%G, 0.0%B
  243    0.37%     0.4%  
  397    0.61%     0.8%
  592    0.90%     0.8%
  831    1.27%     1.2%
 1115    1.70%     1.6%
 1445    2.20%     2.4%
 1821    2.78%     2.7%
 2246    3.43%     3.5%
 2720    4.15%     4.3%
.....

Reported by iliasgiarimis on 2014-03-13 14:36:19