Spektrafilm analog film simulation module - #21967
Open
piratenpanda wants to merge 117 commits into
Open
Conversation
Contributor
Author
|
@kofa73 this should address all the issues your bot found |
Collaborator
|
Is each of your commits compilable? |
Contributor
Author
|
Yes, this should be true as I manually test each one before in my PKGBUILD and do local testing |
piratenpanda
force-pushed
the
spektrafilm
branch
2 times, most recently
from
August 23, 2026 15:42
e950b50 to
d648a87
Compare
Member
|
This is a new module, I'll squash all commits together anyway. |
TurboGit
added
feature: new
Contributor
Author
|
will do a bit of UI changes later today, then I'll collect presets |
Collaborator
|
Note that I did not ask the bots the compare that the processing algo matches Spektrafilm (they were told to look for generic maths issues like NaN). Let me know if you need that reviewed. |
Contributor
Author
|
Thanks, I already have a script for that myself |
…mpression boost option by @Arecsu, better organize print and film tab
… the scan box for positive films
Add a second, independent diffusion filter stage applied after print exposure and before print development in the pipeline, mirroring the existing film-stage diffusion with its own set of controls (on/off, family, strength, scale, warmth). Includes CPU, OpenCL, and GUI paths. The print diffusion controls live in the diffusion tab, below the film-stage diffusion controls. They are grayed out when scan-film mode is active (no print stage).
…al LUT A NaN or infinite input channel makes the chromaticity divide in the expose stage return NaN, and the triangle-to-square map propagates it, so the LUT coordinate is NaN. cubic_base_fraction() then ran (int)floor() on it. The conversion is undefined for NaN and yields INT_MIN in practice; safe_index() negates that into a large positive index and the caller multiplies it by the table stride, so the cubic gather reads far outside the table. Order the comparisons so a NaN falls into the low branch. Every finite and infinite coordinate keeps its previous result, and the behaviour matches the OpenCL twin, which builds the coordinate with fmin/fmax and maps NaN to the low bound. The scalar, NEON and double-precision callers all reach the table through this helper.
The header check tested that the first two dimensions agree and that the third matches the spectral shape, but not that any of them is positive, although the same expression bounds the identifier length and validates the magic, header version and element type. A negative side length passes. The element count multiplies the two equal dimensions, so the sign cancels and both the allocation and the read succeed at a plausible size, while the side length is stored signed and negative. Every cubic gather then multiplies its row index by that negative stride and addresses outside the buffer; the simulation's own copy of the table is allocated at the same wrapped size and its fill loop never runs, so the gather also reads uninitialised memory. A side length below 2 leaves the cubic base index at -1. Reject both, which surfaces as the existing "regenerate the data pack" error. SF_TC_N_MAX is far above the 192 the shipped table uses and keeps the element count clear of overflow.
…rashing build_lut3d() allocated six tables and dereferenced them unconditionally, writing through the first inside its sampling loop and passing all six to pchip3d_prepare(). The spectral exposure table and the gamut compression table were allocated without a check as well. At the highest quality one build_lut3d() call takes four arrays of steps^3 doubles plus two of (steps-1)^3, and a printing simulation makes two such calls, so a single build requests tens of megabytes; builds also run per pipe. On failure the result is a null dereference on the processing thread. Give build_lut3d() and build_cmax_table() a success return and fail the build through the path used for a missing illuminant, which frees the partial simulation and returns a message. The compression table needs this because its lookup dereferences the pointer unconditionally: a NULL there is meaningful only when compression is not oklch. The two float mirror tables keep their existing behaviour, since both consumers test them and fall back to the double originals.
…libration layer_max_raw is a running maximum over the exposure grid, seeded with a large negative sentinel. Three inputs leave a sub-layer's entry unusable. A sub-layer can reach zero legitimately: a zero fitted amplitude, or a CDF that clamps to zero across the whole grid, which the sept variant does from z <= -2.9007. Its share of the channel's density is then zero, and the calibration divides by that share, producing an infinity that the multiply turns into a NaN. A sub-layer can also keep the sentinel, when every sample is NaN. The profile loader validates array shape only, and a JSON null becomes NaN by design because the exporter writes null for non-finite values. The sentinel divides into the channel total and yields an infinite fraction, which propagates into that layer's dmin, dmax and particle count and into the grain sampler; since the total collapses to its own 1e-9 floor, it also scales every healthy sibling layer in the channel by about 1e9. The shipped profiles keep their per-layer maxima positive across every reachable morph corner. A hand-installed pack takes precedence over any downloaded one, so foreign profile data is an ordinary input path. Sanitise the maximum where it stops being a running value: anything not positive and finite becomes zero, which is what a layer carrying no density means, and the inverted comparison catches NaN. The two remaining divides then need only floors to stay finite. The OpenCL copy of the sampler carries the same floor.
Cancelling a data-pack download while the manifest request is in flight jumps to the cleanup label. That block clears the cancel flag and posts the completion idle but does not touch the fetch state, which stays RUNNING after the worker thread returns. Nothing recovers from that. sf_fetch_start() refuses to start while the state reads RUNNING, and the module's data row turns its download control into a cancel button for the same reason, so the only control that can install a pack becomes a no-op until darktable is restarted. The row also keeps re-arming its half-second poll timer, and the completion notice reports the message left by the last status update, so the user is told the download failed while fetching the manifest. The manifest is fetched with the string helper, which installs no progress callback, so a cancel cannot abort that transfer and is first observed on return from it. Record the cancellation the way the per-file loop does.
_ensure_sim() copied the process-global pack pointer, released the lock that guards it, and kept reading through that pointer: the mismatch check, and the simulation build, which reads the spectra table inside a parallel loop and the film defaults, illuminants, dichroics and locus throughout. The lock excludes other threads inside the critical section, not one that has left holding the pointer, and the pack carried no reference count. Any thread entering _ensure_sim() frees the pack when it resolves a different directory or sees a newer fetch generation. The module holds one pack at a time and reloads when the spectral table recorded in an edit changes; a completed download bumps the generation and reprocesses everything, so the full and preview pipes cross the generation change together. The fetch generation is sampled before the lock is taken, so two threads can hold different values. The colour picker calls _ensure_sim() from the GUI thread. The result is a use-after-free of a pack whose JSON tree has been released and whose spectra buffer is gone, read by a darkroom, thumbnail or export pipe mid-build. Give the pack an atomic reference count and take one before releasing the lock. Holding the lock across the whole build would also close the race but would serialise a multi-hundred-millisecond build across every concurrent pipe. The reference is dropped once the build returns; the simulation copies everything it needs and keeps no pointer into the pack. The other two readers of the global do their work inside the lock.
The two status strings live in gui_data. The preview pixelpipe thread compares them against the current verdict and overwrites them while holding only the per-piece lock; the GTK thread reads the same arrays from the idle callback, the half-second data poll and gui_update(). The per-piece lock does not order those accesses, so this is a data race. Both arrays are fixed at 256 bytes and the copy always terminates inside them, so a torn read yields garbled banner text rather than an out-of-bounds access. Take the module's GUI critical section around the compare-and-copy on the writing side and around a snapshot on the reading side, so the banner renders from a consistent pair of strings. The idle post stays outside the section: it needs only the change flag, and posting under the section would nest a main-loop call inside it.
The banner refresh is posted from the pixelpipe thread with a bare module pointer and no source id, and the callback dereferences that pointer to reach gui_data. gui_cleanup() removes the data-poll timeout but nothing removes this source. Leaving the darkroom cleans up each module and frees it in the same loop without draining pending idles, and develop teardown does the same. Leaving also takes the pipe mutexes before the free loop, and a running preview job holds its pipe mutex for the whole run, so a preview that changes the status during teardown posts its idle after the last main-loop dispatch and the module is freed before the source can fire. Keep the source id in gui_data and remove it in gui_cleanup, matching the poll timeout. The id is written from the posting pipe thread and from the GTK thread that runs or cancels it, so it lives under the same GUI critical section as the strings it announces. Not re-posting while one is pending collapses a burst of status changes into a single refresh, which is correct because the callback reads whatever the strings hold when it runs.
The ROI padding and the tiling overlap assumed a fixed 20 um inhibitor spread and a fixed 200 um tail whenever couplers were active. Both are persisted parameters: the spread reaches 60 um and the tail 400 um, and the module hands them to the simulation as absolutes, so the blur that runs is the one the sliders ask for. Any setting above the assumed pair convolves a kernel wider than the padded region. At a tile boundary that clamps the kernel against an internal edge rather than an image edge, which shows as a seam; at the frame edge it is the crop artifact the padding exists to prevent. The tail is the widest component, so the shortfall reaches twice the padded radius at the top of the range. Read the parameters, gating the tail on its weight the way process() gates the dispatch. The neighbouring halation and scatter terms clamp their per-film sigmas to the constants their padding assumes, so a pack cannot outrun the ROI; the coupler tail comes from the GUI instead, and reading the parameter is the equivalent guarantee.
The clump blur is a fixed pixel radius by design, but _max_halo_sigma() converted it through pixel_um, making the padded radius the real sigma times SF_GRAIN_REF_UM/pixel_um. That factor falls below 1 once the full-resolution long edge drops under film_format_mm*1000/SF_GRAIN_REF_UM pixels, which is 3600 px at the 36 mm default and 2400 px for half-frame, so the pad reaches 0.44 px against a dispatched 0.80 px on a small frame. The ratio is independent of the pipe scale, which divides out of both sides. The acutance recovery that follows the clump blur was not in the calculation. Take both in pixels, bounded by their unscaled radii, since the preview correction that scales them never exceeds 1. The tiling overlap comes from the same helper, so an under-computed halo clamps these blurs against an internal tile edge, which is a seam in a tiled export rather than a frame-edge effect. The per-sub-layer dye-cloud blur inside the sampler is a third pixel-domain blur in this stage and is not covered. Its sigma derives from a sub-layer's per-particle optical density, which exists only once the simulation is built, and the simulation is not available at ROI time. Capping the dispatched sigma with a constant would cap a blur the reference does not cap, changing rendered grain on every stock. Covering it needs the sigma plumbed out of the simulation.
The tiling callback claimed 2.5 buffers on the CPU and 4.0 on OpenCL with no fixed overhead. Both are under the real peak. On the CPU, input and output plus the three always-live temporaries -- two three-channel planes and a single-channel blur scratch -- are 3.75 before any spatial stage allocates. Halation is the widest of those stages, holding two single-channel and three three-channel buffers at once, which puts the peak at 6.5; the coupler tail reaches 5.25 and grain 4.5 at three sub-layers. The stages run in sequence, so the largest wins. On OpenCL the always-live set is five float4 buffers and two single-channel ones, a 7.5 baseline with input and output, and the grain stage adds an accumulator plus one buffer per sub-layer. Understating this lets the tiler choose a tile that then fails to allocate. process() runs once per tile, and its answer to a failed temporary allocation is to pass that tile through unchanged, so the export contains unprocessed rectangles inside an otherwise processed frame. Report the constant tables as overhead as well. They are uploaded once and do not scale with tile size, but the two PCHIP sets grow with the quality steps, from about 1 MB at draft to 17 MB at high. Exact-spectral quality builds no tables and reports zero steps, so only the spectral upsampling table counts there.
The grain table set is uploaded once per simulation and device, keyed on a pointer to the exported tables and the device id. Those fields were recorded immediately after the five uploads, before the check that any of them returned NULL. A single failed upload then satisfies the rebuild guard on every later call: the rebuild is skipped, the NULL check below fails again, and the module returns an allocation error for as long as that simulation and device pair lasts. Only a rebuild of the simulation clears it. Output stays correct, since the pipe treats the error as a reason to fall back to the CPU path, so the cost is the loss of GPU acceleration for this module well past the transient memory pressure that caused it. The whole set is under 30 kB, so the condition is rare. Record the key only when every upload succeeded. The rebuild branch releases what it holds before re-uploading, so a later attempt starts clean.
The output-scale picker reused the boost solver. That solver measures with the boost overridden and the gamut compressor forced off, which is correct for the boost, since it sits before the compressor and the knee's asymptote would flatten the measurement. The scale sits after both, so the number it produces describes a transfer the render does not perform. The probe also left out_scale at whatever the parameter held while the trial factor went into the boost slot, so the solved value scaled inversely with the value it was about to replace. Two picks on the same area alternate rather than converge: for a sampled lightness of 0.85 successive picks give 1.60403, 1.0, 1.60403; for 0.5 they give the clamped 4.0, 1.97015, 4.0. The picked colour comes from the module's input and does not change between picks, so the alternation is deterministic, and re-arming the picker over an unchanged area triggers it. With a boost other than 1.0 the delivered result is wrong by that factor as well. Give the scale its own probe, keeping the boost and the compressor as the render has them and overriding only out_scale. out_scale multiplies the finished RGB uniformly and RGB->XYZ is linear, so lightness goes as the cube root of the scale and a single probe at 1.0 solves it in closed form. Probing at 1.0 rather than at the current value makes the answer independent of the slider's position, which is the idempotence the picker documents.
git diff --check reports "new blank line at EOF", which fails the repository's whitespace check.
The film page carries three sections and does not fit a 16:10 FHD panel. One of its rows is redundant: the pipeline already treats "no print stage" and "scan the film" as the same state, and the two controls were coupled anyway, since the paper combobox was made insensitive whenever scanning was on. Give the paper combobox a "none (scan the film)" entry alongside "auto", and drive the scan_film parameter from it. The parameter itself is unchanged, so existing edits keep working and every consumer in the pipeline is untouched; only the widget goes away. Sensitivity now follows the film rather than the scan state. A positive or reversal stock has no print stage, so a paper is not a choice there and selecting one would put the image through a print it does not have; the combobox is insensitive for those films. For a negative it stays live even while scanning, because it is the control that turns scanning off again. The sensitivity update runs on every film change, not only when scan_film moves: selecting "none" on a negative and then switching to a positive stock leaves scan_film already set, and the combobox would otherwise stay live on a film with no print stage. gui_update selects the "none" entry whenever the parameter says scanning, which outranks any stored paper hash: that is the state the pipeline is in.
Build the sections on those two tabs as collapsible sections instead of plain headings. The expanded state persists per section, so a section opened once stays open. Grain, halation, diffusion and scanner keep plain headings: they are short enough already, and a disclosure arrow on a two-section tab costs more than it saves. Exposure stays fixed on both tabs. It is the first thing reached on either and hiding it behind a click costs more than the rows it saves. A fixed section gets the same container and the same reset button, only without the disclosure arrow, so both forms behave identically everywhere else. The section reset button moves into the section header and carries a pointer to the section's container. That also removes the reason the previous arrangement needed sentinels: sections were flat siblings, so the reset had to walk forward from its header until the next one. Every widget in a container now belongs to that section by construction, so the "sf_section" marker and the forward walk both go. The exempt marker and the toggles-last ordering are unchanged. _section_add points self->widget at the new container, so widgets built after it land inside the section without any change at the call sites, and returns the section's root widget. The page box is tracked separately, since self->widget no longer holds it once the first section on a page is open. dt_gui_new_collapsible_section packs with gtk_box_pack_end, which suits a module carrying one trailing section but reverses the order of several, so each expander is repacked at the start. Section labels repeat across the two tabs -- both have exposure and chemistry -- so the configuration names are qualified by tab. The helper that names the resolved paper in the paper combobox's "auto" entry wrote its label to list position 0, which the "none (scan the film)" entry now occupies: the list showed "auto (Kodak Portra Endura)" where the scan entry belonged, and the auto entry below kept its plain text. Record where the auto entry lands when the list is built and relabel there, asking the combobox for its length ahead of the insertion so the two stay in step.
git diff --check reports seven lines with trailing whitespace, which fails the repository's whitespace check. All seven are inside comments and only the trailing space differs; no text changes.
piratenpanda
force-pushed
the
spektrafilm
branch
from
August 24, 2026 04:33
83e0af5 to
0da246e
Compare
Six tabs is more than the module needs, and two of them were short. The diffusion tab held the same four controls twice, once for the film stage and once for the print stage, with nothing else on it. The film-stage diffusion filter sits in front of the taking lens and the print-stage one at the enlarger, so each belongs with the stage it acts on. Move them there as collapsible sections and drop the tab, leaving five. On the film tab diffusion goes above advanced, which holds quality and the adaptation switches and reads as the tail of the page. Both moves are page changes only: the widgets, their parameters, their order and their master toggles are unchanged, and gui_changed() keys off the toggle widgets rather than the page they sit on. Collapse what is left over on the grain and halation tabs: texture on one, and threshold on the other. Scatter and halation are the two stages the tab exists for and stay open; threshold holds the highlight boost and protection that feed both, which is a step removed from either.
_section_add() repoints self->widget at the new section's container, so a widget built before it goes into whichever container is still open. The two diffusion master switches were created ahead of their headings, which put them at the foot of the preceding section: DIR couplers on the film tab, preflash on the print tab. On the tab they came from there was no preceding section and the placement worked. Build them after the heading instead, as the first widget in the section they switch on. They keep the section-exempt marker, so resetting the section leaves the switch alone and only its parameters return to their defaults. Also restore the lead-in comment above the advanced heading, which stayed behind when diffusion moved above it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
after my git mistake, here's another PR. Sorry for the noise. Continuing from #21534