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

Color balance module : add the ASC CDL mode #1734

Merged
merged 14 commits into from Oct 16, 2018

Conversation

Projects
None yet
3 participants
@aurelienpierre
Contributor

aurelienpierre commented Sep 22, 2018

As Blender and many video editors, add a color balance mode based on the ASC color decision list https://en.wikipedia.org/wiki/Color_Decision_List

This allows to color-grade in linear ProPhoto RGB space, whereas the previous version was in gamma-corrected sRGB and led to saturation issues.

The previous mode is retained as an option. The legacy params are imported. The OpenCL and SSE2 versions are provided and working (as far as I tested).

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Sep 23, 2018

Contributor

Warning : with DNG files, changing the main power value gives either a green or a magenta color cast. Tested with NEF, RAF, ARW and CR2 files, it works as expected.

Contributor

aurelienpierre commented Sep 23, 2018

Warning : with DNG files, changing the main power value gives either a green or a magenta color cast. Tested with NEF, RAF, ARW and CR2 files, it works as expected.

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Sep 24, 2018

Contributor

DNG files work now as expected. I have added the fulcrum contrast and master saturation options.

In the pure C variant, just enabling the module gives an unexpected exposure boost. Something is wrong with the Lab <-> ProphotoRGB conversion in C, I can't find what. (I have checked the conversion matrices from 3 sources). Using sRGB fixes the problem for the pure C version (I suspect the gamma conversion does the trick).

SSE and OpenCL versions behave as expected with Prophoto RGB conversion and everything.

I have done a bit of lib refactoring. I think rewriting the SSE and OpenCL color conversions as native dot products would give a boost in performance at some point.

Contributor

aurelienpierre commented Sep 24, 2018

DNG files work now as expected. I have added the fulcrum contrast and master saturation options.

In the pure C variant, just enabling the module gives an unexpected exposure boost. Something is wrong with the Lab <-> ProphotoRGB conversion in C, I can't find what. (I have checked the conversion matrices from 3 sources). Using sRGB fixes the problem for the pure C version (I suspect the gamma conversion does the trick).

SSE and OpenCL versions behave as expected with Prophoto RGB conversion and everything.

I have done a bit of lib refactoring. I think rewriting the SSE and OpenCL color conversions as native dot products would give a boost in performance at some point.

@TurboGit

This comment has been minimized.

Show comment
Hide comment
@TurboGit

TurboGit Sep 24, 2018

Member

Good job Aurélien. I'm probably not the right person to merge this PR, but it looks good to me and works as expected. I expect houz or hanatos to have a deeper look. Thanks.

Member

TurboGit commented Sep 24, 2018

Good job Aurélien. I'm probably not the right person to merge this PR, but it looks good to me and works as expected. I expect houz or hanatos to have a deeper look. Thanks.

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Sep 24, 2018

Contributor

Thanks Pascal ! If you compile my master branch, you will get it along with the Log unbreak profile, and both together work really well to recover high dynamic range images.

Contributor

aurelienpierre commented Sep 24, 2018

Thanks Pascal ! If you compile my master branch, you will get it along with the Log unbreak profile, and both together work really well to recover high dynamic range images.

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Sep 27, 2018

Contributor

I have added HSL sliders in addition to the RGB ones, as I requested here https://redmine.darktable.org/issues/11959.

This doesn't change the data structure (parameters are still RGB). Editing RGB sliders updates HSL ones, and the other way around.

I still have a problem of slight magenta shift with the SSE ProphotoRGB conversion, OpenCL and pure C behave as expected.

I have rewritten SSE color conversion function in algebraic notation instead of SSE intrinsics to improve readability.

Contributor

aurelienpierre commented Sep 27, 2018

I have added HSL sliders in addition to the RGB ones, as I requested here https://redmine.darktable.org/issues/11959.

This doesn't change the data structure (parameters are still RGB). Editing RGB sliders updates HSL ones, and the other way around.

I still have a problem of slight magenta shift with the SSE ProphotoRGB conversion, OpenCL and pure C behave as expected.

I have rewritten SSE color conversion function in algebraic notation instead of SSE intrinsics to improve readability.

@edgardoh

This comment has been minimized.

Show comment
Hide comment
@edgardoh

edgardoh Sep 29, 2018

Contributor

I've playing with this as well, very nice! Results are very good and is very easy to get them.
Maybe add a combo with lift/gamma/gain/(all) so it doesn't take so much space on the UI?
Also, is it possible to have a new group for the entire range (not only shadows/highlights/midtones)? Sometimes I like to create a color cast and then play with each range in particular.

Contributor

edgardoh commented Sep 29, 2018

I've playing with this as well, very nice! Results are very good and is very easy to get them.
Maybe add a combo with lift/gamma/gain/(all) so it doesn't take so much space on the UI?
Also, is it possible to have a new group for the entire range (not only shadows/highlights/midtones)? Sometimes I like to create a color cast and then play with each range in particular.

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Sep 29, 2018

Contributor

Maybe add a combo with lift/gamma/gain/(all) so it doesn't take so much space on the UI?

You mean on the text label ?

Also, is it possible to have a new group for the entire range (not only shadows/highlights/midtones)?

Like a master hue setting ?

Contributor

aurelienpierre commented Sep 29, 2018

Maybe add a combo with lift/gamma/gain/(all) so it doesn't take so much space on the UI?

You mean on the text label ?

Also, is it possible to have a new group for the entire range (not only shadows/highlights/midtones)?

Like a master hue setting ?

@edgardoh

This comment has been minimized.

Show comment
Hide comment
@edgardoh

edgardoh Sep 29, 2018

Contributor
Contributor

edgardoh commented Sep 29, 2018

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Sep 30, 2018

Contributor

I mean to add a combo box with those entries that show/hide each group (or all of them if (all) is selected)

I still don't understand 😁 You want to selectively enable/disable the RGB set of sliders and the HSL ones ?

Exactly. It can be added to the combo box above, maybe as a default.

The problem with that is it would change the maths behind and lose the compatibility with the ACL Color Decision List standard. Technically-speaking, the slope is sort of the master (it's the linear factor), and the first parameter you should set in your workflow. Then, you can come back a bit on it by setting the offset, which, again applies to the whole range but, numerically, weighs more on the low-lights. And as the last step, you fix the power. That's why the mode is called slope/offset/power, in the order you are supposed to adjust the settings.

Contributor

aurelienpierre commented Sep 30, 2018

I mean to add a combo box with those entries that show/hide each group (or all of them if (all) is selected)

I still don't understand 😁 You want to selectively enable/disable the RGB set of sliders and the HSL ones ?

Exactly. It can be added to the combo box above, maybe as a default.

The problem with that is it would change the maths behind and lose the compatibility with the ACL Color Decision List standard. Technically-speaking, the slope is sort of the master (it's the linear factor), and the first parameter you should set in your workflow. Then, you can come back a bit on it by setting the offset, which, again applies to the whole range but, numerically, weighs more on the low-lights. And as the last step, you fix the power. That's why the mode is called slope/offset/power, in the order you are supposed to adjust the settings.

@edgardoh

This comment has been minimized.

Show comment
Hide comment
@edgardoh

edgardoh Sep 30, 2018

Contributor
Contributor

edgardoh commented Sep 30, 2018

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Sep 30, 2018

Contributor

I have added neutralization color-pickers on each hue slider. The idea is to select a color that should be neutral (grey) and compute the right parameters in the color balance to invert its current hue and neutralize it. The saturation is not always on point though. It's still useful to remove indesirable color casts, due to several lighting sources at different color temperature.

Contributor

aurelienpierre commented Sep 30, 2018

I have added neutralization color-pickers on each hue slider. The idea is to select a color that should be neutral (grey) and compute the right parameters in the color balance to invert its current hue and neutralize it. The saturation is not always on point though. It's still useful to remove indesirable color casts, due to several lighting sources at different color temperature.

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 6, 2018

Contributor

Another improvement here : I have bounded the RGB sliders and corrected them according to the luminance.

Now, when you edit a RGB channel, the 2 others are updated so that the luminance is not affected. So, the RGB channels control only the saturation and hue, making them fully independant from the factor (which is a luma correction).

I think that's pretty cool.

Contributor

aurelienpierre commented Oct 6, 2018

Another improvement here : I have bounded the RGB sliders and corrected them according to the luminance.

Now, when you edit a RGB channel, the 2 others are updated so that the luminance is not affected. So, the RGB channels control only the saturation and hue, making them fully independant from the factor (which is a luma correction).

I think that's pretty cool.

@TurboGit

This comment has been minimized.

Show comment
Hide comment
@TurboGit

TurboGit Oct 11, 2018

Member

You need to remove the rawspeed submodule from here.
Also there is a small conflict to be resolved.

I think also that it should be better to rebase all this and maybe even squash all the commits together at this stage. No need to see all the "fix" steps now.

Member

TurboGit commented Oct 11, 2018

You need to remove the rawspeed submodule from here.
Also there is a small conflict to be resolved.

I think also that it should be better to rebase all this and maybe even squash all the commits together at this stage. No need to see all the "fix" steps now.

@TurboGit TurboGit self-assigned this Oct 11, 2018

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 12, 2018

Contributor

Isn't rawspeed listed in the gitignore that it keeps showing up in the commits ?

I will fix that, maybe add color-pickers for lift and gain factors, and I think we wil be good.

Contributor

aurelienpierre commented Oct 12, 2018

Isn't rawspeed listed in the gitignore that it keeps showing up in the commits ?

I will fix that, maybe add color-pickers for lift and gain factors, and I think we wil be good.

@TurboGit

This comment has been minimized.

Show comment
Hide comment
@TurboGit

TurboGit Oct 12, 2018

Member

Ok, let me know when ready. We are approaching the code freeze I suppose. So if we want this in the next release (and I think it would be really nice) I need to review and merge soon. Thanks.

Member

TurboGit commented Oct 12, 2018

Ok, let me know when ready. We are approaching the code freeze I suppose. So if we want this in the next release (and I think it would be really nice) I need to review and merge soon. Thanks.

Aurélien PIERRE added some commits Oct 13, 2018

Aurélien PIERRE
Add comments
Add an optimizer for luma values
Use the luma correction in the color neutralization optimization for better accuracy
Refactor some code
Add comments
Disable the luma normalization for RGB sliders in lift/gamma/gain mode to let it as is.
Aurélien PIERRE
Add comments
Add an optimizer for luma values
Use the luma correction in the color neutralization optimization for better accuracy
Refactor some code
Add comments
Disable the luma normalization for RGB sliders in lift/gamma/gain mode to let it as is.
@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 13, 2018

Contributor

I think it's clean ?

So, last changes:

  1. I added a load of comments to explain the maths happening in the optimizers
  2. I added the optimizer for luma distribution, as promised
  3. these optimizers should be a tad more robust in difficult cases (I added constraints)
  4. I removed the RGB sliders bound (for luma normalization) for the legacy lift/gamma/gain, so people can continue to use it as before. It's still there for the
  5. I cherry-picked the common lib colorspaces_inline_conversions.h that I optimized, corrected, and made more readable by replacing the SS2 special functions by algebraic operations (following the advice of one Rawtherapee dev, because it seems algebraic operations can be more optimized by the compilator).
  6. I added the SIMD() parameter for openmp loops
  7. I added some more info to the labels.

Screenshots:

HSL sliders:
capture du 2018-10-13 01-34-00

RGB sliders:
capture du 2018-10-13 01-34-14

Both sliders:
capture du 2018-10-13 01-34-31

Directions to test:

  • Always set-up the slope, then lift, then gamma in this order.
  • Always set-up the luma (factors) before the color (RGB/hue/saturation).
  • The factor color-pickers take the max luma in the zone for the slope, the min luma in the zone for the offset, the average luma for the power. Choose your zones accordingly. Once you have set 3 samples (slope/offset/power), you can trigger the "optimize luma" optimizer. It will fit the histogram inside the whole dynamic range [0-100 %] and normalize the average luma to 50 %, similarly to the levels but in RGB.
  • The hue color-pickers aim at reverting the color of the samples you give them to neutralize color casts. They take the average color of the zone. Once you have set the 3 samples (slope/offset/gamma), you can trigger the "neutralize color" optimizer. The samples should have a neutral color (grey-ish) in real life in low/mid/high-lights (if you take real saturated colors, I don't garantee the result). That's usefull when you have several white balances due to several light sources in the same picture.
  • Set the master saturation and contrast in last
  • The grey fulcrum is the luma value that is non-affected by the contrast adjustement. In portrait, use the color-picker to select the whole face and get its average value. Then, the contrast you will add will enhance the shape and features. 18 % Luma = 50 % L(ab) so you should always get 18 % if the gamma/power factor was set properly.

Note that the color-pickers are designed for the slope/offset/power version, as well as the HSL -> RGB conversion and the RGB luma normalization. But they still work fairly for lift/gamma/gain.

You will notice that if you try RGB -> HSL -> RGB' conversions on the sliders, for primaries, you will always have an error : RGB = RGB' ± 0.005. That's expected, due to the conversion of color shiftts between a cylindric and a cartesian vectorial space. However, ping me if you get errors >= 0.01, it shouldn't happen.

Enjoy !

Contributor

aurelienpierre commented Oct 13, 2018

I think it's clean ?

So, last changes:

  1. I added a load of comments to explain the maths happening in the optimizers
  2. I added the optimizer for luma distribution, as promised
  3. these optimizers should be a tad more robust in difficult cases (I added constraints)
  4. I removed the RGB sliders bound (for luma normalization) for the legacy lift/gamma/gain, so people can continue to use it as before. It's still there for the
  5. I cherry-picked the common lib colorspaces_inline_conversions.h that I optimized, corrected, and made more readable by replacing the SS2 special functions by algebraic operations (following the advice of one Rawtherapee dev, because it seems algebraic operations can be more optimized by the compilator).
  6. I added the SIMD() parameter for openmp loops
  7. I added some more info to the labels.

Screenshots:

HSL sliders:
capture du 2018-10-13 01-34-00

RGB sliders:
capture du 2018-10-13 01-34-14

Both sliders:
capture du 2018-10-13 01-34-31

Directions to test:

  • Always set-up the slope, then lift, then gamma in this order.
  • Always set-up the luma (factors) before the color (RGB/hue/saturation).
  • The factor color-pickers take the max luma in the zone for the slope, the min luma in the zone for the offset, the average luma for the power. Choose your zones accordingly. Once you have set 3 samples (slope/offset/power), you can trigger the "optimize luma" optimizer. It will fit the histogram inside the whole dynamic range [0-100 %] and normalize the average luma to 50 %, similarly to the levels but in RGB.
  • The hue color-pickers aim at reverting the color of the samples you give them to neutralize color casts. They take the average color of the zone. Once you have set the 3 samples (slope/offset/gamma), you can trigger the "neutralize color" optimizer. The samples should have a neutral color (grey-ish) in real life in low/mid/high-lights (if you take real saturated colors, I don't garantee the result). That's usefull when you have several white balances due to several light sources in the same picture.
  • Set the master saturation and contrast in last
  • The grey fulcrum is the luma value that is non-affected by the contrast adjustement. In portrait, use the color-picker to select the whole face and get its average value. Then, the contrast you will add will enhance the shape and features. 18 % Luma = 50 % L(ab) so you should always get 18 % if the gamma/power factor was set properly.

Note that the color-pickers are designed for the slope/offset/power version, as well as the HSL -> RGB conversion and the RGB luma normalization. But they still work fairly for lift/gamma/gain.

You will notice that if you try RGB -> HSL -> RGB' conversions on the sliders, for primaries, you will always have an error : RGB = RGB' ± 0.005. That's expected, due to the conversion of color shiftts between a cylindric and a cartesian vectorial space. However, ping me if you get errors >= 0.01, it shouldn't happen.

Enjoy !

@TurboGit

This comment has been minimized.

Show comment
Hide comment
@TurboGit

TurboGit Oct 13, 2018

Member

One issue found:

  • open module, select "both" for "color control sliders"
  • click on the color picker IN the color picker module on the left
  • the color balance module is reset to "HSL" mode only instead of both
Member

TurboGit commented Oct 13, 2018

One issue found:

  • open module, select "both" for "color control sliders"
  • click on the color picker IN the color picker module on the left
  • the color balance module is reset to "HSL" mode only instead of both
@TurboGit

This comment has been minimized.

Show comment
Hide comment
@TurboGit

TurboGit Oct 13, 2018

Member

Another one:

  • open a new picture
  • activate color balance
  • let all default value
  • click on the picker for the factor in offset
  • draw an area
    => nothing happens

Is that expected? Maybe a wrong procedure on my side?

Member

TurboGit commented Oct 13, 2018

Another one:

  • open a new picture
  • activate color balance
  • let all default value
  • click on the picker for the factor in offset
  • draw an area
    => nothing happens

Is that expected? Maybe a wrong procedure on my side?

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 13, 2018

Contributor

The first issue is a real issue. For the second, you need to click once on the color picker, draw the area, then click again on the color picker icon to validate the color.

Contributor

aurelienpierre commented Oct 13, 2018

The first issue is a real issue. For the second, you need to click once on the color picker, draw the area, then click again on the color picker icon to validate the color.

@aurelienpierre aurelienpierre changed the title from Color balance module : add the ACD CDL mode to Color balance module : add the ASC CDL mode Oct 15, 2018

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 15, 2018

Contributor

Actually, it still breaks and I have no clue. The faulty lines are:

// Save the patch color for the optimization
dt_iop_colorbalance_data_t *d = (dt_iop_colorbalance_data_t *)self->data;
*d->color_patches_lift = *RGB;
d->color_patches_flags[LIFT] = 1;

They occur in every color-picker callback and are intended to store the RGB values in a private data structure to be fetched later by the optimizer:

typedef struct dt_iop_colorbalance_data_t
{
dt_iop_colorbalance_mode_t mode;
float lift[CHANNEL_SIZE], gamma[CHANNEL_SIZE], gain[CHANNEL_SIZE];
float saturation, contrast, grey;
float color_patches_lift[CHANNEL_SIZE];
float color_patches_gamma[CHANNEL_SIZE];
float color_patches_gain[CHANNEL_SIZE];
int color_patches_flags[LEVELS];
float luma_patches[LEVELS];
int luma_patches_flags[LEVELS];
} dt_iop_colorbalance_data_t;

Commenting these lines fixes the problem, and I absolutely don't see why. It doesn't seem to be an out-of-bound error while indexing the arrays.

The errors that arise in gdb always affect other modules (basecurve) or OpenCL kernels cleanups, so I suspect some memory shift.

Hoping it would help, I have added:

void init_pipe(dt_iop_module_t *self, dt_dev_pixelpipe_t *pipe, dt_dev_pixelpipe_iop_t *piece)
{
piece->data = calloc(1, sizeof(dt_iop_colorbalance_data_t));
self->commit_params(self, self->default_params, pipe, piece);
}
void cleanup_pipe(dt_iop_module_t *self, dt_dev_pixelpipe_t *pipe, dt_dev_pixelpipe_iop_t *piece)
{
free(piece->data);
piece->data = NULL;
}

but it does not.

Contributor

aurelienpierre commented Oct 15, 2018

Actually, it still breaks and I have no clue. The faulty lines are:

// Save the patch color for the optimization
dt_iop_colorbalance_data_t *d = (dt_iop_colorbalance_data_t *)self->data;
*d->color_patches_lift = *RGB;
d->color_patches_flags[LIFT] = 1;

They occur in every color-picker callback and are intended to store the RGB values in a private data structure to be fetched later by the optimizer:

typedef struct dt_iop_colorbalance_data_t
{
dt_iop_colorbalance_mode_t mode;
float lift[CHANNEL_SIZE], gamma[CHANNEL_SIZE], gain[CHANNEL_SIZE];
float saturation, contrast, grey;
float color_patches_lift[CHANNEL_SIZE];
float color_patches_gamma[CHANNEL_SIZE];
float color_patches_gain[CHANNEL_SIZE];
int color_patches_flags[LEVELS];
float luma_patches[LEVELS];
int luma_patches_flags[LEVELS];
} dt_iop_colorbalance_data_t;

Commenting these lines fixes the problem, and I absolutely don't see why. It doesn't seem to be an out-of-bound error while indexing the arrays.

The errors that arise in gdb always affect other modules (basecurve) or OpenCL kernels cleanups, so I suspect some memory shift.

Hoping it would help, I have added:

void init_pipe(dt_iop_module_t *self, dt_dev_pixelpipe_t *pipe, dt_dev_pixelpipe_iop_t *piece)
{
piece->data = calloc(1, sizeof(dt_iop_colorbalance_data_t));
self->commit_params(self, self->default_params, pipe, piece);
}
void cleanup_pipe(dt_iop_module_t *self, dt_dev_pixelpipe_t *pipe, dt_dev_pixelpipe_iop_t *piece)
{
free(piece->data);
piece->data = NULL;
}

but it does not.

Show resolved Hide resolved src/iop/colorbalance.c
Show resolved Hide resolved src/iop/colorbalance.c
Show resolved Hide resolved src/iop/colorbalance.c
@TurboGit

This comment has been minimized.

Show comment
Hide comment
@TurboGit

TurboGit Oct 15, 2018

Member

Seems like the crash is fixed. Cannot reproduce, same on your side now?

Member

TurboGit commented Oct 15, 2018

Seems like the crash is fixed. Cannot reproduce, same on your side now?

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 15, 2018

Contributor

It depends how I build. With cmake, it seems to work. With build.sh, it fails sometimes.

Contributor

aurelienpierre commented Oct 15, 2018

It depends how I build. With cmake, it seems to work. With build.sh, it fails sometimes.

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 15, 2018

Contributor

still outputs, when closing :

Thread 1 "darktable" received signal SIGSEGV, Segmentation fault.
(gdb) bt full
#0  0x00007ffff3e8234c in g_module_close () at /usr/lib/x86_64-linux-gnu/libgmodule-2.0.so.0
#1  0x00007ffff7a7e1af in dt_view_unload_module (view=0x555557817470)
    at /home/aurelien/Documents/Programmes/darktable/src/views/view.c:228
        iter = 0x555557772580
#2  0x00007ffff7a7e1af in dt_view_manager_cleanup (vm=<optimized out>)
    at /home/aurelien/Documents/Programmes/darktable/src/views/view.c:101
        iter = 0x555557772580
#3  0x00007ffff796be61 in dt_cleanup ()
    at /home/aurelien/Documents/Programmes/darktable/src/common/darktable.c:1066
        init_gui = 1
#4  0x00007ffff7a55b41 in dt_gui_gtk_run (gui=<optimized out>)
    at /home/aurelien/Documents/Programmes/darktable/src/gui/gtk.c:1201
        widget = <optimized out>
        allocation = {x = 362, y = 12, width = 401, height = 601}
#5  0x0000555555554830 in main (argc=<optimized out>, argv=<optimized out>)
    at /home/aurelien/Documents/Programmes/darktable/src/main.c:83
Contributor

aurelienpierre commented Oct 15, 2018

still outputs, when closing :

Thread 1 "darktable" received signal SIGSEGV, Segmentation fault.
(gdb) bt full
#0  0x00007ffff3e8234c in g_module_close () at /usr/lib/x86_64-linux-gnu/libgmodule-2.0.so.0
#1  0x00007ffff7a7e1af in dt_view_unload_module (view=0x555557817470)
    at /home/aurelien/Documents/Programmes/darktable/src/views/view.c:228
        iter = 0x555557772580
#2  0x00007ffff7a7e1af in dt_view_manager_cleanup (vm=<optimized out>)
    at /home/aurelien/Documents/Programmes/darktable/src/views/view.c:101
        iter = 0x555557772580
#3  0x00007ffff796be61 in dt_cleanup ()
    at /home/aurelien/Documents/Programmes/darktable/src/common/darktable.c:1066
        init_gui = 1
#4  0x00007ffff7a55b41 in dt_gui_gtk_run (gui=<optimized out>)
    at /home/aurelien/Documents/Programmes/darktable/src/gui/gtk.c:1201
        widget = <optimized out>
        allocation = {x = 362, y = 12, width = 401, height = 601}
#5  0x0000555555554830 in main (argc=<optimized out>, argv=<optimized out>)
    at /home/aurelien/Documents/Programmes/darktable/src/main.c:83
@TurboGit

This comment has been minimized.

Show comment
Hide comment
@TurboGit

TurboGit Oct 15, 2018

Member

And related to OpenCL, I cannot reproduce with standard code path.

Member

TurboGit commented Oct 15, 2018

And related to OpenCL, I cannot reproduce with standard code path.

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 15, 2018

Contributor

So it was because I had

codepaths/openmp_simd=true
codepaths/sse2=true

in darktablerc. Seems to work now.

Contributor

aurelienpierre commented Oct 15, 2018

So it was because I had

codepaths/openmp_simd=true
codepaths/sse2=true

in darktablerc. Seems to work now.

@TurboGit

This comment has been minimized.

Show comment
Hide comment
@TurboGit

TurboGit Oct 15, 2018

Member

And when I replace colorbalance_cdl() implementation with a simple call to colorbalance (in extended.cl) I don't have the crash (the image is not correct of course). So it looks like the issue is in the CL code in colorbalance_cdl(). Now I'm far from expert in OpenCL :(

Member

TurboGit commented Oct 15, 2018

And when I replace colorbalance_cdl() implementation with a simple call to colorbalance (in extended.cl) I don't have the crash (the image is not correct of course). So it looks like the issue is in the CL code in colorbalance_cdl(). Now I'm far from expert in OpenCL :(

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 15, 2018

Contributor

I don't understand : what crash do you get with colorbalance_cdl() OpenCL ? Both versions work with no error on my side, with the last commit.

Contributor

aurelienpierre commented Oct 15, 2018

I don't understand : what crash do you get with colorbalance_cdl() OpenCL ? Both versions work with no error on my side, with the last commit.

@TurboGit

This comment has been minimized.

Show comment
Hide comment
@TurboGit

TurboGit Oct 16, 2018

Member

When I'm leaving dt I get segmentation violation ini different part of the application (not always the same). This seems to indicate that there is some memory corruption somewhere.

Member

TurboGit commented Oct 16, 2018

When I'm leaving dt I get segmentation violation ini different part of the application (not always the same). This seems to indicate that there is some memory corruption somewhere.

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 16, 2018

Contributor

@TurboGit I have commented out the faulty lines that store the current color into the private data structure, like that:

#ifdef OPTIM
    dt_iop_colorbalance_data_t *d = (dt_iop_colorbalance_data_t *)self->data;
    d->luma_patches[GAIN] = XYZ[1];
    d->luma_patches_flags[GAIN] = 1;
#endif

Can you confirm everything works that way so that we ensure these are the true offenders ?

Contributor

aurelienpierre commented Oct 16, 2018

@TurboGit I have commented out the faulty lines that store the current color into the private data structure, like that:

#ifdef OPTIM
    dt_iop_colorbalance_data_t *d = (dt_iop_colorbalance_data_t *)self->data;
    d->luma_patches[GAIN] = XYZ[1];
    d->luma_patches_flags[GAIN] = 1;
#endif

Can you confirm everything works that way so that we ensure these are the true offenders ?

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 16, 2018

Contributor

I think I have found it.

The private data structure (dt_iop_colorbalance_data_t *) is stored in (dt_dev_pixelpipe_t *)pipe->data. Here, I'm writing in (dt_iop_module_t *)self->data which is something else, hence the memory corruption.

But thanks to the barely-commented and non-documented sourcecode of dt, I have not found how to access the pixelpipe from the module structure.

Contributor

aurelienpierre commented Oct 16, 2018

I think I have found it.

The private data structure (dt_iop_colorbalance_data_t *) is stored in (dt_dev_pixelpipe_t *)pipe->data. Here, I'm writing in (dt_iop_module_t *)self->data which is something else, hence the memory corruption.

But thanks to the barely-commented and non-documented sourcecode of dt, I have not found how to access the pixelpipe from the module structure.

@TurboGit

This comment has been minimized.

Show comment
Hide comment
@TurboGit

TurboGit Oct 16, 2018

Member

Ok, no more crashes!

And indeed valgrind (I run a session last night, very very slow under valgrind!) told me that d->luma_patches_flags[GAIN] = 1; was writing 4 bytes out of bound!

So you've found the offenders :)

Member

TurboGit commented Oct 16, 2018

Ok, no more crashes!

And indeed valgrind (I run a session last night, very very slow under valgrind!) told me that d->luma_patches_flags[GAIN] = 1; was writing 4 bytes out of bound!

So you've found the offenders :)

@TurboGit

This comment has been minimized.

Show comment
Hide comment
@TurboGit

TurboGit Oct 16, 2018

Member

No more crashes but I cannot run the optimizer.

Member

TurboGit commented Oct 16, 2018

No more crashes but I cannot run the optimizer.

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 16, 2018

Contributor

So you've found the offenders :)

That only took 18h 🙄

No more crashes but I cannot run the optimizer.

that's normal, it's disabled for debugging. I still need to figure out how to reach the pipe data from the module.

Contributor

aurelienpierre commented Oct 16, 2018

So you've found the offenders :)

That only took 18h 🙄

No more crashes but I cannot run the optimizer.

that's normal, it's disabled for debugging. I still need to figure out how to reach the pipe data from the module.

@aurelienpierre

This comment has been minimized.

Show comment
Hide comment
@aurelienpierre

aurelienpierre Oct 16, 2018

Contributor

Ok, now I store the data in dt_iop_colorbalance_gui_data_t so it should be clean !!! 😄

Contributor

aurelienpierre commented Oct 16, 2018

Ok, now I store the data in dt_iop_colorbalance_gui_data_t so it should be clean !!! 😄

@TurboGit

This comment has been minimized.

Show comment
Hide comment
@TurboGit

TurboGit Oct 16, 2018

Member

Yes, all good now! Thanks.

Member

TurboGit commented Oct 16, 2018

Yes, all good now! Thanks.

@TurboGit TurboGit merged commit dedfa8b into darktable-org:master Oct 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@edgardoh

This comment has been minimized.

Show comment
Hide comment
@edgardoh

edgardoh Oct 16, 2018

Contributor
Contributor

edgardoh commented Oct 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment