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

Put lens correction before exposure in the pixelpipe #1740

Closed

Conversation

Projects
None yet
6 participants
@aurelienpierre
Copy link
Contributor

aurelienpierre commented Oct 4, 2018

Fixes https://redmine.darktable.org/issues/12324 and https://redmine.darktable.org/issues/10974

Having the lens correction module after the exposure one in the pixelpipe results in artificial positive vignetting (assuming vignetting correction is ON) when the black point has been tuned in the exposure module. This PR corrects that.

Put lens correction before exposure in the pixelpipe so the black lev…
…el won't mess up the vignetting correction
@LebedevRI
Copy link
Member

LebedevRI left a comment

You can't do that.
It will shatter existing history stacks.
That is why i haven't touched this.

@aurelienpierre

This comment has been minimized.

Copy link
Contributor Author

aurelienpierre commented Oct 4, 2018

Well, there is something I don't understand here. I have tested it on previously edited pictures, I didn't see anything weird.

@LebedevRI

This comment has been minimized.

Copy link
Member

LebedevRI commented Oct 4, 2018

Enable some module in between the old lens correction and exposure, and compare pfm exports.

@aurelienpierre

This comment has been minimized.

Copy link
Contributor Author

aurelienpierre commented Oct 4, 2018

The current stack is:

202 lens
188 retouch
173 spots
159 exposure
144 tonemap
130 denoiseprofile

The proposed one is:

202 exposure
188 tonemap
173 lens
159 denoiseprofile
144 retouch
130 spots

The retouch module has been merged 3 weeks ago, so I doubt a lot of people will be affected with a lot of edits. And both modules rely on dynamic patches, so their exposure will be updated similarly on the source and target patches.

Nonetheless, I tried editing the same picture with retouch, spots, and tonemap modules and diff the old and new PFM files with Image Magick:
compare -metric RMSE -verbose old.pfm new.pfm -compose src diff.png

The RAW file is a 36 Mpix NEF from a Nikon D810 and the lens is a Nikkor AFS 85mm F1.8G fully corrected (vignetting, CA, distortion), at ISO 180.

With tonemapping enabled:

  Channel distortion: RMSE
    red: 3778.72 (0.0576596)
    green: 3155.78 (0.0481542)
    blue: 2879.02 (0.0439311)
    all: 3292.74 (0.0502441)

With tonemapping disabled:

  Channel distortion: RMSE
    red: 177.957 (0.00271545)
    green: 255.788 (0.00390308)
    blue: 253.788 (0.00387255)
    all: 232.023 (0.00354045)

Looking at the position of the absolute error, the most affected pixels are in blown highlights (hair on this picture), noisy/black areas, and with tonemapping disabled, at the very center of the picture (which might have to do with the lens correction). The patched areas or not affected at all (either with spots or retouch modules):

Diff mask with tonemapping:

diff

Without tonemapping:
diff-2

So, seing the RMSE error magnitude (0.3 %) and distribution without the tonemapping, I think we are mainly dealing with rounding errors here. I don't get the difference between tonemapped/non-tonemapped versions though, but it's still a valid option to force it before the lens correction for retro-compatibility.

@LebedevRI

This comment has been minimized.

Copy link
Member

LebedevRI commented Oct 5, 2018

What about parametric blending in the spot removal module?

@TurboGit TurboGit added the enhancement label Oct 5, 2018

@aurelienpierre

This comment has been minimized.

Copy link
Contributor Author

aurelienpierre commented Oct 5, 2018

Well, it's difficult to test every configuration there, but I blended the spot removal on the input g channel so that some spots are partially masked, some totally masked and some unmasked at all.

The RMSE stats increase a bit and the diff mask shows no difference at all on the spot-corrected areas.

    red: 186.214 (0.00284144)
    green: 267.298 (0.0040787)
    blue: 264.81 (0.00404074)
    all: 242.382 (0.00369852)

diff-3

Again, the error seems to increase only on blown highlights (hair with chromatic aberrations).

I feel like holding this change would penalize the majority of users and pictures for the sake of preserving a retro-compatibility that could be compromised only on some corner-cases.

@LebedevRI

This comment has been minimized.

Copy link
Member

LebedevRI commented Oct 6, 2018

It doesn't really matter if 0.1% of all the processed shots will break or 1000%.
It's aready intentional breakage.

This can't be done by the very reason this is wanted, and is the correct thing
to do - these operations are not commutative. Order matters.

If you have exposure correction with parametric blend on channel's value,
and you change the modules before this module, the parametric blend will
now cover different pixels...

You can even see that without the patch:

  1. reset history stack
  2. enable exposure module
    1. set exposure to +18
    2. set blend to parametric mask
    3. switch to L tab
    4. set input range to 25..100 (i.e. only lower the low bar)
  3. click on "display mask and/or channel" (the icon in the bottom-right corner of the module's blend panel)
  4. Note the blend mask:
    image
  5. In raw black/white levels, lower the white level. That will roughly have the same effect as vignetting correction, just with, well, not so flat shape.
  6. Note the new blend mask:
    image

I think the results are self-explanatory.
I'm not saying this new order isn't correct.
I'm only saying that the existing modules can't be moved around.
It could be done by simply copying existing lens.c to lens2.c, with the correct place in the pipe.
But that is so ugly and intrusive, so it would be great to rewrite the module while there,
getting right the ui this time, and all of the param autodetection.

@LebedevRI LebedevRI added the invalid label Oct 6, 2018

@aurelienpierre

This comment has been minimized.

Copy link
Contributor Author

aurelienpierre commented Oct 6, 2018

I understand what you mean, but I hardly see how it is relevant here. The only modules that could be affected by this change are those between exposure and lens correction, namely the spots and retouch modules. The lens correction doesn't perform any static thresholding (as you show here with the white level, that's an input-independant operation), but only proportionnal local corrections (input-dependant), and at a magnitude of 1.5 EV max. I have shown that the RMSE are neglictible and the differences are not even on the masked areas.

The exposure module performs a static operation: the black level is an offset, independant from the input. That's why it should be put at last. But if you take the block { lens ; spots ; retouch ; tonemap }, no static offset is applied in it (exposure correction -> multiplications -> distributive and commutative), so the modules are mutable inside that block. Besides, the vignetting correction only flattens the exposure on the edges, so whatever blending mask comes after will just have a more even input.

@LebedevRI

This comment has been minimized.

Copy link
Member

LebedevRI commented Oct 6, 2018

The only modules that could be affected by this change are those between exposure and lens correction, namely the spots and retouch modules.

Factually wrong, all the modules between exposure and lens correction,
namely the exposure, spots and retouch modules.
Because you are moving lens correction before exposure. Isn't that the whole purpose?

...

Did you just completely skip everything i wrote about different input => different parametric blend => different mask ==> different output image?

@aurelienpierre

This comment has been minimized.

Copy link
Contributor Author

aurelienpierre commented Oct 6, 2018

Did you just completely skip everything i wrote about different input => different parametric blend => different mask ==> different output image?

I did not. But test it in real life, e.g. not at 18 EV and with the real vignetting correction. The difference it makes is numerically insignificant and visually unnoticeable.

@LebedevRI

This comment has been minimized.

Copy link
Member

LebedevRI commented Oct 6, 2018

I did not. But test it in real life, e.g. not at 18 EV and with the real vignetting correction. The difference it makes is numerically insignificant and visually unnoticeable.

Please explain, if in the real world, this reordering is not noticeable, why do you want to reorder then?

@aurelienpierre

This comment has been minimized.

Copy link
Contributor Author

aurelienpierre commented Oct 6, 2018

Because the black level in the exposure module is messing with the vignetting correction, and the gamma correction in the unbreak input profile module makes it blow, resulting in positive vignetting. The only problem in this stack is the exposure module and its only static setting in this part of the pixelpipe. The lens correction is harmless.

@parafin

This comment has been minimized.

Copy link
Member

parafin commented Oct 6, 2018

So there is visible change? Then it breaks previously processed images. It doesn't matter if the change is for the better.

@aurelienpierre

This comment has been minimized.

Copy link
Contributor Author

aurelienpierre commented Oct 6, 2018

I think you don't understand. But sure, keep it like that for another decade : the more you wait, the easier it is to fix. Have a nice time with you pet software, I'm actually doing photography, and my fork works, so I'm out of here.

@LebedevRI

This comment has been minimized.

Copy link
Member

LebedevRI commented Oct 6, 2018

I think you don't understand.

Which part of "given history stack (XMP) should produce the same image going forward"
are we missing?
In it's current form, this is a breaking change, as i have tried to repeatedly demonstrate.

@bronger

This comment has been minimized.

Copy link
Contributor

bronger commented Oct 6, 2018

Since nobody can assure that the ordering is bug-free (or can never be improved, for that matter), there should be a way to change it without breaking old images. Is it feasible at all to make it flexible at runtime?

@aurelienpierre

This comment has been minimized.

Copy link
Contributor Author

aurelienpierre commented Oct 6, 2018

In it's current form, this is a breaking change, as i have tried to repeatedly demonstrate.

just fucking look at the maths going on in these modules, you will understand that the only immutable part is the black level because it's a static offset. I don't care about XMP order, mathematically, it's the same. Even with masks and parametric blending, it's still safe on 85 % of the picture at least.

@LebedevRI

This comment has been minimized.

Copy link
Member

LebedevRI commented Oct 6, 2018

I have already explained the way forward here:
#1740 (comment)

I'm not saying this new order isn't correct.
I'm only saying that the existing modules can't be moved around.
It could be done by simply copying existing lens.c to lens2.c, with the correct place in the pipe.
But that is so ugly and intrusive, so it would be great to rewrite the module while there,
getting right the ui this time, and all of the param autodetection.

there should be a way to change it without breaking old images.
Is it feasible at all to make it flexible at runtime?

Both of these would require the module priorities (i.e. absolute) order to be stored in the sidecar.
Question that raises:

  1. it is currently not being done, it's not stored in sidecars.
  2. Even if it is implemented, the order will be stored only the next time the image is loaded
  3. If the module order changes before the 'current' module order is stored,
    the factual module order for the image will be wrong.
    So you'd need to hardcode the current module priorities somewhere,
    and store them when the image is first loaded?
  4. Great, all that works.
    Now, a new module is added.
    What happens for the 'old' history stacks, that have old module priorities?
    It is possible that the new module, due to the absolute priorities, will end up
    between some two different modules, as compared to the pristine order.
  5. To be continued?
@parafin

This comment has been minimized.

Copy link
Member

parafin commented Oct 6, 2018

I think the way forward was already explained - create a new module lens2 in the right place while fixing other bugs old lens has.

@parafin

This comment has been minimized.

Copy link
Member

parafin commented Oct 6, 2018

I don't care about XMP order, mathematically, it's the same. Even with masks and parametric blending, it's still safe on 85 % of the picture at least.

I think we have different understanding of what mathematics is. You can't say that it's safe to say 2 * 2 = 4 at least in 85% of cases...

@edgardoh

This comment has been minimized.

Copy link
Contributor

edgardoh commented Oct 8, 2018

It should be possible to define the priorities at run time instead of having it hard-coded, this way, at least for advanced users, the position of a module on the pipe can be changed (at user risk, of course).
It probably also allow to re-use the iop source files. Right now we can't create another instance of a module because it will have the same priority, but if the priority is defined at run time we can just add another instance on a different position.

@LebedevRI LebedevRI added this to the 2.6 milestone Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.