mirror of
https://github.com/storytold/photocraft.git
synced 2026-10-24 09:57:08 -06:00
Address CodeRabbit review: CMYK ink paths convert colours through the document profile (#3356)
Two places turned an RGB or Lab colour into ink with the built-in coated CMYK instead of the document's embedded profile: the exact ink path's tiles (render_tile runs on rayon workers, which never inherit a caller's with_cmyk_space scope), e.g. a solid fill picked in RGB, and the split path's stand-ins (fill and effect colours, converted on the calling thread outside any scope). Both now enter cmyk_space(doc). A test with a synthetic uncoated profile checks a solid fill and a Color Overlay get the profile's inks; each half fails on its own without its fix. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
b54ab8ff30
commit
c88691c0b8
Generated
+1
@@ -4786,6 +4786,7 @@ name = "photocraft-compose"
|
||||
version = "0.13.0"
|
||||
dependencies = [
|
||||
"criterion",
|
||||
"photocraft-cms",
|
||||
"photocraft-color",
|
||||
"photocraft-doc",
|
||||
"photocraft-geom",
|
||||
|
||||
@@ -21,6 +21,7 @@ rayon = { workspace = true }
|
||||
proptest = { workspace = true }
|
||||
criterion = { workspace = true }
|
||||
photocraft-paint = { workspace = true }
|
||||
photocraft-cms = { workspace = true }
|
||||
|
||||
[[bench]]
|
||||
name = "compose"
|
||||
|
||||
@@ -18,7 +18,10 @@
|
||||
//!
|
||||
//! Buffers hold straight-alpha `[C, M, Y, K, A]` in 0..1 ink (0 = no ink).
|
||||
|
||||
use std::sync::Arc;
|
||||
|
||||
use photocraft_color::blend::BlendMode;
|
||||
use photocraft_color::convert::CmykSpace;
|
||||
use photocraft_color::{ColorMode, SampleType};
|
||||
use photocraft_doc::adjust::ToneSpace;
|
||||
use photocraft_doc::{Adjustment, Document, Knockout, Layer, LayerContent};
|
||||
@@ -545,12 +548,15 @@ fn blend_into_fill(backdrop: &mut CmykBuffer, src: &CmykBuffer, l: &Layer, gamma
|
||||
}
|
||||
}
|
||||
|
||||
fn render_tile(doc: &Document, rect: Rect, cx: &Ctx) -> CmykBuffer {
|
||||
/// One tile on ink values. `space` is the document's CMYK profile ([`super::cmyk_space`]): a
|
||||
/// colour given in RGB or Lab (a fill, say) becomes ink through it, and tiles run on worker
|
||||
/// threads, which don't inherit the caller's scope.
|
||||
fn render_tile(doc: &Document, rect: Rect, cx: &Ctx, space: Option<&Arc<CmykSpace>>) -> CmykBuffer {
|
||||
let mut b = CmykBuffer::transparent(rect);
|
||||
// Blends stay clipped to 0..1 whatever the depth: unclipped (HDR) blending would make
|
||||
// brightness above 1, i.e. negative ink. Set explicitly, since a tile can run on a thread
|
||||
// that is in the middle of an RGB composite of a 32-bit document.
|
||||
super::in_blend_space(false, false, || composite_stack(&doc.layers, &mut b, cx));
|
||||
photocraft_color::convert::with_cmyk_space(space, || super::in_blend_space(false, false, || composite_stack(&doc.layers, &mut b, cx)));
|
||||
b
|
||||
}
|
||||
|
||||
@@ -601,7 +607,8 @@ pub fn render_cmyk_tiled(doc: &Document, rect: Rect, tile: i32) -> Option<CmykBu
|
||||
if supports(doc) {
|
||||
let patterns = pattern::PreparedPatterns::new(&doc.patterns, pattern::PREPARED_PATTERN_BYTES).in_mode(doc.pixel_format().mode);
|
||||
let cx = Ctx::for_doc(doc, &patterns);
|
||||
return Some(render_with(rect, tile, |t| render_tile(doc, t, &cx)));
|
||||
let space = super::cmyk_space(doc);
|
||||
return Some(render_with(rect, tile, |t| render_tile(doc, t, &cx, space.as_ref())));
|
||||
}
|
||||
if !split::supports(doc) {
|
||||
return None;
|
||||
@@ -667,11 +674,12 @@ pub fn render_cmyk_bands<E>(doc: &Document, rect: Rect, band_rows: i32, mut sink
|
||||
}
|
||||
let patterns = pattern::PreparedPatterns::new(&doc.patterns, pattern::PREPARED_PATTERN_BYTES).in_mode(doc.pixel_format().mode);
|
||||
let cx = Ctx::for_doc(doc, &patterns);
|
||||
let space = super::cmyk_space(doc);
|
||||
let rows = super::band_rows_for(rect.width(), band_rows);
|
||||
let mut y = rect.y0;
|
||||
while y < rect.y1 {
|
||||
let y1 = y.saturating_add(rows).min(rect.y1);
|
||||
if let Err(e) = sink(render_with(Rect::new(rect.x0, y, rect.x1, y1), RENDER_TILE, |t| render_tile(doc, t, &cx))) {
|
||||
if let Err(e) = sink(render_with(Rect::new(rect.x0, y, rect.x1, y1), RENDER_TILE, |t| render_tile(doc, t, &cx, space.as_ref()))) {
|
||||
return Some(Err(e));
|
||||
}
|
||||
y = y1;
|
||||
|
||||
@@ -262,11 +262,16 @@ fn stand_in_layer(l: &mut Layer, planes: Planes, depth: SampleType, canvas: Rect
|
||||
}
|
||||
|
||||
/// `doc` as an RGB document carrying the `planes` inks' brightness.
|
||||
/// Colours given in RGB or Lab become ink through the document's own CMYK profile, as on the
|
||||
/// exact ink path.
|
||||
pub(super) fn stand_in(doc: &Document, planes: Planes) -> Document {
|
||||
let mut d = doc.clone();
|
||||
d.mode = ColorMode::Rgb;
|
||||
d.icc_profile = None;
|
||||
d.layers.iter_mut().for_each(|l| stand_in_layer(l, planes, doc.depth, doc.bounds()));
|
||||
let space = crate::cmyk_space(doc);
|
||||
photocraft_color::convert::with_cmyk_space(space.as_ref(), || {
|
||||
d.layers.iter_mut().for_each(|l| stand_in_layer(l, planes, doc.depth, doc.bounds()));
|
||||
});
|
||||
d
|
||||
}
|
||||
|
||||
|
||||
@@ -1655,3 +1655,45 @@ fn flattening_keeps_exact_inks() {
|
||||
let stamp = crate::flatten_to_surface(&d, CMYKA8, None);
|
||||
assert_eq!(stamp.pixel(2, 0), vec![0.0, 0.0, 0.0, 1.0, q(0.4)]);
|
||||
}
|
||||
|
||||
/// A colour given in RGB becomes ink through the document's own CMYK profile, not the built-in
|
||||
/// coated one, on both ink paths: a solid fill on the exact path (its tiles render on worker
|
||||
/// threads) and a Color Overlay on the split path (its stand-ins).
|
||||
#[test]
|
||||
fn rgb_colours_become_ink_through_the_document_profile() {
|
||||
let profile = photocraft_cms::synth::cmyk_profile(&photocraft_cms::synth::CmykParams {
|
||||
description: "Test Uncoated".into(),
|
||||
tvi: [0.26, 0.26, 0.26, 0.3],
|
||||
grid_a2b: 5,
|
||||
grid_b2a: 9,
|
||||
..Default::default()
|
||||
});
|
||||
let bytes = profile.to_bytes();
|
||||
let space = photocraft_color::convert::CmykSpace::for_profile(Some(&bytes));
|
||||
assert!(space.is_some(), "a profile other than the built-in one");
|
||||
let rgb = photocraft_color::Color { mode: ColorMode::Rgb, c: [0.2, 0.5, 0.8, 0.0], alpha: 1.0 };
|
||||
let want = photocraft_color::convert::with_cmyk_space(space.as_ref(), || rgb.in_mode(ColorMode::Cmyk).c);
|
||||
let builtin = rgb.in_mode(ColorMode::Cmyk).c;
|
||||
assert!((0..4).any(|i| (want[i] - builtin[i]).abs() > 0.02), "the profiles differ: {want:?} {builtin:?}");
|
||||
let check = |d: &Document, what: &str| {
|
||||
let got = render_cmyk(d, d.bounds()).unwrap_or_else(|| panic!("supported"));
|
||||
for p in &got.px {
|
||||
assert!((0..4).all(|i| (p[i] - want[i]).abs() <= 1.0 / 255.0) && p[4] == 1.0, "{what}: {p:?}, want {want:?} (built-in {builtin:?})");
|
||||
}
|
||||
};
|
||||
|
||||
let mut exact = doc(8, 4);
|
||||
exact.icc_profile = Some(bytes.clone());
|
||||
exact.layers.push(Layer::new("fill", LayerContent::Fill(photocraft_doc::Fill::Solid(rgb))));
|
||||
assert!(supports(&exact));
|
||||
check(&exact, "solid fill");
|
||||
|
||||
let mut overlay = doc(8, 4);
|
||||
overlay.icc_profile = Some(bytes);
|
||||
let mut l = layer(&[(Rect::new(0, 0, 8, 4), [0.0, 0.0, 0.0, 1.0, 1.0])]);
|
||||
l.effects.enabled = true;
|
||||
l.effects.items.push(Effect::ColorOverlay { common: FxCommon::new(BlendMode::Normal, 1.0), color: rgb });
|
||||
overlay.layers.push(l);
|
||||
assert!(!supports(&overlay) && split::supports(&overlay));
|
||||
check(&overlay, "colour overlay");
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user