Stop the HD-DVD VTI read and the extract progress channel from hiding loss

Two audit fixes plus two consistency cleanups, all in the changed 1.6.5 surface:

- hddvd: the VTI clip-order read used `.ok()`, flattening an unreadable
  authored order (a scratched sector under the .vti — the name came from the
  directory, so it is never "absent") into "no order" with no diagnostic. The
  sibling clip-extent arms log every read failure with its code; this one now
  does too, then falls back to the per-clip heuristic exactly as an unauthored
  disc would. Behaviour is otherwise unchanged; the loud line is the point.

- extract: the progress sink hardcoded bytes_unreadable_total: 0 and counted
  every zero-filled hole as good, so a progress-only consumer saw a holed
  extraction climb to a clean 100%. Thread the running unreadable total through
  and report the real good/unreadable split. The authoritative ExtractResult was
  already truthful; only the live channel lied.

- labels: correct a stale comment that described testlog's old lock-based
  capture; it now installs one global subscriber and routes to a thread-local
  sink.

- ps: import SYSTEM_HEADER from consts instead of re-declaring 0xBB, matching
  its sibling stream-id constants and the file's single-source rule.
This commit is contained in:
Matthew Jackson
2026-08-19 09:10:26 -07:00
parent 341e079e1e
commit 7abcfaab72
4 changed files with 61 additions and 21 deletions
+25 -8
View File
@@ -237,6 +237,9 @@ impl Disc {
let mut result = ExtractResult::default();
let total_bytes = required;
let mut done_bytes: u64 = 0;
// Cumulative zero-filled-unreadable bytes, so the live progress channel
// can report the good/unreadable split instead of pinning unreadable at 0.
let mut done_unreadable: u64 = 0;
// CSS per-VTS key cache; only consulted for CSS discs.
let is_css = matches!(base_keys, DecryptKeys::Css { .. });
@@ -271,8 +274,15 @@ impl Disc {
// non-tolerate), so extract_one_file already zero-filled it and
// counted it in bytes_unreadable — one 'lost' bucket covers both
// media damage and decrypt failure.
let (fr, halted) =
extract_one_file(&mut dec, dest, pf, total_bytes, &mut done_bytes, opts)?;
let (fr, halted) = extract_one_file(
&mut dec,
dest,
pf,
total_bytes,
&mut done_bytes,
&mut done_unreadable,
opts,
)?;
result.bytes_good = result.bytes_good.saturating_add(fr.bytes_good);
result.bytes_unreadable = result.bytes_unreadable.saturating_add(fr.bytes_unreadable);
@@ -553,6 +563,7 @@ fn extract_one_file<S: SectorSource>(
pf: &PlannedFile,
total_bytes: u64,
done_bytes: &mut u64,
done_unreadable: &mut u64,
opts: &ExtractOptions,
) -> Result<(FileResult, bool)> {
let final_path = dest.join(&pf.host_rel);
@@ -582,7 +593,7 @@ fn extract_one_file<S: SectorSource>(
finalize_file(writer, &partial_path, pf.size, &final_path)?;
fr.complete = true;
*done_bytes = done_bytes.saturating_add(pf.size);
report(opts, *done_bytes, total_bytes);
report(opts, *done_bytes, *done_unreadable, total_bytes);
return Ok((fr, false));
}
@@ -620,7 +631,7 @@ fn extract_one_file<S: SectorSource>(
left -= n as u64;
}
fr.bytes_good = fr.bytes_good.saturating_add(hole_bytes);
let cont = report(opts, *done_bytes, total_bytes);
let cont = report(opts, *done_bytes, *done_unreadable, total_bytes);
if opts.cancelled(cont) {
return Ok((fr, true));
}
@@ -668,10 +679,11 @@ fn extract_one_file<S: SectorSource>(
}
write_all(&mut writer, &buf[..usable], &partial_path)?;
fr.bytes_unreadable = fr.bytes_unreadable.saturating_add(usable as u64);
*done_unreadable = done_unreadable.saturating_add(usable as u64);
}
written = written.saturating_add(usable as u64);
*done_bytes = done_bytes.saturating_add(usable as u64);
let cont = report(opts, *done_bytes, total_bytes);
let cont = report(opts, *done_bytes, *done_unreadable, total_bytes);
sector_off += batch;
if opts.cancelled(cont) {
// Leave the `.partial`; do NOT rename. The aggregate run
@@ -784,15 +796,20 @@ fn finalize_file(
/// Emit a progress report. Returns `true` to continue, `false` if the sink
/// requested an early stop (or there is no sink — always continue).
fn report(opts: &ExtractOptions, done: u64, total: u64) -> bool {
fn report(opts: &ExtractOptions, done: u64, unreadable: u64, total: u64) -> bool {
match opts.progress {
Some(p) => {
let pp = crate::progress::PassProgress {
kind: crate::progress::PassKind::Mux,
work_done: done,
work_total: total,
bytes_good_total: done,
bytes_unreadable_total: 0,
// `done` counts good AND zero-filled-unreadable bytes together;
// split them so a consumer driven only by the live progress
// channel sees a holed extraction as holed rather than as a clean
// climb to 100%. The final ExtractResult already carries the true
// split — this used to pin unreadable at 0 and call every byte good.
bytes_good_total: done.saturating_sub(unreadable),
bytes_unreadable_total: unreadable,
bytes_pending_total: 0,
bytes_retryable_total: 0,
bytes_total_disc: total,
+26 -4
View File
@@ -907,10 +907,32 @@ impl Disc {
}
// Authored clip order from the VTI clip table (empty if no VTI).
let order: Vec<String> = vti_name
.and_then(|n| udf_fs.read_file(reader, &format!("/HVDVD_TS/{n}")).ok())
.map(|b| parse_vti_clip_order(&b))
.unwrap_or_default();
//
// The VTI name came from `ts_dir.entries`, so a failed read here is a
// real I/O error (a scratched sector under the `.vti`), never an absent
// file. `.ok()` used to flatten the two — dropping the authored order
// with no diagnostic, so a split feature then composed from the per-clip
// heuristic and the operator was never told the authored order existed
// but could not be read. The clip-extent arms below log every read
// failure with its own code; this one now does too. The fallback itself
// is unchanged (no order => per-clip, exactly as an unauthored disc),
// because there is nothing to compose from without the table. Logging is
// exempt from the no-English rule (errors stay numeric).
let order: Vec<String> = match vti_name {
None => Vec::new(),
Some(n) => match udf_fs.read_file(reader, &format!("/HVDVD_TS/{n}")) {
Ok(bytes) => parse_vti_clip_order(&bytes),
Err(e) => {
tracing::warn!(
target: "freemkv::disc",
vti = ?n,
code = e.code(),
"authored clip order unreadable; falling back to the per-clip heuristic"
);
Vec::new()
}
},
};
// Resolve each clip's physical extents once, keyed by lower-case name.
let mut clip_extents: BTreeMap<String, (String, u64, Vec<Extent>)> = BTreeMap::new();
+9 -8
View File
@@ -1952,14 +1952,15 @@ mod apply_tests {
subtitle(0x12A2, "fra"),
],
)];
// Capture through the crate's ONE serialised sink. A process-wide
// `set_global_default` here would poison every other test's callsite
// interest cache for the rest of the binary — `tracing` caches interest
// GLOBALLY, and a global subscriber that answers `never` for foreign
// callsites hard-disables them, so `testlog::capture`'s scoped captures
// (e.g. the `freemkv::disc` log-accounting tests) then see nothing and
// flake. `testlog::capture` serialises every capture under one lock and
// installs no global default, which is the invariant those tests rely on.
// Capture through the crate's ONE global `tracing` subscriber:
// `testlog::capture` installs it exactly once and routes each event to a
// thread-local sink. A process-wide `set_global_default` HERE instead
// would poison every other test's callsite interest cache for the rest of
// the binary — `tracing` caches interest GLOBALLY — and only the first
// `set_global_default` in a process takes effect anyway. The shared
// subscriber answers interest for every callsite and isolates concurrent
// captures per thread, which is the invariant these log-accounting
// assertions rely on.
let ((), events) = crate::testlog::capture(|| {
apply_labels(&labels, &mut titles);
});
+1 -1
View File
@@ -17,7 +17,7 @@ use super::codec::startcode::find_start_code;
const PACK_HEADER_ID: u8 = 0xBA;
/// System header start code suffix.
const SYSTEM_HEADER_ID: u8 = 0xBB;
const SYSTEM_HEADER_ID: u8 = crate::consts::pes_stream_id::SYSTEM_HEADER;
/// Program end start code suffix.
const PROGRAM_END_ID: u8 = 0xB9;