From f4c6cb3beb4016bb23ab3afec9a9189029e6f244 Mon Sep 17 00:00:00 2001 From: piratenpanda Date: Thu, 10 Sep 2026 07:07:50 +0200 Subject: [PATCH 1/6] spektrafilm: make install strictly fail-safe --- src/common/spektra_fetch.c | 80 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 4 deletions(-) diff --git a/src/common/spektra_fetch.c b/src/common/spektra_fetch.c index d1dd15c7c1f..9d759bcfdb0 100644 --- a/src/common/spektra_fetch.c +++ b/src/common/spektra_fetch.c @@ -98,6 +98,7 @@ static struct double progress; char message[256]; guint generation; /* bumped whenever an install changes what is on disk */ + guint finished_idle; /* pending _finished_idle source, 0 when none */ gboolean inited; } _sf = { .state = SF_FETCH_IDLE }; @@ -114,6 +115,7 @@ static void _set_status(const sf_fetch_state_t state, const double progress, const char *msg) { + if(!_sf.inited) return; g_mutex_lock(&_sf.lock); _sf.state = state; if(progress >= 0.0) _sf.progress = CLAMP(progress, 0.0, 1.0); @@ -123,6 +125,11 @@ static void _set_status(const sf_fetch_state_t state, static gboolean _cancelled(void) { + /* Not started, or already torn down: nothing is in flight to cancel, and the + mutex below may have been cleared. Every entry point that takes _sf.lock + tests this first -- the lock only exists between sf_fetch_init() and + sf_fetch_cleanup(), and callers on the GUI side outlive neither reliably. */ + if(!_sf.inited) return FALSE; g_mutex_lock(&_sf.lock); const gboolean c = _sf.cancel; g_mutex_unlock(&_sf.lock); @@ -150,6 +157,17 @@ void sf_fetch_cleanup(void) _sf.thread = NULL; g_mutex_unlock(&_sf.lock); if(t) g_thread_join(t); + + /* The worker's last act is to post _finished_idle, so joining guarantees it + has been posted, not that it has run. It calls sf_fetch_status(), so a + dispatch after the clear below would take a cleared mutex; drop it while + the lock is still valid. Zero means it already ran. */ + g_mutex_lock(&_sf.lock); + const guint idle = _sf.finished_idle; + _sf.finished_idle = 0; + g_mutex_unlock(&_sf.lock); + if(idle) g_source_remove(idle); + g_mutex_clear(&_sf.lock); _sf.inited = FALSE; } @@ -158,6 +176,12 @@ sf_fetch_state_t sf_fetch_status(char *msg, size_t msgsz, double *progress) { + if(!_sf.inited) + { + if(msg && msgsz) msg[0] = '\0'; + if(progress) *progress = 0.0; + return SF_FETCH_IDLE; + } g_mutex_lock(&_sf.lock); const sf_fetch_state_t s = _sf.state; if(msg && msgsz) g_strlcpy(msg, _sf.message, msgsz); @@ -168,6 +192,7 @@ sf_fetch_state_t sf_fetch_status(char *msg, void sf_fetch_cancel(void) { + if(!_sf.inited) return; g_mutex_lock(&_sf.lock); _sf.cancel = TRUE; g_mutex_unlock(&_sf.lock); @@ -878,6 +903,15 @@ typedef struct sf_worker_args_t static gboolean _finished_idle(gpointer user_data) { const gboolean ok = GPOINTER_TO_INT(user_data); + + /* Taken before anything else, and posted under the same lock: if the main + loop reaches this before the worker has recorded the source id, that store + is still in progress and this blocks until it lands, so the id can never be + left behind pointing at a source that has already run. */ + g_mutex_lock(&_sf.lock); + _sf.finished_idle = 0; + g_mutex_unlock(&_sf.lock); + char msg[256] = { 0 }; sf_fetch_status(msg, sizeof(msg), NULL); @@ -901,6 +935,7 @@ static gpointer _fetch_worker(gpointer data) gboolean success = FALSE; char *repo = NULL, *ref = NULL, *manifest_url = NULL, *manifest = NULL; char *base = NULL, *tmpdir = NULL, *destdir = NULL, *profdir = NULL; + char *olddir = NULL; GPtrArray *files = NULL; CURL *curl = NULL; @@ -1034,11 +1069,45 @@ static gpointer _fetch_worker(gpointer data) goto out; } - _rmdir_recursive(destdir); /* replacing an older copy of the same hash */ + /* Replacing an older copy of the same hash: move it aside rather than delete + it, so there is never a moment with no pack at this path. Deleting first + and then failing the rename -- out of space, a permission change, a handle + held open on the file the user is mid-render against -- leaves the user + with nothing, because the out: block below then removes the download too. + The name is dotted like .incoming, and lookups address packs by their exact + %08x name rather than scanning, so neither is ever mistaken for a pack. */ + if(g_file_test(destdir, G_FILE_TEST_IS_DIR)) + { + olddir = g_strdup_printf("%s%s.replaced-%08x", packs, G_DIR_SEPARATOR_S, got_hash); + _rmdir_recursive(olddir); + if(g_rename(destdir, olddir) != 0) + { + /* Cannot move it aside, so it could not be restored either. Stop here + and keep it: a pack that already renders is worth more than the one + being installed, and deleting it to make room would risk ending up + with neither. */ + dt_print(DT_DEBUG_ALWAYS, + "[spektrafilm] cannot move the installed pack aside at %s: %s", + destdir, strerror(errno)); + g_free(olddir); + olddir = NULL; + _set_status(SF_FETCH_FAILED, -1.0, + _("could not replace the installed pack -- the existing one " + "has been kept")); + goto out; + } + } + if(g_rename(tmpdir, destdir) != 0) { dt_print(DT_DEBUG_ALWAYS, "[spektrafilm] cannot install pack into %s: %s", destdir, strerror(errno)); + /* Put the working pack back before reporting the failure. */ + if(olddir && g_rename(olddir, destdir) == 0) + { + g_free(olddir); + olddir = NULL; + } _set_status(SF_FETCH_FAILED, -1.0, _("could not install the downloaded pack")); goto out; } @@ -1055,6 +1124,9 @@ static gpointer _fetch_worker(gpointer data) out: if(!success && tmpdir) _rmdir_recursive(tmpdir); + /* Reached with olddir set only once the new pack is in place, or after a + restore that itself failed; either way the copy it names is superseded. */ + if(olddir) _rmdir_recursive(olddir); if(curl) curl_easy_cleanup(curl); _files_free(files); g_free(manifest); @@ -1065,13 +1137,13 @@ static gpointer _fetch_worker(gpointer data) g_free(tmpdir); g_free(destdir); g_free(profdir); + g_free(olddir); g_mutex_lock(&_sf.lock); _sf.cancel = FALSE; - g_mutex_unlock(&_sf.lock); - if(darktable.gui) - g_idle_add(_finished_idle, GINT_TO_POINTER(success ? 1 : 0)); + _sf.finished_idle = g_idle_add(_finished_idle, GINT_TO_POINTER(success ? 1 : 0)); + g_mutex_unlock(&_sf.lock); return NULL; } From 4ef51fd7d325d537510c1212d9d624a7a6b1f4be Mon Sep 17 00:00:00 2001 From: piratenpanda Date: Thu, 10 Sep 2026 07:19:17 +0200 Subject: [PATCH 2/6] spektrafilm: fix small GPU vs CPU inconsistency --- src/iop/spektrafilm.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/src/iop/spektrafilm.c b/src/iop/spektrafilm.c index c8f9b15bbff..ee63423af4a 100644 --- a/src/iop/spektrafilm.c +++ b/src/iop/spektrafilm.c @@ -2418,8 +2418,15 @@ int process_cl(dt_iop_module_t *self, CLARG(amp[g3]), CLARG(c), CLARG(reset)); SF_CL_STEP("scatter tail accum"); } - const float ws_r = g->scatter_tail_weight[0], ws_g = g->scatter_tail_weight[1], - ws_b = g->scatter_tail_weight[2]; + /* CPU: sf_halation() takes w_s[] as the sim's double and blends with + (1.0 - w_s[c]) * core + w_s[c] * tail. Round the same double once here + rather than reading sf_sim_gpu_t's float mirror, so the constant the + kernel gets is the CPU's to the last bit. The blend itself still runs + in float on-device and in double on the CPU, so this narrows the + divergence to the per-pixel arithmetic instead of adding a second + rounding on top of it. */ + const float ws_r = (float)cl_sc_w[0], ws_g = (float)cl_sc_w[1], + ws_b = (float)cl_sc_w[2]; /* (1-s)*raw + s*scattered, matching sf_halation()'s CPU blend; `plane` doubles as both the pre-scatter `raw` input and the `out` write target -- safe since this is a purely per-pixel elementwise op. */ @@ -2462,9 +2469,18 @@ int process_cl(dt_iop_module_t *self, upstream's a_tot = halation_strength * halation_amount (no curve). */ const float h_eff = d->p.halation_amount; /* per-film halation strength (e.g. a strong-AH stock stays near-zero on - blue and much lower on red/green than a no-AH/redscale stock). */ - const float a_r = g->halation_strength[0] * h_eff, a_g = g->halation_strength[1] * h_eff, - a_b = g->halation_strength[2] * h_eff; + blue and much lower on red/green than a no-AH/redscale stock). + + CPU: a_tot[c] = halation_strength[c] * halation_amount, both doubles + (sf_halation()). Formed the same way here from cl_hal_strength -- the + sim's own double, as the sigma above is -- and rounded once, instead of + multiplying two independently-rounded floats. sf_halation() then keeps + a_tot in double through (raw + a_tot*blur) / (1 + a_tot) per pixel and + the kernel cannot, so the two still part company on that arithmetic; + this only stops them parting company on the constant as well. */ + const float a_r = (float)(cl_hal_strength[0] * (double)h_eff), + a_g = (float)(cl_hal_strength[1] * (double)h_eff), + a_b = (float)(cl_hal_strength[2] * (double)h_eff); err = dt_opencl_enqueue_kernel_2d_args(devid, gd->kernel_halation_apply, w, h, CLARG(plane), CLARG(acc), CLARG(w), CLARG(h), CLARG(a_r), CLARG(a_g), CLARG(a_b)); From f1fa23240bdb5a0f4dc80f774b8e26fc62f53c5e Mon Sep 17 00:00:00 2001 From: piratenpanda Date: Thu, 10 Sep 2026 09:10:18 +0200 Subject: [PATCH 3/6] make GPU and CPU a bit more identical --- data/kernels/spektrafilm.cl | 67 ++++++++++++++++++++++++++++--------- src/common/spektra_core.h | 62 ++++++++++++++++++++++++++++++++++ src/common/spektra_sim.c | 18 +++++----- 3 files changed, 123 insertions(+), 24 deletions(-) diff --git a/data/kernels/spektrafilm.cl b/data/kernels/spektrafilm.cl index 0ff3ef6a649..56b6807ba55 100644 --- a/data/kernels/spektrafilm.cl +++ b/data/kernels/spektrafilm.cl @@ -248,27 +248,64 @@ static float3 sf_pchip3d(__global const float *lut, __global const float *sx, /* ---- base-10 <-> base-2, matching the host's SF_LOG10F/SF_POW10F -------- */ /* spektra_sim.c does NOT call log10f()/exp10f(). It defines - SF_LOG10F(x) = log2f(x) * 0.3010299956639812f - SF_POW10F(x) = exp2f(x * 3.321928094887362f) - so calling log10()/exp10() here was not a more-or-less accurate version of - the same computation, it was a different computation: the host's scaling - multiply carries its own rounding that exp10()/log10() never perform. Use - the identical formulation so only the library ULP gap remains. - - That remaining gap is real but small: OpenCL specs log2/exp2 to <=3 ULP and - most GPUs implement both in hardware, while glibc's log2f/exp2f are - correctly rounded. If the integration test still shows a residue after - this, these two are the next candidates for the portable-polynomial - treatment sf_exp_neg already got -- log2 via exponent extraction plus a - mantissa polynomial, exp2 via sf_exp2i plus a fractional polynomial. */ + SF_LOG10F(x) = sf_log2f(x) * 0.3010299956639812f + SF_POW10F(x) = sf_exp2f(x * 3.321928094887362f) + so calling log10()/exp10() here would not be a more-or-less accurate + version of the same computation, it would be a different one: the host's + scaling multiply carries its own rounding that exp10()/log10() never + perform. + + log2()/exp2() are not usable either, for the reason that governs + sf_exp_neg: OpenCL specs both to <=3 ULP and most GPUs implement them in + hardware, while glibc rounds correctly. That gap lands in the film density + feeding sf_layer_particle, and sf_poisson's accept/reject loop below turns + it into whole-integer grain draws on isolated pixels. sf_exp2f/sf_log2f + below are the same portable polynomials as spektra_core.h's, built only + from correctly-rounded operations, so both sides agree bit-for-bit. Keep + them in lockstep with the host copies -- same constants, same order of + operations -- and do not substitute the library or native_ variants. */ +/* defined below, next to sf_exp_neg, which is the other user */ +static inline float sf_exp2i(int k); + +static inline float sf_exp2f(float x) +{ + const int k = (int)floor(x + 0.5f); + const float t = (x - (float)k) * 0.6931471824645996f; /* ln(2) */ + float p = 0.000198412700f; /* 1/5040 */ + p = p * t + 0.00138888892f; /* 1/720 */ + p = p * t + 0.00833333377f; /* 1/120 */ + p = p * t + 0.0416666679f; /* 1/24 */ + p = p * t + 0.166666672f; /* 1/6 */ + p = p * t + 0.5f; + p = p * t + 1.0f; + p = p * t + 1.0f; + return p * sf_exp2i(k); +} + +static inline float sf_log2f(float x) +{ + const uint xu = as_uint(x); + int e = (int)((xu >> 23) & 0xffu) - 127; + float m = as_float((xu & 0x007fffffu) | 0x3f800000u); + if(m > 1.41421356f) { m *= 0.5f; e += 1; } + const float s = (m - 1.0f) / (m + 1.0f); + const float s2 = s * s; + float p = 0.222222224f; /* 2/9 */ + p = p * s2 + 0.285714298f; /* 2/7 */ + p = p * s2 + 0.400000006f; /* 2/5 */ + p = p * s2 + 0.666666687f; /* 2/3 */ + p = p * s2 + 2.0f; + return (float)e + p * s * 1.4426950216293335f; /* log2(e) */ +} + static inline float sf_log10f(float x) { - return log2(x) * 0.3010299956639812f; + return sf_log2f(x) * 0.3010299956639812f; } static inline float sf_pow10f(float x) { - return exp2(x * 3.321928094887362f); + return sf_exp2f(x * 3.321928094887362f); } /* ---- [gc] Reinhard knee + OkLCh output gamut compression ---------------- */ diff --git a/src/common/spektra_core.h b/src/common/spektra_core.h index 048603a012b..d781060215d 100644 --- a/src/common/spektra_core.h +++ b/src/common/spektra_core.h @@ -349,6 +349,68 @@ SPEKTRA_INLINE float sf_exp_neg(float lam) return p * sf_exp2i(k); } +/* sf_exp2f / sf_log2f: 2^x and log2(x) built from +, -, *, / and the exact + floor/exponent manipulation above, for the reason sf_exp_neg is -- and + this pair is what actually reaches the grain sampler. SF_POW10F and + SF_LOG10F are defined in terms of these rather than the platform + exp2f/log2f, which OpenCL specifies only to <=3 ULP while glibc rounds + correctly: that slack lands in the film density arriving at + sf_layer_particle, and sf_poisson's accept/reject loop turns a one-ULP + density difference into a whole-integer grain count difference wherever a + partial product happens to sit near limit. The result is isolated pixels, + scattered evenly and uncorrelated with image structure, each off by a full + grain quantum rather than by a rounding. + + Every operation here is correctly rounded on both sides by IEEE-754 and by + the OpenCL spec, so the same x yields the same bits. The same caveat as + sf_exp_neg applies: none of these multiply-adds may be contracted into an + FMA on one side and not the other, which is what -ffp-contract=off on the + host and #pragma OPENCL FP_CONTRACT OFF in the kernel are for. + + Both are ~1 ULP against glibc over the domains this module uses, and are + not general-purpose replacements outside them: sf_exp2f assumes the result + stays normal, sf_log2f assumes x is positive and normal. SF_LOG10F floors + its argument at SF_LOG_EPS, which keeps it there. */ +SPEKTRA_INLINE float sf_exp2f(float x) +{ + /* x = k + r, k integer and |r| <= 0.5, so 2^x = 2^k * e^(r ln2) with the + exponential taken over |t| <= 0.347 by a degree-7 Taylor polynomial in + Horner form and 2^k injected exactly. */ + const int k = (int)floorf(x + 0.5f); + const float t = (x - (float)k) * 0.6931471824645996f; /* ln(2) */ + float p = 0.000198412700f; /* 1/5040 */ + p = p * t + 0.00138888892f; /* 1/720 */ + p = p * t + 0.00833333377f; /* 1/120 */ + p = p * t + 0.0416666679f; /* 1/24 */ + p = p * t + 0.166666672f; /* 1/6 */ + p = p * t + 0.5f; + p = p * t + 1.0f; + p = p * t + 1.0f; + return p * sf_exp2i(k); +} + +SPEKTRA_INLINE float sf_log2f(float x) +{ + /* Take the binary exponent off by hand, then fold the mantissa into + [1/sqrt2, sqrt2] so that s = (m-1)/(m+1) stays inside +-0.1716, where + log(m) = 2(s + s^3/3 + s^5/5 + s^7/7 + s^9/9) is good to well under an + ULP. Both steps of the fold are exact. */ + union { float f; uint32_t u; } v; + v.f = x; + int e = (int)((v.u >> 23) & 0xffu) - 127; + v.u = (v.u & 0x007fffffu) | 0x3f800000u; + float m = v.f; + if(m > 1.41421356f) { m *= 0.5f; e += 1; } + const float s = (m - 1.0f) / (m + 1.0f); + const float s2 = s * s; + float p = 0.222222224f; /* 2/9 */ + p = p * s2 + 0.285714298f; /* 2/7 */ + p = p * s2 + 0.400000006f; /* 2/5 */ + p = p * s2 + 0.666666687f; /* 2/3 */ + p = p * s2 + 2.0f; + return (float)e + p * s * 1.4426950216293335f; /* log2(e) */ +} + SPEKTRA_INLINE float sf_poisson(float lam, uint32_t seed) { diff --git a/src/common/spektra_sim.c b/src/common/spektra_sim.c index ff879c539b3..b421ba29dcf 100644 --- a/src/common/spektra_sim.c +++ b/src/common/spektra_sim.c @@ -58,10 +58,6 @@ #include "spektra_core.h" #include -/* ensure C99 math functions for SF_POW10F/SF_LOG10F (exp2f, log2f) */ -#if !defined(exp2f) && !defined(_GNU_SOURCE) -/* exp2f and log2f are C99; every compiler since GCC 4.x / Clang 3.x has them */ -#endif #include #include #include @@ -93,11 +89,15 @@ static inline void neon_mat3_mulv_batch(const float m[9], } #endif /* __ARM_NEON */ -/* Fast pow10 / log10 via exp2f/log2f. Using compiler builtins gives the - optimizer a better chance to inline/reduce them vs libm powf(10, x) - which internally computes exp2(x * log2(10)) with extra overhead. */ -#define SF_POW10F(x) __builtin_exp2f((x) * 3.321928094887362f) /* x * log2f(10) */ -#define SF_LOG10F(x) (__builtin_log2f(x) * 0.3010299956639812f) /* log2f(x) * log10(2) */ +/* pow10 / log10 through spektra_core.h's own exp2/log2, NOT the platform + exp2f/log2f. The kernel has to compute the same thing, and OpenCL specifies + exp2/log2 only to <=3 ULP where glibc rounds correctly, so a library call + here is a library call the GPU cannot match. sf_exp2f/sf_log2f are built + from correctly-rounded operations alone and agree bit-for-bit on both + sides; see their comment for why a ULP here is not a ULP by the time it + reaches the grain sampler. */ +#define SF_POW10F(x) sf_exp2f((x) * 3.321928094887362f) /* x * log2f(10) */ +#define SF_LOG10F(x) (sf_log2f(x) * 0.3010299956639812f) /* log2f(x) * log10(2) */ #define SF_TC_KNEE_T 0.0 /* [gc] InputGamutCompressSpec.knee */ #define SF_TC_KNEE_L 1.0 #define SF_TC_KNEE_P 6.0 From c1f51a6280177e95ce0f9fabee4d8dcb8bc68bc5 Mon Sep 17 00:00:00 2001 From: piratenpanda Date: Thu, 10 Sep 2026 19:38:46 +0200 Subject: [PATCH 4/6] spektrafilm: fix sublayer particle scale --- src/common/spektra_sim.c | 10 +++++++++- src/common/spektra_sim.h | 3 ++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/common/spektra_sim.c b/src/common/spektra_sim.c index b421ba29dcf..923b2b89d37 100644 --- a/src/common/spektra_sim.c +++ b/src/common/spektra_sim.c @@ -3447,8 +3447,16 @@ sf_sim_t *sf_sim_build(const sf_pack_t *pack, if(p->grain_uniformity_scale >= 0.0) for(int c = 0; c < 3; c++) s->grain_uniformity[c] = fmin(s->grain_uniformity[c] * p->grain_uniformity_scale, 0.999); + /* Sub-layer 0 is the coarsest and stays the reference at 1.0: this control + moves the FINER sub-layers relative to it, so it starts at i == 1. + Scaling the whole array instead is an exact no-op -- _sf_build_grain_layers + derives a_coarsest as sig^2*A48/peak[c], peak[c] is linear in + particle_scale[], and particle_area = a_coarsest * particle_scale[l], so a + common factor k cancels and every layer_npart comes out unchanged. The + only value that did anything was exactly 0, where the 1e-9 floors on peak + and particle_area take over and npart explodes, i.e. grain disappears. */ if(p->grain_particle_scale >= 0.0) - for(int i = 0; i < n_scale; i++) particle_scale[i] *= p->grain_particle_scale; + for(int i = 1; i < n_scale; i++) particle_scale[i] *= p->grain_particle_scale; _sf_build_grain_layers(s, film, p->grain_density_min, s->grain_uniformity, s->grain_rms, particle_scale, n_scale); } diff --git a/src/common/spektra_sim.h b/src/common/spektra_sim.h index d343a999afb..de770f79937 100644 --- a/src/common/spektra_sim.h +++ b/src/common/spektra_sim.h @@ -466,7 +466,8 @@ typedef struct sf_sim_params_t * and one slider can move all three channels together. */ double grain_rms_scale; /* -1 = from pack */ double grain_uniformity_scale;/* -1 = from pack */ - double grain_particle_scale; /* -1 = from pack; scales the sub-layer areas */ + double grain_particle_scale; /* -1 = from pack; scales the FINER sub-layers' + areas, sub-layer 0 (coarsest) stays 1.0 */ /* enlarger */ const char *enlarger_illuminant; /* "TH-KG3" */ From bfc8ece7935f4644668d957ba21ce1b7ab27a383 Mon Sep 17 00:00:00 2001 From: piratenpanda Date: Thu, 10 Sep 2026 19:42:07 +0200 Subject: [PATCH 5/6] spektrafilm: make density floor a scale --- src/common/spektra_sim.c | 11 ++++++++ src/common/spektra_sim.h | 8 +++++- src/iop/spektrafilm.c | 60 +++++++++++++++++++++++++++++++++------- 3 files changed, 68 insertions(+), 11 deletions(-) diff --git a/src/common/spektra_sim.c b/src/common/spektra_sim.c index 923b2b89d37..13b5a618520 100644 --- a/src/common/spektra_sim.c +++ b/src/common/spektra_sim.c @@ -1320,6 +1320,7 @@ void sf_sim_params_defaults(sf_sim_params_t *p) p->grain_rms_scale = -1.0; p->grain_uniformity_scale = -1.0; p->grain_particle_scale = -1.0; + p->grain_density_min_scale = -1.0; p->coupler_diffusion_um = -1.0; p->coupler_tail_um = -1.0; p->coupler_tail_weight = -1.0; @@ -3447,6 +3448,16 @@ sf_sim_t *sf_sim_build(const sf_pack_t *pack, if(p->grain_uniformity_scale >= 0.0) for(int c = 0; c < 3; c++) s->grain_uniformity[c] = fmin(s->grain_uniformity[c] * p->grain_uniformity_scale, 0.999); + /* A scale and not an absolute value, for the same reason rms and uniformity + are: the pack's floors are per channel -- kodak_vision3_500t is + 0.12/0.10/0.35 -- and one number replacing all three would flatten a + shape that carries real colour information. Scaling keeps the stock's own + ratios and still reaches any overall floor. Applied after the pack read, + so it lands on the film's own value; sf_pack_film_grain() leaves + p->grain_density_min alone for a stock it does not characterise, and the + scale then multiplies the caller's fallback instead. */ + if(p->grain_density_min_scale >= 0.0) + for(int c = 0; c < 3; c++) p->grain_density_min[c] *= p->grain_density_min_scale; /* Sub-layer 0 is the coarsest and stays the reference at 1.0: this control moves the FINER sub-layers relative to it, so it starts at i == 1. Scaling the whole array instead is an exact no-op -- _sf_build_grain_layers diff --git a/src/common/spektra_sim.h b/src/common/spektra_sim.h index de770f79937..d7e7e0a3ef0 100644 --- a/src/common/spektra_sim.h +++ b/src/common/spektra_sim.h @@ -453,7 +453,12 @@ typedef struct sf_sim_params_t double coupler_tail_weight; /* -1 = from pack */ /* grain reference floor — used for table ranges even when grain itself - * runs in the caller (reference: GrainParams.density_min) */ + * runs in the caller (reference: GrainParams.density_min). + * Resolved by sf_sim_build(), not supplied: the pack's per-film value is read + * over whatever is here and grain_density_min_scale below is then applied, so + * a caller sets the scale and reads this back rather than writing it. What is + * here on entry survives only for a stock the pack does not characterise, + * which is what the (0.03, 0.03, 0.03) default is for. */ double grain_density_min[3]; /* (0.03, 0.03, 0.03) */ /* [dt] Overrides for the per-stock grain statistics the pack supplies, which * are what actually decide how coarse the grain is: rms_granularity sets the @@ -468,6 +473,7 @@ typedef struct sf_sim_params_t double grain_uniformity_scale;/* -1 = from pack */ double grain_particle_scale; /* -1 = from pack; scales the FINER sub-layers' areas, sub-layer 0 (coarsest) stays 1.0 */ + double grain_density_min_scale;/* -1 = from pack */ /* enlarger */ const char *enlarger_illuminant; /* "TH-KG3" */ diff --git a/src/iop/spektrafilm.c b/src/iop/spektrafilm.c index ee63423af4a..33b0194e1bc 100644 --- a/src/iop/spektrafilm.c +++ b/src/iop/spektrafilm.c @@ -95,7 +95,7 @@ #include "common/spektra_core.h" #include "common/spektra_sim.h" -DT_MODULE_INTROSPECTION(1, dt_iop_spektrafilm_params_t) +DT_MODULE_INTROSPECTION(2, dt_iop_spektrafilm_params_t) /* Spatial-scale constants, micrometres on film unless noted (see the LUT module for the full rationale; these are shared with modify_roi_in() and @@ -259,11 +259,17 @@ typedef struct dt_iop_spektrafilm_params_t emulsions layer coarse crystals over fine ones; this moves the finer sub-layers relative to the coarsest, which stays the reference at 1. */ float grain_sublayer_scale; // $MIN: 0.0 $MAX: 2.0 $DEFAULT: 1.0 $DESCRIPTION: "sublayer particle scale" - /* GrainParams.density_min, absolute as the reference has it. The floor each - grain particle sits at, and the reason grain does not vanish entirely in - clear film. Shared with the enlarger and scan table ranges, so it is a - property of the emulsion, not of the drawing. */ - float grain_density_min; // $MIN: 0.0 $MAX: 0.2 $DEFAULT: 0.03 $DESCRIPTION: "density floor" + /* GrainParams.density_min, as a scale on the stock's own measured floor. The + floor each grain particle sits at, and the reason grain does not vanish + entirely in clear film. Shared with the enlarger and scan table ranges, so + it is a property of the emulsion, not of the drawing. + + A scale rather than the reference's absolute value, matching + grain_granularity and grain_uniformity above: the pack's floors are per + channel and three of them cannot be spelled with one slider, so an absolute + control would flatten kodak_vision3_500t's 0.12/0.10/0.35 the moment it was + touched. Scaling keeps the ratios and still reaches any overall floor. */ + float grain_density_min; // $MIN: 0.0 $MAX: 4.0 $DEFAULT: 1.0 $DESCRIPTION: "density floor" /* GrainParams.blur_dye_clouds_um, as a scale on the reference's own 2 um. Each developed crystal produces a small cloud of dye, not a hard dot; this is how far that cloud spreads, in real emulsion units, so it only @@ -678,6 +684,36 @@ int flags(void) return IOP_FLAGS_SUPPORTS_BLENDING | IOP_FLAGS_INCLUDE_IN_STYLES | IOP_FLAGS_ALLOW_TILING; } +/* v1 -> v2: grain_density_min changes meaning from an absolute density to a + scale on the stock's own measured floor. Same type and offset, so the struct + is unchanged and the copy is a straight one -- only the value has to move, + from v1's 0.03 default to a neutral 1.0. + + Every v1 edit rendered on the film's own floor whatever that field said: + sf_pack_film_grain() wrote the pack's value over it before anything read it. + So 1.0 does not approximate a v1 edit, it reproduces it exactly, and a v1 + slider position carries no information worth carrying forward. */ +int legacy_params(dt_iop_module_t *self, + const void *const old_params, + const int old_version, + void **new_params, + int32_t *new_params_size, + int *new_version) +{ + if(old_version != 1) return 1; + + dt_iop_spektrafilm_params_t *n = malloc(sizeof(dt_iop_spektrafilm_params_t)); + if(!n) return 1; + memcpy(n, old_params, sizeof(dt_iop_spektrafilm_params_t)); + n->grain_density_min = 1.0f; + + *new_params = n; + *new_params_size = sizeof(dt_iop_spektrafilm_params_t); + *new_version = 2; + return 0; +} + + dt_iop_colorspace_type_t default_colorspace(dt_iop_module_t *self, dt_dev_pixelpipe_t *p, dt_dev_pixelpipe_iop_t *pi) @@ -1306,7 +1342,7 @@ static sf_sim_t *_ensure_sim(dt_iop_spektrafilm_data_t *d, sp.grain_rms_scale = p->grain_granularity; sp.grain_uniformity_scale = p->grain_uniformity; sp.grain_particle_scale = p->grain_sublayer_scale; - for(int c = 0; c < 3; c++) sp.grain_density_min[c] = p->grain_density_min; + sp.grain_density_min_scale = p->grain_density_min; sp.coupler_diffusion_um = p->couplers_diffusion_um; sp.coupler_tail_um = p->couplers_tail_um; sp.coupler_tail_weight = p->couplers_tail_weight; @@ -3639,7 +3675,7 @@ static void _preset_defaults(dt_iop_spektrafilm_params_t *p) p->grain_granularity = 1.0f; p->grain_uniformity = 1.0f; p->grain_sublayer_scale = 1.0f; - p->grain_density_min = 0.03f; + p->grain_density_min = 1.0f; p->grain_dye_cloud = 1.0f; p->film_format_mm = 36.0f; p->output_luminance_boost = 1.0f; @@ -5083,12 +5119,16 @@ void gui_init(dt_iop_module_t *self) "0 only the coarsest layer is left. no effect on single-layer stocks.")); g->grain_density_min = dt_bauhaus_slider_from_params(self, "grain_density_min"); - dt_bauhaus_slider_set_digits(g->grain_density_min, 3); + /* same shape as granularity above: full range is 0-4, but everything useful + sits close to the stock's own value */ + dt_bauhaus_slider_set_soft_range(g->grain_density_min, 0.25f, 2.5f); gtk_widget_set_tooltip_text( g->grain_density_min, _("the density each crystal sits at even where the film received no\n" "light, which is why grain does not disappear entirely in clear\n" - "areas. typical stocks measure between 0.03 and 0.06.")); + "areas. scales the loaded stock's own measured floor, which is per\n" + "channel, so the balance between channels is kept. typical stocks\n" + "measure between 0.03 and 0.06, cine stocks considerably higher.")); _section_add(self, C_("section", "texture"), "plugins/darkroom/spektrafilm/expand_grain_texture"); From 84bd69c54e32c3febcd0765f1f0c25ba20ac60ca Mon Sep 17 00:00:00 2001 From: piratenpanda Date: Fri, 11 Sep 2026 05:42:52 +0200 Subject: [PATCH 6/6] spektrafilm: keep the print gate on the print diffusion sliders --- src/iop/spektrafilm.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/iop/spektrafilm.c b/src/iop/spektrafilm.c index 33b0194e1bc..395b4819fef 100644 --- a/src/iop/spektrafilm.c +++ b/src/iop/spektrafilm.c @@ -3388,7 +3388,16 @@ static void _toggle_sensitivity(dt_iop_spektrafilm_gui_data_t *g, gtk_widget_set_sensitive(g->diffusion_scale, dif); gtk_widget_set_sensitive(g->diffusion_warmth, dif); - const gboolean pdif = p->print_diffusion_on; + /* scan_film as well as the toggle, because _update_print_sensitivity() gates + these same four on `printing && print_diffusion_on` and this function is + reached without it: gui_changed() pairs the two only for the + print_diffusion_on widget, so a halation, grain, diffusion or boost_ev + change ran this alone and re-enabled all four with the print gate dropped. + print_diffusion_on stays TRUE while scanning -- _update_print_sensitivity() + blanks its tick but deliberately leaves the param -- so the sliders came + back live on a workflow that has no print stage at all. Same hazard the + development sliders are protected from at the end of this function. */ + const gboolean pdif = p->print_diffusion_on && !p->scan_film; gtk_widget_set_sensitive(g->print_diffusion_filter_family, pdif); gtk_widget_set_sensitive(g->print_diffusion_strength, pdif); gtk_widget_set_sensitive(g->print_diffusion_scale, pdif);