-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Update grain.c: grain blending using photographic-paper response. Fixes #11335 #1386
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
Conversation
…c curves A sigmoidal function is used to model a the exposure-density characteristic curve of photographic paper, and the lightness channel is used to modulate the grain accordingly to the characteristic curve. A LUT is used for computational efficiency. The main effect is a fall off of the grain in shadows and highlights accompanied by the skewing of grain distribution (for further info see the discussion on discuss.pixls.us: https://discuss.pixls.us/t/lets-improve-grain/2709/22). A "midtones bias" slider has been added, at 0% the added grain is indistinguishable from the old implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks promising, not too intrusive, but changes are needed.
#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) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
} 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; | ||
|
There was a problem hiding this comment.
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;
}
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static
return density; | ||
} | ||
|
||
float paper_resp_inverse(float density, float mb, float gp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static
} | ||
} | ||
|
||
float dt_lut_lookup_2d_1c(const float *grain_lut, const float x, const float y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static
|
||
static void evaluate_grain_lut(float * grain_lut, const float mb) | ||
{ | ||
for(int i = 0; i < GRAIN_LUT_SIZE; i++) |
There was a problem hiding this comment.
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
@@ -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); |
There was a problem hiding this comment.
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
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
.
@@ -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 }; |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
float paper_resp(float exposure, float mb, float gp) | ||
{ | ||
float density; | ||
float delta = GRAIN_LUT_DELTA_MAX * exp(mb * log(GRAIN_LUT_DELTA_MIN)); |
There was a problem hiding this comment.
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
{ | ||
float density; | ||
float delta = GRAIN_LUT_DELTA_MAX * exp(mb * log(GRAIN_LUT_DELTA_MIN)); | ||
density = (1 + 2 * delta) / (1 + exp( (4 * gp * (0.5 - exposure)) / (1 + 2 * delta) )) - delta; |
There was a problem hiding this comment.
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
float paper_resp_inverse(float density, float mb, float gp) | ||
{ | ||
float exposure; | ||
float delta = GRAIN_LUT_DELTA_MAX * exp(mb * log(GRAIN_LUT_DELTA_MIN)); |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be going in the right direction.
static float paper_resp(float exposure, float mb, float gp) | ||
{ | ||
float density; | ||
float delta = GRAIN_LUT_DELTA_MAX * exp(mb * logf(GRAIN_LUT_DELTA_MIN)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same applies to exp()
, it should probably be expf()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's worth to use dt_fast_expf()
everywhere?
static float paper_resp_inverse(float density, float mb, float gp) | ||
{ | ||
float exposure; | ||
float delta = GRAIN_LUT_DELTA_MAX * exp(mb * logf(GRAIN_LUT_DELTA_MIN)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same applies to exp(), it should probably be expf()
@@ -530,6 +628,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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
There was a problem hiding this comment.
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.
static float paper_resp(float exposure, float mb, float gp) | ||
{ | ||
float density; | ||
float delta = GRAIN_LUT_DELTA_MAX * exp(mb * logf(GRAIN_LUT_DELTA_MIN)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's worth to use dt_fast_expf()
everywhere?
@@ -423,7 +509,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.0 * dt_lut_lookup_2d_1c(data->grain_lut, (noise * strength) * GRAIN_LIGHTNESS_STRENGTH_SCALE, in[0] / 100.0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you push the 100.0 *
into the LUT? It's not much, but would save a few cycles. Or would that give different results? I haven't really tried to understand the logic yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, no problem, it can be moved into the LUT.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other GUI callbacks do the scaling of the slider values here already.
@@ -462,6 +556,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 / 100.0f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of scaling here you should do it in midtones_bias_callback()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.
What about introspection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about it? The rest of that IOP scales in the other place already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If for consistency with the old code in this IOP, then yes, probably better to handle it in callbacks.
But i think in new code it should be done in commit_params()
if needed at all.
Edit: because displaying parameter with one range (0..100%
) in gui, and with another rande in params and introspection (0..1
) will probably be confusing for someone who tries to use introspection.
@@ -530,6 +628,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); |
There was a problem hiding this comment.
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.
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")); |
There was a problem hiding this comment.
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 … 😄
I moved the scaling of midtone_bias into the LUT functions, now the parameter is always a percentage. I added also `dt_fast_expf()`.
I noticed some luminance shifting using `dt_fast_expf()`, changing back to `expf()`.
What remains to be done in your opinion? |
I didn't try it yet, but the code looks ok to me. |
I've tested it but I cannot see a difference when using the midtone-bias slider. I've looked at the image closely in different location. It seems that the image is not changed. The histogram moves a bit, so I suppose that there is some changes done to the image. Where should I looked? How this slider is working? Thanks. |
Look in black and white parts of the image. The grain should get less there. |
Indeed! Thanks Tobias now I see it. Not a strong effect but nice. |
Thank you, I like the result much more than the old grain. |
Thank you all for helping and reviewing the code. |
A sigmoidal function is used to model a the exposure-density characteristic curve of photographic paper, and the lightness channel is used to modulate the grain accordingly to the characteristic curve. A LUT is used for computational efficiency. The main effect is a fall off of the grain in shadows and highlights accompanied by the skewing of grain distribution (for further info see the discussion on discuss.pixls.us: https://discuss.pixls.us/t/lets-improve-grain/2709/22). A "midtones bias" slider has been added, at 0% the added grain is indistinguishable from the old implementation.