3ed86305350cf0c9494858f943e02cac22274d3d
15
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Matthew Jackson
|
3ed8630535 |
Take a lone sample window from the middle of its extent
A title cut into 50-odd clips gives each extent one window's worth of budget. At the extent's head, every clip is sampled at the same relative position and the first clip's window lands on the opening of the feature -- the one stretch with no subtitles in it. |
||
|
Matthew Jackson
|
5c8b4dc7c5 | Note the bounded display-count over-count on a stalled read retry | ||
|
Matthew Jackson
|
841aa1a1c6 |
Drain a sampled run's tail and merge repeated extent memos
A window's last PES stays open in the demuxer until the next PUSI, which under sampling is in a different window or nowhere — so the last display set of every window was discarded, worst where the sample is thinnest. Flush the demuxer at the end of each window. A playlist that lists the same clip twice now merges the second read into that extent's memo (facts OR'd, display count MAXed, coverage the larger of the two) instead of overwriting it. |
||
|
Matthew Jackson
|
14049bb477 | Document the forced-probe redesign in the changelog | ||
|
Matthew Jackson
|
8ffce6b621 |
Sample PGS across the title instead of reading its head
The content-based forced-subtitle probe spent its whole 256 MiB budget
on the first sectors of a title. A feature's subtitles begin minutes in,
so the probe read the opening logos, hit the budget, observed no display
set at all and contributed nothing to any verdict — the vendor label was
always the only input.
The forced predicate is asymmetric: one non-forced display set disproves
forced permanently, while proving forced needs the whole track, and
genuine forced tracks are tiny where full tracks are huge. So the same
budget is now SPREAD over each extent in ~16 MiB windows placed on the
AACS unit grid, sized in proportion to the extent, ending at the extent's
end. Cost is unchanged; placement is not.
Also:
* Per-track early exit. A track that is disproven (and whose label
needs no correcting) stops asking for budget; an extent that owes
evidence only for such tracks is skipped outright, and evidence
already in the cache is never demuxed a second time.
* Content may now DEMOTE a wrong vendor forced flag, in the probe and
in the muxer, behind one shared guard: absence of forced_on_flag
only means something if some other track demonstrably uses it, and
the track must have the shape of a full dialogue track rather than
of a forced-narrative one. On a disc where no track sets the flag,
nothing is demotable.
* A sampled or budget-cut extent's evidence is memoised with the
COVERAGE behind it. It used to be filed under the extent's full key
and replayed to playlists that would have read far more of the clip,
turning a prefix into an absence claim about the whole extent.
|
||
|
Matthew Jackson
|
2efe1425d6 |
refactor(decrypt): one orchestrator owns the no-key decision
There were TWO top-level decrypt paths: decrypt_sectors_impl for CSS and clear media, whose AACS arm was a bare `return Err` stub, and a wholly separate decrypt_sectors_mapped for AACS. Each scheme therefore decided its own answer to "there is no key for these bytes", and nothing held them to the same one. They drifted, in opposite directions, within a single release: css::descramble_region descrambled with a key the sector's own crib had just proven stale — garbage behind an intact clear header, reported Ok. the mapped path returned early for any LBA outside every range, before ever asking whether those bytes were ciphertext, so an unkeyable encrypted unit passed through and extract counted it as good. Both were fixed individually earlier today. This removes the shape that allowed them. decrypt_span is now the single orchestrator: it owns the loop, the refusal, and the loss count, and each scheme supplies only what genuinely differs. apply_aacs_map is a scheme step that reports what it could not open; it no longer decides what that means. The public wrappers (decrypt_sectors, _in_content, _mapped) are unchanged in signature and all funnel through it. Adding a scheme now means adding an arm here, which means answering the refusal question. That is the point. The new test asserts ONE verdict across all three schemes — AACS with no map, AACS with an encrypted unit outside every range, and CSS whose re-crack failed — plus that clear media is NOT a refusal. A per-scheme test cannot hold this: each would keep passing while the two disagreed. Flipping the AACS arm back to pass-through reds it. Also removes the last of the tracing-capture scaffolding. Serialising those captures crate-wide did not fix the 1-in-10 flake, and asserting the predicates directly made the helper, both capture subscribers and an unrelated dead OrderSink unused. Deleted rather than left behind. |
||
|
Matthew Jackson
|
b86f7aef17 |
fix(session): delete two dead accessors, make into_drive fallible
drive() and drive_mut() had ZERO callers — not in libfreemkv, freemkv, autorip, bdemu, keysources or kdb. Deleted rather than converted: dead public API that panics is not an API worth preserving the shape of. into_drive() had two callers and now returns Result. The empty-slot state is reachable through ordinary public use — stage_drive_as_reader moves the drive into the reader slot, and calling into_drive twice moves it out — so the panic was not guarding a caller error. identify() was converted for exactly this reason in this same release; the fix went to one of four public sinks and the other three were left. I deferred this on the assumption the blast radius was large. It was three call sites. Checking beats assuming. Also fixes a REAL FLAKE in the gate, which is worth more than the above. resolve_vid_only_bus_key_gate_reports_true_has_volume_id... failed about one full-suite run in ten while passing every time in isolation. It installed a capturing tracing subscriber to read back the has_volume_id field of a warn. That cannot be made reliable: dispatcher::set_default is THREAD-LOCAL while tracing's callsite-interest cache is GLOBAL. The original author knew, and called rebuild_interest_cache() — necessary but not sufficient. I first serialised every capture in the crate behind one lock (harness::with_captured_tracing, which also removed the same hand-rolled dance from three other sites). Still 1-in-10, because the cache can be re-evaluated against the process-default dispatch rather than the thread-local one. So the predicate is now a named function, handshake_has_volume_id, and the test asserts the VALUE. A boolean does not need a subscriber to check. The gate's hard-error behaviour keeps its own test. Measured: 14 consecutive full-suite runs, 2994 passed, 0 failed. A flaky gate is worse than a missing one — every green after it means less, and this one had been eroding trust in the whole suite. |
||
|
Matthew Jackson
|
5360f8d309 |
test: salvage the orphaned labels/disc triage, and extract build_labels
Thirteen agents triaging src/labels and src/disc died on a saturated
machine, leaving 5,836 insertions across 28 files uncommitted in a
worktree. Recovered by 3-way apply onto twelve commits of drift; zero
conflicts. The diff was archived to freemkv-private first, because a
worktree is not a backup and this one had already nearly been lost.
One production change, and it is the right one: mpls_universal::parse
read every playlist off the disc AND converted the entries to labels in
a single function, so the conversion — stream-type mapping, dedup key,
the dense global counters — could only be reached through a synthetic
UDF image. Extracted to build_labels(&[Playlist]), which unit tests can
drive from already-parsed values. Behaviour-preserving: same iteration
order, same skip-on-error.
Two collisions resolved by hand:
A second mod pass_progress_tests, written independently against the
same survivors as the one committed in
|
||
|
Matthew Jackson
|
58bdb42f8e |
Group whole E-AC-3 frame sets, and keep short reads unit-aligned
Two defects in fixes landed the same day, both found by round 4 auditing round 3's work rather than trusting it. **The E-AC-3 grouping ignored substreamid, so the timeline still doubled.** Confirmed independently by the correctness and conformance lenses and verified by hand: substreamid appeared only in test helpers, never in the production path. Per ETSI TS 102 366 (A/52) Annex E a frame set is independent substream 0 — mandatory, always first — with its dependents, then the OPTIONAL additional independent substreams 1..7 with theirs, all covering the same time period. Treating an additional independent substream as a new access unit advanced the clock a second time for the same 32 ms, which is exactly the doubling the grouping fix existed to prevent. No fixture caught it because every fixture used substreamid 0. is_dependent_substream becomes substream_role -> Starts | Extends: strmtyp 1 extends; strmtyp 0/2 with substreamid != 0 now extends (this was the bug); strmtyp 0/2 with substreamid == 0, legacy AC-3, and reserved strmtyp 3 start. Reserved 3 starts regardless of its id bits, because its BSI layout is undefined so those bits cannot be trusted — an unknown frame is neither merged into an unrelated programme nor silently discarded. The frame set stays ONE sample rather than being split into a separate track for the associated service, and the reasoning is in the module doc: a substream numbered 1..7 with no substream 0 is not conforming, so extracting one would mean renumbering ids and rebuilding frame sets — a transcode, not a remux. Programme selection is the player's job. A stream joined mid-frame-set (first sync is substreamid 3) is skipped with a debug and resyncs at the next id-0, mirroring the orphan-dependent rule: its mandatory id-0 substream was never seen, so it is neither decodable alone nor timeable. MAX_AC3_BUF 128 KiB -> 1 MiB, because an AU is now a whole frame set: worst case 8 independent x 9 substreams x 8192 B = 576 KiB, which the old cap could have dropped mid-hold. **The forced probe's two round-3 fixes cancelled each other.** CHUNK_SECTORS = 1023 exists (with a const assert) so every read starts on a 3-sector AACS aligned-unit boundary; the short-read fix advanced by actual bytes, making the advance a non-multiple of 3. Every later read was then misaligned, DecryptingSectorSource refused it before reading, the stop became ReadFailed, and no verdict was asserted — so content-based forced detection silently fell back to the vendor label on exactly the encrypted discs the 1023 change was written for. A partially-satisfied read now advances only by whole aligned units and re-reads the <=2 residue sectors from the next boundary, feeding only the aligned prefix so nothing is double-fed and no partial unit reaches the parsers. A read that fully satisfies its request still advances by all of it. When less than one aligned unit comes back the bytes are fed and the same LBA is retried twice before stopping, so a starved source cannot spin — verified by raising the retry limit and watching the test hang. Verified red independently here: reverting substream_role to strmtyp-only fails eac3_additional_independent_substream_stays_in_the_frame_set (6 access units where 3 are correct — the doubling, literally) and the mid-frame-set resync test. Reported, not fixed: dec3_box still hardcodes num_ind_sub - 1 = 0 and num_dep_sub = 0, so it under-declares any stream carrying additional independent or dependent substreams now that frame sets arrive whole. DolbyConfig has no fields for either; a real fix needs the parser to surface observed substream counts. That file is another lens's this round. Unverified: no real multi-programme DD+ stream exists here, so defect 1 rests on synthetic Annex-E fixtures. The retail DD+ check (No Time to Die, all substreamid 0) confirms single-programme discs are unaffected. |
||
|
Matthew Jackson
|
5f8dc392c0 |
Sweep the pinned toolchain to Rust 1.97
The Windows UI needs current winsafe, whose real minimum is 1.89 (its manifest under-declares 1.87 while it uses NonNull::from_ref). Rather than stop at the minimum, this goes to current stable and fixes what that costs. The counter-intuitive result: 1.97 is CHEAPER than 1.89. libfreemkv had 54 clippy errors at 1.89 and 6 at 1.97, because clippy tightened the noisy collapsible_if lint in between. Stopping at the minimum would have been the most expensive choice available. Roughly 47 lints across the eight repos, the large majority auto-fixed: libfreemkv 6, freemkv-engine 14, bdemu 8, freemkv-keysources 7, autorip 6, freemkv-unlock 3, freemkv-i18n 3. The hand-fixed ones are a descending sort to sort_by_key(Reverse), four manual checked-division sites, a loop counter replaced by enumerate, and a loop whose first let-else became a while-let. Worth recording for whoever bumps next: clippy is MSRV-AWARE. Those 54 lints only appear once the crate DECLARES 1.89 or later, because let-chains become available. A bare `cargo +1.89 clippy` against a manifest still pinned at 1.87 reports clean and is meaningless — gate with the real precommit script, which is also the only thing that covers build scripts. The pin still sits below the Mac default, so it keeps doing its job: catching lint drift locally before CI sees it. |
||
|
Matthew Jackson
|
a32373ff40 |
Fix fifteen defects across perf, resource, panics and key hygiene
All 21 findings held up under verification; 15 fixed here, 6 deferred to files another agent held this round, 0 rejected. **A defect in my own round-2 probe fix.** CHUNK_SECTORS was 1024, and 1024 % 3 == 1 — verified — so every chunk after the first was misaligned against the 6144-byte AACS aligned unit and would be REJECTED by DecryptingSectorSource's alignment gate. On an encrypted disc the forced-subtitle probe I added last round would have read almost nothing past its first chunk. Now 1023 sectors (341 aligned units) with a const assertion that fails the build if it stops dividing, plus set_unit_base per extent so the source's gate is anchored where the extent actually starts. **The same probe skipped sectors on a short read**, advancing by the REQUESTED count rather than the bytes actually returned, so a partial read silently left a gap in the middle of the evidence. It now advances by n/SECTOR_BYTES and clamps n to the buffer. **Its cache key omitted the PGS PID set**, so a playlist declaring an extra subtitle PID got another playlist's verdict for a track that had never been probed. And the key was the whole extent list, so partial clip sharing missed entirely. Both fixed by keying (start_lba, sector_count, pid) — and per-extent keying was shown SOUND rather than assumed: ForcedTracker is two monotone booleans, so per-extent evidence composes by field-wise OR, order- and grouping-independently. Making that honest required per-extent demux state, so an extent's evidence comes only from its own bytes, and memoising only extents whose read reached a designed stop. **A reachable panic in the timeline.** mkvstream::parse_block accepts a TimestampScale up to i64::MAX, so a video frame can set high_ns = i64::MAX and the next passive frame panicked adding the backstep. In release it wrapped negative instead, firing the straggler clamp for essentially every passive frame — audio and subtitles rewritten onto the wrong point of the output timeline. All four sites saturate. **A public constructor divided by zero**: PrefetchedSectorSource::new_with_events with unit_align == 0. Now InvalidInput, matching its batch_sectors sibling. **Two Debug impls printed key material.** DiscInputs (volume_id, mkb, unit_key_ro, samples) and UnitKeyFile both derived Debug. Nothing logs them today — fixed as prevention, because the next tracing::debug! someone adds is the leak. A doc claim that DiscInputs "contains no secrets" was false and is corrected. **An env-var multiply could overflow** in file_sector_source; now bounded at 64 GiB like its writeback sibling, with the parse split out so the bound is testable without touching process env. **The mp4 demuxer allowed one sample per file byte** — ~64x RAM amplification. Now file_len/16, since only vide/soun tracks are indexed and the shortest legal AC-3 frame is 128 bytes. **Two pipeline concurrency defects**: a consumer apply() error was invisible to the producer, and abandon/finalise had a TOCTOU where a caller could report an unfinalised output. Both fixed with compare-exchange state rather than a bool. **Two per-frame copies removed**, both MEASURED rather than reasoned: the AU assembler now hands its allocation to the frame (same pointer, unchanged capacity, proven by asserting the pointer) and tsmux reuses one Annex-B buffer across frames. Both keep capacity deliberately — a naive split_off would have cost more than it saved. **A comment pointed at the wrong file** for a mirrored constant; the mirror is now compiler-enforced with a const assertion converting 90 kHz ticks to ns, so drift fails the build. Deferred to another agent's files, all confirmed: detect_rate's fractional-twin snap, the mp4 reserve's u32 truncation, round_up_grain's overflow, the quadratic base-key gap fill, and MkvStream's frame cap counting frames rather than bytes. Every fix verified red by reverting it. Also noted for later: DecodeSampleSet still derives Debug over multi-MB of on-disc ciphertext. |
||
|
Matthew Jackson
|
807eb053ca |
Only assert a forced-subtitle verdict the read actually supports
probe_and_set_forced broke out of its read loop on a read error but still
applied whatever partial observation it had accumulated as an authoritative
verdict, overwriting the vendor-label-derived forced flag. A disc that faults
early could have a correct flag replaced by a guess from a fraction of the data.
The file already got the zero-observation case right — it deliberately leaves
the vendor flag alone rather than "assert not-forced from having seen nothing".
The defect was that a PARTIAL observation cut short by a fault was treated as
complete.
The fix rests on the two verdicts not being symmetric evidence.
settled_not_forced() is POSITIVE evidence — a non-forced display set was
actually seen, and no unread data can retract it. is_forced() is an ABSENCE
claim — display sets were seen and none was non-forced — which is only sound if
the read got far enough for the absence to mean something.
So every loop exit now yields a named StopReason, and absence claims are
asserted only for a designed stop:
* Exhausted / Budget → conclusive. The budget is deliberately conclusive: the
natural exit is "every track settled not-forced", which a genuinely forced
track never satisfies, so the budget is the ONLY path by which a real forced
verdict is ever reached. Treating it as inconclusive would disable forced
detection entirely.
* Halted / ReadFailed → inconclusive. A cancelled probe's cut-off point is as
arbitrary as a faulted one, so its absence claim is worth no more.
Evaluated per track, matching the existing observed() gate: a track that already
saw a non-forced set keeps its sound verdict even on a truncated run, while a
forced-so-far sibling keeps the vendor flag.
An inconclusive run is also NOT memoised. The cache key is the extent list and a
disc's playlists share clips, so caching a truncated run would replay one read
fault onto every playlist referencing those extents and deny any later title the
chance to re-read them.
Four tests; three of them verified red by forcing absence_is_conclusive() back
to always-true (the old semantics), while the budget test correctly stays green
either way — confirming the guard was not over-corrected.
Also moves the function doc comment back onto probe_and_set_forced; the earlier
probe commit left it attached to the ForcedProbeCache type alias.
|
||
|
Matthew Jackson
|
a39045adf1 |
Bound the forced-subtitle probe, make it cancellable, and stop re-reading clips
The probe's only natural exit was "every PGS track has shown a non-forced
display set". A genuinely FORCED track never satisfies that, so on the common
authoring — a forced-narrative track for foreign dialogue — the loop read the
title's entire extent set at 2 MiB per call with no byte cap, no time cap and
no halt check. It was also invoked once per title rather than once per distinct
clip, and a disc's playlists overwhelmingly reference the same few clips (main
feature, play-all, seamless-branch variants), so the same physical extents were
re-read 30-150 times. The two defects multiplied: tens of GB, times the
playlist count, off an optical drive.
Reached via ScanOptions::probe_forced_subtitles, whose only consumer is
`freemkv info -v` (freemkv/src/disc_info.rs). The rip path leaves it off. So
the symptom is `info -v` never returning on an ordinary UHD, not a corrupt rip.
Three changes:
* PROBE_BUDGET_SECTORS caps a probe at 256 MiB. A forced track's display sets
appear throughout the title, so a bounded prefix classifies it; the budget
only decides how long we keep looking for a non-forced set before accepting
the forced verdict.
* ScanOptions::halt is now plumbed in and checked per chunk, so `info -v` is
cancellable. The probe previously took no halt at all.
* A ForcedProbeCache memoises verdicts against the title's exact extent list.
Keying on byte-identical input means a hit cannot change any result — it
only removes the re-read.
Verdict application is factored into apply_verdicts so the cached and freshly
probed paths cannot diverge.
Three tests pin the behaviour, each against a reader that counts sectors and
never ends: the budget stops at exactly PROBE_BUDGET_SECTORS, a cancelled halt
reads zero sectors, and a second title with identical extents costs no further
reads while a different extent list still misses the cache.
Also worth recording: the CLI acceptance harness never exercised `info -v`
against an optical drive — it reads ISOs from local SSD, where a full-extent
read is fast enough to hide both defects.
|
||
|
Matthew Jackson
|
3bd2fd23b0 |
Fix audit findings: DTS AMODE bound, key-fetch negative memoization, PGS probe coverage
- dts: accept all 16 legal AMODE channel-arrangement codes (0-15), not just 0-9. Per ETSI TS 102 114 the 6-bit AMODE field has 16 defined arrangements; only 16-63 are reserved. a reference decoder the spec per-AMODE channel table confirms 10-15 are decodable 6/7/8-channel layouts. The old bound of 10 dropped spec-legal multichannel core frames as undecodable, silencing recoverable audio. Add a regression test (literal 0..16 range) that fails if the bound reverts to 10. - keysource: only memoize a NEGATIVE (empty) key-fetch result when every source genuinely ran and none held the key — never when a source Err'd (network down, unreachable). A transient outage was being cached as a permanent "no key" for the fingerprint, permanently dropping a unit that could be recovered once the source came back. Thread an `errored` flag out of the drivers and gate the cache insert on it. Tests cover both the recover-after-outage case and that a genuine absence is still memoized. - pgs_forced_probe: add happy-path coverage feeding real synthetic BD-TS PGS display sets through the full demux -> parse -> observe -> apply path, both a forced verdict landing and a non-forced verdict clearing a vendor flag. - mp4: correct fit_report doc (audio carried is AC-3/E-AC-3 AND DTS/DTS-HD). - scan_iso test: add independent fixture expectations (volume id) so the parity test is no longer purely tautological against a re-run of the same composition. |
||
|
Matthew Jackson
|
3841ae2250 |
disc: detect forced PGS subtitles from stream content for info
Give `info` the same forced-subtitle verdict the muxer derives during a rip, so the two agree. A shared classifier (mux::codec::pgs::ForcedTracker) folds a PGS track's display sets — forced iff every one carries the forced_on_flag — and is used by BOTH the MKV writer and a new scan-time probe that reads the title's PGS streams (reusing the TS demuxer and PGS parser). The probe only overrides a track it actually observed content for, so an undecrypted/unread stream keeps its vendor-derived flag. Gated behind ScanOptions::probe_forced_subtitles (off for the rip path, which detects forced while muxing without a second read). |