diff --git a/core/internal/icc/parser.go b/core/internal/icc/parser.go index 6ecc8a779..5be20b268 100644 --- a/core/internal/icc/parser.go +++ b/core/internal/icc/parser.go @@ -535,16 +535,20 @@ func SampleCurve(curve Curve, t float64) float64 { } } -// GenerateGammaRamp creates a wlr-gamma-control compatible ramp from an ICC profile. -// size is the ramp size (typically 256 or 4096). +// GenerateGammaRamp creates a wlr-gamma-control compatible ramp from an ICC +// profile. // -// Strategy: -// 1. If vcgt exists: resample vcgt entries to `size` entries (linear interpolation) -// 2. If no vcgt but TRC exists: use TRC curves (composite all three channels) -// 3. If neither: return identity ramp +// Only vcgt carries a video card gamma table, which is what the GPU LUT takes. +// rTRC/gTRC/bTRC describe the device-to-PCS transfer function of the display +// itself (roughly t^2.2 for a normal monitor), not a correction to write into +// the ramp, so treating them as one would wash the display out instead of +// calibrating it. A profile without a vcgt table therefore has no ramp. func GenerateGammaRamp(size uint32, profile *Profile) (GammaRamp, error) { - if size == 0 { - return GammaRamp{}, errors.New("icc: ramp size must be > 0") + if size < 2 { + return GammaRamp{}, errors.New("icc: ramp size must be at least 2") + } + if !profile.HasVCGT || profile.VCGT == nil { + return GammaRamp{}, errors.New("icc: profile has no vcgt table, so it has no video card gamma ramp") } ramp := GammaRamp{ @@ -553,38 +557,12 @@ func GenerateGammaRamp(size uint32, profile *Profile) (GammaRamp, error) { Blue: make([]uint16, size), } - // Strategy 1: VCGT exists - resample - if profile.HasVCGT && profile.VCGT != nil { - vcgt := profile.VCGT - for i := uint32(0); i < size; i++ { - t := float64(i) / float64(size-1) - ramp.Red[i] = resampleVCGT(vcgt.Red, t) - ramp.Green[i] = resampleVCGT(vcgt.Green, t) - ramp.Blue[i] = resampleVCGT(vcgt.Blue, t) - } - return ramp, nil - } - - // Strategy 2: TRC exists - use curves - if profile.HasTRC { - for i := uint32(0); i < size; i++ { - t := float64(i) / float64(size-1) - r := SampleCurve(profile.TRC[0], t) - g := SampleCurve(profile.TRC[1], t) - b := SampleCurve(profile.TRC[2], t) - ramp.Red[i] = uint16Clamp(r * 65535.0) - ramp.Green[i] = uint16Clamp(g * 65535.0) - ramp.Blue[i] = uint16Clamp(b * 65535.0) - } - return ramp, nil - } - - // Strategy 3: Identity ramp + vcgt := profile.VCGT for i := uint32(0); i < size; i++ { - v := uint16(float64(i) / float64(size-1) * 65535.0) - ramp.Red[i] = v - ramp.Green[i] = v - ramp.Blue[i] = v + t := float64(i) / float64(size-1) + ramp.Red[i] = resampleVCGT(vcgt.Red, t) + ramp.Green[i] = resampleVCGT(vcgt.Green, t) + ramp.Blue[i] = resampleVCGT(vcgt.Blue, t) } return ramp, nil } diff --git a/core/internal/icc/parser_test.go b/core/internal/icc/parser_test.go index 458eda82b..695721c42 100644 --- a/core/internal/icc/parser_test.go +++ b/core/internal/icc/parser_test.go @@ -152,6 +152,12 @@ func TestGenerateGammaRamp(t *testing.T) { if err != nil { t.Fatalf("ParseFile failed: %v", err) } + if !p.HasVCGT { + if _, err := GenerateGammaRamp(256, p); err == nil { + t.Error("expected an error for a profile without a vcgt table") + } + t.Skip("profile carries no vcgt table, so it has no ramp") + } for _, rampSize := range []uint32{256, 4096} { t.Run("size="+itoa(rampSize), func(t *testing.T) { @@ -297,21 +303,6 @@ func TestParseBytes(t *testing.T) { t.Error("expected error for short data") } }) - - t.Run("IdentityRamp", func(t *testing.T) { - // Generate identity ramp from a profile with no vcgt and no TRC - p := &Profile{} - ramp, err := GenerateGammaRamp(256, p) - if err != nil { - t.Fatalf("GenerateGammaRamp failed: %v", err) - } - if ramp.Red[0] != 0 { - t.Errorf("identity ramp Red[0] = %d, want 0", ramp.Red[0]) - } - if ramp.Red[255] < 65530 { - t.Errorf("identity ramp Red[255] = %d, want ~65535", ramp.Red[255]) - } - }) } // itoa converts a uint32 to string (avoids importing strconv) @@ -486,6 +477,35 @@ func TestGenerateGammaRampSynthetic(t *testing.T) { } } +// A profile without a vcgt table has no video card gamma ramp: the TRC tags +// describe the display's own transfer function, so writing them into the GPU +// LUT washes the output out instead of calibrating it. +func TestGenerateGammaRampRequiresVCGT(t *testing.T) { + trcOnly := &Profile{ + HasTRC: true, + HasVCGT: false, + TRC: [3]Curve{ + {Type: CurveParametric, Gamma: 2.2}, + {Type: CurveParametric, Gamma: 2.2}, + {Type: CurveParametric, Gamma: 2.2}, + }, + } + if _, err := GenerateGammaRamp(256, trcOnly); err == nil { + t.Fatal("expected an error for a profile without a vcgt table") + } + + // A vcgt tag with no channel data is unusable in the same way. + emptyVCGT := &Profile{HasVCGT: true} + if _, err := GenerateGammaRamp(256, emptyVCGT); err == nil { + t.Fatal("expected an error for a profile without a vcgt table") + } + + // The ramp size has to be at least 2: the values are spread over size-1. + if _, err := GenerateGammaRamp(1, &Profile{HasVCGT: true, VCGT: &VCGT{Channels: 3, Entries: 4}}); err == nil { + t.Fatal("expected an error for a ramp size below 2") + } +} + // TestParseDescriptionMalformed feeds desc/mluc tags whose offsets or lengths // point outside the buffer. These must return an error or a clamped string // instead of a slice-bounds panic: `dms icc list` parses every file in the diff --git a/core/internal/server/wayland/gamma_test.go b/core/internal/server/wayland/gamma_test.go index c627bd38e..1e2c7ddc3 100644 --- a/core/internal/server/wayland/gamma_test.go +++ b/core/internal/server/wayland/gamma_test.go @@ -166,8 +166,26 @@ func TestProfileRampWithTemp(t *testing.T) { const size = uint32(256) mid := int(size) / 2 - // An empty profile yields the identity ramp. - profile := &icc.Profile{} + // Only vcgt carries a video card gamma table, so the fixture is a profile + // whose table is the identity ramp: that keeps the expectations below about + // the temperature composition readable. + identityTable := func(entries int) []uint16 { + out := make([]uint16, entries) + for i := range out { + out[i] = uint16(float64(i) / float64(entries-1) * 65535.0) + } + return out + } + profile := &icc.Profile{ + HasVCGT: true, + VCGT: &icc.VCGT{ + Channels: 3, + Entries: 256, + Red: identityTable(256), + Green: identityTable(256), + Blue: identityTable(256), + }, + } identity := GenerateIdentityRamp(size) t.Run("no target keeps the profile as measured", func(t *testing.T) { diff --git a/core/internal/server/wayland/manager.go b/core/internal/server/wayland/manager.go index fa1013479..13bd86846 100644 --- a/core/internal/server/wayland/manager.go +++ b/core/internal/server/wayland/manager.go @@ -124,6 +124,24 @@ func (m *Manager) post(fn func()) { } } +// cloneConfig returns a copy of cfg whose maps belong to the copy. The manager +// mutates the config's maps under configMutex, so a value copy that still +// aliased them would turn a marshal or an iteration after the unlock into a +// concurrent map read/write. +func cloneConfig(cfg Config) Config { + cfg.ICCProfiles = maps.Clone(cfg.ICCProfiles) + cfg.OutputTemps = maps.Clone(cfg.OutputTemps) + return cfg +} + +// configSnapshot returns a clone of the current config that stays valid after +// configMutex is released. +func (m *Manager) configSnapshot() Config { + m.configMutex.RLock() + defer m.configMutex.RUnlock() + return cloneConfig(m.config) +} + func (m *Manager) waylandActor() { defer m.wg.Done() for { @@ -541,9 +559,7 @@ func (m *Manager) markConnectionDead(err error) { } func (m *Manager) recalcSchedule(now time.Time) { - m.configMutex.RLock() - config := m.config - m.configMutex.RUnlock() + config := m.configSnapshot() m.scheduleMutex.Lock() defer m.scheduleMutex.Unlock() @@ -614,9 +630,7 @@ func (m *Manager) SetGeoClient(client geolocation.Client) { } func (m *Manager) getLocation() (*float64, *float64) { - m.configMutex.RLock() - config := m.config - m.configMutex.RUnlock() + config := m.configSnapshot() if config.Latitude != nil && config.Longitude != nil { return config.Latitude, config.Longitude @@ -768,9 +782,7 @@ func (m *Manager) getDeadlineNormal(now time.Time, sched sunSchedule) time.Time func (m *Manager) tomorrowDawn(now time.Time) time.Time { tomorrow := time.Date(now.Year(), now.Month(), now.Day()+1, 0, 0, 0, 0, now.Location()) - m.configMutex.RLock() - config := m.config - m.configMutex.RUnlock() + config := m.configSnapshot() if config.ManualSunrise != nil { dur := time.Hour @@ -957,7 +969,7 @@ func (m *Manager) applyGamma(temp int) { ramp = GenerateGammaRamp(out.rampSize, fallbackTemp, gamma, contrast) } else { ramp = profileRamp - log.Infof("icc: applied ICC ramp to output %d (size=%d, ref=%dK, target=%dK)", out.id, out.rampSize, neutralTemp, targetTemp) + log.Debugf("icc: applied ICC ramp to output %d (size=%d, ref=%dK, target=%dK)", out.id, out.rampSize, neutralTemp, targetTemp) } } else { outTemp := targetTemp @@ -1039,9 +1051,7 @@ func (m *Manager) setGammaBytes(out *outputState, data []byte) error { func (m *Manager) updateStateFromSchedule() { now := time.Now() - m.configMutex.RLock() - config := m.config - m.configMutex.RUnlock() + config := m.configSnapshot() m.scheduleMutex.RLock() times := m.schedule.times @@ -1191,7 +1201,7 @@ func (m *Manager) SetConfig(config Config) error { return err } m.configMutex.Lock() - m.config = config + m.config = cloneConfig(config) m.configMutex.Unlock() m.triggerUpdate() return nil @@ -1361,6 +1371,31 @@ func (m *Manager) syncControls() { }) } +// ensureOutputControls creates the gamma controls when the manager has none. +// Called on the wayland actor goroutine. A default install (night light off, +// gamma and contrast neutral, no profile, no override) has no controls, but +// applying an ICC profile or a per-output temperature still needs them. +func (m *Manager) ensureOutputControls() bool { + if m.controlsInitialized { + return true + } + gammaMgr, ok := m.gammaControl.(*wlr_gamma_control.ZwlrGammaControlManagerV1) + if !ok || gammaMgr == nil { + log.Warn("icc: gamma control manager not available") + return false + } + m.availOutputsMu.RLock() + outs := slices.Clone(m.availableOutputs) + m.availOutputsMu.RUnlock() + if err := m.setupOutputControls(outs, gammaMgr); err != nil { + log.Errorf("icc: failed to initialize output controls: %v", err) + return false + } + m.controlsInitialized = true + log.Info("icc: output controls initialized (gamma was disabled)") + return true +} + func (m *Manager) createControls() { gammaMgr := m.gammaControl.(*wlr_gamma_control.ZwlrGammaControlManagerV1) m.availOutputsMu.RLock() @@ -1396,10 +1431,9 @@ func (m *Manager) destroyControls() { // temperatures to the outputs that are already known, and reports whether // anything was attached. func (m *Manager) loadConfiguredICC() bool { - m.configMutex.RLock() - iccProfiles := m.config.ICCProfiles - outputTemps := m.config.OutputTemps - m.configMutex.RUnlock() + config := m.configSnapshot() + iccProfiles := config.ICCProfiles + outputTemps := config.OutputTemps if len(iccProfiles) == 0 && len(outputTemps) == 0 { return false @@ -1427,10 +1461,9 @@ func (m *Manager) loadConfiguredICC() bool { // output whose name just became known, so a hotplugged output comes up // corrected without an explicit re-apply. func (m *Manager) attachConfiguredICC(outputID uint32, outputName string) { - m.configMutex.RLock() - iccProfiles := m.config.ICCProfiles - outputTemps := m.config.OutputTemps - m.configMutex.RUnlock() + config := m.configSnapshot() + iccProfiles := config.ICCProfiles + outputTemps := config.OutputTemps if len(iccProfiles) == 0 && len(outputTemps) == 0 { m.publishICCState() @@ -1471,9 +1504,14 @@ func (m *Manager) applyConfiguredICCForOutput(out *outputState, outputName strin if iccPath, ok := iccProfiles[outputName]; ok { profile, err := icc.ParseFile(iccPath) - if err != nil { + switch { + case err != nil: log.Warnf("icc: failed to load profile for output %q: %v", outputName, err) - } else { + case !profile.HasVCGT: + // Only vcgt can be written to the GPU ramp; the TRC curve describes + // the display itself and would wash the output out if applied. + log.Warnf("icc: profile %q for output %q has no vcgt table, not attaching it", iccPath, outputName) + default: out.iccPath = iccPath out.iccProfile = profile changed = true @@ -1495,29 +1533,21 @@ func (m *Manager) ApplyICC(outputName, iccPath string) error { if profile.ColorSpace != "RGB" { return fmt.Errorf("icc: unsupported color space %q, only RGB supported", profile.ColorSpace) } - if !profile.HasVCGT && !profile.HasTRC { - return fmt.Errorf("icc: profile has neither vcgt nor TRC curves, cannot generate gamma ramp") + // Only vcgt holds a video card gamma table, which is what the GPU ramp + // takes. The TRC tags describe the display's own transfer function, so a + // profile without vcgt has no correction to apply. + if !profile.HasVCGT { + return fmt.Errorf("icc: %s has no vcgt table, so it has no gamma ramp to apply", iccPath) } // 2. Post to wayland actor thread to apply m.post(func() { - // If output controls aren't initialized (gamma/night-light disabled), - // initialize them now so ICC can work independently. - if !m.controlsInitialized { - gammaMgr, ok := m.gammaControl.(*wlr_gamma_control.ZwlrGammaControlManagerV1) - if !ok { - log.Warnf("icc: gamma control manager not available") - return - } - m.availOutputsMu.RLock() - outs := slices.Clone(m.availableOutputs) - m.availOutputsMu.RUnlock() - if err := m.setupOutputControls(outs, gammaMgr); err != nil { - log.Errorf("icc: failed to initialize output controls: %v", err) - return - } - m.controlsInitialized = true - log.Info("icc: output controls initialized (gamma was disabled)") + defer m.updateStateFromSchedule() + + // ICC works with the night light off, but the gamma controls that carry + // the ramp may not exist yet, so create them before attaching. + if !m.ensureOutputControls() { + return } // Find the output by name @@ -1550,7 +1580,7 @@ func (m *Manager) ApplyICC(outputName, iccPath string) error { m.config.ICCProfiles = make(map[string]string) } m.config.ICCProfiles[outputName] = iccPath - savedConfig := m.config + savedConfig := cloneConfig(m.config) m.configMutex.Unlock() // Save to disk @@ -1558,15 +1588,14 @@ func (m *Manager) ApplyICC(outputName, iccPath string) error { log.Warnf("icc: failed to save config: %v", err) } - // 4. Notify subscribers - m.updateStateFromSchedule() - return nil } // RemoveICC removes the ICC profile from a specific output, reverting to temperature-based gamma. func (m *Manager) RemoveICC(outputName string) error { m.post(func() { + defer m.updateStateFromSchedule() + m.outputs.Range(func(_ uint32, out *outputState) bool { if name, ok := m.outputNames.Load(out.id); ok && name == outputName { out.iccPath = "" @@ -1582,14 +1611,13 @@ func (m *Manager) RemoveICC(outputName string) error { m.configMutex.Lock() delete(m.config.ICCProfiles, outputName) - savedConfig := m.config + savedConfig := cloneConfig(m.config) m.configMutex.Unlock() if err := SaveConfig(savedConfig); err != nil { log.Warnf("icc: failed to save config: %v", err) } - m.updateStateFromSchedule() return nil } @@ -1598,17 +1626,30 @@ func (m *Manager) RemoveICC(outputName string) error { // this must run there; the getters are called from the IPC handlers, the CLI // and the scheduler goroutine, which is why they never touch those fields. // Publishing on ICC changes rather than on every state push also keeps the -// per-profile stat below out of the night light animation path. +// per-profile stat below out of the night light animation path. The status map +// reflects what is attached to an output; the temperature map reports the +// configured value, so a value set for an output that is not known yet is still +// reported instead of snapping the settings slider back to "Default". func (m *Manager) publishICCState() { status := make(map[string]*ICCStatus) temps := make(map[string]int) + m.configMutex.RLock() + for name, temp := range m.config.OutputTemps { + if temp != 0 { + temps[name] = temp + } + } + m.configMutex.RUnlock() + m.outputs.Range(func(_ uint32, out *outputState) bool { name, ok := m.outputNames.Load(out.id) if !ok { return true } - temps[name] = out.outputTemp + if _, configured := temps[name]; !configured && out.outputTemp != 0 { + temps[name] = out.outputTemp + } if out.iccPath != "" { status[name] = describeICCProfile(out.iccPath, out.iccProfile) } @@ -1677,7 +1718,33 @@ func (m *Manager) SetOutputTemp(outputName string, temp int) error { return fmt.Errorf("temperature %d out of range (1000-10000)", temp) } + // Persist first: on a default install (night light off, no profile) no + // gamma control exists yet, and the stored value is what makes + // needsControls() true for the controls the apply below creates. + m.configMutex.Lock() + if m.config.OutputTemps == nil { + m.config.OutputTemps = make(map[string]int) + } + if temp == 0 { + delete(m.config.OutputTemps, outputName) + } else { + m.config.OutputTemps[outputName] = temp + } + savedConfig := cloneConfig(m.config) + m.configMutex.Unlock() + + if err := SaveConfig(savedConfig); err != nil { + log.Warnf("icc: failed to save config: %v", err) + } + m.post(func() { + defer m.updateStateFromSchedule() + + // Without controls there are no outputs to write a ramp for, which is + // why the slider and `dms icc set-temp` used to do nothing until a + // restart on an install that had never enabled the night light. + m.ensureOutputControls() + m.outputs.Range(func(_ uint32, out *outputState) bool { if name, ok := m.outputNames.Load(out.id); ok && name == outputName { out.outputTemp = temp @@ -1688,26 +1755,13 @@ func (m *Manager) SetOutputTemp(outputName string, temp int) error { } return true }) + + // An output whose name is not known yet picks the value up through + // attachConfiguredICC; the published state carries the configured value + // either way, so the settings slider does not snap back to Default. + m.publishICCState() }) - // Persist to config - m.configMutex.Lock() - if m.config.OutputTemps == nil { - m.config.OutputTemps = make(map[string]int) - } - if temp == 0 { - delete(m.config.OutputTemps, outputName) - } else { - m.config.OutputTemps[outputName] = temp - } - savedConfig := m.config - m.configMutex.Unlock() - - if err := SaveConfig(savedConfig); err != nil { - log.Warnf("icc: failed to save config: %v", err) - } - - m.updateStateFromSchedule() return nil } diff --git a/core/internal/server/wayland/manager_test.go b/core/internal/server/wayland/manager_test.go index eca4a9b00..92f501f27 100644 --- a/core/internal/server/wayland/manager_test.go +++ b/core/internal/server/wayland/manager_test.go @@ -1,7 +1,9 @@ package wayland import ( + "encoding/json" "errors" + "fmt" "os" "path/filepath" "sync" @@ -798,3 +800,98 @@ func TestManager_ICCStatusConcurrentPublishAndRead(t *testing.T) { } wg.Wait() } + +// The config is copied by value all over the manager, so a copy has to be a real +// snapshot: the maps must not alias the ones the manager writes under its lock, +// or a state push marshalled on another goroutine becomes a fatal +// "concurrent map iteration and map write". +func TestManager_ConfigSnapshotOwnsItsMaps(t *testing.T) { + m := &Manager{config: DefaultConfig()} + m.config.ICCProfiles = map[string]string{"DP-1": "/tmp/dp1.icc"} + m.config.OutputTemps = map[string]int{"DP-1": 7000} + + snapshot := m.configSnapshot() + + m.configMutex.Lock() + m.config.ICCProfiles["DP-2"] = "/tmp/dp2.icc" + m.config.OutputTemps["DP-1"] = 4000 + m.config.OutputTemps["DP-2"] = 5000 + m.configMutex.Unlock() + + assert.Equal(t, map[string]string{"DP-1": "/tmp/dp1.icc"}, snapshot.ICCProfiles) + assert.Equal(t, map[string]int{"DP-1": 7000}, snapshot.OutputTemps) +} + +// Marshalling the copied config happens on the connection writer goroutine +// while the IPC goroutine stores a profile, so the copy has to be taken under +// the lock and own its maps (run with -race). +func TestManager_ConfigSnapshotConcurrentMarshal(t *testing.T) { + m := &Manager{config: DefaultConfig()} + m.config.ICCProfiles = make(map[string]string) + m.config.OutputTemps = make(map[string]int) + + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + for i := range 500 { + name := fmt.Sprintf("DP-%d", i) + m.configMutex.Lock() + m.config.ICCProfiles[name] = "/tmp/profile.icc" + m.config.OutputTemps[name] = 6000 + i + m.configMutex.Unlock() + } + }() + + for range 500 { + if _, err := json.Marshal(m.configSnapshot()); err != nil { + t.Fatalf("marshal config snapshot: %v", err) + } + } + wg.Wait() +} + +// On a default install (night light off, no profile) no gamma control exists, +// so a per-output temperature used to be written to wayland.json without ever +// being applied or reported: the settings slider snapped back to "Default" and +// the value only took effect after a daemon restart. +func TestManager_SetOutputTempWithoutControlsPublishesValue(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + + m := &Manager{ + config: DefaultConfig(), + cmdq: make(chan cmd, 8), + stopChan: make(chan struct{}), + } + m.wg.Add(1) + go m.waylandActor() + defer func() { + close(m.stopChan) + m.wg.Wait() + }() + + if err := m.SetOutputTemp("DP-1", 7000); err != nil { + t.Fatalf("SetOutputTemp() = %v", err) + } + + // Wait for the posted body, so the assertions below are ordered after it. + done := make(chan struct{}) + m.post(func() { close(done) }) + <-done + + assert.Equal(t, 7000, m.GetOutputTemps()["DP-1"], "the value is reported even without gamma controls") + + path, err := getConfigPath() + if err != nil { + t.Fatalf("config path: %v", err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + var saved Config + if err := json.Unmarshal(data, &saved); err != nil { + t.Fatalf("unmarshal %s: %v", path, err) + } + assert.Equal(t, 7000, saved.OutputTemps["DP-1"], "and persisted to the config file") +} diff --git a/quickshell/Modules/Settings/DisplayConfig/ICCProfileInfoModal.qml b/quickshell/Modules/Settings/DisplayConfig/ICCProfileInfoModal.qml index 856e48195..d6ba704fc 100644 --- a/quickshell/Modules/Settings/DisplayConfig/ICCProfileInfoModal.qml +++ b/quickshell/Modules/Settings/DisplayConfig/ICCProfileInfoModal.qml @@ -198,7 +198,7 @@ DankModal { StyledText { visible: root.details.length === 0 - text: I18n.tr("No information available") + text: I18n.tr("No information available", "No information available") font.pixelSize: Theme.fontSizeMedium color: Theme.surfaceTextMedium }