From 7e988d5d51b02f956751c3caaaefbfded6138b6d Mon Sep 17 00:00:00 2001 From: Rahul A Mistry Date: Wed, 26 Aug 2026 05:53:53 +0530 Subject: [PATCH 1/4] fix: make colour profile changes correct and undoable (cherry picked from commit 4d36c6a54b43f3f6a4d7c738faf3937c1cf9ba63) --- crates/app/src/workspace.rs | 19 +++++--- crates/codec-psd/src/writer/mod.rs | 8 ++++ crates/codec-psd/tests/writer.rs | 63 ++++++++++++++++++++++++++ crates/colormgmt/src/lib.rs | 72 +++++++++++++++++++++++++----- crates/core/src/document.rs | 60 +++++++++++++++++++++++++ crates/core/src/history.rs | 8 ++++ 6 files changed, 213 insertions(+), 17 deletions(-) diff --git a/crates/app/src/workspace.rs b/crates/app/src/workspace.rs index f291d99b..7cdc3f59 100644 --- a/crates/app/src/workspace.rs +++ b/crates/app/src/workspace.rs @@ -4680,8 +4680,9 @@ impl Workspace { /// Assign a profile: same numbers, new interpretation. pub fn assign_profile(&mut self, profile: schist_colormgmt::Profile, cx: &mut Context) { if let Some(doc) = self.doc.as_mut() { - doc.icc_profile = profile.icc_bytes().map(|b| b.to_vec()); - doc.dirty = true; + let mut edit = doc.begin_edit(format!("Assign {}", profile.name())); + edit.set_icc_profile(profile.icc_bytes().map(|b| b.to_vec())); + edit.commit(); doc.damage_all(); } self.status = format!("Assigned {}", profile.name()).into(); @@ -4726,8 +4727,8 @@ impl Workspace { tile.encode_f32(&buf); } } + edit.set_icc_profile(profile.icc_bytes().map(|b| b.to_vec())); edit.commit(); - doc.icc_profile = profile.icc_bytes().map(|b| b.to_vec()); self.status = format!("Converted to {}", profile.name()).into(); self.rebuild_color_transforms(); self.after_change(cx); @@ -4735,10 +4736,14 @@ impl Workspace { /// Toggle soft proofing against a device profile. pub fn toggle_proof(&mut self, profile: schist_colormgmt::Profile, cx: &mut Context) { - self.color.proof = match &self.color.proof { - Some(_) => None, - None => Some(profile), - }; + // Picking a different proof profile while one is active switches + // to it; picking the active one turns proofing off. + let already_proofing_this = self + .color + .proof + .as_ref() + .is_some_and(|p| p.name() == profile.name()); + self.color.proof = (!already_proofing_this).then_some(profile); self.status = if self.color.proof.is_some() { "Proof colors on".into() } else { diff --git a/crates/codec-psd/src/writer/mod.rs b/crates/codec-psd/src/writer/mod.rs index 24d470b0..04ae79c5 100644 --- a/crates/codec-psd/src/writer/mod.rs +++ b/crates/codec-psd/src/writer/mod.rs @@ -127,6 +127,14 @@ fn write_image_resources(b: &mut Buf, doc: &Document) { continue; // written as its own section above } seen_resolution |= res.id == RES_RESOLUTION_INFO; + // `doc.icc_profile` is the authority when it is set. Convert and + // Assign Profile rewrite it, while the preserved resource still + // describes the space the file arrived in; re-emitting that one + // tagged converted pixels with the profile they were converted + // away from, and suppressed the correct tag below. + if res.id == RES_ICC_PROFILE && doc.icc_profile.is_some() { + continue; + } seen_icc |= res.id == RES_ICC_PROFILE; b.bytes(b"8BIM"); b.u16(res.id); diff --git a/crates/codec-psd/tests/writer.rs b/crates/codec-psd/tests/writer.rs index e38141fb..17018fd8 100644 --- a/crates/codec-psd/tests/writer.rs +++ b/crates/codec-psd/tests/writer.rs @@ -276,6 +276,69 @@ fn psd_keeps_u32_lengths_for_the_same_keys() { assert_eq!(block.data, b"preserved payload"); } +#[test] +fn a_converted_profile_replaces_the_preserved_one() { + // Convert/Assign Profile rewrite `doc.icc_profile` while the resource + // preserved from the source file still describes the old space. The + // writer re-emitted that one and suppressed the new tag, so converted + // pixels came back tagged with the profile they were converted away + // from. + let mut doc = base_doc(); + doc.push_layer(solid_layer( + "l", + IntRect::from_xywh(0, 0, 8, 8), + [1, 2, 3, 255], + Depth::Eight, + )); + // A file that arrived carrying one profile... + doc.preserved_resources.push(PreservedResource { + id: 0x040F, + name: Vec::new(), + data: b"OLD-PROFILE-BYTES".to_vec(), + }); + // ...and was then converted to another. + doc.icc_profile = Some(b"NEW-PROFILE-BYTES".to_vec()); + + let back = read_psd(&write_psd(&doc).unwrap()).unwrap(); + assert_eq!( + back.icc_profile.as_deref(), + Some(b"NEW-PROFILE-BYTES".as_slice()), + "the document's own profile must win" + ); + let iccs = back + .preserved_resources + .iter() + .filter(|r| r.id == 0x040F) + .count(); + assert!(iccs <= 1, "the stale profile must not be written alongside"); +} + +#[test] +fn an_untagged_document_keeps_a_preserved_profile() { + // The other direction: with no profile of its own, dropping the + // preserved resource would lose the file's colour space outright. + let mut doc = base_doc(); + doc.push_layer(solid_layer( + "l", + IntRect::from_xywh(0, 0, 8, 8), + [1, 2, 3, 255], + Depth::Eight, + )); + doc.preserved_resources.push(PreservedResource { + id: 0x040F, + name: Vec::new(), + data: b"ONLY-PROFILE".to_vec(), + }); + doc.icc_profile = None; + + let back = read_psd(&write_psd(&doc).unwrap()).unwrap(); + assert_eq!( + back.icc_profile.as_deref(), + Some(b"ONLY-PROFILE".as_slice()), + "the preserved profile must survive" + ); +} + #[test] fn preserves_image_resources_and_resolution() { let mut doc = base_doc(); diff --git a/crates/colormgmt/src/lib.rs b/crates/colormgmt/src/lib.rs index cda5e758..3237159a 100644 --- a/crates/colormgmt/src/lib.rs +++ b/crates/colormgmt/src/lib.rs @@ -94,18 +94,29 @@ impl Profile { } pub fn srgb() -> Profile { - Profile { - profile: Arc::new(ColorProfile::new_srgb()), - bytes: None, - name: "sRGB".into(), - } + Profile::builtin(ColorProfile::new_srgb(), "sRGB") } pub fn display_p3() -> Profile { + Profile::builtin(ColorProfile::new_display_p3(), "Display P3") + } + + /// A built-in profile, serialized so it can be embedded on save. + /// + /// These used to carry `bytes: None`, which made them unusable as + /// assignment targets: `icc_bytes()` returned `None`, so assigning + /// either of the two profiles the UI offers *untagged* the document + /// instead of tagging it, and the next open reinterpreted the pixels + /// against whatever the working space happened to be. + fn builtin(profile: ColorProfile, name: &str) -> Profile { + let bytes = profile.encode().ok().map(Arc::new); + if bytes.is_none() { + log::warn!("could not serialize the built-in {name} profile"); + } Profile { - profile: Arc::new(ColorProfile::new_display_p3()), - bytes: None, - name: "Display P3".into(), + profile: Arc::new(profile), + bytes, + name: name.into(), } } @@ -184,6 +195,14 @@ impl ColorTransform { /// Convert a straight-alpha f32 RGBA buffer in place. /// /// Alpha is carried through untouched: it is coverage, not colour. + /// Transform a straight-alpha f32 RGBA buffer in place. + /// + /// Note this cannot preserve extended range even on the editing path: + /// moxcms clamps to 0..1 while evaluating the transfer curves + /// (`gamma.rs`), so a 32-bit document's out-of-range highlights are + /// clipped by the CMS before we see the output. Removing the clamp + /// below would not change that; it needs either CMS support or a + /// matrix-only path that skips the curves. pub fn apply(&self, pixels: &mut [f32]) { if self.identity || pixels.is_empty() { return; @@ -194,8 +213,8 @@ impl ColorTransform { pixels.copy_from_slice(&src); return; } - // Extended-range output can exceed 0..1; clamp for display and - // restore alpha, which a matrix transform may have touched. + // Clamp for display and restore alpha, which a matrix transform + // may have touched. for (out, inp) in pixels .as_chunks_mut::<4>() .0 @@ -611,4 +630,37 @@ mod tests { .apply(&mut b); assert_eq!(a, b); } + #[test] + fn builtin_profiles_can_be_embedded() { + // `bytes: None` made these unusable as assignment targets: + // `assign_profile` writes `icc_bytes()` onto the document, so + // assigning either of the two profiles the UI offers untagged the + // document rather than tagging it. + for p in [Profile::srgb(), Profile::display_p3()] { + let bytes = p + .icc_bytes() + .unwrap_or_else(|| panic!("{} has no bytes", p.name)); + assert!(bytes.len() > 128, "{} icc is implausibly small", p.name); + assert_eq!(&bytes[36..40], b"acsp", "{} is not an icc profile", p.name); + // And it must round-trip back through the parser. + assert!( + Profile::from_bytes(bytes).is_ok(), + "{} did not parse back", + p.name + ); + } + } + + #[test] + fn conversion_still_leaves_alpha_alone() { + let mut px = vec![0.4f32, 0.2, 0.7, 0.33]; + convert_pixels( + &mut px, + &Profile::srgb(), + &Profile::display_p3(), + Intent::Perceptual, + ) + .unwrap(); + assert!((px[3] - 0.33).abs() < 1e-6, "alpha changed: {}", px[3]); + } } diff --git a/crates/core/src/document.rs b/crates/core/src/document.rs index 2f753bdd..dec7ebd7 100644 --- a/crates/core/src/document.rs +++ b/crates/core/src/document.rs @@ -439,6 +439,14 @@ impl Document { self.selection = (**target).clone(); self.revision += 1; } + EditOp::IccProfileSet { before, after } => { + let target = if dir == Direction::Undo { + before + } else { + after + }; + self.icc_profile = target.clone(); + } } } @@ -798,6 +806,24 @@ impl<'a> EditBuilder<'a> { } /// Replace the selection via closure; captures before/after. + /// Set the document's embedded ICC profile as part of this edit. + /// + /// Assign and Convert to Profile set it outside the edit, so undo + /// restored the pixels but left the new tag in place: the old numbers + /// were then interpreted under the wrong profile, which is worse than + /// either state on its own. + pub fn set_icc_profile(&mut self, profile: Option>) { + let before = self.doc.icc_profile.clone(); + if before == profile { + return; + } + self.doc.icc_profile = profile.clone(); + self.ops.push(EditOp::IccProfileSet { + before, + after: profile, + }); + } + pub fn change_selection(&mut self, f: impl FnOnce(&mut Selection, IntRect)) { let canvas = self.doc.canvas_rect(); let before = Box::new(self.doc.selection.clone()); @@ -1221,4 +1247,38 @@ mod tests { assert!(doc.revision > 0, "repaint was requested"); assert!(!doc.dirty, "but nothing was actually changed"); } + + #[test] + fn the_icc_profile_undoes_with_the_pixels() { + // Convert to Profile rewrites the pixels *and* the tag. Setting + // the tag outside the edit meant undo restored the old numbers + // while leaving the new profile on them, which reads as a + // different colour than either state. + let mut doc = Document::new("t", 32, 32, Depth::Eight); + doc.icc_profile = Some(b"OLD".to_vec()); + + let mut edit = doc.begin_edit("Convert"); + edit.set_icc_profile(Some(b"NEW".to_vec())); + edit.commit(); + assert_eq!(doc.icc_profile.as_deref(), Some(b"NEW".as_slice())); + + doc.undo(); + assert_eq!( + doc.icc_profile.as_deref(), + Some(b"OLD".as_slice()), + "undo must put the original tag back" + ); + doc.redo(); + assert_eq!(doc.icc_profile.as_deref(), Some(b"NEW".as_slice())); + } + + #[test] + fn setting_the_same_profile_records_nothing() { + let mut doc = Document::new("t", 32, 32, Depth::Eight); + doc.icc_profile = Some(b"SAME".to_vec()); + let mut edit = doc.begin_edit("Assign"); + edit.set_icc_profile(Some(b"SAME".to_vec())); + edit.commit(); + assert!(!doc.history.can_undo(), "a no-op must not enter history"); + } } diff --git a/crates/core/src/history.rs b/crates/core/src/history.rs index be8ae895..1e567bcc 100644 --- a/crates/core/src/history.rs +++ b/crates/core/src/history.rs @@ -117,6 +117,14 @@ pub enum EditOp { before: Box, after: Box, }, + /// The document's embedded ICC profile changed (Assign / Convert to + /// Profile). Rides in the same edit as the pixel rewrite so undo puts + /// the pixels and their tag back together; undoing only the pixels + /// left the old numbers interpreted under the new profile. + IccProfileSet { + before: Option>, + after: Option>, + }, } #[derive(Debug, Clone)] From 39f8533d6bbfc56b2c5198244b20b2f7624d7333 Mon Sep 17 00:00:00 2001 From: Rahul A Mistry Date: Wed, 26 Aug 2026 06:21:27 +0530 Subject: [PATCH 2/4] fix: honour group fill opacity and adjustment blend modes (cherry picked from commit e7761e7dd10a59707c9e513fa24be96fe125f74e) --- crates/compositor-gpu/src/composite.wgsl | 12 ++- crates/compositor-gpu/src/plan.rs | 6 +- crates/compositor/src/lib.rs | 120 +++++++++++++++++++++-- 3 files changed, 130 insertions(+), 8 deletions(-) diff --git a/crates/compositor-gpu/src/composite.wgsl b/crates/compositor-gpu/src/composite.wgsl index 64913d69..7ae82207 100644 --- a/crates/compositor-gpu/src/composite.wgsl +++ b/crates/compositor-gpu/src/composite.wgsl @@ -652,7 +652,17 @@ fn composite(@builtin(global_invocation_id) gid: vec3) { d.rgb, ); } - stack[sp - 1u] = vec4(d.rgb + (adjusted - d.rgb) * weight, d.a); + // Mirrors the CPU compositor: the adjustment's + // own blend mode applies, with the adjusted colour + // as the source and `weight` as its alpha. It used + // to be uploaded and then ignored, so every + // adjustment rendered as Normal. + if (op.mode == M_NORMAL) { + stack[sp - 1u] = vec4(d.rgb + (adjusted - d.rgb) * weight, d.a); + } else { + let blended = blend_px(op.mode, vec4(adjusted, weight), d, x, y); + stack[sp - 1u] = vec4(blended.rgb, d.a); + } } } } diff --git a/crates/compositor-gpu/src/plan.rs b/crates/compositor-gpu/src/plan.rs index 32bf15a6..731776ee 100644 --- a/crates/compositor-gpu/src/plan.rs +++ b/crates/compositor-gpu/src/plan.rs @@ -280,8 +280,12 @@ fn emit_layers<'a>( op.opacity = layer.opacity * content_alpha(layer); } LayerKind::Group(g) => { + // Mirrors the CPU compositor: fill opacity is part of + // the pass-through test, or a group at fill 50% would + // render its children at full strength. let pass_through = layer.blend == BlendMode::PassThrough && layer.opacity >= 1.0 + && content_alpha(layer) >= 1.0 && layer.mask.is_none(); if pass_through { emit_layers(&g.children, plan, depth)?; @@ -297,7 +301,7 @@ fn emit_layers<'a>( let mask = plan.mask_ref(layer); let op = plan.op(OP_BLEND); op.mode = mode_id(mode); - op.opacity = layer.opacity; + op.opacity = layer.opacity * content_alpha(layer); op.mask = mask; } } diff --git a/crates/compositor/src/lib.rs b/crates/compositor/src/lib.rs index f4033a48..708aa57c 100644 --- a/crates/compositor/src/lib.rs +++ b/crates/compositor/src/lib.rs @@ -408,8 +408,12 @@ fn composite_layers( scratch.give(src); } LayerKind::Group(g) => { + // Fill opacity has to be part of this: a pass-through + // group renders its children straight into `dst`, so + // a group at fill 50% was drawn at full strength. let pass_through = layer.blend == BlendMode::PassThrough && layer.opacity >= 1.0 + && content_alpha(layer) >= 1.0 && layer.mask.is_none(); if pass_through { composite_layers(doc, &g.children, coord, dst, scratch); @@ -421,7 +425,20 @@ fn composite_layers( } else { layer.blend }; - blend_buf_onto(mode, &group_buf, dst, coord, layer.opacity, layer, doc); + // `content_alpha` is fill opacity. Every other + // blend in this file multiplies it in; the + // isolated-group path did not, so the Fill slider + // did nothing on a group unless it happened to be + // a clip base, where a different path applied it. + blend_buf_onto( + mode, + &group_buf, + dst, + coord, + layer.opacity * content_alpha(layer), + layer, + doc, + ); scratch.give(group_buf); } } @@ -513,11 +530,34 @@ fn apply_adjustment( blend_pixel(layer.blend, Rgba { a: weight, ..src }, base, x, y) } else { let adjusted = params.apply(base); - Rgba { - r: base.r + (adjusted.r - base.r) * weight, - g: base.g + (adjusted.g - base.g) * weight, - b: base.b + (adjusted.b - base.b) * weight, - a: base.a, + // The blend mode was read from the file, stored on the layer + // and uploaded to the shader, then dropped here: every + // "Curves set to Luminosity" or "Levels set to Multiply" + // rendered as Normal, with nothing to say it had been ignored. + if layer.blend == BlendMode::Normal { + Rgba { + r: base.r + (adjusted.r - base.r) * weight, + g: base.g + (adjusted.g - base.g) * weight, + b: base.b + (adjusted.b - base.b) * weight, + a: base.a, + } + } else { + // The adjusted colour is the source, `weight` its alpha, + // exactly as the fill path above treats its own colour. + let blended = blend_pixel( + layer.blend, + Rgba { + a: weight, + ..adjusted + }, + base, + x, + y, + ); + Rgba { + a: base.a, + ..blended + } } }; d[0] = out.r; @@ -946,6 +986,74 @@ mod tests { .select_rect(IntRect::from_xywh(0, 0, 8, 8), SelectOp::Replace); assert_eq!(px(&doc, 20, 20), [5, 6, 7, 255]); } + + #[test] + fn a_groups_fill_opacity_is_honoured() { + // Every blend in this file multiplies in `content_alpha` (fill + // opacity) except the isolated-group one, so the Fill slider did + // nothing on a group unless it happened to be a clip base, where + // a different path applied it. + let build = |fill: f32| { + let mut doc = Document::new("t", 8, 8, Depth::Eight); + // An opaque backdrop, so the group's fill shows as tone + // rather than only as alpha. + doc.push_layer(solid_layer( + "black", + IntRect::from_xywh(0, 0, 8, 8), + [0, 0, 0, 255], + )); + let child = solid_layer( + "white", + IntRect::from_xywh(0, 0, 8, 8), + [255, 255, 255, 255], + ); + let mut group = Layer::new_group("g"); + if let LayerKind::Group(g) = &mut group.kind { + g.children.push(child); + } + group.fill_opacity = fill; + doc.push_layer(group); + px(&doc, 4, 4) + }; + let full = build(1.0); + let half = build(0.5); + assert_ne!(full, half, "fill opacity must change the group"); + assert!(half[0] < full[0], "half fill must be dimmer: {half:?}"); + } + + #[test] + fn an_adjustment_layers_blend_mode_is_used() { + // Parsed, stored, round-tripped to PSD and uploaded to the shader, + // then dropped here: every "Curves set to Luminosity" rendered as + // Normal, with nothing to say it had been ignored. + let build = |mode: BlendMode| { + let mut doc = Document::new("t", 8, 8, Depth::Eight); + doc.push_layer(solid_layer( + "grey", + IntRect::from_xywh(0, 0, 8, 8), + [128, 128, 128, 255], + )); + let mut adj = Layer::new_raster("invert"); + adj.kind = LayerKind::Adjustment(schist_core::AdjustmentData { + kind: schist_core::AdjustmentKind::Invert, + raw: Vec::new(), + params_json: Some("\"Invert\"".into()), + }); + adj.blend = mode; + doc.push_layer(adj); + px(&doc, 4, 4) + }; + let normal = build(BlendMode::Normal); + let multiply = build(BlendMode::Multiply); + assert_ne!( + normal, multiply, + "the blend mode must reach the result (both {normal:?})" + ); + assert!( + multiply[0] < normal[0], + "multiply must darken: {multiply:?} vs {normal:?}" + ); + } } #[cfg(test)] From 7c2a8d49309fa87f35f567ebe9ea09187c282097 Mon Sep 17 00:00:00 2001 From: Rahul A Mistry Date: Thu, 27 Aug 2026 14:19:56 +0530 Subject: [PATCH 3/4] fix: repaint and rebuild transforms when a profile edit is undone --- crates/app/src/workspace.rs | 21 ++++++++++++++++----- crates/core/src/document.rs | 31 ++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/crates/app/src/workspace.rs b/crates/app/src/workspace.rs index 7cdc3f59..3d095062 100644 --- a/crates/app/src/workspace.rs +++ b/crates/app/src/workspace.rs @@ -4251,6 +4251,7 @@ impl Workspace { /// Jump in history: negative = undo n steps, positive = redo n steps. pub fn history_jump(&mut self, steps: i32, cx: &mut Context) { + let profile_before = self.doc.as_ref().and_then(|d| d.icc_profile.clone()); if let Some(doc) = &mut self.doc { if steps < 0 { for _ in 0..(-steps) { @@ -4266,6 +4267,13 @@ impl Workspace { } } } + // The display transform is cached per document and only rebuilt + // where a profile is *set*, so undoing an Assign or Convert left + // the canvas rendering through the transform for the profile that + // was just undone away. + if self.doc.as_ref().and_then(|d| d.icc_profile.clone()) != profile_before { + self.rebuild_color_transforms(); + } self.after_change(cx); } @@ -4738,11 +4746,14 @@ impl Workspace { pub fn toggle_proof(&mut self, profile: schist_colormgmt::Profile, cx: &mut Context) { // Picking a different proof profile while one is active switches // to it; picking the active one turns proofing off. - let already_proofing_this = self - .color - .proof - .as_ref() - .is_some_and(|p| p.name() == profile.name()); + // Compare the profiles themselves, not their names: every + // embedded profile parses back as "Embedded profile", so two + // different device profiles would look identical and switching + // between them would turn proofing off instead. + let already_proofing_this = + self.color.proof.as_ref().is_some_and(|p| { + p.icc_bytes() == profile.icc_bytes() && p.name() == profile.name() + }); self.color.proof = (!already_proofing_this).then_some(profile); self.status = if self.color.proof.is_some() { "Proof colors on".into() diff --git a/crates/core/src/document.rs b/crates/core/src/document.rs index dec7ebd7..8278a790 100644 --- a/crates/core/src/document.rs +++ b/crates/core/src/document.rs @@ -446,6 +446,11 @@ impl Document { after }; self.icc_profile = target.clone(); + // The profile decides what every pixel means, so the whole + // canvas has to repaint. Without this the restored pixels + // kept rendering through the transform built for the other + // profile, and a pure Assign did not repaint at all. + self.damage_all(); } } } @@ -805,7 +810,6 @@ impl<'a> EditBuilder<'a> { } } - /// Replace the selection via closure; captures before/after. /// Set the document's embedded ICC profile as part of this edit. /// /// Assign and Convert to Profile set it outside the edit, so undo @@ -824,6 +828,7 @@ impl<'a> EditBuilder<'a> { }); } + /// Replace the selection via closure; captures before/after. pub fn change_selection(&mut self, f: impl FnOnce(&mut Selection, IntRect)) { let canvas = self.doc.canvas_rect(); let before = Box::new(self.doc.selection.clone()); @@ -1281,4 +1286,28 @@ mod tests { edit.commit(); assert!(!doc.history.can_undo(), "a no-op must not enter history"); } + /// Undoing a profile change has to repaint: the profile decides what + /// every pixel means, and without damage the canvas kept rendering + /// the restored pixels through the transform built for the profile + /// that had just been undone away. + #[test] + fn undoing_a_profile_change_damages_the_canvas() { + let mut doc = Document::new("t", 32, 32, Depth::Eight); + doc.push_layer(Layer::new_raster("bg")); + let mut edit = doc.begin_edit("Assign Profile"); + edit.set_icc_profile(Some(vec![1, 2, 3, 4])); + assert!(edit.commit()); + doc.take_damage(); + + doc.undo().unwrap(); + assert!(doc.icc_profile.is_none(), "the profile was restored"); + assert!( + !doc.take_damage().is_empty(), + "undo left nothing for the canvas to repaint" + ); + + doc.redo().unwrap(); + assert_eq!(doc.icc_profile.as_deref(), Some(&[1, 2, 3, 4][..])); + assert!(!doc.take_damage().is_empty(), "redo repaints too"); + } } From 04bb10969419fda3bdfb8593a1a65c42018e32de Mon Sep 17 00:00:00 2001 From: Rahul A Mistry Date: Thu, 27 Aug 2026 15:04:07 +0530 Subject: [PATCH 4/4] fix: move the soft proof hop here and keep the psd resource layout --- crates/codec-psd/src/writer/mod.rs | 23 +++++-- crates/codec-psd/tests/writer.rs | 39 ++++++++++++ crates/colormgmt/src/lib.rs | 97 ++++++++++++++++++++++++------ 3 files changed, 136 insertions(+), 23 deletions(-) diff --git a/crates/codec-psd/src/writer/mod.rs b/crates/codec-psd/src/writer/mod.rs index 04ae79c5..7bfaabf9 100644 --- a/crates/codec-psd/src/writer/mod.rs +++ b/crates/codec-psd/src/writer/mod.rs @@ -131,11 +131,22 @@ fn write_image_resources(b: &mut Buf, doc: &Document) { // Assign Profile rewrite it, while the preserved resource still // describes the space the file arrived in; re-emitting that one // tagged converted pixels with the profile they were converted - // away from, and suppressed the correct tag below. - if res.id == RES_ICC_PROFILE && doc.icc_profile.is_some() { - continue; + // away from. + // + // It goes in the preserved block's own place rather than being + // appended at the end. The reader always mirrors ICC into + // `doc.icc_profile`, so the substitution fires on every round + // trip, and moving the tag rewrote the resource section's layout + // for a file nothing had actually changed. + let mut data: &[u8] = &res.data; + if res.id == RES_ICC_PROFILE { + seen_icc = true; + // With no profile of its own the document keeps whatever the + // file arrived with, rather than losing its colour space. + if let Some(icc) = &doc.icc_profile { + data = icc; + } } - seen_icc |= res.id == RES_ICC_PROFILE; b.bytes(b"8BIM"); b.u16(res.id); // `name` holds the raw pascal bytes (length byte + content + pad) @@ -148,8 +159,8 @@ fn write_image_resources(b: &mut Buf, doc: &Document) { b.u8(0); } } - b.u32(res.data.len() as u32); - b.bytes(&res.data); + b.u32(data.len() as u32); + b.bytes(data); b.pad_to(2); } diff --git a/crates/codec-psd/tests/writer.rs b/crates/codec-psd/tests/writer.rs index 17018fd8..59928ab1 100644 --- a/crates/codec-psd/tests/writer.rs +++ b/crates/codec-psd/tests/writer.rs @@ -339,6 +339,45 @@ fn an_untagged_document_keeps_a_preserved_profile() { ); } +#[test] +fn a_round_trip_leaves_the_resource_section_where_it_was() { + // The reader always mirrors ICC into `doc.icc_profile`, so the + // substitution above fires on every round trip. Appending the tag at + // the end instead of writing it in the preserved block's own place + // rewrote the resource layout of a file nothing had changed. + let mut doc = base_doc(); + doc.push_layer(solid_layer( + "l", + IntRect::from_xywh(0, 0, 8, 8), + [1, 2, 3, 255], + Depth::Eight, + )); + doc.preserved_resources.push(PreservedResource { + id: 0x040F, + name: Vec::new(), + data: b"THE-PROFILE".to_vec(), + }); + doc.preserved_resources.push(PreservedResource { + id: 0x0421, + name: Vec::new(), + data: b"after-the-profile".to_vec(), + }); + doc.icc_profile = Some(b"THE-PROFILE".to_vec()); + + let once = write_psd(&doc).unwrap(); + let twice = write_psd(&read_psd(&once).unwrap()).unwrap(); + assert_eq!(once, twice, "saving again moved bytes around"); + + let back = read_psd(&once).unwrap(); + let ids: Vec = back.preserved_resources.iter().map(|r| r.id).collect(); + let icc = ids.iter().position(|id| *id == 0x040F).expect("icc kept"); + let after = ids + .iter() + .position(|id| *id == 0x0421) + .expect("the block after it kept"); + assert!(icc < after, "the profile moved to the end: {ids:?}"); +} + #[test] fn preserves_image_resources_and_resolution() { let mut doc = base_doc(); diff --git a/crates/colormgmt/src/lib.rs b/crates/colormgmt/src/lib.rs index 3237159a..308a9524 100644 --- a/crates/colormgmt/src/lib.rs +++ b/crates/colormgmt/src/lib.rs @@ -195,7 +195,6 @@ impl ColorTransform { /// Convert a straight-alpha f32 RGBA buffer in place. /// /// Alpha is carried through untouched: it is coverage, not colour. - /// Transform a straight-alpha f32 RGBA buffer in place. /// /// Note this cannot preserve extended range even on the editing path: /// moxcms clamps to 0..1 while evaluating the transfer curves @@ -253,20 +252,19 @@ impl Default for ColorSettings { } impl ColorSettings { - /// Build the transform for a document with the given embedded profile. + /// Build the display hop for a document with the given embedded + /// profile. /// - /// Soft proofing runs document→proof→display; the two hops are baked - /// into one executor chain by applying them in sequence. + /// Soft proofing runs document → proof → display, applied in + /// sequence. The second hop therefore starts at the *proof* profile: + /// building it from the document profile, as this used to, converts + /// from a space the pixels already left, so Proof Colors was doubly + /// wrong whenever the display profile differed from the document's -- + /// and people make colour decisions against that view. pub fn transform_for(&self, document_icc: Option<&[u8]>) -> ColorTransform { - let source = match document_icc { - Some(bytes) => match Profile::from_bytes(bytes) { - Ok(p) => p, - Err(err) => { - log::warn!("{err:#}; falling back to the working space"); - self.working.clone() - } - }, - None => self.working.clone(), + let source = match &self.proof { + Some(proof) => proof.clone(), + None => self.document_profile(document_icc), }; match ColorTransform::new(&source, &self.display, self.intent) { Ok(t) => t, @@ -280,14 +278,26 @@ impl ColorSettings { /// The proofing hop, if soft proofing is on. pub fn proof_transform(&self, document_icc: Option<&[u8]>) -> Option { let proof = self.proof.as_ref()?; - let source = match document_icc { - Some(bytes) => Profile::from_bytes(bytes).unwrap_or_else(|_| self.working.clone()), - None => self.working.clone(), - }; + let source = self.document_profile(document_icc); // Proofing is colorimetric by definition: it must show the target's // gamut clipping rather than re-map it pleasingly. ColorTransform::new(&source, proof, Intent::RelativeColorimetric).ok() } + + /// The document's own profile, or the working space when it has none + /// or carries one we cannot read. + fn document_profile(&self, document_icc: Option<&[u8]>) -> Profile { + match document_icc { + Some(bytes) => match Profile::from_bytes(bytes) { + Ok(p) => p, + Err(err) => { + log::warn!("{err:#}; falling back to the working space"); + self.working.clone() + } + }, + None => self.working.clone(), + } + } } /// Bake BT.2100 HDR pixels (PQ or HLG signal, straight-alpha RGBA f32) @@ -651,6 +661,42 @@ mod tests { } } + /// Proofing to the very profile the display uses must show exactly + /// what an unproofed document→display conversion shows: the proof hop + /// takes the pixels to P3 and the display hop then has nothing left + /// to do. Building the display hop from the *document* profile + /// instead -- as it used to -- runs sRGB→P3 a second time over pixels + /// that are already P3, so Proof Colors was doubly wrong whenever the + /// display profile differed from the document's, and people make + /// colour decisions against that view. + #[test] + fn the_display_hop_starts_where_the_proof_hop_ended() { + let mut proofed = [0.8f32, 0.2, 0.1, 1.0]; + let mut direct = proofed; + + let proofing = ColorSettings { + working: Profile::srgb(), + display: Profile::display_p3(), + intent: Intent::Perceptual, + proof: Some(Profile::display_p3()), + }; + proofing.proof_transform(None).unwrap().apply(&mut proofed); + proofing.transform_for(None).apply(&mut proofed); + + let plain = ColorSettings { + proof: None, + ..proofing + }; + plain.transform_for(None).apply(&mut direct); + + for (got, want) in proofed.iter().zip(&direct) { + assert!( + (got - want).abs() < 1e-3, + "proof + display applied a second conversion: {proofed:?} vs {direct:?}" + ); + } + } + #[test] fn conversion_still_leaves_alpha_alone() { let mut px = vec![0.4f32, 0.2, 0.7, 0.33]; @@ -663,4 +709,21 @@ mod tests { .unwrap(); assert!((px[3] - 0.33).abs() < 1e-6, "alpha changed: {}", px[3]); } + + /// With proofing off, the display hop is still document → display. + #[test] + fn without_proofing_the_display_hop_is_unchanged() { + let settings = ColorSettings { + working: Profile::srgb(), + display: Profile::display_p3(), + intent: Intent::Perceptual, + proof: None, + }; + let mut pixels = [0.8f32, 0.2, 0.1, 1.0]; + settings.transform_for(None).apply(&mut pixels); + assert!( + (pixels[0] - 0.8).abs() > 1e-3 || (pixels[1] - 0.2).abs() > 1e-3, + "sRGB to Display P3 should have moved the pixel" + ); + } }