From 83600d33356046d34d60125d7a0c7acec83cbd5a Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Sat, 24 Oct 2026 18:39:35 +0200 Subject: [PATCH] Color management: loading the working RGB profile again under its name takes effect at once --- crates/color/src/cms/icc.rs | 19 ++++++++--- crates/color/src/cms/tests_icc.rs | 46 +++++++++++++++++++++++++++ crates/engine/src/cmd/colormgmt.rs | 6 ++++ crates/engine/tests/color_settings.rs | 44 +++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 4 deletions(-) diff --git a/crates/color/src/cms/icc.rs b/crates/color/src/cms/icc.rs index 1ead944e2..8201b8b97 100644 --- a/crates/color/src/cms/icc.rs +++ b/crates/color/src/cms/icc.rs @@ -1,6 +1,7 @@ //! ICC profiles through `moxcms` (pure Rust): built-in RGB spaces and user-supplied `.icc` files, //! and the profiles exports embed ([`encode_builtin`]), written in code from the CMS. +use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Arc, Mutex, OnceLock}; use moxcms::{ @@ -8,7 +9,7 @@ use moxcms::{ RenderingIntent, ToneReprCurve, TransformF32Executor, TransformOptions, Vector3d, }; -use super::{CmsError, Intent, ProfileKind, lab}; +use super::{CmsError, Intent, ProfileKind, lab, profiles_generation}; fn moxcms_intent(i: Intent) -> RenderingIntent { match i { @@ -27,12 +28,17 @@ fn intent_index(i: Intent) -> usize { type Xf = Option>; -/// A direct transform's cache key: destination name, intent index, black-point compensation. -type DirectKey = (String, usize, bool); +/// A direct transform's cache key: destination [`IccProfile::id`], [`profiles_generation`] it was +/// built in, intent index, black-point compensation. The id tells a reloaded profile from the old +/// one of the same name, which settings built before the reload still convert into; the +/// generation only lets the cache drop what reloads left behind. +type DirectKey = (u64, u64, usize, bool); /// A parsed ICC profile plus lazily-built transforms to/from sRGB. pub struct IccProfile { pub name: String, + /// Unique to this instance: never reused, unlike its name or address. + id: u64, pub kind: ProfileKind, /// The file it was loaded from (empty for profiles made in code). source: Vec, @@ -82,8 +88,10 @@ impl IccProfile { other => return Err(CmsError::Unsupported(format!("profile colour space {other:?} is not RGB, CMYK or Gray"))), }; let name = name.or_else(|| text(&profile.description)).unwrap_or_else(|| format!("Untitled {kind:?} profile")); + static NEXT_ID: AtomicU64 = AtomicU64::new(0); Ok(Self { name, + id: NEXT_ID.fetch_add(1, Ordering::Relaxed), kind, source: Vec::new(), profile, @@ -182,8 +190,11 @@ impl IccProfile { if dest.kind != ProfileKind::Rgb { return None; } - let key = (dest.name.clone(), intent_index(intent), bpc); + let generation = profiles_generation(); + let key = (dest.id, generation, intent_index(intent), bpc); let mut cache = self.direct.lock().unwrap_or_else(|e| e.into_inner()); + // an older generation's entries can't match again: drop them so reloads don't pile up + cache.retain(|(k, _)| k.1 == generation); if let Some((_, t)) = cache.iter().find(|(k, _)| *k == key) { return t.clone(); } diff --git a/crates/color/src/cms/tests_icc.rs b/crates/color/src/cms/tests_icc.rs index fa2449bc0..46e44cd76 100644 --- a/crates/color/src/cms/tests_icc.rs +++ b/crates/color/src/cms/tests_icc.rs @@ -180,3 +180,49 @@ fn aliased_localized_names_are_refused() { assert!(super::icc::localized_text_fits(&profile(3, 64))); assert!(!super::icc::localized_text_fits(&profile(4096, 64 << 10))); } + +/// Reloading an RGB profile under the name it had (a new file for "My RGB") changes what CMYK +/// converts to in it: the direct CMYK → RGB transform isn't kept from the old file. +#[test] +fn a_reloaded_rgb_profile_replaces_the_direct_cmyk_transform_into_it() { + let cmyk = register_icc(&icc_bytes(GENERIC_CMYK).unwrap(), Some("Reload Test CMYK".into())).unwrap().name; + let rgb = "Reload Test RGB"; + let cyan = [1.0, 0.0, 0.0, 0.0]; + let convert = || { + let cms = Cms::new(&ColorSettings { rgb: rgb.into(), cmyk: cmyk.clone(), ..ColorSettings::default() }).unwrap(); + cms.cmyk_to_rgb(cyan, Intent::RelativeColorimetric) + }; + register_icc(&icc::builtin_rgb_bytes(SRGB).unwrap(), Some(rgb.into())).unwrap(); + let in_srgb = convert(); + register_icc(&icc::builtin_rgb_bytes(PROPHOTO_RGB).unwrap(), Some(rgb.into())).unwrap(); + let in_prophoto = convert(); + // What a CMYK profile loaded afresh gives in the new file. + let fresh = register_icc(&icc_bytes(GENERIC_CMYK).unwrap(), Some("Reload Test CMYK 2".into())).unwrap().name; + let want = Cms::new(&ColorSettings { rgb: rgb.into(), cmyk: fresh, ..ColorSettings::default() }) + .unwrap() + .cmyk_to_rgb(cyan, Intent::RelativeColorimetric); + assert!(in_srgb.iter().zip(want).any(|(a, b)| (a - b).abs() > 0.05), "sRGB {in_srgb:?} vs ProPhoto {want:?}"); + assert!(in_prophoto.iter().zip(want).all(|(a, b)| (a - b).abs() < 1e-4), "after the reload {in_prophoto:?}, want {want:?}"); +} + +/// The app loads a profile and only then switches to the new settings, so the working spaces +/// built before the reload keep converting in between: a CMYK color they convert after it doesn't +/// leave the old RGB file's transform for the settings built afterwards. +#[test] +fn settings_built_before_a_reload_dont_leave_the_old_transform_to_the_new_ones() { + let cmyk = register_icc(&icc_bytes(GENERIC_CMYK).unwrap(), Some("Stale Test CMYK".into())).unwrap().name; + let rgb = "Stale Test RGB"; + let cyan = [1.0, 0.0, 0.0, 0.0]; + let settings = ColorSettings { rgb: rgb.into(), cmyk: cmyk.clone(), ..ColorSettings::default() }; + register_icc(&icc::builtin_rgb_bytes(SRGB).unwrap(), Some(rgb.into())).unwrap(); + let old = Cms::new(&settings).unwrap(); + let in_srgb = old.cmyk_to_rgb(cyan, Intent::RelativeColorimetric); + register_icc(&icc::builtin_rgb_bytes(PROPHOTO_RGB).unwrap(), Some(rgb.into())).unwrap(); + assert_eq!(old.cmyk_to_rgb(cyan, Intent::RelativeColorimetric), in_srgb, "the old settings keep the old file"); + let in_prophoto = Cms::new(&settings).unwrap().cmyk_to_rgb(cyan, Intent::RelativeColorimetric); + // What a CMYK profile loaded afresh gives in the new file. + let fresh = register_icc(&icc_bytes(GENERIC_CMYK).unwrap(), Some("Stale Test CMYK 2".into())).unwrap().name; + let want = Cms::new(&ColorSettings { cmyk: fresh, ..settings }).unwrap().cmyk_to_rgb(cyan, Intent::RelativeColorimetric); + assert!(in_srgb.iter().zip(want).any(|(a, b)| (a - b).abs() > 0.05), "sRGB {in_srgb:?} vs ProPhoto {want:?}"); + assert!(in_prophoto.iter().zip(want).all(|(a, b)| (a - b).abs() < 1e-4), "after the reload {in_prophoto:?}, want {want:?}"); +} diff --git a/crates/engine/src/cmd/colormgmt.rs b/crates/engine/src/cmd/colormgmt.rs index 91afee24c..40144f13e 100644 --- a/crates/engine/src/cmd/colormgmt.rs +++ b/crates/engine/src/cmd/colormgmt.rs @@ -195,6 +195,12 @@ fn load_profile(s: &mut Session, p: &Value) -> Result { let info = cms::register_icc(&bytes, None).map_err(|e| bad(C, e.to_string()))?; // The bytes are kept, not the path: the file may move or go away. let remembered = s.store_profile(&info.name, &bytes); + // A new file of a working space: the active settings still hold the old one. + let working = cms::active_settings(); + if working.rgb == info.name || working.cmyk == info.name { + cms::set_active(&working).map_err(|e| bad(C, e.to_string()))?; + touch_all(s); + } Ok(json!({"name": info.name, "kind": info.kind, "remembered": remembered})) } diff --git a/crates/engine/tests/color_settings.rs b/crates/engine/tests/color_settings.rs index 80c189b3f..723a70bce 100644 --- a/crates/engine/tests/color_settings.rs +++ b/crates/engine/tests/color_settings.rs @@ -346,3 +346,47 @@ fn a_freeform_gradient_expands_with_its_rgb_values_in_the_working_rgb_space() { assert!(matches!(c, Color::Rgb { .. }) && close, "{c:?} shows {shown:?} for {field:?}"); } } + +/// Loading a new file of the working RGB profile (named after its description, as before) takes +/// effect at once: CMYK colors convert into the new file and the canvas redraws, without choosing +/// the working spaces again. +#[test] +fn loading_the_working_rgb_profile_again_takes_effect_at_once() { + let _g = settings(); + let old = cms::active_settings(); + // A built-in RGB profile's file renamed `name`: padded with spaces, which names drop. + let renamed = |builtin: &str, name: &str| { + let utf16 = |s: &str| s.encode_utf16().flat_map(u16::to_be_bytes).collect::>(); + let mut bytes = cms::icc_bytes(builtin).unwrap().to_vec(); + let (from, to) = (utf16(builtin), utf16(&format!("{name: revision; + // What a CMYK profile loaded afresh gives in the new file. + let fresh = cms::register_icc(&cms::icc_bytes(cms::GENERIC_CMYK).unwrap(), Some("Reload Working CMYK 2".into())).unwrap().name; + let want = cms::Cms::new(&cms::ColorSettings { cmyk: fresh, ..cms::active_settings() }).unwrap().cmyk_to_rgb(cyan, old.intent); + cms::set_active(&old).unwrap(); + assert!(in_p3.iter().zip(want).any(|(a, b)| (a - b).abs() > 0.05), "Display P3 {in_p3:?} vs ProPhoto {want:?}"); + assert!(after.iter().zip(want).all(|(a, b)| (a - b).abs() < 1e-4), "after the reload {after:?}, want {want:?}"); + assert!(redrawn, "the canvas redraws"); +}