feat(display): per-output ICC color profiles and color temperature - #3388
feat(display): per-output ICC color profiles and color temperature#3388KIDult2226 wants to merge 6 commits into
Conversation
|
/claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| log.Info("gamma: output returned, re-establishing controls") | ||
| m.controlsInitialized = true | ||
| }) | ||
| if m.controlsInitialized { |
There was a problem hiding this comment.
This drops the recovery path for monitor sleep/disconnect. When the last output goes away, the remove handler sets m.controlsInitialized = false (line 270). With the old code the returning output ran addOutputControl and then re-marked controlsInitialized = true so the following gamma_size drove the reapply. Now the if m.controlsInitialized guard is false in exactly that case, so nothing is created and gamma/ICC stays dead until the user manually toggles night light.
Keep the previous behaviour: always post addOutputControl, and set controlsInitialized = true when it was cleared.
| case !m.controlsInitialized: | ||
| log.Debugf("applyGamma skipped: controls not initialized") | ||
| return | ||
| case m.lastAppliedTemp == temp && m.lastAppliedGamma == gamma: |
There was a problem hiding this comment.
This manager-level dedupe doesn't include contrast, and it short-circuits before the per-output loop, so two existing paths break:
SetAdjustments(gamma, contrast)with only contrast changed →syncControls→triggerUpdate→applyCurrentTemp→applyGamma(temp)with the same temp and gamma → early return. The contrast change is never written to any output.handleResume(line 1119) resets everyout.lastTemp = 0to force a resend after suspend (Gamma settings not applied after wake from sleep [with patch] #1235), then callsapplyCurrentTemp. Temp and gamma are unchanged across suspend, so this guard returns before the outputs are touched and the forced resend never happens.
Either include contrast in the comparison and have those paths reset m.lastAppliedTemp, or drop this guard and rely on the existing per-output rampCurrent dedupe, which already covers temp/gamma/contrast.
| } else { | ||
| log.Debugf("gamma_size: output %d not found in m.outputs", outputID) | ||
| } | ||
| m.lastAppliedTemp = 0 |
There was a problem hiding this comment.
Dropping out.lastTemp = 0 here (and the matching one in the send-failure path below, line ~936) breaks recovery after a gamma control failure. recreateOutputControl reuses the same outputState, so lastTemp/lastGamma/lastContrast survive the failure. When the new control's gamma_size arrives, failed and rampSize are reset but rampCurrent(temp, gamma, contrast) still returns true, so the output is skipped and the recreated control never receives a ramp — the display stays uncorrected.
m.lastAppliedTemp = 0 only bypasses the manager-level guard; it doesn't clear the per-output state. Keep out.lastTemp = 0 in both places.
| sunrise := time.Date(now.Year(), now.Month(), now.Day(), | ||
| config.ManualSunrise.Hour(), config.ManualSunrise.Minute(), config.ManualSunrise.Second(), 0, now.Location()) | ||
| sunset := time.Date(now.Year(), now.Month(), now.Day(), | ||
| config.ManualSunset.Hour(), config.ManualSunset.Minute(), config.ManualSunset.Second(), 0, now.Location()) |
There was a problem hiding this comment.
This PR reverts three unrelated scheduler fixes that exist on the base commit:
- The
if !sunset.After(sunrise) { sunset = sunset.Add(24 * time.Hour) }adjustment right below this line is gone, so a manual schedule whose night start is past midnight (e.g. sunrise 07:00, sunset 00:30) now produces aSunsetbeforeSunriseon the same day. activeCycle/shiftTimeswere deleted along with their call sites ingetSunPositionNormal,getDeadlineNormalandupdateStateFromSchedule, so early-morning hours no longer map back to yesterday's cycle — the temperature andisDayare wrong between midnight and dawn.- In the location-missing branch below (line 553),
m.schedule = sunSchedule{}was removed, so stale times from a previous config keep driving applies.
None of these are related to ICC; they should be restored.
| // non-ICC outputs get identity (no color shift). | ||
| if !enabled { | ||
| m.applyGamma(neutralTemp) | ||
| m.applyGamma(high) |
There was a problem hiding this comment.
high is the user-configurable HighTemp (SetTemperature, valid down to 1000K), not a neutral point — the removed neutralTemp was a fixed 6500. With night light disabled and HighTemp set to e.g. 5000, every output now gets a permanent 5000K ramp written to it instead of neutral. Use a fixed 6500 here (or DefaultConfig().HighTemp).
Related: syncControls (line 1292) still calls destroyControls() whenever needsControls() is false, so toggling night light off tears the controls down and drops the ICC ramps anyway — which defeats the "ICC applies while night light is off" goal of this PR. needsControls() probably needs to account for configured ICC profiles / per-output temps.
| if err != nil { | ||
| return "", err | ||
| } | ||
| return filepath.Join(configDir, "niri", "dms", "wayland.json"), nil |
There was a problem hiding this comment.
niri is hardcoded, but this dir is compositor-specific in the rest of the codebase — commands_setup.go:275 picks niri/dms, hypr/dms or mango/dms based on the running compositor. On Hyprland/mango/sway this writes ICC and per-output temp config into a niri directory that the rest of DMS never looks at. commands_icc.go:70 has the same hardcoding for the ICC profile dir.
Also worth using utils.XDGConfigHome() here rather than os.UserConfigDir(), so it matches the path resolution used everywhere else in core.
| strOff := binary.BigEndian.Uint32(data[recordStart+8 : recordStart+12]) | ||
| // String offset is absolute from profile start | ||
| strStart := entry.offset + strOff | ||
| strEnd := strStart + strLen |
There was a problem hiding this comment.
Both desc branches can panic on a malformed profile, which takes down the dms daemon (ApplyICC parses a user-picked file in-process, and dms icc list parses every file in the config dir).
strOffandstrLencome straight from the file.strStartis never bounds-checked, andstrEnd := strStart + strLenis uncheckeduint32arithmetic. IfstrStart > len(data)the clamp setsstrEnd = len(data), leavingstrStart > strEnd→data[strStart:strEnd]panics. Same for thedescpath at line 266-267 ifstrLenis large enough to wrapstrEndbelowstrStart.recordStart+12(line 297-299) is only guarded byentry.size < 16, so a tag withsize == 16at the end of the buffer reads past the slice.
Validate strStart <= strEnd <= len(data) (and recordStart+12 <= len(data)) before slicing, and return an error instead.
|
|
||
| onSliderValueChanged: function(newValue) { | ||
| colorTempRow.editing = true | ||
| tempLabel.text = newValue + "K" |
There was a problem hiding this comment.
Assigning to tempLabel.text imperatively destroys its binding permanently. After the first drag the label is a static string: colorTempRow.editing = false no longer restores it, and it stops tracking ICCService.outputTemps[outputName] for any later change (including one made from the CLI or another surface).
The binding on line 537 already handles both cases via colorTempRow.editing/tempSlider.value, so this line just needs to go.
| tempLabel.text = newValue + "K" | |
| colorTempRow.editing = true |
| colorTempRow.editing = true | ||
| tempLabel.text = newValue + "K" |
There was a problem hiding this comment.
Correcting the suggestion in my comment above — it should replace both lines, not just line 571 (otherwise editing = true ends up duplicated):
| colorTempRow.editing = true | |
| tempLabel.text = newValue + "K" | |
| colorTempRow.editing = true |
| } | ||
| } | ||
|
|
||
| DisplayConfirmationModal { |
There was a problem hiding this comment.
This PR deletes the identifyConfigured property, the MonitorIdentifyOverlay Loader that followed this block, and the id: monitorCanvas it depends on (line 560). That removes the working "identify monitors" overlay, which is unrelated to ICC — looks like an accidental revert. Same for the Theme.spacingXXS → 2 changes here and in OutputCard.qml, which swap Theme tokens back for hardcoded values.
Claude reviewSolid feature work, but it reverts several unrelated fixes in the gamma stack and the display settings UI, and the new dedupe breaks two existing paths — those need to be sorted before merge.
Checked: full diff against the merge base, gamma manager lifecycle/dedupe paths, ICC parser bounds handling, IPC handlers, and the new QML service and Display Config rows. Model: claude-opus-5. |
Rebuild of AvengeMedia#3388 on current master: the previous branch was cut from an older base and reverted unrelated fixes in the gamma stack and the display settings UI. ICC profiles: - core/internal/icc: ICC v2/v4 parser (TRC, LUT8/16 and parametric curves, vcgt, description/version/color space) plus gamma ramp generation, with synthetic coverage and optional vendor-profile tests (ICC_TEST_DIR). - wayland manager: load iccProfiles from the compositor's DMS config dir, build the ramp from the profile when one is set (falling back to the temperature ramp when it is missing or unparsable), and re-apply on output hotplug and after resume. - ICC ramps apply while the night light schedule is disabled; outputs without a profile keep an identity ramp. - desc/mluc offsets are validated before slicing, so a malformed profile returns an error instead of panicking the daemon. Per-output temperature: - outputTemps in the same config; SetOutputTemp accepts 1000-10000K and the Display Config slider exposes 3000-10000K, which matters for displays calibrated at a higher white point (e.g. a 7000K profile). Gamma control lifecycle: - needsControls() now keeps the controls alive for configured ICC profiles and per-output temperatures, so toggling the night light off no longer destroys the controls and drops the ramps. - Re-apply paths clear the per-output dedup state instead of adding a manager-level guard, which would have suppressed contrast-only writes and the post-resume forced resend (AvengeMedia#1235), and left a recreated control without a ramp. - Monitor sleep/disconnect still re-establishes controls when an output comes back. IPC / CLI / UI: - wayland.icc.{getStatus,apply,remove,listOutputs,setTemp,getTemps} - dms icc list | info <file> | apply <output> <file> | remove <output> | status - Display Config output card gains a Color Profile row (browse/apply/remove with description, version, color space and active state) and a per-output color temperature slider, backed by Services/ICCService.qml. Config path: - DMSConfigDir() follows the compositor layout (niri/dms, hypr/dms, mango/dms) via the shared compositor detection and utils.XDGConfigHome(), replacing the hardcoded niri path in both the manager and `dms icc`. Tests: TZ=UTC go test ./... (60 packages ok), including red/green coverage for the icc parser bounds checks and needsControls().
d45ea29 to
1c5cae1
Compare
|
Rebased onto current master ( Reverted upstream fixes (restored)
New-code fixes
One small addition beyond the review: Verification: Diff vs master: 13 files, +2406/−13. The 13 removed lines are |
The per-output temperature override was reachable from the Display Config slider and from `wayland.icc.setTemp` over IPC, but not from the CLI. - `dms icc set-temp <output> <kelvin>` (alias `setTemp`) sets a per-output override, 1000-10000K, and 0 clears it so the output follows the night light schedule again. Range checking matches the daemon so an invalid value fails in the CLI instead of over IPC. - `dms icc status` gains a Temp column (the override in K, or `schedule`), so an override can be verified without opening the settings UI. Verified against a running daemon: `dms icc status` reports 7000K for the three overridden outputs and `schedule` for the remaining one.
…e white point The per-output temperature was documented as the white point a profile was produced at, but an output with a profile ignored it entirely: the ramp was generated from the profile and the temperature only applied to outputs without one (or as the fallback when a profile failed to parse). A display calibrated at 7000K therefore got the profile ramp, and enabling the night light had no effect on it at all. - `ProfileRampWithTemp` composes the profile ramp with the ratio between the target temperature ramp and the reference ramp, so the temperature is the white point the profile describes and the night light shifts relative to it. - `applyGamma` passes the night light temperature as the target, and `noTempTarget` when the schedule is disabled, which leaves profiled outputs at their reference white point and drives plain outputs with the neutral ramp. - The re-apply sentinel stays 0, so it cannot collide with an applied temperature. Covered by TestProfileRampWithTemp (reference == target is a no-op, warmer and cooler targets move the expected channels). Also: `dms icc set-temp` help text describes the reference-white-point semantics.
Restoring the upstream "output returned, re-establishing controls" branch made the registry handler establish the gamma controls before the startup post runs, and that post returned early on `m.controlsInitialized`, so the configured ICC profiles and per-output temperatures were never attached: every output reported "(none)" and the night light ran with defaults until the user re-applied a profile by hand. - `initializeControlsAndICC` (the startup post) loads the configuration first and only creates the controls when they are missing. - The loading is split into `loadConfiguredICC` / `applyConfiguredICCForOutput` so it can also run per output. - A hotplugged output now gets its configured profile and temperature as soon as its name is known (name handler) or when its control is created, which also makes the "re-applies on hotplug" claim in the description true. - After attaching, the output's dedup state is cleared so the ramp is written. Covered by TestManager_LoadConfiguredICCWhenControlsAlreadyExist and TestManager_AttachConfiguredICCForNamedOutput.
The per-output value was treated as the white point a profile was produced at, which meant a display with a profile never changed when the user set a temperature: the profile ramp was applied as measured and the value only mattered when a night light target existed. Per-display temperatures were therefore unusable on profiled outputs while the night light was disabled, which is the normal state for users who want each monitor at its own value. - `effectiveTempTarget` decides per output: a per-output override wins over the night light schedule, the schedule applies when there is no override, and `noTempTarget` means neither. `0` is "no override", not 0K. - `applyGamma` composes the override on top of the profile ramp, treating the profile as measured at 6500K (D65), and drives outputs without a profile with the same target so both paths agree. - `dms icc set-temp` help text and the Display Config slider description go back to describing an independent per-output temperature. Covered by TestEffectiveTempTarget (override wins, schedule fallback, none) and the updated TestProfileRampWithTemp (7000K cools, 5000K warms relative to the 6500K reference).
|
Post-review updates now on the branch (
Tests added/updated:
|
The profile row only showed the description, so there was no way to tell which profile a display actually has applied (whitepoint, curve type, file provenance) without leaving the settings page. - `ICCStatus` carries the descriptive metadata: class, tone-curve kind (plus gamma or table size), vcgt channels/entries, white point chromaticity with a derived CCT and standard illuminant name, file size and mtime. - `internal/icc` gains `WhitePointXY`, `WhitePointCCT` (McCamy), `WhitePointName` (D50/D65) and `TRCKind`, all covered by tests. - Display Config's Color Profile row gets an info button that opens `ICCProfileInfoModal` with those fields and the profile path, alongside the existing browse/remove actions. - `dms icc info` reports the white point as name/xy/CCT instead of raw XYZ.
2 participants
Description
Adds per-output ICC color profile support and per-output color temperature to the night light / gamma stack.
ICC profiles
core/internal/iccpackage: ICC v2/v4 parser (TRC curves, LUT8/LUT16 and parametric curves,vcgt, description/version/color space metadata) with unit tests.iccProfilesandoutputTempsfrom the DMS config directory of the running compositor (<config>/<compositor>/dms/wayland.json, for example~/.config/niri/dms/wayland.jsonon niri), builds gamma ramps from the parsed profile per output, falls back to temperature ramps when a profile is missing or unparsable, and re-applies after resume.Per-output temperature
outputTempsin the same config;SetOutputTempaccepts 1000-10000K and the Display Config slider exposes the full 3000-10000K range.0= no override). It is composed on top of an ICC profile ramp, which is treated as measured at 6500K (D65); outputs without a profile are driven with the same value directly.dms icc set-temp <output> <kelvin>sets it from the CLI anddms icc statusreports it.IPC / CLI
wayland.icc.getStatus,wayland.icc.apply,wayland.icc.remove,wayland.icc.listOutputs,wayland.icc.setTemp,wayland.icc.getTemps.dms icc list | info <file> | apply <output> <file> | remove <output> | status | set-temp <output> <kelvin>.UI
quickshell/Services/ICCService.qml.Included fix
LoadConfig()now takes its fallback values fromDefaultConfig()and coversContrast.Validate()rejectsContrast == 0, so night light configs written before that field existed aborted manager construction and took gamma/ICC down entirely (wayland manager not initialized).Type of change
Related issues
Screenshots / video
Display Config output cards with the new Color Profile row (browse / apply / remove, showing an applied profile with version, color space and active state) and the per-output Color Temp slider set to 7000K on the calibrated display:
Checklist
I18n.tr()with translator context, reusing existing terms where possiblemake fmt, added/updated tests,make testpasses (TZ=UTC go test ./...-> 51 ok, 0 fail), andgo mod tidyis cleanmake lint-qmlwith no new warnings