diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index 7367f3c..bb8a83b 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -9,6 +9,9 @@ //! declared [`ParamKind`], not from which parameter it is (ARCH §4.3), so a //! new operation appears in the panel with no change here (FR-DEV-3c). +use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; +use std::sync::Arc; + use dr_decode::RawImage; use dr_gpu::{ AdjustPass, DemosaicedImage, Demosaicer, GpuContext, Histogram, HistogramPass, MaskPass, @@ -33,7 +36,185 @@ use crate::ParamRow; /// milliseconds and its field a few megabytes. const SEGMENT_PROXY_EDGE: u32 = 1600; +/// Which photograph a piece of background work was started for. +/// +/// Minted per session, never reused, and carried by the work rather than +/// looked up when it finishes. A segmentation takes most of a second, so the +/// user can be two frames further on by the time one lands, and the answer to +/// "is this still wanted" has to be decided from what the work *was* rather +/// than from what happens to be open. +/// +/// The alternative — a counter beside the session slot, bumped on every open — +/// is written from four places in `lib.rs` and would apply one photograph's +/// subjects to another the first time somebody added a fifth and forgot. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct SessionId(u64); + +impl SessionId { + pub(crate) fn next() -> Self { + static NEXT: AtomicU64 = AtomicU64::new(1); + Self(NEXT.fetch_add(1, Ordering::Relaxed)) + } +} + +/// Says that nobody is waiting for a job's answer any more. +/// +/// Not a cancellation in the sense of stopping the work: the model is one +/// opaque call of about half a second and `ort` offers no way in. This is +/// checked at the seams there are — before the job starts, and again between +/// the proxy readback and the inference — so a job abandoned while the user +/// was still paging usually costs nothing, and one abandoned mid-inference +/// costs only the run it was already committed to. +/// +/// What it buys in every case is that the next photograph's segmentation is +/// the only one anybody is waiting on. +#[derive(Debug, Clone, Default)] +pub struct Abandon(Arc); + +impl Abandon { + pub fn now(&self) { + self.0.store(true, Ordering::Relaxed); + } + + pub fn asked(&self) -> bool { + self.0.load(Ordering::Relaxed) + } +} + +/// What a finished [`SegmentationJob`] hands back. +/// +/// The rasteriser travels with the subjects because it is needed the instant +/// they arrive and nowhere before. Building it is a shader compile — 23 ms on +/// a desktop, and compiling shaders is among the slowest things a mobile +/// driver does — so building it on adoption put a dropped frame on the one +/// redraw the user is waiting for. Here it is on the thread that was waiting +/// anyway. +/// +/// `None` where the device has no mask rasteriser at all: the session keeps +/// the photograph and loses local adjustments, which is the same bargain the +/// histogram makes. +pub struct Segmented { + seg: Segmentation, + masks: Option, +} + +/// TRACES: FR-DEV-3 +/// A segmentation lifted out of the session that asked for it. +/// +/// [`DevelopSession`] cannot go to a worker. Not because of what it holds — +/// the device, the source texture and the passes are all `Send` — but because +/// it lives behind one `Rc>>` that every callback in the +/// window reaches through, and the window has to keep reaching through it +/// while the work runs. Handing the session over would freeze the interface +/// exactly as thoroughly as blocking on it did. +/// +/// So the work takes a copy of the two things it needs. The device is `Arc`s, +/// the source is shared rather than copied, and the answer comes back as plain +/// data. +pub struct SegmentationJob { + ctx: GpuContext, + source: Arc, + session: SessionId, + abandon: Abandon, +} + +impl SegmentationJob { + /// The photograph this was started for. + pub fn session(&self) -> SessionId { + self.session + } + + /// The handle that tells this job its answer is no longer wanted. + pub fn abandon(&self) -> Abandon { + self.abandon.clone() + } + + /// TRACES: FR-DEV-3 + /// Find the subjects. **Blocking, and roughly two thirds of a second.** + /// + /// `Ok(None)` means abandoned rather than found-nothing: an image with no + /// recognisable subject in it still comes back as `Ok(Some(_))` with an + /// empty instance list, and the panel says so. + /// + /// There is deliberately no `&mut DevelopSession` in scope here. That is + /// the whole point of the split — a caller cannot accidentally hold the + /// session across the half second, because it was never given one. + pub fn run(&self, options: &segmentation::Options) -> Result, String> { + if self.abandon.asked() { + return Ok(None); + } + + // Timed and logged because this is the feature's largest cost and the + // split between the two halves decides where any further work goes. 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(SEGMENT_PROXY_EDGE)?; + let proxied = started.elapsed(); + + // The one seam inside the run. Past here the model owns the thread + // until it is done. + if self.abandon.asked() { + return Ok(None); + } + + let seg = segmentation::compute(&self.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, + ); + + let masks = MaskPass::new(&self.ctx) + .inspect_err(|e| log::warn!("no mask rasteriser on this device: {e}")) + .ok(); + Ok(Some(Segmented { seg, masks })) + } + + /// Render the *unedited* image to a CPU buffer at proxy size. + /// + /// 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). + /// + /// A throwaway [`AdjustPass`] with a neutral graph rather than the + /// session's own — which this could not reach from here in any case, and + /// must not: reusing it would overwrite the frame the histogram reads and + /// leave the view showing an unedited image until the next redraw. + /// + /// This is `export_pixels`, which is ungated: an export is not the display + /// round-trip AC-8 forbids, and neither is this. + fn neutral_proxy(&self, max_edge: u32) -> Result<(Vec, usize, usize), String> { + let (sw, sh) = self.source.size(); + let scale = (max_edge as f32 / sw.max(sh) as f32).min(1.0); + let (w, h) = ( + ((sw as f32 * scale) as u32).max(1), + ((sh as f32 * scale) as u32).max(1), + ); + + let neutral = EditGraph::default_chain(); + let mut pass = AdjustPass::new(&self.ctx); + pass.render(&self.source, &neutral.compose(), w, h) + .map_err(|e| format!("could not render the segmentation proxy: {e}"))?; + let (rgba, pw, ph) = pass + .export_pixels() + .map_err(|e| format!("could not read the segmentation proxy: {e}"))?; + + // Straight to float RGB, dropping alpha. The values stay display- + // encoded because that is what the model was trained on — one of the + // few places in this codebase where not linearising is correct. + let rgb = rgba + .chunks_exact(4) + .flat_map(|p| [p[0] as f32 / 255.0, p[1] as f32 / 255.0, p[2] as f32 / 255.0]) + .collect(); + Ok((rgb, pw as usize, ph as usize)) + } +} + pub struct DevelopSession { + /// This session's name, for work that outlives the frame it started on. + id: SessionId, /// Kept so the session can build GPU resources after construction. /// /// The distance fields behind a subject mask are made when a layer is @@ -50,7 +231,7 @@ pub struct DevelopSession { /// a history the *call sites* had to remember would be one press of undo /// away from wrong every time a control is added. history: History, - demosaiced: DemosaicedImage, + demosaiced: Arc, adjust: AdjustPass, /// TRACES: FR-DSP-7 /// Optional, because a session that cannot count its frames is still a @@ -142,10 +323,11 @@ impl DevelopSession { graph.set_orientation(orientation); let history = History::new(&graph); Self { + id: SessionId::next(), ctx: ctx.clone(), graph, history, - demosaiced, + demosaiced: Arc::new(demosaiced), adjust: AdjustPass::new(ctx), histogram: HistogramPass::new(ctx) .inspect_err(|e| log::warn!("no histogram on this device: {e}")) @@ -864,84 +1046,60 @@ impl DevelopSession { // Segmentation (S15, docs/segmentation.md) // ---------------------------------------------------------------------- + /// This session's name, carried by any work started against it. + pub fn id(&self) -> SessionId { + self.id + } + /// TRACES: FR-DEV-3 - /// Compute the region map this image's local masks select from. + /// Everything a segmentation needs, so it can be run somewhere else. /// - /// **Blocking, and roughly half a second.** The caller is responsible for - /// running it off the UI thread — see the worker in `lib.rs`. It is - /// exposed as a plain blocking call rather than something async because - /// what it needs is a GPU context and a CPU core, not a runtime. - pub fn segment(&mut self, ctx: &GpuContext, options: &segmentation::Options) -> Result<(), String> { - // 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) - .inspect_err(|e| log::warn!("no mask rasteriser on this device: {e}")) - .ok(); + /// Taking the job is cheap — two `Arc` bumps and a texture handle — and + /// nothing about the session is borrowed past the call, which is what + /// lets the window go on drawing while the answer is being found. + pub fn segmentation_job(&self) -> SegmentationJob { + SegmentationJob { + ctx: self.ctx.clone(), + source: self.demosaiced.clone(), + session: self.id, + abandon: Abandon::default(), } + } + + /// TRACES: FR-DEV-3 + /// Take on a segmentation found elsewhere. + /// + /// The caller is responsible for checking that this result was computed + /// for *this* session — see [`SegmentationJob::session`]. Nothing here can + /// tell one photograph's subjects from another's, and a mismatch is + /// silent: the masks would rasterise, the overlay would draw, and the + /// outlines would simply follow a subject that is not in the picture. + pub fn adopt_segmentation(&mut self, found: Segmented) { + // Kept rather than replaced where there is one already: a second + // segmentation of the same photograph would otherwise throw away a + // working rasteriser for an identical one. + self.masks = self.masks.take().or(found.masks); // The fields themselves are built per *layer*, on demand — there are // none yet, and building one per detected object would transform // several megapixels for masks the user may never make. - self.segmentation = Some(seg); + self.segmentation = Some(found.seg); self.subjects = None; self.subject_key = 0; - Ok(()) } - /// Render the *unedited* image to a CPU buffer at proxy size. + /// Find the subjects and take them on, blocking until both are done. /// - /// Goes through a throwaway [`AdjustPass`] with a neutral graph rather - /// than the session's own. Reusing `self.adjust` would overwrite the frame - /// the histogram reads and leave the view showing an unedited image until - /// the next redraw — a visible flicker for the sake of not allocating. - /// - /// This is `export_pixels`, which is ungated: an export is not the display - /// round-trip AC-8 forbids, and neither is this. - fn neutral_proxy( - &self, - ctx: &GpuContext, - max_edge: u32, - ) -> Result<(Vec, usize, usize), String> { - let (sw, sh) = self.demosaiced.size(); - let scale = (max_edge as f32 / sw.max(sh) as f32).min(1.0); - let (w, h) = ( - ((sw as f32 * scale) as u32).max(1), - ((sh as f32 * scale) as u32).max(1), - ); - - let neutral = EditGraph::default_chain(); - let mut pass = AdjustPass::new(ctx); - pass.render(&self.demosaiced, &neutral.compose(), w, h) - .map_err(|e| format!("could not render the segmentation proxy: {e}"))?; - let (rgba, pw, ph) = pass - .export_pixels() - .map_err(|e| format!("could not read the segmentation proxy: {e}"))?; - - // Straight to float RGB, dropping alpha. The values stay display- - // encoded because that is what the model was trained on — one of the - // few places in this codebase where not linearising is correct. - let rgb = rgba - .chunks_exact(4) - .flat_map(|p| [p[0] as f32 / 255.0, p[1] as f32 / 255.0, p[2] as f32 / 255.0]) - .collect(); - Ok((rgb, pw as usize, ph as usize)) + /// Test-only, and deliberately: a session-shaped blocking call is exactly + /// the shape that put two thirds of a second on the UI thread in the first + /// place, and leaving it public would invite the next caller to reach for + /// it. A test has nothing else to be doing. + #[cfg(test)] + fn segment(&mut self, options: &segmentation::Options) -> Result<(), String> { + if let Some(found) = self.segmentation_job().run(options)? { + self.adopt_segmentation(found); + } + Ok(()) } pub fn has_segmentation(&self) -> bool { @@ -2005,6 +2163,96 @@ mod tests { out } + // ---------------------------------------------------------------------- + // Segmentation off the UI thread + // ---------------------------------------------------------------------- + + /// The property the whole arrangement rests on. + /// + /// If someone puts an `Rc`, a `Cell` or a raw pipeline handle into + /// `SegmentationJob`, this stops compiling — which is the only warning + /// there would be, since the call site in `masks_ui` would then fail with + /// a lifetime error a long way from the cause. + #[test] + fn a_job_and_its_answer_can_cross_a_thread() { + fn is_send() {} + is_send::(); + is_send::(); + is_send::(); + } + + /// Two sessions over the same file are still two photographs as far as a + /// late result is concerned, because opening one twice is opening it + /// twice. + #[test] + fn every_session_has_its_own_identity() { + let Some(ctx) = headless() else { return }; + let rgba: Vec = (0..16 * 16).flat_map(|_| [128, 128, 128, 255]).collect(); + let open = || { + DevelopSession::open_rgb(&ctx, &rgba, 16, 16, dr_types::Orientation::NORMAL) + .expect("session") + }; + let (a, b) = (open(), open()); + assert_ne!(a.id(), b.id()); + assert_eq!(a.id(), a.id(), "and stable within one session"); + } + + #[test] + fn a_job_carries_the_session_it_was_taken_from() { + let Some(ctx) = headless() else { return }; + let rgba: Vec = (0..16 * 16).flat_map(|_| [128, 128, 128, 255]).collect(); + let session = + DevelopSession::open_rgb(&ctx, &rgba, 16, 16, dr_types::Orientation::NORMAL) + .expect("session"); + assert_eq!(session.segmentation_job().session(), session.id()); + } + + /// Abandoning before the run reaches the proxy must cost nothing at all — + /// this is the case that fires when the user pages on while a job is still + /// waiting for a thread. + #[test] + fn an_abandoned_job_does_no_work() { + let Some(ctx) = headless() else { return }; + let rgba: Vec = (0..16 * 16).flat_map(|_| [128, 128, 128, 255]).collect(); + let session = + DevelopSession::open_rgb(&ctx, &rgba, 16, 16, dr_types::Orientation::NORMAL) + .expect("session"); + + let job = session.segmentation_job(); + job.abandon().now(); + + let started = std::time::Instant::now(); + let out = job.run(&crate::segmentation::Options::default()); + assert!( + matches!(out, Ok(None)), + "abandoned is not an error and not an empty answer: {:?}", + out.map(|o| o.is_some()) + ); + assert!( + started.elapsed() < std::time::Duration::from_millis(50), + "it returned without loading the model" + ); + } + + /// A finished segmentation is adopted whole, and the session says so. + #[test] + fn adopting_a_result_gives_the_session_its_subjects() { + let Some(ctx) = headless() else { return }; + let rgba: Vec = (0..100 * 100).flat_map(|_| [128, 128, 128, 255]).collect(); + let mut session = + DevelopSession::open_rgb(&ctx, &rgba, 100, 100, dr_types::Orientation::NORMAL) + .expect("session"); + assert!(!session.has_segmentation()); + + let job = session.segmentation_job(); + let Ok(Some(found)) = job.run(&crate::segmentation::Options::default()) else { + eprintln!("no model; skipping"); + return; + }; + session.adopt_segmentation(found); + assert!(session.has_segmentation()); + } + // ---------------------------------------------------------------------- // The overlay's clip rectangle // ---------------------------------------------------------------------- @@ -2026,7 +2274,7 @@ mod tests { DevelopSession::open_rgb(ctx, &rgba, 100, 100, dr_types::Orientation::NORMAL) .expect("session"); session - .segment(ctx, &crate::segmentation::Options::default()) + .segment(&crate::segmentation::Options::default()) .ok()?; Some(session) } diff --git a/ui/dr-ui/src/lib.rs b/ui/dr-ui/src/lib.rs index c7d45ae..a4f2a61 100644 --- a/ui/dr-ui/src/lib.rs +++ b/ui/dr-ui/src/lib.rs @@ -1769,7 +1769,7 @@ pub fn run(paths: Vec) -> Result<()> { }); } - masks_ui::wire(&window, &session, &rows, &redraw, gpu.clone()); + masks_ui::wire(&window, &session, &rows, &redraw); { let weak = window.as_weak(); diff --git a/ui/dr-ui/src/masks_ui.rs b/ui/dr-ui/src/masks_ui.rs index d94f5f2..5f23d89 100644 --- a/ui/dr-ui/src/masks_ui.rs +++ b/ui/dr-ui/src/masks_ui.rs @@ -5,7 +5,7 @@ //! [`crate::segmentation`], which is the same split every other `*_ui` module //! in this crate draws. //! -//! # The one thing this module does decide +//! # The things this module does decide //! //! Selecting a mask layer re-scopes the adjust panel to that layer's chain. //! That happens because [`crate::develop::DevelopSession::rows`] answers @@ -13,16 +13,109 @@ //! `sync_rows` afterwards — there is no second panel and no duplicated //! control-building code. It is worth stating plainly because the absence of //! code is easy to mistake for an omission. +//! +//! And whether a segmentation that has finished is still wanted. Finding the +//! subjects takes most of a second on a 22 MP frame, so it runs on a worker +//! and the window polls for the answer — which means the answer can arrive +//! for a photograph the user has left. [`delivery`] is that rule, kept as a +//! named function with tests because both of its ways of being wrong are +//! silent: applied to the wrong image it draws outlines that follow a subject +//! which is not in the picture, and discarded too eagerly it throws away work +//! the user waited for. use std::cell::RefCell; use std::rc::Rc; use slint::{ComponentHandle as _, ModelRc, VecModel}; -use crate::develop::DevelopSession; +use crate::develop::{Abandon, DevelopSession, Segmented, SessionId}; use crate::segmentation; use crate::{sync_rows, AppWindow, MaskRow, ParamRow, SubjectRow}; +/// How often the window looks to see whether the model has finished. +/// +/// The interval `apply_when_ready` polls a sidecar fetch at, for the same +/// reason: a tick that finds nothing costs a `try_recv` on an empty channel, +/// and twenty a second is imperceptible against a result that took most of a +/// second to produce. +const POLL: std::time::Duration = std::time::Duration::from_millis(50); + +/// What to do with a segmentation that has finished. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Delivery { + Apply, + Discard, +} + +/// Does this answer still belong to the photograph on screen? +/// +/// The session a job was taken from names the photograph it is about, and +/// sessions are never reused — so re-opening the same file is a different +/// answer to this, correctly: the second session has no segmentation and its +/// panel says so. +fn delivery(computed_for: SessionId, open: Option) -> Delivery { + match open { + Some(id) if id == computed_for => Delivery::Apply, + _ => Delivery::Discard, + } +} + +fn open_session(session: &Rc>>) -> Option { + session.borrow().as_ref().map(|s| s.id()) +} + +/// The one segmentation that may be in flight. +/// +/// One at a time. Two runs would be two model loads, two proxy readbacks and +/// two cores for a single answer, and the second to land would overwrite the +/// first — so the cost buys nothing. The button is already insensitive while +/// `segmenting` is set, but that is a property of the window; this is the +/// invariant, so it lives where it can be tested. +#[derive(Default)] +struct Running { + /// The photograph the running job was started for. + started_for: Option, + /// Tells that job nobody wants its answer. + abandon: Abandon, + /// Held here rather than by its own closure. A timer that kept itself + /// alive to be able to stop itself would be an `Rc` cycle — one leaked + /// timer and one leaked channel per photograph segmented — and owning it + /// here also means starting the next job drops the previous timer instead + /// of leaving it polling a channel nothing will send on. + poll: Option>, +} + +impl Running { + /// Claim the slot for `id`, or refuse because a run for that same + /// photograph is already under way. + /// + /// A job left over from a photograph the user has since left is *not* a + /// reason to refuse: it is abandoned and displaced. Refusing would leave + /// the next photograph's "Find subjects" doing nothing for as long as a + /// run nobody wants takes to finish, which is exactly the wait this whole + /// change exists to remove. + fn start(&mut self, id: SessionId, abandon: Abandon) -> bool { + if self.started_for == Some(id) { + return false; + } + self.abandon.now(); + self.started_for = Some(id); + self.abandon = abandon; + true + } + + /// Give the slot up, if `id` still holds it. + /// + /// Abandoning on the way out covers the discard case and costs nothing in + /// the success case, where the run it names has already finished. + fn finish(&mut self, id: SessionId) { + if self.started_for == Some(id) { + self.abandon.now(); + self.started_for = None; + } + } +} + /// Push every mask-related property from the session into the window. pub(crate) fn sync(window: &AppWindow, session: &Rc>>) { let slot = session.borrow(); @@ -106,60 +199,50 @@ pub(crate) fn sync_overlay_view(window: &AppWindow, session: &DevelopSession) { } /// Install the panel's callbacks. -#[allow(clippy::too_many_arguments)] pub(crate) fn wire( window: &AppWindow, session: &Rc>>, rows: &Rc>, redraw: &Rc, - gpu: Option, ) { + let running: Rc> = Rc::default(); + // --- computing the region map ----------------------------------------- { let weak = window.as_weak(); let session = session.clone(); let redraw = redraw.clone(); let rows = rows.clone(); + let running = running.clone(); window.on_segment_image(move || { let Some(w) = weak.upgrade() else { return }; - let Some(ctx) = gpu.clone() else { - log::warn!("no GPU context; cannot segment"); + // Both the photograph and the device come out of the session, so + // no session is nothing to look at and nothing to look with. + let Some(job) = session.borrow().as_ref().map(|s| s.segmentation_job()) else { return; }; + let id = job.session(); - // **Blocking, on the UI thread, and flagged rather than hidden.** - // - // Half a second of watershed and inference. Moving it to a worker - // needs the session — which owns GPU resources and is behind a - // `RefCell` shared with every callback here — to be reachable from - // another thread, and that is a restructuring of the develop - // session rather than a change to this call. - // - // So it blocks, and the button says "Finding regions…" first: a - // frozen window with a stale label is the version of this that - // reads as a crash. See the note in `segmentation.rs` — this is - // the largest rough edge in the feature. + let claimed = running.borrow_mut().start(id, job.abandon()); + if !claimed { + return; + } + + // Set before the thread rather than by it, so there is no moment + // in which the press has been taken and nothing on screen says so. + // The button reads "Looking…" and goes insensitive off this. w.set_segmenting(true); - // Let the label reach the screen before the stall begins. - slint::Timer::single_shot(std::time::Duration::from_millis(16), { - let weak = w.as_weak(); - let session = session.clone(); - let rows = rows.clone(); - let redraw = redraw.clone(); - move || { - let Some(w) = weak.upgrade() else { return }; - let options = segmentation::Options::default(); - if let Some(s) = session.borrow_mut().as_mut() { - if let Err(e) = s.segment(&ctx, &options) { - log::warn!("segmentation failed: {e}"); - } - } - w.set_segmenting(false); - sync(&w, &session); - sync_rows(&w, &rows, &session); - redraw(&w); - } + + let (tx, rx) = std::sync::mpsc::channel(); + let options = segmentation::Options::default(); + std::thread::spawn(move || { + // A failed send means the window stopped waiting — the user + // moved on, or the app is closing. Neither is worth reporting: + // the answer was unwanted before it existed. + let _ = tx.send(job.run(&options)); }); + + watch(&w, id, rx, &session, &rows, &redraw, &running); }); } @@ -379,6 +462,77 @@ pub(crate) fn wire( } } +/// Wait for a segmentation without the window waiting with it. +/// +/// A repeating timer rather than a callback from the worker, because Slint +/// properties may only be touched from the thread that owns the event loop and +/// this is the shape `apply_when_ready` already uses for the sidecar fetch. +/// +/// Two things end the wait, and only one of them is the answer arriving. The +/// other is the photograph changing underneath: that is checked *first*, on +/// every tick, so the abandonment reaches the worker while it may still be in +/// the proxy — and so the next photograph's "Find subjects" is available +/// within a tick rather than at the end of a run nobody wants. +fn watch( + window: &AppWindow, + id: SessionId, + rx: std::sync::mpsc::Receiver, String>>, + session: &Rc>>, + rows: &Rc>, + redraw: &Rc, + running: &Rc>, +) { + let weak = window.as_weak(); + let session = session.clone(); + let rows = rows.clone(); + let redraw = redraw.clone(); + let slot = running.clone(); + + let timer = Rc::new(slint::Timer::default()); + let stop = Rc::downgrade(&timer); + timer.start(slint::TimerMode::Repeated, POLL, move || { + let done = || { + if let Some(t) = stop.upgrade() { + t.stop(); + } + }; + + if delivery(id, open_session(&session)) == Delivery::Discard { + // Nothing is touched on the way out. `segmenting` belongs to the + // photograph now open — which may well have a run of its own going + // — and the only route to here is through `reset`, which cleared + // it for that image already. + slot.borrow_mut().finish(id); + done(); + return; + } + + let Ok(answer) = rx.try_recv() else { return }; + slot.borrow_mut().finish(id); + done(); + + let Some(w) = weak.upgrade() else { return }; + match answer { + Ok(Some(found)) => { + if let Some(s) = session.borrow_mut().as_mut() { + s.adopt_segmentation(found); + } + } + // Abandoned. Unreachable from here in practice — the check above + // catches every case that raises the flag — and handled rather + // than asserted, because the cost of being wrong is one wasted + // sync against a panic in a photographer's hands. + Ok(None) => {} + Err(e) => log::warn!("segmentation failed: {e}"), + } + w.set_segmenting(false); + sync(&w, &session); + sync_rows(&w, &rows, &session); + redraw(&w); + }); + running.borrow_mut().poll = Some(timer); +} + /// Clear the panel when the open image changes. /// /// Its own function rather than a call to [`sync`] with an empty session, @@ -394,3 +548,101 @@ pub(crate) fn reset(window: &AppWindow) { window.set_subject_rows(ModelRc::new(VecModel::::default())); window.set_editing_mask(false); } + +#[cfg(test)] +mod tests { + use super::*; + + /// The ordinary case: the answer comes back to the photograph that asked. + #[test] + fn a_result_for_the_open_photograph_is_applied() { + let open = SessionId::next(); + assert_eq!(delivery(open, Some(open)), Delivery::Apply); + } + + /// The bug this rule exists for. Two thirds of a second is long enough to + /// press "Find subjects", think better of it and swipe to the next frame — + /// and the result that lands then describes a picture nobody is looking at. + #[test] + fn a_result_for_a_photograph_the_user_has_left_is_discarded() { + let asked = SessionId::next(); + let now_open = SessionId::next(); + assert_eq!(delivery(asked, Some(now_open)), Delivery::Discard); + } + + /// Back to the library, or a frame that failed to decode: there is no + /// session to apply anything to. + #[test] + fn a_result_arriving_with_nothing_open_is_discarded() { + assert_eq!(delivery(SessionId::next(), None), Delivery::Discard); + } + + /// Re-opening the same file is a new session, so a result outstanding from + /// the previous visit does not land in it. Conservative on purpose: the + /// alternative is keying on the path, and the second visit's panel would + /// then be filled from a proxy rendered before the first visit's edits. + #[test] + fn re_opening_the_same_file_does_not_inherit_a_result() { + let first_visit = SessionId::next(); + let second_visit = SessionId::next(); + assert_ne!(first_visit, second_visit); + assert_eq!(delivery(first_visit, Some(second_visit)), Delivery::Discard); + } + + /// Pressing the button twice must not put two model runs on two cores for + /// one answer. + #[test] + fn one_photograph_cannot_start_two_segmentations() { + let mut running = Running::default(); + let id = SessionId::next(); + assert!(running.start(id, Abandon::default())); + assert!(!running.start(id, Abandon::default()), "already looking"); + } + + /// And the other half of that rule: a run left over from a photograph the + /// user has left must not hold the slot against the one now on screen. + #[test] + fn a_new_photograph_displaces_a_run_nobody_is_waiting_for() { + let mut running = Running::default(); + let stale = Abandon::default(); + assert!(running.start(SessionId::next(), stale.clone())); + + assert!(running.start(SessionId::next(), Abandon::default())); + assert!( + stale.asked(), + "the displaced run is told its answer is unwanted" + ); + } + + /// A run that finishes releases the slot, so the same photograph can be + /// segmented again — after a failed attempt worth retrying, say. + #[test] + fn finishing_frees_the_slot() { + let mut running = Running::default(); + let id = SessionId::next(); + assert!(running.start(id, Abandon::default())); + running.finish(id); + assert!(running.start(id, Abandon::default())); + } + + /// A timer left over from an earlier job reports in after the slot has + /// moved on. It must not cancel the run that now holds it, or the user + /// would be able to start a third while the second is still going. + #[test] + fn a_late_finish_does_not_release_someone_elses_slot() { + let mut running = Running::default(); + let departed = SessionId::next(); + assert!(running.start(departed, Abandon::default())); + + let now_open = SessionId::next(); + let live = Abandon::default(); + assert!(running.start(now_open, live.clone())); + + running.finish(departed); + assert!(!live.asked(), "the live run is left alone"); + assert!( + !running.start(now_open, Abandon::default()), + "and it still holds the slot" + ); + } +} diff --git a/ui/dr-ui/src/segmentation.rs b/ui/dr-ui/src/segmentation.rs index 966f9b3..4234901 100644 --- a/ui/dr-ui/src/segmentation.rs +++ b/ui/dr-ui/src/segmentation.rs @@ -24,6 +24,11 @@ //! runs **once per image, when the user asks**, and never on the frame path. //! Every interaction it enables — click a subject, grow a mask, change a //! falloff — reads its cached output. +//! +//! All of it is on a worker. [`compute`] takes an owned buffer and a +//! `GpuContext`, which is what makes that possible — nothing here touches the +//! develop session, and [`crate::develop::SegmentationJob`] is the piece that +//! carries the proxy render across with it. use std::sync::Arc;