Merge pull request #4053 from marcoarnulfo/fix/icc-reload-direct-cache

Color management: loading the working RGB profile again under its name takes effect at once

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<Arc<TransformF32Executor>>;
 
-/// 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<u8>,
@@ -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();
         }

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:?}");
+}

crates/engine/src/cmd/colormgmt.rs

@@ -195,6 +195,12 @@ fn load_profile(s: &mut Session, p: &Value) -> Result<Value> {
     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}))
 }
 

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::<Vec<u8>>();
+        let mut bytes = cms::icc_bytes(builtin).unwrap().to_vec();
+        let (from, to) = (utf16(builtin), utf16(&format!("{name:<width$}", width = builtin.len())));
+        let at = bytes.windows(from.len()).position(|w| w == from).unwrap();
+        bytes[at..at + to.len()].copy_from_slice(&to);
+        bytes
+    };
+    let name = "Reload RGB";
+    let dir = vectorcraft_testkit::temp_dir("reload-working-rgb");
+    let load = |s: &mut Session, builtin: &str| {
+        let path = dir.join(format!("{}.icc", builtin.len()));
+        std::fs::write(&path, renamed(builtin, name)).unwrap();
+        let r = s.execute("color.loadProfile", &json!({"path": path.to_str().unwrap()})).unwrap();
+        assert_eq!(r["name"], name);
+    };
+    let cmyk = cms::register_icc(&cms::icc_bytes(cms::GENERIC_CMYK).unwrap(), Some("Reload Working CMYK".into())).unwrap().name;
+    let cyan = [1.0, 0.0, 0.0, 0.0];
+    let convert = || cms::active().cmyk_to_rgb(cyan, old.intent);
+    let mut s = session();
+    load(&mut s, cms::DISPLAY_P3);
+    s.execute("edit.colorSettings", &json!({"rgb": name, "cmyk": cmyk})).unwrap();
+    let in_p3 = convert();
+    let revision = s.doc().unwrap().revision;
+    load(&mut s, cms::PROPHOTO_RGB);
+    let after = convert();
+    let redrawn = s.doc().unwrap().revision > 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");
+}