bc5e3453b48c2fc45bfd8d795618a9844c62ba47
1163
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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.
|
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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.
|
||
|
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.
|
||
|
Matthew Jackson
|
807eb053ca |
Only assert a forced-subtitle verdict the read actually supports
probe_and_set_forced broke out of its read loop on a read error but still
applied whatever partial observation it had accumulated as an authoritative
verdict, overwriting the vendor-label-derived forced flag. A disc that faults
early could have a correct flag replaced by a guess from a fraction of the data.
The file already got the zero-observation case right — it deliberately leaves
the vendor flag alone rather than "assert not-forced from having seen nothing".
The defect was that a PARTIAL observation cut short by a fault was treated as
complete.
The fix rests on the two verdicts not being symmetric evidence.
settled_not_forced() is POSITIVE evidence — a non-forced display set was
actually seen, and no unread data can retract it. is_forced() is an ABSENCE
claim — display sets were seen and none was non-forced — which is only sound if
the read got far enough for the absence to mean something.
So every loop exit now yields a named StopReason, and absence claims are
asserted only for a designed stop:
* Exhausted / Budget → conclusive. The budget is deliberately conclusive: the
natural exit is "every track settled not-forced", which a genuinely forced
track never satisfies, so the budget is the ONLY path by which a real forced
verdict is ever reached. Treating it as inconclusive would disable forced
detection entirely.
* Halted / ReadFailed → inconclusive. A cancelled probe's cut-off point is as
arbitrary as a faulted one, so its absence claim is worth no more.
Evaluated per track, matching the existing observed() gate: a track that already
saw a non-forced set keeps its sound verdict even on a truncated run, while a
forced-so-far sibling keeps the vendor flag.
An inconclusive run is also NOT memoised. The cache key is the extent list and a
disc's playlists share clips, so caching a truncated run would replay one read
fault onto every playlist referencing those extents and deny any later title the
chance to re-read them.
Four tests; three of them verified red by forcing absence_is_conclusive() back
to always-true (the old semantics), while the budget test correctly stays green
either way — confirming the guard was not over-corrected.
Also moves the function doc comment back onto probe_and_set_forced; the earlier
probe commit left it attached to the ForcedProbeCache type alias.
|
||
|
Matthew Jackson
|
7322f4dd8a |
Record that the failed-vs-absent key source arm is unreachable
The `Ok(_) | Err(_)` arm in resolve_and_apply_traced conflates "this source had no entry for the disc" with "this source failed", and reports both as KeyNode::NoEntry. An operator whose key server is returning 502s is therefore told their disc is not in the database. The conflation is real but LATENT, and fixing it here would change nothing an operator can see, because no shipped KeySource ever returns Err: KeydbSource::get_unit_keys maps a load/parse failure to Ok(Vec::new()), OnlineSource::get_unit_keys is Ok(self.query(ctx)) where query returns empty on transport error, HTTP status, oversize body and bad JSON alike, and MultiSource discards inner Errs. Only test doubles return Err. FetchOutcome::errored in drive_unit_keys / drive_fmts_indexes is dead for the same reason — the right contract, honoured by no source. autorip already works around the missing signal by re-probing the service over HTTP (probe_online_reachability / key_service_transient_status), and its own comment names the incident: "the online keysource swallows every failure (transport error, 502, timeout)". So the fix belongs at the source boundary in freemkv-keysources, with Disc::aacs_error as the channel the operator actually reads — not in this trace. Documented here so the next reader does not assume the arm works, and does not "fix" a dead path as I nearly did twice. |
||
|
Matthew Jackson
|
4ed245868e |
Revert "Distinguish a failed key source from one with no entry"
This reverts commit
|
||
|
Matthew Jackson
|
50f37462db |
Remove reference-decoder citations from a public MIT-licensed repo
This crate is MIT licensed. Comments citing another decoder's internal symbols and reproducing its tables verbatim create licence risk that no engineering benefit justifies, so every such citation is replaced with the primary source: ETSI TS 102 114 §5.3.1. Eleven sites across src/mux/codec/dts.rs and src/mux/mp4/audio.rs. The technical substance is unchanged in every case — the deficit-sample-count semantics, the reserved-field skips, the invalid LFF value, and the 16 legal AMODE codes are all spec facts and are now attributed as such. Four CHANGELOG entries that named a validator are reworded; the "no a reference decoder" dependency claim stays, since stating what this project does NOT depend on carries no risk. I initially argued this was a false positive on the grounds that the project's hygiene rules name internal infrastructure and reverse-engineering material, not open-source citations, and that a channel-count table from a standard is fact rather than expression. That reasoning missed the point: the exposure is MIT distributing text derived from GPL/LGPL sources, and that is the maintainer's risk to weigh, not mine. Reversed in full. |
||
|
Matthew Jackson
|
22a3e3fd01 |
Distinguish a failed key source from one with no entry
resolve_and_apply_traced collapsed `Ok(_) | Err(_)` into a single KeyNode::NoEntry step, so a key source that FAILED — server unreachable, keydb unreadable, malformed entry — was recorded identically to one that simply had no entry for this disc. The front-end renders that trace, so it told the operator their disc is not in the database when the real cause was a fixable infrastructure problem. drive_unit_keys and drive_fmts_indexes were refactored this cycle to preserve exactly this distinction; this path had not been. KeyNode gains a SourceFailed variant and the two arms are split. freemkv's trace renderer matches KeyNode exhaustively with no catch-all, so its arm is added in the same change — otherwise the consumer would not build. Also made ETSI TS 102 114 the primary authority for DTS_AMODE_COUNT's comment rather than a reference decoder internal symbol, and pointed it at this crate's own cross-checked DTS_AMODE_LAYOUT / DTS_AMODE_CH tables. A round-2 finding asked for every a reference decoder and a reference decoder citation in the DTS parser to be stripped as a public-repo hygiene violation. Rejected: the project's rules (scan-secrets.sh, CLAUDE.md) prohibit internal infrastructure references and reverse-engineering material, and a reference-decoder citation is neither. The AMODE channel-count table is a factual table from the standard, not expression copied from an implementation. Citing the spec plus a corroborating implementation is how a decodability gate should be justified. |
||
|
Matthew Jackson
|
99c5fd3500 |
Reference keyframes per track, size the DTS reserve, correct two claims
The ReferenceBlock offset was computed for ANY video track, but the keyframe tick it measures against was recorded in a single global slot gated to the PRIMARY video track. On a title with two video tracks — an MVC base plus secondary view, or a multi-angle disc — a secondary track's non-keyframe therefore referenced a keyframe on a different track, or 0 (a self-reference) when the primary had not produced one yet. The tick is now recorded per track, so a non-keyframe can only reference a keyframe on its own track. The faststart moov-hole estimate modelled every audio track as (E-)AC-3 at 1536 samples per frame. 1.6.0 added DTS to the writer's carried set, and a DTS core AU is commonly 512 samples — a third of that — so a DTS track's sample table was under-reserved threefold and the mux fell back to moov-at-end, losing faststart on exactly the files 1.6.0 newly supports. mvc_frame_emits_blockgroup_additional_and_reference asserted only that the non-keyframe's ReferenceBlock was Some(_). Its non-MVC sibling, added in the same commit, pins the exact offset; this one now does too, so a mutant emitting a constant or wrong-signed offset no longer passes. The comment above the mp4 sample budget claimed file_len stops a crafted file inflating allocations "past the file's own size". Each indexed sample costs ~52 bytes, so the real ceiling is ~52x file_len (still capped by MAX_SAMPLE_COUNT). The bound is real; the comment overstated how tight it is. |
||
|
Matthew Jackson
|
d4c913e0d3 |
Validate stream-selection PIDs per class, not across both
StreamSelection::apply validated a listed PID by scanning ALL streams, so a PID named in the wrong class's filter passed validation — an audio filter listing a subtitle PID, say. `keeps` then matched it against the audio streams only, so the requested track was silently absent from the output. That is precisely the outcome this validation documents itself as preventing: "fail loud rather than silently emit an MKV missing a requested track". Each filter is now checked against its own stream class. The `listed_pids` helper existed only for the cross-class scan and is removed rather than left behind as dead code. Test covers both directions plus the sanity case, and asserts a rejected selection leaves the title unpruned. |
||
|
Matthew Jackson
|
0e23a6b291 |
Halve the per-frame allocation and copy on the m2ts NAL video path
The NAL path called length_prefixed_to_annex_b, which allocates a whole-frame Vec of its own, then copied the result into a second whole-frame Vec — two full-frame allocations and two full-frame copies per video frame. The crate already has append_length_prefixed_as_annex_b, which writes the conversion straight into a destination buffer; it is the same code path with the intermediate removed. The destination is also sized once up front instead of starting from Vec::new(), which re-grew from zero capacity inside every conversion. On a UHD HEVC title muxed to m2ts:// — ~200k video frames averaging ~310 KB of ES at 60 Mb/s — that removes roughly 62 GB of allocation and 62 GB of memcpy. Behaviour is unchanged: the existing tsmux conversion tests, including the non-NAL passthrough and Annex-B default pair added last round, all still pass. A first attempt reused a persistent scratch buffer across frames, which does not work: the buffer is handed out as Cow::Owned and so can never be returned. A right-sized single allocation gets most of the win without restructuring the function around the borrow. |
||
|
Matthew Jackson
|
bcf47cc4ca |
Fix ddts numeric truncation and the decrypt pool's poison asymmetry
ddts CoreSize wrapped to zero on a maximum-size core. core_size is FSIZE + 1 and FSIZE is itself 14 bits, so the maximum is 16384 — one past what the 14-bit CoreSize field holds — and push()'s mask turned that into 0, declaring an empty core frame. Clamped to 16383 instead: one byte short beats telling a decoder there is no core. Proven red first (the field read back as 0). ddts avg/max bitrate under-declared every non-integral frame rate. It computed sample_rate / frame_samples first, so a 512-sample core at 48 kHz truncated 93.75 frames/s to 93. Multiplying before dividing, with round-to-nearest, keeps the precision. set_decrypt_threads skipped the pool swap on a poisoned lock while DECRYPT_THREADS had already been updated, so the new thread count was reported as taking effect while the stale pool kept serving. decrypt_pool() deliberately recovers from poisoning for exactly this reason; the setter now does the same. The pool Arc is immutable once stored, so a prior panic cannot have left it half-written. The CoreSize test decodes the value back out of the emitted box rather than restating the clamp — a first draft asserted the clamp arithmetic against itself, which would have passed against the unfixed writer. |
||
|
Matthew Jackson
|
a9dc3d7244 |
Make encrypt_unit report a refused slice, and expand its key once
Two defects in the encrypt_unit promoted to public API last round, both found by round 2 auditing that new code. It returned silently without encrypting when the slice was shorter than ALIGNED_UNIT_LEN. Its own contract requires the caller to set the container's encrypted flag BEFORE calling — the header is the key seed — so a silent no-op leaves a unit advertised as encrypted while still carrying plaintext, with nothing for an authoring caller to check. It now returns bool and is #[must_use], so ignoring the refusal is a compile-time warning; every call site was updated to assert on it. bool rather than Result deliberately: a wrong buffer length is a programming error at a library boundary, not a disc condition, and a new Error variant would mean a new numeric code plus its rendering in another repo. It also drove CBC from the single-block aes_ecb_encrypt, rebuilding the AES key schedule for each of the 383 blocks in a unit — an order of magnitude slower than its inverse, which expands the key once via aes_cbc_decrypt. The missing counterpart aes_cbc_encrypt now exists alongside it, and encrypt_unit calls it, so the two directions are symmetric in structure as well as in result. For an authoring caller encrypting a 90 GB image that removes ~5.6 billion redundant key expansions. New test pins the boundary: ALIGNED_UNIT_LEN - 1 returns false and leaves the buffer byte-identical, ALIGNED_UNIT_LEN succeeds. The existing round-trip and padding-asymmetry tests still pass, so the CBC rewrite is provably the same transform. |
||
|
Matthew Jackson
|
94a876664b |
Correct six stale comments and doc claims
All six describe code that does something different from what they say, which is the class of defect that gets a maintainer to write a bug on purpose. docs/clpi.md presented the CLPI stream-PID entry as byte-aligned 2/2/2/4/4-byte fields with a 32-bit fine-entry count. It is one 80-bit packed block — reserved(10) + EP_stream_type(4) + num_EP_coarse(16) + num_EP_fine(18) + EP_map_start_address(32) — and num_EP_fine is 18 bits. Anyone parsing to the doc's offsets would read garbage. Replaced with the real bit layout. docs/udf.md said read_directory()'s recursion cap is 3; MAX_DIR_DEPTH is 8. TROUBLESHOOTING.md called Pass 1 `recovery::copy`. The engine's `sweep` is documented as "Pass 1 of a multipass rip"; `copy` is the dispatch verb that chooses between sweep and patch. This inconsistency was mine, introduced in the 1.6.0 doc rewrite. docs/drive-access.md already said `sweep` and was right — a round-2 finding claimed the opposite on the grounds that `recovery::sweep` appears nowhere else in this crate, which it cannot, being in another crate. io/pipeline.rs cited `disc::patch` as WRITE_THROUGH_DEPTH's caller; that moved to freemkv-engine in 1.6.0 and no `patch` exists here. truehd.rs's doc on mlp_major_sync_crc_ok said the trailer is compared big-endian while the body compares u16::from_le_bytes — and a big-endian compare was the bug the function was fixed for, so the comment described the defect rather than the code. sector/decrypting.rs claimed the decorator owns "the only mutable state (its call-count cap and spent flag)". DecryptingSectorSource has no such fields and no KeyFetch field at all in this revision. |
||
|
Matthew Jackson
|
7030de4ec9 |
Apply stream selection on the live path, and treat a halt as a clean stop
Three defects around MuxOptions in the mux driver. MuxInput::Live never applied MuxOptions.selection, so a caller's audio/subtitle selection was silently ignored on the live-drive path while the field's own documentation said it was applied before the demux pipeline is built. The Iso and Session arms both apply it; Live now does the same, in the same place — before resolve_inline_base_map, which is keyed on extents and so unaffected by pruning the stream list. The header gate returned Error::MkvInvalid whenever headers had not resolved. On the prefetch-highway path a halt landing while the pump is blocked in a read can end the stream as Ok(None) rather than Err(Halted), so the loop breaks with headers unresolved through no fault of the data. Reporting that as MkvInvalid tells the consumer its disc is malformed and skips the stop-preserves-staging path that a clean completed=false triggers. The gate now re-checks halt first. MuxOptions.selection's doc claimed it was applied without naming the one input it is not applied to. That exception lived only in an internal comment at the Url match arm, where a caller reading the public field docs would never see it. It is now on the field, pointing at InputOptions::selection instead. |
||
|
Matthew Jackson
|
c0434e87de |
Fix the DVD MPEG-audio codec mapping and the fabricated AACS docs
parse_audio_attr mapped DVD audio_coding_mode 2 to Codec::Mpeg1 — the MPEG-1
VIDEO variant. Codec::kind() reports Video for it, so a DVD MPEG-audio stream
was classified and handled as video everywhere downstream. Modes 2 and 3 are
both MPEG audio Layer II (3 adds the MPEG-2 multichannel extension), so both
map to Codec::Mp2. A test now walks every coding mode and asserts each result's
kind() is Audio, so no mode can map to a non-audio codec again.
docs/aacs.md documented an entire keydb-resolving API that does not exist:
ScanOptions::with_keydb, Disc::open_title, reader.read_unit(). None of those
symbols appear anywhere in the crate, and ScanOptions has no keydb field — its
own doc comment says "libfreemkv is lookup-free — it resolves no keys". A
reader following that page would conclude the library reads keydb.cfg, which
inverts the actual design: the caller resolves keys out-of-band through a
KeySource and applies them with Disc::decrypt_with.
The section is rewritten against the real API, and the AacsState table's
`key_source` type corrected from KeySource to KeyOrigin.
Worth recording: the first replacement example I wrote was itself wrong. It
used `input("disc://...")`, which resolve.rs explicitly rejects with
Error::DiscUrlNotDirect — live disc must go through Drive::open + Disc::scan +
DiscStream::new. Every symbol and signature in the committed example was
checked against the source rather than assumed.
|
||
|
Matthew Jackson
|
ec5cd31ae1 |
Stop Mp4Sink losing audio frames and writing an empty sample entry
Mp4Sink::write returned Ok(()) without recording the sample whenever an audio track's frame would not parse into a sample entry. Two consequences, both silent: leading audio frames were lost until one frame parsed, and a track whose frames never parsed disappeared from the output entirely — finish()'s retain() removed the sample-less trak and the run reported success. That contradicts this crate's stated policy that a skipped track is never silently dropped. The drop was never necessary. audio_entry is read in exactly one place, build_trak, reached only from build_moov inside finish() — nothing on the write path consumes it. So write() now records every sample and derives the entry opportunistically from whichever frame parses first. That makes build_trak's `audio_entry.unwrap_or_default()` reachable, which would emit an stsd declaring entry_count=1 around an EMPTY sample entry: a structurally invalid mp4 returned as success. finish() therefore drops any audio track it cannot describe, with a tracing::warn! naming the codec and sample count. Dropping rather than erroring is deliberate. It matches finish()'s existing treatment of sample-less tracks, keeps an export whose video is fine from failing outright, and needs no new error code — a new code would mean a new i18n key across 29 locale files in another repo, which is not this change's scope. The track's bytes stay unreferenced in mdat: wasted space in a valid file, which is the cheaper failure. Test pins both halves — moov describes only the video track, and the unparseable audio bytes still reach mdat rather than being discarded at write time. |
||
|
Matthew Jackson
|
a1304f9e78 |
Stop the mp4 demuxer dropping tracks silently or inventing sample offsets
Three defects in the mp4:// read path, all of the same family: a damaged source was remuxed minus a track, or with fabricated data, and the run reported success. Silent drops. Eight paths dropped a whole track on malformed input with no report of any kind, so an mp4:// source missing its audio looked like a clean run. Each now emits a tracing::warn! naming the track and the missing or inconsistent table (tracing English is permitted in this crate; the numeric error codes are unchanged). The non-A/V handler case is debug!, since skipping a timecode or hint track is normal. Fabricated offsets. sample_offsets ended with a `while offsets.len() < sizes.len()` loop that packed unplaced samples after the last known offset. Those samples have no known location, so the invented offsets made the reader pull frame data from arbitrary file bytes — the exact "emit garbage" outcome the stco/stsc presence guards refuse. It now returns the short list and the caller drops the track. Short stts. `durations.get(i).unwrap_or(0)` gave every sample past the end of a short stts a duration of 0, collapsing the whole tail onto one timestamp. That is the same degenerate timing the `durations.is_empty()` guard was written to refuse, so the guard now refuses both cases. Two shared test fixtures were internally inconsistent and only passed because the reader was lenient: stsz declared 3 samples while stsc placed 1, and the hostile-stsz fixture's stsc/stts covered a single sample. Both are now consistent. The hostile fixture keeps its lying stsz count — that lie is what it tests — but its stsc and stts now cover whatever count survives the file_len bound, so it exercises the allocation bound rather than the inconsistency guards. Two new tests pin the new refusals by mutating the consistent fixture: an stsc that places 1 of 3 samples, and an stts that covers 1 of 3. |
||
|
Matthew Jackson
|
a39045adf1 |
Bound the forced-subtitle probe, make it cancellable, and stop re-reading clips
The probe's only natural exit was "every PGS track has shown a non-forced
display set". A genuinely FORCED track never satisfies that, so on the common
authoring — a forced-narrative track for foreign dialogue — the loop read the
title's entire extent set at 2 MiB per call with no byte cap, no time cap and
no halt check. It was also invoked once per title rather than once per distinct
clip, and a disc's playlists overwhelmingly reference the same few clips (main
feature, play-all, seamless-branch variants), so the same physical extents were
re-read 30-150 times. The two defects multiplied: tens of GB, times the
playlist count, off an optical drive.
Reached via ScanOptions::probe_forced_subtitles, whose only consumer is
`freemkv info -v` (freemkv/src/disc_info.rs). The rip path leaves it off. So
the symptom is `info -v` never returning on an ordinary UHD, not a corrupt rip.
Three changes:
* PROBE_BUDGET_SECTORS caps a probe at 256 MiB. A forced track's display sets
appear throughout the title, so a bounded prefix classifies it; the budget
only decides how long we keep looking for a non-forced set before accepting
the forced verdict.
* ScanOptions::halt is now plumbed in and checked per chunk, so `info -v` is
cancellable. The probe previously took no halt at all.
* A ForcedProbeCache memoises verdicts against the title's exact extent list.
Keying on byte-identical input means a hit cannot change any result — it
only removes the re-read.
Verdict application is factored into apply_verdicts so the cached and freshly
probed paths cannot diverge.
Three tests pin the behaviour, each against a reader that counts sectors and
never ends: the budget stops at exactly PROBE_BUDGET_SECTORS, a cancelled halt
reads zero sectors, and a second title with identical extents costs no further
reads while a different extent list still misses the cache.
Also worth recording: the CLI acceptance harness never exercised `info -v`
against an optical drive — it reads ISOs from local SSD, where a full-extent
read is fast enough to hide both defects.
|
||
|
Matthew Jackson
|
ea72e6df5f |
Give every audio and video codec its own registered Matroska CodecID
MkvTrack::audio's catch-all was `_ => CODEC_AC3`, and ebml.rs defined no A_AAC, A_MPEG/L2, A_MPEG/L3, A_FLAC or A_OPUS constant at all. A CodecID names the payload, so any of those codecs was written into the MKV declaring AC-3 while carrying something else — a player either refuses the track or decodes noise. Reachable through two ordinary paths, both verified: ifo.rs:577 maps DVD audio_coding_mode 3 to Codec::Mp2, so a DVD with MPEG audio muxed to mkv:// produced a track declaring A_AC3 over MP2 bytes; and mp4/read.rs:479 maps the `mp4a` sample entry to Codec::Aac for an mp4:// source. codec/mod.rs already has working parsers for MP2, MP3, AAC, FLAC and Opus, so the pipeline carried these codecs end to end and only the container label was wrong. MkvTrack::video had the same shape: `_ => CODEC_MPEG2` announced Codec::Mpeg1 and Codec::Av1 as MPEG-2 video. V_MPEG1 and V_AV1 added. Both catch-alls stay, because MkvTrack::audio/video return Self and have no error channel, but they are now reachable only by a non-audio/non-video or Unknown codec routed there in error. Two tests enumerate every real codec of each kind and cross-check each against Codec::kind(), so a codec added to the enum later cannot silently inherit another codec's ID. Both proven red first: Aac declared A_AC3 before the fix. |
||
|
Matthew Jackson
|
b2c7490b5b |
Parse H.264 parameter sets with the avcC parser, not the hvcC one
TsMuxer::write_frame handed every NAL video track's codec_private to hvcc_to_annex_b. An H.264 track carries an avcC record, whose box layout is different, so the parser returned None and no parameter sets were emitted — and params_written was set unconditionally, so it never retried. H.264 muxed to m2ts:// reached the player with no SPS/PPS and was undecodable, silently: frame_count still advanced and the mux reported success. AVC is the dominant Blu-ray video codec, so this was not an edge case. The correct dispatch already existed and was already used by demux_sink::annexb_param_sets. tsmux simply never got the codec: it knew only PIDs and a NAL-or-not bool, so it could not tell hvcC from avcC. Rather than add a second setter, set_nal_video(track, bool) becomes set_video_codec(track, Codec). One fact decides both the ES framing and the parameter-set parser, so the two can no longer disagree — and that disagreement is precisely this defect. The default stays Codec::Hevc, which is the behaviour the bool's `true` default encoded, so a caller that never calls it is unaffected. Proven red first, end-to-end through M2tsStream::create with a real avcC record: before the fix neither the SPS nor the PPS reached the transport stream. |
||
|
Matthew Jackson
|
3db4106253 |
Stop documenting the recovery API that 1.6.0 deleted
Disc::sweep, Disc::patch, Disc::copy, SweepOptions and PatchOptions have zero occurrences in src/ — recovery moved to freemkv-engine — but they were still documented in 30 places across README.md, TROUBLESHOOTING.md, six files under docs/, seven src/ doc comments and a Cargo.toml comment. README.md is the crate's GitHub front page and carried a full multi-pass code example that cannot compile. Two of the src/ references were intra-doc LINKS to deleted items ([`disc::Disc::copy`], [`disc::Disc::patch`] in scsi/mod.rs). They produced no warning on a normal `cargo doc` only because they sit on pub(crate) items; `--document-private-items` reports both, and they are gone now. The README example is deleted rather than rewritten against the engine's API: libfreemkv documenting a downstream crate's API on its own front page is the drift that produced this, and it cannot even depend on it. The src/ references become plain code spans naming freemkv_engine::recovery::* — deliberately not links, for the same reason. docs/rip-recovery.md was 202 lines about relocated code. It now documents only what this crate owns — Drive::read, SenseFamily, DiscStream's adaptive batch halving — plus the read-path design constraints, which belong with the code that enforces them, and points at freemkv-engine/src/recovery/ for the strategy. api-design.md's module tree is regenerated from the real src/disc/ and src/drive/ layouts instead of hand-patched; it had listed sweep.rs, patch.rs, mapfile.rs and read_error.rs, none of which exist. Three stale facts surfaced while rewriting and are corrected: the read timeouts are 10 s / 60 s, not the documented 1.5 s / 30 s; Drive::reset and SgIoTransport::reset no longer exist at all, so "no SCSI reset from any read path" is now stated as the stronger fact it has become; and verify_title, listed as a progress-emitting operation, was removed entirely. CHANGELOG.md keeps its references — those are the historical record of the releases that shipped the API. |
||
|
Matthew Jackson
|
d34979ac57 |
Assert ReferenceBlock per block instead of scanning for the byte 0xFB
Two tests checked only find_id(&data, ebml::REFERENCE_BLOCK).is_some(). ReferenceBlock's EBML ID is the single byte 0xFB, so that asks whether one exists ANYWHERE in the output — not which blocks carry one. Inside a BlockGroup, keyframe-ness is signalled by the ABSENCE of ReferenceBlock, so "which" is the entire question. Measured rather than assumed. A mutant emitting a ReferenceBlock on every BlockGroup, keyframes included — which reintroduces the exact 1.6.0 defect these tests were added for, every frame reading as a non-keyframe — passed mvc_frame_emits_blockgroup_additional_and_reference untouched. The duration-bearing roundtrip test did catch it, via its MkvStream read-back rather than via the presence check. A mutant corrupting the offset VALUE was invisible to both. all_block_groups() parses every BlockGroup in emission order with its Block flags, relative timestamp, duration and decoded signed ReferenceBlock. first_block_group() now delegates to it, so there is one walker rather than two. Both tests assert per block: the keyframe carries NO ReferenceBlock, each non-keyframe carries one, the offset equals the distance back to the keyframe, and the SimpleBlock-only 0x80 flag is clear throughout. Both tests previously took muxer.writer.into_inner() WITHOUT calling finish(), leaving cluster sizes unwritten — an unparseable file, which is why they could only byte-scan in the first place. They now write through SharedWriter and finish(), so the assertions run against a well-formed Matroska file. Verified: with the fixes in place, the every-block mutant and the off-by-one-offset mutant both fail. |
||
|
Matthew Jackson
|
bf2d15f39c |
Cover the non-NAL video path, including the wiring that selects it
set_nal_video had exactly one production caller and zero test callers. The
branch it gates decides whether a video track's ES goes through
length_prefixed_to_annex_b, and MPEG-2 and VC-1 must not: they are already
start-code ES. Getting it wrong is silent — frame_count still increments,
so the mux reports success while emitting a video-less file.
Five tests, each verified against a real mutant:
* non-NAL ES passes through byte-for-byte, and the default path still
converts. Both use a deliberately length-prefix-SHAPED payload so a
wrongly-applied conversion rewrites the leading four bytes into a start
code — a payload the converter happened to leave alone would let a
mutant pass.
* a non-NAL keyframe arms params_written, so following non-keyframes are
not dropped by the pre-keyframe guard.
* set_nal_video rejects an out-of-range track instead of panicking.
* M2tsStream::create wires a VC-1 track to the non-NAL path.
That last one matters more than it looks. The four TsMuxer-level tests set
the flag themselves, so deleting the set_nal_video loop from
M2tsStream::create left all 2366 tests passing — the exact mutant the
finding named went undetected until a test drove the real wiring. It now
also catches the subtler mutant of widening the matches! arm to include
Vc1.
|
||
|
Matthew Jackson
|
d09ed76e07 |
Promote AACS unit encryption to real API, and assert decrypt byte-exactly
encrypt_unit becomes public library API rather than a #[cfg(test)] helper.
Authoring an encrypted disc image is a legitimate use of this crate, and
the capability was already written four times over: a pub(crate) test-only
copy in aacs/content.rs plus three hand-rolled duplicates in decrypt.rs,
sector/decrypting.rs and disc/extract.rs. All four now call one function,
removing ~110 lines of duplicated cipher code that could drift from
decrypt_unit independently.
It mirrors decrypt_unit's purity contract: crypto only, no encrypted-flag
handling, because where that flag lives is container-specific (CPI bits in
byte 0 for BD-TS, elsewhere for HD-DVD-PS). Callers set the flag BEFORE
encrypting — bytes 0..16 are the key seed left in plaintext, so touching a
header byte afterwards changes the key a decryptor derives. That footgun is
documented at the function and at every call site.
Two tests pin it: an exact round trip through both directions, and the one
place the pair is deliberately asymmetric — decrypt_unit restores
all-zero-on-disc packets to zero, and the test proves an all-zero plaintext
packet enciphers to non-zero bytes so it is never mistaken for padding.
That asymmetry was previously only prose.
Two decrypt tests were also weaker than their own names:
* aacs_clear_trailing_partial_passes_through asserted only is_ok(), so a
mutant corrupting the clear partial while returning Ok passed. It now
snapshots the buffer and asserts byte equality, matching the
none_keys_is_noop pattern already in the file.
* aacs_decorator_decrypts_encrypted_unit_via_map checked only that 0x47
reappeared at the 192-byte stride, leaving corruption in the other 6112
bytes undetected. The plaintext is fully known, so it now asserts
byte-exact recovery against it.
Both were verified red first by mutating the production path.
|
||
|
Matthew Jackson
|
f76688a0dc |
Make the ddts speaker mask agree with its own channel count
The ddts box declares both a channel count and a 16-bit speaker mask, and a decoder may trust either. dts_channel_layout ended in a `_ => 0x0007` catch-all describing five speakers (C + L/R + Ls/Rs), so for AMODE 6, 7, and 10 through 15 the box contradicted the count DTS_AMODE_CH declared alongside it — provoking a downmix or an outright decode error. AMODE 6 (L + R + S) is the reachable case: its S is a single centre-surround, not the Ls/Rs pair, so it is three channels and 0x0012, not four and 0x0006. AMODE 13/14/15 do not occur on retail media, which ships a 5.1 AMODE-9 core plus an extension substream. Replace the catch-all with the full 16-entry ETSI TS 102 114 mask table. Every entry is cross-checked against DTS_AMODE_CH by counting the speakers its bits imply: sixteen independent constraints, all satisfied, and that table is itself pinned to the per-AMODE channel counts in ETSI TS 102 114. AMODE is a 6-bit field, so the reserved 16..=63 are reachable from a malformed stream. The old `unwrap_or(6)` invented a channel count for them that no mask could match; parse_dts now refuses those frames rather than guessing a layout it cannot name. One existing fixture placed the ext-sync pattern at f[4..8], which incidentally set f[7]=0x25 → AMODE 20. That test is about where the pattern sits, not about AMODE, so the frame is now spec-legal (AMODE 9, 48 kHz) with the pattern moved into the payload proper. |
||
|
Matthew Jackson
|
840cb9aef0 |
Drop the comment that promised tests this crate no longer has
The bisect ReadAction regression tests moved to freemkv-engine with the recovery strategy, but their explanatory block stayed behind — seventeen lines describing a `let _ = handle_read_error(..)` bug and asserting "the tests below prove the required ReadAction values are produced". There are no tests below it; handle_read_error is not even resolvable here any more. Anyone auditing whether that bug is still guarded would read this and conclude yes. The block moved to the engine alongside the tests it describes. |
||
|
Matthew Jackson
|
f3e80c8499 |
Route conduct reports through GitHub instead of a private address
The enforcement contact was an address on a domain used for internal infrastructure, which does not belong in a public repository. GitHub needs no mailbox to exist and is reachable for anyone who can read this file. |
||
|
Matthew Jackson
|
c812a32f3d |
mux: fix keyframe signalling for BlockGroup frames
Inside a Matroska BlockGroup the SimpleBlock 0x80 keyframe bit is
reserved and is always written as 0; keyframe-ness is carried only by
the presence or absence of a ReferenceBlock child. Both halves of the
round-trip got this wrong:
- the reader skipped past ReferenceBlock and read the reserved bit,
so every BlockGroup frame came back as a non-keyframe;
- write_block_group discarded its `keyframe` argument and never
emitted a ReferenceBlock, on the assumption that only intra frames
(PGS subtitles) reached that path.
The MPEG-2 parser stamps a per-frame duration on I, P and B pictures
alike, so all MPEG-2 video is written as a BlockGroup. That makes the
assumption false and left no video frame looking like a keyframe on
read-back. Downstream, mkv:// -> m2ts:// dropped every video frame (the
TS muxer discards non-key video until the first keyframe) while still
reporting success, and mkv:// -> mkv:// and the stdio round-trip failed
E6008, because the MKV muxer opens a cluster only on a track-0 video
keyframe and so wrote nothing at all. HEVC was unaffected: it carries no
per-frame duration, so it takes the SimpleBlock path where the flag bit
is authoritative.
Verified on a real CSS DVD: 841 keyframes out of 11440 video packets
survive a re-mux, matching the I-picture count in the source bitstream.
Also stop running non-NAL video through the Annex-B converter. MPEG-2
and VC-1 elementary streams are already start-code framed, so
length-prefix conversion corrupts them. TsMuxer takes a per-track
nal_video flag, defaulting to the previous behaviour, which M2tsStream
sets from each video stream's codec.
|
||
|
Matthew Jackson
|
eedd27e352 |
mux: document that the Url arm selects via InputOptions.selection
The Url mux path prunes streams inside input() via InputOptions.selection; MuxOptions.selection only applies on the File/Session arms. A Url-source caller must set InputOptions.selection — noting it so a future caller does not put the selection on MuxOptions and silently keep every track (the bug the GUI hit). |
||
|
Matthew Jackson
|
d8e5b97c86 |
1.6.0: remove recovery strategy (moved to freemkv-engine) + trim dead surface
The sweep/patch recovery strategy, mapfile, retry-decision state machine,
section-recover, and damage classification move out of libfreemkv into the
new freemkv-engine crate. libfreemkv keeps the raw single-shot read and
SCSI-fact translation (SenseFamily stays in scsi).
- Delete disc/{sweep,patch,mapfile,read_error,section_recover}.rs, the
Disc::copy/sweep/patch methods, the Copy/Sweep/Patch option+result types,
classify_damage/DamageSeverity, progress_snapshot_from_mapfile, and the
three recovery integration tests.
- Trim public surface the recovery deletion orphaned: delete the dead
READ_PIPELINE_DEPTH const, the write-side SectorSink/FileSectorSink (no
consumer), and the DriveSpeed enum (its one live use — set max drive
speed — becomes Drive::SPEED_MAX_KBPS). Make mapfile_path_for,
decrypt_sectors_mapped pub(crate); gate NoopEvents to test.
- Version 1.6.0.
|
||
|
MattJackson
|
0151e199ef | changelog: 1.6.0 section (layering API, mux fixes, engine relocation, stream selection); date 1.5.2 | ||
|
MattJackson
|
ea99c82e32 |
mux: end-to-end test that stream selection drops excluded-PID frames
A title declaring two audio PIDs, pruned to one via StreamSelection::apply before build_iso_pipeline, must never surface a frame from the excluded PID. Proves the declaration-driven seam end-to-end through the full highway (read -> demux -> codec parse): the demuxer is built from the pruned title.streams, so the excluded PID is untracked and skipped, and both track headers and frames follow the pruned list. Builds on the existing synthetic-TS harness. |
||
|
MattJackson
|
8822c29905 |
disc: add DiscTitle audio_streams/subtitle_streams/video_streams accessors
Typed iterators over the audio/subtitle/video streams, cleaner than matching on the Stream enum for the common iterate-the-tracks case (stream selection, the desktop UI info panel, disc-info). Additive; all lib tests pass on 1.86. |
||
|
MattJackson
|
0ee8341aea |
mux: StreamSelection primitive + apply sites for per-title stream selection
The demux pipeline is declaration-driven off DiscTitle.streams (build_demux_state,
DiscStream::new, and the MKV writer all key off that list), so 'which streams to
keep' is already a pipeline capability with no public knob. This adds the knob:
- mux/select.rs: StreamSelection { audio, subtitle: PidFilter::All | Only(Vec<u16>) }
+ apply(&mut DiscTitle): keep Video always, keep Audio/Subtitle whose PID the
filter lists, prune the rest (and the parallel codec_privates in lockstep);
error SelectionPidUnknown on a listed PID absent from the title (fail loud, not
a silently-missing track). Pure; 6 unit tests. Re-exported at crate root.
- Error::SelectionPidUnknown (E6014).
- MuxOptions gains (+ derives Default now) applied in mux_stream's
Iso/Session arms before the highway/DiscStream builds demux state (and before
probe_and_remap's DVD AC-3 PID rewrite). InputOptions gains applied
in input()'s iso arm right after the title-index bounds check.
PIDs not languages -- language->PID is caller/engine policy. Default All/All is a
no-op (apply gated on !is_all()), so the no-selection path is byte-identical:
nothing below the title-finalization line changes (ts/ps/demux_thread/
pipelined_stream/mkv/disc untouched). All 2488 lib tests pass on 1.86.
|
||
|
MattJackson
|
d22d09c898 |
error: add is_disc_level_no_key classifier (re-exported)
Distinguishes a WHOLE-DISC key failure (E_NO_DISC_KEY / E_KEYDB_LOAD / E_AACS_NO_KEYS -- every title fails identically) from a per-title skippable stub. The engine's multi-title loop uses it to fail-fast on the first no-key title instead of iterating all N. Additive. |
||
|
MattJackson
|
43564d5752 |
error: re-export is_halt + is_skippable_title_stub at crate root
The engine's multi-title rip loop classifies per-title mux failures (halt vs skippable stub vs hard) using these typed classifiers instead of E-code string matching. Additive; no behavior change. |
||
|
MattJackson
|
508a2c3376 |
disc: promote locate_ranges to pub (engine multipass reads it)
Small pub promotion + fmt one-lining. The relocated multipass progress reporting in freemkv-engine needs locate_ranges externally. No behavior change. |
||
|
MattJackson
|
45a16991ff |
drive: promote extract_scsi_context to pub
Small pure error->(status,sense) introspection helper the relocated sweep/patch will need externally. Same category as the prior WritebackFile/ resolve_content_key_map promotions -- infra, not policy. No behavior change. |
||
|
MattJackson
|
ca0daaea07 |
io: promote WritebackFile to pub
Bounded-cache buffered File replacement used across mux/extract/sweep/patch -- general I/O infrastructure, not recovery policy. freemkv-engine's relocated sweep/patch need to construct it directly once those methods leave this crate. No behavior change. |
||
|
MattJackson
|
a02bbbdd24 |
scsi: promote SenseFamily to a lib-level SCSI-fact primitive
Moved SenseFamily::from_sense_key + is_wedge_family from disc/read_error.rs into scsi/mod.rs (with its own tests) and re-exported at the crate root. This is pure SCSI sense-code classification -- objective hardware fact, zero recovery-policy opinion -- so it belongs in the library primitives, unlike the retry-DECISION state machine (ReadCtx/PassSummary/ReadAction/ handle_read_error) built on top of it, which is freemkv's specific recovery strategy and is moving to freemkv-engine next. disc/read_error.rs and disc/section_recover.rs now import SenseFamily from crate::scsi instead of defining/re-exporting their own copy. No behavior change. Precommit green on Rust 1.86 (fmt+clippy+test). |
||
|
MattJackson
|
ba8114f29b |
disc: promote resolve_content_key_map + encrypted_content_ranges to pub
The upcoming freemkv-engine crate needs both to build the multipass sweep/patch recovery strategy externally over Disc's public API. Everything else sweep/patch touch on Disc was already pub; these were the only two gaps. No behavior change -- visibility only. |
||
|
Matthew Jackson
|
8421c227cd |
mux: fix DTS core-header false-drops + close TrueHD/mux gate coverage
DTS core decodability gate (core_header_drop_reason) — full ETSI TS 102 114
spec-conformance sweep against a reference decoder the spec core-header rules and
a reference decoder parse_frame_header:
- deficit_samples: only require ==32 for NORMAL frames (FTYPE==1). A
TERMINATION frame (FTYPE==0, the last frame of a stream) legitimately
carries fewer and is fully decodable; the old unconditional check dropped
it on every stream that ends on one — a guaranteed per-track silence gap.
Matches a reference decoder (normal_frame && deficit != DCA_PCMBLOCK_SAMPLES) and
a reference decoder (branches on normal_frame).
- reserved bit (after RATE): both reference decoders SKIP it (a reference decoder
skip_bits1, a reference decoder bits_skip1 "Reserved field") and never reject on it.
Rejecting was a false-drop that silenced any real stream whose encoder
set the bit. Relaxed to read-and-discard; DropReason::ReservedBit removed.
Swept and confirmed spec-correct as-is (no change): npcmblocks multiple-of-8,
frame_size>=96, audio_mode>=16 (a reference decoder-permissive), sample-rate validity
table (matches avpriv_dca_sample_rates incl 96k/192k at 14/15), LFE flag==3
invalid, PCMR bits table (matches a reference decoder sample_res {16,16,20,20,0,24,24,0}).
Bit-read order verified field-by-field against a reference decoder. bit_rate is left
unvalidated (lenient, never-false-drop direction) as before.
Tests: termination frame with small deficit is kept; normal frame with bad
deficit is dropped; reserved-bit-set frame is kept. make_bad_dts_core now
uses an invalid LFE flag (duration-neutral) instead of the relaxed reserved
bit.
TrueHD: add coverage for the EXTENDED major-sync header CRC path (ms[25]&1,
mshdr=28+2+2n) — previously zero-tested, the exact path a shipped endianness
bug once used to silently drop whole 7.1/Atmos tracks. Trailer is an
independently-computed oracle (separate CRC-16/0x2D, anchored to the 0x4FF7
catalogue value, NOT crc16_mlp), stored little-endian; test asserts accept,
body-corruption reject, and big-endian-trailer reject.
mux driver: extract the finish completion mapping into pure mux_run_completed
so the finalize_failed -> completed=false branch (reachable only via real
write-thread wedge timing) is unit-tested; add an out-of-range
MuxInput::Session title_index test asserting a clean Error::MuxTrackRange
(E9011) instead of a panic.
|
||
|
Matthew Jackson
|
b79ff71b43 |
Fix read-fault misclassification, DTS AMODE channel table, and untestable guards
- resolve_fmts_key_map: distinguish a genuinely-not-FMTS disc from a
transient live-drive read fault. read_filesystem now returns the new
Error::UdfNotFilesystem for a deterministic tag/format mismatch (no AVDP,
no partition descriptor, no FSD); resolve maps only UdfNotFilesystem (fs)
and UdfNotFound (.tbl absent) to Ok(None), and PROPAGATES DiscRead / other
I/O faults so a marginal AACS 2.1 disc fails loud instead of silently
dropping forensic content under a base-Unit-Key-only map.
- DTS_AMODE_CH (mp4/audio.rs): extend 10→16 entries
{1,2,2,2,2,3,3,4,4,5,6,6,6,7,8,8} (the spec per-AMODE channel table / ETSI TS 102 114) so
the spec-legal high AMODEs that now pass the decodability gate declare
their true channelcount (AMODE 13→7, 14/15→8) instead of a truncated 6.
- session.rs resolve_keys "called before scan" guard is now testable:
from_parts_for_test takes Option<Disc>; added a test that a disc-less
session returns a clean DeviceNotReady Err rather than panicking.
- mp4/read.rs: a track with samples but a missing/malformed stts (mandatory
per ISO/IEC 14496-12) is dropped rather than emitting all-zero timestamps,
matching the existing stco/stsc guards; all-tracks-dropped → Mp4Invalid.
- Remove the inert MuxInput::Iso.key_map field (the Iso path re-derives its
map inside build_iso_pipeline); the live path keeps Live.key_map.
All four fixes are mutation-verified.
|
||
|
Matthew Jackson
|
9b3e281f4d |
mux: distinguish FMTS phase-probe read fault from wrong key; cover multi-CPS
Fix 1 (correctness): resolve_fmts_key_map's per-index phase probe read a single representative segment via read_unit (whose read_sectors(...).ok()? swallows read errors into None) with no fault fallback. A transient live-drive read fault (e.g. NOT READY 2/04/3E on the BU40N) made every probe read return None, giving even==0 && odd==0 — indistinguishable from a genuine wrong key — so resolve_tie_phase returned FmtsKeyMissing and aborted the entire multi-title rip even though the forensic index keys were valid and in hand. Extract the probe into probe_index_phase, which returns Phase / WrongKey / ReadFault. It mirrors the anchor loop's tolerance: it tries multiple same-index segments and only concludes WrongKey once a read actually succeeded and decrypted to no clean parity. If every read of every same-index segment faults it returns ReadFault; the caller then leaves the phase unresolved so the range-builder defaults to Phase::All (decrypt both parities, demux drops the garbled alternate half) instead of aborting. A wrong key can never be masked as a read fault: ReadFault requires that not a single read succeeded, so zero decrypt evidence. Fix 2 (test coverage): resolve_mux_key_map's multi-CPS branch was only exercised with an all-zero source, so pick(), the KeyFetch cold path, and the fail-loud DecryptFailed guard never ran. Add tests over real AACS ciphertext (via aacs_encrypt_unit_for_test) covering pick() selecting the correct pool index, the DecryptFailed guard firing when a clean sample matches no held/fetched key, and the fetch cold path recovering a missing unit key. Mutation-verified. Fix 3 (doc): move the reader_event_fn EventKind->MuxEvents mapping doc off session_mux_keys onto reader_event_fn. |