Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revamp D65 chroma correction workflow #15461

Closed

Conversation

jenshannoschwalm
Copy link
Collaborator

@jenshannoschwalm jenshannoschwalm commented Oct 21, 2023

With the modern scene-referred workflows we do chroma correction in three parts:

  1. We use the 'camera reference' data found in temperature module assuming they are D65
  2. translate what we have to working profile in 'input color profile'
  3. Do a "refinement step" in color calibration assuming we had D65 in step 1

This leads to improved color data in follwing modules - thus it was introduced.

Unfortunately there are some downsides from this approach.
Some modules want accurate white balance data for best quality, most notably
highlights and raw ca correction.
Both modules modify data found in the rgb channels towards "best guessed white" but can't
do so as those data are currently not available.

This PR implements a
chroma struct in dev (like a proxy) holding all required and available white balance data for
the current image (D65, as_shot and currently used) allowing runtime
a) check - are we using D65?
b) access of modules to the other coeffs for improved results
c) easier and safe checks for chroma correction related trouble messages

EDIT: (25.10) temperature modules gets a button introducing this mode and colorin does the "late correction".

Fixes #14518

Related to #15121 and discussions about highlights & opposed

@wpferguson
Copy link
Member

Some modules want accurate white balance data for best quality, most notably
highlights and raw ca correction.

And denoise profile.

@rawfiner thoughts?

@jenshannoschwalm
Copy link
Collaborator Author

And denoise profile.

@rawfiner thoughts?

Exactly :-) This would be a prerequisite ...

@jenshannoschwalm jenshannoschwalm force-pushed the temp_and_chroma branch 3 times, most recently from ec4684d to 27cbe27 Compare October 23, 2023 04:51
@rawfiner
Copy link
Collaborator

@wpferguson indeed, denoise profile in Y0U0V0 mode is working better with an accurate white balance

@wpferguson
Copy link
Member

Just tested. Great improvement on denoise profiled :-D. I have a directory of Play Raws and I('ve gotten 2 crashes trying to import it, so there's something going on. I'll see if I can track it down.

@jenshannoschwalm
Copy link
Collaborator Author

Leave this for a day or two. Will need more care.

@wpferguson
Copy link
Member

It appears to be choking on RAF files. Is there something different about Fuji white balance?

@wpferguson
Copy link
Member

@jenshannoschwalm, not your bug. See #15477

@jenshannoschwalm
Copy link
Collaborator Author

@jenshannoschwalm, not your bug. See #15477

Here is constant work on this so force pushing what is stable working yet.

Next will be proposal for white balance improved.

So far working good with better trouble messages and infrastructure only...

@jenshannoschwalm jenshannoschwalm force-pushed the temp_and_chroma branch 2 times, most recently from 7fd0433 to ef3eb85 Compare October 25, 2023 16:13
@jenshannoschwalm jenshannoschwalm marked this pull request as ready for review October 25, 2023 16:15
@jenshannoschwalm
Copy link
Collaborator Author

Ok, from my side it seems to be working fine all over, would love

  1. testing this new workflow @rawfiner maybe
  2. feedback on design flaws - (anything just plain wrong?)

There are now 4 commits

1&2 introduce the new chroma struct, some refactoring and make sure the module trouble message work fine. So something i suggest to get reviewed @TurboGit

3&4 introduce the simplistic user interface, temperature module version required bumping (required both for user interface and for cache safety without playing with iop cache data). Implementation in colorin needs more love for sure, will follow soon if tests go fine.

@jenshannoschwalm jenshannoschwalm added feature: enhancement current features to improve feature: new new features to add scope: image processing correcting pixels scope: color management ensuring consistency of colour adaptation through display/output profiles release notes: pending labels Oct 25, 2023
@jenshannoschwalm jenshannoschwalm changed the title Revamp chroma correction workflow (I) Revamp D65 chroma correction workflow Oct 25, 2023
@wpferguson
Copy link
Member

Reading the log messages I found

All chroma related warnings will be served via the colorbalancergb module instances.

Did you mean color calibration module instances?

I'm testing now. Do you have any particular test cases you want me to try?

@jenshannoschwalm
Copy link
Collaborator Author

jenshannoschwalm commented Oct 25, 2023

Did you mean color calibration module instances?

YES FOR SURE! channelmixerrgb or color calibration, just fixed the commit message :-) THANKS!

@jenshannoschwalm
Copy link
Collaborator Author

Do you have any particular test cases you want me to try?

Nope. Whatever and However your workflow is. Modules resulting in a visible difference will mostly be those mentioned above.

@TurboGit
Copy link
Member

YES FOR SURE! channelmixerrgb or color calibration, just fixed the commit message :-) THANKS!

I was commenting about this too...

Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review on the code.

src/develop/develop.h Show resolved Hide resolved
src/iop/temperature.c Outdated Show resolved Hide resolved
@@ -1622,6 +1661,7 @@ void reload_defaults(dt_iop_module_t *module)
dt_bauhaus_combobox_add(g->presets, C_("white balance", "user modified"));
// old "camera neutral", reason: better matches intent
dt_bauhaus_combobox_add(g->presets, C_("white balance", "camera reference"));
dt_bauhaus_combobox_add(g->presets, C_("white balance", "late camera reference"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need more comment about this as I'm not sure what it represents exactly and will have trouble during translation to French. Maybe a better wording for this new option is possible?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tooltip added below is:
set white balance to as shot and later correct to camera reference point
But then what is the difference between camera reference and late camera reference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree and the wording is just "preliminary". I am not even sure we want the extra button at all. From my understanding - and if all reviewers and testers agree on this - i would

  1. keep old behaviour for current image developments
  2. use new algo by selecting the reset to default
  3. use new algo for new edits.

But all are open for discussion.

src/iop/temperature.c Outdated Show resolved Hide resolved
src/iop/temperature.c Outdated Show resolved Hide resolved
@jenshannoschwalm jenshannoschwalm force-pushed the temp_and_chroma branch 2 times, most recently from 45eca0d to 5e77ade Compare October 25, 2023 17:53
@wpferguson
Copy link
Member

Tested highlight reconstruction, raw ca and denoise profiled.

Highllight reconstruction worked without issue in all modes. It seemed to me that I might have got a little more detail in the recovered highlights, but it could be just my imagination.

raw ca worked well. I tested against the roof image in the demosiacing improvement issue, as well as a couple of others. raw ca either cleaned up all of the ca or most of it. For the most of it instances applying the ca module finished it up.

denoise profiled was night and day for me. I shoot lots of high ISO images (sports, bad lights, high shutter speeds). denoise profiled in master leaves white "noise" artifacts in the dark areas and doesn't always do a really good job of denoising the lighter areas, because it doesn't have access to the white balance coefficients. So after denoising, I apply an instance of D or S with fine denoise to get rid of the artifacts, and then maybe a second instance (either denoise medium or denoise coarse) to smooth out some of the luma noise. This process yields a fairly good result and keeps the room warm :). With this PR the dark areas have no white artifacts, and the lighter areas have some luma noise on really high ISO images. One instance of D or S cleans up the luma noise if I need to. The resulting image is noticeably better than what I can currently achieve from master.

@TurboGit
Copy link
Member

denoise profiled was night and day for me. I shoot lots of high ISO images (sports, bad lights, high shutter speeds).

I tested on some high iso pictures and I hardly see any difference. How did you test?

@jenshannoschwalm
Copy link
Collaborator Author

From theory in highlights the blown out parts should be corrected more to white instead of yellowish. For raw ca correction the New stuff seems almost always better.

@jenshannoschwalm
Copy link
Collaborator Author

One more experience, also the LMMSE algo works slightly better.

@todd-prior
Copy link

profiled in master leaves white "noise" artifacts in the dark area

While unrelated to this PR wrt your comment...the default slider for preserve shadows I have found leads to a lot of that... simply dropping that back in a measured way really helps.... likely you have explored this but if not then you could check...

@wpferguson
Copy link
Member

I tested on some high iso pictures and I hardly see any difference. How did you test?

It appears that I tested apples against oranges. I used a high ISO image that I had previously developed and compared the noise reduction of that to the PR. I failed to take into account the other steps that I used developing the image in the first place which had an effect on the output. Once I developed them the same and the only difference was the noise reduction, the difference was much closer.

I am glad you asked the question and made me go back and look my method because in that testing I found a nasty little bug unrelated to this PR. Issue coming soon.

Since introducing the modern chroma correction workflow we do the chroma correction in two phases.

First   we use the daylight/D65 coeffs in temperature
Second  we refine for better data in colorbalancergb.

Unfortunately
1) some modules would prefer better white-balanced data, notably highlights, raw chromatic aberration
   as their algorithms assume white-is-white / resp all rgb signals are equal for greytones.
2) due to the implementation in temperature we don't have all coeffs (currently used, D65 and as-shot)
   publicly available but only internal in gui data.
3) warnings are difficult to handle

This pr implements and uses a struct `dt_dev_chroma_t` in `dt_develop_t` holding all relevant data.

The temperature module reads them all and we can check at all stages of the pipe via dt_dev_check_d65(),
also we avoid keeping coeffs in temperature gui data.

Some refactoring and renaming for readability
All chroma related warnings will be served via the channelmixerrgb (color calibration) module instances.

The one being `chroma.adaptation` also knows about the temperature module status,
so it writes the trouble status there too.

For increased responsivness and situations without UI updates (initial loading in darkroom)
we update the troubles after the preview pipe has finished.
We now have a second bulb button on the very right introducing the "late D65" mode.
The effect will be:
1. the temperature module will process data with "as-shot" coeffs and keeps track
   on this in the dev chroma struct.
2. also the processed_max and dsc data are using this
3. See colorin module how this get's resolved

Otherwise the algos have not changed. Presets are supported as before
Preliminary code for evaluation of the algo
1. only cpu code so far
2. some performance penalty
@Donatzsky
Copy link

How does this interact with custom WB coefficients?
I use custom WB coefficients as explained by AP in this video, to correct the yellow cast of my camera. Will this invalidate that in some way?

I'm on stable, so can't test it, but I can share a raw and my coefficients.

@jenshannoschwalm
Copy link
Collaborator Author

Will this invalidate that in some way?

Short answer is no, i suppose you mean having presets for temperature.

This pr changes the pixelpipe color maths only for the new "D65-late" button. All modules until colorin will use the as-shot coeffs (those are often - not always - better than the known D65 coeffs) and are finally fixed to D65 in the input stage of colorin. So no interference for other workflows.

@Donatzsky
Copy link

Short answer is no, i suppose you mean having presets for temperature.

Not sure we are talking about the same thing, although I do use a preset. What I've done is create custom D65 coefficients by taking a photo of a calibrated white screen, and used that to get what you see in the attached screenshot. Without, my photos have a pronounced yellow tone, but with these coefficients color calibration CAT is pretty much spot-on.

Screenshot_2023-10-26_20-48-37

This pr changes the pixelpipe color maths only for the new "D65-late" button. All modules until colorin will use the as-shot coeffs (those are often - not always - better than the known D65 coeffs) and are finally fixed to D65 in the input stage of colorin. So no interference for other workflows.

So, if I understand correctly, there will be a new "D65-late" option (in addition to the current four) in white balance that should usually work better than "camera reference". And if the user chooses any of the others, then everything works as now.

@jenshannoschwalm
Copy link
Collaborator Author

#15505 has replaced this so closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: enhancement current features to improve feature: new new features to add scope: color management ensuring consistency of colour adaptation through display/output profiles scope: image processing correcting pixels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some minor refresh issue on color calibration module
6 participants