diff --git a/core/dr-gpu/Cargo.toml b/core/dr-gpu/Cargo.toml index 143614b..94cd406 100644 --- a/core/dr-gpu/Cargo.toml +++ b/core/dr-gpu/Cargo.toml @@ -60,3 +60,8 @@ name = "local" # `export_pixels` to feed the model, which is the ungated export path. The # watershed, and the region-graph transfer that needs the gate, is not involved. + +[[test]] +name = "masked_outputs" +# Needs the distance transform, which lives with the model half of dr-segment. +required-features = [] diff --git a/core/dr-gpu/tests/masked_outputs.rs b/core/dr-gpu/tests/masked_outputs.rs new file mode 100644 index 0000000..33c6d1e --- /dev/null +++ b/core/dr-gpu/tests/masked_outputs.rs @@ -0,0 +1,191 @@ +//! Every path that produces pixels must apply the masks. +//! +//! The display, the export and the thumbnail run the same generated shader, +//! and that shader always emits a block per active mask layer. Bind the empty +//! placeholder instead of the real array and every one of those blocks +//! multiplies by zero: the local adjustments are simply absent, with no error +//! and no warning. +//! +//! That is what happened — exports and thumbnails both took the unmasked +//! path — and it is invisible from inside either one. The only way to see it +//! is to render the same edit twice and compare, which is what this does. + +use dr_gpu::{AdjustPass, DemosaicedImage, GpuContext, MaskPass, SubjectMasks}; +use dr_pipeline::descriptor::ParamId; +use dr_pipeline::mask::{MaskLayer, MaskSource, MaskStack}; +use dr_pipeline::operation::compose_full; +use dr_pipeline::{ops, Framing}; +use dr_segment::Shaped; +use dr_types::ColourSpace; + +const PROXY: u32 = 24; + +fn ctx() -> Option { + pollster::block_on(GpuContext::new_headless()).ok() +} + +fn grey(ctx: &GpuContext, size: u32) -> DemosaicedImage { + let data: Vec = (0..size * size).flat_map(|_| [128, 128, 128, 255]).collect(); + DemosaicedImage::from_rgba8(ctx, &data, size, size).expect("upload") +} + +/// Coverage over the left half, as a detection would produce. +fn left_half() -> Vec { + (0..PROXY * PROXY) + .map(|i| if i % PROXY < PROXY / 2 { 255 } else { 0 }) + .collect() +} + +fn brightening_stack() -> MaskStack { + let mut stack = MaskStack::new(); + let mut layer = MaskLayer::new( + "m1", + MaskSource::Subject { + signature: 1, + index: 0, + class: "person".into(), + score: 0.9, + }, + ); + layer.set_param("exposure", ParamId("exposure"), 2.0); + layer.feather = 0.0; + stack.push(layer); + stack +} + +/// Render `stack` at `out` pixels, exactly as the session's shared helper +/// does: fields, array, then a masked render. +fn render_at(ctx: &GpuContext, stack: &MaskStack, out: u32) -> Vec { + let source = grey(ctx, 64); + + let coverage = left_half(); + let fields: Vec> = stack + .active() + .map(|_| { + Shaped::build( + &coverage, + PROXY as usize, + PROXY as usize, + 128, + dr_segment::Morphology::None, + 0.0, + ) + .distance + }) + .collect(); + let refs: Vec<&[f32]> = fields.iter().map(|f| f.as_slice()).collect(); + let subjects = SubjectMasks::upload(ctx, &refs, PROXY, PROXY).expect("upload"); + + let mut masks = MaskPass::new(ctx).expect("mask pass"); + let array = masks + .render(stack, None, Some(&subjects), PROXY, PROXY) + .expect("rasterise"); + + let shader = compose_full(&ops::chain(), &Framing::new(), ColourSpace::Srgb, stack); + let mut adjust = AdjustPass::new(ctx); + adjust + .render_masked(&source, &shader, out, out, Some(array)) + .expect("render"); + adjust.export_pixels().expect("readback").0 +} + +/// The same edit rendered *without* the array bound — what export and +/// thumbnail were doing. +fn render_unmasked(ctx: &GpuContext, stack: &MaskStack, out: u32) -> Vec { + let source = grey(ctx, 64); + let shader = compose_full(&ops::chain(), &Framing::new(), ColourSpace::Srgb, stack); + let mut adjust = AdjustPass::new(ctx); + adjust.render(&source, &shader, out, out).expect("render"); + adjust.export_pixels().expect("readback").0 +} + +fn luma(pixels: &[u8], size: u32, x: u32, y: u32) -> u8 { + pixels[((y * size + x) * 4) as usize] +} + +/// The fault itself, stated as a test: the two paths must not agree. +/// +/// If binding the array made no difference, the masks would not be reaching +/// the shader at all — which is precisely the bug this file exists for. +#[test] +fn binding_the_masks_changes_the_result() { + let Some(ctx) = ctx() else { + eprintln!("no adapter; skipping"); + return; + }; + let stack = brightening_stack(); + const OUT: u32 = 64; + + let masked = render_at(&ctx, &stack, OUT); + let unmasked = render_unmasked(&ctx, &stack, OUT); + + let left_masked = luma(&masked, OUT, 8, 32); + let left_unmasked = luma(&unmasked, OUT, 8, 32); + + assert!( + left_masked > left_unmasked + 40, + "with the array bound the masked half must brighten: {left_masked} \ + against {left_unmasked}" + ); + assert!( + (120..=136).contains(&left_unmasked), + "and without it the layer contributes nothing at all, silently: \ + {left_unmasked}" + ); +} + +/// A thumbnail is the same edit at a small size, so it must carry the same +/// local adjustments. This is the reported bug. +#[test] +fn a_thumbnail_sized_render_still_carries_its_masks() { + let Some(ctx) = ctx() else { + eprintln!("no adapter; skipping"); + return; + }; + let stack = brightening_stack(); + + for out in [16u32, 32, 64, 128] { + let pixels = render_at(&ctx, &stack, out); + let inside = luma(&pixels, out, out / 8, out / 2); + let outside = luma(&pixels, out, out - out / 8 - 1, out / 2); + + assert!( + inside > outside + 40, + "at {out}px the masked half should be brighter: {inside} against \ + {outside}" + ); + } +} + +/// One mask array, every output size. The array is rasterised in source space +/// and sampled through the framing map, so a thumbnail and a full-size export +/// must reach the same *proportion* of the frame — not merely both be +/// non-empty. +#[test] +fn the_mask_covers_the_same_fraction_at_every_size() { + let Some(ctx) = ctx() else { + eprintln!("no adapter; skipping"); + return; + }; + let stack = brightening_stack(); + + let fraction = |out: u32| { + let pixels = render_at(&ctx, &stack, out); + let lit = (0..out * out) + .filter(|i| pixels[(*i as usize) * 4] > 150) + .count(); + lit as f32 / (out * out) as f32 + }; + + let small = fraction(32); + let large = fraction(128); + assert!( + (small - large).abs() < 0.05, + "the same edit should mask the same share of the frame at any size: \ + {small:.3} at 32px against {large:.3} at 128px" + ); + assert!( + (0.4..0.6).contains(&small), + "and that share is the left half: {small:.3}" + ); +} diff --git a/core/dr-pipeline/src/lens.rs b/core/dr-pipeline/src/lens.rs index f0855f3..f6c0b99 100644 --- a/core/dr-pipeline/src/lens.rs +++ b/core/dr-pipeline/src/lens.rs @@ -54,7 +54,7 @@ use std::fmt::Write as _; -use crate::descriptor::{Attribute, OpDescriptor, ParamId}; +use crate::descriptor::{OpDescriptor, ParamId}; use crate::operation::{Helper, Uniform}; /// A coordinate-domain operation, applied before the source is sampled. @@ -205,6 +205,7 @@ fn sanitise(id: &str) -> String { #[cfg(test)] mod tests { use super::*; + use crate::descriptor::Attribute; use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor}; static DESC_A: OpDescriptor = OpDescriptor { diff --git a/core/dr-pipeline/src/operation.rs b/core/dr-pipeline/src/operation.rs index 66337fc..3a5df12 100644 --- a/core/dr-pipeline/src/operation.rs +++ b/core/dr-pipeline/src/operation.rs @@ -24,7 +24,7 @@ use std::fmt::Write as _; use dr_types::{ColourSpace, Transfer}; -use crate::descriptor::{Attribute, OpDescriptor, ParamId, Presentation}; +use crate::descriptor::{OpDescriptor, ParamId, Presentation}; use crate::framing::{Framing, FRAMING_UNIFORM_FIELDS}; use crate::mask::MaskStack; @@ -714,6 +714,7 @@ pub(crate) fn sanitise(id: &str) -> String { #[cfg(test)] mod tests { use super::*; + use crate::descriptor::Attribute; use crate::descriptor::{LocalizedKey, OpId, ParamDescriptor}; static DESC_A: OpDescriptor = OpDescriptor { diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index 39ada07..7367f3c 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -697,6 +697,38 @@ impl DevelopSession { /// case and the one that must cost nothing: the adjust pass then binds its /// own placeholder and the generated shader has no layer block to read it /// with. + /// Render `shader` at `w`×`h` with this edit's masks bound. + /// + /// **Every path that produces pixels must come through here.** The + /// generated shader always declares the mask binding and always emits a + /// layer block for each active layer; binding the empty placeholder + /// instead multiplies every one of them by zero. That is not an error and + /// logs nothing — the local adjustments simply are not there. Exports and + /// thumbnails both did exactly that. + /// + /// The mask array is rasterised in source space at proxy size and sampled + /// through the framing map, so one array is correct at every output size: + /// a 256px thumbnail and a 24 MP export bind the same texture. + fn render_with_masks( + &mut self, + shader: &dr_pipeline::operation::ComposedShader, + w: u32, + h: u32, + ) -> Result<(), String> { + let ctx = self.ctx.clone(); + self.ensure_subject_fields(&ctx); + + let masks = self + .rasterise_masks() + .then(|| self.masks.as_ref().and_then(|p| p.array())) + .flatten(); + + self.adjust + .render_masked(&self.demosaiced, shader, w, h, masks) + .map(|_| ()) + .map_err(|e| e.to_string()) + } + /// Returns whether the array is now valid for the current stack. /// /// Split from reading the array back because the render below needs @@ -843,8 +875,20 @@ impl DevelopSession { // The model reads the photograph as captured, not as edited: the // segmentation must survive an exposure change, or every slider would // invalidate the masks that depend on it (docs/segmentation.md §3). + // Timed and logged, because this blocks the interface and the size of + // that stall is the feature's largest open risk. A number from the + // device it actually runs on beats an estimate from the desktop. + let started = std::time::Instant::now(); let (rgb, rw, rh) = self.neutral_proxy(ctx, SEGMENT_PROXY_EDGE)?; + let proxied = started.elapsed(); + let seg = segmentation::compute(ctx, &rgb, rw, rh, options)?; + log::info!( + "segmented {rw}×{rh}: {} subject(s), proxy {:.0} ms, total {:.0} ms", + seg.instances().len(), + proxied.as_secs_f32() * 1000.0, + started.elapsed().as_secs_f32() * 1000.0, + ); if self.masks.is_none() { self.masks = MaskPass::new(ctx) @@ -1301,19 +1345,8 @@ impl DevelopSession { // Rasterise the masks first: the shader addresses array slices by // index, so the array has to describe *this* stack before it is bound. - // Any layer whose *shape* changed needs its field rebuilt before the - // rasteriser reads it. Keyed, so a feather drag reaches neither. - let ctx = self.ctx.clone(); - self.ensure_subject_fields(&ctx); - - let masks = self - .rasterise_masks() - .then(|| self.masks.as_ref().and_then(|p| p.array())) - .flatten(); - let texture = self - .adjust - .render_masked(&self.demosaiced, &shader, w, h, masks) - .map_err(|e| e.to_string())?; + self.render_with_masks(&shader, w, h)?; + let texture = self.adjust.output().ok_or("nothing was rendered")?; // The import is fallible on format and usage only, and both are fixed // in `AdjustPass`'s texture descriptor — so a failure here is a @@ -1436,9 +1469,7 @@ impl DevelopSession { let (w, h) = self.graph.output_size(sw, sh); let shader = self.graph.compose_for(space); - self.adjust - .render(&self.demosaiced, &shader, w, h) - .map_err(|e| e.to_string())?; + self.render_with_masks(&shader, w, h)?; let (pixels, rw, rh) = self.adjust.export_pixels().map_err(|e| e.to_string())?; dr_export::Frame::in_space(rw, rh, pixels, space).map_err(|e| e.to_string()) @@ -1465,9 +1496,7 @@ impl DevelopSession { let (w, h) = fit(fw, fh, edge.max(1), edge.max(1)); let shader = self.graph.compose_for(dr_types::ColourSpace::Srgb); - self.adjust - .render(&self.demosaiced, &shader, w, h) - .map_err(|e| e.to_string())?; + self.render_with_masks(&shader, w, h)?; let (pixels, rw, rh) = self.adjust.export_pixels().map_err(|e| e.to_string())?; Ok((rw, rh, pixels))