Repository navigation
agx, sigmoid: follow the output's peak for HDR export - #22669
Conversation
0fb7547 to
b746e1c
Compare
|
Please let me have the bots check this out. Working with inter-module parameter passing between white balance and color calibration has burnt me badly, so I'd like to have them check out everything is safe here. |
|
@nicknezis "I have not merged master into the topic branch." - this is false, your last commit is exactly such a merge. You must never do that; you should rebase your code on master, not merge master onto your branch. |
Sorry about that, you’re right: the last commit was a merge of master. I’ve rebased the branch onto current master and force-pushed, so it’s now four linear commits with no merge, each of which builds on its own. The final code is unchanged. Happy to address whatever the bots turn up. |
9e68897 to
466f878
Compare
|
There is also code style, we always use *d for the iop_module data and *p for parameters. |
Wrong output1. AgX loses its shoulder above about 600 nits, and its look moves the peak
In SDR, the last half stop before scene white adds 0.01 EV of output. At 1000 nits it adds 0.4 EV. Shadows and middle gray do not change. The curve warnings ( The shoulder needs a design for a white above 1.0, and the look should work relative to the effective white. 2. filmic raises midtone contrast and crushes shadows to reach the HDR white
Shadows at -2 EV are 5 times darker, and +2 EV is 2.3 times brighter. The graph and the contrast-clamp warning do not show this (issue 6). A hand-set 493% target white does the same, so this follows from filmic's design. It contradicts the option's promise that one edit renders correctly for SDR and HDR, though. To keep the midtones, the scene range would have to grow with the headroom. If filmic cannot do that, leave it out of the option or document the contrast change. 3. filmic color science v3 to v5 clips the raised white at 203 nits
With these versions, turning the option on is worse than leaving it off. The shadows are crushed as in issue 2, and the output clips at SDR white by +2 EV. I checked neutral gray only. With chroma preservation, the clip applies to the norm before the ratios are reapplied, so the exact ceiling for colored pixels can differ. A migrated edit with spline v1 has a related problem. Spline v1 takes the white linearly ( Either make the option insensitive and inactive for color science below v6 and for spline v1, or carry the higher white through those paths. 4. sigmoid's curve fit overflows with extreme settings at a high PQ peak
These settings are extreme. Contrast and target black are far outside their soft ranges (0.7 to 3 and 0 to 1%). The overflow is also older than this branch. With skew 1.0, the same settings fail at 100% white with the option off, on the base commit too. The option still adds failures. The settings in the table work at the 1600% slider maximum and fail only at a peak that the slider cannot reach. Keep the fit finite over the whole peak range the preference allows, or limit the combination. Wrong feedback while editing5. The AgX pivot picker evaluates the slider's curveTo keep the picked patch's brightness, For example, set the slider to 50%, turn the option on, use SDR output, and pick a neutral patch at +2 EV. The patch's output drops from 0.610 to 0.463 (-0.40 EV). With the slider at 100% and a 1000-nit pipe, it drops from 0.680 to 0.610 (-0.16 EV). I computed these values with the module's curve code, not in the GUI. The picker should evaluate the curve that the pipe renders, through 6. filmic's graph, labels and contrast warning use the slider valueThe graph builds its spline from The attached screenshots use the PQ Rec2020 display profile on an SDR X server, so the center view shows PQ code values. Turning the option on changed 419,500 pixels in the center image. The 270x155 graph area stayed pixel-identical. Off: On: 7. AgX's graph leaves its axes on an HDR display and does not refresh when the headroom changesThe graph uses the headroom of the full pipe ( 8. A changed PQ peak preference does not reach open darkroom pipescolorout reads the preference only when it commits ( In a GTK3/X11 test, I changed the peak from 2001 to 3001 nits and closed the preferences. An 850x500 area of the center view stayed pixel-identical. After I left and re-entered the darkroom, 276,949 pixels in that area changed. To fix this, compare the peak on Documentation9. The tooltips and the release note promise more than AgX and filmic deliverThe three tooltips ( Files and how to reproduceAttached files:
|
466f878 to
6741a46
Compare
Plan for kofa73's review of PR #22669 (output headroom)The core change: AgX expands its SDR render instead of fitting the curve to the peakItems 1, 5, 7 and 9 all come from the same cause. At the moment we feed the HDR Proposal. When "target white from output" is on, AgX always builds its
Expected neutral-ramp output at a 1000-nit PQ peak, computed from kofa73's
The proposed curve keeps the SDR roll-off: the last half stop adds 0.035 EV The alternative is a new AgX shoulder designed for whites above 1.0, which Item by item
Commit series after the rework (rebased on master, each one building)
How I'll check it
Decisions for Nick and @kofa73
|
Thank you. This coding style issue should be corrected now. |
|
I don't use HDR. If you don't break my simple SDR renders, I'm OK. However, AFAIK, Blender 5 did HDR work for AgX. Maybe you should look into that. I'll run another round later, but I also have stuff to do for 5.8, and I must prioritise that. |
Revised plan based on Blender 5's AgX.The core change: follow Blender 5's AgX HDR designItems 1, 5, 7 and 9 all come from the same cause. At the moment we feed the HDR What Blender 5 doesBlender 5 ships an "AgX - HDR 1000 nits" view for Rec.2100 PQ and HLG
Proposal: port that design into darktable's AgXWith "target white from output" on and headroom
The curve itself never sees a white above 1.0, so neither the fallback Neutral ramp in nits, from a Python model of darktable's AgX curve.
The two approaches agree up to middle gray and both stop at the peak. The The fallback, if kofa73 prefers it, is a new AgX shoulder designed for Item by item
Commit series after the rework (rebased on master, each one building)
How I'll check it
Decisions
|
6741a46 to
e57610d
Compare
e57610d to
fc5d53c
Compare
nicknezis
changed the title
TurboGit
added
priority: medium
There was a problem hiding this comment.
🟡 Changes recommended
Numerical overflow, export-setting isolation and persistence, and HDR graph consistency remain unresolved.
5 open issues
What changed in this PR
Makes AgX and sigmoid adapt to output brightness so one edit can serve SDR and HDR exports.
Changes:
- Adds per-pipe output headroom and a configurable PQ peak.
- Adapts tone mapping while preserving legacy edits.
- Updates HDR controls, graph rendering, and release notes.
| File | Description |
|---|---|
| src/libs/export.c | Adds PQ peak control. |
| src/iop/sigmoid.c | Scales target white for HDR. |
| src/iop/agx.c | Adds HDR mapping and GUI support. |
| src/develop/pixelpipe_hb.h | Declares output headroom. |
| src/develop/pixelpipe_hb.c | Initializes output headroom. |
| src/develop/pixelpipe_cache.c | Includes headroom in cache identity. |
| src/common/iop_profile.c | Derives headroom from output settings. |
| RELEASE_NOTES.md | Documents adaptive HDR output. |
| data/kernels/agx.cl | Adds OpenCL HDR mapping. |
| data/darktableconfig.xml.in | Registers PQ peak preference. |
Review effort: Balanced
ac5161a to
36824ab
Compare
|
Ok, i am definitely out of reviewing here. |
TurboGit
left a comment
TurboGit
left a comment
There was a problem hiding this comment.
This looks ok by me. I have also tested the integration tests (thanks). Looks good too. I will regenerate the expected output to ensure a closer match when run on my machine.
I'll sleep on this and will merge tomorrow if nothing wrong come to mind.
|
I'll get the bots to have another look, too |
|
Codex (one could say it's an AgX bug, not HDR related - setting pivot output to 100% should probably be prevented): Findings
Keep the fit finite when the pivot approaches or equals target white, or provide a valid limiting curve for that case. A constant shoulder-power cap alone is insufficient. Both CPU and OpenCL processing receive the affected host-computed parameters; GPU behavior was not exercised. |
|
Some of the items here are marked with "commit message documents this", but the described behaviours still sound weird to me. Claude:
|
Thanks for running the bots over it. Taking the findings in order: 1. Black highlights with pivot target output at 100% (f77da38) Reproduced, and the cause is as described: with the pivot at target white, the rise left for the shoulder is clamped to
Blocking pivot output at 100% would also avoid it, but that changes the slider range for everyone, so I've left it as your call. 2. Preset upgrade reading past a short blob (aef5b12) The v8 to v9 upgrade now checks that the blob holds the nine leading ints before copying it, and drops the preset otherwise (tested with a truncated v8 preset in data.db). The older upgrade steps and 3. Presets silently changing the PQ peak (aef5b12) Fixed. A preset now stores the peak only when its profile is PQ or "image settings", and 0 otherwise, which leaves the current peak alone when applied. Presets from before this PR are upgraded with 0 as well, so they no longer reset it to 1000. Multi-preset export puts the peak back afterwards. 4. Pivot picker depending on the display (705fe68) Fixed. Pivot target output is an SDR value, so the picker now reads it from the SDR curve, as on master, whatever display it is used on. 5. The "follow output peak" toggle only working one way That is by design, following @TurboGit's suggestion in #22661: following the output is a hidden param, on for new edits, and the toggle only exists so that edits made before this PR can opt in. Once it's on, the module behaves like a new instance. If you'd rather it stayed visible so it can be switched back, that's a small change and I'm happy to make it. What I didn't verify: the OpenCL path (no GPU here) and the GUI (picker, presets dialog) and multi-preset export, beyond building and the darktable-cli and data.db checks above. |
colorout maps 1.0 to the 203 nit reference white for PQ and HLG, so a tone mapper needs to know how far above that the output can go before it can use an HDR display's range. Nothing in the pipe says so today. The pipe now carries an output headroom, the peak of its output relative to 203 nits, set together with the output profile: 1 for SDR, 1000/203 for HLG (the nominal display colorout's OOTF assumes), and for PQ the new "PQ peak" setting of the export module (default 1000 nits) divided by 203. The setting is only shown when the export profile is PQ or taken from the image. It is part of the pipe cache hash, since the PQ peak can change without the profile changing, and a darkroom on a PQ display reprocesses when it changes there. Related: darktable-org#22664
colorout maps 1.0 to the 203 nit reference white for PQ and HLG, so AgX's target white, which stops at 100%, caps an HDR export at reference white, and one history cannot render correctly for both SDR and HDR. AgX now follows the output's peak, using the pipe's output headroom H and the HDR design of Blender 5's AgX (AgXHLG.py in github.com/EaryChow/AgX_LUT_Gen): - the curve's output, target white included, is relative to the output's peak; the shoulder power is multiplied by H^log10(2), so the shoulder stays smooth at the higher peak - after linearizing, a second sigmoid in log2 space moves the pivot output down by H, so shadows and the pivot keep their SDR brightness and the highlights extend up to the peak - per-channel darkening saturates highlights; a new "HDR purity" slider (default 50%, as in Blender) sets how much of that hue and saturation shift is kept In SDR (H = 1) nothing changes. The curve itself never sees a white above 1.0, so the regular shoulder always reaches the peak and the look's slope, lift and power keep their fixed point there. New instances and presets follow the output. Existing edits are migrated with it off, so they render exactly as before, and a "follow output peak" toggle, shown only while it is off, opts them in. The headroom is only known once colorout has committed, so the curve is rebuilt at process time. The shoulder power is capped at its slider maximum, since a higher one makes the curve fit overflow for a high pivot target at the highest PQ peaks. On DT_SIGNAL_DEVELOP_UI_PIPE_FINISHED the GUI reads the darkroom full pipe's headroom, with a trylock on busy_mutex, so that cache hits are covered too. The graph, its markers, the curve warnings and the pivot picker then show what the center view renders and follow display profile changes. In HDR the graph's top is the peak and its guide labels are relative to reference white. Fixes darktable-org#22664
Like AgX, sigmoid's target white sets how bright an HDR export peaks, since colorout maps 1.0 to the 203 nit reference white for PQ and HLG. Sigmoid now multiplies its target white by the pipe's output headroom, capped at the slider's own maximum of 1600% (3248 nits). Above that the curve fit can overflow for extreme contrast, skew and target black, which renders midtones black. In SDR nothing changes. The curve fit moves out of commit_params() so it can be redone at process time, when the headroom is known. As in AgX, new instances and presets follow the output, existing edits are migrated with it off, and a "follow output peak" toggle, shown only while it is off, opts them in. Related: darktable-org#22664
The eight hdr_* fields of tone_mapping_params_t, and their copy in the OpenCL kernel, become one hdr_params_t member. _set_hdr_params(), _hdr_darken() and _agx_hdr() now take only that struct. No change in output.
The PQ peak was read from the export module's setting whenever an image's output profile was set, so changing it while a batch ran also changed the images still waiting in that batch. The export module now reads it once, when the export is queued, and the job stores it with its other settings. The job hands it to the pipes it runs through a per-thread value in iop_profile.c, rather than by writing the setting back, which would change the darkroom and undo a new value picked in the export module during the batch. darktable-cli and other pipes still read the setting.
Export presets did not include the PQ peak, so applying a preset kept whatever peak was set, and multi-preset export could not give each preset its own peak. The peak is now stored after print_dpi, so the export module's params go to version 9. Presets saved by older versions get the default peak of 1000 nits. Since an export job keeps the peak it was queued with, each preset of a multi-preset export renders with its own peak.
darktable-cli could only set the PQ peak through the export module's setting, with --core --conf plugins/lighttable/export/pq_peak_nits=... It now takes --pq-peak <nits>, from 203 to 10000, which applies to the exports of that run only.
With pivot target output at 100%, the rise left for the shoulder is clamped to _epsilon, and _scale() raised it to -power in float. Above a shoulder power of about 6.4 that overflowed to inf, the scale became zero, and _scaled_sigmoid() returned NaN, so everything above the pivot rendered black. This already happened in SDR with shoulder power 7 and above; following an HDR output's peak raises the power enough to hit it at the default 3.3. _scale() now computes the powers in double. Normal settings render as before.
The pivot picker read the picked value off the curve for the darkroom's output, so on an HDR display it stored a different pivot target output than the same pick on an SDR display, and the edit's SDR export then depended on the display it was made on. Pivot target output is an SDR value that HDR output keeps, so the picker now uses the SDR curve, as it did before HDR output.
Every export preset stored the PQ peak, and presets from before it gave 1000 nits. Applying an sRGB preset, or an older one, therefore changed the PQ peak while its slider was hidden, and later PQ exports used that value. Multi-preset export also left the last preset's peak behind when the current profile was not PQ. Presets now store the peak only for a PQ or "image settings" profile, and 0 otherwise, which leaves the peak as it is when applied. Older presets get 0 too. Multi-preset export puts the peak back afterwards. The upgrade of an older preset also checks that it is long enough before copying it.
9fbf221 to
9430cb4
Compare
If you think about it, setting pivot at 100% means everything from the pivot to the white point becomes white, the shoulder is basically horizontal -> the pivot is the white point (everything above it clips). That only makes sense if the pivot input is on the right edge of the graph (there is no shoulder at all). |
I have checked that and found no issue. |
|
I have tested using the GUI & export module using the Standard export: 
Using export profil 
From Darkroom, using display profil:
Note that when 
|
Thanks for testing, and for checking the OpenCL path. PQ export looking too dark. Your PQ export (mire1_01) looks like PQ code values shown as if they were SDR: grey whites, lifted blacks, low saturation. That is what a PQ JPEG looks like in a browser or any viewer that doesn't decode PQ, because PQ puts reference white (SDR 100%) at 203 nits, about 58% of the signal, and the Rec2020 primaries aren't converted either. JPEG has no CICP tag, and darktable's PQ ICC profile has no This PR doesn't make it darker; it only adds range above reference white. mire1.cr2 with the default sigmoid workflow and no edits, on master and on this branch:
Shown as SDR, master's PQ export looks the same, very slightly dimmer (mean code 0.418 vs 0.431). Darkroom with the PQ display profile. Same cause: darktable sends PQ codes to a screen that isn't in PQ mode, so the center view looks like your PQ export. Master does the same. Filmstrip thumbnails too light. This one isn't from this PR; it comes from #20562's fix (4b0878d). Since then the pipe encodes PQ with 1.0 = 203 nits ( |
Would be great to fix that in a follow-up PR. Thanks. |
|
To me this is good to go, @kofa73 ok on your side or do you see something still broken on AgX? |
|
No objections from me. |
Summary
colorout maps 1.0 to the 203 nit reference white for PQ and HLG, so a tone mapper's target white decided how bright an HDR export peaked, and it had to be set by hand for every target. Following the design agreed with @TurboGit in #22661, the export now decides and the tone mappers adapt on their own, so one edit exports to SDR, HLG and PQ.
--pq-peak <nits>. PQ export presets store it, and an export keeps the peak it was queued with.AgXHLG.pyin EaryChow/AgX_LUT_Gen). The curve's output, target white included, is relative to the output's peak, with the shoulder power multiplied by H^log10(2) and capped at the slider's maximum of 10, where the curve fit would otherwise overflow. A second sigmoid in log2 space then moves the pivot output down by H, so shadows and the pivot keep their SDR brightness and the highlights extend to the peak. A new "HDR purity" slider (default 50%, Blender's value) sets how much of that step's hue and saturation shift is kept. The graph, its markers and the curve warnings use the headroom of the darkroom's full pipe, read onDT_SIGNAL_DEVELOP_UI_PIPE_FINISHED(so cache hits are covered too). The pivot picker keeps working on the SDR curve, since pivot target output is an SDR value.SDR output is unchanged in both modules (H = 1 gives bit-identical output). Following the output is a hidden param, on for new instances, the auto-applied defaults and the built-in presets. Existing edits are migrated with it off and render exactly as before (option (a) in the design discussion). For those, a "follow output peak" toggle is shown, and only while it is off, so a user can opt an old edit in. Params versions go to AgX 8 and sigmoid 4.
Filmic is not included: its color science v3-v5 clip at 1.0 before the output power, and its spline raises midtone contrast to reach a higher white. It will follow in its own PR. Grain, vignetting, framing and watermark, which sit after the tone mappers, were checked by reading the code: none clips at 1.0 (vignette only with its legacy "unbound" off). Grain's LUT is clamped at L = 100, so grain fades out above reference white rather than clipping.
Referenced issue
Fixes #22664
Checklist
src/tests/integration/where the pixelpipe is touched, ordarktable-clias a headless smoke test._(), new preferences are registered indata/darktableconfig.xml.in.RELEASE_NOTES.mdentry was added (only needed if fixing an issue in a release). Do not reference GitHub issues.Test instructions
What was run (Linux, CPU only,
-DUSE_AI=OFF -DUSE_LUA=OFF -DUSE_GMIC=OFF, on master 748066d):Each commit builds without warnings (
git rebase --exec).Integration tests 0097, 0151 (sigmoid), 0174, 0175 (AgX), 0001 and 0014 pass with OpenCL off.
darktable-clion a neutral PFM ramp (one patch per EV, 0.18 x 2^EV) with AgX's and sigmoid's default params, following the output ("follow") and migrated ("legacy"). Output in nits, decoded from PQ and HLG:SDR output with the param on and off is bit-identical for both modules. No NaNs.
AgX with pivot target output 18/90/99/100%, shoulder power 3.3 and 10, and SDR, PQ 1000/4000/10000 and HLG output renders without black highlights or NaNs. Pivot output 100% with shoulder power 7 or more already rendered black on master in SDR (float overflow in the shoulder fit);
_scale()now works in double, which fixes both.A Sony ARW from the integration test images with the AgX workflow's auto-applied defaults: the brightest highlights reach 195 nits in SDR and 976 nits in PQ 1000 and HLG, with the same history.
New integration tests 0182-0189 (in darktable-tests, hlrecovery.arw): sigmoid to sRGB and PQ, AgX to sRGB and HLG, each as a new edit and as an edit made before this PR. SDR pairs are byte-identical; new edits reach 907 nits (sigmoid, PQ 1000) and 1000 nits (AgX, HLG), old edits stay at about 203. Old edits rendered by current master match their expected images (max dE 1.16).
darktable-cli --pq-peak 4000gives the same output as the setting at 4000; out-of-range values are rejected.An export keeps the PQ peak it was queued with:
dt_control_export()stores it with the job, which hands it to its pipes through a per-thread value instead of rewriting the setting.Export presets store the PQ peak (export module params version 9) when their profile is PQ or "image settings", and 0 (leave the peak alone) otherwise. A version 8 preset written into data.db was upgraded on startup with 0 inserted after print_dpi and the rest unchanged; a truncated one is dropped.
The AgX pivot picker reads the pivot from the SDR curve, as on master, so a pick doesn't depend on the display.
The export module was checked under Xvfb: "PQ peak" shows for PQ Rec2020 and is hidden for sRGB.
Not verified: the OpenCL path on a GPU (no GPU in the build container); the AgX and sigmoid GUI (the opt-in toggle, the HDR purity slider, the AgX graph and picker) and the darkroom refresh after changing the PQ peak, which were not opened; the batch-export path with the PQ peak changed mid-batch was not run (the job keeps the peak it was queued with; darktable-cli still reads the setting); saving and applying an export preset in the GUI (the stored layout and the upgrade of old presets were checked).
AI assistance
Written with Claude Code (Anthropic), which implemented the change and ran the tests above.