From 0936f60340075e02ac34fcfca6abe5bf6c47968a Mon Sep 17 00:00:00 2001 From: Rahul A Mistry Date: Wed, 26 Aug 2026 06:05:57 +0530 Subject: [PATCH 01/10] fix: cap decoded element count in the affinity object graph (cherry picked from commit 1b7cc7bc6f32d68f1ff671172702f2fef9651b30) --- crates/codec-affinity/src/graph.rs | 71 ++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/crates/codec-affinity/src/graph.rs b/crates/codec-affinity/src/graph.rs index ee77c06d..d3bc1dd1 100644 --- a/crates/codec-affinity/src/graph.rs +++ b/crates/codec-affinity/src/graph.rs @@ -38,6 +38,14 @@ pub fn tag_name(t: u32) -> String { .collect() } +/// Ceiling on how many elements one decoded array may hold. +/// +/// Packed formats let a small amount of input describe a large number of +/// values: a boolean array stores eight per byte, so the decoded `Vec` can +/// be hundreds of times the size of the bytes it came from. Real Affinity +/// graphs sit far below this. +const MAX_ARRAY_ELEMENTS: usize = 1 << 24; + /// One field value. Arrays of anything become [`Value::Array`]. #[derive(Debug, Clone, PartialEq)] pub enum Value { @@ -394,6 +402,20 @@ impl Parser<'_> { return Ok(Value::Bool(byte != 0)); } let count = self.c.u32()? as usize; + // `take` bounds the *input*, but each packed bit expands into a + // `Value` in the output, so one byte of file becomes eight enum + // values. That is roughly a 256x amplification, enough for a + // 100 MB file to ask for tens of gigabytes. + // + // Deliberately not the `count > remaining` check the sibling array + // readers use: eight booleans legitimately fit in one byte, so + // that test would reject valid files. The cap is on the decoded + // element count instead. + if count > MAX_ARRAY_ELEMENTS { + return Err(malformed(format!( + "boolean array of {count} elements is over the {MAX_ARRAY_ELEMENTS} limit" + ))); + } let bytes = self.c.take(count.div_ceil(8))?; Ok(Value::Array( (0..count) @@ -605,3 +627,52 @@ impl Parser<'_> { self.nodes.len() - 1 } } + +#[cfg(test)] +mod tests { + use super::*; + + /// A parser positioned at the field bytes for one packed boolean array. + fn parser(bytes: &[u8]) -> Parser<'_> { + Parser { + c: Cursor::new(bytes), + nodes: Vec::new(), + by_id: HashMap::new(), + depth: 0, + aux: 0, + } + } + + fn bool_array(count: u32, packed: &[u8]) -> Vec { + let mut b = count.to_le_bytes().to_vec(); + b.extend_from_slice(packed); + b + } + + #[test] + fn a_huge_boolean_array_is_rejected() { + // Eight bools per byte means a small file describes an enormous + // decoded Vec: one input byte becomes eight `Value`s, so the + // output is hundreds of times the input. + let bytes = bool_array(u32::MAX, &[0xFF; 8]); + assert!(matches!( + parser(&bytes).bools(true), + Err(AffinityError::Malformed(_)) + )); + } + + #[test] + fn an_ordinary_boolean_array_still_decodes() { + // The `count > remaining` guard the sibling readers use would + // reject this: eight booleans legitimately arrive in one byte. + let bytes = bool_array(8, &[0b1010_1010]); + match parser(&bytes).bools(true).expect("valid array") { + Value::Array(v) => { + assert_eq!(v.len(), 8); + assert_eq!(v[0], Value::Bool(false)); + assert_eq!(v[1], Value::Bool(true)); + } + other => panic!("got {other:?}"), + } + } +} From eab7176719bdfd6bc73e5163be0a949bda9be794 Mon Sep 17 00:00:00 2001 From: Rahul A Mistry Date: Wed, 26 Aug 2026 06:48:24 +0530 Subject: [PATCH 02/10] fix: stop descriptor parsing on an unknown value type (cherry picked from commit d23e6af57075dc215bfe1c8207a4689277037d9c) --- crates/psd-descriptor/src/lib.rs | 77 ++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 4 deletions(-) diff --git a/crates/psd-descriptor/src/lib.rs b/crates/psd-descriptor/src/lib.rs index f8121ef1..f3d16723 100644 --- a/crates/psd-descriptor/src/lib.rs +++ b/crates/psd-descriptor/src/lib.rs @@ -150,13 +150,24 @@ pub fn parse_versioned(data: &[u8]) -> Option { read_descriptor(&mut cur) } +/// Most items one descriptor or list may hold. Real Photoshop descriptors +/// are far smaller; a count past this means the stream is not what we +/// think it is. +const MAX_ITEMS: usize = 4096; + fn read_descriptor(cur: &mut Cur) -> Option { let _name = cur.unicode()?; let class = cur.key()?; let count = cur.u32()? as usize; + // A corrupt count claiming millions of entries used to be truncated to + // 4096 and parsing continued, which left the cursor mid-stream: a + // nested descriptor's parent then read the remains as its own fields. + // An over-count is a parse failure, not something to carry on from. + if count > MAX_ITEMS { + return None; + } let mut items = HashMap::new(); - // Guard against corrupt counts claiming millions of entries. - for _ in 0..count.min(4096) { + for _ in 0..count { let key = cur.key()?; let value = read_value(cur)?; items.insert(key, value); @@ -182,8 +193,11 @@ fn read_value(cur: &mut Cur) -> Option { } "VlLs" => { let count = cur.u32()? as usize; + if count > MAX_ITEMS { + return None; + } let mut out = Vec::new(); - for _ in 0..count.min(4096) { + for _ in 0..count { out.push(read_value(cur)?); } Value::List(out) @@ -192,7 +206,12 @@ fn read_value(cur: &mut Cur) -> Option { // Types we don't need: consume their fixed payloads so the walk // stays in sync, or bail out if the size isn't knowable. "obj " | "type" | "GlbC" | "alis" | "tdta" => return None, - _ => Value::Unknown, + // An unrecognised signature has an unknown payload size, so the + // cursor is now pointing into the middle of it. Carrying on read + // that payload as the next key and value, and the nonsense was + // then re-encoded on save. Bail out the way the known-unhandled + // types above already do. + _ => return None, }) } @@ -551,4 +570,54 @@ mod encode_tests { let d = parse(&b.finish()).unwrap(); assert_eq!(d.get("masterFXSwitch").unwrap().as_bool(), Some(true)); } + /// A descriptor whose first value has an unrecognised type, and whose + /// payload happens to spell a valid key/value pair. + /// + /// That is the case that matters: skipping the signature but not the + /// payload let the walk read the payload as the *next* item and carry + /// on, so the parse succeeded with values that were never in the file. + fn descriptor_with_unknown_type() -> Vec { + let mut b = Vec::new(); + b.extend_from_slice(&0u32.to_be_bytes()); // empty unicode name + b.extend_from_slice(&0u32.to_be_bytes()); // class key length + b.extend_from_slice(b"null"); + b.extend_from_slice(&2u32.to_be_bytes()); // two items + // Item 1: an unknown value type whose 16-byte payload reads as a + // complete key + long value. + b.extend_from_slice(&0u32.to_be_bytes()); + b.extend_from_slice(b"Ky01"); + b.extend_from_slice(b"ZZZZ"); + b.extend_from_slice(&0u32.to_be_bytes()); + b.extend_from_slice(b"Ky02"); + b.extend_from_slice(b"long"); + b.extend_from_slice(&7i32.to_be_bytes()); + b + } + + #[test] + fn an_unknown_value_type_stops_rather_than_desyncing() { + // Previously the cursor consumed the 4-byte signature but not the + // payload, so the walk read that payload as the next item and + // returned a descriptor holding a value the file never contained. + // A layer style or SoCo fill using an unhandled type therefore + // came back with arbitrary colours and offsets, which were then + // re-encoded on save. + let bytes = descriptor_with_unknown_type(); + let parsed = parse(&bytes); + assert!( + parsed.is_none(), + "an unknown type must fail the parse rather than inventing \ + items, got {parsed:?}" + ); + } + + #[test] + fn an_absurd_item_count_fails_rather_than_truncating() { + let mut b = Vec::new(); + b.extend_from_slice(&0u32.to_be_bytes()); + b.extend_from_slice(&0u32.to_be_bytes()); + b.extend_from_slice(b"null"); + b.extend_from_slice(&u32::MAX.to_be_bytes()); + assert!(parse(&b).is_none(), "an over-count must not be truncated"); + } } From d1c03018bfb7dfb1b349dba3c6ed40bbe999a2fe Mon Sep 17 00:00:00 2001 From: Rahul A Mistry Date: Wed, 26 Aug 2026 06:55:41 +0530 Subject: [PATCH 03/10] fix: refuse impossible layer counts and truncate names on a char boundary (cherry picked from commit 4a6b48ccb36a0c918d662ae62b9ff78612acf36e) --- crates/codec-psd/src/writer/buf.rs | 34 +++++++++++++++++++++++++++++- crates/codec-psd/src/writer/mod.rs | 11 ++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/crates/codec-psd/src/writer/buf.rs b/crates/codec-psd/src/writer/buf.rs index 3e5d7c00..a0cfbb50 100644 --- a/crates/codec-psd/src/writer/buf.rs +++ b/crates/codec-psd/src/writer/buf.rs @@ -68,7 +68,14 @@ impl Buf { /// PSD layer names are padded to 4, image-resource names to 2. pub fn pascal(&mut self, s: &str, align: usize) { let bytes = s.as_bytes(); - let n = bytes.len().min(255); + // Truncate on a char boundary. Cutting at byte 255 could land + // mid-codepoint, and this is the name non-unicode-aware readers + // show, so it would render as a replacement character there. The + // real name is regenerated as `luni` either way. + let mut n = bytes.len().min(255); + while n > 0 && !s.is_char_boundary(n) { + n -= 1; + } let start = self.data.len(); self.data.push(n as u8); self.data.extend_from_slice(&bytes[..n]); @@ -153,4 +160,29 @@ mod tests { let v = b.into_vec(); assert_eq!(&v[..8], &[0, 0, 0, 0, 0, 0, 0, 2]); } + #[test] + fn a_long_name_truncates_on_a_char_boundary() { + // The pascal name is what non-unicode-aware readers show, and a + // cut at byte 255 could land inside a codepoint. + let mut b = Buf::new(); + // 128 two-byte chars is 256 bytes: the 255 limit lands mid-char. + let name: String = std::iter::repeat_n('é', 128).collect(); + assert_eq!(name.len(), 256); + b.pascal(&name, 2); + let out = b.into_vec(); + let n = out[0] as usize; + assert_eq!(n % 2, 0, "must not cut a two-byte char in half"); + assert!( + std::str::from_utf8(&out[1..1 + n]).is_ok(), + "the truncated name must still be valid utf-8" + ); + } + + #[test] + fn an_ascii_name_is_unaffected() { + let mut b = Buf::new(); + b.pascal("Background", 2); + let out = b.into_vec(); + assert_eq!(out[0] as usize, "Background".len()); + } } diff --git a/crates/codec-psd/src/writer/mod.rs b/crates/codec-psd/src/writer/mod.rs index 24d470b0..23a80622 100644 --- a/crates/codec-psd/src/writer/mod.rs +++ b/crates/codec-psd/src/writer/mod.rs @@ -241,6 +241,17 @@ fn write_layer_and_mask_info(b: &mut Buf, doc: &Document, psb: bool) -> Result<( let layer_info_at = b.reserve_len(psb); // A negative count declares that the merged image's alpha channel is // real transparency rather than a spot/alpha channel. + // + // `prepared` includes two extra records per group, so past 32767 + // entries this cast wrapped and the file declared a nonsense layer + // count. Refuse instead of writing something unreadable. + if prepared.len() > i16::MAX as usize { + return Err(PsdError::Unsupported(format!( + "{} layer records exceeds the {} the format can declare", + prepared.len(), + i16::MAX + ))); + } b.i16(-(prepared.len() as i16)); for p in &prepared { write_layer_record(b, p, psb); From 03596ff71de5e2d9c38aa297dbe3ec896ae8eb56 Mon Sep 17 00:00:00 2001 From: Rahul A Mistry Date: Wed, 26 Aug 2026 06:02:56 +0530 Subject: [PATCH 04/10] fix: write adjustment layer settings when saving a psd (cherry picked from commit df05a6d34212b4dd183ad486bb24e459eaeec611) --- crates/adjustments/src/lib.rs | 252 +++++++++++++++++++++++++++++ crates/codec-psd/Cargo.toml | 3 + crates/codec-psd/src/writer/mod.rs | 36 ++++- crates/codec-psd/tests/writer.rs | 68 ++++++++ crates/core/src/layer.rs | 28 ++++ 5 files changed, 385 insertions(+), 2 deletions(-) diff --git a/crates/adjustments/src/lib.rs b/crates/adjustments/src/lib.rs index e788b568..76f468fd 100644 --- a/crates/adjustments/src/lib.rs +++ b/crates/adjustments/src/lib.rs @@ -917,6 +917,151 @@ pub fn parse_psd(kind: AdjustmentKind, raw: &[u8]) -> Params { } } +/// Encode `params` back into the PSD block payload for `kind`. +/// +/// Returns `None` for kinds this crate cannot round-trip; the caller then +/// falls back to whatever raw bytes the file arrived with. +/// +/// Adjustment layers created in Schist carry their settings only in +/// `params_json`, which no PSD reader understands. Without an encoder the +/// writer emitted an empty raster layer, so every adjustment layer a user +/// made was destroyed by saving. Each encoder here is the exact inverse of +/// the parser above it, which is what the round-trip tests check. +pub fn encode_psd(kind: AdjustmentKind, params: &Params) -> Option> { + match (kind, params) { + (AdjustmentKind::Invert, Params::Invert) => Some(Vec::new()), + (AdjustmentKind::Posterize, Params::Posterize { levels }) => { + Some((*levels as u16).to_be_bytes().to_vec()) + } + (AdjustmentKind::Threshold, Params::Threshold { level }) => { + let v = (level * 255.0).round().clamp(0.0, 65535.0) as u16; + Some(v.to_be_bytes().to_vec()) + } + ( + AdjustmentKind::BrightnessContrast, + Params::BrightnessContrast { + brightness, + contrast, + }, + ) => { + let mut out = Vec::with_capacity(7); + out.extend_from_slice(&(brightness.round() as i16).to_be_bytes()); + out.extend_from_slice(&(contrast.round() as i16).to_be_bytes()); + out.extend_from_slice(&0i16.to_be_bytes()); // mean + out.push(0); // lab flag + Some(out) + } + (AdjustmentKind::Levels, Params::Levels(l)) => { + let mut out = Vec::with_capacity(2 + 29 * 10); + out.extend_from_slice(&2u16.to_be_bytes()); // version + let mut record = |c: &LevelsChannel| { + let q = |v: f32| (v * 255.0).round().clamp(0.0, 255.0) as u16; + out.extend_from_slice(&q(c.input_black).to_be_bytes()); + out.extend_from_slice(&q(c.input_white).to_be_bytes()); + out.extend_from_slice(&q(c.output_black).to_be_bytes()); + out.extend_from_slice(&q(c.output_white).to_be_bytes()); + let gamma = (c.gamma * 100.0).round().clamp(1.0, 1000.0) as u16; + out.extend_from_slice(&gamma.to_be_bytes()); + }; + record(&l.rgb); + record(&l.red); + record(&l.green); + record(&l.blue); + // Photoshop always stores 29 records; the rest are identity. + let identity = LevelsChannel::default(); + for _ in 4..29 { + record(&identity); + } + Some(out) + } + ( + AdjustmentKind::HueSaturation, + Params::HueSaturation { + hue, + saturation, + lightness, + colorize, + .. + }, + ) => { + let mut out = Vec::with_capacity(10); + out.extend_from_slice(&2u16.to_be_bytes()); // version + out.extend_from_slice(&u16::from(*colorize).to_be_bytes()); + out.extend_from_slice(&(hue.round() as i16).to_be_bytes()); + out.extend_from_slice(&(saturation.round() as i16).to_be_bytes()); + out.extend_from_slice(&(lightness.round() as i16).to_be_bytes()); + Some(out) + } + (AdjustmentKind::Curves, Params::Curves(c)) => { + let channels = [&c.rgb, &c.red, &c.green, &c.blue]; + let mut bitmap = 0u32; + for (i, curve) in channels.iter().enumerate() { + if curve.points.len() >= 2 { + bitmap |= 1 << i; + } + } + let mut out = Vec::new(); + out.push(0); // padding + out.extend_from_slice(&1u16.to_be_bytes()); // version + out.extend_from_slice(&bitmap.to_be_bytes()); + for curve in channels { + if curve.points.len() < 2 { + continue; + } + let points: Vec<_> = curve.points.iter().take(32).collect(); + out.extend_from_slice(&(points.len() as u16).to_be_bytes()); + for (inp, outp) in points { + let q = |v: f32| (v * 255.0).round().clamp(0.0, 255.0) as u16; + out.extend_from_slice(&q(*outp).to_be_bytes()); + out.extend_from_slice(&q(*inp).to_be_bytes()); + } + } + Some(out) + } + ( + AdjustmentKind::BlackWhite, + Params::BlackWhite { + reds, + yellows, + greens, + cyans, + blues, + magentas, + }, + ) => { + let mut b = descriptor::Builder::new("null"); + b.double("Rd ", *reds as f64) + .double("Yllw", *yellows as f64) + .double("Grn ", *greens as f64) + .double("Cyn ", *cyans as f64) + .double("Bl ", *blues as f64) + .double("Mgnt", *magentas as f64); + Some(versioned(b.finish())) + } + (AdjustmentKind::SolidColor, Params::SolidColor { rgba }) => { + let mut b = descriptor::Builder::new("null"); + b.color( + "Clr ", + rgba[0] as f64 * 255.0, + rgba[1] as f64 * 255.0, + rgba[2] as f64 * 255.0, + ); + Some(versioned(b.finish())) + } + _ => None, + } +} + +/// A descriptor with the prefix `descriptor::parse_versioned` expects: a +/// u16 layer-block version then a u32 descriptor version, six bytes in all. +fn versioned(body: Vec) -> Vec { + let mut out = Vec::with_capacity(body.len() + 6); + out.extend_from_slice(&16u16.to_be_bytes()); + out.extend_from_slice(&16u32.to_be_bytes()); + out.extend_from_slice(&body); + out +} + fn parse_brightness(raw: &[u8]) -> Params { // Legacy 'brit': brightness i16, contrast i16, mean i16, lab u8. match (be_i16(raw, 0), be_i16(raw, 2)) { @@ -2536,4 +2681,111 @@ mod curve_editing_tests { assert!(out.r > 0.7, "red curve did not apply"); assert!((out.b - 0.5).abs() < 0.01, "blue was changed too"); } + /// Every kind the reader understands must survive encode -> parse. + #[test] + fn adjustment_params_round_trip_through_psd_bytes() { + let cases: Vec<(AdjustmentKind, Params)> = vec![ + (AdjustmentKind::Invert, Params::Invert), + (AdjustmentKind::Posterize, Params::Posterize { levels: 7 }), + ( + AdjustmentKind::Threshold, + Params::Threshold { + level: 100.0 / 255.0, + }, + ), + ( + AdjustmentKind::BrightnessContrast, + Params::BrightnessContrast { + brightness: 25.0, + contrast: -40.0, + }, + ), + ( + AdjustmentKind::HueSaturation, + Params::HueSaturation { + hue: 30.0, + saturation: -20.0, + lightness: 15.0, + colorize: true, + lightness_desaturates: false, + reciprocal_saturation: false, + }, + ), + ( + AdjustmentKind::BlackWhite, + Params::BlackWhite { + reds: 10.0, + yellows: 20.0, + greens: 30.0, + cyans: 40.0, + blues: 50.0, + magentas: 60.0, + }, + ), + ]; + for (kind, params) in cases { + let raw = + encode_psd(kind, ¶ms).unwrap_or_else(|| panic!("{kind:?} has no encoder")); + let back = parse_psd(kind, &raw); + assert_eq!(back, params, "{kind:?} did not round-trip"); + } + } + + #[test] + fn levels_round_trip_through_psd_bytes() { + let levels = Levels { + rgb: LevelsChannel { + input_black: 10.0 / 255.0, + input_white: 240.0 / 255.0, + output_black: 5.0 / 255.0, + output_white: 250.0 / 255.0, + gamma: 1.25, + }, + ..Levels::default() + }; + let params = Params::Levels(levels); + let raw = encode_psd(AdjustmentKind::Levels, ¶ms).unwrap(); + assert_eq!(raw.len(), 2 + 29 * 10, "photoshop expects 29 records"); + assert_eq!(parse_psd(AdjustmentKind::Levels, &raw), params); + } + + #[test] + fn curves_round_trip_through_psd_bytes() { + let curves = Curves { + rgb: Curve { + points: vec![(0.0, 0.0), (128.0 / 255.0, 200.0 / 255.0), (1.0, 1.0)], + }, + ..Curves::default() + }; + let params = Params::Curves(curves); + let raw = encode_psd(AdjustmentKind::Curves, ¶ms).unwrap(); + assert_eq!(parse_psd(AdjustmentKind::Curves, &raw), params); + } + + #[test] + fn solid_colour_round_trips_through_psd_bytes() { + let params = Params::SolidColor { + rgba: [64.0 / 255.0, 128.0 / 255.0, 192.0 / 255.0, 1.0], + }; + let raw = encode_psd(AdjustmentKind::SolidColor, ¶ms).unwrap(); + match parse_psd(AdjustmentKind::SolidColor, &raw) { + Params::SolidColor { rgba } => { + for (a, b) in rgba + .iter() + .zip([64.0 / 255.0, 128.0 / 255.0, 192.0 / 255.0, 1.0]) + { + assert!((a - b).abs() < 1.0 / 255.0, "{rgba:?}"); + } + } + other => panic!("parsed back as {other:?}"), + } + } + + #[test] + fn kinds_without_an_encoder_say_so() { + // Better an honest None, so the caller keeps whatever raw bytes the + // file arrived with, than a wrong block. + assert!(encode_psd(AdjustmentKind::Vibrance, &Params::Unsupported).is_none()); + assert!(encode_psd(AdjustmentKind::GradientMap, &Params::Unsupported).is_none()); + } } diff --git a/crates/codec-psd/Cargo.toml b/crates/codec-psd/Cargo.toml index 7751545f..54477c3a 100644 --- a/crates/codec-psd/Cargo.toml +++ b/crates/codec-psd/Cargo.toml @@ -9,6 +9,9 @@ schist-core.workspace = true schist-compositor.workspace = true schist-color.workspace = true schist-psd-descriptor.workspace = true +# Adjustment layers re-encode their live parameters on save. +schist-adjustments.workspace = true +serde_json.workspace = true miniz_oxide.workspace = true anyhow.workspace = true # Layers' channel data decompresses independently; large documents diff --git a/crates/codec-psd/src/writer/mod.rs b/crates/codec-psd/src/writer/mod.rs index 23a80622..d8268df8 100644 --- a/crates/codec-psd/src/writer/mod.rs +++ b/crates/codec-psd/src/writer/mod.rs @@ -360,8 +360,8 @@ fn prepare_entry(entry: &Entry<'_>, doc: &Document, psb: bool) -> Result (IntRect::EMPTY, empty_channels(doc)), }; Ok(prepare_common(layer, doc, psb, channels, bounds)) @@ -407,6 +407,16 @@ fn prepare_common( /// Preserved blocks plus a regenerated unicode name. fn build_extras(layer: &Layer, doc: &Document) -> Vec<([u8; 4], Vec)> { let mut out = vec![(*b"luni", unicode_name_payload(&layer.name))]; + // An adjustment layer's settings live in its own block. Layers created + // in Schist carry them only in `params_json`, which no PSD reader + // understands, so without re-encoding here the layer was written as an + // empty raster and every adjustment the user made was lost on save. + // A preserved block is stale once the parameters have been edited, so + // the encoded form wins and the old one is dropped below. + let adjustment = match &layer.kind { + LayerKind::Adjustment(data) => encode_adjustment(data), + _ => None, + }; // Effects are re-encoded from the layer's own style, so any preserved // block is stale by definition. 'lrFX' is the pre-CS legacy form, // which we never write. @@ -424,8 +434,17 @@ fn build_extras(layer: &Layer, doc: &Document) -> Vec<([u8; 4], Vec)> { if vector && (&block.key == b"vmsk" || &block.key == b"vsms" || &block.key == b"SoCo") { continue; } + // Drop the stale settings block when we have re-encoded it. + if let Some((key, _)) = &adjustment { + if &block.key == key { + continue; + } + } out.push((block.key, block.data.clone())); } + if let Some(entry) = adjustment { + out.push(entry); + } if let Some(payload) = encoded { out.push((*b"lfx2", payload)); } @@ -433,6 +452,19 @@ fn build_extras(layer: &Layer, doc: &Document) -> Vec<([u8; 4], Vec)> { out } +/// The settings block for an adjustment layer, preferring the live +/// parameters over whatever the file arrived with. +/// +/// Returns `None` when the parameters cannot be encoded, so the caller +/// keeps the preserved bytes rather than writing a block that would be +/// read back as something else. +fn encode_adjustment(data: &schist_core::AdjustmentData) -> Option<([u8; 4], Vec)> { + let json = data.params_json.as_deref()?; + let params: schist_adjustments::Params = serde_json::from_str(json).ok()?; + let payload = schist_adjustments::encode_psd(data.kind, ¶ms)?; + Some((data.kind.psd_key(), payload)) +} + /// Vector mask and fill blocks for a shape layer, so the shape survives as /// a shape rather than as a picture of one. fn shape_blocks(layer: &Layer, doc: &Document) -> Vec<([u8; 4], Vec)> { diff --git a/crates/codec-psd/tests/writer.rs b/crates/codec-psd/tests/writer.rs index e38141fb..06bb0348 100644 --- a/crates/codec-psd/tests/writer.rs +++ b/crates/codec-psd/tests/writer.rs @@ -276,6 +276,74 @@ fn psd_keeps_u32_lengths_for_the_same_keys() { assert_eq!(block.data, b"preserved payload"); } +#[test] +fn an_adjustment_layer_made_in_schist_survives_a_save() { + // The critical one: adjustment layers created in the app carry their + // settings only in `params_json`. With no encoder the writer emitted + // an empty raster layer, so saving destroyed every adjustment the user + // had made, crash-recovery snapshots included. + use schist_core::{AdjustmentData, AdjustmentKind, LayerKind}; + + let params = schist_adjustments::Params::Posterize { levels: 6 }; + let mut doc = base_doc(); + let mut layer = Layer::new_raster("Posterize"); + layer.kind = LayerKind::Adjustment(AdjustmentData { + kind: AdjustmentKind::Posterize, + raw: Vec::new(), + params_json: Some(serde_json::to_string(¶ms).unwrap()), + }); + doc.push_layer(layer); + + let back = read_psd(&write_psd(&doc).unwrap()).unwrap(); + let layer = &back.tree.layers[0]; + match &layer.kind { + LayerKind::Adjustment(data) => { + assert_eq!(data.kind, AdjustmentKind::Posterize); + assert_eq!( + schist_adjustments::parse_psd(data.kind, &data.raw), + params, + "the settings must come back" + ); + } + other => panic!("came back as {other:?}, not an adjustment layer"), + } +} + +#[test] +fn editing_a_photoshop_adjustment_layer_writes_the_new_settings() { + // The other half: a layer that arrived from a PSD keeps a preserved + // block. Once the parameters are edited that block is stale, so the + // re-encoded one has to win or the save silently reverts the edit. + use schist_core::{AdjustmentData, AdjustmentKind, LayerKind, RawBlock}; + + let edited = schist_adjustments::Params::Threshold { + level: 200.0 / 255.0, + }; + let mut doc = base_doc(); + let mut layer = Layer::new_raster("Threshold"); + layer.kind = LayerKind::Adjustment(AdjustmentData { + kind: AdjustmentKind::Threshold, + raw: 50u16.to_be_bytes().to_vec(), + params_json: Some(serde_json::to_string(&edited).unwrap()), + }); + // The stale block as it came off disk. + layer.extras.push(RawBlock { + key: *b"thrs", + data: 50u16.to_be_bytes().to_vec(), + }); + doc.push_layer(layer); + + let back = read_psd(&write_psd(&doc).unwrap()).unwrap(); + match &back.tree.layers[0].kind { + LayerKind::Adjustment(data) => assert_eq!( + schist_adjustments::parse_psd(data.kind, &data.raw), + edited, + "the edit must win over the preserved block" + ), + other => panic!("came back as {other:?}"), + } +} + #[test] fn preserves_image_resources_and_resolution() { let mut doc = base_doc(); diff --git a/crates/core/src/layer.rs b/crates/core/src/layer.rs index fda3db00..7bbee69d 100644 --- a/crates/core/src/layer.rs +++ b/crates/core/src/layer.rs @@ -114,6 +114,34 @@ impl AdjustmentKind { }) } + /// The 4-char PSD block key for this kind, the inverse of + /// [`Self::from_psd_key`]. + pub fn psd_key(&self) -> [u8; 4] { + use AdjustmentKind::*; + match self { + Levels => *b"levl", + Curves => *b"curv", + HueSaturation => *b"hue2", + BrightnessContrast => *b"brit", + BlackWhite => *b"blwh", + SolidColor => *b"SoCo", + GradientFill => *b"GdFl", + PatternFill => *b"PtFl", + Invert => *b"nvrt", + Posterize => *b"post", + Threshold => *b"thrs", + ColorBalance => *b"blnc", + Vibrance => *b"vibA", + Exposure => *b"expA", + PhotoFilter => *b"phfl", + GradientMap => *b"grdm", + SelectiveColor => *b"selc", + ChannelMixer => *b"mixr", + // A kind we do not model, carrying the key it arrived with. + Other(key) => *key, + } + } + pub fn display_name(&self) -> &'static str { use AdjustmentKind::*; match self { From d40e19ceb5d7e2428a45852fea69285382e85b78 Mon Sep 17 00:00:00 2001 From: Rahul A Mistry Date: Wed, 26 Aug 2026 07:48:44 +0530 Subject: [PATCH 05/10] fix: stop dropping psd blocks and smart object sources on save (cherry picked from commit a279d1dbd016a95ad0e8fdab0079c8c41fc03168) --- crates/codec-psd/src/lib.rs | 1 + crates/codec-psd/src/reader/layers.rs | 76 +++++++++--- crates/codec-psd/src/reader/mod.rs | 4 + crates/codec-psd/src/smart.rs | 164 ++++++++++++++++++++++++++ crates/codec-psd/src/writer/mod.rs | 46 +++++++- crates/codec-psd/tests/writer.rs | 151 ++++++++++++++++++++++++ crates/color/src/lib.rs | 12 +- crates/core/src/document.rs | 14 +++ crates/core/src/layer.rs | 8 ++ 9 files changed, 454 insertions(+), 22 deletions(-) create mode 100644 crates/codec-psd/src/smart.rs diff --git a/crates/codec-psd/src/lib.rs b/crates/codec-psd/src/lib.rs index e7d03d04..fe8ba595 100644 --- a/crates/codec-psd/src/lib.rs +++ b/crates/codec-psd/src/lib.rs @@ -17,6 +17,7 @@ pub mod effects; pub mod error; mod reader; +mod smart; pub mod vector; mod writer; pub mod zip; diff --git a/crates/codec-psd/src/reader/layers.rs b/crates/codec-psd/src/reader/layers.rs index 7780e227..7ca69a6f 100644 --- a/crates/codec-psd/src/reader/layers.rs +++ b/crates/codec-psd/src/reader/layers.rs @@ -21,6 +21,12 @@ const MAX_LAYER_DIM: i32 = 400_000; pub struct ParsedLayers { pub layers: Vec, + /// Global Layer Mask Info, verbatim. + pub global_layer_mask: Vec, + /// Document-level additional layer information blocks, verbatim. + /// `Lr16`/`Lr32`/`Layr` are excluded: the writer regenerates the + /// layer tree, so echoing the old copy back would emit it twice. + pub preserved_layer_info: Vec, /// The layer count was negative: the first alpha channel of the merged /// composite is real transparency (matters only for flattened files). pub merged_alpha: bool, @@ -39,6 +45,8 @@ struct Rec { name: String, lsct: Option, adjustment: Option, + /// The layer's blending-ranges block, verbatim. + blending_ranges: Vec, extras: Vec, } @@ -66,6 +74,8 @@ pub fn parse_layer_and_mask_info( if sec_len == 0 { return Ok(ParsedLayers { layers: Vec::new(), + global_layer_mask: Vec::new(), + preserved_layer_info: Vec::new(), merged_alpha: false, }); } @@ -80,21 +90,27 @@ pub fn parse_layer_and_mask_info( (Vec::new(), false) }; - // Global Layer Mask Info: u32 length + data. - // TODO: preserve this block for round-trip; the writer will need it. + // Global Layer Mask Info: u32 length + data. Kept verbatim; the + // writer used to emit a zero length here, dropping it. + let mut global_layer_mask = Vec::new(); if sec.remaining() >= 4 { let gl = sec.u32()? as usize; - sec.skip(gl.min(sec.remaining()))?; + let gl = gl.min(sec.remaining()); + global_layer_mask = sec.take(gl)?.to_vec(); } // Trailing document-level "additional layer information" blocks // ('Patt', 'FMsk', 'Txt2', ...). Spec quirk: at document level these are // padded to 4-byte boundaries (layer-record-level blocks pad to 2). // - // TODO: preserve these verbatim for round-trip. For now we only - // *interpret* 'Lr16'/'Lr32'/'Layr', which is where Photoshop actually - // stores the layer tree for 16/32-bit documents (Layer Info above is - // empty in those files). + // 'Lr16'/'Lr32'/'Layr' are where Photoshop stores the layer tree for + // 16/32-bit documents (Layer Info above is empty in those files), so + // they are interpreted rather than preserved -- the writer builds + // them from the tree. Everything else rides through untouched: + // pattern definitions, linked smart objects and the rest used to be + // read and thrown away, so opening such a file and saving it lost + // them, while the README promised the opposite. + let mut preserved_layer_info: Vec = Vec::new(); while sec.remaining() >= 12 { let sig = sec.sig4()?; if &sig != b"8BIM" && &sig != b"8B64" { @@ -112,15 +128,24 @@ pub fn parse_layer_and_mask_info( let pad = (4 - len % 4) % 4; sec.skip(pad.min(sec.remaining()))?; - if layers.is_empty() && matches!(&key, b"Lr16" | b"Lr32" | b"Layr") { - let (l, ma) = parse_layer_info(&mut block, header)?; - layers = l; - merged_alpha |= ma; + if matches!(&key, b"Lr16" | b"Lr32" | b"Layr") { + if layers.is_empty() { + let (l, ma) = parse_layer_info(&mut block, header)?; + layers = l; + merged_alpha |= ma; + } + continue; } + preserved_layer_info.push(RawBlock { + key, + data: block.take(block.remaining())?.to_vec(), + }); } Ok(ParsedLayers { layers, + global_layer_mask, + preserved_layer_info, merged_alpha, }) } @@ -224,11 +249,13 @@ fn parse_layer_record(cur: &mut Cursor, header: &Header) -> Result Result Result { // 4. Layer & Mask Information. let parsed = layers::parse_layer_and_mask_info(&mut cur, &header)?; + let global_layer_mask = parsed.global_layer_mask; + let preserved_layer_info = parsed.preserved_layer_info; let mut tree_layers = parsed.layers; // 5. Merged image data. Only decoded when the file is flattened (zero @@ -65,6 +67,8 @@ pub fn read_psd(bytes: &[u8]) -> Result { } doc.icc_profile = res.icc_profile; doc.preserved_resources = res.preserved; + doc.global_layer_mask = global_layer_mask; + doc.preserved_layer_info = preserved_layer_info; if !color_mode_data.is_empty() { doc.preserved_resources.insert( 0, diff --git a/crates/codec-psd/src/smart.rs b/crates/codec-psd/src/smart.rs new file mode 100644 index 00000000..c8dbdfb7 --- /dev/null +++ b/crates/codec-psd/src/smart.rs @@ -0,0 +1,164 @@ +//! Smart-object payloads, as a private additional-layer-info block. +//! +//! The README says "Smart objects keep their source pixels, so +//! transforming one repeatedly costs no more quality than transforming it +//! once". That was true inside a session and false across a save: +//! `LayerKind` has only `Raster`/`Group`/`Adjustment`, the payload rides +//! on `Layer::smart`, and the writer never serialized it. After +//! save-and-reopen the layer was a plain raster of its last +//! rasterization, and every further transform degraded it. +//! +//! Photoshop's own `SoLd`/`PlLd` descriptors point at pixels held +//! elsewhere in the file (`lnk2`), which we preserve verbatim but do not +//! author. Rather than pretend to write that graph, this stores the +//! source in a block of our own -- the same trick the type tool already +//! plays with `PsTx`. Photoshop ignores keys it does not know, so the +//! file stays valid there; Schist reads its own smart objects back. + +use schist_color::Depth; +use schist_core::{blit_rgba8, Affine, Filter, IntRect, Layer, SmartObject, TileMap}; + +/// Private block key. Not an Adobe key: "Sc" for Schist, "So" for smart +/// object. +pub const SMART_BLOCK_KEY: [u8; 4] = *b"ScSo"; + +/// Format revision, so a later change can be told apart from this one. +const VERSION: u32 = 1; + +/// Guard against a corrupt or hostile length claiming gigabytes. +const MAX_SOURCE_PIXELS: u64 = 200_000_000; + +fn filter_code(f: Filter) -> u8 { + match f { + Filter::Nearest => 0, + Filter::Bilinear => 1, + Filter::Bicubic => 2, + } +} + +fn filter_from_code(v: u8) -> Filter { + match v { + 0 => Filter::Nearest, + 2 => Filter::Bicubic, + _ => Filter::Bilinear, + } +} + +/// Serialize a layer's smart object, or `None` if it has none. +pub fn write_smart(layer: &Layer) -> Option> { + let smart = layer.smart.as_deref()?; + let bounds = smart.source_bounds; + if bounds.is_empty() { + return None; + } + let (w, h) = (bounds.width() as usize, bounds.height() as usize); + + let mut rgba = Vec::with_capacity(w * h * 4); + for y in bounds.top..bounds.bottom { + for x in bounds.left..bounds.right { + rgba.extend_from_slice(&smart.source.pixel(x, y).to_u8()); + } + } + + let mut out = Vec::new(); + out.extend_from_slice(&VERSION.to_be_bytes()); + let name = smart.name.as_bytes(); + out.extend_from_slice(&(name.len() as u32).to_be_bytes()); + out.extend_from_slice(name); + for v in [ + smart.transform.a, + smart.transform.b, + smart.transform.c, + smart.transform.d, + smart.transform.tx, + smart.transform.ty, + ] { + out.extend_from_slice(&v.to_be_bytes()); + } + out.push(filter_code(smart.filter)); + for v in [bounds.left, bounds.top, bounds.right, bounds.bottom] { + out.extend_from_slice(&v.to_be_bytes()); + } + let packed = miniz_oxide::deflate::compress_to_vec_zlib(&rgba, 6); + out.extend_from_slice(&(packed.len() as u32).to_be_bytes()); + out.extend_from_slice(&packed); + Some(out) +} + +/// Parse a `ScSo` payload back into a smart object. +/// +/// Returns `None` for anything malformed: a layer that loses its smart +/// wrapper still has its rasterized pixels, so declining is always +/// better than failing the whole open. +pub fn read_smart(data: &[u8], depth: Depth) -> Option { + let mut c = Cursor { data, at: 0 }; + if c.u32()? != VERSION { + return None; + } + let name_len = c.u32()? as usize; + let name = String::from_utf8_lossy(c.take(name_len)?).into_owned(); + let transform = Affine { + a: c.f32()?, + b: c.f32()?, + c: c.f32()?, + d: c.f32()?, + tx: c.f32()?, + ty: c.f32()?, + }; + let filter = filter_from_code(c.u8()?); + let bounds = IntRect::new(c.i32()?, c.i32()?, c.i32()?, c.i32()?); + if bounds.is_empty() { + return None; + } + let pixels = bounds.width() as u64 * bounds.height() as u64; + if pixels > MAX_SOURCE_PIXELS { + log::warn!("smart object claims {pixels} source pixels; ignoring it"); + return None; + } + let packed_len = c.u32()? as usize; + let packed = c.take(packed_len)?; + let expected = pixels as usize * 4; + let rgba = + miniz_oxide::inflate::decompress_to_vec_zlib_with_limit(packed, expected.max(1)).ok()?; + if rgba.len() < expected { + return None; + } + + let mut source = TileMap::default(); + blit_rgba8(&mut source, depth, bounds, &rgba[..expected]); + Some(SmartObject { + source, + source_bounds: bounds, + transform, + filter, + name, + }) +} + +/// A big-endian reader that returns `None` rather than panicking on a +/// short buffer. +struct Cursor<'a> { + data: &'a [u8], + at: usize, +} + +impl<'a> Cursor<'a> { + fn take(&mut self, n: usize) -> Option<&'a [u8]> { + let end = self.at.checked_add(n)?; + let out = self.data.get(self.at..end)?; + self.at = end; + Some(out) + } + fn u8(&mut self) -> Option { + Some(self.take(1)?[0]) + } + fn u32(&mut self) -> Option { + Some(u32::from_be_bytes(self.take(4)?.try_into().ok()?)) + } + fn i32(&mut self) -> Option { + Some(i32::from_be_bytes(self.take(4)?.try_into().ok()?)) + } + fn f32(&mut self) -> Option { + Some(f32::from_be_bytes(self.take(4)?.try_into().ok()?)) + } +} diff --git a/crates/codec-psd/src/writer/mod.rs b/crates/codec-psd/src/writer/mod.rs index d8268df8..d9dd4c1b 100644 --- a/crates/codec-psd/src/writer/mod.rs +++ b/crates/codec-psd/src/writer/mod.rs @@ -13,6 +13,7 @@ //! them, which is the inverse of what the reader folds up. use crate::error::PsdError; +use crate::PSB_U64_KEYS; use schist_color::{ColorMode, Depth}; use schist_core::{ Document, IntRect, Layer, LayerKind, LayerMask, MaskTileMap, TileCoord, TileMap, TILE_SIZE, @@ -214,6 +215,8 @@ struct Prepared { name: String, /// Additional layer info blocks: (key, payload). extras: Vec<([u8; 4], Vec)>, + /// The layer's blending-ranges block, verbatim. + blending_ranges: Vec, } struct MaskOut { @@ -264,8 +267,28 @@ fn write_layer_and_mask_info(b: &mut Buf, doc: &Document, psb: bool) -> Result<( b.pad_to(2); b.patch_len(layer_info_at, psb); - // --- Global layer mask info (none) --- - b.u32(0); + // --- Global layer mask info --- + // Preserved verbatim; a zero length here dropped the block from + // every file that had one. + b.u32(doc.global_layer_mask.len() as u32); + b.bytes(&doc.global_layer_mask); + + // --- Document-level additional layer information --- + // Pattern definitions, linked smart objects and the rest, exactly as + // they arrived. `Lr16`/`Lr32`/`Layr` are not in here: the layer tree + // above is regenerated, so echoing the old copy back would write it + // twice. Spec quirk: these pad to 4 bytes, not 2. + for block in &doc.preserved_layer_info { + b.bytes(b"8BIM"); + b.bytes(&block.key); + if psb && PSB_U64_KEYS.contains(&block.key) { + b.u64(block.data.len() as u64); + } else { + b.u32(block.data.len() as u32); + } + b.bytes(&block.data); + b.pad_to(4); + } b.patch_len(section_at, psb); Ok(()) } @@ -302,8 +325,10 @@ fn write_layer_record(b: &mut Buf, p: &Prepared, psb: bool) { } None => b.u32(0), } - // Layer blending ranges: regenerated as "none". - b.u32(0); + // Layer blending ranges, verbatim. Emitting a zero length here lost + // any "Blend If" the file arrived with. + b.u32(p.blending_ranges.len() as u32); + b.bytes(&p.blending_ranges); b.pascal(&p.name, 4); for (key, payload) in &p.extras { b.bytes(b"8BIM"); @@ -338,6 +363,7 @@ fn prepare_entry(entry: &Entry<'_>, doc: &Document, psb: bool) -> Result".into(), extras: vec![(*b"lsct", lsct_payload(3, None))], + blending_ranges: Vec::new(), }), Entry::GroupHeader(layer, open) => { let mut p = prepare_common(layer, doc, psb, empty_channels(doc), IntRect::EMPTY); @@ -401,6 +427,7 @@ fn prepare_common( mask, name: layer.name.clone(), extras: build_extras(layer, doc), + blending_ranges: layer.blending_ranges.clone(), } } @@ -431,6 +458,10 @@ fn build_extras(layer: &Layer, doc: &Document) -> Vec<([u8; 4], Vec)> { if &block.key == b"lfx2" || &block.key == b"lrFX" { continue; } + // Regenerated from `Layer::smart` below. + if block.key == crate::smart::SMART_BLOCK_KEY { + continue; + } if vector && (&block.key == b"vmsk" || &block.key == b"vsms" || &block.key == b"SoCo") { continue; } @@ -448,6 +479,13 @@ fn build_extras(layer: &Layer, doc: &Document) -> Vec<([u8; 4], Vec)> { if let Some(payload) = encoded { out.push((*b"lfx2", payload)); } + // The smart object's source pixels. Photoshop's own `SoLd`/`PlLd` + // descriptors point at pixels held elsewhere in the file, which we + // preserve but do not author; this is our own block, ignored by + // readers that do not know it. + if let Some(payload) = crate::smart::write_smart(layer) { + out.push((crate::smart::SMART_BLOCK_KEY, payload)); + } out.extend(shape_blocks(layer, doc)); out } diff --git a/crates/codec-psd/tests/writer.rs b/crates/codec-psd/tests/writer.rs index 06bb0348..3d38235b 100644 --- a/crates/codec-psd/tests/writer.rs +++ b/crates/codec-psd/tests/writer.rs @@ -741,3 +741,154 @@ fn an_ordinary_layer_gains_no_vector_blocks() { "a vector mask appeared from nowhere" ); } + +/// Four classes of PSD data were read and discarded, then regenerated as +/// empty on save: the Global Layer Mask Info block, the document-level +/// additional-info blocks (`Patt` pattern definitions, `lnk2` linked +/// smart objects, `Txt2`, `FMsk`) and per-layer blending ranges. Open a +/// Photoshop file that has any of them, save, and they were gone — while +/// the README says every block is preserved byte-for-byte. +#[test] +fn document_level_blocks_survive_a_round_trip() { + let mut doc = base_doc(); + doc.push_layer(solid_layer( + "art", + IntRect::from_xywh(0, 0, 8, 8), + [10, 20, 30, 255], + Depth::Eight, + )); + doc.global_layer_mask = vec![0, 1, 0, 60, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 128, 0]; + doc.preserved_layer_info = vec![ + RawBlock { + key: *b"Patt", + data: b"pattern definitions go here".to_vec(), + }, + RawBlock { + key: *b"lnk2", + data: b"linked smart object".to_vec(), + }, + ]; + + let back = read_psd(&write_psd(&doc).unwrap()).unwrap(); + assert_eq!(back.global_layer_mask, doc.global_layer_mask); + let keys: Vec<[u8; 4]> = back.preserved_layer_info.iter().map(|b| b.key).collect(); + assert_eq!(keys, vec![*b"Patt", *b"lnk2"]); + assert_eq!( + back.preserved_layer_info[0].data, + b"pattern definitions go here" + ); + assert_eq!(back.preserved_layer_info[1].data, b"linked smart object"); + // And the layer tree still reads back, so the extra blocks did not + // knock the section lengths out of step. + assert_eq!(back.tree.len(), 1); + assert_eq!(pixel(&back, 0, 1, 1), [10, 20, 30, 255]); +} + +/// Photoshop's "Blend If" sliders live in the per-layer blending-ranges +/// block. It was skipped on read and written back as a zero length. +#[test] +fn layer_blending_ranges_survive_a_round_trip() { + let mut doc = base_doc(); + let mut layer = solid_layer( + "art", + IntRect::from_xywh(0, 0, 8, 8), + [10, 20, 30, 255], + Depth::Eight, + ); + // A composite range plus one channel range: eight bytes each. + layer.blending_ranges = vec![ + 0, 0, 255, 255, 0, 0, 255, 255, // composite + 0, 20, 200, 255, 0, 0, 255, 255, // channel 0 + ]; + let expected = layer.blending_ranges.clone(); + doc.push_layer(layer); + + let back = read_psd(&write_psd(&doc).unwrap()).unwrap(); + assert_eq!(back.tree.layers[0].blending_ranges, expected); +} + +/// A layer that never had them still writes none, so an ordinary file +/// does not grow a block Photoshop would have omitted. +#[test] +fn a_layer_without_blending_ranges_writes_none() { + let mut doc = base_doc(); + doc.push_layer(solid_layer( + "art", + IntRect::from_xywh(0, 0, 8, 8), + [10, 20, 30, 255], + Depth::Eight, + )); + let back = read_psd(&write_psd(&doc).unwrap()).unwrap(); + assert!(back.tree.layers[0].blending_ranges.is_empty()); + assert!(back.global_layer_mask.is_empty()); + assert!(back.preserved_layer_info.is_empty()); +} + +/// The README says "Smart objects keep their source pixels, so +/// transforming one repeatedly costs no more quality than transforming it +/// once". That was true inside a session and false across a save: the +/// payload rides on `Layer::smart`, which the writer never serialized, so +/// after save-and-reopen the layer was a plain raster of its last +/// rasterization and every further transform degraded it. +#[test] +fn smart_objects_keep_their_source_pixels_across_a_save() { + let mut doc = base_doc(); + let mut layer = solid_layer( + "placed", + IntRect::from_xywh(0, 0, 16, 16), + [200, 40, 60, 255], + Depth::Eight, + ); + // A source twice the size of what is on the canvas, scaled down — + // exactly the case that degrades once the source is gone. + let mut source = schist_core::TileMap::default(); + let buf = [200u8, 40, 60, 255].repeat(32 * 32); + blit_rgba8( + &mut source, + Depth::Eight, + IntRect::from_xywh(0, 0, 32, 32), + &buf, + ); + let mut smart = schist_core::SmartObject::wrap(source, "placed.png"); + smart.transform = schist_core::Affine { + a: 0.5, + b: 0.0, + c: 0.0, + d: 0.5, + tx: 0.0, + ty: 0.0, + }; + smart.filter = schist_core::Filter::Bicubic; + layer.smart = Some(Box::new(smart)); + doc.push_layer(layer); + + let back = read_psd(&write_psd(&doc).unwrap()).unwrap(); + let smart = back.tree.layers[0] + .smart + .as_deref() + .expect("the smart object should have survived the save"); + assert_eq!(smart.name, "placed.png"); + assert_eq!(smart.filter, schist_core::Filter::Bicubic); + assert_eq!(smart.transform.a, 0.5); + assert_eq!(smart.transform.d, 0.5); + assert_eq!(smart.source_bounds, IntRect::from_xywh(0, 0, 32, 32)); + // The full-resolution source, not the 16x16 rasterization. + assert_eq!(smart.source.pixel(31, 31).to_u8(), [200, 40, 60, 255]); + assert_eq!(smart.source.pixel(20, 20).to_u8(), [200, 40, 60, 255]); +} + +/// An ordinary layer gains no smart-object block, so files do not grow a +/// key for something they do not have. +#[test] +fn an_ordinary_layer_gains_no_smart_object_block() { + let mut doc = base_doc(); + doc.push_layer(solid_layer( + "art", + IntRect::from_xywh(0, 0, 8, 8), + [10, 20, 30, 255], + Depth::Eight, + )); + let bytes = write_psd(&doc).unwrap(); + assert!(!bytes.windows(4).any(|w| w == b"ScSo")); + assert!(read_psd(&bytes).unwrap().tree.layers[0].smart.is_none()); +} diff --git a/crates/color/src/lib.rs b/crates/color/src/lib.rs index a7d32293..bf9752f1 100644 --- a/crates/color/src/lib.rs +++ b/crates/color/src/lib.rs @@ -145,8 +145,16 @@ pub fn f32_to_u8(v: f32) -> u8 { #[inline] pub fn u16_to_f32(v: u16) -> f32 { - // PSD 16-bit uses 0..=32768 internally, but we normalize storage to the - // full u16 range and convert at the codec boundary. + // The full u16 range, which is also what the PSD codec reads and + // writes -- there is no conversion at the codec boundary. + // + // Photoshop's 16-bit *editing* model is 15+1 bit (0..=32768 with + // 32768 as full scale), and this comment used to claim we converted + // for it. We do not, in either direction, so the claim was simply + // untrue; whether the samples on disk carry that range or the full + // u16 one is a question only a Photoshop-authored fixture can + // settle, and changing the scaling on a guess would halve or double + // the brightness of every 16-bit file we read. v as f32 / 65535.0 } diff --git a/crates/core/src/document.rs b/crates/core/src/document.rs index 2f753bdd..7c5e1a42 100644 --- a/crates/core/src/document.rs +++ b/crates/core/src/document.rs @@ -55,6 +55,18 @@ pub struct Document { pub history: History, /// PSD image resources we preserve for round-trip fidelity. pub preserved_resources: Vec, + /// The PSD Global Layer Mask Info payload, verbatim. + /// + /// Read and discarded, then written back as a zero length -- so the + /// block was silently dropped on every save. + pub global_layer_mask: Vec, + /// Document-level additional layer information blocks, verbatim: + /// `Patt` pattern definitions, `lnk2` linked smart objects, `Txt2`, + /// `FMsk` and anything else a writer put there. Also read and + /// discarded before; open a file with pattern definitions or linked + /// smart objects, save, and all of it was gone -- while the README + /// promised every block preserved byte-for-byte. + pub preserved_layer_info: Vec, /// Monotonic counter bumped on every visible change; views compare it /// to decide whether to recomposite. pub revision: u64, @@ -108,6 +120,8 @@ impl Document { selected: Vec::new(), history: History::new(), preserved_resources: Vec::new(), + global_layer_mask: Vec::new(), + preserved_layer_info: Vec::new(), revision: 0, guides: Vec::new(), last_selection: None, diff --git a/crates/core/src/layer.rs b/crates/core/src/layer.rs index 7bbee69d..0e902b3c 100644 --- a/crates/core/src/layer.rs +++ b/crates/core/src/layer.rs @@ -237,6 +237,13 @@ pub struct Layer { pub kind: LayerKind, /// Preserved PSD blocks (text engine data, effects, smart object refs…). pub extras: Vec, + /// The layer's PSD blending-ranges block, verbatim. + /// + /// Photoshop's "Blend If" sliders live here. The reader skipped the + /// block and the writer emitted a zero length, so a file with custom + /// ranges lost them on the first save. Nothing in Schist interprets + /// them yet; they ride through so the round trip is honest. + pub blending_ranges: Vec, /// Layer effects. Empty by default, so a layer costs nothing extra. pub style: crate::style::LayerStyle, /// Set when this layer is a vector shape: its pixels are generated @@ -279,6 +286,7 @@ impl Layer { mask: None, kind: LayerKind::Raster(RasterLayer::default()), extras: Vec::new(), + blending_ranges: Vec::new(), style: crate::style::LayerStyle::default(), shape: None, shape_key: 0, From a292d1c94990687f2665678535780f4b6fe98370 Mon Sep 17 00:00:00 2001 From: Rahul A Mistry Date: Wed, 26 Aug 2026 06:29:47 +0530 Subject: [PATCH 06/10] fix: fix the soft proof second hop and psd fill opacity --- Cargo.lock | 2 + crates/codec-psd/src/reader/layers.rs | 14 +++- crates/codec-psd/src/writer/mod.rs | 13 ++++ crates/codec-psd/tests/writer.rs | 47 +++++++++++++ crates/colormgmt/src/lib.rs | 95 ++++++++++++++++++++++----- plugins/codecs-common/src/lib.rs | 83 +++++++++++++++++++++-- 6 files changed, 230 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9a7455d3..891617d2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5655,11 +5655,13 @@ dependencies = [ "log", "miniz_oxide", "rayon", + "schist-adjustments", "schist-codec-affinity", "schist-color", "schist-compositor", "schist-core", "schist-psd-descriptor", + "serde_json", "thiserror 2.0.20", ] diff --git a/crates/codec-psd/src/reader/layers.rs b/crates/codec-psd/src/reader/layers.rs index 7ca69a6f..2cabdfc1 100644 --- a/crates/codec-psd/src/reader/layers.rs +++ b/crates/codec-psd/src/reader/layers.rs @@ -47,6 +47,8 @@ struct Rec { adjustment: Option, /// The layer's blending-ranges block, verbatim. blending_ranges: Vec, + /// 'iOpa': fill opacity, 0..=255. Absent means fully filled. + fill_opacity: Option, extras: Vec, } @@ -275,6 +277,7 @@ fn parse_layer_record(cur: &mut Cursor, header: &Header) -> Result { + rec.fill_opacity = data.first().copied(); + } _ => { // Adjustment layers: interpret the kind, keep the raw // payload. ('lyid' and every other key — interpreted or not @@ -653,7 +665,7 @@ fn make_layer( name: rec.name, visible: rec.visible, opacity: rec.opacity as f32 / 255.0, - fill_opacity: 1.0, + fill_opacity: rec.fill_opacity.map_or(1.0, |v| v as f32 / 255.0), blending_ranges: rec.blending_ranges, blend: BlendMode::Normal, // callers overwrite clipping: rec.clipping, diff --git a/crates/codec-psd/src/writer/mod.rs b/crates/codec-psd/src/writer/mod.rs index d9dd4c1b..c639eacc 100644 --- a/crates/codec-psd/src/writer/mod.rs +++ b/crates/codec-psd/src/writer/mod.rs @@ -462,6 +462,12 @@ fn build_extras(layer: &Layer, doc: &Document) -> Vec<([u8; 4], Vec)> { if block.key == crate::smart::SMART_BLOCK_KEY { continue; } + // Fill opacity is regenerated from the layer below, so a + // preserved copy is stale: echoing it back wrote the file's + // original value over whatever the user set in Schist. + if &block.key == b"iOpa" { + continue; + } if vector && (&block.key == b"vmsk" || &block.key == b"vsms" || &block.key == b"SoCo") { continue; } @@ -486,6 +492,13 @@ fn build_extras(layer: &Layer, doc: &Document) -> Vec<([u8; 4], Vec)> { if let Some(payload) = crate::smart::write_smart(layer) { out.push((crate::smart::SMART_BLOCK_KEY, payload)); } + // 'iOpa' is one byte of fill opacity plus three of padding. Photoshop + // omits the block at 100%, which is what a reader assumes when it is + // absent, so only write it when it says something. + let fill = (layer.fill_opacity.clamp(0.0, 1.0) * 255.0).round() as u8; + if fill != 255 { + out.push((*b"iOpa", vec![fill, 0, 0, 0])); + } out.extend(shape_blocks(layer, doc)); out } diff --git a/crates/codec-psd/tests/writer.rs b/crates/codec-psd/tests/writer.rs index 3d38235b..4c55a58a 100644 --- a/crates/codec-psd/tests/writer.rs +++ b/crates/codec-psd/tests/writer.rs @@ -892,3 +892,50 @@ fn an_ordinary_layer_gains_no_smart_object_block() { assert!(!bytes.windows(4).any(|w| w == b"ScSo")); assert!(read_psd(&bytes).unwrap().tree.layers[0].smart.is_none()); } + +/// Fill opacity is what makes "Fill 0% plus a drop shadow" show only the +/// shadow. The reader hard-coded it to 1.0 and left the 'iOpa' block in +/// `extras`, so a Photoshop file with Fill 0% opened fully opaque; the +/// writer then echoed the preserved block back, so changing Fill in +/// Schist saved the file's original value over it. +#[test] +fn round_trips_fill_opacity() { + let mut doc = base_doc(); + let mut layer = solid_layer( + "shadowed", + IntRect::from_xywh(0, 0, 8, 8), + [10, 20, 30, 255], + Depth::Eight, + ); + layer.fill_opacity = 0.0; + doc.push_layer(layer); + + let back = read_psd(&write_psd(&doc).unwrap()).unwrap(); + assert_eq!(back.tree.layers[0].fill_opacity, 0.0); + + // And editing it saves the new value, not the one that came in. + let mut edited = back; + edited.tree.layers[0].fill_opacity = 0.5; + let again = read_psd(&write_psd(&edited).unwrap()).unwrap(); + assert!( + (again.tree.layers[0].fill_opacity - 0.5).abs() <= 1.0 / 255.0, + "got {}", + again.tree.layers[0].fill_opacity + ); +} + +/// Photoshop omits 'iOpa' at 100%, which readers take to mean fully +/// filled, so writing it unconditionally would bloat every layer. +#[test] +fn a_fully_filled_layer_writes_no_fill_block() { + let mut doc = base_doc(); + doc.push_layer(solid_layer( + "plain", + IntRect::from_xywh(0, 0, 8, 8), + [10, 20, 30, 255], + Depth::Eight, + )); + let bytes = write_psd(&doc).unwrap(); + assert!(!bytes.windows(4).any(|w| w == b"iOpa")); + assert_eq!(read_psd(&bytes).unwrap().tree.layers[0].fill_opacity, 1.0); +} diff --git a/crates/colormgmt/src/lib.rs b/crates/colormgmt/src/lib.rs index cda5e758..2162d269 100644 --- a/crates/colormgmt/src/lib.rs +++ b/crates/colormgmt/src/lib.rs @@ -234,20 +234,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, @@ -261,14 +260,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) @@ -611,4 +622,56 @@ mod tests { .apply(&mut b); assert_eq!(a, b); } + /// 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:?}" + ); + } + } + + /// 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" + ); + } } diff --git a/plugins/codecs-common/src/lib.rs b/plugins/codecs-common/src/lib.rs index 940926c9..1adf3e1c 100644 --- a/plugins/codecs-common/src/lib.rs +++ b/plugins/codecs-common/src/lib.rs @@ -131,7 +131,16 @@ fn export_flat( match format { // JPEG has no alpha and takes a quality setting. ImageFormat::Jpeg => { - let rgb = image::DynamicImage::ImageRgba8(img).to_rgb8(); + // JPEG has no alpha, and `to_rgb8` simply drops it. A straight + // alpha composite leaves rgb at 0 where nothing was painted, + // so every transparent area came out black. Matte onto white + // instead, which is what Photoshop offers by default. + let mut rgb = image::RgbImage::new(doc.width, doc.height); + for (dst, src) in rgb.pixels_mut().zip(img.pixels()) { + let a = src[3] as f32 / 255.0; + let matte = |c: u8| (c as f32 * a + 255.0 * (1.0 - a)).round() as u8; + *dst = image::Rgb([matte(src[0]), matte(src[1]), matte(src[2])]); + } let mut encoder = image::codecs::jpeg::JpegEncoder::new_with_quality( &mut out, options.quality.clamp(1, 100), @@ -173,8 +182,17 @@ macro_rules! simple_codec { $exts } fn probe(&self, bytes: &[u8]) -> bool { - let magic: &[&[u8]] = $magic; - magic.iter().any(|m| bytes.starts_with(m)) + // Each alternative is a list of (offset, bytes) that must + // all match. A bare prefix is not enough for every format: + // "RIFF" alone matches wav and avi as well as webp. + let magic: &[&[(usize, &[u8])]] = $magic; + magic.iter().any(|alt| { + alt.iter().all(|(at, want)| { + bytes + .get(*at..at + want.len()) + .is_some_and(|got| got == *want) + }) + }) } fn import(&self, bytes: &[u8]) -> anyhow::Result { import_with($format, bytes, $name) @@ -205,7 +223,7 @@ simple_codec!( "PNG", ImageFormat::Png, &["png"], - &[b"\x89PNG"] + &[&[(0, b"\x89PNG")]] ); simple_codec!( JpegCodec, @@ -213,7 +231,7 @@ simple_codec!( "JPEG", ImageFormat::Jpeg, &["jpg", "jpeg"], - &[b"\xFF\xD8\xFF"] + &[&[(0, b"\xFF\xD8\xFF")]] ); simple_codec!( WebPCodec, @@ -221,7 +239,10 @@ simple_codec!( "WebP", ImageFormat::WebP, &["webp"], - &[b"RIFF"] + // "RIFF" alone is any RIFF container; webp also declares itself at + // offset 8. Without that, a .wav was handed to the webp decoder, + // because `decode_file` probes before it looks at the extension. + &[&[(0, b"RIFF"), (8, b"WEBP")]] ); simple_codec!( TiffCodec, @@ -229,7 +250,13 @@ simple_codec!( "TIFF", ImageFormat::Tiff, &["tif", "tiff"], - &[b"II*\x00", b"MM\x00*"] + // Classic TIFF plus BigTIFF, which uses version 43 instead of 42. + &[ + &[(0, b"II*\x00")], + &[(0, b"MM\x00*")], + &[(0, b"II+\x00")], + &[(0, b"MM\x00+")], + ] ); pub struct CommonCodecsPlugin; @@ -504,4 +531,46 @@ mod tests { assert!(white > 240, "reference white bakes near white: {white}"); assert!(spec >= white, "speculars roll off above white: {spec}"); } + #[test] + fn the_webp_probe_does_not_claim_every_riff_file() { + // `decode_file` probes before it consults the extension, so a wav + // was handed to the webp decoder and failed with "decoding WebP". + let mut wav = b"RIFF".to_vec(); + wav.extend_from_slice(&[0; 4]); + wav.extend_from_slice(b"WAVEfmt "); + assert!(!WebPCodec.probe(&wav), "a wav is not a webp"); + + let mut webp = b"RIFF".to_vec(); + webp.extend_from_slice(&[0; 4]); + webp.extend_from_slice(b"WEBPVP8 "); + assert!(WebPCodec.probe(&webp), "a real webp must still probe"); + } + + #[test] + fn the_tiff_probe_accepts_bigtiff() { + assert!(TiffCodec.probe(b"II*\x00rest"), "classic little-endian"); + assert!(TiffCodec.probe(b"MM\x00*rest"), "classic big-endian"); + assert!(TiffCodec.probe(b"II+\x00rest"), "bigtiff little-endian"); + assert!(TiffCodec.probe(b"MM\x00+rest"), "bigtiff big-endian"); + assert!(!TiffCodec.probe(b"II!\x00rest"), "and nothing else"); + } + + #[test] + fn transparent_areas_export_to_jpeg_as_white_not_black() { + // JPEG has no alpha and `to_rgb8` just drops it. A straight-alpha + // composite leaves rgb at 0 where nothing was painted, so every + // transparent region came out black. + let mut doc = Document::new("t", 8, 8, Depth::Eight); + doc.push_layer(Layer::new_raster("empty")); + let bytes = export_flat(&doc, ImageFormat::Jpeg, &ExportOptions::default()).unwrap(); + + let img = image::load_from_memory_with_format(&bytes, ImageFormat::Jpeg) + .unwrap() + .to_rgb8(); + let px = img.get_pixel(4, 4); + assert!( + px[0] > 200 && px[1] > 200 && px[2] > 200, + "transparent should matte to white, got {px:?}" + ); + } } From 9b10ccdf45fad3860af94b923031b8f69a39a0b1 Mon Sep 17 00:00:00 2001 From: Rahul A Mistry Date: Wed, 26 Aug 2026 08:08:58 +0530 Subject: [PATCH 07/10] fix: keep bit depth through non-psd import and export (cherry picked from commit 766c500cce00ea47424286b850f7eb84f483b7e3) --- crates/core/src/document.rs | 34 +++++ crates/core/src/lib.rs | 3 +- plugins/codecs-common/src/lib.rs | 221 ++++++++++++++++++++++++++++++- 3 files changed, 255 insertions(+), 3 deletions(-) diff --git a/crates/core/src/document.rs b/crates/core/src/document.rs index 7c5e1a42..95698671 100644 --- a/crates/core/src/document.rs +++ b/crates/core/src/document.rs @@ -1038,6 +1038,40 @@ impl StrokeEdit { /// Fill a whole raster layer tilemap region from an RGBA8 buffer /// (importer/test convenience; not undoable). +/// Blit straight-alpha f32 RGBA into a tile map at the map's own depth. +/// +/// The u8 form below is the common path, but it caps everything that goes +/// through it at 8 bits per channel -- which is why every non-PSD import +/// used to collapse a 16-bit scan to half its precision on the way in. +pub fn blit_rgba_f32(tiles: &mut TileMap, depth: Depth, rect: IntRect, rgba: &[f32]) { + use crate::tile::TILE_SIZE; + assert_eq!( + rgba.len(), + rect.width() as usize * rect.height() as usize * 4 + ); + let w = rect.width() as usize; + for coord in TileCoord::covering(&rect) { + let trect = coord.rect(); + let clip = trect.intersect(&rect); + if clip.is_empty() { + continue; + } + let buf = tiles.get_mut_or_insert(coord, depth); + for y in clip.top..clip.bottom { + let sy = (y - rect.top) as usize; + let ly = (y - trect.top) as usize; + for x in clip.left..clip.right { + let sx = (x - rect.left) as usize; + let lx = (x - trect.left) as usize; + let s = (sy * w + sx) * 4; + let px = schist_color::Rgba::new(rgba[s], rgba[s + 1], rgba[s + 2], rgba[s + 3]); + buf.set(ly * TILE_SIZE as usize + lx, px); + } + } + } + tiles.prune_blank(); +} + pub fn blit_rgba8(tiles: &mut TileMap, depth: Depth, rect: IntRect, rgba: &[u8]) { use crate::tile::TILE_SIZE; assert_eq!( diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index 65ec76bc..591446f6 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -20,7 +20,8 @@ pub mod tile; pub use annotate::{Artboard, CountGroup, LayerComp, LayerCompState, Note, Slice}; pub use blend::BlendMode; pub use document::{ - blit_rgba8, Document, DocumentId, EditBuilder, Guide, PreservedResource, StrokeEdit, + blit_rgba8, blit_rgba_f32, Document, DocumentId, EditBuilder, Guide, PreservedResource, + StrokeEdit, }; pub use geom::IntRect; pub use history::{Edit, EditOp, History, LayerProps}; diff --git a/plugins/codecs-common/src/lib.rs b/plugins/codecs-common/src/lib.rs index 1adf3e1c..da6c302d 100644 --- a/plugins/codecs-common/src/lib.rs +++ b/plugins/codecs-common/src/lib.rs @@ -9,7 +9,7 @@ use anyhow::Context as _; pub use heif::HeifCodec; use image::ImageFormat; use schist_color::Depth; -use schist_core::{blit_rgba8, Document, IntRect, Layer}; +use schist_core::{blit_rgba8, blit_rgba_f32, Document, IntRect, Layer}; use schist_plugin_api::{CodecPlugin, ExportOptions, PluginManifest, PluginRegistry}; mod affinity; @@ -57,6 +57,23 @@ fn png_cicp(bytes: &[u8]) -> Option<[u8; 4]> { None } +/// Which depth a decoded image deserves. +/// +/// Everything landed on `Depth::Eight` via `to_rgba8()`, so a 16-bit png +/// or a 16/32-bit float tiff lost half its precision or more on the way +/// in, permanently and with no warning. +/// +/// An HDR png that has been tone-mapped to sRGB above is 8-bit by then, +/// so this only sees the untouched sources. +fn depth_for(color: image::ColorType) -> Depth { + use image::ColorType::*; + match color { + L16 | La16 | Rgb16 | Rgba16 => Depth::Sixteen, + Rgb32F | Rgba32F => Depth::ThirtyTwo, + _ => Depth::Eight, + } +} + fn import_with(format: ImageFormat, bytes: &[u8], title: &str) -> anyhow::Result { let mut decoder = image::ImageReader::with_format(std::io::Cursor::new(bytes), format) .into_decoder() @@ -79,6 +96,7 @@ fn import_with(format: ImageFormat, bytes: &[u8], title: &str) -> anyhow::Result let cicp = (format == ImageFormat::Png) .then(|| png_cicp(bytes)) .flatten(); + let mut tone_mapped = false; let rgba = match cicp { Some([primaries, transfer @ (16 | 18), 0, 1]) => { // Bake from the decoder's full precision: HDR PNGs are @@ -87,6 +105,7 @@ fn import_with(format: ImageFormat, bytes: &[u8], title: &str) -> anyhow::Result match schist_colormgmt::bake_hdr_to_srgb(&mut pixels, primaries, transfer) { Ok(()) => { icc = None; // the pixels are sRGB now + tone_mapped = true; let bytes: Vec = pixels .iter() .map(|v| (v.clamp(0.0, 1.0) * 255.0 + 0.5) as u8) @@ -102,7 +121,53 @@ fn import_with(format: ImageFormat, bytes: &[u8], title: &str) -> anyhow::Result _ => img.to_rgba8(), }; - flat_document(title, w, h, rgba.as_raw(), icc) + // The tone-mapped HDR branch above has already collapsed to 8 bits; + // anything untouched keeps the depth it arrived with, so a 16-bit + // scan does not lose half its precision on the way in. + if tone_mapped { + return flat_document(title, w, h, rgba.as_raw(), icc); + } + match depth_for(img.color()) { + Depth::Eight => flat_document(title, w, h, rgba.as_raw(), icc), + Depth::Sixteen => { + let src = img.to_rgba16(); + let deep: Vec = src.as_raw().iter().map(|&v| v as f32 / 65535.0).collect(); + deep_document(title, w, h, &deep, Depth::Sixteen, icc) + } + Depth::ThirtyTwo => deep_document( + title, + w, + h, + img.to_rgba32f().as_raw(), + Depth::ThirtyTwo, + icc, + ), + } +} + +/// `flat_document` for a source that carries more than 8 bits a channel. +fn deep_document( + title: &str, + w: u32, + h: u32, + rgba: &[f32], + depth: Depth, + icc: Option>, +) -> anyhow::Result { + anyhow::ensure!(rgba.len() == w as usize * h as usize * 4, "buffer size"); + let mut doc = Document::new(title, w, h, depth); + doc.icc_profile = icc; + let mut layer = Layer::new_raster("Background"); + blit_rgba_f32( + &mut layer.as_raster_mut().unwrap().tiles, + depth, + IntRect::from_size(w, h), + rgba, + ); + doc.push_layer(layer); + doc.damage_all(); + doc.dirty = false; + Ok(doc) } fn export_flat( @@ -128,6 +193,37 @@ fn export_flat( // reads as sRGB elsewhere, so embed it wherever the format can. let icc = doc.icc_profile.clone(); use image::ImageEncoder as _; + + // `bit_depth` was only ever consulted to pick the dither level, so + // "export 16-bit png" was not achievable: every path built an 8-bit + // buffer. png and tiff carry 16 bits per channel; jpeg and webp do + // not, so they stay at 8 whatever is asked. + if options.bit_depth > 8 && matches!(format, ImageFormat::Png | ImageFormat::Tiff) { + let deep: Vec = pixels + .iter() + .map(|v| (v.clamp(0.0, 1.0) * 65535.0 + 0.5) as u16) + .collect(); + // The encoder reads these back as native-endian `u16`s and + // byte-swaps for the container itself, so handing it big-endian + // bytes would write every sample swapped. + let raw: Vec = deep.iter().flat_map(|v| v.to_ne_bytes()).collect(); + let (w, h) = (doc.width, doc.height); + if format == ImageFormat::Png { + let mut encoder = image::codecs::png::PngEncoder::new(&mut out); + if let Some(icc) = icc { + let _ = encoder.set_icc_profile(icc); + } + encoder.write_image(&raw, w, h, image::ExtendedColorType::Rgba16)?; + } else { + let mut encoder = image::codecs::tiff::TiffEncoder::new(&mut out); + if let Some(icc) = icc { + let _ = encoder.set_icc_profile(icc); + } + encoder.write_image(&raw, w, h, image::ExtendedColorType::Rgba16)?; + } + return Ok(out.into_inner()); + } + match format { // JPEG has no alpha and takes a quality setting. ImageFormat::Jpeg => { @@ -162,6 +258,33 @@ fn export_flat( image::ExtendedColorType::Rgba8, )?; } + // WebP and TIFF take a profile as well; only png and jpeg were + // wired up, so a wide-gamut document exported to either came out + // untagged. + ImageFormat::WebP => { + let mut encoder = image::codecs::webp::WebPEncoder::new_lossless(&mut out); + if let Some(icc) = icc { + let _ = encoder.set_icc_profile(icc); + } + encoder.write_image( + img.as_raw(), + doc.width, + doc.height, + image::ExtendedColorType::Rgba8, + )?; + } + ImageFormat::Tiff => { + let mut encoder = image::codecs::tiff::TiffEncoder::new(&mut out); + if let Some(icc) = icc { + let _ = encoder.set_icc_profile(icc); + } + encoder.write_image( + img.as_raw(), + doc.width, + doc.height, + image::ExtendedColorType::Rgba8, + )?; + } _ => img.write_to(&mut out, format)?, } Ok(out.into_inner()) @@ -573,4 +696,98 @@ mod tests { "transparent should matte to white, got {px:?}" ); } + + /// Every non-psd import was forced through `to_rgba8()` and + /// `Document::new(.., Depth::Eight)`, so a 16-bit scan lost half its + /// precision permanently and with no warning. + #[test] + fn a_sixteen_bit_png_keeps_its_precision() { + let mut img: image::ImageBuffer, Vec> = image::ImageBuffer::new(4, 2); + // A value that has no 8-bit representation: 0x0101 is the nearest + // 8-bit-expressible neighbour either side. + for (_, _, p) in img.enumerate_pixels_mut() { + *p = image::Rgba([0x0180, 0x8000, 0xFFFF, 0xFFFF]); + } + let mut bytes = std::io::Cursor::new(Vec::new()); + img.write_to(&mut bytes, ImageFormat::Png).unwrap(); + + let doc = PngCodec.import(&bytes.into_inner()).unwrap(); + assert_eq!(doc.depth, Depth::Sixteen); + let px = doc.tree.layers[0].as_raster().unwrap().tiles.pixel(1, 1); + // 0x0180 / 65535 == 0.005889..., which rounds to 2/255 == 0.00784 + // if it goes through 8 bits. + assert!( + (px.r - 0x0180 as f32 / 65535.0).abs() < 1e-4, + "red came back as {} (8-bit quantised is {})", + px.r, + 2.0 / 255.0 + ); + } + + /// And an 8-bit source stays 8-bit, so ordinary files do not quadruple + /// in memory for nothing. + #[test] + fn an_eight_bit_png_stays_eight_bit() { + let mut img = image::RgbaImage::new(4, 2); + img.fill(200); + let mut bytes = std::io::Cursor::new(Vec::new()); + img.write_to(&mut bytes, ImageFormat::Png).unwrap(); + assert_eq!( + PngCodec.import(&bytes.into_inner()).unwrap().depth, + Depth::Eight + ); + } + + /// `bit_depth` was only consulted to pick the dither level, never to + /// choose an output depth, so "export 16-bit png" was unreachable. + #[test] + fn export_honours_the_requested_bit_depth() { + let mut doc = Document::new("t", 8, 4, Depth::Sixteen); + let mut layer = Layer::new_raster("Background"); + let buf: Vec = [0.00589f32, 0.5, 1.0, 1.0].repeat(8 * 4); + schist_core::blit_rgba_f32( + &mut layer.as_raster_mut().unwrap().tiles, + Depth::Sixteen, + IntRect::from_size(8, 4), + &buf, + ); + doc.push_layer(layer); + + let deep = PngCodec + .export_with( + &doc, + &ExportOptions { + bit_depth: 16, + dither: false, + ..Default::default() + }, + ) + .unwrap(); + let back = PngCodec.import(&deep).unwrap(); + assert_eq!(back.depth, Depth::Sixteen, "export dropped to 8 bits"); + let px = back.tree.layers[0].as_raster().unwrap().tiles.pixel(1, 1); + assert!((px.r - 0.00589).abs() < 1e-4, "got {}", px.r); + + // The default is still 8-bit. + let shallow = PngCodec.export(&doc).unwrap(); + assert_eq!(PngCodec.import(&shallow).unwrap().depth, Depth::Eight); + } + + /// jpeg cannot carry 16 bits, so asking for it must not fail the + /// export. + #[test] + fn a_format_without_sixteen_bit_still_exports() { + let mut doc = Document::new("t", 8, 4, Depth::Eight); + doc.push_layer(Layer::new_raster("Background")); + let bytes = JpegCodec + .export_with( + &doc, + &ExportOptions { + bit_depth: 16, + ..Default::default() + }, + ) + .unwrap(); + assert!(JpegCodec.probe(&bytes)); + } } From 24cbc554710210080ef14975fd08fb2d0d4f87d2 Mon Sep 17 00:00:00 2001 From: Rahul A Mistry Date: Thu, 27 Aug 2026 14:18:00 +0530 Subject: [PATCH 08/10] fix: pad preserved blocks relatively and keep smart object depth --- crates/codec-psd/src/smart.rs | 46 +++++++++++--- crates/codec-psd/src/writer/mod.rs | 25 +++++++- crates/codec-psd/tests/writer.rs | 98 ++++++++++++++++++++++++++++++ crates/core/src/document.rs | 4 +- 4 files changed, 158 insertions(+), 15 deletions(-) diff --git a/crates/codec-psd/src/smart.rs b/crates/codec-psd/src/smart.rs index c8dbdfb7..eef3b360 100644 --- a/crates/codec-psd/src/smart.rs +++ b/crates/codec-psd/src/smart.rs @@ -16,14 +16,21 @@ //! file stays valid there; Schist reads its own smart objects back. use schist_color::Depth; -use schist_core::{blit_rgba8, Affine, Filter, IntRect, Layer, SmartObject, TileMap}; +use schist_core::{ + blit_rgba8, blit_rgba_f32, Affine, Filter, IntRect, Layer, SmartObject, TileMap, +}; /// Private block key. Not an Adobe key: "Sc" for Schist, "So" for smart /// object. pub const SMART_BLOCK_KEY: [u8; 4] = *b"ScSo"; -/// Format revision, so a later change can be told apart from this one. -const VERSION: u32 = 1; +/// Format revision. +/// +/// v1 stored 8-bit samples, which quantised a 16-bit source the moment it +/// was saved -- the exact loss the block exists to prevent. v2 stores +/// f32, so a deep source survives; v1 payloads are still read. +const VERSION: u32 = 2; +const VERSION_U8: u32 = 1; /// Guard against a corrupt or hostile length claiming gigabytes. const MAX_SOURCE_PIXELS: u64 = 200_000_000; @@ -53,10 +60,15 @@ pub fn write_smart(layer: &Layer) -> Option> { } let (w, h) = (bounds.width() as usize, bounds.height() as usize); - let mut rgba = Vec::with_capacity(w * h * 4); + // f32 per channel: `to_u8()` here threw away 8 bits of a 16-bit + // source on every save, which is what this block exists to avoid. + let mut rgba = Vec::with_capacity(w * h * 16); for y in bounds.top..bounds.bottom { for x in bounds.left..bounds.right { - rgba.extend_from_slice(&smart.source.pixel(x, y).to_u8()); + let px = smart.source.pixel(x, y); + for c in [px.r, px.g, px.b, px.a] { + rgba.extend_from_slice(&c.to_be_bytes()); + } } } @@ -92,7 +104,8 @@ pub fn write_smart(layer: &Layer) -> Option> { /// better than failing the whole open. pub fn read_smart(data: &[u8], depth: Depth) -> Option { let mut c = Cursor { data, at: 0 }; - if c.u32()? != VERSION { + let version = c.u32()?; + if version != VERSION && version != VERSION_U8 { return None; } let name_len = c.u32()? as usize; @@ -117,15 +130,28 @@ pub fn read_smart(data: &[u8], depth: Depth) -> Option { } let packed_len = c.u32()? as usize; let packed = c.take(packed_len)?; - let expected = pixels as usize * 4; - let rgba = + let sample = if version == VERSION { 4 } else { 1 }; + let expected = pixels as usize * 4 * sample; + // The decompression limit below is the real bound on host memory, so + // it has to know how wide a sample is. + let raw = miniz_oxide::inflate::decompress_to_vec_zlib_with_limit(packed, expected.max(1)).ok()?; - if rgba.len() < expected { + if raw.len() < expected { return None; } let mut source = TileMap::default(); - blit_rgba8(&mut source, depth, bounds, &rgba[..expected]); + if version == VERSION_U8 { + blit_rgba8(&mut source, depth, bounds, &raw[..expected]); + } else { + let floats: Vec = raw[..expected] + .as_chunks::<4>() + .0 + .iter() + .map(|b| f32::from_be_bytes(*b)) + .collect(); + blit_rgba_f32(&mut source, depth, bounds, &floats); + } Some(SmartObject { source, source_bounds: bounds, diff --git a/crates/codec-psd/src/writer/mod.rs b/crates/codec-psd/src/writer/mod.rs index c639eacc..8387d94b 100644 --- a/crates/codec-psd/src/writer/mod.rs +++ b/crates/codec-psd/src/writer/mod.rs @@ -279,15 +279,34 @@ fn write_layer_and_mask_info(b: &mut Buf, doc: &Document, psb: bool) -> Result<( // above is regenerated, so echoing the old copy back would write it // twice. Spec quirk: these pad to 4 bytes, not 2. for block in &doc.preserved_layer_info { - b.bytes(b"8BIM"); + // 8B64 marks a block whose length is u64 whatever the key, so a + // block that arrived that way has to go back out that way: an + // 8B64 block with a key outside `PSB_U64_KEYS` would otherwise be + // read as u64 and rewritten as u32, and `as u32` would silently + // truncate anything over 4 GiB. + let wide = + block.data.len() > u32::MAX as usize || (psb && PSB_U64_KEYS.contains(&block.key)); + b.bytes(if wide && !PSB_U64_KEYS.contains(&block.key) { + b"8B64" + } else { + b"8BIM" + }); b.bytes(&block.key); - if psb && PSB_U64_KEYS.contains(&block.key) { + if wide { b.u64(block.data.len() as u64); } else { b.u32(block.data.len() as u32); } b.bytes(&block.data); - b.pad_to(4); + // Pad relative to the payload length, not to the absolute file + // offset. `pad_to(4)` disagreed with the reader, which skips + // `(4 - len % 4) % 4`, whenever a block did not happen to start + // 4-aligned -- which depends on layer name lengths and channel + // sizes, so real files hit it constantly and everything after the + // first misaligned block was silently dropped. + for _ in 0..(4 - block.data.len() % 4) % 4 { + b.u8(0); + } } b.patch_len(section_at, psb); Ok(()) diff --git a/crates/codec-psd/tests/writer.rs b/crates/codec-psd/tests/writer.rs index 4c55a58a..92880e39 100644 --- a/crates/codec-psd/tests/writer.rs +++ b/crates/codec-psd/tests/writer.rs @@ -939,3 +939,101 @@ fn a_fully_filled_layer_writes_no_fill_block() { assert!(!bytes.windows(4).any(|w| w == b"iOpa")); assert_eq!(read_psd(&bytes).unwrap().tree.layers[0].fill_opacity, 1.0); } + +/// The writer padded each preserved document-level block to the absolute +/// file offset while the reader skips padding relative to the payload +/// length. They disagree whenever a block does not happen to start +/// 4-aligned, which depends on layer name lengths and channel sizes, so +/// everything after the first misaligned block was silently dropped. +#[test] +fn preserved_blocks_survive_an_odd_alignment() { + let mut doc = base_doc(); + // A two-character name shifts the following alignment by two. + doc.push_layer(solid_layer( + "ab", + IntRect::from_xywh(0, 0, 8, 8), + [10, 20, 30, 255], + Depth::Eight, + )); + // An odd-length global mask block moves it again. + doc.global_layer_mask = vec![1, 2, 3, 4, 5]; + doc.preserved_layer_info = vec![ + RawBlock { + key: *b"Patt", + data: b"pattern".to_vec(), + }, + RawBlock { + key: *b"lnk2", + data: b"linked smart object".to_vec(), + }, + ]; + + let back = read_psd(&write_psd(&doc).unwrap()).unwrap(); + let keys: Vec<[u8; 4]> = back.preserved_layer_info.iter().map(|b| b.key).collect(); + assert_eq!(keys, vec![*b"Patt", *b"lnk2"], "a block was dropped"); + assert_eq!(back.preserved_layer_info[1].data, b"linked smart object"); + assert_eq!(back.global_layer_mask, doc.global_layer_mask); +} + +/// Payload lengths of every residue mod 4, so the padding cannot be right +/// by luck. +#[test] +fn preserved_blocks_round_trip_at_every_alignment() { + for pad in 0..4usize { + let mut doc = base_doc(); + doc.push_layer(solid_layer( + "ab", + IntRect::from_xywh(0, 0, 8, 8), + [10, 20, 30, 255], + Depth::Eight, + )); + doc.preserved_layer_info = vec![ + RawBlock { + key: *b"Patt", + data: vec![7u8; 4 + pad], + }, + RawBlock { + key: *b"Txt2", + data: b"second".to_vec(), + }, + ]; + let back = read_psd(&write_psd(&doc).unwrap()).unwrap(); + assert_eq!( + back.preserved_layer_info.len(), + 2, + "payload length {} lost a block", + 4 + pad + ); + } +} + +/// The smart-object block stored 8-bit samples, so placing a 16-bit image +/// in a 16-bit document and saving quantised it away — the exact loss the +/// block exists to prevent. +#[test] +fn a_smart_object_source_keeps_more_than_eight_bits() { + let mut doc = Document::new("t", 64, 48, Depth::Sixteen); + let mut layer = Layer::new_raster("placed"); + // A value with no 8-bit representation. + let deep = 0x0180 as f32 / 65535.0; + let mut source = schist_core::TileMap::default(); + let buf: Vec = [deep, 0.5, 1.0, 1.0].repeat(16 * 16); + schist_core::blit_rgba_f32( + &mut source, + Depth::Sixteen, + IntRect::from_xywh(0, 0, 16, 16), + &buf, + ); + layer.smart = Some(Box::new(schist_core::SmartObject::wrap(source, "deep.png"))); + doc.push_layer(layer); + + let back = read_psd(&write_psd(&doc).unwrap()).unwrap(); + let smart = back.tree.layers[0].smart.as_deref().expect("smart object"); + let px = smart.source.pixel(4, 4); + assert!( + (px.r - deep).abs() < 1e-4, + "red came back as {} (8-bit quantised would be {})", + px.r, + 2.0 / 255.0 + ); +} diff --git a/crates/core/src/document.rs b/crates/core/src/document.rs index 95698671..959d027b 100644 --- a/crates/core/src/document.rs +++ b/crates/core/src/document.rs @@ -1036,8 +1036,6 @@ impl StrokeEdit { } } -/// Fill a whole raster layer tilemap region from an RGBA8 buffer -/// (importer/test convenience; not undoable). /// Blit straight-alpha f32 RGBA into a tile map at the map's own depth. /// /// The u8 form below is the common path, but it caps everything that goes @@ -1072,6 +1070,8 @@ pub fn blit_rgba_f32(tiles: &mut TileMap, depth: Depth, rect: IntRect, rgba: &[f tiles.prune_blank(); } +/// Fill a whole raster layer tilemap region from an RGBA8 buffer +/// (importer/test convenience; not undoable). pub fn blit_rgba8(tiles: &mut TileMap, depth: Depth, rect: IntRect, rgba: &[u8]) { use crate::tile::TILE_SIZE; assert_eq!( From a7bfa3908a43459a09ab4f73d19eb4af8e96688f Mon Sep 17 00:00:00 2001 From: Rahul A Mistry Date: Thu, 27 Aug 2026 15:01:06 +0530 Subject: [PATCH 09/10] fix: drop the dead descriptor variant and assert the exported profiles --- crates/psd-descriptor/src/lib.rs | 1 - plugins/codecs-common/src/lib.rs | 27 +++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/crates/psd-descriptor/src/lib.rs b/crates/psd-descriptor/src/lib.rs index f3d16723..de7ece62 100644 --- a/crates/psd-descriptor/src/lib.rs +++ b/crates/psd-descriptor/src/lib.rs @@ -18,7 +18,6 @@ pub enum Value { Unit(String, f64), List(Vec), Object(Descriptor), - Unknown, } impl Value { diff --git a/plugins/codecs-common/src/lib.rs b/plugins/codecs-common/src/lib.rs index da6c302d..1ddc24d9 100644 --- a/plugins/codecs-common/src/lib.rs +++ b/plugins/codecs-common/src/lib.rs @@ -482,6 +482,33 @@ mod tests { assert_eq!(doc2.icc_profile.as_deref(), Some(display_p3.as_slice())); } + /// Only png and jpeg were wired to `set_icc_profile`, so a + /// wide-gamut document exported to tiff or webp came out untagged and + /// every other application read it as sRGB. + /// + /// Asserted on the bytes: `image`'s own tiff decoder cannot read back + /// the `IccProfile` tag its encoder writes, so a round-trip through + /// it would test the reader rather than what we emit. + #[test] + fn tiff_and_webp_exports_carry_the_profile() { + let display_p3 = moxcms::ColorProfile::new_display_p3().encode().unwrap(); + let mut doc = Document::new("t", 4, 4, Depth::Eight); + doc.push_layer(schist_core::Layer::new_raster("l")); + doc.icc_profile = Some(display_p3.clone()); + + for (name, bytes) in [ + ("tiff", TiffCodec.export(&doc).unwrap()), + ("webp", WebPCodec.export(&doc).unwrap()), + ] { + assert!( + bytes + .windows(display_p3.len()) + .any(|w| w == display_p3.as_slice()), + "{name} lost the profile" + ); + } + } + #[test] fn png_cicp_pq_bakes_to_srgb() { // Three PQ greys: black, ~203-nit reference white, ~1000 nits. From f9851da07906d4b3b3c7e10909aec91102f3120f Mon Sep 17 00:00:00 2001 From: Rahul A Mistry Date: Thu, 27 Aug 2026 15:04:38 +0530 Subject: [PATCH 10/10] fix: leave the soft proof hop to the colour pr --- crates/colormgmt/src/lib.rs | 95 +++++++------------------------------ 1 file changed, 16 insertions(+), 79 deletions(-) diff --git a/crates/colormgmt/src/lib.rs b/crates/colormgmt/src/lib.rs index 2162d269..cda5e758 100644 --- a/crates/colormgmt/src/lib.rs +++ b/crates/colormgmt/src/lib.rs @@ -234,19 +234,20 @@ impl Default for ColorSettings { } impl ColorSettings { - /// Build the display hop for a document with the given embedded - /// profile. + /// Build the transform for a document with the given embedded profile. /// - /// 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. + /// Soft proofing runs document→proof→display; the two hops are baked + /// into one executor chain by applying them in sequence. pub fn transform_for(&self, document_icc: Option<&[u8]>) -> ColorTransform { - let source = match &self.proof { - Some(proof) => proof.clone(), - None => self.document_profile(document_icc), + 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(), }; match ColorTransform::new(&source, &self.display, self.intent) { Ok(t) => t, @@ -260,26 +261,14 @@ 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 = self.document_profile(document_icc); + let source = match document_icc { + Some(bytes) => Profile::from_bytes(bytes).unwrap_or_else(|_| self.working.clone()), + None => self.working.clone(), + }; // 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) @@ -622,56 +611,4 @@ mod tests { .apply(&mut b); assert_eq!(a, b); } - /// 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:?}" - ); - } - } - - /// 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" - ); - } }