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

Add a built-in lens correction module. #7092

Closed
wants to merge 9 commits into from
Closed

Add a built-in lens correction module. #7092

wants to merge 9 commits into from

Conversation

FreddieWitherden
Copy link
Contributor

This draft PR adds support for a manufacturer lens correction module which can correct distortion, chromatic aberration, and vignetting using the lens correction coefficients stored in the RAW file by the manufacturer. Currently Sony and Fuji APS-C cameras are supported.

Note that in order to test the module it is necessary to start Darktable with --library :memory: and one must also be using the absolute latest Git revision of Exiv 0.27. The library flag is require since this PR must pull some additional data out of the RAW file into dt_image_t. However, dt_image_t appears to be cached in a SQL database which will require changes in order to serialise the new structure.

@johnny-bit
Copy link
Member

There are couple of things you might want to consider:

  • our build system allows module to be conditionally built based on presence of library the module depends on, for example you might want to check out lut3d
  • given that module only works for certain sony & fuji cameras with certain data, you might want to disable/hide completely module based on what image is being processed, for example you can check (i think) rotate pixels and white balance modules on that :)
  • a tiny code nitpick - you don't need to include color-picker-proxy as your module doesn't include color pickers at all :)

@FreddieWitherden
Copy link
Contributor Author

Thank you for your feedback.

our build system allows module to be conditionally built based on presence of library the module depends on, for example you might want to check out lut3d

One nice aspect of this module is that it is dependency free. All correction is performed by mlens.c.

given that module only works for certain sony & fuji cameras with certain data, you might want to disable/hide completely module based on what image is being processed, for example you can check (i think) rotate pixels and white balance modules on that :)

I think I handle this with the check in reload_defaults which if img->exif_correction_type is 0 sets module->hide_enable_button. Let me know if there is a preferred means, however.

a tiny code nitpick - you don't need to include color-picker-proxy as your module doesn't include color pickers at all :)

Fixed.

@johnny-bit
Copy link
Member

One nice aspect of this module is that it is dependency free.

Right, I meant that it's working depends on having exiv2 v 0.27.4 (or dev branch?) but it's not actually IN module, but outside of it... But if i see correctly you already handle that case...

Let me know if there is a preferred means, however.

uh oh... @TurboGit will surely guide that ;)

@johnny-bit
Copy link
Member

2 more things of which i forgot ;)

It would be awesome if you could add unit tests according to https://github.com/darktable-org/darktable/tree/master/src/tests/unittests :)
And it might be worth considering having also integration tests for results once you're certain that the algorithms are stable according to https://github.com/darktable-org/darktable/tree/master/src/tests/integration

@TurboGit
Copy link
Member

I think I handle this with the check in reload_defaults which if img->exif_correction_type is 0 sets module->hide_enable_button. Let me know if there is a preferred means, however.

There is already some modules doing that, for example: rawdenoise or cacorrect which are only RAW pictures. And all this is happening in reload_defaults() indeed and plays with the module->hide_enable_button plus a message on the iop to explain why it is disabled.

@TurboGit TurboGit self-requested a review November 27, 2020 19:29
@FreddieWitherden
Copy link
Contributor Author

The relevant patches have now been merged into the 0.27-maintenance branch of Exiv2 so this should be ready for testing.

@aurelienpierre
Copy link
Member

I have mixed feelings about that:

  1. why not add that to the current lens correction module ? Having 2 modules for the same task is not great.
  2. the interpolation is not really an interpolation, it only computes the coordinates of the nearest neighbour, if I'm not mistaken, and that will not produce good results for any kind of sharp pattern (fences, grids, etc.)

@FreddieWitherden
Copy link
Contributor Author

  1. why not add that to the current lens correction module ? Having 2 modules for the same task is not great.

The existing module has a dependency on an external library (lensfun) whereas this one does not.

  1. the interpolation is not really an interpolation, it only computes the coordinates of the nearest neighbour, if I'm not mistaken, and that will not produce good results for any kind of sharp pattern (fences, grids, etc.)

I'm not entirely sure what you mean here. In the module there are two types of interpolation: model coefficient interpolation which is accomplished using a linear spline (see interpolate), and actual pixel data interpolation which is performed using dt_interpolation_compute_sample.

@aurelienpierre
Copy link
Member

The existing module has a dependency on an external library (lensfun) whereas this one does not.

I understand, but workflow-wise, having 2 different modules to perform the same task is weird. Also, users have asked a way to tune the Lensfun profiles for distortion and vignetting, which this PR does, but separately. It would be nicer to blend this feature in the lens.cc module as an additional path, use macros to enable the Lensfun path if the lib is detected, and otherwise just offer the option developed here. Then add user corrections over the Lensfun profiles too.

I'm not entirely sure what you mean here. In the module there are two types of interpolation: model coefficient interpolation which is accomplished using a linear spline (see interpolate), and actual pixel data interpolation which is performed using dt_interpolation_compute_sample.

Ok that's good then.

@FreddieWitherden
Copy link
Contributor Author

The existing module has a dependency on an external library (lensfun) whereas this one does not.

I understand, but workflow-wise, having 2 different modules to perform the same task is weird. Also, users have asked a way to tune the Lensfun profiles for distortion and vignetting, which this PR does, but separately. It would be nicer to blend this feature in the lens.cc module as an additional path, use macros to enable the Lensfun path if the lib is detected, and otherwise just offer the option developed here. Then add user corrections over the Lensfun profiles too.

Although the functionality is similar the means by which they accomplish it is somewhat different. A good example of this is fine-tuning. It is trivial in my module (which is spline based) as you simply tweak the values towards/away from unity. This is all done as a pre-processing step with no overhead. For lensfun, whose models are monomial coefficient based, this is simply not possible as the model response is not linear to changes in the coefficients. Thus one would need to do corrections on a per-pixel basis, computing the displacement vector and rescaling appropriately. This is not only messy but also expensive as you're piling on additional square root operations. The result is two quite different implementations.

Similarly, for OpenCL things are different. For lensfun you need to compute the distorted coordinates by calling lensfun methods on the CPU in one big array and then pass it into the kernel. This is slow, both due to the computational cost of the operations and the time required for the copy. In contrast, my module can be completely offloaded.

@aurelienpierre
Copy link
Member

aurelienpierre commented Dec 4, 2020

Although the functionality is similar the means by which they accomplish it is somewhat different.

I know that.

Right now, in lens.cc, you have one big process() function. What I mean is, rename that function process_lensfun(), and include the process() of the current PR in there too, renamed as a process_builtin(). Then, replace the lens.cc process() with:

void process(args)
{
  if(method == DT_LENS_BUILT_IN)
    process_builtin(args);
#ifdef LENSFUN
  else if(method == DT_LENS_LENSFUN);
    process_lensfun(args)
#endif
   else
     // some fall-back or error log
}

and add a method parameter taking values into a

typedef enum dt_lens_method_t
{
   DT_LENS_BUILT_IN = 0,
   DT_LENS_LENSFUN = 1
} dt_lens_method_t;

that can be set through a combobox, by the user, in GUI. Of course the DT_LENS_LENSFUN will be disabled in GUI if the program is compiled without Lensfun support.

That way, we can merge the feature in the same module, while still having different code pathes. Add the parameters you need in the parameters structure, and split other functions like distort_transform() or process_cl() with the same logic as process().

@TurboGit
Copy link
Member

I would really like to see Aurélien's proposal being tested/implemented. I seems a good candidate avoid code duplication, BUT... beware that in such module we need all the transforms routines:

  • distort_transform
  • distort_backtransform
  • distort_mask

Those are mandatory to have the masks and other modules to work properly when having the transformation activated.

@TurboGit TurboGit added this to the 3.6 milestone Dec 22, 2020
@TurboGit TurboGit added the feature: enhancement current features to improve label Dec 22, 2020
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.

try to integrate this into current lens.cc
implements the transform routines

@FreddieWitherden
Copy link
Contributor Author

I believe that the two distort transform routines are already implemented, and I can look into implementing the mask function. However, my big issue is around the changes to dt_image_t, which seem to have a negative interaction with the SQL based caching mechanisms in Darktable. This is code I am unfamiliar with and I am hesitant to make any changes to the DB side of things.

@TurboGit
Copy link
Member

I believe that the two distort transform routines are already implemented

I see, sorry I missed that.

@TurboGit
Copy link
Member

However, my big issue is around the changes to dt_image_t, which seem to have a negative interaction with the SQL based caching mechanisms in Darktable.

Not sure to see your problem. But if we can help when you'll have detailed report about the issue when implemented.

@aurelienpierre
Copy link
Member

@FreddieWitherden don't hesitate to ask if you need help or explanations about dt's guts.

@TurboGit
Copy link
Member

@FreddieWitherden : Just checking if this PR is still on your radar and if you are ok to integrate that into the current lens module. If you need help please let us know. TIA.

@FreddieWitherden
Copy link
Contributor Author

It is still on my radar, but I do not think I'll have time to revisit it until the summer I'm afraid.

@aurelienpierre
Copy link
Member

Otherwise, we can merge with current module if you want. The feature is cool, I would like it ASAP in master. Unless you want to do it yourself, which would be understandable.

@FreddieWitherden
Copy link
Contributor Author

FreddieWitherden commented Jan 16, 2021

Otherwise, we can merge with current module if you want. The feature is cool, I would like it ASAP in master. Unless you want to do it yourself, which would be understandable.

No problem at all, please go ahead.

@github-actions
Copy link

This pull request did not get any activity in the past 30 days and will be closed in 365 days if no update occurs. Please verify it has no conflicts with the master branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work.

@janfri
Copy link
Contributor

janfri commented May 25, 2021

Otherwise, we can merge with current module if you want. The feature is cool, I would like it ASAP in master. Unless you want to do it yourself, which would be understandable.

No problem at all, please go ahead.

Is there any progress? If I understand @FreddieWitherden correct, he means the current implementation could be merged, as @aurelienpierre suggested.

Is there any chance to get this feature into darktable 3.6?

@FreddieWitherden
Copy link
Contributor Author

So I had a look at wiring up OpenCL support, although it seems like in the current approach there is quite a bit of code duplication depending on what interpolation scheme is being used. Ideally, these would be factored out. Everything else, however, including the spline evaluation seems relatively straightforward, though.

@sgotti
Copy link
Contributor

sgotti commented Oct 17, 2022

I'm using this module for some time (with Fuji cameras and Fuji and Tamron lenses) and it works really well! Great work @FreddieWitherden!

The latest version that reads exif data on file open without updating the DB fixed different issues.
(Probably the PR needs some cleanup since there're some unwanted changes with submodules).

Additionally the module logic/code has been also used inside ART by @agriggio and it's used by default when the lens correction metadata is available.

@FreddieWitherden
Copy link
Contributor Author

The latest version that reads exif data on file open without updating the DB fixed different issues. (Probably the PR needs some cleanup since there're some unwanted changes with submodules).

Yes, I think that the module is in relatively good shape. It would be nice if it could get merged (in one form or another) since some of the lenses Sony and Fuji are bringing out have a lot of distortion.

@TurboGit
Copy link
Member

The module maybe, but the PR is not really in good shape. You need to rebase it and avoid all modifications on submodules (LibRAW, OpenCL, integration...).

A second module for lens distortion correction is not really clean. But I may understand that merging it in the current lens correction module may be difficult. That being said having a built-in mode in the current lens correction module will be better I think. We could for example add a mode "auto" (or build-in) in the current module when data are available from EXIF.

This has already been discussed BTW and I still think it would be the way to go.

@TurboGit TurboGit added difficulty: average some changes across different parts of the code base documentation-pending a documentation work is required release notes: pending and removed no-pr-activity labels Oct 21, 2022
@TurboGit
Copy link
Member

@FreddieWitherden : Let me know if you plan on working on the redesign of the module here for integration into the current lens correction module. Otherwise we could just close, let me know. TIA.

@FreddieWitherden
Copy link
Contributor Author

@FreddieWitherden : Let me know if you plan on working on the redesign of the module here for integration into the current lens correction module. Otherwise we could just close, let me know. TIA.

I don't think I'll have the time to look into this in the near future I'm afraid.

@sgotti
Copy link
Contributor

sgotti commented Oct 26, 2022

@TurboGit I'm finishing a PR that will merge @FreddieWitherden work inside the lens module.

I'm splitting it in multiple commits since the current lens module needs to build also without lensfun and requires a lot of refactoring to decouple both saved parameters (with issues if these values changes in new lensfun versions) and internal data from lensfun type and values. It's not just a bunch of ifdefs and I understand why it was done as a separate module...

@FreddieWitherden I'll mark the commits containing your changes as authored by you if it's ok for you.

@TurboGit
Copy link
Member

@TurboGit I'm finishing a PR that will merge @FreddieWitherden work inside the lens module.

That's a great news to see this project being taken, thanks!

@paolodepetrillo
Copy link
Contributor

I had started working on adding support for Olympus (micro four thirds) to this PR, should be able to add it to @sgotti version.

I was able to figure out distortion and CA by comparing the exif tags in the raw file to the output of Adobe DNG Converter. Distortion and CA are encoded as a polynomial a + br^2 + cr^4 + d*r^6, same as the WarpRectilinear DNG opcode, and can easily be converted to splines like Sony and Fuji use.

Not sure how vignetting works - these cameras have an option 'shading compensation' which applies the vignetting to the actual raw image contents, so I don't know if the same information is also included in a tag that could be used if the option wasn't enabled when the photo was taken.

sgotti added a commit to sgotti/darktable that referenced this pull request Oct 27, 2022
Add embedded metadata lens correction.
* Implements @FreddieWitherden correction alghoritms developed by
  Freddie Witherden in pull request
  darktable-org#7092
* Add a method combobox to select the correction method to use.
* Defaults to the "embedded metadata" method if correction metadata is available

Co-authored-by: Freddie Witherden <freddie@witherden.org>
sgotti added a commit to sgotti/darktable that referenced this pull request Oct 27, 2022
Add embedded metadata lens correction.
* Implements @FreddieWitherden correction alghoritms developed by
  Freddie Witherden in pull request
  darktable-org#7092
* Add a method combobox to select the correction method to use.
* Defaults to the "embedded metadata" method if correction metadata is available

Co-authored-by: Freddie Witherden <freddie@witherden.org>
sgotti added a commit to sgotti/darktable that referenced this pull request Oct 27, 2022
Add embedded metadata lens correction.
* Implements @FreddieWitherden correction alghoritms developed by
  Freddie Witherden in pull request
  darktable-org#7092
* Add a method combobox to select the correction method to use.
* Defaults to the "embedded metadata" method if correction metadata is available

Co-authored-by: Freddie Witherden <freddie@witherden.org>
@sgotti
Copy link
Contributor

sgotti commented Oct 27, 2022

Created PR #12714

@sgotti
Copy link
Contributor

sgotti commented Oct 27, 2022

I had started working on adding support for Olympus (micro four thirds) to this PR, should be able to add it to @sgotti version.

I was able to figure out distortion and CA by comparing the exif tags in the raw file to the output of Adobe DNG Converter. Distortion and CA are encoded as a polynomial a + b_r^2 + c_r^4 + d*r^6, same as the WarpRectilinear DNG opcode, and can easily be converted to splines like Sony and Fuji use.

Not sure how vignetting works - these cameras have an option 'shading compensation' which applies the vignetting to the actual raw image contents, so I don't know if the same information is also included in a tag that could be used if the option wasn't enabled when the photo was taken.

@paolodepetrillo Great! Noticed that ART (that uses the @FreddieWitherden algorithm) added corrections based on DNG files metadata. Also this could be also added and perhaps that code can be of help also to you for olympus lens corrections.

@elstoc elstoc removed the documentation-pending a documentation work is required label Nov 8, 2022
@greenoid
Copy link

I need help with guessing the distortion formula of the embedded distortion correction values embedded in newer Panasonic Lumix S (full-frame) RW2 raw files. None of the correction formulars of lensfun does the job. Distortion correction parameters are embedded as an array of 16 integer values in the EXIF oem extension, position is know, if have example photos, don't know how the mathematics of anti distortion works.

@roth-m
Copy link

roth-m commented Feb 24, 2024

I need help with guessing the distortion formula of the embedded distortion correction values embedded in newer Panasonic Lumix S (full-frame) RW2 raw files. None of the correction formulars of lensfun does the job. Distortion correction parameters are embedded as an array of 16 integer values in the EXIF oem extension, position is know, if have example photos, don't know how the mathematics of anti distortion works.

I workon MFT Panasonic Lumix raws.
I have matched 2 WarpRectilinear coefficients out of the 4 in Tag 0x0119. I'm also investigating Tag 0x011B which contains much more coefficients but I could not correlate one of them with any DNG OpCodeList WarpRectilinear one.
I can provide you with tools to dump coefficients of these 2 EXIF Tags and coefficients from DNG OpCodeList3.
I'm currently using R on data with focal + Tags dumps + DNG dumps.

@kmilos
Copy link
Contributor

kmilos commented Jun 20, 2024

I have matched 2 WarpRectilinear coefficients out of the 4 in Tag 0x0119.

As RT has borrowed from dt, you could claim something back now? 😉 @roth-m

See also:

https://syscall.eu/archive.html
https://www.andrewj.com//mft/algorithms.php

@roth-m
Copy link

roth-m commented Jun 20, 2024

I have matched 2 WarpRectilinear coefficients out of the 4 in Tag 0x0119.

As RT has borrowed from dt, you could claim something back now? 😉 @roth-m

See also:

https://syscall.eu/archive.html https://www.andrewj.com//mft/algorithms.php

Unfortunately the references you mention provide approximative formulas (sometimes it is really bad) for Barrel distorsion.

DNG is based on Brown Conrady distorsion and I believe the data in Pana Exif have to be reversed.
Indeed, the formulas in https://www.mdpi.com/1424-8220/16/6/807 work nearly perfectly for 1 channel (as you know CA is applied on DNGs coefficients).

I did a dump of DNG coefficients for 10000 pictures (same principle as it has been done for Olympus) and successfully fitted polynoms (order 3) on all coefficients using 3 to 5 coefficients in Exif. But not sure it would fit for all lenses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: average some changes across different parts of the code base feature: enhancement current features to improve
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet