chore(info): savestate_features 2 -> 3, with the test that backs it - #327
Merged
Conversation
The .info claimed "serialized" (2) while retro_load_game reports zero
serialization quirks and its comment claims run-ahead. Level 3
("deterministic") is what zero quirks actually asserts, so the two now
agree.
Rather than take the code comment on trust, add
test/tools/test_runahead_determinism.c, which reproduces RetroArch's
run-ahead loop directly: warm up, serialize, run a window while
digesting every frame's video and audio, retro_unserialize back, and
replay the same window with the same (empty) input. It asserts four
things -- video digests match, audio digests match, the state
re-converges byte-for-byte, and two successive rollbacks agree with
each other.
Measured on Iron Soldier, Skyhammer and Kasumi Ninja (240 warmup / 120
window), all three agree:
PASS video_replay_identical 120/120 frames bit-identical
FAIL audio_replay_identical 1 frame differs
PASS state_reconverges 2490368 bytes, byte-identical
PASS repeated_rollback_agrees pass 2 == pass 3, video and audio
The one failure is the first frame after the first rollback, off by
~0.05% RMS (Iron Soldier: 2079.86 vs 2080.96) with an identical sample
count; frames 1..119 match bit for bit. Successive rollbacks agree
completely including that frame, which is the regime run-ahead actually
runs in -- it rolls back every frame, so it never sees the transition.
yarc.j64 passes all four.
So some audio state is re-derived rather than restored by the load
path. The I2S ring buffer is ruled out: DACPrepareFrame reseeds
writePos/writeCount to 2 every frame and DSPSampleCallback clamps its
read index to i2sWriteCount, so no sample from a previous frame can be
read. The culprit is elsewhere in the DSP/DAC path.
The test is deliberately NOT in `make test` while that assertion fails;
it gets its own `make runahead-determinism` target. Wire it into the
suite once the gap is closed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
tests
Regression:
|
| ROM | Status | Details | Diff |
|---|---|---|---|
| jagniccc | ✅ PASS | 0 pixels differ | - |
| yarc | ✅ PASS | 0 pixels differ | - |
| jagniccc (determinism) | ✅ PASS | identical across runs | - |
| yarc (determinism) | ✅ PASS | identical across runs | - |
| jagniccc (frameskip) | ✅ PASS | skip=0 matches skip=3 | - |
| yarc (frameskip) | ✅ PASS | skip=0 matches skip=3 | - |
| jagniccc (save state) | ✅ PASS | round-trip matches | - |
| yarc (save state) | ✅ PASS | round-trip matches | - |
| jagniccc (rewind) | ✅ PASS | rewind matches | - |
| yarc (rewind) | ✅ PASS | rewind matches | - |
Platform: Darwin arm64
Updated by CI at 2026-08-06T02:11:58.851Z
Regression:
|
| ROM | Status | Details | Diff |
|---|---|---|---|
| jagniccc | ✅ PASS | 0 pixels differ | - |
| yarc | ✅ PASS | 0 pixels differ | - |
| jagniccc (determinism) | ✅ PASS | identical across runs | - |
| yarc (determinism) | ✅ PASS | identical across runs | - |
| jagniccc (frameskip) | ✅ PASS | skip=0 matches skip=3 | - |
| yarc (frameskip) | ✅ PASS | skip=0 matches skip=3 | - |
| jagniccc (save state) | ✅ PASS | round-trip matches | - |
| yarc (save state) | ✅ PASS | round-trip matches | - |
| jagniccc (rewind) | ✅ PASS | rewind matches | - |
| yarc (rewind) | ✅ PASS | rewind matches | - |
Platform: Linux aarch64
Updated by CI at 2026-08-06T02:11:50.417Z
Regression:
|
| ROM | Status | Details | Diff |
|---|---|---|---|
| jagniccc | ✅ PASS | 0 pixels differ | - |
| yarc | ✅ PASS | 0 pixels differ | - |
| jagniccc (determinism) | ✅ PASS | identical across runs | - |
| yarc (determinism) | ✅ PASS | identical across runs | - |
| jagniccc (frameskip) | ✅ PASS | skip=0 matches skip=3 | - |
| yarc (frameskip) | ✅ PASS | skip=0 matches skip=3 | - |
| jagniccc (save state) | ✅ PASS | round-trip matches | - |
| yarc (save state) | ✅ PASS | round-trip matches | - |
| jagniccc (rewind) | ✅ PASS | rewind matches | - |
| yarc (rewind) | ✅ PASS | rewind matches | - |
Platform: Linux x86_64
Updated by CI at 2026-08-06T02:11:47.788Z
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the core metadata to claim deterministic save states (savestate_features = "3") and adds a new diagnostic test + Makefile target intended to validate run-ahead style determinism via repeated serialize/unserialize replays.
Changes:
- Add
test/tools/test_runahead_determinism.cto replay windows across rollbacks and compare per-frame video/audio fingerprints. - Add
make runahead-determinismtarget to build/run the new determinism tool. - Update
dist/info/virtualjaguar_libretro.infotosavestate_features = "3".
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/tools/test_runahead_determinism.c | New determinism test tool that mimics RetroArch run-ahead rollback/replay and compares frame digests. |
| Makefile | Adds a runahead-determinism phony target and build/run recipe for the new tool. |
| dist/info/virtualjaguar_libretro.info | Updates advertised savestate feature level from 2 to 3. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+111
to
+112
| /* Raw per-frame sample count and RMS, kept alongside the digests so a | ||
| * failure can say WHAT differed, not just that something did. */ |
Comment on lines
+420
to
+424
| if (cap.count != pass1_count) { | ||
| first_video_diff = 0; | ||
| } else { | ||
| for (i = 0; i < pass1_count; i++) { | ||
| if (cap.video[i] != pass1_video[i] && first_video_diff < 0) |
Comment on lines
+296
to
+304
| char state_a[] = "/tmp/vj_runahead_a.state"; | ||
| char state_b[] = "/tmp/vj_runahead_b.state"; | ||
| char state_c[] = "/tmp/vj_runahead_c.state"; | ||
| uint8_t *blob_b = NULL, *blob_c = NULL; | ||
| size_t len_b = 0, len_c = 0; | ||
| int state_stable = 0; | ||
| char detail_v[256], detail_a[256], detail_s[256], detail_r[256]; | ||
| int failed = 0; | ||
|
|
Comment on lines
+386
to
+387
| /* 7. State C. */ | ||
| harness_save_state(&cfg, state_c); |
Comment on lines
+1461
to
+1467
| # replays the same frames after retro_unserialize, and asserts the video and | ||
| # audio come back identical. This is the evidence behind | ||
| # `savestate_features = 3` in dist/info/, and behind reporting zero | ||
| # serialization quirks: run-ahead, rewind and netplay all assume a state is a | ||
| # complete snapshot. | ||
| # | ||
| # NOT part of `make test`: one assertion (audio_replay_identical) is a known |
Narrows the known audio_replay_identical failure without fixing it yet. dspFlagsRetireDelay/dspPreStoreBank looked like the answer -- audio-path state that DSPStateLoad explicitly retires instead of restoring, which produces exactly the observed pass-1-differs / pass-2==pass-3 shape. It was implemented behind a state-version gate and measured: no change on Iron Soldier, Skyhammer or Kasumi Ninja. The retire window is two instruction slots wide, so a frame boundary practically never lands inside it. Reverted rather than kept -- it changed the save-state format for no measurable benefit. Also ruled out and recorded: the event queue (all nine callbacks are in event.c's registry, nothing is dropped by the pointer/id round trip), jaguar_prng_state (init-only, never advances during retro_run), dspgo_poll_count (pinned to 0 while audio is non-silent), and the 68K bus/scale counters (balanced or 1x-inert). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> EOF
The DAC register file lives in jagMemSpace at $F1A148-$F1A157, and no STATE_SAVE_BUF covered it. retro_serialize saves jaguarMainRAM (the low 2 MB of jagMemSpace), tomRam8 and jerry_ram_8 -- and jerry_ram_8 is a SEPARATE array in jerry.c, not the $F10000 window of jagMemSpace. So LTXD, RTXD, SCLK, SMODE, LRXD, RRXD and SSTAT survived a load only by accident, as whatever the previous run happened to leave behind. DACPrepareFrame seeds the resampler's interpolation endpoints from LTXD/RTXD every frame, so the first frame after retro_unserialize started from the wrong endpoints and then converged as real DSP writes landed. Measured as a ~0.05% RMS difference on exactly one frame (2079.86 vs 2080.96 on Iron Soldier), identical sample count, reproducible on Iron Soldier, Skyhammer and Kasumi Ninja. Found by instrumenting DACPrepareFrame and diffing a natural run against a post-rollback replay field by field: every DSP field matched (pc, control, flags, acc, pipeline pointers, scoreboard, registers) and only LTXD/RTXD differed. That diff is what turned a week's worth of plausible suspects into a one-line answer -- see the test header for the four candidates that looked guilty and were not. Fields are appended to the DAC block behind STATE_VERSION_DAC_REGISTERS (v8); older layouts consume nothing and keep their existing behaviour. SCLK/SMODE feed the resample ratio, so the loader re-derives it via DACUpdateSCLKRate once they hold restored values. test/tools/test_runahead_determinism now passes all four assertions on Iron Soldier, Skyhammer, Kasumi Ninja and yarc, so it joins `make test` (yarc unconditionally, Iron Soldier when the private corpus is present). test_state_compat's deliberate DAC-block-size tripwire fired at 37 -> 51 bytes, exactly as designed; its expected size and layout comment are updated. The v1/v2/v3 fixture offsets are unaffected -- the new fields are appended, not interleaved. Audio pair re-verified per CLAUDE.md: Iron Soldier 1 presence RMS 1175.7 (develop baseline ~1175, envelope 200-25000), Skyhammer clipping 0 saturation runs. Full suite exits 0 with no skipped checks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
the
core
Collaborator
Author
Root-caused and fixed — all four assertions now passThe instrumented diff pinned it immediately. Dumping every input the frame's audio is derived from at Every DSP field was identical — pc, control, flags, accumulator, modulo, remainder, bank, Z/N/C, pipeline pointers, scoreboard, r0/r1/r30/r31. Only LTXD/RTXD differed. The bug
So the entire DAC register file (LTXD, RTXD, SCLK, SMODE, LRXD, RRXD, SSTAT) was in no saved region. It survived a load only by accident, as whatever the previous run left behind.
FixSerialize them in VerificationAll four assertions green on Iron Soldier, Skyhammer, Kasumi Ninja and yarc: The test now joins
Audio pair re-verified per CLAUDE.md, since this touches What this closes
Four candidates that looked guilty and were not (the D_FLAGS retire delay was implemented, measured, and reverted) are recorded in the test header so nobody re-spends the time. |
This was referenced Aug 7, 2026
JoeMatt
added a commit
that referenced
this pull request
Aug 14, 2026
* ci: add CodeRabbit config and a Kimi second-opinion PR reviewer Copilot review is unavailable — every recent PR got "Copilot was unable to review this pull request because the user who requested the review has reached their quota limit", which is silence, not approval. Two independent reviewers so one running dry is not the same as a PR going unreviewed. .coderabbit.yaml — tuned to what actually breaks here rather than generic defaults. Per-path instructions for the C89/MSVC rules (mid-block declarations first), the big-endian GET/SET macro contract, "never trust a source comment for hardware behaviour, cite the JTRM or a netlist", the savestate-field trap behind #327/#400, the audio pair rule from PR #170, and the CI ephemeral-port flake (#356). Generated and vendored trees are excluded from review: cpuemu.c alone is ~1.8 MB of machine-generated 68K dispatch, and the bios sources are bin2c hex tables. The exemption list matches scripts/c89-lint.sh's skip_file. .github/workflows/kimi-review.yml + .github/scripts/kimi_review.py — dependency -free second opinion using the KIMI_KEY_* secrets. Runs on PRs into develop/master and on "@Kimi review". Self-contained rather than a third-party action so the auth shape stays visible in one file and there is no action version to drift. Assumption worth flagging: KIMI_KEY_VALUE is sent as the bearer token and KIMI_KEY_ID as an identifying header. If the provider issues an AK/SK pair needing a signed request, the job prints an explicit 401 diagnostic naming the one function to change. Endpoint and model are overridable via the KIMI_API_BASE and KIMI_MODEL repo variables without touching code. The job fails SOFT throughout: an advisory review must never red-X a PR that compiles and passes its tests. Verified locally: python compiles, workflow YAML parses, and the diff filter was exercised against two real commits -- it passes #442's five files through and reduces the generated-matrix commit 3c4992d to nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci(kimi): point at the Kimi for Coding endpoint, not the Moonshot one The first live run 401'd with "Invalid Authentication". The key was fine and the protocol shape was fine -- the host was wrong. Keys minted in the Kimi Code console (kimi.com/code/console) belong to the "Kimi for Coding" SUBSCRIPTION, which is served from api.kimi.com/coding and is a different product from the pay-per-token Moonshot platform at api.moonshot.ai. A key from one 401s against the other. Defaults now: KIMI_API_BASE https://api.kimi.com/coding/v1 (OpenAI-compatible base; an Anthropic-compatible base exists at .../coding/) KIMI_MODEL k3-256k Both remain repo variables, so retargeting needs no code change. The 401 and 400/404 diagnostics now name the specific cause -- which host a key belongs to, and the tier-dependent model list (k3, k3-256k, kimi-for-coding, kimi-for-coding-highspeed) -- so the next misconfiguration is self-describing instead of a bare status code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- 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.
Spotted while updating the libretro/docs page:
dist/info/claimedsavestate_features = "2"("serialized") whileretro_load_gamereports zero serialization quirks and its comment claims run-ahead. Zero quirks is the level-3 ("deterministic") assertion, so the two disagreed. This makes them agree — but verifies the claim first rather than trusting the comment.The test
test/tools/test_runahead_determinism.creproduces RetroArch's run-ahead loop directly: warm up, serialize, run a window while digesting every frame's video and audio,retro_unserializeback, replay the same window with the same (empty) input — then roll back and replay a third time.That third pass is the load-bearing one. Pass 1 is the only pass reached without an intervening
retro_unserialize, so anything the load path fails to restore shows up as a pass-1-vs-pass-2 difference and nowhere else. Run-ahead rolls back on every frame, so it lives entirely in the post-rollback regime that pass 2 vs pass 3 measures.Results
Identical on Iron Soldier, Skyhammer and Kasumi Ninja (240 warmup / 120 window).
yarc.j64passes all four.The single failure is the first frame after the first rollback, off by ~0.05% RMS (Iron Soldier: 2079.86 vs 2080.96) with an identical sample count. Frames 1..119 match bit for bit, and successive rollbacks agree completely including that frame.
So the level-3 claim holds for everything run-ahead actually depends on, and there is a real but bounded gap: some audio state is re-derived rather than restored by the load path.
What is already ruled out
The I2S ring buffer, despite being absent from
DACStateSave.DACPrepareFramereseedsi2sWritePos/i2sWriteCountto 2 every frame, andDSPSampleCallbackclamps its read index toi2sWriteCount— so no sample from a previous frame can ever be read. The culprit is elsewhere in the DSP/DAC path.Wiring
The test gets its own
make runahead-determinismtarget and is deliberately not inmake testwhile that assertion fails — a permanently-red suite entry is worse than no entry. Wire it in once the gap is closed.make runahead-determinism RUNAHEAD_ROM="path/to/game.j64"No core source is touched by this PR — only the
.infovalue, the Makefile target, and the new test.