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

Highlights reconstruction : introduce new method #10711

Conversation

aurelienpierre
Copy link
Member

@aurelienpierre aurelienpierre commented Dec 28, 2021

We merge the spirit of guided filters, wavelets multiscale processing and isotropic diffusion to introduce a new highlights reconstruction method that guides the laplacian of the R, G, B channels with the laplacian of the norm. This goes in the usual RAW highlights module.

What it does:

  • restore the expected luminance of clipped regions, compared to their surround (otherwise clipped regions are recorded darker than they should),
  • smoothen some sharp artifacts,
  • get some color back,
  • copy-paste details from the norm to the clipped channels.

What it does not:

  • inpaint color (too difficult to identify legitimate objects to sample color from and avoid color bleeding across "edges" that may not be there anymore),
  • deal with chromatic aberrations,
  • Xtrans sensors.

Where it fails:

  • chromatic aberrations and other sharp color issues are not properly corrected.

Notes:

It works better in conjunction with filmic reconstruction, which uses a similar method but with different priorities (smoothness first).

Limitations:

  • No XTrans, and I don't think the method can be adapted (how to do meaningful laplacians in a non-regular CFA pattern ?)
  • No OpenCL.

Demo : https://discuss.pixls.us/t/guiding-laplacians-to-restore-clipped-highlights/28493

@aurelienpierre aurelienpierre marked this pull request as draft December 28, 2021 00:18
@aurelienpierre aurelienpierre added feature: enhancement current features to improve scope: image processing correcting pixels labels Dec 28, 2021
@aurelienpierre aurelienpierre added this to the 4.0 milestone Dec 28, 2021
@elstoc elstoc added the documentation-pending a documentation work is required label Dec 28, 2021
@rawfiner
Copy link
Collaborator

This image shared by @TurboGit on #10716 (which was problematic for @jenshannoschwalm ) seems also problematic here:

https://drive.google.com/file/d/1XFKbgAkbLfckY-JZKrn4mP9MsxLTc59c/view?usp=sharing

@aurelienpierre
Copy link
Member Author

This image has a too large clipped region, it's beyond reconstructibility. Also, it falls under "poor photographying skills" since clipped highlights should only be primary light sources (where you can't avoid it), not 25% of the image.

@rawfiner
Copy link
Collaborator

I still think the algorithm should fallback nicely in such case, inpainting everything in grey for instance. For such case where big clouds are clipped, we don't really need details, just to avoid a too strong color cast.

Also, I found a strange behavior on this image:
https://drive.google.com/file/d/1yMHMH9N_xc-GBSCbTq7m4qyRL2YirFJf/view?usp=sharing

Capture d’écran du 2021-12-28 20-25-53

@aurelienpierre
Copy link
Member Author

I still think the algorithm should fallback nicely in such case, inpainting everything in grey for instance.

It's not possible. "Grey" is possible only after a full chromatic adaptation by enforcing R = G = B. We are here before demosaicing, we have no idea if the illuminant is right or not, we know no color at that point, only sparse gradients. Degrading to grey is the responsibility of filmic reconstruction that happens after proper color profiling and adaptation, meaning it has a sane and sanitized color framework to make assumptions upon colors.

Also, I found a strange behavior on this image:

Latest commit should deal with that. Thanks for your tests !

@rawfiner
Copy link
Collaborator

Latest commit should deal with that.
Indeed!

Another small issue, when zooming into the image in the darkroom, image is surrounded with a row and column of red or blue pixels
Capture d’écran du 2021-12-29 11-46-52

Also, with new commits, the results on the puffin image are worse than before, the orange parts become green whatever the feathering values.

@aurelienpierre
Copy link
Member Author

aurelienpierre commented Dec 29, 2021

Another small issue, when zooming into the image in the darkroom, image is surrounded with a row and column of red or blue pixels

It's normal, I don't demosaic the first/last rows and columns yet.

Also, with new commits, the results on the puffin image are worse than before, the orange parts become green whatever the feathering values.

What is the last commit that works here ?

@rawfiner
Copy link
Collaborator

6cc7c8a (remove WB corrections) is the first bad commit

@TurboGit
Copy link
Member

As we are at it, why the highlight recovery module default to "clip"? Can't we use a safe default like "reconstruct in LCh"?

@Thanatomanic
Copy link

Hi @aurelienpierre I liked to try this out, but I get compilation errors on Windows MSYS2:

[696/889] Building C object lib/darktable/plugins/CMakeFiles/highlights.dir/introspection_highlights.c.obj
FAILED: lib/darktable/plugins/CMakeFiles/highlights.dir/introspection_highlights.c.obj
C:\msys64\mingw64\bin\cc.exe -DAVIF_DLL -DGDK_DISABLE_DEPRECATED -DGDK_VERSION_MIN_REQUIRED=GDK_VERSION_3_22 -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_MIN_REQUIRED -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_56 -DGTK_DISABLE_DEPRECATED -DGTK_DISABLE_SINGLE_INCLUDES -DHAVE_BUILTIN_CPU_SUPPORTS -DHAVE_CONFIG_H -DHAVE_GAME -DHAVE_GMIC -DHAVE_GPHOTO2 -DHAVE_GPHOTO_25_OR_NEWER -DHAVE_GRAPHICSMAGICK -DHAVE_HTTP_SERVER -DHAVE_ICU -DHAVE_ISO_CODES -DHAVE_LENSFUN -DHAVE_LIBAVIF=1 -DHAVE_LIBEXIV2_WITH_ISOBMFF=1 -DHAVE_LIBHEIF=1 -DHAVE_LIBRAW=1 -DHAVE_LIBSECRET -DHAVE_MAP -DHAVE_OPENCL -DHAVE_OPENEXR -DHAVE_OPENJPEG -DHAVE_OSMGPSMAP_110_OR_NEWER -DHAVE_SQLITE_324_OR_NEWER -DNATIVE_ARCH -DSQLITE_CORE -DSQLITE_ENABLE_ICU -DUSE_LUA -D_POSIX_THREAD_SAFE_FUNCTIONS -D_USE_MATH_DEFINES -D__GDK_KEYSYMS_COMPAT_H__ -D__USE_MINGW_ANSI_STDIO=1 -Dhighlights_EXPORTS -IC:/msys64/home/Roel/darktable/build-hr/lib/darktable/plugins -IC:/msys64/home/Roel/darktable/src/iop -IC:/msys64/home/Roel/darktable/src -IC:/msys64/home/Roel/darktable/src/external/LuaAutoC -IC:/msys64/home/Roel/darktable/build-hr/bin -isystem C:/msys64/home/Roel/darktable/src/external -isystem C:/msys64/home/Roel/darktable/src/external/OpenCL -isystem C:/msys64/mingw64/include/glib-2.0 -isystem C:/msys64/mingw64/lib/glib-2.0/include -isystem C:/msys64/mingw64/include/gtk-3.0 -isystem C:/msys64/mingw64/include/pango-1.0 -isystem C:/msys64/mingw64/include/harfbuzz -isystem C:/msys64/mingw64/include/freetype2 -isystem C:/msys64/mingw64/include/libpng16 -isystem C:/msys64/mingw64/include/fribidi -isystem C:/msys64/mingw64/include/cairo -isystem C:/msys64/mingw64/include/lzo -isystem C:/msys64/mingw64/include/pixman-1 -isystem C:/msys64/mingw64/include/gdk-pixbuf-2.0 -isystem C:/msys64/mingw64/include/atk-1.0 -isystem C:/msys64/mingw64/include/libxml2 -isystem C:/msys64/mingw64/include/libsoup-2.4 -isystem C:/msys64/mingw64/include/OpenEXR -isystem C:/msys64/mingw64/include/lensfun -isystem C:/msys64/mingw64/include/librsvg-2.0 -isystem C:/msys64/mingw64/include/json-glib-1.0 -isystem C:/msys64/mingw64/include/openjpeg-2.4 -isystem C:/msys64/mingw64/include/libsecret-1 -isystem C:/msys64/mingw64/include/GraphicsMagick -isystem C:/msys64/mingw64/include/osmgpsmap-1.0 -Wall -Wno-format -Wshadow -Wtype-limits -Wvla -Wold-style-declaration -Wno-unknown-pragmas -Wno-error=varargs -Wno-format-truncation -Wno-error=address-of-packed-member -std=c99 -fopenmp -march=native -msse2 -g -mfpmath=sse -O3 -DNDEBUG -O3 -ffast-math -fno-finite-math-only -fexpensive-optimizations -fvisibility=hidden   -DUNICODE -D_UNICODE -Werror -Wfatal-errors -include common/module_api.h -include iop/iop_api.h -MD -MT lib/darktable/plugins/CMakeFiles/highlights.dir/introspection_highlights.c.obj -MF lib\darktable\plugins\CMakeFiles\highlights.dir\introspection_highlights.c.obj.d -o lib/darktable/plugins/CMakeFiles/highlights.dir/introspection_highlights.c.obj -c C:/msys64/home/Roel/darktable/build-hr/lib/darktable/plugins/introspection_highlights.c
In file included from C:/msys64/home/Roel/darktable/build-hr/lib/darktable/plugins/introspection_highlights.c:60:
C:/msys64/home/Roel/darktable/src/iop/highlights.c: In function 'heat_PDE_diffusion':
C:/msys64/home/Roel/darktable/src/iop/highlights.c:1256:31: error: 'B_SPLINE_TO_LAPLACIAN_2' undeclared (first use in this function)
 1256 |           covariance_HF[c] += B_SPLINE_TO_LAPLACIAN_2 * (neighbour_pixel_HF[k][c] - means_HF[c]) * (neighbour_pixel_HF[k][guiding_channel] - means_HF[guiding_channel]) / 9.f;
      |                               ^~~~~~~~~~~~~~~~~~~~~~~
compilation terminated due to -Wfatal-errors.

Any idea for a fix? I'm building from an empty directory, as is normal.

@aurelienpierre
Copy link
Member Author

As we are at it, why the highlight recovery module default to "clip"? Can't we use a safe default like "reconstruct in LCh"?

It's really not safe.

Hi @aurelienpierre I liked to try this out, but I get compilation errors on Windows MSYS2:

Fixed by 99f9958, I forgot to commit one file where that constant is defined.

@TurboGit
Copy link
Member

It's really not safe.

Can be worst than "clip" :)

@aurelienpierre
Copy link
Member Author

Can be worst than "clip" :)

Clip is the only white-balance-agnostic method. All the others rely on the quality of the WB… which is not guaranteed.

@aurelienpierre aurelienpierre force-pushed the highlights-reconstruction-guided-laplacian branch from 99f9958 to b21cc1f Compare December 30, 2021 23:09
@jenshannoschwalm
Copy link
Collaborator

@aurelienpierre don't know if #10716 has a chance to get into dt codebase too but if we bumb the module version it might be good to leave room in modules parameters for lets say two ints and floats each. Wouldn't hurt but would make testing for other folks much easier.

Or maybe do a seperate pr just handling the module version to prepare for such a scenario?

@aurelienpierre
Copy link
Member Author

@jenshannoschwalm actually @rawfiner raised a good point : why are we taking the trouble of recovering HL before demosaicing ? We need as many branches as CFA patterns and a test demosaicing that will be ditched.

So I wonder if this wouldn't be better in a new module after demosaicing.

@jenshannoschwalm
Copy link
Collaborator

For me this seems to be good too. Would also

  • allow non-bayer sensors
  • defer the green channel handling completely to the demosaicer ...

Would offer to implement the modules framework ...

@aurelienpierre
Copy link
Member Author

aurelienpierre commented Dec 31, 2021

Great, I'll take it. If you reserve me 3 floats in addition of the clipping threshold in the params structure, then I can rebase my work on yours without legacy import.

@rawfiner
Copy link
Collaborator

To sum up what I think between being before or after demosaic for highlight reconstruction:

before demosaic after demosaic
performance no downscaling in darkroom downscaling in darkroom helps for preview performance
preview accuracy perfect (no downscaling) maybe imperfect (need to generate previews from a downscaled image, and downscaling may make some pixel value go below the threshold)
detect pixels to correct straightforward maybe need to select all pixels > threshold, and then extend the selection to the surrounding ones to take into account the fact that demosaic combined invalid values with valid ones, making invalid values lower than the threshold
code maintenance / camera support may not be easy to support all CFA camera support is straightforward. Probably a bit less code, and less complicated.

I think it is worth trying to have the module after demosaic for both of your approaches:

  • for @aurelienpierre 's approach, it would help a lot for preview performance
  • I would like a lot being able to @jenshannoschwalm approach as well on my fuji files with xtrans CFA :-)

@aurelienpierre
Copy link
Member Author

Notice that I already postfilter the clipping mask with a 5x5 box blur because the reconstruction creates overshoots otherwise.

@jenshannoschwalm
Copy link
Collaborator

On second thought , processing per channel will be 4-fold, correct?

@aurelienpierre
Copy link
Member Author

On second thought , processing per channel will be 4-fold, correct?

what do you mean ?

@jenshannoschwalm
Copy link
Collaborator

At least for the segmentation I do, in Bayer space the size for width / height are half size compared to after demosaicing.

@rawfiner
Copy link
Collaborator

Indeed if you did segmentation using red or blue channel, you get 4x more pixels after demosaic, so it can result in a slowdown

@aurelienpierre
Copy link
Member Author

You can always downsample though.

@matze
Copy link
Contributor

matze commented Jan 14, 2022

No strange behavior on AMD although it is a bit slow.

@aurelienpierre
Copy link
Member Author

I know it's slow, but I don't see any workaround…

@aurelienpierre
Copy link
Member Author

Note: I have changed the sequence a bit, for the sake of run time.

The number of wavelets scales is limited to 8, meaning a max radius of 256 px. But since the process is now iterative, we need to double the padding size to avoid straight artifacts at tile's edges. So the max padding is 512 px.

The result is very large blown areas, with radii larger than 512 px will not be corrected at all. It's either this or insane runtimes.

@SoupyGit
Copy link

SoupyGit commented Feb 8, 2022

"The result is very large blown areas, with radii larger than 512 px will not be corrected at all. It's either this or insane runtimes."

Just a thought, could potentially have both, with a user drop down to select blown areas: small or large, where small allows for many iterations, but large allows only for one.

@MStraeten
Copy link
Collaborator

MStraeten commented Feb 8, 2022

Build fails on win10:

In file included from C:/msys64/home/me/darktable/build/lib/darktable/plugins/introspection_highlights.c:71:
C:/msys64/home/SHM/darktable/src/iop/highlights.c:1564: error: unterminated #if
 1564 | #if HAVE_OPENCL
      |

introduced with dec805c
line 1797 needs to be uncommeted

@aurelienpierre aurelienpierre force-pushed the highlights-reconstruction-guided-laplacian branch from 5b1f2b8 to 3c2c685 Compare March 31, 2022 20:27
@aurelienpierre
Copy link
Member Author

I solved conflicts, rebased and fixed the anisotropic diffusion kernel similarly to what @flannelhead did for diffuse.c.

@TurboGit this is ready to go !

@TurboGit
Copy link
Member

TurboGit commented Apr 1, 2022

I've not looked at the code yet, testing it I see that I have some magenta color cast on tow test images.

I'm using Filmic and recontruct is threshold is negative -0.8IL.

First image, module highlight reconstruction set to

  1. clip
    image

  2. LCh
    image

  3. color
    image

  4. guider laplacian
    image

Second image:

  1. clip
    image

  2. LCh
    image

  3. color
    image

  4. guider laplacian
    image

Is that a wrong use on my side?

Also I see that the the X-Trans (RAF) are not supported, I suppose this is expected?

@TurboGit
Copy link
Member

TurboGit commented Apr 1, 2022

If I change the white rel exposure in Filmic it somehow solves the issue. To me this method seems to be very sensitive to while being too strong, again not sure it is expected. Until the doc is there it is hard for me to guess all that :)

@TurboGit
Copy link
Member

TurboGit commented Apr 1, 2022

With Filmic v6 it is even harder to get a decent output with this algorithm even when tweaking the white:

image

@TurboGit
Copy link
Member

TurboGit commented Apr 1, 2022

With Filmic default setting in v5:
image

in v6:
image

@aurelienpierre
Copy link
Member Author

@TurboGit It's important to understand that this algo doesn't care about colors. There is no desaturation or no explicit color propagation like the other methods do. What is done is:

  1. attempt to copy-paste details (modelled as the laplacian) from valid channels to clipped channels,
  2. attempt to diffuse the content from neighbouring regions.

This means that the algo will have an hard time with large areas, for which you need to increase the diameter of reconstruction and the number of iterations, at the expense of runtime.

Also, since filmic v6 does not desaturate highlights mandatorily, obviously magenta casts are more visible.

@TurboGit
Copy link
Member

TurboGit commented Apr 1, 2022

@TurboGit It's important to understand that this algo doesn't care about colors. There is no desaturation or no explicit color propagation like the other methods do. What is done is:

1. attempt to copy-paste details (modelled as the laplacian) from valid channels to clipped channels,

2. attempt to diffuse the content from neighbouring regions.

This means that the algo will have an hard time with large areas, for which you need to increase the diameter of reconstruction and the number of iterations, at the expense of runtime.

Also, since filmic v6 does not desaturate highlights mandatorily, obviously magenta casts are more visible.

Ok, will test again with that in mind. Thanks for the explanation!

@TurboGit
Copy link
Member

TurboGit commented Apr 1, 2022

I have hard time to find a picture where it makes a difference with the other algorithm. I generally find that "color reconstruction" works best without the color cast and that Laplacian gives almost the same result but with noise added with the image is high iso.

@aurelienpierre : Can you share a picture where this algorithm gives results that cannot be achieved with the others?

@TurboGit
Copy link
Member

TurboGit commented Apr 2, 2022

There is a missing update somewhere. To reproduce:

  1. select an image
  2. in highlight reconstruction select laplacian
  3. using filmstrip change image
  4. see that the highlight reconstruction is still laplacian
  5. try to click on iteration slider
  6. see error, update is now done and the slider has vanished

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.

GUI update bug.

Messing with the LF layers causes non-smooth edge artifacts around
areas to reconstruct.

The cost of using only HF layers is more iterations
and/or larger reconstruction radii are needed.

The additional benefit is now the wavelets decomposition
can be rewritten from top to bottom, meaning only 3 buffers
(LF, HF, synthesis) need to be stored, instead of all HF scales.
This will prevent tiling in most situations.
@aurelienpierre
Copy link
Member Author

@TurboGit GUI bug fixed, it was an easy one.

Next commit changes slightly the reconstruction logic to fix an artifact I uncovered. It actually removes some operation.

It will allow to rewrite the wavelets decomposition to be sequential instead of parallel, meaning only 3 buffers (LF, HF, synthesis) will be needed instead of having to store all HF buffers. That will prevent tiling and the huge perf penalty that goes with it.

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.

Looks ok, thanks!

@TurboGit TurboGit merged commit 7258b1c into darktable-org:master Apr 9, 2022
@elstoc elstoc added documentation-complete needed documentation is merged in dtdocs and removed documentation-pending a documentation work is required labels May 28, 2022
@aurelienpierre aurelienpierre deleted the highlights-reconstruction-guided-laplacian 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
documentation-complete needed documentation is merged in dtdocs feature: enhancement current features to improve scope: image processing correcting pixels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants