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.

Sign up for GitHub

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

Jump to bottom

Update grain.c: grain blending using photographic-paper response. Fixes #11335 #1386

Merged
merged 4 commits into from Jan 17, 2017
Merged

Update grain.c: grain blending using photographic-paper response. Fixes #11335 #1386

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e3ecb7
Update grain.c: grain blending using photographic-paper characteristi…
arctee Dec 14, 2016
31a278b
Update grain.c
arctee Dec 16, 2016
b2e9fea
Update grain.c
arctee Dec 17, 2016
56b3077
remove `dt_fast_expf()`
arctee Jan 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
86 changes: 83 additions & 3 deletions src/iop/grain.c
View file
Open in desktop
Expand Up @@ -42,6 +42,11 @@

#define GRAIN_SCALE_FACTOR 213.2

#define GRAIN_LUT_SIZE 128
#define GRAIN_LUT_DELTA_MAX 2.0
#define GRAIN_LUT_DELTA_MIN 0.005
#define GRAIN_LUT_PAPER_GAMMA 1.0

#define CLIP(x) ((x < 0) ? 0.0 : (x > 1.0) ? 1.0 : x)
DT_MODULE_INTROSPECTION(1, dt_iop_grain_params_t)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to

DT_MODULE_INTROSPECTION(2, dt_iop_grain_params_t)

Copy link
Contributor Author

@arctee arctee Dec 16, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fast comments to the code. What does DT_MODULE_INTROSPECTION do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most importantly, you changed the size of params struct, so old history stacks, with params struct of old size are now broken.
Bumping version (1->2) at least prevents them from being broken silently.

As for DT_MODULE_INTROSPECTION(), it is just a magic word for introspection generator to create varous functions to be able to generate/modify the params from code easily.


Expand All @@ -59,20 +64,23 @@ typedef struct dt_iop_grain_params_t
_dt_iop_grain_channel_t channel;
float scale;
float strength;
float midtones_bias;
} dt_iop_grain_params_t;

typedef struct dt_iop_grain_gui_data_t
{
GtkBox *vbox;
GtkWidget *label1, *label2, *label3; // channel, scale, strength
GtkWidget *scale1, *scale2; // scale, strength
GtkWidget *scale1, *scale2, *scale3; // scale, strength, midtones_bias
} dt_iop_grain_gui_data_t;

typedef struct dt_iop_grain_data_t
{
_dt_iop_grain_channel_t channel;
float scale;
float strength;
float midtones_bias;
float grain_lut[GRAIN_LUT_SIZE * GRAIN_LUT_SIZE];
} dt_iop_grain_data_t;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add:

int legacy_params(dt_iop_module_t *self, const void *const old_params, const int old_version, void *new_params,
                  const int new_version)
{
  if(old_version == 1 && new_version == 2)
  {
    typedef struct dt_iop_grain_params_v1_t
    {
      _dt_iop_grain_channel_t channel;
      float scale;
      float strength;
    } dt_iop_grain_params_v1_t;

    dt_iop_grain_params_v1_t *o = old_params;
    dt_iop_grain_params_t *n = new_params;

    n->channel = o->channel;
    n->scale = o->scale;
    n->strength = o->strength;
    n->midtones_bias = 0.0; // ??? _must_ produce the same results as before this pr

    return 0;
  }
  return 1;
}


Expand Down Expand Up @@ -327,6 +335,58 @@ static double _simplex_2d_noise(double x, double y, uint32_t octaves, double per
return total;
}

float paper_resp(float exposure, float mb, float gp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static

{
float density;
float delta = GRAIN_LUT_DELTA_MAX * exp(mb * log(GRAIN_LUT_DELTA_MIN));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ man 3 log :

LOG(3)                                                                                                                  Linux Programmer's Manual                                                                                                                 LOG(3)

NAME
       log, logf, logl - natural logarithmic function

SYNOPSIS
       #include <math.h>

       double log(double x);

I'm not sure you intended it to actually use double versions

density = (1 + 2 * delta) / (1 + exp( (4 * gp * (0.5 - exposure)) / (1 + 2 * delta) )) - delta;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these suffix-less constants seem scary, too easy to unintentionally do int-based operation i'd say.
Append .0f

return density;
}

float paper_resp_inverse(float density, float mb, float gp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static

{
float exposure;
float delta = GRAIN_LUT_DELTA_MAX * exp(mb * log(GRAIN_LUT_DELTA_MIN));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for paper_resp()

exposure = -log((1 + 2 * delta) / (density + delta) - 1) * (1 + 2 * delta) / (4 * gp) + 0.5;
return exposure;
}

static void evaluate_grain_lut(float * grain_lut, const float mb)
{
for(int i = 0; i < GRAIN_LUT_SIZE; i++)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe

#ifdef _OPENMP
#pragma omp parallel for default(none) collapse(2)
#endif

{
for(int j = 0; j < GRAIN_LUT_SIZE; j++)
{
float gu = (double)i / (GRAIN_LUT_SIZE - 1) - 0.5;
float l = (double)j / (GRAIN_LUT_SIZE - 1);
grain_lut[j * GRAIN_LUT_SIZE + i]= paper_resp(gu + paper_resp_inverse(l, mb, GRAIN_LUT_PAPER_GAMMA), mb, GRAIN_LUT_PAPER_GAMMA) - l;
}
}
}

float dt_lut_lookup_2d_1c(const float *grain_lut, const float x, const float y)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static

{
const float _x = CLAMPS((x + 0.5) * (GRAIN_LUT_SIZE - 1), 0, GRAIN_LUT_SIZE - 1);
const float _y = CLAMPS(y * (GRAIN_LUT_SIZE - 1), 0, GRAIN_LUT_SIZE - 1);

const int _x0 = _x < GRAIN_LUT_SIZE - 2 ? _x : GRAIN_LUT_SIZE - 2;
const int _y0 = _y < GRAIN_LUT_SIZE - 2 ? _y : GRAIN_LUT_SIZE - 2;

const int _x1 = _x0 + 1;
const int _y1 = _y0 + 1;

const float x_diff = _x - _x0;
const float y_diff = _y - _y0;

const float l00 = grain_lut[_y0 * GRAIN_LUT_SIZE + _x0];
const float l01 = grain_lut[_y0 * GRAIN_LUT_SIZE + _x1];
const float l10 = grain_lut[_y1 * GRAIN_LUT_SIZE + _x0];
const float l11 = grain_lut[_y1 * GRAIN_LUT_SIZE + _x1];

const float xy0 = (1.0 - y_diff) * l00 + l10 * y_diff;
const float xy1 = (1.0 - y_diff) * l01 + l11 * y_diff;
return xy0 * (1.0f - x_diff) + xy1 * x_diff;
}

const char *name()
{
Expand Down Expand Up @@ -423,7 +483,7 @@ void process(struct dt_iop_module_t *self, dt_dev_pixelpipe_iop_t *piece, const
noise = _simplex_2d_noise(x + hash, y, octaves, 1.0, zoom);
}

out[0] = in[0] + ((100.0 * (noise * (strength))) * GRAIN_LIGHTNESS_STRENGTH_SCALE);
out[0] = in[0] + 100 * dt_lut_lookup_2d_1c(data->grain_lut, noise * strength * GRAIN_LIGHTNESS_STRENGTH_SCALE, in[0] / 100);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100 * dt_lut_lookup_2d_1c(..., in[0] / 100)

Move that 100 into curve, to avoid dividing, and then multiplying

out[1] = in[1];
out[2] = in[2];
out[3] = in[3];
Expand Down Expand Up @@ -452,6 +512,14 @@ static void strength_callback(GtkWidget *slider, gpointer user_data)
dt_dev_add_history_item(darktable.develop, self, TRUE);
}

static void midtones_bias_callback(GtkWidget *slider, gpointer user_data)
{
dt_iop_module_t *self = (dt_iop_module_t *)user_data;
if(self->dt->gui->reset) return;
dt_iop_grain_params_t *p = (dt_iop_grain_params_t *)self->params;
p->midtones_bias = dt_bauhaus_slider_get(slider) / 100;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ 100
here and everywhere, isn't that an integer division?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this does not seem nice, it is better to make p->midtones_bias contain the same scale as the slider, and if needed, let the commit_params() do the scaling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not an integer division. So I should add .0f.
Ok nice, I'll move the scaling of the parameter in commit_params().

dt_dev_add_history_item(darktable.develop, self, TRUE);
}

void commit_params(struct dt_iop_module_t *self, dt_iop_params_t *p1, dt_dev_pixelpipe_t *pipe,
dt_dev_pixelpipe_iop_t *piece)
Expand All @@ -462,6 +530,9 @@ void commit_params(struct dt_iop_module_t *self, dt_iop_params_t *p1, dt_dev_pix
d->channel = p->channel;
d->scale = p->scale;
d->strength = p->strength;
d->midtones_bias = p->midtones_bias;

evaluate_grain_lut(d->grain_lut, d->midtones_bias);
}

void init_pipe(struct dt_iop_module_t *self, dt_dev_pixelpipe_t *pipe, dt_dev_pixelpipe_iop_t *piece)
Expand All @@ -484,6 +555,7 @@ void gui_update(struct dt_iop_module_t *self)

dt_bauhaus_slider_set(g->scale1, p->scale * GRAIN_SCALE_FACTOR);
dt_bauhaus_slider_set(g->scale2, p->strength);
dt_bauhaus_slider_set(g->scale3, p->midtones_bias * 100.0);
}

void init(dt_iop_module_t *module)
Expand All @@ -496,7 +568,7 @@ void init(dt_iop_module_t *module)
module->params_size = sizeof(dt_iop_grain_params_t);
module->gui_data = NULL;
dt_iop_grain_params_t tmp
= (dt_iop_grain_params_t){ DT_GRAIN_CHANNEL_LIGHTNESS, 1600.0 / GRAIN_SCALE_FACTOR, 25.0 };
= (dt_iop_grain_params_t){ DT_GRAIN_CHANNEL_LIGHTNESS, 1600.0 / GRAIN_SCALE_FACTOR, 25.0, 0.0 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess that the new default behavior should be to produce the grain that makes use of this pr.
I.e. that default should not be 0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is definitely a good idea.

memcpy(module->params, &tmp, sizeof(dt_iop_grain_params_t));
memcpy(module->default_params, &tmp, sizeof(dt_iop_grain_params_t));
}
Expand Down Expand Up @@ -530,6 +602,14 @@ void gui_init(struct dt_iop_module_t *self)
gtk_box_pack_start(GTK_BOX(self->widget), GTK_WIDGET(g->scale2), TRUE, TRUE, 0);
gtk_widget_set_tooltip_text(g->scale2, _("the strength of applied grain"));
g_signal_connect(G_OBJECT(g->scale2), "value-changed", G_CALLBACK(strength_callback), self);

/* midtones bias */
g->scale3 = dt_bauhaus_slider_new_with_range(self, 0.0, 100.0, 1.0, p->midtones_bias * 100, 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, or you could make that slider have 0..1 range.
I'm not sure whether it is better or worse than percents though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current strength slider is in percent, so this one should behave the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, also.

dt_bauhaus_widget_set_label(g->scale3, NULL, _("midtones bias"));
dt_bauhaus_slider_set_format(g->scale3, "%.0f%%");
gtk_box_pack_start(GTK_BOX(self->widget), GTK_WIDGET(g->scale3), TRUE, TRUE, 0);
gtk_widget_set_tooltip_text(g->scale3, _("amount of midtones bias from the photographic paper response modeling"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a sentence about what low/high values do. Not critical, but who reads the manual … 😄

g_signal_connect(G_OBJECT(g->scale3), "value-changed", G_CALLBACK(midtones_bias_callback), self);
}

void gui_cleanup(struct dt_iop_module_t *self)
Expand Down