dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions Packages Projects Releases 20 Wiki Activity

Find the subjects without stopping the window

Browse Source 
"Find subjects" took the UI thread for two thirds of a second on a 22 MP
frame — a proxy render, a readback and a YOLO pass through `ort` — and for
that time the interface was simply gone. The panel apologised for it rather
than hiding it: a "Looking…" label, and a 16 ms `single_shot` so the label
reached the screen before the freeze began, with a comment saying the obvious
fix needed the develop session restructured and was not being taken.

The obstacle was never `Send`. `DevelopSession` is `Send` — the device, the
source texture and the passes all are. What cannot go to a worker is the
`Rc<RefCell<Option<DevelopSession>>>` 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 what it needs instead. A `SegmentationJob` is the
device, the demosaiced source behind an `Arc`, and the name of the session
that asked. Taking one is two `Arc` bumps; running one is 495 ms on this
desktop; none of it touches the session, and there is deliberately no
`&mut DevelopSession` in scope for a caller to hold across it. The proxy
render travels with it rather than staying behind — a `GpuContext` and a
texture handle are both `Send`, and the model was never the only expensive
half. So does building the mask rasteriser, which is a shader compile:
adopting the result was costing 23 ms, a dropped frame on the one redraw the
user is waiting for, and the rasteriser is needed exactly when the subjects
arrive and never before. What is left on the UI thread is a microsecond.

The answer comes back through a channel a `slint::Timer` polls, which is the
shape `apply_when_ready` already uses for a sidecar fetch.

**A result can outlive the photograph it describes.** Two thirds of a second
is long enough to press the button, think better of it and swipe to the next
frame — and the result landing then would fill the panel with subjects that
are not in the picture, drawing outlines around a dog two photographs back.
Nothing downstream can tell: the masks rasterise and the overlay draws either
way. So every session is minted with an id, a job carries the id it was taken
from, and `delivery` compares the two before anything is applied. An id
rather than a counter beside the session slot, because that slot is written
from four places in `lib.rs` and the fifth would be the one that forgot.

A discard touches nothing on the way out. `segmenting` belongs to whichever
photograph is open now, which may well have a run of its own going, and
clearing it would re-enable a button that is correctly insensitive.

One run at a time, and abandonment is what stops that being a trap. A job
left over from a photograph the user has left is displaced rather than waited
for — otherwise the next frame's "Find subjects" would do nothing for the
length of a run nobody wants, which is the wait this exists to remove. `ort`
offers no way into the inference, so abandoning is checked at the seams there
are: before the job starts, and between the readback and the model. Abandoned
early it costs nothing, abandoned mid-inference it costs the run it was
already committed to, and either way the answer is dropped at the channel.

`DevelopSession::segment` survives as a test-only convenience. Left public it
is precisely the shape that put two thirds of a second on the UI thread in the
first place, and the next caller would reach for it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-22 12:26:11 +02:00
co-authored by Claude Opus 5
parent 586698db00
commit c0e1179936
4 changed files with 612 additions and 107 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ui/dr-ui/src/masks_ui.rs
+289 -37
View File
@@ -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<SessionId>) -> Delivery {
match open {
Some(id) if id == computed_for => Delivery::Apply,
_ => Delivery::Discard,
}
}
fn open_session(session: &Rc<RefCell<Option<DevelopSession>>>) -> Option<SessionId> {
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<SessionId>,
/// 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<Rc<slint::Timer>>,
}
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<RefCell<Option<DevelopSession>>>) {
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<RefCell<Option<DevelopSession>>>,
rows: &Rc<VecModel<ParamRow>>,
redraw: &Rc<dyn Fn(&AppWindow)>,
gpu: Option<dr_gpu::GpuContext>,
) {
let running: Rc<RefCell<Running>> = 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<Result<Option<Segmented>, String>>,
session: &Rc<RefCell<Option<DevelopSession>>>,
rows: &Rc<VecModel<ParamRow>>,
redraw: &Rc<dyn Fn(&AppWindow)>,
running: &Rc<RefCell<Running>>,
) {
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::<SubjectRow>::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"
);
}
}