agx: Fix crash during initialisation

[kofa73]

Fixed a crash during initialisation caused by callback triggered before all controls created; also: remove redundant soft limits (matching hard limits).
bt.txt

[kofa73]

Cause: null pointer dereference during initialisation: slider params change in gui_init -> triggers gui_changed -> calls _update_curve_warnings, which tries to adjust not yet created UI elements.

To prevent this, I added a gboolean initialized to dt_iop_agx_gui_data_t. Would darktable.gui->reset be a better option? I'd prefer either over null-checks everywhere.

Relevant part of backtrace:

#6  0x00007bc6dd6458d0 in <signal handler called> () at /lib/x86_64-linux-gnu/libc.so.6
#7  dt_bauhaus_widget_set_quad_paint (widget=0x0, f=0x0, paint_flags=16, paint_data=0x0) at /home/kofa/darktable/src/bauhaus/bauhaus.c:1100
        w = 0x0
#8  0x00007bc65ff842b0 in _update_curve_warnings (self=<optimized out>) at /home/kofa/darktable/src/iop/agx.c:2089
        g = 0x60605ebf5d80
        p = <optimized out>
        warnings_enabled = 1
        params = {black_relative_ev = -10, white_relative_ev = 6.5, range_in_ev = 16.5, curve_gamma = 2.20000005, pivot_x = 0.606060624, pivot_y = 0.45865643, target_black = 0, toe_power = 1.54999995, toe_transition_x = 0.606060624, toe_transition_y = 0.45865643, toe_scale = -0.502375484, need_convex_toe = 0, toe_fallback_coefficient = 2.92516279, toe_fallback_power = 3.6998713, slope = 2.79999995, intercept = -1.23831332, target_white = 1, shoulder_power = 1.54999995, shoulder_transition_x = 0.606060624, shoulder_transition_y = 0.45865643, shoulder_scale = 0.702158868, need_concave_shoulder = 0, shoulder_fallback_coefficient = 3.6125803, shoulder_fallback_power = 2.03757882, look_lift = 0, look_slope = 1, look_power = 1, look_saturation = 1, look_original_hue_mix_ratio = 0, look_tuned = 0, restore_hue = 0}
#9  0x00007bc65ff87d2d in gui_changed (self=0x60605ccdf140, widget=<optimized out>, previous=0x7fffb5911754) at /home/kofa/darktable/src/iop/agx.c:2144
        g = 0x60605ebf5d80
        p = 0x60605e60c180
        post_curve_primaries_available = 1
#10 0x00007bc6ddc5fd88 in dt_iop_gui_changed (action=0x60605ccdf140, widget=widget@entry=0x60605ffa6650, data=data@entry=0x7fffb5911754) at /home/kofa/darktable/src/develop/imageop.c:3969
        module = 0x60605ccdf140
#11 0x00007bc6ddaec60f in _slider_value_change (w=0x60605ffa6650) at /home/kofa/darktable/src/bauhaus/bauhaus.c:3384
        prevf = 0.180000007
        i = <optimized out>
        s = <optimized out>
        f = <optimized out>
        previ = 1043878380
        prevs = 20972
        val = <optimized out>
        d = <optimized out>
        __inst = <optimized out>
        __t = <optimized out>
        __r = <optimized out>
#12 0x00007bc6ddaf08f6 in dt_bauhaus_slider_set_soft_range (widget=0x60605ffa6650, soft_min=<optimized out>, soft_max=1) at /home/kofa/darktable/src/bauhaus/bauhaus.c:1017
#13 0x00007bc65ff8820f in _create_basic_curve_controls_box (self=0x60605ccdf140, g=0x60605ebf5d80) at /home/kofa/darktable/src/iop/agx.c:2181
        box = 0x60605e576ab0
        slider = 0x60605ffa6650
        section = 0x7fffb5912180
        controls = 0x60605ebf5e90
        box = <optimized out>
        slider = <optimized out>
        section = <optimized out>
        controls = <optimized out>
#14 gui_init (self=0x60605ccdf140) at /home/kofa/darktable/src/iop/agx.c:2800
        g = 0x60605ebf5d80
        notebook_def = {name = 0x7bc6ddf1779d "tabs", process = 0x7bc6ddd0e430 <_action_process_tabs>, elements = 0x60605b434150, fallbacks = 0x0, no_widget = 0}
        settings_page = <optimized out>
        settings_section = <optimized out>
        curve_page_parent = <optimized out>
#15 0x00007bc6ddc57fa7 in dt_iop_gui_init (module=module@entry=0x60605ccdf140) at /home/kofa/darktable/src/develop/imageop.c:1283

[TurboGit]

Would darktable.gui->reset be a better option?

If you can fix this with this it would be better indeed as this is a common pattern already used in many places.

[kofa73]

The lock is actually added in imageop, I just have to use it. Thanks!

[kofa73]

This seems to break drawing the UI. I have to leave now, will continue in the evening/tomorrow.

[dterrahe]

Is there a particular reason why you call gui_update from gui_init? It normally gets called to update the gui when (new) params are available. That is not necessarily the case yet at gui_init time and it means you'll always call it at least twice before anything is shown.

[kofa73]

Is there a particular reason why you call gui_update from gui_init? It normally gets called to update the gui when (new) params are available. That is not necessarily the case yet at gui_init time and it means you'll always call it at least twice before anything is shown.

Right, thanks. I think that can be removed.
I'll have to add _update_curve_warnings to gui_update, so the indicators are properly redrawn when an image is loaded into the darkroom. I'll debug more tomorrow.

[kofa73]

Seems to be working fine for me with the latest updates. I'll publish an AppImage on pixls and ask others to test, as well.

[kofa73]

I've found some UI elements not properly initialised. In 1ef7e76, I've tried extracting all shared operations into functions, so gui_update and gui_changed can work independently without repeated code. I'm not aware of more issues, but let's see if people report anything.

[dterrahe]

I've found some UI elements not properly initialised.

Probably due to the if (darktable.gui->reset) return; in gui_changed. gui_update gets called with reset set.

I like the pattern of always calling gui_changed after gui_update (maybe should have enforced that in dt_iop_gui_update itself) because it means any path changing underlying data will go through the gui adjustment phase and that's the logical place to show/hide/adjust anything when needed. Of course people can still make the wrong choice between gui_init, reload_defaults, gui_update and gui_changed (reminds me ashift needs a good look sometime), but at least it would be a standard pattern rather than understanding different approaches in each module. Makes covering all the paths in (mainly manual) tests somewhat easier.

But if what you have now works for all cases I'm not going to argue over it.

[kofa73]

Before anyone misunderstands: I'm not arguing, I'm asking. I'm lost in the UI framework, never having worked with any UI at all, except for the most trivial 'hello world' demos, and even that not in C/GTK/bauhaus.

Probably due to the if (darktable.gui->reset) return; in gui_changed. gui_update gets called with reset set.

Without which (or adding a separate 'initialized' flag) we crash, because adjusting slider settings in gui_init triggers gui_changed, and we do not have all controls laid out yet.

There are adjustments I need to do when the UI is (re)initialised, and some of that overlaps with things I need to do when the GUI is changed.
In gui_update:

• set the checkboxes, as they are not managed by bauhaus
• set the offset, scale and default of the pivot_x slider so it gets shown with proper EV value, and its default so it can be double-clicked and reset to 0 EV (_update_pivot_slider_settings)
• draw the curve
• paint or hide the curve warnings
• hide / display some UI elements that depend on checkboxes ('reverse all' checkbox -> the primaries reversal sliders; auto-gamma -> gamma slider).

Then, when a picker callback is used:

• redraw the curve
• paint/hide the curve warnings
• set offset, scale, default of pivot_x

When the GUI changes:

• redraw the curve
• paint/hide the curve warnings
• set offset, scale, default of pivot_x

So, what I'm asking is: what would be the most robust way to make sure everything gets initialised/updated all the time it must be, and we don't run into crashes because of the not yet completely laid out UI? _update_pivot_slider_settings needs to be called if the one of the sliders black_exposure_picker/white_exposure_picker/security_factor is dragged, but also if the widget is null (when called from gui_update). I could replace the explicit call from gui_update and run it through gui_changed, but that special handling (burdening gui_changed with the special case to also handle the null-widget, when it's called from gui_update) feels wrong to me, but I cannot find any other way. See below:

void gui_changed(dt_iop_module_t *self,
                 GtkWidget *widget,
                 void *previous)
{
  dt_iop_agx_gui_data_t *g = self->gui_data;

  if (darktable.gui->reset) return;

  dt_iop_agx_params_t *p = self->params;

  if(widget == g->black_exposure_picker) {...}
  if(widget == g->white_exposure_picker) {...}
  if(widget == g->security_factor) {...}

  if(g && p->auto_gamma) {...}

// this is the redrawing part that we need for both gui_init and gui_change

  if(!widget)
  {
    // called from gui_init
    _update_pivot_slider_settings(g->basic_curve_controls.curve_pivot_x, p);
  }

  _update_redraw_dynamic_gui(self, g, p);
}

void gui_update(dt_iop_module_t *self)
{
  const dt_iop_agx_gui_data_t* const g = self->gui_data;
  const dt_iop_agx_params_t* const p = self->params;

  gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->auto_gamma),
                               p->auto_gamma);
  gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->disable_primaries_adjustments),
                               p->disable_primaries_adjustments);
  gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->completely_reverse_primaries),
                               p->completely_reverse_primaries);

  // don't call _update_pivot_slider_settings and _update_redraw_dynamic_gui
  gui_changed(self, NULL, NULL);
}

[kofa73]

Or I could remove that 'if':

void gui_changed(dt_iop_module_t *self,
                 GtkWidget *widget,
                 void *previous)
{
  dt_iop_agx_gui_data_t *g = self->gui_data;

  if (darktable.gui->reset) return;

  dt_iop_agx_params_t *p = self->params;

  if(widget == g->black_exposure_picker)
  {
    ...
    _update_pivot_x(old_black_ev, old_white_ev, self);
  }

  if(widget == g->white_exposure_picker)
  {
    ...
    _update_pivot_x(old_black_ev, old_white_ev, self);
  }

  if(widget == g->security_factor)
  {
    ...
    _update_pivot_x(old_black_ev, old_white_ev, self);

    darktable.gui->reset++;
    dt_bauhaus_slider_set(g->black_exposure_picker, p->range_black_relative_ev);
    dt_bauhaus_slider_set(g->white_exposure_picker, p->range_white_relative_ev);
    darktable.gui->reset--;
  }

  if(g && p->auto_gamma) {...}

  _update_pivot_slider_settings(self);

  _update_redraw_dynamic_gui(self, g, p);
}

And remove the call to _update_pivot_slider_settings from _update_pivot_x, then make sure everywhere where we call _adjust_relative_exposure_from_exposure_params, we also call _update_pivot_slider_settings:

• _adjust_relative_exposure_from_exposure_params ('camera icon' callback)
• color_picker_apply
• gui_changed (either because an exposure slider was moved, or we were called from gui_update).

I'm sorry I'm commenting so much.

[dterrahe]

Before anyone misunderstands: I'm not arguing, I'm asking.

No worries at all! I prefer more questions rather than less. The more people understand and think about this stuff, the better. And the more I explain, the more chance to realise opportunities for improvements. I'm just sorry I'm making you go through so much churn. When you're fed up, just start ignoring me; if you have no crashes and it works, your obligations are fulfilled.

and we do not have all controls laid out yet.

I didn't spend enough time analysing to understand this. Are you saying not all widgets have been created by gui_init yet? They should be, no? Configuring them later shouldn't lead to crashes? Looking at your original crash log, I don't understand why dt_bauhaus_slider_set_soft_range ends up calling _slider_value_change if you got there via dt_iop_gui_init. The latter should set reset and _slider_set_normalized checks that befor calling _slider_value_change. I have been messing with/simplifying stuff there lately, so maybe I messed up. But I don't get crashes here, so when did that start happening?

but also if the widget is null (when called from gui_update).

wait! what?

EDIT: oh, I guess the parameter to gui_changed that gui_update fills with NULL, ok.

In gui_update:

• set the checkboxes, as they are not managed by bauhaus

Everything after this is dependent on changes that the user can make (gui_changed) as well as changes from loading a new params (gui_update). So I'd put them in gui_changed and also call that from gui_update.

// this is the redrawing part that we need for both gui_init and gui_change
 
if(!widget)

Or I could remove that 'if':

This. That if just means you didnt do it when called from gui_update when you just said it needed to be called.

But as far as I can tell you still have

if (darktable.gui->reset) return;

which would completely prevent this from being executed when called from gui_init anyway.

I don't have time to do a deep dive into this (this week) so I just hope you can make it all work without crashing and don't worry too much about whether you are using the intended patterns correctly.

And also don't worry about optimisations in gui_changed where certain widget changes dont require layout/formatting changes; there seem to be few of those and the logic would just get unnecessarily complex.

[kofa73]

but also if the widget is null (when called from gui_update).

wait! what?

EDIT: oh, I guess the parameter to gui_changed that gui_update fills with NULL, ok.

No:
agx#_create_basic_curve_controls_box -> bauhaus#dt_bauhaus_slider_set_soft_range -> bauhaus#_slider_value_change -> imageop#dt_iop_gui_changed -> agx#gui_changed -> agx#_update_curve_warnings -> bauhaus#dt_bauhaus_widget_set_quad_paint(w = NULL)

_create_basic_curve_controls_box adjusts a slider (dt_bauhaus_slider_set_soft_range). As a result, _slider_value_change -> dt_iop_gui_changed -> agx#gui_changed -> agx#_update_curve_warnings -> bauhaus#dt_bauhaus_widget_set_quad_paint is called. And the problem is, in _update_curve_warnings, we are trying to paint something that is only created later:

agx#_create_basic_curve_controls_box will call bauhaus#dt_bauhaus_slider_set_soft_range:

  // curve_pivot_y_linear
  slider = dt_color_picker_new(self, DT_COLOR_PICKER_AREA | DT_COLOR_PICKER_DENOISE, dt_bauhaus_slider_from_params(section, "curve_pivot_y_linear_output"));
  controls->curve_pivot_y_linear = slider;
  dt_bauhaus_slider_set_format(slider, "%");
  dt_bauhaus_slider_set_digits(slider, 2);
  dt_bauhaus_slider_set_factor(slider, 100.f);
  dt_bauhaus_slider_set_soft_range(slider, 0.f, 1.f); <------ this line
... agx#_create_basic_curve_controls_box continues, and **later** sets up curve_toe_power:

  // curve_toe_power
  slider = dt_bauhaus_slider_from_params(section, "curve_toe_power");
  controls->curve_toe_power = slider;
  dt_bauhaus_slider_set_soft_range(slider, 1.f, 5.f);
  gtk_widget_set_tooltip_text(slider, _("contrast in shadows\n"
                                        "higher values keep the slope nearly constant for longer,\n"
                                        "at the cost of a more sudden drop near black"));

So, because of that slider value change, we ended up here: (-> bauhaus#_slider_value_change -> imageop#dt_iop_gui_changed -> agx#gui_changed -> agx#_update_curve_warnings):

static void _update_curve_warnings(dt_iop_module_t *self)
{
  const dt_iop_agx_gui_data_t *g = self->gui_data;
  const dt_iop_agx_params_t *p = self->params;

  if(!g) return;

  const gboolean warnings_enabled = dt_conf_get_bool("plugins/darkroom/agx/enable_curve_warnings");
  const tone_mapping_params_t params = _calculate_tone_mapping_params(p);

  dt_bauhaus_widget_set_quad_paint(g->basic_curve_controls.curve_toe_power,     <------ this line passes curve_toe_power, but it's still null
                                    params.need_convex_toe && warnings_enabled
                                    ? dtgtk_cairo_paint_warning : NULL, CPF_ACTIVE, NULL);
  dt_bauhaus_widget_set_quad_paint(g->basic_curve_controls.curve_shoulder_power,
                                    params.need_concave_shoulder && warnings_enabled
                                    ? dtgtk_cairo_paint_warning : NULL, CPF_ACTIVE, NULL);
}

And then:
-> bauhaus#dt_bauhaus_widget_set_quad_paint(w = NULL) CRASH

[dterrahe]

Does _create_basic_curve_controls_box get called from gui_init, where reset>0, or only later? That would also probably mean that shortcuts/actions don't get set up for those widgets?

On phone so can't research myself, sorry.

[kofa73]

Does _create_basic_curve_controls_box get called from gui_init, where reset>0

Yes.

1. imageop#dt_iop_gui_init
2. agx#gui_init
3. agx#_create_basic_curve_controls_box
4. bauhaus#dt_bauhaus_slider_set_soft_range
5. bauhaus#_slider_value_change
6. imageop#dt_iop_gui_changed
7. agx#gui_changed -- this was not protected with if (!darktable.gui->reset) return;
8. agx#_update_curve_warnings
9. bauhaus#dt_bauhaus_widget_set_quad_paint (with widget = null, because it the toe_power slider, next to which we want to paint the warning, is not created yet; it's created later in _create_basic_curve_controls_box)

[kofa73]

I don't know why / how we got there. From my reading of bauhaus.c, the setters eventually end up in _slider_set_normalized, and there _slider_value_change is not called if 'reset' is on.

static void _slider_set_normalized(dt_bauhaus_widget_t *w, float pos)
{
...
  if(!darktable.gui->reset)
  {
    d->is_changed = -1;
    _slider_value_change(w);
  }
}

Note that after the crash occurred, I restarted darktable, and was unable to crash it again. Also, no user has ever reported such a crash.
What makes me worried is that if the reset-check in _slider_set_normalized was ineffective, then the check in my gui_changed could be just as useless (whatever the root cause is).

Note: I updated the PR description with the original backtrace.

[dterrahe]

the setters eventually end up in _slider_set_normalized, and there _slider_value_change is not called if 'reset' is on.

Exactly. That's what mystified me.

was unable to crash it again

Ah. Since this was in init, I assumed it was 100% reproducible. Unless it happens only while creating multiple instances?

[kofa73]

I'll make one more attempt at reproducing this. It occurred when I loaded the square-cropped version of https://discuss.pixls.us/t/editing-intent-and-choosing-a-direction/54140/23. Immediately after, opening the same images caused no crash.

[kofa73]

I cannot reproduce it with master -- and that's not a surprise, given what we have seen.
The only issue in the image stack is a param warning for borders:

48.5024 [iop_validate_params] `borders' failed for type "float", field: aspect (-1.00000000 - [1.000000..3.000000] : default -1.000000)

The changes in this PR hardly count as a bugfix:

• if the issue is because something somewhere breaks the darktable.gui->reset protection, then it will also break it for the defensive check in gui_changed()
• there has been some clean-up (not calling gui_update from gui_init; calling gui_changed from gui_update; reduction of duplications), which could be beneficial (I still have one change that I have not pushed).

So, if it's not a bugfix, do we want to merge it before or after the release?

[TurboGit]

So, if it's not a bugfix, do we want to merge it before or after the release?

After the release.