fix(vrc7): carry the live OPLL in the save state so rewind resumes the music - #398
Merged
Conversation
…e music Closes the most actionable open row in `docs/accuracy-ledger.md`, open since v2.2.3 and latent since the ADR 0006 VRC7 audio landing. THE DEFECT `Vrc7::save_state` wrote the *shadow* OPLL register bytes — `addr_latch`, `data_latch`, `silenced`, `regs[0..64]` — and never the live synthesizer: not `self.opll`, not `opll_clock_counter`, not `last_opll_sample`. `load_state` restored those shadow bytes and never replayed them into the chip either. So after a rewind, a netplay rollback, or a TAS/save-state restore, the FM voice resumed from whatever envelope and phase state it happened to be holding — an arbitrary point in an unrelated note. Banking, IRQ, mirroring and PRG-RAM had always round-tripped correctly; this was audio-only, and only on mapper 85 with `mapper-audio` on. It was not serializable at the time it was found: `rustynes_apu::Opll` exposed no serialization surface at all, and its envelope generators, phase accumulators and LFO are private. That is what made this "a change of its own" rather than a release-cut drive-by, and it is what this commit does. THE SNAPSHOT SURFACE `Opll::snapshot` / `Opll::restore`, versioned by `OPLL_SNAPSHOT_VERSION` with a fixed `OPLL_SNAPSHOT_LEN`. Carried: the 64-byte register shadow, the current register address, the test flag, the key-status mask, the EG counter, both LFO phases and the AM output, the per-channel patch numbers, the user patch pair (writeable through `$00-$07`), all 18 operator slots in full — phase accumulators, envelope state machines, feedback history, the derived TLL/RKS and rate fields, and the pending-update mask — plus the per-channel outputs and the mix. NOT carried, because they are constants of construction and serializing them would be writing a copy of the binary into the save file: `waves` (the 1024-entry sine / half-sine tables), `tll_rks` (~128 KiB of TLL + RKS tables), and `patch_set[2..]` (the chip's patch ROM, selected by `chip_type`). `chip_type` itself is written as a tag ONLY, never assigned from — its job is to reject a cross-chip restore, since a YM2413 blob loaded into a VRC7 is structurally valid in every field and would silently reinterpret all 18 slot patches against the wrong instrument set. The reader is bounds-checked on every field and decodes the entire blob into locals BEFORE touching `self`, so a truncated or hand-edited save leaves the synthesizer on its previous state rather than half-overwritten — this parses untrusted input, and the caller reports the error and keeps running. Enum tags (`EgState`, `ChipType`) are explicit `to_tag`/`from_tag` mappings rather than `as u8`, so reordering a variant cannot silently reinterpret existing states. Its error type is deliberately NOT `ApuSnapshotError`: the blob rides in the *mapper* section of whichever board carries the chip, which is versioned independently of `APU_SNAPSHOT_VERSION`. Sharing the type would assert a coupling between two schemas that must be free to move apart. THE MAPPER SECTION VRC7 goes to section **v2**, appending `opll_clock_counter` (u16), `last_opll_sample` (i16) and the OPLL blob after the VRAM. Additive: `load_state` still accepts v1, and a v1 blob leaves the synthesizer untouched — which is precisely the pre-fix behaviour, so an old save is no worse than it always was rather than newly silent. A build without `mapper-audio` has no synthesizer to describe, so it writes v1 and validates-then-ignores a v2 tail. That keeps the property this crate's feature documentation and ADR 0004 promise in both directions — an audio build's save loads in a no-audio build, and a no-audio build's save loads everywhere — which is why `VRC7_SECTION_VERSION` is build-dependent rather than a flat 2. THE ALTERNATIVE, AND WHY IT LOST Replaying `audio.regs` through `Opll::write_reg` on load needs no new format and is the repair a reader thinks of first. It restarts every keyed-on channel's envelope at attack, so every rewind frame produces an audible transient. The ledger recorded that there was no oracle to adjudicate which was worse; carrying the state verbatim removes the question, because it reproduces the sound that was actually playing. TESTS `vrc7_save_state_carries_the_live_opll_so_audio_resumes_identically` keys a note on channel 0 with a real melodic patch, advances 20,000 CPU cycles so the envelope is well past attack and the phase accumulators hold values no reset could coincidentally match, saves, then compares 4,000 mixed samples from the source against 4,000 from a FRESH mapper restored from the blob — equal sample for sample. Mutation-checked: making the tail carry a *reset* synthesizer reproduces the pre-fix failure on the first divergent sample. Plus, at the `Opll` level: a 2,000-sample stream round-trip into a fresh chip; snapshot -> restore -> snapshot byte idempotence (which catches a field written but not read back, a case the stream test can miss); cross-chip rejection; truncation rejection asserting the target is left UNMUTATED; unknown-version rejection; and a corrupt envelope-state tag. At the mapper level: a v1 back-compat load and a truncated-v2-tail rejection. `Opll` is now registered in `snapshot_schema_audit.rs`. That audit — the standing field-vs-schema check that found the v2.2.3 PPU and APU gaps mechanically — knew only about the three chips inside the console, so it could not see this surface at all. A save-state surface no audit can see is exactly how a gap this size survives four releases, so the surface is registered in the same change that creates it. Registering it immediately caught two of my own exclusion entries as false admissions (`chip_type` and `patch_set` ARE written), which is the audit working. Out of scope, checked and left alone: `nsf_expansion.rs` holds an `Opll` too and also carries no phase, but that is a written decision with a stated rationale (an NSF driver re-establishes channel state on the next play call), not an undocumented gap. VERIFICATION Nothing on the synthesis path moved, so emulation output is unchanged — but this touches `rustynes-apu` and `rustynes-mappers`, so the contract was verified rather than assumed: AccuracyCoin **141/141 (100.00%)** via the authoritative RAM decoder, nestest 0-diff. Workspace clippy, `mapper-audio`-off clippy, rustdoc with warnings denied, the `no_std` cross-build, and 124 workspace test binaries all green.
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a determinism/save-state gap for the VRC7 (mapper 85) FM audio path by snapshotting/restoring the live OPLL synthesizer state, so rewind/rollback/TAS restore resumes VRC7 music sample-identically instead of restarting from an arbitrary envelope/phase.
Changes:
- Add
Opll::snapshot/Opll::restorewith a fixed-size, versioned blob and robust validation to safely parse untrusted save-state bytes. - Extend the VRC7 mapper save-state section to v2 (appending OPLL runtime state + timing counters) and add regression tests for sample-identical resume plus back-compat.
- Register OPLL in the snapshot schema audit and update the accuracy ledger + changelog entry to reflect the remediation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/accuracy-ledger.md | Marks the VRC7 OPLL save-state continuity item as remediated and documents the chosen approach + tests. |
| crates/rustynes-test-harness/tests/snapshot_schema_audit.rs | Adds OPLL to the schema audit so future fields must be classified (derived/config/gap). |
| crates/rustynes-mappers/src/m085_vrc7.rs | Adds v2 tail to mapper 85 save state to carry live OPLL state; adds targeted tests. |
| crates/rustynes-apu/src/opll.rs | Implements the OPLL snapshot/restore schema and extensive validation + unit tests. |
| crates/rustynes-apu/src/lib.rs | Re-exports OPLL snapshot constants and error type for downstream use. |
| CHANGELOG.md | Documents the user-visible fix in the Unreleased “Fixed” section. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review found a real defect in the previous commit, and the reason no test could
have caught it is the more useful half.
THE DEFECT
`load_state` gated its accept set on `VRC7_SECTION_VERSION`:
if version != 1 && version != VRC7_SECTION_VERSION { reject }
That constant is the version this build WRITES, and it is deliberately **1** on
a `mapper-audio`-off build (no synthesizer to describe). So on that build the
condition collapsed to `version != 1` and a v2 blob was rejected outright with
`UnsupportedVersion(2)` — the precise opposite of the validate-then-ignore
portability ADR 0004 asks for, and the opposite of what the constant's own doc
comment asserted two lines above it.
What a build can WRITE and what it must ACCEPT are different sets, and only the
first varies by feature. Deriving one from the other reads as tidy and silently
couples them. The check now compares against the literals 1 and 2, with the
reason recorded at both the check and the constant.
WHY NOTHING CAUGHT IT
The default build takes the other branch and was correct throughout, so every
gate stayed green. CI **linted** the `--no-default-features` shape
(`cargo clippy -p rustynes-mappers --no-default-features`) and never **ran** it.
A configuration that is compiled but never executed is not covered, and this is
what that costs.
`cargo test -p rustynes-mappers --no-default-features` is now a CI step, Linux
only and scoped to the one crate whose behaviour is feature-gated this way.
One extra compile of a mid-sized crate, so it runs on every PR rather than
waiting for a full run.
TESTS
`vrc7_load_state_accepts_a_v2_blob_on_every_build` asserts the property directly.
On a `mapper-audio` build `save_state` already emits v2; on a no-audio build it
emits v1, so the test synthesizes the v2 shape — correct, because that path
validates the tail's LENGTH on every build and reads its CONTENTS only where
there is a synthesizer.
Mutation-checked in BOTH configurations, which is the part that documents the
hazard: restoring the old condition turns the test red under
`--no-default-features` with `UnsupportedVersion(2)`, and leaves it green on the
default build. A single-configuration mutation check would have reported the bug
as absent.
Also from review: `save_state`'s `Vec::with_capacity` unconditionally reserved
`VRC7_V2_TAIL_LEN`, over-allocating ~1.3 KiB per save on a build that never
writes that tail. Now `cfg`-gated.
Declined: aligning `OpllR::u32`'s `copy_from_slice` with `OpllR::patch`'s
`try_into` destructuring. The difference is deliberate — `patch` destructures
positionally so its field order is visibly the same list `OpllW::patch` writes,
which is the property that keeps the two halves from drifting; a scalar read has
no such list to mirror.
Owner
Author
|
Antigravity review addressed — the blocking finding was real. Fixed in b080ece. Blocking — cross-build compatibility. Confirmed and fixed exactly as The part worth keeping is why no gate caught it: the default build takes the Suggestion — unconditional Nitpick — |
….6 cut Merging `main` after the release SILENTLY DROPPED this entry, and reported no conflict while doing it. Worth writing down, because "mergeable: MERGEABLE" is what GitHub said right up to the moment the text disappeared. The release moved everything out of `[Unreleased]` into `[2.3.6]`, deleting the `### Fixed` heading this entry was anchored under. Git saw one side delete a region and the other side add a line inside it, resolved in favour of the deletion, and produced a clean tree. No marker, no warning -- the entry was simply gone from the merged CHANGELOG. Caught by inspecting `git merge-tree --write-tree` output before merging rather than trusting the mergeability flag; all three post-release fix branches were affected the same way. The fix is mechanical (re-anchor under a fresh `### Fixed` in the now-empty `[Unreleased]`), but the failure mode is not: a CHANGELOG entry is exactly the kind of content whose absence nothing downstream detects.
…3.6] My own error, and the diagnosis that produced it is worth recording. Merging `main` after the v2.3.6 cut did not DROP this entry, as I concluded -- it MOVED it. The release relocated the whole `[Unreleased]` block into `[2.3.6]`, and git carried this branch's addition along with the block it was written inside. I checked for the entry only under `[Unreleased]`, saw nothing, called it dropped, and re-added it there. Two copies: one correctly under `[Unreleased]`, one wrongly inside a released section describing work that release does not contain. The check was too narrow, not wrong in kind: `git merge-tree --write-tree` was the right instrument and it did show the real merged content. I searched four lines of it instead of the whole file. Both review bots caught the duplicate. Removing the `[2.3.6]` copy; the `[Unreleased]` one stays, which is where a fix that ships in the NEXT release belongs.
Two defects from a second review pass, both in code this PR introduced, and both
of a kind the tests as written could not see.
A HAND-EDITED SAVE STATE COULD CRASH THE EMULATOR
`commit_slot_update` indexes the TLL table as `[block_fnum][tl][kl]` with
dimensions `[128][64][4]`, and `Opll::restore` handed it raw deserialized bytes.
A `tl` of 255 computes an index of 524,539 into a 32,768-entry table:
index out of bounds: the len is 32768 but the index is 524539
`blk_fnum` reaches the same tables through `>> 5` (128 rows) and `>> 8` (16), so
an unmasked u16 indexes far past both. A save state is a file on disk --
untrusted input, and module 60 is explicit that every field read from untrusted
bytes is bounds-checked before use.
Every register field is now masked to its hardware width AT THE PARSE BOUNDARY:
the 13 patch parameters to their documented widths, `blk_fnum` to 0x0FFF (a
legal value is `(blk3 << 9) | fnum9`), `fnum` to 9 bits, `blk` to 3, the
single-bit flags to 1, and `number` into slot range. This is parse-don't-
validate rather than a repair: these are register fields of fixed bit width, so
a wider value does not describe a chip state that exists.
The test is careful about one thing worth stating, because the obvious version
of it is useless. An all-`0xFF` blob is REJECTED by `EgState::from_tag` before a
single numeric field is read -- so the naive hostile input passes by accident and
reports the emulator safe. The test therefore repairs the envelope-state tags and
leaves everything else hostile: the interesting input is the one that satisfies
every explicit check and is still nonsense. Mutation-checked -- removing the `tl`
mask alone reproduces the panic.
`load_state` WAS NOT ATOMIC
The v2 tail introduced a failure that can occur AFTER the core fields are
assigned, which the v1 layout could not: v1 validated its whole length and
version up front, so once it began writing it could not fail. A truncated or
corrupt v2 tail returned `Err` with `prg_0`, `chr`, the IRQ state and 2 KiB of
VRAM already overwritten -- a mapper in neither its old state nor its new one,
while the caller reports the load as failed and keeps running.
`Opll::restore` was already atomic internally, and that is exactly what made this
easy to miss: the guarantee existed one level down and was silently discarded one
level up. `load_state` now validates the tail length and parses the synthesizer
into a staged clone BEFORE the first write, then commits infallibly. The
no-audio build performs the same length validation, so the same blob is accepted
or refused identically on every build.
The truncation test asserted only on the return value, which is why review found
this and the test did not. It now gives the target different state from the
source and asserts it is byte-identical after the rejected load. Mutation-checked
-- moving the length guard back after the writes turns it red.
ALSO TAKEN
`tail[0..2].try_into()` over manual `[tail[0], tail[1]]` indexing, guaranteed by
the preceding length check.
DECLINED
Bounds-checking `patch_number`, `wave_table_idx` and `s.number` as suggested:
traced, and none of them index anything. `wave_table_idx` selects through a
`match idx { 0 => .., _ => .. }`; `patch_number` is only ever compared to zero;
`s.number` is stored and compared, never used as a subscript. The reachable path
was elsewhere, through the patch parameters and `blk_fnum` -- which is why it was
worth tracing each one instead of masking on the strength of the report.
VERIFIED
AccuracyCoin 141/141 on the authoritative RAM decoder, nestest 0-diff. Workspace
clippy, `mapper-audio`-off clippy AND tests, rustdoc warnings-denied, the no_std
cross-build, 124 workspace test binaries.
Owner
Author
|
Antigravity review addressed — both blocking findings were real, and both were Blocking — What makes this one worth naming: Blocking — unvalidated indices from a save state. Real, and reachable — but The reachable path is Every register field is now masked to its hardware width at the parse boundary. Suggestion — Suggestion — advance the slice instead of a Nitpick — Nitpick — |
# Conflicts: # CHANGELOG.md
…underflow-proof Two review findings. A LEAKED diff3 CONFLICT MARKER `CHANGELOG.md` carried a stray `||||||| 3f99a3d`. My resolver checked for `<<<<<<<`, `=======` and `>>>>>>>` and reported zero markers left -- but this repo sets `merge.conflictStyle = diff3`, which adds a fourth marker naming the merge BASE. I verified the wrong alphabet and believed the answer. Content was unaffected (the base section was empty, so only the marker line survived), and all three post-release branches had it. THE NO-AUDIO TAIL-LENGTH CHECK COULD UNDERFLOW data.len() - core_expected < VRC7_V2_TAIL_LEN cannot underflow today: the guard at the top of `load_state` already proved `data.len() >= core_expected`. But the proof lives in a different statement thirty lines away, and the failure mode if it ever moves is not a panic -- the subtraction wraps to a huge value, the comparison goes false, and a TRUNCATED BLOB IS SILENTLY ACCEPTED. That is the worst available outcome for a save-state length check, and it is one refactor away. Rewritten as `data.len() < core_expected + VRC7_V2_TAIL_LEN`, which is correct independent of anything above it. Not worth spreading a correctness argument across two distant statements to save an addition. Verified in both feature configurations: `mapper-audio` on (20 VRC7 tests) and off (16), plus no-audio clippy.
Owner
Author
|
Antigravity review addressed. Both findings taken. Blocking — leaked Suggestion — Nitpick — |
…found
Review pushed back that my "every register field is masked at the parse
boundary" claim did not match the code. It was right, and the pushback was worth
far more than the specific fields it named.
THE FIXED PAYLOAD WAS HIDING BUGS
The previous test used one all-`0xFF` payload. Replacing it with a deterministic
pseudo-random sweep (64 rounds, xorshift, no dependency) found THREE more panics
immediately -- and one of them the fixed payload actively CONCEALED:
* `eg_shift` is used as a shift amount, `1u32 << eg_shift`, which panics at 32
and above. Legal values are `13 - eg_rate_h`, so 0..=13. The all-`0xFF` blob
never reached it, because with every byte 0xFF `update_requests` is also
all-ones -- so `commit_slot_update` RECOMPUTED `eg_shift` before
`calc_envelope` could use the restored one. A blob that is maximally hostile
in one dimension can be harmless in another.
* `output[0] + output[1]`, the operator feedback pair, summed as two arbitrary
`i32`s. The field is `i32` for headroom in that sum; the values are only ever
`i32::from(out)` with `out: i16`. Clamped to i16 on restore.
* `eg_rate_l` INDEXES `EG_STEP_TABLES`, outer dimension 4. Legal values are
`rks & 3`.
That last one is the one to remember. Review named `eg_rate_l` explicitly. I
replied that I had traced it and it reached no subscript. My "trace" was a grep
pipeline that emitted nothing because the pipeline itself was broken, and I read
the empty output as evidence of absence. This is the second time tonight I
verified with a broken instrument and believed the answer -- the first being a
conflict-marker check that did not know about diff3.
WHAT CHANGED
`eg_shift` clamped to `EG_SHIFT_MAX`, `output` clamped to i16, and the flag
fields masked to their real widths: `eg_rate_l & 3`, `eg_rate_h & 0x0F`,
`rks & 0x0F`, `type_flags & 3`, `key_flag`/`sus_flag`/`test_flag & 1`.
The comment that overclaimed now states the scope precisely -- what is masked,
and for each field that is NOT, the specific mechanism that makes it safe
(matched-on, clamped at use, recomputed before use, or self-masking index). It
also points at the sweep, because that is what keeps the claim honest rather than
a second round of hand-tracing.
Soaked at 4,000 rounds in both debug (overflow checks on) and release before
settling at 64 in the committed test.
VERIFIED
AccuracyCoin 141/141 on the authoritative RAM decoder, nestest 0-diff. Workspace
clippy, no-audio clippy and tests, the no_std cross-build, VRC7 20/20.
Owner
Author
|
You were right, and pushing back was worth far more than the specific fields "The PR claims every register field is masked … however these are unmasked."
The first one the fixed payload actively concealed: with every byte The third is the one I owe you a correction on. You named Now:
AccuracyCoin 141/141, nestest 0-diff. |
# Conflicts: # CHANGELOG.md
Review asked whether `adr` and `patch_number` are bounded, noting `adr` could index `self.reg` (length 64) on a subsequent data-port write. Answering that by reading the code is what I did twice tonight, and both times the reading was wrong -- once because a grep pipeline was broken and I took its empty output as proof. So the sweep answers it instead. The randomized sweep only called `calc()`. Synthesis alone never reaches `write_reg`, so any restored field consumed on a subsequent PORT write would sail straight past it -- which is precisely the hole review pointed at. Each round now also drives 64 randomized `write_reg` / `read_reg` pairs against the restored chip, interleaved with `calc()`. Clean, including a 3,000-round soak in debug with overflow checks on. That is a stronger statement than the trace review asked for: not "I looked and `adr` is never an index", but "randomized values in `adr` survive being used as one, on a restored chip, through the port". Covering both paths is also cheaper than adjudicating which fields reach a subscript -- and it keeps working when someone later adds an index that does.
Owner
Author
|
Answered with the fuzzer rather than a trace, because tracing is the instrument You are right that the sweep had a hole: it only called Each round now also drives 64 randomized That is a stronger claim than the one you asked for: not "I looked, and On |
Third review round flagging `patch_number`. It is still not a subscript -- it appears only as `patch_number[ch] == 0` comparisons and `patch_number[ch] = num as i32` assignments, and `set_patch_internal` bounds-checks its own `num` against `patch_set.len()` before indexing. The randomized sweep exercises it with arbitrary values, through both synthesis and the register port, and finds nothing. Clamped anyway, and the reason is not appeasement. Every other field in this parse is constrained by its own WIDTH: the value that comes out cannot describe a chip state that does not exist. `patch_number` was the one exception, safe only because nothing currently indexes it -- an invariant that lives in other functions and holds until someone adds an index. The legal domain is 0..=15 (the `$3x` high nibble is four bits), so clamping costs nothing and makes the field's safety self-evident from the parse rather than from a survey of its consumers. Three reviewers reading the same code and reaching the same worry is decent evidence the invariant was too subtle to be load-bearing, even if each individual report was wrong about the mechanism. No fidelity cost, and the existing tests prove it: `snapshot -> restore -> snapshot` byte idempotence and the 2,000-sample stream comparison both still pass, which they could not if a legitimate value were being clamped.
Owner
Author
|
Clamped — though I want to be accurate about why, since this is the third round It is still not a subscript: But the argument for clamping it does not depend on that. Every other field in Three reviewers reading the same code and landing on the same worry is decent No fidelity cost, and the existing tests prove that rather than assert it: |
# Conflicts: # CHANGELOG.md
Antigravity review (Gemini via Ultra)This PR appends the live OPLL synthesizer state to the VRC7 mapper save-state section (bumping it to v2) so that audio continuity is maintained across rewinds, while fully preserving backward and cross-feature compatibility. Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
doublegate
added a commit
that referenced
this pull request
Aug 19, 2026
#404) * docs(plans): add the v2.3.7 "Overtone" plan v2.3.7 had no plan doc. Every release since v1.3.0 has one under `to-dos/plans/`, and this one is unusual in that the release is already half-written: the VRC7 OPLL save-state defect closed in #398, and six further PRs sit merged in `[Unreleased]`. Writing the doc late means it has to RECORD those rather than propose them, or the next session rebuilds work that is already on `main`. WHAT THE DOC LEADS WITH, AND WHY IT IS NOT THE FEATURE Section A0 is "READ THIS FIRST", and it is about how Pixel Provenance shipped NON-FUNCTIONAL for four releases: run-ahead defaults to 1, and its per-frame rollback cleared the provenance store after the visible frame was harvested and before the UI could take the lock, while a comment two lines above asserted the opposite. Audio Provenance rides the same rollback and inherits the same hazard, so four things are marked non-negotiable -- carry the store around the rollback by extending the existing `take_provenance`/`put_provenance` path, reason about netplay rollback SEPARATELY because it uses the same `restore_quiet` but is not the same case, drive the real produce path at `run_ahead = 1` in a mutation-checked test, and let no comment claim a behaviour the code lacks. Putting the design first and the hazard in a footnote is how a project gets the same bug twice. WHAT NOT TO BUILD A reuse table maps each need to the mechanism that already exists: `set_attrib_context` already pushes PC + cycle down once per instruction, `EventKind::ApuWrite` already intercepts `$4000-$4017`, the PPU's `write_attrib` is the shape to copy, and `set_pixel_provenance` is the API shape. With the caveat that decides the design: `EventRec` carries no PC and no CPU cycle and is scanline-oriented rather than sample-oriented, so the event log is the interception POINT worth reusing and is not the record. Without that sentence the obvious move is to extend it, and the mismatch surfaces late. TWO THINGS THE ORIGINAL SPEC DID NOT HAVE Workstream B is recorded as DONE in detail, including the finding that hand-tracing located one of four reachable panics while a randomized sweep found the rest -- and that the single fixed all-`0xFF` payload CONCEALED one, because all-ones `update_requests` forced a recompute that hid it. That is written as a directive for workstream A rather than as history: if Audio Provenance grows any parse or restore surface, fuzz it instead of reasoning about it. And sizing is settled with the number rather than left to judgement: ~734 samples/frame at 44.1 kHz against 61,440 pixels/frame for the video store, 1.2%. The register-write side rides at 1.789 MHz and the sample side does not, which is the distinction that actually matters when bounding the store. VERIFIED Every claim was checked against the tree rather than carried over from the four-release plan: all nine `file:line` citations resolve to the code they name, the three line counts are exact, `rustynes-apu` genuinely declares only `std` today (so the first step is real work), both benches exist, and neither the panel nor `docs/audio-provenance.md` exists yet. markdownlint passes. Documentation only -- no code, and no CHANGELOG entry, since a plan doc is not user-visible behaviour. * feat(apu): audio provenance — the instruction behind every mixed cycle Point at a moment in the frame and read why it sounds like that. This is the APU counterpart of pixel provenance and deliberately the same shape: a per-register write attribution answering "what wrote this, and from which instruction", and a per-CPU-cycle mix trace answering "what were the channels actually doing". Surfaced at Tools -> Audio -> Audio Provenance. Output-only, runtime-default-off, and not serialized, so the deterministic audio contract is unaffected whether it is armed or not. ## Why this is not wiring up panels that already exist Every ingredient but one already shipped. audio_scope.rs plots the per-channel waveforms; audio_mixer.rs exposes per-channel gain; Apu::pulse1_out() and its siblings expose live channel outputs; the Trace Logger has PC and cycle; the Event Viewer already intercepts and classifies $4000-$4017 writes as EventKind::ApuWrite. What existed nowhere is the link between a sample and the instruction that caused it. EventRec carries kind / scanline / dot / addr / value -- no PC, no CPU cycle -- and it is scanline-oriented rather than sample-oriented. So the event log is the interception POINT this feature reuses; it is not the record. ## Cadence: per CPU cycle, and why that is the honest choice The mix is computed once per CPU cycle (1.789 MHz NTSC) and handed to blip, which decimates to 44,100 Hz -- roughly one output sample per 40.6 CPU cycles. The plan specified a PER-SAMPLE record at ~734 records/frame. The code disproved that estimate and it is corrected in the plan doc rather than quietly replaced. Recording at output rate would mean choosing which of those ~40 mixes "is" the sample, and band-limited synthesis makes that choice ill-posed: an output sample is a weighted sum of transitions across the filter kernel, not a copy of one instant. A tool that picked one anyway would be answering a question its own signal chain cannot answer, and would do it confidently. So the trace records what was genuinely mixed, at the cadence it was mixed: 29,781 records/frame NTSC against the pixel store's 61,440, which is 0.48x the video record count -- cheaper than the video side, not the 1.2% the estimate claimed. MIX_CAP is sized from DENDY (35,464 cycles/frame), not from the NTSC figure that comes to mind first; sizing it from NTSC would silently truncate the last 16% of every Dendy frame. Over the cap the trace reports truncated() rather than returning a short buffer that looks complete. ## Phase 1 -- register attribution One slot per address across $4000-$4017, each holding (value, cpu_cycle, pc). LAST WRITE, NOT A HISTORY. The question is "what is the register holding, and who put it there"; a ring would need a retention policy nobody has a principled value for, and the Event Viewer already keeps the per-frame write SEQUENCE. This keeps the per-register CAUSE, which it does not. A `written` flag rather than a sentinel cycle, because cycle 0 is a legitimate value: the reset sequence performs real writes, and a sentinel would misreport the earliest writes in a run as "never written". $4014 (OAM DMA) and $4016 (controller strobe) fall inside the range and are not APU registers. They are tracked anyway and labelled for what they are: the range is what the bus already classifies as ApuWrite, one contiguous index space costs two slots, and a hole would invite off-by-one arithmetic at every call site. The attribution is NOT cleared per frame -- "which instruction last wrote $4003" has an answer that legitimately predates this frame. It is cleared on a cold boot, where the history it describes genuinely ended. ## Phase 2 -- the mix trace Per CPU cycle: the five channel outputs that went into the mix, the expansion contribution, and the result. The index IS the cycle offset from first_cycle, so no per-record timestamp is stored. Channel values are the raw pre-mix outputs (0-15 for the pulses, triangle and noise; 0-127 for the DMC) -- what the non-linear mixer consumes. They are NOT scaled by the frontend's mixer gains: those are a presentation control, and recording post-gain values would make the record describe the user's slider rather than the chip. dominant() compares each channel's share of ITS OWN full scale, not raw magnitude, because the raw values are not commensurable -- a DMC 127 and a pulse 15 are both full scale on different scales. ## Phase 3 -- attribution plumbing The split follows the precedent pixel provenance set: the bus has the PC, the APU has the destination register, and the PC is pushed down once per instruction from the existing debug block in Nes::run_frame. rustynes-cpu is untouched. Both push-down sites are mirrored -- run_frame and step_instruction -- so single-stepping through a $4003 store in the debugger attributes the write to the stepped instruction rather than to whatever run_frame last left latched. Recording happens in Apu::write_register BEFORE the write dispatches, so the recorded value is what the CPU put on the bus rather than whatever a channel decided to keep. Both mix paths record: the v2.3.5 default-configuration fast specialization and the gated general path. A record that existed on only one of two byte-identical paths would be a trap for whoever next changed the other. ## The trap this feature inherited, closed up front Pixel provenance shipped NON-FUNCTIONAL for four releases (v2.3.2 through v2.3.6) because run-ahead's per-frame rollback cleared the store AFTER the visible frame was harvested and BEFORE the frontend released the emulator lock, so the UI could never observe a populated record. A comment two lines above the clear asserted the opposite, and that prose is what stopped anyone checking. Audio provenance rides the identical rollback. So the carry landed in the same change as the feature, not after a bug report: - Nes::take_audio_provenance / put_audio_provenance, called around restore_quiet in RunAhead::finish. - Save-state loads and netplay rollback still clear, unchanged -- those are genuine timeline changes. Run-ahead's rollback is not; it returns to the timeline it just left. - runahead_preserves_audio_provenance drives the real produce path at run_ahead = 1, the default, and looks at the first moment the UI could. Mutation-checked: dropping the stash turns it red. - A control test proves a plain run populates the trace, so a failure of the run-ahead test cannot be misread as a bad assertion. Both assertions are floored at 20,000 records rather than "non-empty", because the APU's 8-cycle reset sequence alone produces eight records -- a non-emptiness check would pass on a run that emulated nothing at all. One further note recorded because it cost time: a single run_frame immediately after from_rom can advance ZERO cycles, since the PPU starts at a frame boundary. The control runs three frames for that reason, exactly as the pixel-provenance control does. ## Phase 4 -- the panel The panel reads the CORE for the armed state every frame rather than keeping a mirror. The v2.3.6 pixel panel kept one and edge-detected on it, which desynced permanently the moment a ROM load installed a fresh Nes: checkbox ticked, core unarmed, no way back but unticking and re-ticking. It distinguishes three empty states rather than rendering one confident blank report: not armed, armed but nothing recorded yet, and trace truncated. Register rows carry their SIDE-BAND effects, because naming the right instruction and then describing the wrong effect is its own failure. A write to $4003 does not merely set the period -- it also loads the length counter, resets the duty sequencer, and restarts the envelope. Those annotations were confirmed against this emulator's own implementation (Pulse::write_timer_hi, Triangle::write_linear, Apu::write_status, and the $4017 alignment comment in Apu::write_register), not from memory. ## Workstream C -- the disarmed cost, measured three times The plan required re-running apu_throughput after the plumbing landed, to confirm the shipped default was unmoved. It was not unmoved, and the diagnosis in between was wrong. The configuration that matters is feature-compiled-in / arm-off, because crates/rustynes-frontend/Cargo.toml pulls rustynes-core with debug-hooks unconditionally. "Default-off" describes the runtime arm, not the code, so every user compiles this in and a feature nobody enabled can still charge them. C2a -- MixRecord was built BEFORE the arm test, so a disarmed build recomputed all five channel outputs every CPU cycle, and Pulse::output is not free (it calls muted(), which calls sweep_target()). Measured +14% to +23%. Fixed by hoisting the arm check to the top of the function. C2b -- with the check first, a quiet-host A/B still measured +9.2% / -2.0% / +9.7%. Diagnosed as Apu field-layout disturbance from four new inline fields (reg_attrib, mix_trace, attrib_pc, attrib_cycle) and fixed by consolidating them behind a single Option<Box<AudioProvenance>>. Re-measured: +7.98% / +2.88% / +11.03%, order-bias control +0.11% / +0.76% / +0.67%. THE DIAGNOSIS WAS WRONG. The consolidation is kept because one pointer is the better shape, but it is not what fixed anything, and the claim that it would is recorded rather than deleted. C2c -- the actual cause. The tell was in the numbers all along: the absolute costs were +33 us, +15 us and +65 us, wildly non-uniform. A branch taken once per CPU cycle costs a constant number of cycles and therefore a constant number of microseconds; it cannot vary four-fold across workloads. record_mix was still being INLINED into tick_with_external -- the arm check skipped the WORK, but the five output() calls were still emitted inside the hot function, inflating it past the point where the mixer and the channel ticks kept their register allocation and their I-cache line. The body is now outlined behind #[cold] + #[inline(never)], leaving exactly one null test on the hot path. Final measurement, disarmed, against an order-bias control that drifted -0.8% to -1.4% over the same interval: apu_tick_silent_frame -0.63% (control -1.41%) net +0.8%, within drift apu_tick_active_frame -5.60% (control -0.80%) net -4.8% apu_tick_active_frame_with_ext +0.00% (control -0.29%) net +0.3%, within drift (p = 0.99) The -4.8% is NOT claimed as an optimization. It is code-layout luck in the favourable direction, the same phenomenon that produced +11% in the unfavourable one, and an unrelated future change will erase it. The project bar is >3% same-runner AND byte-identical AND attributable to a mechanism; an effect nobody can point at a mechanism for does not meet it, and adopting it would be adopting noise. Method: apu_throughput built twice (with and without debug-hooks) with both binaries copied aside BEFORE any measurement, so no run is contaminated by its own compile -- the ab_check.sh hazard AGENTS.md records. Executed on a host held below 1.00 load, A -> B -> A, the trailing A being the order-bias control. full_frame was deliberately not run: a whole-frame bench dilutes an APU effect by roughly 5x, and both regressions were found at the instrument with the resolution to see them. Recorded as a deliberate omission, not an oversight. Three lessons carried into docs/performance.md: - A default-off feature can charge the default path without executing one line of its own code. Twice here, by two different mechanisms. - A branch that skips the work does not skip the code. An early return leaves the whole body inlined in the caller, costing registers and I-cache even though it never executes. - Non-uniform absolute deltas rule out a per-cycle mechanism. Read the microseconds before the percentages when deciding what KIND of cost you are looking at. ## Determinism and the save state Output-only throughout. Nothing recorded is read back into synthesis, and none of it is serialized. The new Apu field is registered in snapshot_schema_audit.rs as output-only with a written reason -- the audit caught all four original fields the moment they were added and refused to pass until they were classified. The attribution is deliberately NOT carried in a save state, for the same reason the PPU's write_attrib is not: a restored state's registers were not written by any instruction this session ran, so carrying PCs across a restore would report a timeline that no longer exists. ## Provenance Implemented from the NESdev wiki and from this repository's own APU implementation. No reference-emulator source was consulted; the register side-effect table was confirmed by reading rustynes-apu, not from memory. ## Files - crates/rustynes-apu/src/provenance.rs (new, 540 lines) -- RegWrite, Slot, RegisterAttribution (24 slots), MixRecord, MixTrace (MIX_CAP from Dendy), AudioProvenance, AudioProvenanceStash; 9 unit tests. - crates/rustynes-apu/src/apu.rs (+177) -- the audio_prov field, the outlined record_mix_armed, the write_register hook, and the arm/accessor API. - crates/rustynes-apu/Cargo.toml (+10) -- the debug-hooks feature. - crates/rustynes-core/{Cargo.toml,src/nes.rs} (+79) -- feature forwarding, the two mirrored push-down sites, per-frame re-anchoring, cold-boot clear, and the Nes-level API including the run-ahead stash pair. - crates/rustynes-frontend/src/debugger/audio_provenance_panel.rs (new, 305) -- the inspector, with the register side-effect table. - crates/rustynes-frontend/src/{debugger/mod.rs,ui_shell.rs} (+40) -- panel registration and the Tools -> Audio menu entry. - crates/rustynes-frontend/src/runahead.rs (+89) -- the stash carry around restore_quiet, plus the control and regression tests. - crates/rustynes-test-harness/tests/snapshot_schema_audit.rs (+14). - docs/audio-provenance.md (new, 265), docs/performance.md (+62), mkdocs.yml, CHANGELOG.md, to-dos/plans/v2.3.7-overtone-plan.md. ## Verification - cargo fmt --all --check: clean. - clippy -D warnings: workspace --all-targets; rustynes-apu with debug-hooks; rustynes-frontend with scripting, scripting+hd-pack, retroachievements, full; and BOTH wasm32-unknown-unknown invocations (default and wasm-canvas). - RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps: clean. - cargo build -p rustynes-core --target thumbv7em-none-eabihf --no-default-features: builds (no_std stays clean). - cargo test --workspace: 124 suites green, 0 failures. - cargo test -p rustynes-apu --features debug-hooks provenance: 9 passed. - cargo test -p rustynes-frontend audio_provenance: 2 passed. - cargo test -p rustynes-test-harness --test snapshot_schema_audit: 7 passed. - AccuracyCoin: 100.00% over 141 assigned tests via the authoritative RAM decoder (the framebuffer decoder's 120 is the known grid-stride bug). - nestest: nestest_pc_c000_matches_golden_log passing, 0-diff. This release touches rustynes-apu, so the accuracy gates are VERIFIED rather than true by construction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(apu): honest attribution for reset writes, and five review corrections Review of PR #404 found one behavioural defect in the attribution contract, one place where this feature's own documentation asserted behaviour the code did not have, and four inaccurate claims. All were real; one further suggestion is declined on the merits with its reasoning recorded. ## A reset write claimed an instruction caused it `Apu::reset` performs an internal `write_register($4015, 0)` modelling the warm-reset silencing of the channels. That reaches the attribution table through the ordinary CPU path, so it was stamped with whatever `attrib_pc` happened to be latched -- and the panel would then name a specific, innocent instruction as the cause of the one register a user looks at right after pressing Reset. A provenance tool producing a confident wrong answer is worse than no tool, and this is the exact failure the feature exists to prevent, reproduced by the feature itself. `RegWrite` gains a `WriteOrigin` (`Instruction` | `Reset`), and the panel prints "APU reset (not an instruction)" instead of a PC for the latter. The two alternatives were both worse and were rejected explicitly: suppressing the record would leave the slot advertising the register's PREVIOUS value after reset genuinely changed it, and a sentinel PC would be indistinguishable from a real write to address zero. `a_reset_write_is_not_attributed_to_an_instruction` asserts the origin, the value and the cycle separately, because they fail independently -- the origin can be corrected while the value goes stale. ## Prose that asserted behaviour the code did not have `docs/audio-provenance.md` stated that "save-state loads and netplay rollback still clear" the attribution. They did not. `Nes::restore_inner` cleared both PPU provenance stores and had no audio equivalent, so a restored state kept register attribution from a timeline that no longer existed -- the precise thing the surrounding paragraph condemns. Fixed by clearing it there, beside the two PPU clears. This is the same failure mode that let Pixel Provenance ship non-functional for four releases: a doc describing intent, and nobody checking the code because the text said it was fine. Second occurrence in this feature's short life, which is why the comment at the fix site says so. Harmless for run-ahead: `RunAhead::finish` takes the store BEFORE `restore_quiet` and puts it back after, so `audio_prov` is `None` at the clear and the call is a no-op on that path. ## The two mix paths were recording different things The gated general path had a post-gain `ext` in scope and recorded that; the default fast path recorded the raw `external`. So the two byte-identical paths disagreed -- exactly the divergence this feature's own "both paths record" rule exists to prevent, introduced in the change that wrote the rule. Resolved toward the documented semantic rather than toward the variable that happened to be in scope: both now record the RAW value, consistent with the five channel fields being raw pre-gate outputs. Review suggested zeroing it when the expansion mask bit is clear; that is declined because it would make ONE field follow the user's mixer sliders while five describe the chip. On a muted expansion channel the panel now reports what the cartridge produced, exactly as it reports a muted pulse's output rather than zero. ## Documentation corrections - The audio API block had been inserted between `set_pixel_provenance`'s doc comment and its signature, splitting one sentence across sixty lines and attaching three paragraphs about PIXEL provenance to the head of `set_audio_provenance`'s rustdoc. Block relocated after `pixel_provenance`; the split sentence rejoined. - `MixTrace`'s sizing comment conflated two different numbers. An NTSC frame of 29,781 records is ~465 KiB; the ALLOCATION is `MIX_CAP` records reserved up front, which is 576 KiB, because the cap is sized from Dendy. Both stated. - `dominant()` claimed to compare each channel's share of the NON-LINEAR mixer. It divides by each channel's own full scale, which is linear and gives a different ordering, since the mixer weights the TND group differently from the pulses and is not proportional in either. The computation is right and was wearing a false label; the label is corrected and what it does NOT answer is stated. - The NTSC row claimed a flat 29,781 CPU cycles/frame. Hardware alternates 29,780 and 29,781 -- the odd-frame skipped pre-render dot, which this repo already documents at its own frame-duration constant. 29,781 is the upper bound on a trace's record count, not a fixed figure. - The panel footer said registers with no row "have not been written since the last cold boot". Arming allocates a fresh table, so the true statement is "since audio provenance was enabled" -- which matters because everyone arms it mid-session. - A warning-sign emoji in a UI string, against the project's no-emoji rule (the same rule that blocked PR #385). Removed; the colored label already carries the state. - `CHANGELOG.md` carried the full three-finding measurement chronology. Module 40 says deep engineering narrative stays out of the changelog; trimmed to the user-visible outcome with a pointer to `docs/performance.md` §v2.3.7 C2. ## Gaps in my own verification, closed - The plan required `cargo clippy -p rustynes-apu --all-targets --no-default-features` and `cargo test -p rustynes-apu --no-default-features` precisely because this release adds a feature to a chip crate, and I had not run either. Both pass (151 tests). - `audio_expansion` -- the standing APU audio regression gate, 25 tests -- was missing from the verification list and had not been run. It passes, and it is now in both `docs/audio-provenance.md` and the plan. - The run-ahead regression test has TWO assertions and I had mutation-checked it once. A single mutation trips the first assertion and leaves the second vacuous, so both were checked separately: dropping the stash fails on "the rollback disarmed audio provenance", and re-arming with a fresh empty store passes that assertion and fails on "(0 records)". The control stayed green under both. - The panel's `pin` is ROM-bound state and was not registered with `clear_rom_bound_analysis`, whose own doc comment says the next ROM-bound panel is "one line away from being correct instead of one omission away from being wrong". I was the omission; it is registered now. `follow` is deliberately kept -- it is a display preference, not a measurement, and a ROM load should not quietly undo a setting the user chose. ## Declined Auto-disarming the core when the panel is closed. The sibling Pixel Provenance panel does not do this, and making one of two adjacent panels silently discard an arm the user deliberately set is worse than either uniform behaviour. If it should change it should change for both, as a deliberate change of its own -- the "treat it as a class" discipline this project already applies. ## Verification fmt clean; clippy `-D warnings` across the workspace, `rustynes-apu` with `debug-hooks` and with `--no-default-features`, four frontend feature combos, and both wasm32 invocations; rustdoc clean; `no_std` thumbv7em builds; 124 workspace suites green; `rustynes-apu` provenance 10 passed; frontend audio provenance 2 passed; `audio_expansion` 25 passed; AccuracyCoin 100.00% over 141 assigned tests via the authoritative RAM decoder; nestest 0-diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(frontend): iterate APU register addresses instead of casting indices The register table walked `REG_NAMES.iter().enumerate()` and rebuilt each address as `0x4000 + u16::try_from(i).unwrap_or(0)`. The fallback is the problem: an out-of-range index would fold silently onto `$4000` and render a WRONG row rather than none at all, in a panel whose entire purpose is to avoid answering wrongly. It cannot trigger today -- `REG_NAMES` is a fixed 24-entry array -- but "unreachable" and "harmless if reached" are different properties, and only one of them was true. Zipping the address range against the name table makes the bad case unrepresentable rather than merely unlikely, and reuses the `REG_BASE` / `REG_COUNT_U16` constants the attribution table already exports instead of repeating the `0x4000` literal at a second site. It also avoids the `as u16` the review suggested, which this workspace's clippy configuration denies via `cast_possible_truncation`. Reached the constants through `rustynes_core`'s re-export rather than depending on `rustynes-apu` directly, per the workspace rule that downstream consumers depend on `rustynes-core`. ## On the review's blocking finding: it is incorrect, and was verified so The Antigravity pass reported that `clear_audio_provenance_history` and `set_attrib_context` are called unconditionally in `Nes::reset`, `Nes::restore_quiet` and `Nes::run_frame`, and that this breaks `cargo build -p rustynes-core --no-default-features`. Checked rather than assumed, because a blocking claim deserves evidence either way: - Both `clear_audio_provenance_history` sites sit inside existing `#[cfg(feature = "debug-hooks")]` blocks, beside the two PPU clears. - The `run_frame` push-down is DOUBLY gated: `#[cfg(feature = "debug-hooks")]` on `if self.exec_logging`, and again on the inner block. - The exact command named builds clean, exit code 0. So does the CI cross form, `--target thumbv7em-none-eabihf --no-default-features`. The likely source of the confusion is a comment two lines above the call reading "Unconditional rather than gated on the store being armed" -- which is about the runtime ARM, not the feature. Declined on the evidence; the suggestion in the same review was taken, and taken further than proposed. ## Verification fmt clean; clippy `-D warnings` across the workspace, four frontend feature combos, and both wasm32 invocations; rustdoc clean; `rustynes-core` `--no-default-features` builds for both the host and thumbv7em; 124 workspace suites green; frontend audio provenance 2 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(apu): attribute $4014 and $4016, which the docs said were tracked The Antigravity reviewer flagged this on #404 as a suggestion, and it was a real defect: the two claims below were false. docs/audio-provenance.md:88 crates/rustynes-apu/src/provenance.rs (REG_COUNT doc comment) "$4014 (OAM DMA) and $4016 (controller strobe) ... are tracked anyway and labelled for what they are" They could not be. Attribution is recorded inside `Apu::write_register`, and `Bus::write` routes only 0x4000..=0x4013 | 0x4015 | 0x4017 => self.apu.write_register(..) to it. Both flagged addresses are handled entirely on the bus -- $4014 arms the OAM DMA burst, $4016 buffers the controller strobe to the next M2-low boundary -- so neither ever reached the recorder. Their reserved slots stayed permanently empty while three places said otherwise. This is the same shape as the Pixel Provenance failure: prose asserting behaviour the code does not implement, in the doc AND the source comment, which is why it read as intentional. Fixed by routing rather than by softening the prose, so the claims become true: `Apu::record_bus_handled_register_write` records the cause exactly as `write_register` would and dispatches nothing, called from both bus arms. The emulation of both addresses stays where the bus already implements it -- this only fills the slot. Both call sites and the method are `#[cfg(feature = "debug-hooks")]`, so the shipped default build does not contain this code at all, and the recorder only writes to the provenance table -- it cannot perturb emulation even when armed. MUTATION-CHECKED PER CALL SITE, not once. Removing the $4014 call fails with "$4014 must be attributed"; removing the $4016 call fails with the $4016 message; restoring passes. Mutating only one would have proven only half the test, which is the trap recorded in CLAUDE.local.md. Verified (run, not asserted, since this touches rustynes-core): AccuracyCoin 141/141 (100.00%) RAM decoder, authoritative nestest 1 passed fmt / clippy workspace + debug-hooks / no_std thumbv7em / rustdoc clean All gate results were read from exit codes rather than from empty output, per the standing rule that an absent signal is not a pass. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the most actionable open row in
docs/accuracy-ledger.md— open sincev2.2.3, latent since the ADR 0006 VRC7 audio landing.
Rewind a VRC7 game and the music came back wrong.
Vrc7::save_statewrotethe shadow OPLL register bytes and never the live synthesizer (
opll,opll_clock_counter,last_opll_sample), andload_statenever replayed themeither — so after a rewind, netplay rollback, or TAS/save-state restore the FM
voice resumed from an arbitrary point in an unrelated note. Banking, IRQ,
mirroring and PRG-RAM always round-tripped correctly; this was audio-only, and
only on mapper 85.
It was not serializable when it was found:
rustynes_apu::Opllexposed noserialization surface at all. That is what made it "a change of its own" rather
than a release-cut drive-by.
What lands
Opll::snapshot/Opll::restore(OPLL_SNAPSHOT_VERSION1, fixedOPLL_SNAPSHOT_LEN). Carried: the register shadow, the EG and LFO counters, theper-channel patch selection, the user patch pair, all 18 operator slots in full
(phase accumulators, envelope state machines, feedback history), and the
per-channel outputs.
Not carried, because they are constants of construction:
waves,tll_rks, andpatch_set[2..](the chip's patch ROM).chip_typeis written as a tag only— a YM2413 blob restored into a VRC7 is structurally valid in every field and
would silently reinterpret all 18 slot patches against the wrong instrument set,
so it is rejected instead.
The reader decodes the whole blob into locals before touching
self, so atruncated or hand-edited save leaves the synthesizer on its previous state rather
than half-overwritten. This parses untrusted input — a save state is a file on
disk. Enum tags are explicit
to_tag/from_tagmaps, notas u8, so reorderinga variant cannot reinterpret existing states.
VRC7 mapper section v2, appending the blob after the VRAM. Additive: a v1
blob still loads and leaves the synthesizer where the old build left it, so an
old save is no worse than it was rather than newly silent. A build without
mapper-audiohas no synthesizer to describe, so it writes v1 andvalidates-then-ignores a v2 tail — preserving the cross-feature portability
ADR 0004 promises, which is why the version byte is build-dependent.
The alternative, and why it lost
Replaying
audio.regsthroughOpll::write_regon load needs no new format andis the repair a reader thinks of first. It restarts every keyed-on channel's
envelope at attack, so every rewind frame produces an audible transient. The
ledger recorded that no oracle could adjudicate which was worse; carrying the
state verbatim removes the question, because it reproduces the sound that was
actually playing.
Tests
vrc7_save_state_carries_the_live_opll_so_audio_resumes_identicallykeys a note,advances 20,000 CPU cycles so the envelope is well past attack, saves, then
compares 4,000 mixed samples from the source against 4,000 from a fresh
mapper restored from the blob — equal sample for sample.
Mutation-checked: making the tail carry a reset synthesizer reproduces the
pre-fix failure on the first divergent sample.
Plus, at the
Oplllevel — a 2,000-sample stream round-trip into a fresh chip;snapshot → restore → snapshot byte idempotence (catches a field written but
not read back, which the stream test can miss); cross-chip rejection; truncation
rejection asserting the target is left unmutated; unknown-version rejection; a
corrupt envelope-state tag. At the mapper level — v1 back-compat and a truncated
v2 tail.
Opllis now registered insnapshot_schema_audit.rs. That audit found thev2.2.3 PPU and APU gaps mechanically, but it only knew about the three chips
inside the console, so it could not see this surface at all. A save-state surface
no audit can see is exactly how a gap this size survives four releases.
Registering it immediately caught two of my own exclusion entries as false
admissions (
chip_typeandpatch_setare written) — the audit working.Out of scope, checked and left alone:
nsf_expansion.rsalso holds anOpllandalso carries no phase, but that is a written decision with a stated rationale
(an NSF driver re-establishes channel state on the next play call), not an
undocumented gap.
Verification
Nothing on the synthesis path moved, so emulation output is unchanged — but this
touches
rustynes-apuandrustynes-mappers, so the contract was verified,not assumed:
cargo clippy --workspace --all-targets -- -D warningscargo clippy -p rustynes-mappers --no-default-features(audio off)RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-depsno_stdcross-build (thumbv7em-none-eabihf)