The same scrub as the previous commit, over the files it did not reach:
the variant-MKB layout notes, the 2.1 segment index observations, the
PPS-revert regressions and the playlist-twin tiebreak.
Measurements keep their numbers — "a v70 `0x2d` body = 46_100*2 + 16"
is the useful part, and the title it came from never was.
A forced verdict is an absence claim, and a sampled run sees a fraction
of a track. Measured on real discs: tracks exist that flag about a
quarter of their display sets and leave the rest unflagged, so a sample
that catches one flagged set and nothing else promotes a full dialogue
track to forced -- which players then force on screen. A sampled run now
needs two display sets; a run that read every extent end to end has no
unread gap and may still promote off one.
Fifteen references across six files named the discs a defect was first
seen on. The parser leak found earlier was not an isolated slip — the
same habit runs through the mux comments, the changelog and the AACS
content verdict, where a title name was standing in for the shape of
the problem.
Every one is replaced with the property that actually mattered: a
multi-clip title, a UHD Dolby Vision profile 7 dual-layer stream, a
disc carrying an authored-bad TS packet. The comments are more useful
for it — the reader needs to recognise the shape on a disc they have,
not the one we happened to have.
`SEG_MainFeature` stays: the parser matches on that literal, so it is
a format token rather than a title.
Measured on a real title: the same 256 MiB budget cut into sixteen
windows per extent took 41.8s against the head-first read's 6.6s, because
every jump collapsed the source's read batching into three-sector calls.
Expected observations depend on total bytes read, not on how finely they
are cut, and measured subtitle density (a display set every ~30-50 MB of
clip) makes a 32 MiB window about even money on its own.
The pixelogic walk finds the feature playlist's section by name and ends
it at the next `SEG_`/`SF_`/`FPL_` marker. Those markers are section
NAMES, and a project's trailing sections — the per-language notice,
disclaimer and dub-credit cards — carry none. On 8 of the 11
affected-format discs in the corpus the feature playlist is the last
NAMED section in the blob, so the terminator never fires and the walk
consumes the whole tail of the file as more of the feature's stream
list.
The card names are `{lang3}_{card}`, which passes `is_stream_token`, so
each one advances an STN counter, and a card whose name collides with a
catalogued component emits a label outright. Measured on the worst disc:
95 entries past the end of a 9-audio/21-PG list, five phantom audio
labels at STN 10-14 from `*_AC` notice cards (`AC` reads as the AC-3
codec), and 94 uncatalogued-component occurrences — which also took the
parse from High to Medium confidence and fired the vocabulary-gap
warning on four components that are deliberately not catalogued. A
second disc fabricated one subtitle label from a token in a following
playlist section named `FP_SingAlong`, which `FPL_` does not match.
What every section has, named or not, is a stream list that opens with
its video slots. So a `Video Stream N` entry repeating one this section
already listed is the first entry of the NEXT section, and ends this
one. Distinct video entries are kept, since a section may legitimately
list a secondary video stream; the memo of them is bounded at the BD STN
table's ceiling so disc bytes cannot grow it.
Replaying all 11 blobs through `assign_labels` before and after: the two
discs above lose exactly their phantom labels (11→6 and 5→4), the other
nine are byte-identical.
One residue is pinned rather than papered over: a card's name precedes
its own section's video slot, so a forward-only walk can still count the
FIRST card after the last real slot. It sits at the tail of a list
nothing follows in, so it can renumber nothing — at worst it costs a
parse its High confidence.
No other parser in src/labels/ walks a flat entry sequence with a
terminator set; the rest scope each stream to a structural range or read
its number off the entry itself. paramount and criterion gain immunity
pins for the boundary property specifically: a stream list cannot run
into the next element's, and a missing element boundary shortens the
list rather than extending it.
A track that flags some of its own display sets and not others proves the
authoring house makes the distinction, so it needs no sibling to
corroborate the flag being in use -- but it still has to look like a full
dialogue track before a forced label is cleared. A small track with a
couple of flagged signs is a forced track, and demoting it is the mistake
the shape test exists to prevent.
A title cut into 50-odd clips gives each extent one window's worth of
budget. At the extent's head, every clip is sampled at the same relative
position and the first clip's window lands on the opening of the feature
-- the one stretch with no subtitles in it.
Two halves of the same subtitle-labelling bug class. The numbering half
landed already: an entry the parser could not parse still occupies an STN
slot, so skipping it shifted every later label onto the wrong stream. This
is the other half — an entry the parser counts correctly but cannot
INTERPRET.
`DUB` names the forced-narrative subtitle track authored to accompany a
language's dubbed audio presentation: the signs and on-screen-text pass a
viewer still needs once the dialogue itself is dubbed. It is the same
editorial class as `*_TXT_FOR_`, spelled differently by some authoring
runs. Uncatalogued, it matched no component arm, so the token signalled
neither audio nor subtitle and the whole stream record was dropped at the
domain guard — a genuine forced track left with no forced qualifier even
after the numbering was right.
Evidence, from two independent discs in the corpus: the token appears only
inside the PG list, embedded in an otherwise contiguous run of
`{lang}[_{region}]_TXT_FOR_` siblings — one forced-narrative slot per
localized language — and takes exactly the slot where that language's
forced entry belongs. Both discs also carry that language's FULL subtitle
track as a separate, separately-labelled slot, so the DUB entry is not it.
The stream it lands on is a sparse PG track, the signature of a forced
pass rather than full dialogue. Deliberately not added to vocab::qualifier:
that maps free-form English label text, where a bare "dub" means dubbed
AUDIO. The forced-subtitle reading is specific to this token grammar.
The corpus sweep that found it also produced four components that are
deliberately NOT catalogued. They are per-language notice and disclaimer
clip names that merely collide with the `{lang3}_{component}` token shape;
each occurrence sits in a one-video-stream section next to the disclaimer
entry it names. They carry no editorial meaning, and mapping them would
attach a qualifier to a stream on the strength of a filename.
Which is also why an unmapped component now reports once per parse rather
than once per occurrence. It was a debug line nobody reads, and that is how
this gap survived to a user complaint; but a per-occurrence warn would bury
the signal under dozens of routine collisions on an ordinary disc. One
bounded, deduplicated line names the distinct components and says plainly
that any forced/SDH/commentary meaning they carry went unapplied. The
backing set is capped and truncates by chars, not bytes — the components
come from untrusted disc bytes, and a byte-offset slice can split a
multi-byte sequence and panic.
Two existing tests encoded the wrong behaviour and are corrected: the STN
numbering test expected the forced run to skip the DUB slot, and the
unclassifiable-slot test used DUB as its example of a token with no
meaning. The latter now uses a genuinely uncatalogued component.
A window's last PES stays open in the demuxer until the next PUSI, which
under sampling is in a different window or nowhere — so the last display
set of every window was discarded, worst where the sample is thinnest.
Flush the demuxer at the end of each window.
A playlist that lists the same clip twice now merges the second read into
that extent's memo (facts OR'd, display count MAXed, coverage the larger
of the two) instead of overwriting it.
The content-based forced-subtitle probe spent its whole 256 MiB budget
on the first sectors of a title. A feature's subtitles begin minutes in,
so the probe read the opening logos, hit the budget, observed no display
set at all and contributed nothing to any verdict — the vendor label was
always the only input.
The forced predicate is asymmetric: one non-forced display set disproves
forced permanently, while proving forced needs the whole track, and
genuine forced tracks are tiny where full tracks are huge. So the same
budget is now SPREAD over each extent in ~16 MiB windows placed on the
AACS unit grid, sized in proportion to the extent, ending at the extent's
end. Cost is unchanged; placement is not.
Also:
* Per-track early exit. A track that is disproven (and whose label
needs no correcting) stops asking for budget; an extent that owes
evidence only for such tracks is skipped outright, and evidence
already in the cache is never demuxed a second time.
* Content may now DEMOTE a wrong vendor forced flag, in the probe and
in the muxer, behind one shared guard: absence of forced_on_flag
only means something if some other track demonstrably uses it, and
the track must have the shape of a full dialogue track rather than
of a forced-narrative one. On a disc where no track sets the flag,
nothing is demotable.
* A sampled or budget-cut extent's evidence is memoised with the
COVERAGE behind it. It used to be filed under the extent's full key
and replayed to playlists that would have read far more of the clip,
turning a prefix into an absence claim about the whole extent.
Sweep of every parser in src/labels/ for the numbering bug fixed in
pixelogic: a blob lists one entry per STN slot, but the parser advances
its per-kind counter only for entries it can use, so every entry it
skips shifts all later labels onto the wrong stream.
Three parsers were affected; the rest key each label off a number the
blob states outright and are immune.
* paramount — `aud` / `sub` are the STN-ordered stream lists, and
`forced_sub` / `*_com1_idx` index those same cells. A cell with an
empty language was skipped without consuming its slot, so every
label behind it bound one stream early while the vendor's own
positional indices still pointed at the raw cell. `stream_number`
is now the cell's 1-based position. The forced flag is the payload
here, so the shift lands `forced` on a full-dialogue track.
* mpls_universal — its counters must agree with the stream list
`disc::bluray` builds from the same STN entries, since that list is
what `apply_labels` counts against. They disagreed twice: a
`coding_type == 0` padding entry was counted here and dropped
there, and a PG coding_type in an audio STN slot (a layout
`mpls::parse_stream_entry` has a dedicated arm for) was counted as
audio here and built as a subtitle there. Both rules now live in
one `label_type_for`.
* deluxe — a binding construction whose Language `getstatic` did not
resolve was skipped outright. It is still an STN slot; it just has
nothing to label. It now advances the counter, with the list it
belongs to taken from its CodingType argument or, failing that,
from what its binding type's resolved siblings showed.
Two paramount tests asserted the renumbering as if it were the spec
(`empty_middle_slot_does_not_inflate_stream_number`,
`audio_stream_numbering_skips_empty_slots`) and are rewritten. Immunity
pins added for ctrm, dbp and criterion so the property cannot rot.
Also scrubs two commercial disc titles from the menu-graphic filename
examples in png_filenames and vocab.
Two comments identified the disc the STN-numbering bug was found on by its
vendor project name. The reproduction does not need it: what matters is the
SHAPE of the token list — placeholder slots, region-only tokens, an
uncatalogued component — not which release happened to exhibit it. Both now
describe the shape.
The remaining `SEG_MainFeature` references stay. That is a vendor section name
the parser matches on at pixelogic.rs:88, not a disc identifier — it is the
format's vocabulary, like `FPL_` or the `eng_MLP_` stream tokens beside it, and
removing it would break the parser.
Also drops the last prohibited citation from the changelog: an mp4:// bullet
said "no ffmpeg". The website changelog page is REGENERATED from this file at
release time, so a scrub of the site alone would have been reverted by the next
release.
A pixelogic feature section lists one entry per STN slot. Only the
entries that parse were advancing the counters, so every surviving
label was renumbered 1..N and applied to the wrong stream.
Three kinds of entry were being skipped:
* `PG Stream N` placeholders — the subtitle twin of `Audio Stream N`,
which was already counted. The old comment claimed the corpus showed
subtitle tokens align without counting them; on a disc that has both
placeholders and later editorial tokens they do not.
* region-only tokens (`fra_CF_`, `spa_LS_`) — REGIONS sets `variant`
but neither `is_audio` nor `is_subtitle`, so the token is dropped.
* tokens whose distinguishing component is uncatalogued (`jpn_DUB_`).
On UHD_Crime101_WW_150728 the PG list is 18 slots: five placeholders,
four region-only tokens, and `jpn_DUB_`, with the seven `*_TXT_FOR_`
forced-narrative tokens at STN 11-16 and 18. Counting only the ten
that parse put `eng_SDH_` on STN 1 and the seven forced markers on STN
2-8 — the disc's FULL subtitle tracks. `labels::apply_labels` turns a
Forced qualifier into `SubtitleStream::forced`, which the muxer writes
as Matroska `FlagForced`, so the output offered an "English (forced)"
track carrying 1731 display sets of complete English dialogue while the
real 19-set forced track went unflagged.
Unclassifiable entries carry no stream type, so they advance the list
currently being enumerated; sections run video -> audio -> PG and
`domain` follows the last entry whose type was known.
The fvi provenance fix, the key-service outage codes, the settings BOM
data-loss fix and the per-track-kind picker rows all landed on dev without a
changelog line. A release that ships undocumented fixes is one nobody can
audit later.
The online key service returned HTTP 502 for about seven hours. Every rip in
that window ended with
key: online > no entry > NO KEY
Error: E7022 No key source has a decryption key for this disc (id: 422EB...)
which reads as "this disc is not in the key database". Operators went hunting
for a VUK that was never missing; the correct action was to wait.
E7022 is a claim about the WORLD: every source answered, and none holds a key
for this disc. A source that could not be reached made no such claim -- nothing
at all was learned. `resolve_and_apply_traced` collapsed the two anyway, with
its own comment naming the incident and pointing at the fix.
Three codes for the three different operator actions, each a variant with a
number and no English (the library ships none):
E7028 KeyServiceUnavailable unreachable / DNS / timeout / 5xx -> wait
E7029 KeyServiceUnauthorized 401 or 403 -> fix token
E7030 KeyServiceRateLimited 429 -> back off
None carries a payload: the key-service URL and its resolved address are
operator-confidential and must not ride out in a Display an operator pastes
into a bug report.
`resolve_and_apply_traced` now keeps `Err` and `Ok(empty)` apart. A source that
answered and holds nothing still records `KeyNode::NoEntry` -- that is the one
true "I looked, it is not there". A source that FAILED records an empty path,
and its reason is stamped onto `Disc::aacs_error`, the channel
`ensure_decryptable_keys` already reads for the E7017-vs-E7022 split. The gate
gained three arms alongside `AacsVidUnavailable` and raises the source's own
code.
`KeyOutcome` deliberately gains no variant. It is matched exhaustively by every
front-end's trace renderer (freemkv's `pipe::render_resolution_trace`,
autorip's `keysource::render_resolution_trace`), and this fix must not become a
breaking change across four repos to say something the error code already says
precisely. Dropping the false `NoEntry` node is enough for the trace line.
The three codes join `is_disc_level_no_key`: a service that is down, refusing
the token or throttling is down for every title, so a rip loop must stop rather
than issue N doomed requests -- and on 429, dig the hole deeper.
`FetchOutcome::errored`, documented as unreachable-in-production, now fires for
real: the negative-result cache stops memoising a transient outage.
Red before green: `key_source_failure_is_not_reported_as_a_missing_disc_key`
drives KeySource -> resolve -> aacs_error -> ensure_decryptable twice over the
same disc and asserts the verdicts differ. With the old conflation restored it
fails on the trace node, and with that assertion removed it fails
`left: 7022, right: 7028` at the gate.
The `fvi://` arm of `output()` passed the destination `.fvi` path as
`FviSink::create`'s `source_path`, so every index named itself as its
own source. `SourceInfo::default()` supplied the rest, making
`source.medium` always "file" and `source.title` always 0 — three
header members wrong, where FVI_FORMAT.md §6.2 defines `source` as
describing the input.
Beyond the wrong data, it made the output unreproducible: two machines
indexing identical bytes emitted different files purely from where
they wrote them, and a local filesystem path leaked into a shareable
file.
`output()` cannot see the source, so thread the provenance down from
the driver, which can: `mux_stream` derives a `SourceInfo` per
`MuxInput` arm and passes it through `drive_mux` to `output()`. Per the
one-method-per-action rule this is a signature change, not an
`output_with_source()` variant; the parameter is `Option<&SourceInfo>`
so a caller with no provenance declares none rather than back-filling
the destination. `SourceInfo`/`Medium` become public API.
What each arm can honestly reach:
- Session: everything — device path, the caller's title index, the
title's playlist, the scanned volume id.
- Url: the source URL, its scheme's medium, `title_index`, and the
playlist off the opened stream's scanned title.
- Iso: the image path and playlist. The title index is not in
`MuxInput::Iso` (it carries a scanned `DiscTitle`, which has no
index), so it stays 0.
- Live: medium and playlist. The reader is an opaque
`Box<dyn SectorSource>` with no path, and again no title index.
Unreachable members are left empty rather than guessed — the sink
already omits the empty ones.
Every other sample-table parser (stco, stsc, stts, ctts) had a "count lie"
test proving the declared count is bounded by what the box actually holds,
and stco/stsc had their own arithmetic pinned. parse_stss had neither - no
test in this file ever called it with real entries, only with a too-short
buffer. Added the same two: two distinct entries read from their own
offsets (catching the o = 8 + i*4 arithmetic and the per-entry bounds
check), and a declared count of 3 backed by only 2 real entries (catching
the same "trust the box, not the count" contract the other four parsers
already had).
parse_stco, parse_stsc, parse_stts, parse_ctts and parse_stss all open with
the same "if b.len() < 8" guard before reading count = be32(b, 4), but no
fixture anywhere in this file ever called any of them with a buffer shorter
than 8 bytes - every test builds a complete box. A <-to-== mutant of that
guard only rejects a buffer of EXACTLY 8 bytes and lets everything shorter
fall through to an out-of-bounds be32 read, and nothing was exercising that
fall-through to notice. One test now drives all five through every length
from 0 to 8.
Also: co64's 8-byte offsets were never actually built by any co64 fixture
in this file (only the 32-bit stco path was exercised), so its manual
byte-by-byte u64 assembly was unconstrained the same way parse_elst's was.
And sample_offsets's sidx only advanced correctly by coincidence in every
existing fixture, because none of them placed three or more samples in a
single chunk back to back - the only shape where reusing the wrong sample's
size becomes observable.
Documented the <=8 boundary as equivalent across all five parsers (and the
matching start < end in sample_offsets): the per-entry guard immediately
below always breaks on the first entry at that exact boundary, so both
branches converge on the same empty result. Confirmed by re-running each
mutation against the full suite.
asc_len == 0 and end > b.len() reject for different reasons - a useless
zero-length ASC, and a truncated one - and nothing distinguished the || from
an && that would only reject when both are true simultaneously, or the >
from a < that would reject the common case of an esds with bytes after the
ASC (more child boxes, padding) instead of only a genuine truncation.
Documented the | in read_descriptor_len's accumulator as the same
shift-then-mask equivalent already recorded in audio.rs's BitReader::read:
the shift always vacates exactly the bits the mask fills, so | and ^ can't
disagree.
mdhd_language's guard was written as "b.len() < off + 2" (reject too short)
but nothing distinguished that from "reject anything not exactly off + 2" -
a buffer one byte longer than the minimum has to keep working, and a buffer
missing the field entirely has to return None rather than read past the end.
parse_stsd's mp4a path had no test at all: the AAC codec_private extraction
depends on both codec == Aac and body.len() >= 28 being true together, and
with no mp4a fixture anywhere in this file neither half of that condition,
nor the boundary itself, was constrained. Also added the same header-length
boundary check parse_elst already had (< 8 vs <= 8 - proved equivalent this
time, since the very next guard on the empty slice catches the <= 8 case
too), and one test walking every recognised audio fourcc (ac-3/ec-3/mp4a/the
four dtsX variants) so a deleted match arm for any of them fails loudly
instead of silently dropping that track.
Nothing asserted the scan actually STOPS at cap rather than one match past
it, or that the declared box size is decoded from its own four bytes rather
than an adjacent one - every existing fixture used sizes small enough that
all but the last size byte are zero, so an index slip reading the wrong byte
would read the same zero and go unnoticed.
Stream::read has its own s.size > MAX_ALLOC_BYTES cap, a separate call site
from read_moov's over the same policy - checked they agree (both reject
strictly greater than the cap, exact cap allowed) and they do, so this is not
one of tonight's one-policy-two-copies bugs. But the boundary itself and the
one-byte-over case were unasserted, and Mp4Reader::write returning an error
(mp4:// is read-only) had no test either.
Building Mp4Reader directly in the test (its fields are private but visible
within this module) over the existing FakeBigReader avoids a real 256 MiB
backing file for the boundary case.
Nothing asserted that a hdlr other than vide/soun gets dropped rather than
folded into the audio branch, that the per-track PID formulas
(0x1011/0x1100 + track_idx) use the right operator and the right operand,
that a sample-less track still advances track_idx for the next one, or that
the cross-track sample_budget is actually decremented (as opposed to grown or
divided) by each track's real count. All four were reachable with a single
track_idx == 0, which made every existing fixture blind to +/-/* confusion on
these sites - track_idx never moved past 0 in any of them.
Also let audio_trak_missing omit stsz, needed to build a sample-less track for
the track_idx test.
parse_elst's existing tests used segment_duration/media_time values that were
almost all zero or 0xFF bytes, so an index slip in the version-1 byte
extraction (reading a neighbouring byte, or one outside the entry entirely)
could return the same value by coincidence and the test wouldn't notice. Added
a fixture with every byte distinct and nonzero so any wrong offset is caught.
elst_offset_ticks's `empty_movie_ticks > 0` guard on the Some(mts) arm looked
like a pure optimisation, but shifting its boundary lets an
empty_movie_ticks == 0 call fall into the division instead of skipping it -
and a zero movie timescale (unreachable through from_reader, which filters it,
but not through this function's own contract) makes that division panic.
Pinned the boundary directly so the function stays safe on its own terms.
Documented nine further mutants as equivalent rather than chasing them:
media_edits/odd_rate and the None-arm's empty_movie_ticks check only gate a
tracing::warn!, never the returned offset, and parse_elst's b.len() < 8 vs
<= 8 boundary computes the same empty Vec either way once available = 0 is
worked through. Confirmed by re-running each mutation against the full test
suite.
The remaining triage items after tonight's HIGH fixes: 1,290 lines, almost
all tests. Covers disc/mod.rs's DVD scan path (with real minimal VMG/VTS IFO
fixtures rather than mocks), drive/mod.rs, labels/class_reader.rs and
labels/mod.rs — the two biggest untriaged survivor clusters in the crate —
plus hevc.rs and ps.rs.
One production change, and it is an extraction rather than a behaviour
change: MacScsiTransport::open mapped the shim's negative failure sentinels
to typed errors inline, where nothing could reach it without a real IOKit
FFI call. It is now map_shim_open_error, so the mapping can be pinned. It
matters because collapsing -5 into the DeviceNotFound catch-all turns
"another process holds the drive" into "no such drive", and an operator
chasing the wrong problem is worse than a blunt error.
Gate green on the pinned toolchain including the secrets scanner.
read_moov's forward-progress guard (box_size < header_len, OR'd with the
EOF check) and the MAX_ALLOC_BYTES cap were only exercised on inputs well
away from their boundaries, so a mutation testing pass found the exact
edges unasserted: a size-8 (header-only) moov, a box that overruns the
file by exactly the amount the OR/AND distinction can see, and a payload
of precisely MAX_ALLOC_BYTES. Added a shared FakeBigReader (lifted out of
an existing test's local struct so a new test can reuse it) to exercise
the MAX_ALLOC_BYTES boundary without a multi-hundred-MiB backing file.
Every mutation-testing survivor in this file is a | with ^ flip inside a
bitstream packer: BitReader::read's accumulate step, the push closures in
dac3_box/dec3_box/ddts_box, and the multi-field extractions in parse_eac3
and parse_dts. All nine are the same shape: shift an accumulator left by
exactly the width of the next field, then OR it in, so the two operands
never share a set bit and | and ^ agree on every input. Confirmed by
running cargo-mutants against just these nine mutations after the existing
test suite (which already exercises each function's field values) - all
nine still survive, as expected for a genuinely equivalent mutant. Recorded
the reasoning once at BitReader::read so nobody spends time chasing it
site by site.
Three pack_language mutants and two detect_rate boundary mutants survive
mutation testing with no test able to close them, and it's not for lack
of trying: they're equivalent by construction. Recording the proofs next
to the code so nobody re-chases them:
- (b[0] - 0x60) as u16, shifted << 10 then truncated to u16, is congruent
mod 65536 to (b[0] + 0x60) as u16 shifted the same way, because
0x60 * 2 * 1024 is an exact multiple of 65536. The same swap on the
second letter (shifted only << 5) is NOT equivalent, which is why only
the first letter's mutant survives.
- The two | with ^ mutations that OR the three packed fields together
are equivalent because the fields (a lowercase letter minus 0x60, so
1..=26) always fit in 5 bits and never share a set bit once shifted
into their 0/5/10 positions.
- detect_rate's tolerance and tie-break comparisons only diverge from
their <= mutants on an exact 0.5 fps distance or an exact tie, and a
brute-force search over every achievable integer-nanosecond median
found no case that lands on either boundary bit-exactly.
MP4 track timing was numerically unasserted. Every operator in the
PTS-to-ticks, duration, tkhd_dur and ctts chain could be flipped and the
whole suite stayed green, because no test decoded an output file and checked
a concrete number — the existing tests assert box presence and gross
container shape only. That is the crate's worst failure mode: a title muxes
"successfully" with silently wrong A/V sync or total duration, and nothing
above can tell.
The new tests build tracks with known PTS deltas and compare the emitted
stts, ctts and tkhd.duration against computed values.
Also lifted the faststart slack rule out of the match guard into
faststart_fits(). A leftover hole of 1-7 bytes cannot be expressed as any
ISO-BMFF box, since a box header is 8 bytes, so finish() must fall back to
moov-at-end rather than write a free box that lies about its own size. The
condition now has a name and a test instead of being an unexplained
`g == 0 || g >= 8` inside a pattern guard.
Drive::read and read_fua split any request larger than the transport's
transfer limit into chunks and slice the caller's buffer by count * 2048.
The up-front length check is the only thing between an undersized buffer and
a "range end index out of range" panic out of a public API, and the comment
above it records that this was once a live panic.
Every existing read test stays on the single-chunk path, where an undersized
buffer is already tolerated and returns Err(DiscRead), so the guard itself
had no coverage at all and a mutation run flipped its arithmetic freely.
The test drives a mock transport with a small transfer limit and asserts the
two paths agree: an undersized buffer is an error either way, and behaviour
on a caller mistake does not depend on the drive's transfer limit. Confirmed
by hand that both the reported * -> + mutation and a < -> > flip now fail.
The escape-stripper comments named a third-party decoder as the authority
for the cumulative-zero rule. This repo is public and does not cite other
implementations; the rule is specified in ITU-T H.264 §7.3.1, which is the
citation that belongs here anyway.
No behaviour change — comments only. The scan-secrets gate caught it.
A mutation run over mux/mkv.rs and mux/mkvstream.rs left 148 survivors.
Reading them turned up no wrong code, but a lot of code whose output
nothing ever looked at. Most of that is on the read side: parse_track
had a dedicated arm for Language, TrackName, FlagForced, Video and
Channels and not one of them was checked, so a re-mux could have lost
the audio language, the subtitle forced flag, every track label, the
resolution and the channel layout with the suite still green. Ten of
the eleven CodecID comparisons were unasserted too — only HEVC was
pinned — so any of them could have been mis-wired and the stream would
have gone to the wrong parser. The round-trip test now writes a real
three-track title through the muxer and reads it back through the
reader, and a separate test walks every registered CodecID.
The BPS statistics tag was the worst of the write side. Its test
asserted `file_bytes.contains("800")`, which a wrong bitrate passes
trivially — 80000 contains "800". Both the tag and the back-patched
Segment duration are now decoded and compared to a computed number, on
a title that declares no duration so the whole max_block_ticks →
seconds → bits chain is exercised. Cue points get the same treatment:
their CueTrack and CueClusterPosition were never read back, on either
the keyframe path or the i16-forced-split path, which are two hand-
written copies of the same three fields.
The rest closes arithmetic that only a bad disc reaches: a zero frame
rate or zero display-aspect denominator (both divisions), a
TimestampScale that does not fit an i64, a cluster timestamp of exactly
i64::MAX, a TrackNumber of 65537 that truncates onto the valid track 1,
and the shortest legal Block at both VINT widths. Two tests separate a
clean end of stream from a device failure: swallowing the second one
truncates the output at a bad sector and reports the rip complete.
Also pinned: only the first video and first audio track may be default
(the de-duplication lives in MkvStream::create and had no test at all),
the activation trigger is the first VIDEO track rather than track 0,
the measured field order reaches the file rather than just the helper
that computes it, and a Blu-ray 3D base/dependent pair builds the merge
instead of shipping two unrelated H.264 tracks.
96 of the 148 mutants verified killed by hand. Of the remainder, most
are equivalent — disjoint-bit `|` that `^` cannot change, delete-arm
mutants whose fallback is the same constant, guards on tracing calls —
and the write_frame branch at 1452 is unreachable: a cluster is always
open by the time it can be entered.
The mux/codec parsers (startcode, h264, hevc, dts) had 300 surviving
mutants between them, and it turned out to be for the reason you'd
fear: the exp-Golomb readers and the AU-boundary bitstream scanners had
essentially no direct unit coverage, only indirect exercise through
full-frame parse() calls that never touched the actual edge cases.
Direct fixes to test gaps:
- The shared BitReader's read_ue truncation guard (`leading_zeros >
31`) and skip_start_code's 4-byte-vs-3-byte boundary check had no
test at their exact boundary. Added tests that hit the boundary
precisely; a `>=`/`==`/`<=` typo either rejects a legal 31-leading-
zero code or reads one byte past the buffer.
- H.264's private SpsReader duplicates the same read_bits/read_ue
shapes with no tests of its own at all (only reached through
multi-field SPS parsing, several fields deep). Added direct tests.
- HEVC's per-AU trailing-zero strip after the last NAL (no start code
following) walks `end` down to trim padding; a wrong-direction typo
there walks off the end of the buffer instead of terminating -
exactly the "loop must make positive progress on malformed input"
class. Added a test with a zero-padded trailing NAL.
- HEVC's SEI match guards (`sei_mastering.is_none()` /
`sei_content_light.is_none()`) implement "first HDR10 value in the
title wins" - untested, and a naive test using both-messages-per-AU
can't even exercise the guards because the whole-scan early return
above them already handles that case. Split into single-message-
per-AU tests that actually reach the arms.
- parse_mastering_display/parse_content_light_level's length guards
were `< N` with no boundary test; one-byte-short input now confirmed
to return None instead of indexing out of bounds.
- DTS's drain_front collapses duplicate offset-0 PTS markers after
rebasing; untested, and the visible effect (front_pts()) can't tell
a working collapse from a broken one since it already returns the
right marker either way - the actual defect is unbounded growth of
pts_marks over a long recording, so the new test asserts the bound
directly across repeated drains.
- DTS's dts_core_samples/dts_core_sample_rate header-length guard and
next_core_boundary's syncword-length guard got exact-boundary tests
the same way; also caught a nblks `<<`/`>>` direction bug candidate
in the mutant (confirmed the real code is correct, just untested).
Real bug found and fixed, not just a test gap:
H.264's parse_sps_high_profile_ext re-implemented emulation-prevention
byte stripping inline (a window scan: match `00 00 03` at position i,
advance 3, else advance 1) instead of calling the existing
unescape_ebsp_prefix used by slice-header parsing. On a run of 3+ real
zero bytes ahead of an 0x03 - non-conformant, but this is disc bytes,
not a spec-clean encoder - the two disagreed: unescape_ebsp_prefix's
cumulative zero counter (matching the H.264 reference decode process
and libavcodec's RBSP extractor) treats it as an escape and drops the
0x03; the window scan treats it as real payload and keeps it,
corrupting the SPS bits read after it. Extracted the shared rule into
`unescape_ebsp` (parameterized on output length so both the 16-byte
slice-header prefix and the unbounded SPS case can share it) and
pointed both call sites at the one implementation. Added a regression
test pinning the shared function's behaviour on the input that used to
separate them.
All new tests hand-verified against the actual mutation (operator
flipped or guard replaced by hand, confirmed red, then restored) per
the mutation-testing brief, not just written and trusted.
A 12,330-mutant run left 159 survivors across these three files, all from
missing assertions rather than wrong code — every gap here is a test, no
production logic changed.
Two shapes accounted for most of them:
- Buffer-cap constants (MAX_PES_BUFFER_TOTAL, MAX_PS_BUFFER,
MAX_BD_PES_PAYLOAD, PES_BUFFER_INIT_CAP) were only ever read by tests
through their own symbol, so a mutated `*`/`-` in the constant's
definition changes what the symbol itself evaluates to and every
self-referential assertion still passes. Pinned each against a literal
computed independently in the test.
- Several `>`/`==` boundary checks on framing lengths (MPEG-2 pack header,
system header, BD-TS adaptation field) were only ever exercised with
slack in the buffer, never at the exact byte the check exists for.
Added exact-fit cases for the pack header, system header, and
psi_payload_base's AF-consumes-everything boundary.
Real, higher-value gaps closed along the way:
- ts.rs's per-PID discontinuity_flag and the NULL-TS concealment marker
both require adaptation_field_length > 0 before trusting the AF flags
byte; neither branch had a test proving af_len == 0 (no flags byte at
all, ordinary payload underneath) is left alone.
- header_remaining (PES header spillover across TS packets) only had
single-continuation-packet coverage, which can't distinguish `-=` from
`+=`/`*=` because the corrupted value never gets read again. Added a
case spanning two continuations.
- ps.rs's parse_stream_id_extension (used for HD-DVD 0xFD routing) walks
nine optional PES-header/extension fields with a `pos +=` each; only
the PTS/DTS pair had ever been exercised. One test now arms every
field and checks the walk lands on the right byte.
- find_ps_boundary's `sc + 3 >= len` guard had no test at sc == 0 with a
bare 3-byte start code, the case a `+` -> `-` mutation turns into a
debug-mode subtract-overflow panic on ordinary tail-of-buffer input.
- tsmux.rs: an oversized video access unit must go out as a single
unbounded-length PES; the `is_video || small-enough` guard that
enforces this had no test with a video frame actually over the
bounded-PES threshold, so a `||` -> `&&` mutant survived (it would
silently split a keyframe across several look-alike-independent PES
units). Also pinned the PES-length and PTS big-endian encodes at
values above 255 / with bit 29+ set, where a `>>`/`<<` swap first
becomes observable.
Every test above was verified by hand: applied the exact mutation,
confirmed the test fails (or the specific panic fires), then reverted.
Left unclosed, all confirmed equivalent by hand-tracing rather than
just left alone:
- Every `<<8 | byte` PID/length bit-combine (ts.rs pid/PAT/PMT parsing,
ps.rs dvd_audio_pid/hddvd_extended_pid/parse_pts): the two halves
never share a bit, so `|` and `^` produce identical output for every
input - no test can tell them apart.
- ts.rs's `af_len > 183` check in process_packet: fully subsumed by the
`payload_start >= TS_PACKET_BYTES` check three lines later for every
af_len that could trip it.
- ts.rs's out-of-range `pid_index` sentinel (-1 vs 1): unreachable, since
a TS PID is masked to 13 bits (max 8191) and the table is always sized
to at least 8192.
- A cluster of "push an empty slice on an exact boundary" mutants in
tsmux.rs's write_pes_chain (offset < hdr_len, af_bytes stuffing
guards): the guarded write becomes a length-0 write_all, a no-op
either way.
Not reached this pass, for lack of a clean seam within the time
available - ps.rs's extract_packets bounded-PES-length exact-fit
checks (lines 278/282/303, the `sc+6>len` / `sc+6+pes_len>len` /
force-flush cap arithmetic). The first two need a scenario where
"proceed vs. wait one more byte" is observable in the packet list, and
the third only shows up at a start-code offset (sc) that survives to
the moment the cap check runs - in this code path sc is always 0 once
an unbounded PES buffer starts accumulating, since nothing before it
ever drains. Didn't find a construction in the time available; flagged
rather than papered over with a self-referential assert.
DriveId::from_drive issues three data-in commands. The two GET CONFIGURATION
calls both clamp on bytes_transferred, with a comment noting it is
device-reported and untrusted. INQUIRY, three lines above them, discarded it
and decoded bytes 8..43 unconditionally.
The buffer is pre-zeroed, so a drive answering GOOD status with a short or
empty data phase — a USB-SATA bridge mid-wedge does exactly this — produced
blank vendor, product and revision strings and a byte 0 of 0x00. Every
platform enumerator gates on raw_inquiry[0] & 0x1F == the optical peripheral
type, and 0x00 is DIRECT ACCESS, so the drive silently disappeared from the
device list instead of reporting that its identity probe had failed. The
operator sees no drive at all rather than an error.
Anything shorter than the SPC-4 standard 36-byte header is now
E9058 DriveInquiryShort, and the buffer is truncated to what actually
arrived so nothing decodes past it. Exactly 36 bytes is still accepted: the
vendor-specific tail is optional.
This is the same defect as the READ CAPACITY short-transfer bug fixed
earlier today, in the same crate, found the same way.
mod.rs had two independent implementations of "pick the winning label
parser". select_result owns the rule — highest confidence wins, and on a tie
the earlier entry in PARSERS wins — and carries a regression test for a past
bug where analyze() picked the LAST equal-confidence parser instead of the
first. extract(), the path that actually ships, re-derived the same rule with
its own inline scan and had no test of its own.
The tie-break is load-bearing rather than incidental: array order encodes a
trust ordering, with the hand-vetted parsers registered ahead of the ones
that detect on any BD-J disc. A later parser winning a tie means the disc
gets labels from a less trusted source, silently.
extract now collects its candidates and calls select_result. Confirmed by
flipping the tie-break to prefer the later entry: the shared test fails,
where before the fix that mutation was invisible to the whole suite.
This is the seventh instance tonight of one policy implemented twice with
only one copy hardened, and the second in this file after the chapter
mark_type filter.
An FMTS forensic segment interleaves two variants at the unit level: the
disc carries both halves and we hold the key for exactly one parity.
Decrypting the alternate half with our key produces garbage; leaving it as
ciphertext is correct, because the muxer drops untouched ciphertext cleanly.
The decision lived inline in apply_aacs_map's per-unit closure, where no test
could reach it. A mutation run flipped the subtraction to an addition and the
parity comparison, and every test still passed — so the gate that decides
which half of a forensic segment we decrypt was entirely unasserted.
It is now unit_is_our_phase(). The index arithmetic also saturates and
clamps: both inputs come from the key map, so a unit below its own range
start or a zero unit size means a malformed map, and neither may panic on
debug overflow or a divide by zero inside a library a long-running service
depends on.
Four of the five surviving mutants now fail. The fifth, dividing by the unit
size versus multiplying, is equivalent rather than uncovered: aligned offsets
are exact multiples of the unit size, so the two differ only by a factor of
unit_sectors squared, and unit_sectors is the constant 3 — odd, so parity is
preserved on every reachable input. The proof is recorded in the test instead
of a test that pretends to cover it.
fill_extents skipped an exhausted or zero-sector extent by calling itself,
which costs a stack frame per skipped extent. Nothing filters
sector_count == 0 out of a UDF or MPLS extent list, so a malformed disc
declaring a long run of empty extents recursed once per extent before
reading a single sector. Rust does not guarantee tail-call elimination, so
that overflows the stack — which aborts the process rather than returning
an io::Error, taking a long-running service down with it.
The skip is now a loop. The regression test runs on a 256 KiB stack, where
the recursive version dies and the loop finishes immediately.
Two cases of the same shape: one policy implemented twice, with only one
copy hardened.
Disc::read_capacity decoded buf[0..4] from READ CAPACITY (10) without
checking that the transport actually delivered four bytes, even though its
comment claims to mirror decode_read_capacity — which has exactly that
check, and documents why. A drive answering GOOD with an empty data phase
leaves the buffer zeroed, so last_lba decodes to 0 and the probe reports a
one-sector disc instead of an error. It now calls the shared decoder rather
than re-deriving it.
collect_chapter_summary filtered chapters on mark_type <= 1, counting the
reserved type 0. PlaylistMark's own doc says filters must test == 1, and
disc/bluray.rs did; the labels path did not, inflating the public
chapter_count and letting a playlist whose only marks are reserved pass the
chapter_count == 0 skip. Both sites now share PlaylistMark::is_chapter_mark
so the copies cannot drift again.
Both fixes were confirmed red before green.
Every job above proves libfreemkv builds; none proved anything built on
it does. That gap bit today one level up — an engine signature change
broke autorip and went unnoticed, because consumer CI only fires on a
push to that consumer, and nobody pushed one.
libfreemkv sits underneath all five dependents, so a break here costs
more than a break anywhere else in the project. It is now the place the
question gets asked, since it is the place the change happened.
cargo check --all-targets only: each dependent has its own suite for its
own behaviour. This answers the narrower question that went unanswered.
Every other repo's CI now runs clippy with --all-targets. libfreemkv,
the crate the other seven build against and the one held up as the
reference workflow, was the last one still linting the library only — so
its ~3,000 tests, by far the largest body of test code in the project,
had never been linted at all. Turning the flag on surfaced 74 findings.
Most were mechanical and applied with clippy --fix. The rest, by hand:
- Four discarded Results in decrypt.rs. css::descramble_region returns a
Result and four CSS tests threw it away, so a descramble that FAILED
would have surfaced as a confusing buffer-comparison mismatch instead
of the actual error. They expect() now.
- A dead `kp` field on the PlantedWalk fixture. The test deliberately
asserts Kp as the explicit AES-G3(dk, 1) relation from [C] §3.2.4
rather than against a stored value — its doc comment says so — which
makes the field not just unused but a trap: the obvious "fix" of
asserting against it would quietly weaken the test to comparing the
fixture with itself. Removed.
- Two hand-rolled ICB counters in the HD-DVD fixtures, a needless mut,
three vec!s that only ever needed arrays, a filter_map whose every arm
was Some, and a Vec::new()+push chain.
- Doc list indentation in mkv.rs and mp4/read.rs, which was mis-rendering
in the generated docs.
- A five-[u8; 16]-tuple return type named FourLevelParts.
Three lints are allowed at the specific sites, with reasons, because
they are wrong for this domain: the underscores in the bitstream-header
literals mark BITFIELD boundaries, not digit groups, so regrouping them
uniformly would satisfy the lint by destroying the only thing they
encode; and in three table-validation loops the loop variable is the
domain value under test (a DTS SFREQ code, an AMODE value, a palette
entry number), which is what the assertion messages name.
Matthew Jackson