diff --git a/core/dr-gpu/src/adjust.rs b/core/dr-gpu/src/adjust.rs index de75ced..ce16c7d 100644 --- a/core/dr-gpu/src/adjust.rs +++ b/core/dr-gpu/src/adjust.rs @@ -448,7 +448,7 @@ fn numbered(src: &str) -> String { mod tests { use super::*; use dr_decode::{CfaPattern, CropRect, RawImage}; - use dr_pipeline::ops::{exposure, saturation}; + use dr_pipeline::ops::{colour_mixer, exposure, saturation}; use dr_pipeline::EditGraph; use crate::Demosaicer; @@ -494,6 +494,32 @@ mod tests { .expect("demosaic") } + /// A white disc on black, centred in a `w`x`h` frame. + /// + /// The one shape that makes anisotropy unmissable: any transform that + /// scales the axes unequally returns it as an ellipse, and the ratio of + /// the ellipse's axes *is* the error. + fn disc_rgba(w: u32, h: u32, radius: f32) -> Vec { + let mut rgba = vec![0u8; (w * h * 4) as usize]; + for y in 0..h { + for x in 0..w { + let dx = x as f32 - w as f32 / 2.0; + let dy = y as f32 - h as f32 / 2.0; + let v = if (dx * dx + dy * dy).sqrt() < radius { + 255 + } else { + 0 + }; + let i = ((y * w + x) * 4) as usize; + rgba[i] = v; + rgba[i + 1] = v; + rgba[i + 2] = v; + rgba[i + 3] = 255; + } + } + rgba + } + fn read_centre(ctx: &GpuContext, tex: &wgpu::Texture) -> [u8; 4] { let (w, h) = (tex.width(), tex.height()); read_pixel(ctx, tex, w / 2, h / 2) @@ -1077,6 +1103,91 @@ mod tests { ); } + #[test] + fn each_colour_band_gets_its_own_pipeline() { + // The bug this closes, end to end and through one cache: the mixer + // emits code only for the bands that are set, but the cache key was + // the set of *active operations*, which is "colour_mixer" whichever + // band that is. A red adjustment and a blue one hashed alike, so the + // second render reused the first's compiled pipeline and uploaded its + // uniform into the first band's slot — whichever band compiled first + // kept acting and every other slider did nothing. + // + // Ordered red first deliberately: red is the first band declared, and + // is the one users reported as the only one that worked. + let Some(ctx) = ctx() else { return }; + let mut pass = AdjustPass::new(&ctx); + let img = blue_image(&ctx); + + // `None` renders the chain untouched, which is the baseline the two + // band settings are measured against. + fn band( + ctx: &GpuContext, + pass: &mut AdjustPass, + img: &DemosaicedImage, + set: Option<(&'static str, f32)>, + ) -> [u8; 4] { + let mut g = EditGraph::default_chain(); + if let Some((id, v)) = set { + g.set_param(colour_mixer::ID, dr_pipeline::ParamId(id), v); + } + let shader = g.compose(); + let t = pass.render(img, &shader, 16, 16).expect("render"); + read_centre(ctx, t) + } + + let neutral = band(&ctx, &mut pass, &img, None); + // Red first, so its pipeline is the one in the cache when blue asks. + let reds_turn = band(&ctx, &mut pass, &img, Some(("red_sat", 100.0))); + let blues_turn = band(&ctx, &mut pass, &img, Some(("blue_sat", -100.0))); + + let spread = |p: [u8; 4]| p[2].abs_diff(p[0]); + assert_eq!( + spread(reds_turn), + spread(neutral), + "a blue pixel is outside the red band, so red must leave it alone" + ); + assert!( + spread(blues_turn) + 8 < spread(neutral), + "blue at -100 must desaturate a blue pixel: {neutral:?} -> {blues_turn:?}" + ); + } + + /// A demosaiced image whose pixels sit at the centre of the blue band. + /// + /// Red and green equal, blue well above them, which `rgb_to_hcl` reads as + /// exactly 240 degrees — full weight to blue, none to any other band. + fn blue_image(ctx: &GpuContext) -> DemosaicedImage { + let size = 16u32; + let mut data = vec![0u16; (size * size) as usize]; + for y in 0..size { + for x in 0..size { + let c = CfaPattern::Rggb.colour_at(x, y); + data[(y * size + x) as usize] = if c == 2 { 12000 } else { 3000 }; + } + } + let raw = RawImage { + width: size, + height: size, + data, + cfa_pattern: CfaPattern::Rggb, + black_level: [0; 4], + white_level: 16383, + wb_coeffs: [1.0, 1.0, 1.0, 1.0], + color_matrix: Some([1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0]), + crop: CropRect { + x: 0, + y: 0, + width: size, + height: size, + }, + }; + Demosaicer::new(ctx) + .expect("demosaicer") + .run(&raw) + .expect("demosaic") + } + #[test] fn moving_a_slider_does_not_recompile() { // The property the pipeline cache exists for. Recompiling per frame @@ -1135,12 +1246,9 @@ mod tests { /// TRACES: FR-DSP-1 | AC-8 /// A disc on a non-square frame, rendered through a quarter turn. /// - /// Geometry, asserted on real pixels rather than on the generated WGSL. - /// A circle is the one shape that makes anisotropy unmissable: any - /// transform that scales the axes unequally turns it into an ellipse, and - /// the ratio of the ellipse's axes *is* the error. Reading the shader and - /// reasoning about which space `p` lives in is exactly how a plausible - /// formula gets written twice. + /// Geometry, asserted on real pixels rather than on the generated WGSL — + /// reading the shader and reasoning about which space `p` lives in is + /// exactly how a plausible formula gets written twice. #[test] fn a_quarter_turn_keeps_a_circle_circular() { let Some(ctx) = ctx() else { return }; @@ -1148,21 +1256,7 @@ mod tests { // 3:2, so an aspect mistake is a 2.25x distortion rather than a // subtlety. The disc is centred and comfortably inside the frame. let (w, h) = (180u32, 120u32); - let radius = 40.0f32; - let mut rgba = vec![0u8; (w * h * 4) as usize]; - for y in 0..h { - for x in 0..w { - let dx = x as f32 - w as f32 / 2.0; - let dy = y as f32 - h as f32 / 2.0; - let inside = (dx * dx + dy * dy).sqrt() < radius; - let i = ((y * w + x) * 4) as usize; - let v = if inside { 255 } else { 0 }; - rgba[i] = v; - rgba[i + 1] = v; - rgba[i + 2] = v; - rgba[i + 3] = 255; - } - } + let rgba = disc_rgba(w, h, 40.0); let src = DemosaicedImage::from_rgba8(&ctx, &rgba, w, h).expect("upload"); let mut graph = dr_pipeline::EditGraph::default_chain(); @@ -1200,24 +1294,7 @@ mod tests { let Some(ctx) = ctx() else { return }; let (w, h) = (180u32, 120u32); - let radius = 34.0f32; - let mut rgba = vec![0u8; (w * h * 4) as usize]; - for y in 0..h { - for x in 0..w { - let dx = x as f32 - w as f32 / 2.0; - let dy = y as f32 - h as f32 / 2.0; - let v = if (dx * dx + dy * dy).sqrt() < radius { - 255 - } else { - 0 - }; - let i = ((y * w + x) * 4) as usize; - rgba[i] = v; - rgba[i + 1] = v; - rgba[i + 2] = v; - rgba[i + 3] = 255; - } - } + let rgba = disc_rgba(w, h, 34.0); let src = DemosaicedImage::from_rgba8(&ctx, &rgba, w, h).expect("upload"); let mut graph = dr_pipeline::EditGraph::default_chain(); @@ -1243,6 +1320,59 @@ mod tests { ); } + /// TRACES: FR-DEV-3 | FR-DSP-1 + /// The same disc again, straightened *and* turned. + /// + /// The case neither test above reaches, and the one a portrait photograph + /// hits every time. A quarter turn — the user's or the file's EXIF tag — + /// swaps the frame's axes, so the space the straightening happens in is no + /// longer the source's: measuring a 2:3 frame with a 3:2 aspect stretches + /// one axis against the other by 2.25, and the rotation that follows comes + /// out as a shear. Each transform alone looks right, which is exactly why + /// it survived: only the pair is wrong. + #[test] + fn straightening_a_turned_frame_keeps_a_circle_circular() { + let Some(ctx) = ctx() else { return }; + + let (w, h) = (180u32, 120u32); + let rgba = disc_rgba(w, h, 34.0); + let src = DemosaicedImage::from_rgba8(&ctx, &rgba, w, h).expect("upload"); + + // Every route to a swapped frame: the button, the file's tag, and the + // two composed. All three reach the shader as one permutation, and a + // fix that only covers one of them is not a fix. + for (name, turns, tag) in [ + ("a user quarter turn", 1, 1u16), + ("an EXIF-portrait file", 0, 6), + ("both, composed", 2, 6), + ] { + let mut graph = dr_pipeline::EditGraph::default_chain(); + graph.set_orientation(dr_types::Orientation::from_exif(tag)); + graph.rotate_quarters(turns); + graph.set_param(dr_pipeline::framing::ID, dr_pipeline::framing::ANGLE, 20.0); + + let (ow, oh) = graph.output_size(w, h); + assert_eq!((ow, oh), (h, w), "{name}: the frame should be portrait"); + + let mut pass = AdjustPass::new(&ctx); + pass.render(&src, &graph.compose(), ow, oh).expect("render"); + let (pixels, rw, rh) = pass.export_pixels().expect("read back"); + + let lit = |x: u32, y: u32| pixels[((y * rw + x) * 4) as usize] > 128; + let across = (0..rw).filter(|&x| lit(x, rh / 2)).count(); + let down = (0..rh).filter(|&y| lit(rw / 2, y)).count(); + + assert!(across > 0 && down > 0, "{name}: the disc vanished"); + let ratio = across as f32 / down as f32; + assert!( + (ratio - 1.0).abs() < 0.08, + "{name}: a straightened circle came back {across} across by \ + {down} down (ratio {ratio:.3}); the turn and the angle are \ + disagreeing about which frame they act in" + ); + } + } + #[test] fn the_output_is_importable_by_a_compositor() { // Every condition Slint checks before it will adopt a texture diff --git a/core/dr-pipeline/src/framing.rs b/core/dr-pipeline/src/framing.rs index 33863c1..11016ab 100644 --- a/core/dr-pipeline/src/framing.rs +++ b/core/dr-pipeline/src/framing.rs @@ -674,18 +674,32 @@ impl Framing { ", ); - s.push_str( + // The frame `p` is measured in is the one the *user* is looking at, + // and a quarter turn — the file's or the user's — has already swapped + // its axes. Measuring a portrait frame with the landscape aspect + // stretches one axis against the other by `(w/h)²`, which the + // permutation below silently undoes but the straightening above does + // not: a rotation is only a rotation in a space whose axes carry the + // same scale, so in the stretched one it comes out as a shear. + let frame_aspect = if self.swaps_axes() { + " let frame_aspect = vec2(f32(src_dims.y) / f32(src_dims.x), 1.0);\n" + } else { + " let frame_aspect = aspect;\n" + }; + + let _ = write!( + s, " - // Into the crop rect. - uv = u.crop_rect.xy + uv * u.crop_rect.zw; - var p = (uv - vec2(0.5)) * aspect; -", + // Into the crop rect, then into the framed image's own centred space. +{frame_aspect} uv = u.crop_rect.xy + uv * u.crop_rect.zw; + var p = (uv - vec2(0.5)) * frame_aspect; +" ); if self.angle != 0.0 { // Done in the aspect-corrected space, which is the whole reason - // `p` is scaled by `aspect` above: a rotation applied to raw 0..1 - // coordinates on a non-square image shears it rather than + // `p` is scaled by `frame_aspect` above: a rotation applied to raw + // 0..1 coordinates on a non-square image shears it rather than // turning it, and that reads as a rendering fault. s.push_str( " @@ -706,12 +720,14 @@ impl Framing { if turns != 0 { // An exact coordinate permutation rather than a rotation through // the matrix above, which would resample a transform that has an - // exact answer. Applied to `p`, so the aspect scaling has to be - // undone and reapplied across the swap. + // exact answer. It is also where the two aspects meet: `p` arrives + // scaled by the framed image's and has to leave scaled by the + // source's, so each axis is divided by the one it is read from and + // multiplied by the one it is written to. let permutation = match turns { - 1 => " p = vec2(p.y * aspect.x, -p.x / aspect.x);", + 1 => " p = vec2(p.y * aspect.x, -p.x / frame_aspect.x);", 2 => " p = -p;", - _ => " p = vec2(-p.y * aspect.x, p.x / aspect.x);", + _ => " p = vec2(-p.y * aspect.x, p.x / frame_aspect.x);", }; let _ = write!( s, @@ -1283,6 +1299,53 @@ mod tests { assert!(f.wgsl_prologue().contains("u.framing_angle")); } + #[test] + fn a_turned_frame_is_measured_by_its_own_aspect() { + // `p` is the space the straightening rotates in, so it has to be the + // space the *user* sees. Once a turn has swapped the axes — the + // button's or the file's — the source's aspect is the wrong ruler: + // measuring a 2:3 frame with a 3:2 aspect stretches one axis against + // the other, and the rotation that follows shears instead of turning. + // + // The pixels are asserted in `dr-gpu`'s + // `straightening_a_turned_frame_keeps_a_circle_circular`; this is the + // same claim at the level the code is written at. + for turns in [1u8, 3] { + let mut f = Framing::new(); + f.rotate_quarters(i32::from(turns)); + f.set_param(ANGLE, 5.0); + let src = f.wgsl_prologue(); + assert!( + src.contains( + "let frame_aspect = vec2(f32(src_dims.y) / f32(src_dims.x), 1.0);" + ), + "{turns} turns must measure the frame turned:\n{src}" + ); + assert!(src.contains("* frame_aspect;"), "{src}"); + } + + // An even turn leaves the axes where they were, so the two rulers are + // the same one and nothing has to be recomputed. + for turns in [0u8, 2] { + let mut f = Framing::new(); + f.rotate_quarters(i32::from(turns)); + f.set_param(ANGLE, 5.0); + assert!( + f.wgsl_prologue().contains("let frame_aspect = aspect;"), + "{turns} turns should reuse the source aspect" + ); + } + + // And the baseline reaches it the same way a button press does: a + // file stored sideways is a turned frame whether or not it was edited. + let mut sideways = Framing::new(); + sideways.set_baseline(dr_types::Orientation::from_exif(6)); + sideways.set_param(ANGLE, 5.0); + assert!(sideways + .wgsl_prologue() + .contains("f32(src_dims.y) / f32(src_dims.x)")); + } + #[test] fn a_quarter_turn_corrects_for_aspect_across_the_swap() { // `p` is scaled by the source aspect, so a permutation that exchanges