Beep6581 / RawTherapee

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

"Preferred DCP Profile" setting is becoming obsolete(?) #2032

Closed Beep6581 closed 9 years ago

Beep6581 commented 9 years ago

Originally reported on Google Code with ID 2048

I'm currently looking at improving DCP support.

In that is adding matrix interpolation based on white balance setting, which is mandated
by the DNG standard. This means that you derive new matrices based on the two you typically
have, which generally is for StdA and D65. So if the user asks for D50 you interpolate
a mix between the two.

Looking at the code this seems to render the "Preferred DCP Profile" (= Preferred DCP
Calibration Illuminant) setting obsolete, as this is not intended to be configurable
if following the standard.

What I could do is to have "WB interpolated" as one option, which would be the default
and the standard DNG behavior (ie automatically follow your white balance setting with
interpolated matrices and huesatmap), and then one could keep the possibility to override
that by hard-setting to one calibration illuminant.

Another alternative would be to remove the setting alltogether. I'd prefer that, but
I don't know the rationale behind it, and what the pp3 compatibility policy is.

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

Beep6581 commented 9 years ago

Reported by torger@ludd.ltu.se on 2013-11-13 15:46:55

Beep6581 commented 9 years ago
As the bundled DCPs only use one matrix and thus the setting has not been used for those,
I think it would be uncontroversial to just remove it.

Reported by torger@ludd.ltu.se on 2013-11-13 19:42:46

Beep6581 commented 9 years ago
For sure it would be a serious compatibility break... But:

1. there would be no point to provide a solution that is worse than the default solution
in our first Stable release after the several year olds V3 Stable

2. how different the image would be if the profiles where interpolated instead of selected?
Regarding the WB, is the work done by the WB tool or by the DCP profile? Or both?

Reported by natureh.510 on 2013-11-15 05:09:02

Beep6581 commented 9 years ago
For the matrix interpolation one needs a white balance temperature. StdA is 2850 degrees,
D65 is 6500, if current temperature is 5500, ie closer to D65, a linear interpolation
is made a mix of 72% D65 matrix and 28% StdA matrix. Ie any changes in how the "current
temperature" is derived will cause changes in color rendering, although I'd say they
will be very slight.

I pick the current temperature as-is from the current ColorTemp, ie no change there.

In the DNG reference code a WB xy coordinate is derived from the DCPs color matrices
(two, one StdA and one D65) and the RGB multiplicators, and then some color model to
get a temperature from that. AFAIK RT uses the dcraw D65 matrix and the multiplicators
and a model to get to a temperature. It's not unlikely that we would like to improve
RT's white balance model when there are more than one color matrix available, and when/if
that improvement is made there will be another slight shift in color rendering (probably
undetectable in most cases).

The largest difference will occur with a temperature in the middle between StdA and
D65, ie 4700 degrees. I have not really checked how large difference it is, I think
it's small, but haven't made an A/B test.

Another alternative concerning white balance is to only pick the multiplicators into
the DCP code and make an internal white balance calculation, taken from the DNG reference
code, and use that only for DCP matrix derivation. Then we get the same behavior as
the DNG reference and it's not affected by any white balance model changes inside RT.

Actually I'm leaning towards this solution now, as the DNG specification on page 80
actually specifies this white balance model, ie you're supposed to use this model when
mixing the matrices, and any multi-matrix DCP building tool would expect the software
to follow the DNG specification.

My overall goal with the DCP code is to "follow the DNG specification", and consider
any deviations as bugs.

Following the DNG specification means to use their white balance model and do mixing.
The option to select color matrix is not supposed to be there. We could keep it as
a "debug" or interesting testing feature, actually that could be a good idea, but I
strongly suggest that the default behavior should be matrix mixing according to the
specification.

I will update the DCP patch so it uses the DNG spec white balance model.

Reported by torger@ludd.ltu.se on 2013-11-15 09:15:45

Beep6581 commented 9 years ago
No I will not update the DCP patch :-), added a new issue for it instead, issue 2037,
as I think it's better handled separately after studying the issue in more detail:
current wb handling is compliant with DNG 1.2. We need some refinements as mentioned
for DNG 1.4 compliance, but we can expect very tiny changes in color rendering as a
result of that, and which will only take effect when a dual matrix DCP is loaded.

Still need to decide on:

A) keep "preferred DCP Profile" but add "interpolated" as default option
B) remove "preferred DCP Profile" setting
C) do A) and fix it so it shows correct names of the calibration illuminants and what's
actually in the chosen DCP, and is renamed in gui to the more correct "Preferred DCP
illuminant"
D) do nothing, ie don't follow the DNG spec and never interpolate matrices

I prefer B or C, think D is really bad and A is quite bad. 

C will mess up compatibility too, since the current code does not look at what actually
is available in the profile, but you have the same choices anyway, and none of them
actually matches the typical content which is StdA and D65. Ie, the current setting
does not make sense! Confusing name, confusing contents, and against the DNG spec.

Concerning changes color rendering and backwards compatibility I see this issue like
this: removing it will not affect bundled profiles, as those have only one matrix,
concerning third-party profiles (ie nearly always from Adobe) current behavior should
be seen as a bug not a feature, and as color rendering change by implementing correct
behavior can be assumed to be quite small. I think we can live with possibly upsetting
those having used the "wrong" matrix in third party profiles to create some effect.

Reported by torger@ludd.ltu.se on 2013-11-15 11:22:28

Beep6581 commented 9 years ago

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

Beep6581 commented 9 years ago
Option C seems to be the best to me. Regarding the name of the "Preferred DCP illuminant",
one could use a concatenation of the techincal name and description... although anyone
using this option would probably know the technical name.

Reported by michaelezra000 on 2013-11-15 12:53:25

Beep6581 commented 9 years ago
Well, I realize it would be "DCP Illuminant" only, as "preferred" comes from the fact
that the current tool does not relate to what's in the file, it picks one which is
most similar to the preferred.

The advantage of that is that regardless of DCP profile you choose you can keep the
same setting. I don't like to keep a setting that don't make sense though. It would
make more sense to replace it with a mix slider how you would mix the color matrices,
but I just don't see what value it would provide.

The reason the setting is there in the first place is as far as I understand a hack
when it was discovered that Adobe's profiles actually contains two matrices, but the
actual DNG spec was never read. If you read the DNG specification you clearly see that
dual matrices is a very normal case, and you also clearly see that it's specified how
the matrices should be mixed, and mixing them otherwise or using only one of them is
breaking the standard and I don't see why we would have to do that for this setting.

Reported by torger@ludd.ltu.se on 2013-11-15 13:05:07

Beep6581 commented 9 years ago
I recall that Olli did not interpolate between the illuminants as there was criticism
regarding Adobe's approach, hence a single matrix approach was chosen.

Reported by michaelezra000 on 2013-11-15 13:34:37

Beep6581 commented 9 years ago
Okay. I'm no color science expert but I do not find it surprising that someone has raised
criticism to the approach, as it does seem a bit simplistic to just linearly interpolate.

In terms of winning DNG acceptance rigid color models which can be easily criticized
put into the standard may be a mistake. I also find it a big limitation of DNG/DCP
that only two matrices can be provided. I think MF digital backs typically have more.

Anyway I'm swaying over to your suggestion, keeping the setting but changing it slightly:

* Renaming it in GUI to "DCP Illuminant"
* Only making it active when there is more than one illuminant in the DCP, otherwise
  disabled
* Show the actual illuminant names
* Have "DNG Interpolated" as default, which is the DNG spec way to do it

What do you think?

Reported by torger@ludd.ltu.se on 2013-11-15 13:48:52

Beep6581 commented 9 years ago
All sounds good:)
About the default, considering that custom defaults can be made anyway, this will be
most automated. 

If I were to make a DCP for specific lighting conditions, 
 - would DNG interpolation still take place? 
 - what illuminant would show in RT as available option?

Reported by michaelezra000 on 2013-11-15 13:56:33

Beep6581 commented 9 years ago
The Illuminant comes from the "CalibrationIlluminant[1|2]" tag which is exif standard
and has only a fixed set of values.

If I read the standard correctly you cannot specify any other illuminant than those
fixed values, which are these by the way:
http://www.awaresystems.be/imaging/tiff/tifftags/privateifd/exif/lightsource.html
(DNG ref code has its own way to translate the non-obvious of these to a temperature
value which I've borrowed, but I don't think it's a part of any standard)

If you make a custom DCP you make it with only one matrix, like all current RT's bundled
DCPs, and then no interpolation can take place, and the "DCP Illuminant" drop-down
will be disabled. You need two matrices to interpolate. The only reason to make it
with two matrices is to make a DCP that can adapt to different lighting conditions,
good for bundled DCPs, but does not make sense when you make a custom one for a specific
light condition.

With a custom DCP with strange lighting you'll probably set illuminant to 0, ie unknown.
It does not matter anyway as it won't be used and won't be displayed.

Reported by torger@ludd.ltu.se on 2013-11-15 14:12:30

Beep6581 commented 9 years ago
(The illuminant-to-temperature conversion affects how interpolation is made, so it's
a mistake in the DNG specification to not include that, while being rigorous on the
whitepoint to temperature conversion formulas.)

Reported by torger@ludd.ltu.se on 2013-11-15 14:17:14

Beep6581 commented 9 years ago
Since you've already worked on that solution and that Michael is okay, i'll add my vote
to solution C, even if solution B wouldn't have annoyed me much, or any of our numerous
user [ ...silence and smiles in the room... ] that are using dual illuminant DCP profiles...

Reported by natureh.510 on 2013-11-15 14:48:16

Beep6581 commented 9 years ago
:-)

As usual I had an egoistic reason for starting working with this, I do use Adobe's
profiles and have wanted to use the "Neutral" ones. I've realized that are not so many
with me as I thought though :-), despite that you can get these profiles for free via
Adobe DNG Converter.

Anyway, I also noted that currently "preferredProfile" is saved as a boolean, so it's
already broken as is. Should I add a new entry called "DCPIlluminant" or should I change
preferredprofile to an integer, or what should I do? I'm guessing adding a new entry.

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

Beep6581 commented 9 years ago
became messy, so I got a bit tempted to remove it, but on the other hand being able
to play with this is just what I think RawTherapee is about, providing a little bit
more control than the general converter, so it should be there, whatever it takes!
;-)

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

Beep6581 commented 9 years ago
Interesting to note: forward matrices are almost always the same for adobe profiles
(haven't found a single one that's different), ie the setting have no effect at all!

Colormatrices are different though, but that only has effect when calculating white
balance temperature (which is not implemented yet), it's not used for color correction
as any new DCP has a forward matrix (which takes precedence)

As the setting does not seem to have any effect on any current profiles I'm now getting
back to that we just remove it.

What do you think?

Reported by torger@ludd.ltu.se on 2013-11-15 17:03:14

Beep6581 commented 9 years ago
Can stay low until after the weekend, I'm going to have a computer-free weekend now.

Reported by torger@ludd.ltu.se on 2013-11-15 17:04:55

Beep6581 commented 9 years ago
>As the setting does not seem to have any effect on any current profiles I'm now getting
back to that we just remove it.

I recall that this setting had effect when switching between Daylight and Tungsten
when using the dual illuminant profiles.

Reported by michaelezra000 on 2013-11-15 17:09:36

Beep6581 commented 9 years ago
Enjoy the weekend!:)

Reported by michaelezra000 on 2013-11-15 17:09:55

Beep6581 commented 9 years ago
Re #15: Save to the new format, but Read both (and convert if necessary), depending
on the PP3VERSION (that's whats it's used for after all :) ).

Reported by natureh.510 on 2013-11-15 17:18:17

Beep6581 commented 9 years ago
#20: the reason for that was because the previous version used the color matrices only,
but this is incorrect behavior (that's why you got crazy colors with many of adobes
DCP profiles). If forward matrices are available those should be used, because the
correction tables will then be adapted for those.

RT's bundled profiles only have one ColorMatrix, and no ForwardMatrix. Adobes have
2 CM and 2 FM, CMs are different FMs are equal.

I noticed this when already implemented the feature, so the job is already done :-),
but it won't have any effect for current profiles. Maybe on Adobe's latest profiles...
they must have 2 FMs for a reason. It won't get worse than the current anyways, as
it was broken.

Reported by torger@ludd.ltu.se on 2013-11-15 17:21:41

Beep6581 commented 9 years ago
Here's the patch, extremely little tested, if you want to have a look on how the tool
became. The patch has a fuji dcraw fix in it too (just a diff on my whole tree)

Reported by torger@ludd.ltu.se on 2013-11-15 17:24:43


Beep6581 commented 9 years ago
#22: leaning towards not reading it at all (the patch does that) in this particular
case, as the setting was broken. It was read/written as a bool but there were 4 choices
in the combobox.

Reported by torger@ludd.ltu.se on 2013-11-15 17:37:01

Beep6581 commented 9 years ago
#21 thanks! If you see a post from me this weekend I've failed, this is the last before
Monday, I promise ;-). It's been waaay too much screen time for me the past month.

Reported by torger@ludd.ltu.se on 2013-11-15 18:05:37

Beep6581 commented 9 years ago
(Ok, I failed keeping away)

I just got a response from Eric Chan (one of the Lightroom developers) concerning forward
matrices. In some cases color correction is better handled through the tables only,
and in those cases both forward matrices will be the same.

I've also got examples of when forward matrices are different. So the work is not in
vain, the interpolation code and the calibration illuminant combobox will provide a
function :-).

(I don't know why I see only the same matrices in all my DCPs both with my code and
with dcptool, but I've copied around DCPs on my development box so I'm not sure what
they come from, could be that old versions of DNG converter has simpler DCPs that new
Lightroom. I'll check with the DCPs provided with my Lightroom installation later.)

Reported by torger@ludd.ltu.se on 2013-11-16 09:37:10

Beep6581 commented 9 years ago
Found out the reason to my DCP confusion.

Adobe has two profiles directories one called "Adobe Standard" and one called "Camera".
In the Adobe Standard directory there's only one profile per camera providing a "default
look" (ie the Adobe look, afaik it's not intended to be neutral but to be a specific
look which is the same (within reason) regardless of camera you use), while in the
Camera directory there's several profiles with different looks (Landscape, Portrait,
Neutral etc). Against better knowledge I had ssumed that the "Camera" profiles would
be the more advanced and detailed, but turns out to be the other way around.

The "Adobe Standard" profiles have separate forward matrices and two huesatdeltas tables
and a looktable too, while "Camera" profiles have equal forward matrices, no huesatdeltas
at all, but a looktable, meaning that with Camera profiles there can be only one look
despite dual illuminants, but with Adobe Standard you get the full functionality of
a dual illuminant DCP profile.

When I get back to the development box I'll do testing with Adobe Standard profiles
so I actually get to test all my implemented functionality...

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

Beep6581 commented 9 years ago
I'm having huuuuuge problems to get this setting to work. It's so frustrating getting
stuck on a damn combobox.

Problem seems to be that the settings architecture is not really designed for dynamic
choices, ie that the contents in the combobox changes depending on choice of DCP. It
becomes a problem in the thumbnail browser. Signals go around and cause general chaos.

Reported by torger@ludd.ltu.se on 2013-11-17 17:25:58

Beep6581 commented 9 years ago
I have something working now.

When selecting multiple files and changing settings in the file browser you cannot
do every operation, eg you can't change to "second illuminant" unless you also change
to a DCP which has dual illuminants (or have one already selected). I think it's "good
enough" for such a rarely used option.

Reported by torger@ludd.ltu.se on 2013-11-17 19:00:55


Beep6581 commented 9 years ago

Reported by torger@ludd.ltu.se on 2013-11-17 19:02:51