Beep6581 / RawTherapee

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

White and black point database #1736

Closed Beep6581 closed 9 years ago

Beep6581 commented 9 years ago

Originally reported on Google Code with ID 1752

White and black point values are specific not only to each camera, but also to each
channel and at a specified sensitivity (ISO). This is a feature request for handling
of a database of camera white-point/black-point values.

Continuation of idea from issue 1695.

Reported by entertheyoni on 2013-03-04 03:00:12

Beep6581 commented 9 years ago
As it could take a long time until RT has solved this issue it would be useful to include
in the manual a small description of this issue and directions (to use DNG converted
raws or try to correct it by the raw WP & Black level controls).

Today I run on a Panasonic GH3 raw shot at ISO 500. The standard Dcraw's Black Point
is 143 while the correct for this ISO500 is 155, 153, 155, 153 ..
The result was a very bad conversion,  a magenta fog at the darks, no contrast and
noise visibility up.

Reported by iliasgiarimis on 2013-03-16 02:23:05

Beep6581 commented 9 years ago
Issue 1615 has been merged into this issue.

Reported by entertheyoni on 2013-04-06 00:39:18

Beep6581 commented 9 years ago
https://code.google.com/p/rawtherapee/issues/detail?id=1695#c44
https://code.google.com/p/rawtherapee/issues/detail?id=1695#c47
https://code.google.com/p/rawtherapee/issues/detail?id=1631#c3
https://code.google.com/p/rawtherapee/issues/detail?id=1631#c11

Reported by entertheyoni on 2013-04-06 00:39:21

Beep6581 commented 9 years ago
Issue 1005 has been merged into this issue.

Reported by entertheyoni on 2013-04-12 23:03:26

Beep6581 commented 9 years ago
For simplicity I think the file format should be a human-readable text file which is
distributed with RawTherapee and can be hand-edited, it's read at startup. Then users
can add their own changes if they can't wait for the next release. 

This sort of database is also something that needs user input, and you don't need to
be a programmer to contribute. Having it in an simple text file makes the threshold
lower to contribute.

I think XML is a bit clumsy to read for humans, so maybe the more lightweight JSON
format could be an alternative? Here's a suggestion:

"cameras": [
    "Make1 BasicModel": {
        "d65_matrix": [ 7530,-1942,-255,-4318,11390,3362,-926,1694,7649 ],
        "ranges": { "black": 10, "white": 1000 }
    },
    "Make1 AdvancedModel": {
        "d65_matrix": [ 7530,-1942,-255,-4318,11390,3362,-926,1694,7649 ],
        "ranges": {
            "black": [
            { "iso": "default", "levels": [ 10, 20, 10, 20 ] },
        { "iso": 3200,      "levels": [ 50, 60, 50, 60 ] }
            ],
        "white": [
            { "iso": "default", "levels": [ 10000, 11000, 10000, 11000 ] },
            { "iso": 3200,      "levels": [ 11000, 11000, 10000, 11000 ] }
            ]
    }
    }
}

The BasicModel contains just the data you find today in dcraw adobe_coeff(), a single
d65 color matrix, a single value black level, and a single value white level. The Advanced
Model contains per channel levels per ISO.

The advantage of the JSON format is that we can make parsing flexible, ie values may
be an array or may be a single value, so you only need to expand to arrays when the
camera needs it. The format can also be easily expanded to include new constant types
in the future.

The values in this database would then override hard-coded defaults found in dcraw
and RT code (if any). For simplifying maintenance we would probably move RT hard-coded
defaults to this file rather than having it in the code, and only leave the original
dcraw values hard-coded.

If you only would like to override white level but not black level or matrix, you would
just provide a white level for that camera.

Reported by torger@ludd.ltu.se on 2013-10-08 12:20:04

Beep6581 commented 9 years ago
Seems sensible, and I'm okay in the principle, but will we need to add a new library
in order to read AND write the JSON's file format? Why writing? Because it could be
used as a replacement file format of the procparams before doing the XMP stuff, given
that features like local editing, multiple procparams' set and HDR fusion needs a more
elaborated file format (i.e. tree like, with constants possibly becoming structures)
than plain keyfiles! The transition toward XMP support may be easier then :)

Reported by natureh.510 on 2013-10-08 12:49:43

Beep6581 commented 9 years ago
Yes we would need a library, haven't looked into that yet. When I've used JSON in other
projects I've used cJSON which is just one C file which you checkin and use in the
project rather than using some lib somewhere. The JSON format is lightweight and is
simple to parse thus no need for heavyweight lib that needs to be updated now and then.

A C++ library is JsonCpp, which is a bit larger probably want that as a lib.

Possibly one could start off with cJSON (ie no more dependency, just one more file
to compile) and then replace it later with some bigger C++ library if JSON get broader
use in the software.

Reported by torger@ludd.ltu.se on 2013-10-08 13:00:34

Beep6581 commented 9 years ago
cJSON supports writing too by the way, but I guess C++-oriented programmers would like
a genuine C++ library. (I'm myself mainly a C programmer.)

Reported by torger@ludd.ltu.se on 2013-10-08 13:02:14

Beep6581 commented 9 years ago
Hombre, is would't it be easier/more beneficial to adopt XMP formatting for the pp3
file, at least partially, instead of an entirely new format? XMP branch currently has
translation of pp3 to XMP. To avoid conflict with other XMP sidecars we could call
it .xmprt, which can remain in place until all XMP branch issues are resolved. Just
to be clear, what I am proposing is only the XML structure of the XMP.

Reported by michaelezra000 on 2013-10-08 13:19:10

Beep6581 commented 9 years ago
I think it's okay to have JSON only for this format, and for that cJSON is a good library,
I'll just include the single file to compile. I'd like to get this deployed quite soon
so we have a remedy for all those "incorrect white level for camera XYZ" bug reports,
and advanced users can start contributing black/white levels and matrices with minimum
effort from developers.

We can always change structure and/or file format later on, with this we get something
reasonably smooth to start gathering all these values.

Reported by torger@ludd.ltu.se on 2013-10-08 14:27:40

Beep6581 commented 9 years ago
A question, would it be worthwhile to also add lens distortion constants to this file,
intended for camera compacts with integrated lens?

It would be used as a complement to auto distortion correction and lens profiles available
today.

As you may know many of the modern compact cameras have huge barrel distortion at the
wide end (it's a new design trend, probably not going to go away), which is silently
corrected when the camera makes JPEGs, and also silently corrected by the commercial
raw converters.

Maybe we could have a simple lens distortion entry like this:

"lens_distortion": [
   { "focal_length":15.1, "amount":-0.0151, "edge_crop": 0.01 },
   { "focal_length":20.0, "amount":-0.0020 }
]

and then for example the "auto distortion correction" could use this value instead
of matching the jpeg, when available. Or it could be applied more automatically if
available (ie no need to press a button). As many high end compacts are designed this
way today, this could be a good feature to have.

This is also the type of data that works very well for user contribution.

Reported by torger@ludd.ltu.se on 2013-10-08 14:59:33

Beep6581 commented 9 years ago

Reported by torger@ludd.ltu.se on 2013-10-08 15:00:09

Beep6581 commented 9 years ago
Re #10: Fine :)
Re #11: +1

Reported by natureh.510 on 2013-10-08 15:14:01

Beep6581 commented 9 years ago
About the lens distortion, it would be better to make that optional, as distorted image
may look more natural or more desirable.

Custom profile builder already allows auto-distortion correction, including linear
interpolation of the distortion amount based on provided key points (focal & amount).
We, however, failed to provide a project repository for it, so may be this can address
that deficiency:)

Reported by michaelezra000 on 2013-10-08 15:30:01

Beep6581 commented 9 years ago
Ok, it was just a sidetrack, we'll see. I'll start with matrix and levels.

Reported by torger@ludd.ltu.se on 2013-10-08 15:32:48

Beep6581 commented 9 years ago
I've started this, hopefully I'll come to the finish line soon, if not I'll announce
it here.

Reported by torger@ludd.ltu.se on 2013-10-08 19:46:38

Beep6581 commented 9 years ago
Please include an option for "White level depending on aperture"

Some models change WP when using all apertures wider than f/2.8 because there is a
digital upscaling to compensate for pixel vignetting ..http://www.dxomark.com/index.php/eng/Publications/DxOMark-Insights/F-stop-blues
... Canons do compensate in camera for this.

Canon 40D, Canon 35mm/f1.4, ISO100. f/1.4=16383 - f/2.0=14333 - f/2.8=13824 - f/4.0=13824
...
But its more complicated .. with a Zenit 55/f2.0 (no electrical connection with the
body) there is no compensation. Not sure what happens with third party lenses with
electrical communication.

Reported by iliasgiarimis on 2013-10-09 07:55:26

Beep6581 commented 9 years ago
Thanks for the information.

The likely answer is that Canon makes this compensation only for lenses the camera
recognizes, and it will thus only care for Canon's own lenses. The third-party don't
try to trick the camera into being a Canon-branded lens as far as I know so I think
it's quite safe to assume that no digital scaling will happen with third-party lenses.

This type of information could surely exist in this "camera constants" file, I'll start
with the basics now and we can add more features further on.

We'd have to know more details about this scaling issue, how many cameras that have
it (may be overkill to support it if it's just a few older(?) Canon models), for what
lenses, if it's uniform over the whole sensor area or is some sort of inverse vignetting
to it etc.

Reported by torger@ludd.ltu.se on 2013-10-09 08:32:22

Beep6581 commented 9 years ago
Okay, here's a big fat patch. I have not cleaned out the small uncommitted bug fixes
patches which also is in this (thumbnail rotation 1996, leaf exif parse data, 1994),
and it also includes dcraw patch cleanup 1995 as they are kind of together.

There patch adds third party cJSON.[c,h] in the rtengine dir, MIT license stable very
lightweight ANSI-C JSON api. I do not expect this file need to change ever, so putting
the source there instead of yet another lib dependency seemed like a good thing.

There's also the camconsts.[cc,h] which parses the new camera constants file at startup
and puts it in a camera constants store which can be called throughout the program.
It's C++ with some C as cJSON is C and I'm a C programmer :-), hope it's okay.

There's the 'camconsts' text file itself in the options dir which is installed alongside
options file. Maybe it should be somewhere else, but that was the best thing I came
up with without creating a new data dir. The 'camconsts' now only contains documentation
and entries for Phase One digital back, one could move out all hard-coded RT-specific
constants, which now is in rawimage.cc (moved from dcraw.c), but it's okay to have
it there too. The priority order is 'camconsts' overrides 'rawimage' which overrides
'dcraw' constants. You can have partial information, ie only add a white level and
let the matrix come from dcraw, but I think it's best to have complete info in camconsts
when one adds an entry.

I've done brief testing and it seems to work. It would be nice to have some real camera
entry with multi-iso black/white levels and files to test with it, which should work
with this patch.

I can split the patch if required, the leaf/thumbnail fixes are pretty isolated safe
simple fixes though.

Reported by torger@ludd.ltu.se on 2013-10-09 19:35:24


Beep6581 commented 9 years ago
The attached patch supports dcraw color matrix and multi-channel white levels/black
levels per iso.

Further features like compact camera lens distortion parameters or digital wide aperture
lens scaling could be added later, but I think we first should test and commit with
these basic features.

Reported by torger@ludd.ltu.se on 2013-10-09 19:39:38

Beep6581 commented 9 years ago
I did not get a chance to compile yet, but read the patch. Thank you for this excellent
work!

Reported by michaelezra000 on 2013-10-10 02:07:46

Beep6581 commented 9 years ago
Here's an updated patch with a small fix: only bring in constants if not loaded from
file, ie if we open a DNG the constants are already there in the file (as specified
by the DNG converter), we don't override that.

The logic could be made more advanced than it is, now it enables constant loading if
dcraw calls adobe_coeff(), otherwise not. Some formats will have black level from file
but matrix from adobe_coeff(), but if so we would just store an entry with only matrix,
so this should be appropriate.

In other words, the fix is there to not override constants loaded from DNGs.

Reported by torger@ludd.ltu.se on 2013-10-10 12:29:55


Beep6581 commented 9 years ago
I tried RT with the patch and it seems to work just fine:)

Reported by michaelezra000 on 2013-10-11 14:21:55

Beep6581 commented 9 years ago
I tried the patch on changeset b6b2c383b921 (RT 4.0.11.68).
It compiles ok and seems to work fine.

Nevertheless, when using tonemapping, I can identify (with GIMP) some very small differences
in the output generated 4.0.11.68 and 4.0.11.68+patch. The patch is probably not involved
as I can also identify differences in 4.0.11.60, 4.0.11.67 and 4.0.11.68!
The differences are hardly noticeable. I have still to investigate to confirm it. Perhaps
it is only a rounding artefact.

@Hombre: WindowsInnoSetup.iss.in should be modified to take into account camconsts
file. I will PM you for some minor modif.

Branch: default
Version: 4.0.11.68+patch
Changeset: 6e7efe041dbf
Compiler: gcc 4.6.1
Processor: generic x86
System: Windows
Bit depth: 32 bits
Gtkmm: V2.22.0
Build type: Release
Build flags:  -m32  -mthreads -mstackrealign -mtune=generic -fopenmp -mwindows -DNDEBUG
-msse2 -mfpmath=sse -O3
Link flags: -m32 -mthreads  -static-libgcc -Wl,--large-address-aware,--verbose -mtune=generic
-mwindows -s -O3
OpenMP support: ON
MMAP support: ON

Reported by gaaned92 on 2013-10-12 14:34:47

Beep6581 commented 9 years ago
The differences when using Tonemapping could be caused by Issue 1879. See comments #15
- #31 of Issue 1879 for details.

Ingo

Reported by heckflosse@i-weyrich.de on 2013-10-13 14:27:29

Beep6581 commented 9 years ago
What's the status of the patch torger? I can do the Installer part if you want (comment
#24), before or after your commit. I guess you modified it since you've already committed
part of it (related to the other issues)?

Reported by natureh.510 on 2013-10-13 17:00:20

Beep6581 commented 9 years ago
The patch should be okay except for the #24 installer stuff, ie the code should be working
as far as I know, I've done some testing myself. (I've committed thumbnail and rtexif
parts which was unrelated to this).

I'm a bit busy now for a few days so it would be fine if you do the Installer etc.
If you want to rename files or move stuff around I'm fine with that too.

Reported by torger@ludd.ltu.se on 2013-10-14 15:16:19

Beep6581 commented 9 years ago
#18 "We'd have to know more details about this scaling issue, how many cameras that
have it (may be overkill to support it if it's just a few older(?) Canon models), for
what lenses, if it's uniform over the whole sensor area or is some sort of inverse
vignetting to it etc."

I just checked some samples from 1Dx and 5DIII and the scaling exists starting from
f/2.8 and increases as we go wider and it's uniform over the whole sensor. 

Reported by iliasgiarimis on 2013-10-15 13:30:49

Beep6581 commented 9 years ago
Great, then we could surely add such functionality it would fit perfectly into this.
I'll wait until we have this base patch committed before adding new features though.

Reported by torger@ludd.ltu.se on 2013-10-15 14:09:34

Beep6581 commented 9 years ago
Hi torger,

I've update your patch:

1. the text file is now named camconst.json, as it seems to be an acceptable extension
(at least for FireFox)

2. It is now located in rtengine, and is copied next to rawtherapee.exe in the installation
folder

3. I've added a preamble: "DO NOT EDIT!" ...this is RT's file.
The user will have to create its own one, identically named, in his settings' folder
(next to options), where he'll add his values (see the preamble in camconst.json)

4. The main "merge" logic starts in camconst.cc, line 305.

5. I've added a debug test in init.cc, line 51, on "DummyMake"/"DummyModel")

I hope that it's okay and will still be okay for #28/29.

I'll cleanup the debug code before committing, when you'll give me the GO! (or the
GO BACK :) )

Reported by natureh.510 on 2013-10-16 01:39:26


Beep6581 commented 9 years ago
Great! I'm fine with all changes, please commit :-)

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

Beep6581 commented 9 years ago
Done. Should we close this issue?

Reported by natureh.510 on 2013-10-16 12:08:21

Beep6581 commented 9 years ago
Yep, setting it to fixed. We can do followups in separate issues to keep it more focused
(eg adding new features like scaling for the canons.)

Reported by torger@ludd.ltu.se on 2013-10-16 12:18:29

Beep6581 commented 9 years ago
(I entered issue 2007 to deal with the raw scaling issue)

Reported by torger@ludd.ltu.se on 2013-10-16 14:20:55