diff --git a/CHANGELOG.md b/CHANGELOG.md index de0d94a..95ae168 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,50 @@ # Changelog +## 0.13.16 (2026-04-26) + +### Architecture: single `Progress` trait + `PassProgress` struct + +Pre-0.13.16 the rip API leaked internal mapfile concepts (`pos`, +`bytes_good`, `work_done`, `bytes_pending`, `Finished`/`NonTrimmed`) +into per-pass positional callbacks. Consumers reinvented the math each +time, and the v0.13.15 UI bug surfaced exactly because of this — autorip's +web JS computed `progress_pct` from `bytes_good` while the backend +computed from `pos`, silent drift, frozen UI bar. + +This release replaces both `Disc::copy::on_progress` and +`Disc::patch::on_progress` `Fn(u64, u64, u64)` callbacks with a single +`Progress` trait and `PassProgress` struct (new `progress` module). + +```rust +pub struct PassProgress { + pub kind: PassKind, // Sweep | Trim {reverse} | Scrape {reverse} | Mux + pub work_done: u64, + pub work_total: u64, + pub bytes_good_total: u64, + pub bytes_total_disc: u64, +} + +pub trait Progress { + fn report(&self, p: &PassProgress); +} + +impl Progress for F { ... } +``` + +Both `CopyOptions::on_progress` and `PatchOptions::on_progress` are +renamed to `progress: Option<&dyn Progress>`. Closure callers update +trivially; struct callers gain a clean named-field shape with no +positional-arg confusion. + +`PassKind` carries the semantic (sweep vs trim vs scrape vs mux) so +consumers can label phases without reinventing detection logic. The +`Mux` variant is reserved for v0.13.17 when the mux pipeline emits +progress; not yet emitted by libfreemkv code. + +`Disc::patch` reports `Trim {reverse}` for retry passes with +`block_sectors >= 2` and `Scrape {reverse}` when `block_sectors == 1` +(the per-sector final pass). Direction comes through `reverse: bool`. + ## 0.13.15 (2026-04-26) ### Breaking: `on_progress` callback gains `pos` parameter diff --git a/Cargo.toml b/Cargo.toml index cabc425..d0912c5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "libfreemkv" -version = "0.13.15" +version = "0.13.16" edition = "2024" rust-version = "1.86" license = "AGPL-3.0-only" diff --git a/src/disc/mod.rs b/src/disc/mod.rs index d5e6485..66b69ea 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -1418,9 +1418,15 @@ impl Disc { ); } - if let Some(cb) = opts.on_progress { + if let Some(reporter) = opts.progress { let stats = map.stats(); - cb(stats.bytes_good, pos, total_bytes); + reporter.report(&crate::progress::PassProgress { + kind: crate::progress::PassKind::Sweep, + work_done: pos, + work_total: total_bytes, + bytes_good_total: stats.bytes_good, + bytes_total_disc: total_bytes, + }); } } } @@ -1470,13 +1476,11 @@ pub struct CopyOptions<'a> { /// `skip_on_error`. The skipped region is marked `non-trimmed` for later /// trimming/scraping by `Disc::patch`. pub skip_forward: bool, - /// Callback fired per inner-loop iteration with - /// `(bytes_good, pos, total_bytes)`. `pos` is the current sweep - /// position — true Pass 1 progress, including skipped-forward NonTrimmed - /// ranges. `bytes_good` is the count of `Finished` (clean) sectors, - /// which doesn't advance through bad zones. UI should display `pos` - /// for "swept" progress and `bytes_good` for "real data recovered". - pub on_progress: Option<&'a dyn Fn(u64, u64, u64)>, + /// Per-iteration progress reporter. v0.13.16 architecture: the library + /// emits a single `PassProgress` shape via the `Progress` trait; + /// consumers compute their own derived percentages / ETAs from it. No + /// more positional `(bytes_good, pos, total)` callbacks. + pub progress: Option<&'a dyn crate::progress::Progress>, pub halt: Option>, } @@ -1520,10 +1524,8 @@ pub struct PatchOptions<'a> { /// bad zone and won't recover during this attempt. `0` disables the /// guard (run to completion or halt). pub wedged_threshold: u64, - /// Callback fired per inner-loop iteration with - /// `(bytes_good, pos, total_bytes)`. `pos` is the current LBA-byte - /// position within the patch walk; for reverse passes it counts down. - pub on_progress: Option<&'a dyn Fn(u64, u64, u64)>, + /// Per-iteration progress reporter. See `CopyOptions::progress`. + pub progress: Option<&'a dyn crate::progress::Progress>, pub halt: Option>, } @@ -1604,6 +1606,12 @@ impl Disc { if opts.reverse { bad_ranges.reverse(); } + // work_total = sum of all bad-range bytes. on_progress's third arg + // is this value; the second arg (work_done) is incremented per block + // attempted. UI consumers (autorip) compute pass_progress_pct = + // work_done / work_total — true 0..100% per pass per RIP_DESIGN.md §16. + let work_total: u64 = bad_ranges.iter().map(|(_, sz)| *sz).sum(); + let mut work_done: u64 = 0; tracing::trace!( target: "freemkv::disc", phase = "patch_start", @@ -1612,6 +1620,7 @@ impl Disc { reverse = opts.reverse, wedged_threshold = opts.wedged_threshold, num_ranges = bad_ranges.len(), + work_total, "Disc::patch entered" ); @@ -1694,9 +1703,30 @@ impl Disc { break 'outer; } - if let Some(cb) = opts.on_progress { + // Track work done in this pass for the per-pass progress bar. + // Each block iterated counts as work, regardless of read + // outcome — a failed retry is still progress through the + // bad-range walk. + work_done = work_done.saturating_add(block_bytes); + + if let Some(reporter) = opts.progress { let s = map.stats(); - cb(s.bytes_good, pos, total_bytes); + let kind = if block_sectors == 1 { + crate::progress::PassKind::Scrape { + reverse: opts.reverse, + } + } else { + crate::progress::PassKind::Trim { + reverse: opts.reverse, + } + }; + reporter.report(&crate::progress::PassProgress { + kind, + work_done, + work_total, + bytes_good_total: s.bytes_good, + bytes_total_disc: total_bytes, + }); } } } diff --git a/src/lib.rs b/src/lib.rs index 282f028..aadecfd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -88,6 +88,7 @@ pub mod mux; pub mod pes; pub(crate) mod platform; pub mod profile; +pub mod progress; pub mod scsi; pub mod sector; pub(crate) mod speed; diff --git a/src/progress.rs b/src/progress.rs new file mode 100644 index 0000000..50a8b15 --- /dev/null +++ b/src/progress.rs @@ -0,0 +1,70 @@ +//! Pipeline-progress reporting for the rip pipeline. +//! +//! v0.13.16 architecture rule: ONE progress signal type. Every long-running +//! pipeline operation (`Disc::copy`, `Disc::patch`, mux) emits the same +//! `PassProgress` shape via the `Progress` trait. Consumers (autorip) compute +//! a single `PipelineStats` derived view and never reach into per-pass +//! internals. +//! +//! Why this matters: pre-0.13.16 the API leaked `pos`, `bytes_good`, +//! `work_done`, `bytes_pending`, `Finished/NonTrimmed` mapfile semantics — +//! and consumers reinvented the math each time they wanted a percentage. +//! UIs ended up reading one source while server-side computed from another, +//! producing wrong percentages without anyone noticing. + +/// Identifies which pipeline phase the progress event belongs to. +/// +/// Consumers can render a phase-specific label (e.g. "Sweep", "Trim +/// (reverse)", "Scrape", "Mux") or just use a generic "Pass N" label. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PassKind { + /// `Disc::copy` — initial sweep across the entire disc. + Sweep, + /// `Disc::patch` retry pass with `block_sectors >= 2`. `reverse=true` + /// means walking bad ranges from highest to lowest LBA. + Trim { reverse: bool }, + /// `Disc::patch` final pass at 1 sector per block. + Scrape { reverse: bool }, + /// Demux ISO → output (MKV / M2TS / network). Single phase that runs + /// after all rip passes complete. + Mux, +} + +/// One progress sample from a pipeline phase. +/// +/// `work_done / work_total` is the per-pass percentage — always 0..=100% +/// regardless of which kind of pass is running. `bytes_good_total` is the +/// cumulative count of confirmed-clean bytes across the whole rip; useful +/// for the "data recovered" stat the user sees. +#[derive(Debug, Clone, Copy)] +pub struct PassProgress { + pub kind: PassKind, + /// Bytes processed in this pass so far. Monotonically non-decreasing. + pub work_done: u64, + /// Total bytes this pass will process. Constant for the duration of + /// the pass. + pub work_total: u64, + /// Cumulative bytes confirmed clean (`Finished` mapfile state) across + /// every pass run on this rip. Doesn't change across pass boundaries. + pub bytes_good_total: u64, + /// Total disc capacity in bytes. Constant. + pub bytes_total_disc: u64, +} + +/// A consumer of pipeline progress events. Library code calls +/// `Progress::report` once per inner-loop iteration (throttling is the +/// consumer's job — `report` is cheap; the library doesn't gate it). +/// +/// No `Send`/`Sync` bound — `report` is always called from the same thread +/// running the rip pipeline, so closures with non-`Sync` captures (e.g. +/// `RefCell`) work directly. Blanket impl below lets +/// callers pass closures without explicit struct types. +pub trait Progress { + fn report(&self, p: &PassProgress); +} + +impl Progress for F { + fn report(&self, p: &PassProgress) { + (self)(p) + } +} diff --git a/tests/integration_progress_and_halt.rs b/tests/integration_progress_and_halt.rs index ef2ff9e..1729628 100644 --- a/tests/integration_progress_and_halt.rs +++ b/tests/integration_progress_and_halt.rs @@ -175,17 +175,25 @@ fn test_disc_copy_progress_callback_fires() { let calls = Arc::new(AtomicU64::new(0)); let last_bytes = Arc::new(AtomicU64::new(0)); - let calls_cb = calls.clone(); - let last_bytes_cb = last_bytes.clone(); - let progress = move |bytes: u64, _pos: u64, _total: u64| { - calls_cb.fetch_add(1, Ordering::Relaxed); - last_bytes_cb.store(bytes, Ordering::Relaxed); + struct CountingReporter { + calls: Arc, + last_bytes: Arc, + } + impl libfreemkv::progress::Progress for CountingReporter { + fn report(&self, p: &libfreemkv::progress::PassProgress) { + self.calls.fetch_add(1, Ordering::Relaxed); + self.last_bytes.store(p.bytes_good_total, Ordering::Relaxed); + } + } + let reporter = CountingReporter { + calls: calls.clone(), + last_bytes: last_bytes.clone(), }; let opts = CopyOptions { decrypt: false, - on_progress: Some(&progress), + progress: Some(&reporter), ..Default::default() };