1055 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
Matthew Jackson	4f4b1ed222	fix(labels): make deluxe master-enum selection deterministic ... identify_master_enums picks, for each fingerprint, the best candidate class out of CandidatePool. Its tie-break only prefers an exact ldc count over an inexact one, so two candidates that are BOTH inexact but both within LDC_COUNT_TOLERANCE are decided purely by iteration order — and the pool was a HashMap. Rust seeds HashMap per instance, so this is not merely unstable across runs: the new test resolves the SAME jar to both Alpha and Beta within a single process, across 16 iterations. The same disc could emit different commentary/SDH/descriptive labels on consecutive rips of unchanged input, with nothing in the output saying the choice was arbitrary. BTreeMap fixes it by construction rather than by a sort someone can forget to keep. The pool is capped at MAX_CANDIDATE_CLASSES, so the ordering cost is irrelevant. The test runs the whole identification sixteen times and asserts one distinct winner. A single run cannot distinguish deterministic from lucky, and the seed does not change within a process — so repetition is what makes this a test rather than a hope. Found by the round-9 labels pass, which was dispatched specifically because every one of the ten lenses had reported leaving deluxe.rs unread. 1,159 new lines that nobody had opened.	2026-07-30 18:49:59 -07:00
Matthew Jackson	944e6a8b09	fix: align the Linux fsync error with macOS, and clear three stale docs ... Round 9 findings, triaged and verified against the pinned tree. writeback_file: a bounded-fsync WorkerLost returned bare ErrorKind::Other on Linux where macOS returns EIO. Round 8 fixed the Linux arm to return Err at all — the right fix — but stopped short of matching the value, so a consumer distinguishing timeout / halt / lost-worker had nothing to branch on for the third case on one platform. Now EIO on both. Three doc comments described the pre-fix behaviour, one of them for longer than the bug existed: linux.rs durable_sync still said "all three fallbacks return Ok(())" mod.rs sync_all still said Linux silently swallows fsync failures and callers must not treat Ok(()) as a durability barrier mod.rs SequentialSink::finish repeated the same caveat All three now say what the code does: a bounded-fsync failure is an Err on every platform, so Ok(()) IS a durability barrier. A doc that describes a fixed bug is worse than no doc — it tells a caller to write a workaround for something that no longer exists. au_assembly: discard_gap_before duplicated drop_marks_before's mark-retirement body verbatim and added one statement. Mine, from earlier today. It now calls it. Two copies of the same retirement loop is exactly how the two call sites would drift back together. clpi: ClpiStream's audio_format / audio_rate / video_format / video_rate are decoded from untrusted on-disc bytes on every parse and read by nothing. The identically-named fields consumed in disc/bluray.rs belong to mpls::StreamEntry, not to this struct — checked, because an earlier round wrongly called a live function dead. Deleted, along with the seven test assertions that pinned them; the tests that pin pid, coding_type and language remain. Also removed a section-header comment orphaned by the get_extents deletion, describing a fixture that no longer exists.	2026-07-30 18:39:47 -07:00
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 c610285. Kept mine — it covers the distinct-counters case and the Progress blanket impl, which theirs does not — but theirs had three clamp tests mine lacked: good_pct, bad_pct and pending_pct also clamp an overshoot, and I had only tested that for work_pct. Merged those in as one test and proved each of the three clamps load-bearing by removing them individually. An unused_parens warning in a new fixture. Method note, recorded because it cost real time: git apply --3way STAGES its result, so `git diff` reads empty and the tree looks untouched. I nearly concluded the patch had silently failed. Worse, the first attempt piped through `head -20`, so `echo exit=$?` reported head's status rather than git's — the same mistake this audit has already documented once. Check the real exit status, and check --cached, not just the working tree.	2026-07-30 16:36:13 -07:00
Matthew Jackson	8b8bcff106	test: pin five untrusted-input guards in the AACS 2.1 and CSS paths ... Second pass over src/aacs and src/css. No production change; the only non-test edits are two fixture bytes and one test rename. Five latent panics on untrusted data, every guard correct and none tested — so each was free to be deleted: variant.rs:224 a 0x04 record not a multiple of 5 indexes p_uv[0..4] off a one-byte tail variant.rs:269 a 0x0c record shorter than the 0x04 slot count slices past the cvalue table stevenson.rs:177 short sector read -> index 138 into a 129-byte slice stevenson.rs:208 a crib longer than the 1920-byte encrypted region -> index 2058 into 2048 stevenson.rs:272 a header periodic all the way to offset 0 -> subtract with overflow That last one is reachable from ORDINARY DVD data — constant or padding bytes are periodic. Verified on HEAD: widening the guard to <= 0x80 passes all 64 css tests unmutated. media_key_variant_from_kp had only a soft-correction test, so every step past that early return was unexecuted. The new two-slot fixture puts the covering slot at index 1, so the uvs[1 + 5*idx] and cvalues[idx*16] strides stop multiplying by zero. derive.rs:319 + -> - confirmed killable, as the first pass predicted: p == 0 makes (p-1)..32 underflow. Every prior fixture used a uv whose lowest set bit was 4, 10 or 11, so trailing_zeros() was never 0. One fixture bug caught and fixed rather than papered over: a |= mutant first SURVIVED because mk[14]'s 0x04 bit happened to be set, making OR and XOR agree. The byte is now clear and an assert_eq! pins it, so the fixture cannot drift back into agreeing with the mutation it exists to catch. walk_mkb_be24_high_byte_is_honored renamed to walk_mkb_be24_middle_byte_is_honored. Its 0x00_0110 length exercises the << 8 term only, which is why << 16 -> >> 16 survived it. The name was the lie; both framings are worth having, and the comment now points at the genuine high-byte test at 0x01_0004. derive.rs 146:32 and 154:30 stay untested, now with a proof rather than a judgement: bit_pos == -1 requires current_v_mask == 0xFFFF_FFFF, and calc_v_mask can never return that — its loop condition holds at !v_mask == 0, so it always shifts at least once. Both branches are reachable only after the walk has gone non-convergent and is heading for the bounded exit, where the return value is undefined. Termination is already pinned. Equivalents proven by observing green, including six more OR/XOR pairs on provably disjoint bit fields, and the two KEY_CORRECTION_DATA sites where the constant is the documented all-zero placeholder so x ^ 0 == x | 0. Those become killable only if a real per-licensee KCD is wired in. A partial confirmation sweep (138 of 415 mutants before the box saturated) found 135 caught, one timeout that is itself a detection, and exactly one survivor — the KEY_CORRECTION_DATA equivalent above.	2026-07-30 16:27:03 -07:00
Matthew Jackson	d5a9e70700	fix(udf): read the Metadata File Location from the partition map ... read_filesystem hardcoded the Metadata File's File Entry at block 0 of the physical partition — 'the metadata file ICB is at physical partition lba 0'. UDF 2.50 2.2.10 records where it actually lives, as a partition-relative Uint32 at offset 40 of the Metadata Partition Map. That field is the only thing on the volume that says where the entry is; block 0 is merely where authoring tools usually put it. On a conformant volume that recorded it elsewhere, block 0 holds something that is not a File Entry, metadata_start falls back to partition_start, the File Set Descriptor read there carries the wrong tag, and the volume is rejected as UdfNotFilesystem. Worse, a volume with a decoy file set at block 0 — as a rewritten or dual-structure volume can have — does not error at all: it mounts a different filesystem and reports success. Verified on HEAD: reverting the lookup reds four tests, e.g. the metadata partition beginning at 2000 where the map records 33754069. The recorded location is trusted only when the map's partition type identifier reads '*UDF Metadata Partition'. A Virtual (2.2.8) or Sparable (2.2.9) map is ALSO ECMA-167 3/10.7.3 Type 2 and records unrelated fields at offset 40, so its bytes must never be read as a location. Deleting that guard reds its own test. Block 0 stays in the candidate chain, so a volume whose map is absent or wrong but whose Metadata File does sit there keeps mounting exactly as before. This is additive, not a behaviour swap. Also 30 tests and ~55 more mutants across read_icb_extents, read_file_limited, read_inline_data, the prefetch stubs, parse_dstring and parse_udf_name. The metadata-partition branch — the branch EVERY real BD-ROM takes — had no test at all; nothing in the crate built a two-partition-map volume. Closes the max_bytes gap flagged earlier: 259 > -> == and > -> < now die on both the declared-size and the inline-ICB paths. Equivalents proven by application, notably two guards that read as protective but are unreachable: pm1_len is a single byte so 440 + pm1_len < 2048 always holds, and ad_offset + l_ad <= 2048 is enforced upstream so off + ad_size never exceeds the block. Bit 0 (Existence) stays unread, deliberately. ECMA-167 4/14.4.4 makes it a display hint, not a statement that the file is absent, and UDF 2.50 2.3.4.2 carries it through as the DOS hidden attribute. For a ripper the consequences are asymmetric: honouring it can silently drop a real .m2ts from the title list, ignoring it costs an extra name in a listing. Known structural limit, not fixed: read_filesystem takes only the FIRST extent of the Metadata File, so a fragmented metadata partition would map every sector past that extent to the wrong place. metadata_start being a single base LBA is what forbids the fix.	2026-07-30 16:09:35 -07:00
Matthew Jackson	0bc8d7af9c	test: constrain the AACS key-map gap fill, the PSI walk, and MP4 field offsets ... Third pass over src/mux/. 40 survivors killed, no production change. resolve.rs — the deleted-statement cluster is now fully constrained. All 14 deletable statements probed; 9 were already caught, 5 survived: c.sort_unstable() in fill_base_key_gaps. Every existing case handed it cuts already in LBA order, but IndividualSegment.tbl is a record list. Verified on HEAD: deleting the sort passes all 54 resolve tests. The mutant lays a base-key fill straight over a forensic segment. last_idx = idx (FMTS gap fill) and last_idx = hit (multi-CPS cache hit). An extent with nothing to sample must inherit its neighbour's CPS unit; the mutants fall back to the first unit's key. Exactly the shape this file's own comments name — wrong key, no error, lost_bytes == 0. Both check_halt()? polls in probe_fmts_index_keys. These cannot be killed by outcome, since a later poll returns Halted too. The tests count reads instead, which is what the don't-hammer-a-struggling- drive rule actually says: after a Stop the drive is asked for zero content sectors. The four unresolved += 1 arms each got a test, and deleting each fails exactly one — one-to-one, so no fixture passes for the wrong reason. A control test pins that the baseline table resolves, so an expect_err cannot succeed for an unrelated reason. ts.rs::scan_streams was never entered. Six killed, two of which return wrong answers that look right: reading the PAT/PMT CRC as a table entry invents a stream on PID 546 out of CRC bytes, and dropping the ES_info_length skip decodes a descriptor as an entry and loses the one after it. Every existing PMT fixture declares ES_info_length = 0; a real BD PMT carries a registration descriptor on essentially every entry. Also ISO/IEC 13818-1 2.4.4.3 program_number == 0 is the network PID, not a program. mp4/read.rs — 13. Height read as the width beside it; channelcount; the 4-byte base-128 descriptor varint (every existing esds fixture uses a single byte); all three optional ES_Descriptor fields, whose loss is silent (an AAC track just loses its CodecPrivate); first-vs-last media edit, which is A/V desync of the difference; and the version-1 mvhd timescale offset, emitted by any writer whose duration exceeds 32 bits. dts.rs — 7, from a real cargo-mutants run over the file rather than guesswork. Including a buffer that IS the syncword, which is the state a sync split across PES packets lands in the moment its last byte arrives. Equivalents proven by application, not argued: the sample_encrypted_units guard pair is mutually redundant by construction (total*p/9 < total for p <= 8), so either alone is equivalent and both together are not; the PMT section_len guard is dead code where its PAT twin panics; three of the seven EXSS_HEADER_MIN_BYTES arithmetic mutants still sum to 10.	2026-07-30 16:05:37 -07:00
Matthew Jackson	e99b634635	fix(mux): a backstop discard is a discontinuity; a stream-start trim is not ... drop_marks_before retired discontinuity marks alongside timing marks at both of its call sites. At stream start that is right. At the MAX_AU_BUFFER backstop it is not, and the two are now separate. The backstop fires when 8 MiB accumulate with no AU start code in them — corrupt or hostile input — and throws the run away. There IS a prior AU in that case, and whatever emits next definitively does not continue it: a decoder handed that picture resolves its references against frames separated from it by megabytes of discarded data. Retiring the flag meant the resync gate (resync.rs, driven from mux/disc.rs) never armed, so the broken picture went out looking sound. Silent corruption is the one class of loss this crate refuses to have. At stream start the opposite holds. Bytes ahead of the first access-unit delimiter are the tail of an AU that began before sync, and there is no prior AU to be discontinuous from. Marking it would arm the gate at the head of every title and drop its opening GOP. That risk is why this was a decision rather than a fix, and splitting the call sites is what avoids paying it. Recorded as a sticky flag, not an offset mark. A mark placed at the new base is retired moments later by the pre-sync trim that follows resync — the gap has to outlive the bytes that caused it. I found that by writing the test first and watching it fail with the mark approach. The discard is a discontinuity whether or not the source signalled one, and a signalled one on discarded bytes still reaches the AU that follows; both directions are tested. Note the first over-cap run is NOT a discard: the next AU's delimiter is still at buf[0], so it force-flushes as an over-long access unit and loses nothing. Only a run with no opener at all reaches the backstop. The tests push twice for that reason — the single-push version passes without the fix. Swapping either call site for the other fails: reverting the backstop reds the two gap tests, and arming the gate at stream start reds the third.	2026-07-30 14:52:39 -07:00
Matthew Jackson	f9d081ed45	test: drive the AACS 2.1 variant chain to a Media Key, and pin AES-G3 ... 163 of 322 surviving mutants across src/aacs and src/css. No production line changed — every function read correct; the finding was always an absent test. Two structural holes, both verified against HEAD before landing. variant.rs had no test that ever produced a Media Key. Every terminal assertion in the module was an Err classification — NotVariantMkb, SoftCorrectionRequired, OnlineChallengeRequired. So the entire 2.1 success path (VARIANTS lookup, VKD selection, Kpnew, the final unwrap, the verify gate) was pinned by nothing, and that path produces the Media Key that becomes the VUK that decrypts every byte of a 2.1 disc. Built the first complete planted variant MKB: the VARIANTS entry is chosen as Kvn ^ 1 so the real VKD sits behind a decoy at table index 1, making the lookup load-bearing rather than incidentally correct. That one fixture kills 23 operator mutants across three functions. aesg3 — the subset-difference tree node function — was in the survivor list as replaceable by [0; 16], meaning every device key in the crate would derive the same Processing Key. It is caught today only as a side effect of a negative test added after the mutation run; nothing asserted the relation itself. Pinned now via the spec relation ([C] 3.2.2) using the FORWARD primitive, with s0 transcribed independently rather than read back from AESG3_SEED, so the test cannot agree with a mutated constant. Same shape in derive.rs: plant_mkb was one slot with zero descent, so slot indexing was the identity permutation and the ancestor-descent branch never ran — which is why 39 of recover_dk_position's mutants survived. Added a 3-slot fixture keyed at index 2 and a four-level descent fixture whose expected Processing Key is written out as an explicit aesg3 chain rather than computed by calc_pk_from_dk; a fixture built by the function under test moves with its own mutations. Two latent panics on untrusted input now have tests: a 0x05 cvalue table shorter than the 0x04 slot index, and a drive declaring more payload than the 32772-byte response buffer holds. 23 equivalents claimed with reasoning, and confirmed empirically where possible — all eight css/lfsr mutants were run and exactly the seven disjoint-bit-lane ones survived. Explicitly NOT claimed equivalent: derive.rs 146:32 and 154:30 are reachable, but only on the non-convergent bounded-exit path where the function's sole contract is termination. A test there would pin defined-but-meaningless output. Noted for the next pass: the pre-existing walk_mkb_be24_high_byte_is_honored used total length 0x0110, whose high byte is zero — it exercised the middle byte only, which is why << 16 -> >> 16 survived it. Left in place; a real one was added at 0x01_0004.	2026-07-30 14:44:35 -07:00
Matthew Jackson	3e13a155fa	refactor(clpi): delete the unused EP-map to sector-extent path ... get_extents had no caller anywhere in the ecosystem, and neither did anything feeding it. Removed: get_extents, resolved_ep_map, full_pts, full_spn, parse_cpi, EpCoarse, EpFine, the ep_coarse/ep_fine fields, the unused version field, and the 35 tests that exercised them. This reverses the fix in b2e1982, deliberately. That commit corrected a real truncation — out_time past the last EP entry resolved to last_ep_spn + 1, dropping everything from the final I-frame to EOF — and the fix stands in history as the record of what was wrong. But the defect existed for eight audit rounds precisely because the code had no caller: nothing exercised it, so nothing noticed. Keeping speculative infrastructure alive on the strength of a comment saying it is reserved for a path that does not exist is how that happens again. ClipInfo keeps source_packet_count (read by disc/bluray.rs) and streams (the CLPI/MPLS cross-validation in labels/clpi_audit.rs). 1,139 lines out, gate green in debug and release.	2026-07-30 14:39:22 -07:00
Matthew Jackson	b2e1982051	fix(clpi): resolve out_time past the last EP entry to the end of the clip ... ClipInfo::get_extents fell back to `last EP SPN + 1` whenever out_time lay past the last entry-point. EP entries mark I-frames (BD-ROM Part 3, CPI / EP map) and a clip's final GOP lies after the last one, so a PlayItem covering a whole clip — whose OUT_time is the presentation end — always lands in that arm. The extent then stopped one source packet after the last I-frame. Measured on a fixture with 200,000 source packets and the last EP at SPN 131,072: sector_count came back 12,289 where covering the clip needs 18,750. Everything from the last entry point to EOF is outside the returned extent. Scope, stated plainly: get_extents has NO callers anywhere in the ecosystem today — it is #[allow(dead_code)] and documented as reserved for the timestamp-range read path. Nothing ships this loss. It is fixed now because a latent truncation in extent arithmetic is far cheaper to correct before it has callers than after. The SPN at-or-after an out-of-range out_time is the end of the clip, source_packet_count, with .max(last + 1) so a disc that under-declares its own packet count against its own EP map still yields a sane bound. Also 174 mutants killed across clpi, mpls, ifo and ebml — the first time any of these four files has been examined. And ebml's 8-byte VINT back-patch was duplicated verbatim in end_master and end_master_buf with its top four payload octets unreachable through either (they need a 16 MiB..256 TiB buffer); extracted to fixed_width_vint8 and tested across the full 56-bit payload, no behaviour change. 38 of ebml's 46 survivors are one equivalence cluster: every | in write_size / read_id / read_size / read_uint_val ORs into disjoint bit lanes, where ^ is the identical operation. Applied all 38 at once — green — then spot-checked four individually.	2026-07-30 14:24:57 -07:00
Matthew Jackson	84a77f6a0e	test: pin FMTS read_plan unit indexing to an unaligned range start ... Five surviving mutants in AacsKeyMap::read_plan, all in the arithmetic that decides which half of a forensic segment this disc's key opens. The existing coverage used a forensic range starting exactly on the extent's first unit. Under that shape several wrong formulas agree with the right one by arithmetic accident: (lba + range_start) / us and (lba - range_start) * us both produce the correct kept set. Real ranges are not shaped like that. A range start comes from a source packet number — start_spn * 192 through clip_byte_to_lba in mux/resolve.rs — and 192-byte packets bear no relation to the 3-sector aligned unit, so range_start % 3 is whatever the disc says. Getting the parity wrong does not crash. It reads and decrypts the ALTERNATE variant's half: the units this key does not open decrypt to garbage, the units it does open are skipped. AACS 2.1 forensic marking is precisely what makes the two halves differ, so the failure is silent — a full-length rip carrying the wrong variant. Two fixtures are needed because no single one kills both: an unaligned range start with the extent beginning on it inverts the halves under the + form, and an extent offset one unit-remainder from the range start inverts them under the * form. Also pinned the short-tail guard. is for a remnant SMALLER than a unit — bytes with no following unit to desync. Widened to <=, the last whole unit of every extent bypasses the phase gate, so a forensic segment ending at an extent boundary contributes one alternate-variant unit to the rip.	2026-07-30 14:18:39 -07:00
Matthew Jackson	9de88969ca	test: constrain the DiscStream loss surface and the empty-title guards ... Second mutation pass over src/mux/. 26 survivors killed, no production change. Verified on HEAD before landing: each mutation below passes all 1,237 mux tests unmutated-suite. The priority item was the honest-loss-reporting surface. Both DiscStream::errors and DiscStream::lost_bytes could return a constant with nothing failing — a rip that lost sectors would report zero loss to the caller. This project has already shipped one defect of that shape (a total decryption failure reported as an empty title, exit 0). Driven now through two short-read fills so both land on values that are neither 0 nor 1 and differ from each other; no constant and no field swap survives. MkvStream::finish -> Ok(()) also survived. MkvMuxer::finish has the zero-frame MkvInvalid guard and two tests cover it, but the Stream wrapper above it could return Ok unconditionally and bypass the guard entirely — the empty-title defence was one layer thinner than it looked. au_assembly: pinned au_opener_from behaviourally to the normative byte values for all four modes, with negative cases for codes that are explicitly not openers (MPEG-2 slice 0x01..0xAF, user data 0xB2, extension 0xB5, sequence end 0xB7 per 13818-2 Table 6-1; VC-1 0x0A/0x0B/0x0C; H.264 SPS/PPS/IDR-slice). au_assembly and codec/ hold independent copies of these constants; they agree today, and comparing constants would not catch logic drifting apart, so both sides are now pinned to the spec instead of to each other. demux_sink::sanitize: every filename component demux:// writes comes from disc-controlled text, so the path-separator arm is a traversal guard. Deleting it now fails, including an end-to-end case where base = "../evil/Title" must produce exactly one file inside the chosen directory. stts_and_ctts_expand renamed to stts_expands_runs_to_per_sample_deltas_in_order and given runs with distinct deltas AND distinct lengths. Its old name claimed ctts coverage it never had, which is why the composition-time chain went unconstrained for eight rounds; the doc comment now points at the tests that do cover ctts. Correction to the previous pass: codec/truehd.rs flush -> vec![] IS equivalent. Applied it, full mux suite green. TrueHD buffers across PES but parse emits every complete unit immediately, so a residual buffer at EOF is a truncated access unit and is correctly discarded. The vec![Default::default()] variants are genuinely different and are killed. Deliberately not constrained: mkv::set_opening_capture (diagnostics behind a process-global tracing check, flaky under the parallel runner), and the three stdio.rs header paths (StdioStream holds concrete io::Stdin/Stdout and cannot be driven without a production refactor to injectable Read/Write).	2026-07-30 14:13:33 -07:00
Matthew Jackson	170fd0c064	test: constrain MP4 composition timing, MLP substream directory, and codec-private absence ... Mutation testing over src/mux/. No production change — 49 survivors killed, all proven red before green. The MP4 composition-time chain was entirely unconstrained: VideoTiming::ctts, build_ctts and parse_ctts could each return a constant and the suite stayed green. Confirmed on HEAD: build_ctts -> vec![] passes all 1,220 mux tests. A demuxed B-frame title presenting in decode order would have shipped. The cause is a test whose name asserts coverage its body does not deliver — stts_and_ctts_expand builds an stts box and never touches ctts, and write_then_read_round_trip asserts sample sizes and keyframe flags but not one PTS. Same shape as the set_speed forwarding finding, different disguise. mlp_num_substreams / mlp_substr_header_size: every TrueHD fixture in the crate uses one substream and no extraword, so both could return a constant and agree with all of them. These position mlp_parity_ok's window over the AU header, so a constant mis-windows the parity check on exactly the multi-substream AUs that carry 7.1 and Atmos. CodecPrivate absent vs empty: mkv.rs writes Some(bytes) verbatim and omits the element on None (RFC 9559 5.1.4.1.24), so a zero-length Some emits a track header asserting the config IS empty. Four parsers could return Some(vec![]) before any frame. Also: mandatory ISO/IEC 14496-12 boxes (tkhd, vmhd, smhd, dinf, mdhd) could each build empty; HEVC num_extra_slice_header_bits (H.265 7.3.2.3) was never non-zero in any fixture, so the slice-type offset skip was unexercised; chapter names from the disc go straight into <ChapterString> and the & escape must run first; a stray 0x47 in a payload must not latch a TS resync. Documented as equivalent rather than killed: CodecParser::flush and the three parser flush bodies that differ from the mutant only by a tracing call, and DropTally::log_summary.	2026-07-30 13:39:02 -07:00
Matthew Jackson	55b97ac576	fix(udf): skip deleted File Identifier Descriptors ... read_directory decoded file-characteristics bit 1 (Directory) and bit 3 (Parent) but never bit 2 (Deleted) — ECMA-167 4/14.4.4. It followed the ICB of a descriptor naming a file that no longer exists. 4/14.4.3 permits a deleted FID's ICB field to specify an extent of length zero, so it need not point at a File Entry at all. Following it reads whatever descriptor occupies that metadata LBA: - deleted DIRECTORY FID: recursion lands on the File Set Descriptor (tag 256), hits the not-a-File-Entry arm and returns Err. One stale descriptor in one directory fails enumeration of the entire volume. - deleted FILE FID: read_file_size returns Ok(0) for a non-File-Entry tag, so a deleted name is reported as a real zero-byte file. Both directions of the same shape at once: a recoverable condition becoming a hard failure, and a non-existent entry becoming a plausible success. Bit 0 (Existence) is still unread. Skipping hidden files could hide real content, so it is left alone deliberately rather than folded in. Found by mutation testing: 48 of the 51 surviving mutants in read_directory are killed by the 17 tests added here, covering the short_ad decode byte by byte (ECMA-167 4/14.14.1), the extent-type mask, the AD bounds guard at the exact sector end, the tag-261 File Entry directory arm that no test reached at all, the multi-sector read offset, the FID stride including L_IU, and the nesting cap. Three survivors are genuine equivalents and are documented as such: l_fi > 0 vs >= 0 (the emptiness guard below reaches the same state), and the << 32 / | in the visited-set key (meta_start is constant across a walk, and the two halves are disjoint).	2026-07-30 13:32:06 -07:00
Matthew Jackson	c610285910	test: constrain SectorSource speed forwarding and PassProgress percentages ... Mutation testing left both unconstrained. sector/mod.rs — set_speed on the Box<dyn> and &mut dyn forwarding impls could be replaced with an empty body and nothing failed. This one hides better than the read methods because the trait's own default body is already a no-op, so a forwarder that swallowed the call is indistinguishable from a source with no speed control. Consequence is a silently absent value, not a wrong one: the recovery path lowers read speed through a damaged region, and a swallowed call leaves the drive at full speed while the caller believes it slowed down. Routed through a generic S: SectorSource bound, since a direct call on a &mut dyn receiver auto-derefs to the vtable and never enters the forwarding body. progress.rs — 42 survivors. All four percentage accessors could return a constant, read the wrong byte counter, or have their divide-by-zero guard inverted. Added exact-value tests (25%, not 'some percentage'), both sides of each guard, the overshoot clamp, and one test setting all three disc counters to distinct values at once — without it, a swapped field still passes every single-counter test. The Progress blanket impl for closures could return a constant true. That return value is the cancellation signal, so a constant-true body makes every closure-based consumer uncancellable. Each mutation applied, observed red, reverted.	2026-07-30 13:26:31 -07:00
Matthew Jackson	e4b1e5b19e	docs: correct info invocation and read timeouts ... TROUBLESHOOTING step 2 said `freemkv info`, which needs a source URL; the drive route is `freemkv info disc://`. architecture.md quoted 1.5 s / 30 s for the read timeouts. The constants are READ_TIMEOUT_MS = 10_000 and READ_RECOVERY_TIMEOUT_MS = 60_000 (src/scsi/mod.rs:72,94).	2026-07-30 13:21:04 -07:00
Matthew Jackson	8d4a6d54a4	Constrain five behaviours that mutation testing showed nothing constrained ... Fifteen surviving mutants killed, from the highest-risk class: functions a mutant could replace wholesale with a constant while all 2,555 tests passed. None of the code was wrong. In every case a test was absent, which is why eight rounds of reading never found any of them. The one that generalises is in sector/mod.rs. Its existing test READS as covering `read_sectors` on the `&mut dyn SectorSource` forwarding impl — it takes a `&mut dyn`, calls the method, checks the spy. But the receiver auto-derefs and dispatches through the vtable straight to the spy, so the forwarding body is never entered. An earlier round hit this exact trap on `set_unit_base` and fixed it with a generic helper; the read path kept the test that looked right. Verified by stubbing the forwarding impl to Ok(0): the new test fails, the old one passes. That makes a tenth distinct shape of bad test in this audit, and the mutation list is how to find the rest — any forwarding-impl method in it has the same problem. decrypt.rs's two existing gate tests assert only `dropped == 0`, which is precisely what the `Ok(0)` mutant returns; one asserts nothing else at all. A wrapper that decrypts nothing therefore looked correct while the caller muxed scrambled MPEG. Now pinned by descrambling a real CSS sector and comparing against the plaintext it was built from — not against a re-derived descramble, which would only assert the code agrees with itself. css/mod.rs's `is_scrambled_uncracked` turns out to have no production callers at all; the enum is matched directly. Its three tests all assert only the true direction, which is exactly why the `-> true` mutant survived. It is public API, so a consumer routing on it would, under that mutant, refuse to rip every clear DVD. aacs/inf.rs's MKB drive read had no test whatsoever. Now pinned byte-for-byte across multi-pack concatenation, the single-pack case, a genuinely empty response, and error propagation — an unreadable MKB must surface as an error, not as an empty one. aacs/derive.rs's nine mutants are killed with planted MKBs built by inverting the AACS relations, so no real key material is involved. The assertions land on the derived Media Key rather than the intermediate positions: a recovered position that does not actually walk to the planted key is no better than None. A fixture-guard test asserts the planted MKB parses, since an unparseable one would make every `-> None` body look right. 2570 lib tests, debug and release.	2026-07-30 12:41:28 -07:00
Matthew Jackson	93e1436fc0	Test that the Media Key verifier actually rejects a wrong key ... `km_verifies` is the gate deciding whether a candidate Media Key belongs to the disc. The MK-pool brute force in resolve.rs runs every candidate through it, so a version that said yes to everything would accept whichever candidate it tried first and the rip would continue with a wrong Media Key — wrong VUK, wrong title keys, garbage plaintext, and no error raised anywhere. Nothing tested it. Whole-crate mutation testing reported `replace km_verifies -> bool with true` as SURVIVING: the body could be replaced with `true` and all 2,556 tests still passed. A verification routine whose verification was itself unverified, in the most safety-critical function in the crate. The implementation is correct — it matches the AACS relation `AES-D(km, mk_dv)[0..8] == 01 23 45 67 89 AB CD EF`. Only the defence was missing. No real key material is required to test it. That relation means a valid record for any chosen km is just `AES-E(km, <the constant> || anything)`, so the fixture is self-contained. The test asserts three things: the key the record was built for verifies; a key differing by ONE BIT does not, which is the assertion that kills the mutant and is a near-miss rather than a random key; and an MKB carrying no verify record does not default to yes, because unverifiable and verified are different answers. Confirmed by reintroducing the exact reported mutation and watching this test fail. This is the first defect found by mutation testing rather than by reading. Eight audit rounds and a security lens that read this file in full all missed it, because it is not a wrong line — it is an absent test, and only an instrument that asks "would anything notice if this were broken?" can see that.	2026-07-30 12:14:14 -07:00
Matthew Jackson	18f8b285c4	Bound the BD-J label parsers, and stop a crafted disc hanging the scan ... Ten defects in code no previous round had ever scoped. `src/labels/` identifies a disc's studio by parsing jar archives and JVM class files off untrusted media, so every byte here is attacker-controllable — and 813 of its lines were executed by no test at all. The worst is a non-terminating loop. A fallback stream-number scan advanced with `saturating_add`, and the comment says why: a crafted XML "must not overflow (panic in debug, wrap-to-0 in release)". Once the counter pins at u16::MAX and that number is taken, the loop cannot exit. So a fix for an overflow panic produced an unbounded hang, which is strictly worse — a panic is observable and catchable, and catch_unwind cannot interrupt a live loop. Reachable from about 8 MB of XML. Where the same overflow appears in the deluxe decoder the fix is checked_add and stop, NOT saturation — twice wrong there, because saturating would peg every stream past the ceiling at one number and apply_labels binds on (type, number), silently mislabelling tracks. A correctness bug wearing the costume of success. Round 7 capped the ldc-string retention per class; nothing capped the aggregate, so a 64 MiB jar held that budget for every class at once. Same defect one level up, which is the shape that keeps recurring in this directory. Four other amplifications are bounded the same way, each with a stated headroom and a paired test proving real media passes untouched — the tightest is 5x on a label length, the loosest 2000x on the stream numbering space, against BD's 32-per-type STN_table limit. Two are not caps at all: a quadratic membership scan became a set, and an attacker-derived length added to a cursor without saturation now cannot wrap. Nothing is excluded by either. A `#[cfg(test)]` hand-copy of a shipping parser was the ninth bad test this audit has found, and the first proven by mutation rather than inspection: deleting the guard from the REAL function left all 26 tests green, including the one named for that guard. Pointed at the real function, the same mutation fails. Separately, all three failure arms of the bounded fsync returned Ok(()) on both macOS and Linux, so sync_all reported success for a durability barrier that never ran. Only macOS was in scope; the Linux twin is fixed here too, because a platform disagreeing with its sibling about whether a failed sync is an error is the class that already produced an over-length SCSI CDB macOS rejected and the other two truncated. Note the behaviour change: a mux whose final sync times out on a wedged mount now fails rather than exiting 0. Three of the caps are proven by wall-clock deadline rather than an operation count, with 18-80x margin on the passing side. On a heavily oversubscribed machine those could flake.	2026-07-30 11:30:24 -07:00
Matthew Jackson	c63dafcf1a	Key each FMTS extent from its own CPS unit, and stop a bad ICB tag reading ... as an empty directory On an AACS 2.1 disc the non-forensic gap fill hardcoded pool slot 0 as "the" base Unit Key, so every content LBA outside a forensic segment was keyed with CPS unit 1's key even on a disc carrying several CPS units. It does not fail loudly — it produces garbage plaintext. The gap fill now resolves each extent's own base key from its ciphertext, sharing the sampling and slot-picking the multi-CPS path already had rather than adding a second copy, and memoised in the existing per-disc cache. A disc with one base key still short-circuits with zero extra reads, which the existing probe-cost test pins. Forcing the slot back to a constant fails four tests, so the choice is load-bearing rather than incidental. A directory ICB whose descriptor tag is neither File Entry nor Extended File Entry (ECMA-167 4/14.9, 4/14.17) was turned into a successfully-read EMPTY directory, indistinguishable from a genuinely empty one, while the same tag on a file ICB was already a hard error. Fifth instance in this audit of a failure converted into a plausible success value, and the second in this very function — round 5 fixed a read error becoming a file size of zero here. An unrecorded extent (ECMA-167 4/14.14.1.1: allocated but not recorded, logically zeros that still occupy file space) was dropped entirely rather than contributing its length, so every later extent landed at the wrong file offset. Silent corruption, not an error. Extents now carry a recorded flag and the hole emits zeros without touching the media. The Volume Descriptor Sequence was swept at hardcoded sectors 32..64 while the anchor's own Main VDS Extent pointer was parsed into a comment and ignored; ECMA-167 3/10.2.1 defines that extent by the field, not by position, so a conformant volume placing it elsewhere failed to mount. drive_status decoded byte 5 as Media Status without checking the event header's NEA bit or notification class (MMC-6 §6.7), so a reply carrying no media event descriptor decoded as "no disc". The drive is untrusted input here, and this is the works-on-my-drive class. Two more tests were found asserting the defects they sit next to — one requiring unrecorded extents to be dropped, one that four drive fixtures built non-conformant replies the corrected decoder rightly rejects. Both rewritten. Combined with the DTS one in the previous commit that makes three tests this round that locked a bug in as intended behaviour, which is a different and worse failure than the tautological tests found so far: a tautology fails to catch a regression, these actively defend the defect. Not fixed, adjacent: extract_one_file streams extents sequentially and will now READ an unrecorded extent's sectors rather than writing guaranteed zeros. Offsets are right, and pressed media reads as zeros there, but it is not zero-guaranteed the way read_file now is; that needs a recorded flag through PlannedFile.	2026-07-30 11:15:52 -07:00
Matthew Jackson	46eb88c51f	Feed the CSS crack the canonical extent order, and stop Resolution faking 1080p ... Seven defects in the code the test suite executes least — 913 lines of disc/mod.rs alone are run by no test at all, which is why this round scoped from coverage rather than from what previous rounds said they had read. Disc::scan_image kept its own copy of the crack's extent ordering and fed crack_key_outcome largest-cell-first. That is the fifth instance in this audit of a local reimplementation drifting from the canonical one, and the cost here is a key that does not descramble the feature: picking by sector count bypasses the capacity gate and can select a different VTS entirely. The copy is gone — which title comes from the canonical order the scan already applied, and the extents are handed over in playback order, exactly as decrypt_keys_for_title does. Its doc records why the duplicate existed so it cannot grow back. Resolution::pixels returned 1920x1080 for Unknown. That is the FOURTH instance of one trap and the other three were in this same file, two of them fixed hours earlier — without sweeping for siblings, which is the whole reason this one survived. It now returns (0, 0), and the sweep was done properly this time: every remaining Unknown arm across the crate is honest, and the two ColorSpace sites that look like fabrication are emitting H.273 code point 2, which is the spec's own "unspecified". Two callers carried local Unknown-to-zero workarounds — precisely the cost of making callers responsible for a lie — and one is now redundant. BD-ROM Part 3 code 0xA2 is the lossy secondary DTS stream, not lossless Master Audio. A test asserted the wrong mapping as intended behaviour, so correcting the code failed it; the test is deleted with a note pointing at its replacement. That is a NEW failure mode for this audit: not a test that cannot fail, but one that locks the defect in. There is no DtsExpress variant to map to, so it takes the lossy DTS-HD member and the approximation is documented. Also: DiscSession::identify could panic through drive_mut once the public API allows an absent drive — two siblings were converted in an earlier round and this one was missed; an extent end that added without saturating where the rest of the crate saturates; a diag reason string restating the comparator's sort keys and drifting from them, now derived from them; and a short read that advanced the offset by the full request, silently skipping the gap. That last one existed twice, in two reads with the same shape, now merged so they cannot drift apart. The short-read policy is a judgement call I could not derive from a spec: no skip_errors is a hard error, with skip_errors zero-fills and charges the loss. It deliberately does not retry mid-unit, because resuming inside an AACS aligned unit would trade a silent gap for a silent decrypt desync — the worse of the two.	2026-07-30 11:12:55 -07:00
Matthew Jackson	dea968f32b	Stop AudioChannels and SampleRate fabricating a value for Unknown ... Three copies of the same two mappings existed. The canonical accessors returned 6 channels and 48000 Hz for Unknown; a third copy in diag.rs returned 0. The honest one was the copy. A plausible wrong answer is worse than an obvious one. Six channels at 48 kHz is indistinguishable from a real 5.1 track, so every caller became responsible for remembering to check the variant first — and this crate walked into exactly that: the json:// sink reported a confident 5.1 for audio whose neighbouring fields said "unknown". That was fixed at the call site earlier in this audit; this fixes it at the source. The accessors now return 0, which is what both in-crate call sites already coerced Unknown to by hand, so their guards are gone and the behaviour is unchanged. Zero is also obviously wrong if it ever reaches output, where six is not. The diag.rs duplicates are deleted rather than corrected — a fourth copy would have drifted too. Their only caller was a trace line in the same file, now on the canonical accessors. Their tests moved across and gained the Unknown case, which is the point: restoring either fabricated value fails both. Found by the round-7 correctness agent while fixing the json:// sink; it flagged the third copy as out of its scope rather than touching it.	2026-07-30 10:09:01 -07:00
Matthew Jackson	079c9b1327	Add a seeded robustness harness for the untrusted-input parsers ... Five parsers that take bytes straight off a disc are now swept with generated input asserting one property: they return Ok or Err and never panic. That is this crate's own hard rule, and the class seven rounds of reading is worst at. Written in-crate rather than with cargo-fuzz, which needs a nightly toolchain this project does not use, and without proptest or arbitrary, because one dev-dependency is a deliberate posture and the parsers take plain byte slices. What is given up is coverage-guided mutation, which is the real loss. What is gained is determinism: the same seed replays the same cases anywhere, so a CI failure reproduces locally verbatim. Three generators, and the second is the one that matters. Pure random bytes die at the magic check and exercise the entry guards only; prefixing valid magic is what reaches the parser body; mutating a mostly-zero record is what reaches the offset and count arithmetic a hostile image would lie about. That claim is MEASURED, not asserted. A harness whose cases all bounce off the entry guards is the fuzzing equivalent of a test that cannot fail, so one test counts how many generated cases parse to completion: 15,606 of 60,000, about 26%. If a future change to a guard drops that to zero, the test fails rather than continuing to report a meaningless pass. Two further tests pin that the three generators produce different bytes and that a seed replays identically. 1.2M cases across all five targets found nothing. On this evidence that is a real negative rather than an empty one. The first version of this file was itself broken in the way this audit keeps finding: its two meta-tests set FREEMKV_HARNESS_CASES and raced, because the test harness runs them in parallel and env mutation is unsound there. The budget is a parameter now, and the environment is read once at the call site. Two crate-internal parsers widened from private to pub(crate) so the harness can reach them. No public API change.	2026-07-30 09:45:56 -07:00
Matthew Jackson	327087c70e	Make five tests capable of failing, and stop the presence probe unmounting the disc ... The worst of the five was a regression suite that never touched the code it guarded: nine batch-count tests called `safe_batch_count` and `buggy_batch_count`, both defined in the test file itself. The u16 truncation they exist to prevent could be reintroduced in sector/prefetched.rs with every one of them green. They now drive the real producer through the public API, and reinstating the truncation fails five of the nine. Worth recording that the symptom has changed since the original fix: the unit-alignment clamp below floors a zero batch at three sectors, so the bug is now a twenty-fold throughput cliff rather than the stall it once was. The MP4 reserve test's only numeric case was dominated by the floor and the buffer, so BYTES_PER_SAMPLE could be zeroed without failing it. It now has a case where the per-sample term dominates. The zero-count guard in FileSectorSource was likewise unfalsifiable — seek-past-EOF and a zero-length read both succeed — so the test now observes the file cursor. The AACS media-key ambiguity guard had no test at all; the pool scan is extracted so the verifier can be injected, because a genuine two-key collision needs one ciphertext decrypting under two AES-128 keys to plaintexts sharing a 64-bit magic, which is a 2^64 search and not a fixture. macOS implemented the documented cheap, side-effect-free presence probe by building a full exclusive transport — which force-unmounts the disc. Linux and Windows issue one TEST UNIT READY with no unmount; macOS was the outlier. It now walks the IOKit registry for the media object instead. The C shim's registry reads assumed CoreFoundation types the registry does not guarantee, so a driver publishing a CFNumber where a CFString was expected aborted the process from inside public API. Types are checked and a wrong type treated as absent. The unbounded waitpid on the unmount child is now a polled deadline, and the last-resort match gained the NULL check its two siblings already had. The empty-CDB guard existed only on Linux while a shared helper's comment claimed all three backends had it. Moved into the helper, so the comment is now true and macOS and Windows are covered. One finding was REJECTED with evidence rather than fixed. The TrueHD buffer-cap test was indeed bogus, but MAX_TRUEHD_BUF turns out to be unreachable by any input: the parser only retains data when the buffer is shorter than the declared AU, and that declaration is twelve bits, so the worst case is 8189 bytes against a 256 KiB cap. An exhaustive sweep over all 65536 AU headers confirmed it. The fixture now sits at the reachable ceiling and asserts that instead. The cap itself is left in place as defence, unreachable by construction, matching how the AC-3 resync guard was handled earlier in this audit. Two behaviour changes worth naming: Linux's empty-CDB error becomes InvalidCdbLength rather than a transport failure, and an unknown device now reports absent media rather than a not-found error, because the registry cannot tell an empty drive from a missing one. The latter is a conflation of the kind this audit has fixed three times; it is recorded for the next round rather than left silent.	2026-07-30 09:26:56 -07:00
Matthew Jackson	b8fa5e74dc	Stop the live rip path muxing Blu-ray 3D differently from the ISO path ... Five defects, four of them the same shape: a local reimplementation of logic the crate already had, which had drifted from it. Each is now fixed by calling the canonical version rather than by patching the copy. DiscStream::new — the live disc:// path — built every parser through the plain codec lookup and never asked whether a video stream was an MVC dependent view, though resolve::build_demux_state does. The same 3D disc therefore muxed correctly from an ISO and incorrectly ripped live. The open-coded loop is gone; both paths now call build_demux_state. collect_psi_section reimplemented the continuity-counter gap test and disagreed with process_packet in the same file: it tolerated neither a duplicate packet nor an adaptation-field-only packet, which per ISO/IEC 13818-1 §2.4.3.3 does not increment the counter. A spec-legal PMT continuation was read as desync and the title's stream list came back empty. Both callers now share one `cc_is_gap`, and a duplicate packet's payload is no longer appended twice — doing so would have corrupted the section the check exists to protect. The json:// sink called the channel-count and sample-rate accessors unconditionally, and both fabricate a concrete value for Unknown, so it reported a confident 5.1 at 48 kHz for audio whose format was unknown while its own neighbouring string fields said "unknown". The keys are now omitted, matching mkv.rs. This matters more than it did: a sample-rate ladder fixed earlier in this audit means Unknown now reaches consumers that used to receive a wrong-but-concrete value. For an audio:// or sub:// sink the reference video track's output is filtered out, so its first PTS was never recorded and every delay was computed against zero — baking a wrong DELAY into the filename. The reference is now recorded whenever a frame is on the reference track, independent of whether that track has an output, so a normal title gets a correct delay; where no reference is ever observed the tag is omitted rather than guessed. A third copy of the channel/sample-rate mapping exists in src/diag.rs and was left alone as outside the confirmed set. It is the same drift shape and is recorded for the next round.	2026-07-30 09:18:33 -07:00
Matthew Jackson	3f7d7af472	Bound three allocations an untrusted disc can drive without limit ... The Program Stream demuxer appended every fed byte and enforced its 4 MiB cap only inside a branch reached once a start code had been found. Input containing no start code anywhere therefore hit no cap at all, and since a whole title is fed through this demuxer, a zero-filled or ciphertext VOB extent buffered the entire title — up to ~90 GB. When the buffer holds no start code, only a two-byte `00 00` prefix can begin a PS unit on the next feed, so that is kept and the rest dropped. The bound is exact rather than a heuristic: a start code can straddle a feed boundary by at most its first two bytes, so no real byte is discarded, and a test feeding `FF FF 00 00` then `01 E0 ...` pins that. The existing test named for this case fed a real start code first, so the cap it exercised was the in-PES one. Renamed to say what it covers. The BD-J label path had a different shape to anything found so far: the cap is on the COMPRESSED size of a disc file while the allocation scales with the decompressed size. A `.class` gated only by a path prefix inflates to the 64 MiB ceiling, yielding ~33M retained strings from `ldc` operands or ~67M pushes onto a symbolic stack whose depth was unbounded despite the Code attribute's own `max_stack` being parsed and then ignored. Bounded both, the stack by `max_stack` itself (JVMS §4.7.3). The VMG TT_SRPT title count is an untrusted u16 with no de-duplication, so ~800 KB of crafted IFO re-parsed one PGC 65535 times. Capped at 99, the DVD-Video maximum, so no conformant disc is clipped. Every cap carries stated headroom against real media, and each has a test locking that real media still passes. I rewrote three of the new assertions before landing them. They compared the result against the very constant under test — `total <= MAX_TT_SRPT_TITLES` — which passes vacuously the moment someone raises the constant, the most likely future regression and the seventh instance of this tautology shape in this audit. They now assert literals derived from the spec. The TT_SRPT fixture also had to change: with 65535 identical entries the de-duplication collapsed them on its own and the cap was never what bounded the result, so the test passed with the cap removed entirely. Distinct entries defeat dedup and leave the cap as the only guard; de-duplication now has its own fixture. Verified by raising each of the three constants and confirming all three tests fail.	2026-07-30 09:17:16 -07:00
Matthew Jackson	fdd473d7e9	Remove emulation-prevention bytes before reading the H.264 slice header ... The bytes after a NAL header are EBSP, not RBSP: ISO/IEC 14496-10 §7.4.1 has the encoder insert 0x03 after any 0x00 0x00, and §7.3.1 removes it before parsing. The measured-picture-type parse read the raw NAL instead, on the stated reasoning that slice_type is too early for an escape to intervene. That holds only up to a point. first_mb_in_slice is ue(v), so a value of 65535 or more needs sixteen leading zero bits and opens the payload with 0x00 0x00, which an encoder must then escape. A UHD frame is ~32,400 macroblocks, so a conforming Blu-ray never reaches it — but 8K does, and the disc is untrusted input. Such a stream decoded slice_type against a byte the encoder had inserted and reported the wrong picture type: a wrong result rather than an error, which is the class this lens exists for. The prefix is un-escaped into a 16-octet buffer rather than the whole NAL: the two ue(v) fields are at most 32 bits each, so nothing longer can be needed, and it keeps a per-frame allocation proportional to the frame off the path. The test pins both directions. It asserts the un-escaped prefix decodes to first_mb_in_slice = 65535 and slice_type = 2, AND that the raw EBSP does NOT — without that second assertion the test would pass whether or not the fix were present, which is the failure mode this audit has now found four times. The bit string was derived independently rather than by hand: my first attempt at the fixture was wrong by one nibble and the test caught it. Also covers the cases that must NOT be unescaped: a 0x03 not preceded by 00 00 is ordinary payload, and 00 00 03 03 keeps its second 0x03 because the escape resets the zero run.	2026-07-30 08:49:51 -07:00
Matthew Jackson	05fed1b0e0	Log the OS error when a SCSI command fails on Windows and macOS ... Both backends discarded the platform's own error code on the execute hot path — the one every READ(10) of a rip goes through — and collapsed every cause to the same status-0xFF transport failure. On Windows, open() and reset() in the same file both capture the Win32 error; execute() did not. That left ERROR_INVALID_PARAMETER (a struct layout regression, the exact class this file's SDK-layout tests exist to catch), ERROR_ACCESS_DENIED and ERROR_GEN_FAILURE (a genuinely wedged drive) indistinguishable, with nothing in the log to tell a code bug from a hardware one. On macOS the same, and worse: the file had no tracing calls at all, where the Linux and Windows backends both log their execute failures. Its open() carefully decodes the shim's sentinel into typed variants instead of flattening them, but execute() threw the IOKit return away — so another process taking exclusive access mid-rip and a real hardware wedge produced identical, empty diagnostics. Logged rather than added to the error type: the typed variant is public API, and the recovery classification is deliberately the same for all of these. What was missing is the breadcrumb, not the distinction. Two further findings from the same sweep were rejected. Windows reset() always returning Ok(()) and macOS ignoring timeout_ms are both already documented in the code as deliberate, and the reporter flagged them for completeness rather than as defects. Neither fix has a test: reaching either branch needs a failing ioctl or a failing IOKit call, and both files are compiled only on their own platform.	2026-07-30 08:42:02 -07:00
Matthew Jackson	d444afbdfc	Turn a release-only slice panic into an error, and stop calling 32 kHz 48 kHz ... Four round-6 findings. FileSectorSource::read_sectors guarded its output buffer with a debug_assert, which is compiled out in release — so an undersized buffer panicked with 'range end index out of range' instead of returning an error, out of a public SectorSource impl where the length is caller input. Drive::read_fua already carries this exact guard, with a comment recording the same panic being fixed there, and PrefetchedSectorSource has a regression test for the same case; this impl had been given neither. The new test is red in release for precisely the predicted reason: 'range end index 8192 out of range for slice of length 2049'. parse_track's sample-rate ladder ended in an unconditional S48, so any SamplingFrequency below 44100 was recorded as 48 kHz. A 32000 Hz AC-3 or DTS track is legal and common in broadcast-sourced content, and the wrong rate then propagated into the reconstructed AudioStream. Anything below the lowest mapped rate is now Unknown, which is what the crate's canonical SampleRate::from_hz already returned — the ladder disagreed with it. The ladder itself stays, because the MKV element is a float and wants tolerance rather than exact equality. shim_open_exclusive used the mach port from IOMainPort without checking the return; on failure the port is left untouched and every IOKit call below ran against an uninitialised value. shim_list_drives in the same file does check it. build.rs treated cc and ar as successful if the process merely SPAWNED, so a genuine compile error in the macOS C shim produced no object file and surfaced later as an unexplained link failure against a missing symbol. The shim is macOS-only and is neither linted nor compiled on the other two platforms, so a mistake in it has exactly one chance to be noticed. The last two have no test: one needs IOMainPort to fail, the other needs a deliberately broken C shim, and neither is reachable from the test harness. Both mirror a correct sibling in the same file, which is the evidence available.	2026-07-29 22:56:33 -07:00
Matthew Jackson	921404d135	Fix five clippy errors that only appear on the target CI lints ... CI's clippy job runs on ubuntu-latest, so cfg(target_os = "linux") code is what the gate actually compiles — and none of it is built by clippy on a Mac. Five `-D warnings` errors were sitting in drive/linux.rs, scsi/linux.rs, io/writeback/linux.rs and the Linux arm of drive/mod.rs: four collapsible let-chains and one manual `% n == 0`. CI was red on the lint job while the local gate reported all green. Collapsed into let-chains, which the declared toolchain supports, and folded drive/mod.rs's length precondition into its chain so the body no longer needs a nested block. Found because an agent working on the SCSI backends reported the lints in passing while checking that a Windows-only file compiled. Worth noting how it stayed hidden: every one of these files is cfg-gated to a platform this machine is not, so no amount of local gating would have surfaced them. The companion change to the precommit script closes that hole.	2026-07-29 22:37:47 -07:00
Matthew Jackson	399c3d2769	Reject an over-length CDB on every transport, splice H.264 param sets in place ... Two fixes from round 5. The Linux and Windows backends truncated a CDB longer than 16 bytes (`cdb.len().min(16)`) where macOS returned InvalidCdbLength. Under SPC-4 a command's length is fixed by its opcode group code, so a shortened CDB is not a shorter form of the same command — it is a DIFFERENT command, and the drive executes it and answers GOOD with data for a request nobody made. A silently wrong result on the layer everything else sits on. Rather than mirror the guard a third time it now lives in scsi::mod as checked_cdb_len, with all three backends routed through it, so it cannot drift per platform again. That also makes it testable everywhere: each platform module is cfg-gated to its own host, so a guard inlined into linux.rs and windows.rs would have had no test coverage on any single machine. The shared helper is the only place the behaviour can be asserted on every platform's CI. The two existing macOS tests were tautological — they replicated the guard's logic inline instead of calling it, so they would have passed with the guard deleted. They now call the real helper. Separately, the H.264 keyframe parameter-set re-assert grew a few-hundred-byte prefix buffer to the full access-unit size, copied the whole frame into it, and dropped the presized buffer: one extra whole-frame allocation and copy per keyframe. A UHD title is ~200,000 frames of 150-400 KB with a keyframe every second or two, so that is thousands of avoidable multi-hundred-KB copies per title, each large enough to go through mmap. It now splices into the reserved headroom in place. This mirrors the identical fix already made in hevc.rs, which the H.264 path had drifted from. Byte-for-byte equivalence is pinned by a test whose expected literals were captured from the pre-change implementation, and which I confirmed still passes when the old build-and-copy code is restored. The no-reallocation claim is measured rather than argued: a counter over 30 bare keyframes, which reports 30 of 30 against the old path and 0 with the splice. The reallocation test initially passed even with PARAM_REASSERT_HEADROOM set to zero, because a small parameter set fits in the presize's incidental slack — it proved the fixture did not reallocate, not that the headroom prevented it. Its SPS is now large enough that the constant is load-bearing, so zeroing it fails the test. Not verified: no runtime behaviour on Linux or Windows: no drive, no ioctl. Both files were confirmed to compile for their own targets.	2026-07-29 22:34:22 -07:00
Matthew Jackson	dc5b67ed46	Stop reporting an uncrackable CSS disc as N empty titles ... Same shape as the mkv:// conflation fixed earlier in this round, found by looking for it deliberately. E7023 carried two conditions with opposite correct responses: one title on a multi-VTS DVD failing its own re-crack, where skipping it and finishing the rest is right, and the main feature's crack failing outright, which is disc-wide and dooms every title identically. Because both raised the same code and that code is in is_skippable_title_stub, an uncrackable disc walked all N titles printing "title skipped, it was empty" and exited 0. The disc-wide condition gets E7027 CssNoDiscKey, mirroring the AACS-side E7022 NoDiscKey it is the analogue of, and joins is_disc_level_no_key. The per-title raise keeps E7023 and stays skippable. Because the engine's classifier already tests is_disc_level_no_key before the skippable branch, this reaches the right outcome downstream with no change there: such a disc now stops on the first title and reports no-key instead of returning success with nothing written. Disc::css_error deliberately still stores CssKeyMissing — autorip matches that variant on the field to pick the CSS rather than AACS message, and what consumers classify on is the gate's returned verdict, which is the only thing that changed. Two neighbouring CSS raises were examined and deliberately left alone: the no-key branch in the same function is genuinely unreachable via ensure_decryptable and documented as defensive, and resolve_dvd_title_key is per-title on both of its call paths. Verified by removing the new code from is_disc_level_no_key, which fails both new tests; each pins both directions so neither can silently flip. Not proven end to end against a real uncrackable disc — none available.	2026-07-29 22:29:51 -07:00
Matthew Jackson	5c6a6d0785	Round 5: reject a degenerate fixed lace, bound the pending buffer by bytes ... Five fixes. Three are real defects with regression tests; two are bounds that were expressible but not expressed. A fixed-size lace (RFC 9559 §10.3.4) whose body is empty declared n frames and carried none. The divisibility check passed, because 0 % n is 0, and `chunks` yields nothing on an empty slice whatever width it is given — so the clamp that existed to avoid chunks(0) returned zero frames where the Lacing Head said n. The whole lace vanished with no error raised and the caller saw a clean short block. A zero-size frame cannot be a valid frame, so it is now malformed. A disc read failure while fetching a directory entry's ICB became a file size of zero rather than an error. Zero is indistinguishable from a genuinely empty file, so an unreadable ICB on a damaged disc silently changed which titles a caller saw as present — read_directory already fails hard on its entry-budget guard, so propagating is also what the surrounding code does. read_file_size still returns Ok(0) for an ICB whose tag is neither File Entry nor Extended File Entry, which is a real zero and not a failure. The pending-frame buffer was capped at 4096 frames, which does not bound memory: frames are arbitrarily large and a UHD video frame runs to a few hundred KB, so the existing cap permitted over a gigabyte. Now bounded by bytes as well, at 64 MiB. round_up_grain overflowed for inputs within one grain of u64::MAX — div_ceil then multiply — and the wrapped product is small, turning the largest possible estimate into a negligible reserve. It saturates, and the reserve is clamped to what a `free` box's 32-bit size field can actually hold, since writing a larger one truncated the size and left mdat beyond a box claiming to be far shorter. No real title comes close; a 90 GB UHD title estimates a few MiB. The AC-3 resync guard now advances the PTS cadence like both of its sibling branches, so the three paths out of that block cannot disagree. This one is defensive and has NO test: reaching it needs input that both parses frames and leaves a megabyte of residue, and the parser's own carry rules drop pre-sync junk and cap a partial frame at 8192 bytes, so no such input was found. Stated here rather than covered by a test that would pass either way. Two findings from this round were rejected on inspection. A reported panic in the .mpls suffix check does not exist: the `.get(..)` on the line above returns None off a char boundary and `filter` never runs its closure, so the byte index is unreachable. A test written for it passed against the unfixed code, which is what surfaced the error.	2026-07-29 22:28:43 -07:00
Matthew Jackson	f5e169efb3	Stop reporting a corrupt mkv:// source as an empty title ... E6008 meant two unrelated things: "this title produced no muxable frames", which is a benign stub worth skipping, and "the source file is malformed", which is not. Because a single code carried both, is_skippable_title_stub answered yes to the second one — so feeding a truncated or corrupt mkv:// input made the engine classify it SkippableStub, print a notice saying the title was empty, and exit 0. Silent data loss reported as success. Split into E9053 MkvSourceInvalid for the read path (25 raise sites across mkvstream.rs and ebml.rs's read primitives) and E9054 MkvUnencodable for the four write-side sites, which are the encoder refusing to emit a body at or above the 56-bit VINT limit — an output limit with no input involved, so calling it a corrupt source would be wrong in the other direction. E6008 keeps only the zero-frame guard it was documented to mean. Kept one code for the whole read path rather than one per raise site: nothing a consumer does differs between a bad VINT, a non-UTF-8 string element, a truncated body and a child overrunning its parent. E9052 is the model for when a carve-out earns its keep — laced blocks name one specific RFC 9559 §10.3 feature with its own diagnosis. Also fixed meta_sink.rs raising MkvInvalid for a serde_json encode failure in the json:// sink, where no MKV is involved at all; it now matches the identical guard in mux/meta.rs. Reverting the split at the single code() arm reproduces the old classification: 22 tests fail, including both new assertions. The opposite direction is pinned too — dropping E6008 from the predicate fails the genuine-stub test, which drives the real muxer end to end.	2026-07-29 22:11:53 -07:00
Matthew Jackson	0bbceed985	Round 4: fix 26 defects across crypto, resource use and codec paths ... Twenty-six confirmed findings from the fourth audit round, landed as one cluster because they were found by agents working over disjoint file sets. The one worth calling out is a pair of AACS tests that could not fail. Both asserted CBC behaviour against a hand-rolled expectation that happened to be IV-independent, so replacing AACS_IV with sixteen zero bytes left them passing — they were pinning the code's own arithmetic, not the published constant. Replaced with a literal witness of the published IV plus the NIST SP 800-38A F.2.2 CBC-AES128 vector, and verified the other way round: zeroing AACS_IV now fails three tests. The rest are allocation and correctness work on hot paths: the Annex-B writer in demux_sink allocated and freed a whole-frame Vec per frame, which for a UHD title is ~200,000 allocations over the mmap threshold plus the page faults to first-touch each one; it now reuses a buffer on the writer, and still takes the NAL prefix width from the configuration record rather than assuming four. Six findings whose real fix lives in a consumer crate are recorded for re-filing rather than patched here.	2026-07-29 22:09:52 -07:00
Matthew Jackson	4fcd28b487	Parse MKV lacing, route by real TrackNumber, honour NAL length size and edit lists ... Four conformance defects in the read paths, two of them silent corruption. **Lacing was ignored entirely.** RFC 9559 §10.2 defines Xiph, EBML and fixed-size lacing, where one Block carries several frames; the reader took the Block payload verbatim, so a laced Block became a single "frame" consisting of a lacing header followed by concatenated frames — garbage to the codec parser, no error. Audio tracks from other muxers commonly use lacing, so an ordinary foreign MKV was silently mangled. All three modes are now parsed: Xiph 255-run sizes including the trailing-zero rule for exact multiples of 255, EBML unsigned first size plus SIGNED VINT deltas with the 2^((7*n)-1)-1 bias of §10.3.3, and fixed-size even division, with the last frame's size deduced from the remainder. Laced timestamps follow §10.3.5: the first frame takes the Block timestamp and the rest are spaced by the track's DefaultDuration, else BlockDuration/count, else shared with a warn. Parsing was chosen over refusing because refusal would leave freemkv unable to remux common foreign audio at all, and each mode is about fifteen lines. A malformed lacing header now raises a NEW code, E_MKV_LACING_INVALID = 9052, deliberately NOT MkvInvalid — because is_skippable_title_stub classifies MkvInvalid as a skippable nav stub, so reusing it would have recreated the exact conflation that is still open as a separate finding. A test asserts the new code is not skippable. **TrackNumber was assumed to be 1..N in TrackEntry order.** RFC 9559 §5.1.4.1.1 only requires it to be non-zero and unique, so sparse or unordered numbers are legal. Block routing and codec_private both computed track + 1. A real TrackNumber map is now built, recorded only for TrackEntries that yield a stream so dropped track types no longer shift the mapping. Verified red here independently, and the failure mode is worse than mis-routing: with track + 1 restored, a buttons track's payload was attributed to the AUDIO stream — wrong payload into the wrong codec parser. **The NAL length prefix was hardcoded to 4 bytes.** lengthSizeMinusOne lives in avcC byte 4 and hvcC byte 21 (ISO/IEC 14496-15 §5.3.3.1.2, §8.3.3.1.2) and was never read, so a source declaring 1- or 2-byte prefixes had its raw prefixed bytes emitted verbatim with no start codes. All four conversion sites now derive the width from the track's own configuration record. **Edit lists were ignored.** No edts/elst was parsed, so the presentation timeline an edit list defines (ISO/IEC 14496-12 §8.6.5/§8.6.6) was dropped — which is how encoder delay is normally expressed. Leading empty edits and the first media edit's media_time are now applied to both dts and pts, with the movie vs media timescale distinction respected. A list needing more than a constant shift applies the leading edit and warns rather than presenting the result as faithful. 17 tests. I reproduced the lacing mutant independently: returning the body whole kills five of them, including the exact-payload and malformed-header cases. Still open and deliberately untouched: the MkvInvalid / is_skippable_title_stub conflation across ~20 reader raise sites. It is a cross-cutting error.rs change and E_MKV_LACING_INVALID is the template for it.	2026-07-29 21:53:19 -07:00
Matthew Jackson	9527bc1e13	Anchor forensic segments to the forensic clip, and stop CPS branching on resolve order ... Two correctness defects in AACS 2.1 / FMTS key-map resolution, both order- or anchor-dependent, and both able to abort a whole disc or silently garble it. **The single-CPS short-circuit depended on which title resolved first.** `pool_len` counted the WHOLE unit-key pool, and resolve_fmts_key_map appends the disc's forensic index keys to that same caller-owned pool. The count was captured before THIS call's FMTS branch but not before earlier titles', so once any forensic title resolved, every later title saw a pool larger than one and fell into multi-CPS sampling — 8 random reads per extent, and a whole-disc DecryptFailed if no pooled key opened a menu extent's samples. A disc that ripped fine when a non-forensic playlist sorted first failed when a forensic one did. Forensic keys are now tagged FMTS_POOL_TAG_BASE = 1 << 24 and the short-circuit asks single_base_key_slot(), which excludes them. The old 1000 tag was NOT kept, and the reasoning is worth recording: base CPS ids are Unit_Key_RO.inf position + 1 and that count is a BE16, so 1000 sits inside a genuinely reachable id space. 1<<24 cannot collide. The id field is cosmetic — decrypt.rs indexes the pool by slot and reads only the key — so widening the tag is safe. **Forensic segment SPNs were anchored to the wrong clip.** They live in the forensic feature clip's byte space, but were mapped through clip_byte_to_lba(&title.extents, ..), which treats byte 0 as the start of the title's FIRST extent. Any playlist not beginning with the forensic clip mapped every segment to the wrong LBA: either the anchor probe sampled the wrong clip and the whole-disc resolve aborted with FmtsKeyMissing, or — worse — a forensic index key was applied to non-forensic sectors while the real forensic units kept the base key, giving silently garbled output with no error at all. The correct anchor turns out to be a DISC fact, not title data: an AACS 2.1 disc names its forensic feature BDMV/STREAM/<clip>.fmts, and carries one IndividualSegment.tbl, so the SPNs are in that one clip's byte space. A new forensic_clip_extents() finds the unique .fmts in the already-walked UDF tree, and those extents now drive the segment arithmetic, the addressability filter, and the index probe — whose title parameter is gone, since its reads were mis-anchored too. "Does this title carry forensic content" is now "does it read the forensic clip's sectors" rather than "do the segment bytes land somewhere in the concatenation". Where the clip is NOT identifiable — no .fmts, or several, making the SPN space ambiguous — on a disc that does carry a non-empty table, the resolve now fails loud with FmtsKeyMissing rather than guessing an anchor. That is a deliberate behaviour change: a hypothetical disc with two .fmts clips hard-fails where it previously produced a possibly-wrong map. Failing loud beats silently garbled output, and inventing an anchor was not acceptable. This site had been flagged independently three times — by the agent that added the FMTS per-disc memo, by the round-4 correctness lens with a concrete scenario, and by the round-4 conformance pass. Verified red here independently: reverting single_base_key_slot to count the whole pool fails both new tests. The agent's own evidence was probe_reads 48 vs 40 (the 8 extra sampling reads) and an E7013 DecryptFailed whole-disc abort, and for the anchor an E7026 FmtsKeyMissing on a [trailer, forensic] extent list. All six pre-existing FMTS tests pass unchanged through the new anchor, including the exact-cost assertions (40 probe reads, one key-service call per disc, one UDF walk for 60 titles), so the round-3 memoisation wins are intact.	2026-07-29 21:37:50 -07:00
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.	2026-07-29 21:33:18 -07:00
Matthew Jackson	62450e19bd	Make two public-API panics return errors, and drop two shipped citations ... **The session panics are reachable, and my earlier triage of them was wrong.** `DiscSession::scan` and `resolve_keys` both did `self.drive.as_mut().expect(..)`. I previously downgraded these to LOW on the grounds that no shipped consumer calls them after the drive has been staged into the reader slot. That is the wrong test: `stage_drive_as_reader` is a PUBLIC method that empties the drive slot, so the public surface permits the sequence, and a library must not panic from public API regardless of what current callers happen to do. Both now return Error::DeviceNotReady. **A shipped doc comment cited a third-party source FILE** as the authority for the CLPI ProgramInfo layout ("Layout per the BD CLPI spec clpi_parse.c"). Now cites the Blu-ray Disc Read-Only Format Part 3 CLIPINF specification. **The CHANGELOG justified a muxer decision by naming a commercial competitor** ("MakeMKV's rip of the same disc omits it", "matching MakeMKV"). Reworded to stand on its own terms: the element is optional in RFC 9559, nothing requires it for interlaced SD, and the 40 ms DefaultDuration is the frame rate the source actually carries. The leak gate is extended for both new classes — third-party `*_parse.c` / `*_dec.c` / `*_demux.c` style filenames, and a competitor named as authority. Narrowing that rule took two attempts, which is worth recording. A bare `\.(c|cpp|cc)` pattern produced eight false positives: this repo has its own C shim (`macos_shim.c`) that build.rs and the docs legitimately reference, and the pattern also matched the Rust field access `p.cc`. It now matches only the suffixes typical of third-party media-library sources. This is the second false-positive round on this rule — the first flagged ffmpeg INVOCATIONS in the test harness — so the lesson is that a hygiene pattern needs testing in both directions before it lands, exactly like any other code.	2026-07-29 21:21:29 -07:00
Matthew Jackson	013881ac06	Restore the MP4 conformance fixes I clobbered while landing another agent's work ... detect_rate's nearest-match fix, the colr HLG/BT.470 fix and their four tests were silently reverted. Cause: the r3fix-silent worktree was cut BEFORE the conformance commit landed, and I landed its work by copying whole files into the main tree. mp4/mod.rs was in both agents' file sets, so silent's copy — built on the older base — overwrote the conformance changes wholesale. The gate stayed green throughout, because reverting a fix and its tests together is perfectly consistent. Re-applied the conformance commit's diff for that file with a three-way merge; both agents' changes to mp4/mod.rs now coexist (final_report, UndescribableAudio and the max(track_id) id fix are all still present alongside RATE_TOLERANCE_FPS and the colr resolver delegation). Only mp4/mod.rs was affected. mp4/audio.rs and mkv.rs were in no other agent's file set and were intact. Process lesson, recorded because I would otherwise repeat it: NEVER land a parallel agent's work by copying whole files, when its worktree was cut at an older base than HEAD. Apply its DIFF (git apply -3), or rebase its worktree first. Copying files silently discards anything committed to those files in the interim, and no test can catch it because the tests disappear with the code.	2026-07-29 21:03:26 -07:00
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.	2026-07-29 21:00:55 -07:00
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.	2026-07-29 20:47:31 -07:00
Matthew Jackson	3efa6211f3	Make six silent mux failures observable ... All six confirmed against the code. The governing rule this cluster serves: a lossy or degraded outcome is never silent, because a corrupt rip the user does not know about is the worst failure available. **A 3D MKV re-mux silently lost one eye.** The BlockGroup read path had arms for BLOCK / BLOCK_DURATION / REFERENCE_BLOCK only, so BLOCK_ADDITIONS fell into the skip arm — while the writer does emit BlockAdditions > BlockMore > BlockAdditional for the MVC dependent view. Reconstruction was judged out of scope and the reasoning is recorded: PesFrame has no side-payload field and the header parser never reads BlockAdditionMapping, so there is no dependent-view track to route the AU to. Instead the loss is now LOUD — counted in bytes and events, warned once, and surfaced through MkvStream's errors()/lost_bytes(), which the driver already samples into MuxOutcome. One detail in the finding was wrong and is corrected: the re-mux does NOT still advertise the mvcC mapping, because the header parser ignores that element, so the output is a plain 2D H.264 track. **An all-titles rip silently skipped real titles.** The header-buffer-cap overflow returned Error::MkvInvalid, and is_skippable_title_stub matches exactly E_MKV_INVALID | E_CSS_KEY_MISSING — verified here — so a 512 MiB-of-frames title was classified as an empty nav/menu PGC stub and dropped. It now has its own E9051 / MuxHeaderBufferExceeded { bytes }, outside the skippable set. **The public pre-mux report contradicted the file.** Mp4Sink::finish() drops an audio track it cannot describe, which I chose last round over failing an export whose video is fine — but mp4_fit_report still listed that stream as included, so the application's plan and the actual output disagreed. Fixed at both levels: Mp4SkipReason is now non_exhaustive with NoSamples and UndescribableAudio, Mp4Sink::final_report() describes the FILE rather than the plan, and for the boxed dyn Stream path a defaulted Stream::undelivered_streams() carries the information out to MuxOutcome::undelivered_streams with a driver-side warn. **MP4 track ids could collide.** ids were assigned before the retain that drops sample-less tracks, while next_id came from the post-retain count, so [1,3] yielded next_id 3. Now max(track_id) + 1, saturating. **Stream selection silently skipped its codec_privates prune** when the lists were not the same length — but codec_privates is consumed POSITIONALLY and trailing extras are documented as benign, so the length-equality guard was itself the bug. The prune now runs unconditionally by index. **The m2ts_mux scaffolding armed params_written on both the absent and the unparseable codec_private arms** — the same defect already fixed in tsmux.rs. Split into params_attempted (a latch, since retrying identical bytes cannot help) and params_emitted, with a warn on each failure arm and an accessor so the eventual wiring and its test can observe it. Each fix verified red by mutating back to the prior behaviour: errors() 0 vs 1, E6008 vs E9051, final_report [0,1] vs [0], next_track_id 3 vs [1,3], and the selection prune resolving index 1 to the wrong track's record. API surface deliberately widened: MuxOutcome gains a public field and Mp4Sink becomes public. Nothing in-repo breaks. Note a behaviour change on the mkv:// input path — a 3D re-mux now reports non-zero loss, so a consumer treating errors > 0 as disc damage will trip on it. That is intended: the outcome IS degraded.	2026-07-29 20:46:10 -07:00
Matthew Jackson	9ad68dd092	Fix four MP4 conformance defects against the standards ... **dec3 declared a 0 kbit/s AC-3 substream.** parse_dolby routes bsid < 11 to parse_ac3, which leaves data_rate_kbps = 0 and keeps the AC-3 bsid, yet dolby_sample_entry wrapped that config in ec-3/dec3 for any Codec::Ac3Plus track. ETSI TS 102 366 Annex F.4 assigns ac-3/dac3 to an AC-3 bitstream and F.6 assigns ec-3/dec3 to an Enhanced AC-3 one, so the entry now follows the SYNCFRAME that was actually parsed, not the playlist's codec label. Computing an AC-3 data rate and keeping ec-3 was rejected: it fixes one field while bit_stream_identification and num_dep_sub keep misdescribing the stream. The bsid threshold is hoisted into one constant so parser and entry-chooser cannot drift. Adjacent defect fixed in the same box: data_rate is 13 bits from a u16 source, so push's mask WRAPPED anything above 8191 (9000 became 808); it now saturates. **colr tagged HLG as PQ, and PAL as BT.601.** video_colr carried a second, drifted copy of the ColorSpace-to-CICP map: transfer 16 (PQ) for every BT.2020 stream with no HdrFormat override, and 6 (BT.601) for Bt470bg. Per ITU-T H.273 Table 3, HLG is 18 and BT.470-6 System B/G is 5 — and mkv::cicp_for_video already got both right. video_colr now delegates to that shared resolver, so the duplicated table is gone and cannot drift again. It keeps only its own decision about WHETHER to emit the box, since an absent colr and an all-unspecified colr mean the same thing per ISO/IEC 14496-12. **Exact 24.000 / 30.000 / 60.000 fps was declared 23.976 / 29.97 / 59.94.** detect_rate took the FIRST STD_RATES entry within 0.5 fps, and every 1000/1001 entry precedes its integer twin 0.024 fps away — a 0.1% error across the whole track's mdhd and stts. Fixed as nearest-wins rather than by reordering the table: reordering fixes today's table and re-breaks the moment someone appends a rate, while nearest-wins is order-independent. Verified red here independently by reverting to first-match, which fails exactly the two timing tests. **ddts MultiAssetFlag was set from has_extension.** In the DTSSpecificBox (ETSI TS 102 114) that flag signals more than one audio ASSET. A DTS-HD MA/HRA track is one asset whose extension substream carries the XLL/XBR component, so setting it from "an EXSS sync follows the core" sent a parser looking for a second asset descriptor while StreamConstruction simultaneously said there was no extension — the box contradicting itself. This module parses the core header only and never reads the EXSS asset table, so 0 is the only honest declaration. A StreamConstruction index for core+EXSS was deliberately NOT invented: that field is a table lookup that could not be confirmed against the standard, and a wrong index is worse than an under-declaration. DTS_AMODE_LAYOUT's masks were independently re-derived against ETSI TS 102 114 §5.3.1 and all 16 are CORRECT — only three adjacent comments were wrong (the AMODE 2/3/4 annotations were rotated by one, and AMODE 9's said "5.1 with LFE" when 0x0007 is the 5.0 mask and LFE is OR'd in separately). Comments corrected. Every one of the eight new tests decodes the field back OUT of the emitted bytes — data_rate from the dec3 body's leading 13 bits, MultiAssetFlag from bit 48 of the ddts tail, colr from the nclx payload inside a real stsd, and the frame rate from mdhd.timescale plus stts.sample_delta of a fully muxed MP4 — rather than restating arithmetic. This audit has already caught one of my own tests doing the latter. NOT fixed, root-caused and recorded at the site instead: an A_PCM/INT/BIG track ships with no BitDepth, which the Matroska Codec Specifications make a MUST. The width exists on disc (BD LPCM signals it in the ES header byte 3, DVD in the IFO audio attribute byte 1) but neither source reaches MkvTrack::audio, and the fix needs a new AudioStream member plus a deferred setter in files another agent held this round. Guessing 16 was rejected — it would confidently misdecode every 24-bit disc — as was refusing the track, which would regress the 16-bit majority that currently plays by accident.	2026-07-29 20:29:17 -07:00
Matthew Jackson	13897e14f0	Resolve the forensic key map once per disc, not once per playlist ... resolve_content_key_map loops every title into resolve_mux_key_map, which called resolve_fmts_key_map FIRST — before the CpsUnitCache — and on an FMTS disc returned immediately. So every playlist re-derived facts that belong to the DISC: a full UDF walk plus /AACS/IndividualSegment.tbl, and on an FMTS disc the anchor probe, the 32-index phase probe, and a fetch.fmts_indexes round trip. On a 60-playlist disc, measured on a synthetic fixture: 840 -> 14 metadata reads, 2,400 -> 40 probe reads, and 60 -> 1 key-service calls. Worst case before was up to 256 probe reads and 32 key-service calls per title. The 60 redundant key-service round trips are a strong candidate for the keyserver storm seen in the field. Two memos behind a pub(crate) DiscKeyCache. The table memo (UDF walk + tbl parse) is disc-invariant outright — nothing in that path mentions the title — and runs on EVERY disc, so a plain BD benefits too. Only the deterministic negatives are memoised as "not FMTS"; a DiscRead fault propagates uncached so a later title retries. A blind once-per-disc hoist of the PROBES was rejected as unsafe, and this is the load-bearing reasoning: the title enters through clip_byte_to_lba, which decides which segments are addressable and which LBA every probed clip byte reads from, so two titles with different extent lists probe different physical bytes. A hoist would serve title B an answer derived from title A's media and could silently turn a per-title FmtsKeyMissing into a success. The extent list is the ONLY per-title input, so keying on it is exactly sufficient — matching the ForcedProbeCache precedent. Result-identity was proved, not assumed: NEITHER probe reads the key pool. Verified here independently — probe_fmts_index_keys takes no keys parameter at all; index keys come from `fetch`, and the anchor's reply feeds the phase probe. So the pool's growth across titles, the one thing that does change between calls, cannot move a memoised value, and the result is order-independent. A test resolves three titles through a shared memo and through fresh memos and asserts both the per-title ranges and the final key pool (keys, slots, order) are identical. Not memoised, deliberately: fail-loud FmtsKeyMissing, and any run where an index hit a read fault — that is a property of a transient drive fault, not of the extents, and caching it would spread one bad read across 59 playlists. A fully-memoised title now does zero I/O, which made the old in-loop halt polls unreachable for it, so a check_halt on entry was added with a test that cancels after warming the memos. Also corrects my own overstatement from last round: the CpsUnitCache doc now says plainly that on an FMTS disc it removes NO reads, because this function returns before the extent loop ever runs. Five mutations, all verified red. Pre-existing bug flagged but not fixed: filter_addressable_segments only checks that a segment's START byte maps to some LBA in the title, so a play-all playlist can pass the filter while mapping segment bytes into the wrong clip, whose anchor then returns empty and aborts the sweep.	2026-07-29 20:27:50 -07:00
Matthew Jackson	b4bf0daa82	Group E-AC-3 dependent substreams into one access unit ... The AC-3 parser's own module doc stated the assumption: "AC3 frames are self-contained and always start with syncword 0x0B77". True for legacy AC-3, false for E-AC-3 above 5.1. Per ETSI TS 102 366 (A/52) Annex E, byte 2 of an E-AC-3 syncframe is strmtyp(2) | substreamid(3) | frmsiz[10:8], and an access unit is one INDEPENDENT substream plus every DEPENDENT substream that follows it until the next independent one. The parser emitted one PES frame per syncframe, so a decoder saw each dependent substream as a standalone frame with no parent — including the AC-3-core + E-AC-3-dependent form Blu-ray uses for Dolby Digital Plus. The extra channels were lost and the timeline ran at 2x. The bit position is cross-checked against code already in the tree: the existing frmsiz parse takes byte2 & 0x07 as its high bits, which is only consistent with strmtyp occupying byte2's top two bits. Legacy AC-3 is excluded by bsid < 11, where byte 2 is crc1 and reading strmtyp there would be nonsense. Reserved strmtyp 3 is treated as INDEPENDENT so an unknown type starts a fresh AU rather than merging into an unrelated one. The AU carries the INDEPENDENT substream's PTS, and only the independent substream advances the clock — dependents cover the same time period and add zero duration. That is what removes the doubled timeline. A trailing AU that can still grow is HELD across the PES boundary, because the boundary is unknowable until the next independent sync; the whole AU is re-scanned next call, so there is no shift and no double-count in the loss tally. Plain AC-3 is never held, which keeps DVD/AC-3 latency and behaviour unchanged. A latent pre-existing bug surfaced while testing this: a new PES's PTS was re-stamping an AU that began in an earlier PES, a constant one-frame shift. Fixed with a PtsAnchor so a PES timestamp applies to the first AU that STARTS in that PES's own bytes, while a genuine PTS jump is still adopted. Nine tests. Verified red against five mutations, each killing a specific set: reverting to the pre-fix behaviour kills 8 while plain_ac3_frames_are_not_grouped_or_delayed SURVIVES as the no-regression guard — reproduced independently here. Stamping the dependent's PTS kills 6; not holding across PES kills 4; holding plain AC-3 too kills 15; neutering the PTS anchor kills exactly the 2 split-across-PES timing tests. Three sibling defects found and deliberately NOT fixed, all in mp4/audio.rs: dec3 hardcodes num_dep_sub = 0 (and a nonzero value changes the box LAYOUT, not just a field, per Annex F/G); parse_eac3 ignores strmtyp/substreamid entirely; and a 7.1 DD+ track is still labelled 5.1 because the channel count comes from the independent substream while the extra channels are described by the dependent's chanmap, which nothing parses. Not verified: no real E-AC-3-with-dependents sample exists here, so all evidence is synthetic frames plus the spec layout. The multi-independent-substream case (num_ind_sub > 1, main + associated audio in one PID) is deliberately treated as one AU per independent substream and is untested.	2026-07-29 20:26:51 -07:00
Matthew Jackson	ef36b452ad	Fix three defects in last round's own fixes ... Round 3 audited the round-1/2 fix commits rather than trusting them, and found three defects in that new code. This is why the pin moves each round. 1. LICENCE REGRESSION, and it was mine. Reverting the "distinguish a failed key source" commit also restored a verbatim reference-decoder table citation in src/mux/codec/dts.rs, because both changes were in that one commit. The MIT licence cleanup was silently undone at HEAD and nothing caught it. The citation is replaced with ETSI TS 102 114 §5.3.1 again, and — more importantly — the rule now lives in the leak gate instead of in my memory. scan-secrets.sh gains LICENCE_RE, which flags ff_dca*, dcadec, l-smash, libav*, and bare ffmpeg/FFmpeg as REF-IMPL-CITATION. `no ffmpeg` is explicitly allowed via negative lookbehind: stating what this project does NOT depend on carries no risk and is a genuine selling point. Verified by re-introducing the citation (gate fails) and removing it (gate clean). 2. TsMuxer armed params_written even when the avcC/hvcC parser returned None, so a track whose codec_private exists but will not parse was muxed to BD-TS with no VPS/SPS/PPS ever emitted — undecodable video, reported as success, with no log line. Last round's fix corrected WHICH parser is used and left this half untouched. Arming the flag is still right (retrying identical bytes cannot succeed) but it is no longer silent: it now warns with the track, codec and codec_private length. 3. test_aes_cbc_roundtrip defined a LOCAL fn aes_cbc_encrypt that SHADOWED the production primitive, so it round-tripped a copy of the algorithm against itself and never touched crypto::aes_cbc_encrypt — the function this cycle added. Any mutation to the shipped code passed it. The shadow is deleted and the test now calls the real primitive; verified by mutating crypto::aes_cbc_encrypt, which now fails it and previously would not have.	2026-07-29 20:08:09 -07:00
Matthew Jackson	f338552969	Pin the toolchain to Rust 1.87 ... The Windows UI needs winsafe, whose current release requires rustc 1.87. The alternative was pinning winsafe back to an older release, which would bake a stale API surface into a brand-new UI permanently to dodge one minor version. The pin's purpose is to sit BELOW the Mac default so clippy drift is caught locally before CI, not to stay on 1.86 specifically, so 1.87 preserves the discipline exactly. Verified before moving anything, not after: `cargo +1.87 clippy -- -D warnings` and `cargo +1.87 fmt --check` are clean across all eight repos, and the full precommit gate (fmt + clippy + tests) passes on libfreemkv, autorip, bdemu, freemkv-engine and freemkv-keysources. Zero new lints, zero formatting drift.	2026-07-29 20:02:02 -07:00
Matthew Jackson	38aa895038	Memoise multi-CPS key-map sampling per extent, not per title ... resolve_content_key_map calls resolve_mux_key_map once per title, and on a multi-CPS disc that path issues 8 random single-unit reads per extent. A disc's playlists overwhelmingly reference the same few clips — main feature, play-all, per-chapter and seamless-branch variants — so the same physical extents were re-sampled from the drive once per playlist. On a 60-playlist / 15-clip disc that is ~2,400 non-sequential 6144-byte reads, roughly 8 minutes of pure seeking at 200 ms per seek, before the mux starts. Now ~600 reads. Keyed per EXTENT — (format, start_lba, sector_count) — rather than per title's whole extent list, which is finer-grained than the forced-subtitle probe's cache and strictly better here: a play-all playlist sharing 4 of 5 extents with the main feature still hits on those 4. Why a cached pool index is provably identical to a recomputed one, verified rather than assumed: * `pick` iterates the pool IN ORDER and returns the FIRST index whose key decrypts a sample to clean. * The pool is APPEND-ONLY. Checked across the whole crate: only `push`, with no insert/remove/clear/retain/sort/dedup/truncate/drain/swap/reverse anywhere. So appended keys can only land AFTER a matched index, and the first match for the same samples cannot shift. * The samples are a pure function of the three values in the key, read from read-only optical media. Two outcomes are deliberately NOT cached, which is what makes this safe rather than merely faster: * the inherited index (`None if samples.is_empty() => last_idx`) is per-TITLE state, not a property of the extent — caching it would let one title's carry-in index leak into another title's clear extent, i.e. a WRONG key; * the fail-loud DecryptFailed verdict, so a retry after a key source banks the missing key re-samples instead of inheriting a stale answer. Halt is still polled before the cache lookup, so cancellation is unchanged. resolve_mux_key_map keeps its exact signature and delegates with a fresh cache, so there is no public API change. ContentFormat gains Eq + Hash (additive). Four tests, and the two mutants that matter both verified red: disabling the cache short-circuit fails the hit and recompute-equivalence tests, and wrongly caching the inherited index fails multi_cps_inherited_index_is_not_cached.	2026-07-29 19:49:55 -07:00
Matthew Jackson	e3676e7cdf	Build BlockGroups in memory so the hot path never seeks ... Every BlockGroup frame back-patched its element size via ebml::end_master, which does two stream_position() calls and two real seeks. BufWriter does not override Seek::stream_position, so each position query is seek(Current(0)) = flush_buf + lseek — and each of those flushed the 4 MiB BufWriter while every position-moving seek reset WritebackPipeline::last_flush_pos. The buffer never got to do its job. An MPEG-2 title takes this path for EVERY frame (the parser stamps a per-frame duration, so I, P and B all become BlockGroups) — roughly 350,000 per feature. A BlockGroup's size is knowable before writing, so there is no need to back-patch at all. New seek-free twins start_master_buf / end_master_buf patch a placeholder by buffer INDEX instead of file offset, and build_block_group assembles the whole element into a persistent buffer that write_block_group and its MVC sibling take, fill, write once, and hand back — including on the error path — so the allocation is made once rather than per frame. Measured with a counting writer that, like BufWriter, does not override stream_position, over 200 frames: seek calls position-moving plain before 912 451 plain after 112 51 MVC before 2516 1253 MVC after 116 53 Per-frame cost is now zero; the residual is the header, the per-cluster back-patch and Cues. For 350k BlockGroups that is 1.4M seek calls removed on the plain path, 4.2M on an MVC title. end_master is deliberately NOT changed for its other callers. The Cluster master genuinely streams — frames are appended to an open cluster over time, so its body cannot be buffered without holding a whole cluster in memory — and the rest (EBML header, Tracks, Info, Chapters, Cues) run once, not per frame. Byte-identity is the safety property, and it is structural: end_master always patches a FIXED-width 8-byte VINT, and the buffered pair writes and patches exactly that same placeholder, so the encodings cannot differ. Verified by capture-then-compare over 200 frames (17 keyframes / 183 non-keyframes, multiple clusters, BlockDuration present and absent, both reference branches) — output byte-for-byte identical. Three tests now pin it permanently: buffered vs seeking output byte-for-byte for empty/tiny/multi-byte bodies, the same for NESTED masters (the MVC path nests BlockAdditions > BlockMore, where an index-arithmetic slip would surface), and end_master_buf erroring rather than panicking on a position outside the buffer. All three verified red against a deliberately divergent placeholder width. A note on verifying this kind of change: comparing emitted MKV across two worktrees at different commits shows a spurious 14-byte difference, because MuxingApp/WritingApp embed the build's git SHA twice. Compare at the same base.	2026-07-29 19:33:31 -07:00