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

Fix pipe order #2905

Merged
merged 3 commits into from
Sep 16, 2019
Merged

Conversation

aurelienpierre
Copy link
Member

@aurelienpierre aurelienpierre commented Sep 3, 2019

Rewrite the default pipe order to be more compliant with signal processing, with no care for working colour spaces (Lab or RGB).

Explanations

Let us split the pixelpipe into 4 subsets :

camera RGB -> [input color profile] -> scene-referred colour pipe -> [camera transform] -> display-referred pipe - > [output colour profile] -> display RGB

The subsets will be detailed below. [input color profile] and [output color profile] are stopping points defined by the colorin and colourout IOPs. [camera transform] is a stopping point defined by basecurve or filmic IOPs, aka transfer functions that aim at raising mid-tones while adding non-linear contrast, similarly to what film emulsions are doing.

Camera RGB

Pictures are formed through this path:

emitted light -> [scene/surfaces/shapes (reflection) ] -> [atmosphere (diffusion)] -> [lens (refraction)] -> [sensor (discrete sampling)] -> RAW file.

Each step adds errors and defects, so we need to revert them in the opposite order:

RAW file -> [mosaiced denoise] -> [demosaic] -> [demosaiced denoise] -> [lens profile / deblur] -> [atmosphere dehaze] -> [surfaces perspective straigthen] -> reconstructed light emission signal.

On this reconstructed light emission, we then apply cropping (clip), pixel moving (liquify, spot, retouch), amplification (exposure), and finally go to [input color profile] where camera RGB is matched to human colour sensitivity through XYZ profiles.

Notice that, before input colour profiles, camera RGB is not to be confused with a colour signal since camera spectral sensitivity and metamerism has nothing to do with human ones. Only filters treating the RGB layers as gradient fields should be there, no operation decoupling luminance and chrominance should ever happen there, because luminance and chrominance are meaningful only from an human perceptual perspective, and the camera -> human mapping has not yet been performed.

Scene-referred colour pipe

Here go the Lab and RGB ops that need linearly encoded colours to perform and can work on pixels values outside of [0; 1] range:

  • denoising: nlmeans
  • colour reconstruction: defringe
  • spectral operations: highpass, lowpass, sharpen, atrous
  • further colour calibration/profiling: colorchecker, lut3d, channelmixer
  • colour adjustement working in RGB (whether they expect linear RGB or not): colorbalance, rgbcurve, rgblevels, basicadj

Display-referred pipe

These are the colour adjustments belonging to the legacy image processing workflow, expecting non-linear encodings raising mid-tones and pixels values clamped between [0; 1], so mostly the Lab colour adjustments:

  • global contrast enhancement: colisa, tonecurve, levels, shadhi,
  • local contrast enhancement: bilat,
  • Lab colour adjustments: colorcorrection, velvia, vibrance, colorzones
  • clipped highlights reconstruction: colorreconstruct, just before going in colorout

Display RGB

Arguably, no module should ever be there, because their colour would be unmanaged (possibly an issue for borders and watermark raster inputs), but I haven't touched at this part.

@agusmba
Copy link

agusmba commented Sep 3, 2019

I love the logic of it.

It would be very nice to also have this explanation in the documentation so that end users can understand what a sensible pipeline looks like.

aurelienpierre added a commit to aurelienpierreeng/ansel that referenced this pull request Sep 3, 2019
aurelienpierre added a commit to aurelienpierreeng/ansel that referenced this pull request Sep 3, 2019
aurelienpierre added a commit to aurelienpierreeng/ansel that referenced this pull request Sep 3, 2019
@jakubfi
Copy link
Contributor

jakubfi commented Sep 3, 2019

EDIT: Test results below can be safely ignored in context of what this change does to how image is rendered. They basically show unintended side-effects only. Leaving it here just for future reference.

I wanted to see how it affects existing edits, which is an obvious question to ask here. I've tested it on 227 test images from my collection by exporting them with both pipes and then using ImageMagick MAE metric (mean absolute error (normalized), average channel error distance) on all channels to check the difference between each pair of exported images (https://imagemagick.org/script/command-line-options.php#metric Note: MAE between full-black and full-white images is 65535). This is by no means a comprehensive test, I just wanted to see what it does for me.

Results:

  • MAE=0 - 160 images
  • MAE<1 - 13 images
  • MAE<100 - 20 images
  • MAE<200 - 20 images
  • MAE<300 - 8 images
  • MAE<400 - 4 images
  • MAE<500 - 2 images

I selectively checked edits on images that show differences and they fall into two categories:

  • most of them (with high MAE) are unedited images with default basecurve enabled (my edits usually have basecurve disabled)
  • some of them (with small MAE) are edits with things like shadows and highlights or local contrast enabled

I'm attaching an archive with sample renders done with both pipes (jpgs due to attachment size). 1 is old, 2 is new. MAE values in res_mae.txt: dt-test-copy.zip

@aurelienpierre
Copy link
Member Author

aurelienpierre commented Sep 3, 2019

Existing edits, for which an history already exist, should see no change with this because their pipe order is already set. This should only affect the default order when opening a new picture (with no history).

Could you please check that the module order is unaffected for pictures having already history ?

Your results are concerning.

@jakubfi
Copy link
Contributor

jakubfi commented Sep 3, 2019

@aurelienpierre retraced my steps and found a mistake I made - I tested master against your toneequalizer branch instead of fix-pipe-order. This shouldn't be a problem anyway (no toneequalizer enabled and fix-pipe-order merged there too), but my bad anyway.

  • All the images I tested already had a sidecar written with a default edit (so at least basecurve enabled).
  • Retested it with a one sample image, this time on fix-pipe-order branch. Exported image looks different.
  • Module order for an image with an xmp changes:
    master:
    master
    fix-pipe-order:
    pipe

@aurelienpierre
Copy link
Member Author

@edgardoh is the module order in the pipe not stored in and fetched from database once defined ?

@jakubfi
Copy link
Contributor

jakubfi commented Sep 3, 2019

@aurelienpierre xmp for the image in question: DSC09027.ARW.xmp.txt
History stored in db:

sqlite> select * from history where imgid=13854;
13854|0|2|flip|����|1||9|0||20.0
13854|1|6|basecurve||1||9|0||23.0

@aurelienpierre
Copy link
Member Author

aurelienpierre commented Sep 3, 2019

So the enabled modules keep their previously defined order, it's the disabled ones that get reordered, right ? But then, why is colorin not in history ?

@aurelienpierre
Copy link
Member Author

aurelienpierre commented Sep 3, 2019

Ok, confirmed. colorin and colorout and possibly other always-on modules are not stored in database or XMP history if users have not changed their default params, so this PR can result in modules flipping around them on image re-opening, compared to the previous state which has not been saved. We need the always-on and on-by-default modules to be included in history at image opening.

@jakubfi
Copy link
Contributor

jakubfi commented Sep 4, 2019

@aurelienpierre okay, so this is basically a bug at this point. I've updated my original comment above to not confuse people reading this PR.

@parafin
Copy link
Member

parafin commented Sep 4, 2019

It's probably a good idea then first to add all enabled modules to the history. I seem to remember it could be useful for history compression also. But upgrade path has to be coded carefully...

@aurelienpierre
Copy link
Member Author

aurelienpierre commented Sep 4, 2019

It's probably a good idea then first to add all enabled modules to the history. I seem to remember it could be useful for history compression also. But upgrade path has to be coded carefully...

I agree to that, although I have no idea on how to do that.

@TurboGit
Copy link
Member

TurboGit commented Sep 5, 2019

So we want to add into the history all active but hidden modules. Right?

Do we want them to appear also into the history GUI?

This is an area where I have never acked dt but I could try....

@parafin
Copy link
Member

parafin commented Sep 5, 2019

I guess yes, history stack should be transparently mapped from DB/XMP to GUI.

TurboGit added a commit to TurboGit/darktable that referenced this pull request Sep 6, 2019
…story.

This ensures that the corresponding module have an entry into the database
and into the XMP. This is needed to have proper handling of the reorder
of the modules.

Also, the corresponding entries are hidden from the history GUI.

Part of darktable-org#2905.
TurboGit added a commit to TurboGit/darktable that referenced this pull request Sep 6, 2019
…story.

This ensures that the corresponding module have an entry into the database
and into the XMP. This is needed to have proper handling of the reorder
of the modules.

Also, the corresponding entries are hidden from the history GUI.

Part of darktable-org#2905.
@TurboGit
Copy link
Member

TurboGit commented Sep 6, 2019

Ok, I have a first proposal for this, see #2918

Can you test this PR to see if it fixes the issue?

I also need a careful review of the code here as this is a quite delicate area.

@TurboGit TurboGit self-assigned this Sep 6, 2019
@TurboGit TurboGit added this to the 2.8 milestone Sep 6, 2019
@TurboGit TurboGit added the bugfix pull request fixing a bug label Sep 6, 2019
TurboGit added a commit to TurboGit/darktable that referenced this pull request Sep 7, 2019
…story.

This ensures that the corresponding module have an entry into the database
and into the XMP. This is needed to have proper handling of the reorder
of the modules.

Also, the corresponding entries are hidden from the history GUI.

Part of darktable-org#2905.
@TurboGit TurboGit self-requested a review September 9, 2019 08:24
aurelienpierre added a commit to aurelienpierreeng/ansel that referenced this pull request Sep 14, 2019
TurboGit added a commit that referenced this pull request Sep 14, 2019
…story.

This ensures that the corresponding module have an entry into the database
and into the XMP. This is needed to have proper handling of the reorder
of the modules.

Part of #2905.
TurboGit added a commit to TurboGit/darktable that referenced this pull request Sep 14, 2019
This ensures that the corresponding module have an entry into the database
and into the XMP. This is needed to have proper handling of the reorder
of the modules.

Part of darktable-org#2905.
@TurboGit
Copy link
Member

@aurelienpierre : is that ready ? if so I'll probably merge soon with my PR to enable all hidden module into the history.

@aurelienpierre
Copy link
Member Author

This part is ready yes.

@TurboGit
Copy link
Member

Ok, so let's merge this! Better sooner than later. Thanks.

@TurboGit TurboGit merged commit 86ed840 into darktable-org:master Sep 16, 2019
aurelienpierre added a commit to aurelienpierreeng/ansel that referenced this pull request Sep 16, 2019
aurelienpierre added a commit to aurelienpierreeng/ansel that referenced this pull request Sep 21, 2019
aurelienpierre added a commit to aurelienpierreeng/ansel that referenced this pull request Sep 21, 2019
sync the module order on PR darktable-org#2905
aurelienpierre added a commit to aurelienpierreeng/ansel that referenced this pull request Sep 21, 2019
sync the module order on PR darktable-org#2905
@aurelienpierre aurelienpierre deleted the fix-pipe-order branch December 12, 2022 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants