From f76e024f418fbe9283803cca4e1f6c63e0f3b975 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Tue, 18 Aug 2026 15:17:54 +0200 Subject: [PATCH] Straighten a portrait frame in the frame the user is looking at MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The framing prologue built the centred position `p` by scaling with the source's aspect, then straightened, then permuted the quarter turns. On a landscape frame those are one space and it worked. Once a turn has swapped the axes — the rotate button, or a file whose EXIF tag says the camera was held sideways — they are not: `p` was measured with the source's ruler on a frame that is no longer that shape, stretching one axis against the other by (w/h)², which is 2.25 on a 3:2 photograph. The quarter-turn permutation happened to undo that stretch, so rotation alone looked right, which is how this survived. The straighten in between did not, and a rotation in a space whose axes carry different scales is a shear. `p` is now built in `frame_aspect` — the frame as the user sees it — and the permutation becomes the one place the two rulers meet: each axis divided by the aspect it is read from, multiplied by the aspect it is written to. Asserted on pixels rather than on the generated WGSL, because reading the shader and reasoning about which space `p` lives in is how the wrong formula got written in the first place: a disc, straightened by 20° on a turned 3:2 frame, must come back circular by every route to a swapped frame — the button, the tag, and the two composed. Co-Authored-By: Claude Opus 5 --- core/dr-gpu/src/adjust.rs | 210 ++++++++++++++++++++++++++------ core/dr-pipeline/src/framing.rs | 85 +++++++++++-- 2 files changed, 244 insertions(+), 51 deletions(-) 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