docs(adr-0013): hi-res needs an extraction rule, not a wider region - #321
Conversation
The C5.15/C10.04/C9 hi-res rows were parked behind "widen the capture region past 256x224". That framing is wrong, and the measurement showing why is worth recording before anyone builds to it. A minimal Mode 5 frame was rendered as a throwaway scene and each host asked what it emits: snes9x (libretro) 256x224 ordinary -> 512x224 width doubled Mesen2 (Lua) 256x239 ordinary -> 512x478 width AND height The two references do not agree on the SHAPE of a hi-res frame, never mind its pixels: Mesen2 line-doubles and snes9x does not. A contract that simply said "512 wide now" would compare a 224-row picture against a 478-row one. With the earlier finding that even/subscreen columns agree within 0.4-3% across all three references while odd/mainscreen columns diverge 33-35% pairwise, the answer is: take even columns, take even rows from Mesen2, and get a 256x224 sample back. The REGION SIZE DOES NOT CHANGE. What a hi-res scene needs is a declared per-scene EXTRACTION, and `Scene` gaining a field that names it. This measurement was only possible because the hosts now reject an out-of-contract geometry loudly instead of silently hashing it (#320) -- the rejection is what printed the table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughUpdates ADR-0013 with host-specific hi-res framebuffer geometry, per-scene extraction modes, canonical 256×224 sampling, golden blocking for mainscreen disagreement, and exact framebuffer validation before hashing. ChangesFramebuffer oracle documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates ADR 0013 (AccuracySNES framebuffer oracle) to document why Mode 5 hi-res scenes should be handled via a per-scene extraction rule (normalizing host output back to a canonical 256×224 sample) rather than by widening the capture region.
Changes:
- Adds a dated supplement capturing measured hi-res framebuffer geometries from snes9x (libretro) vs Mesen2 (Lua).
- Records the implication that hi-res needs an extraction rule (even columns; and for Mesen2 also even rows) to recover a consistent 256×224 sample.
- Documents that out-of-contract geometry must be rejected loudly (and notes this prerequisite is now satisfied).
Suppressed comments (2)
docs/adr/0013-accuracysnes-framebuffer-oracle.md:139
- These table entries use
xfor dimensions (256x224,512x478), but the documentation elsewhere uses×for pixel dimensions (e.g.docs/libretro.md:38). Using×here would keep resolution notation consistent across the docs.
| host | ordinary frame | Mode 5 hi-res frame |
|---|---|---|
| snes9x (libretro) | 256x224 | **512x224** — width doubled |
| Mesen2 (Lua) | 256x239 | **512x478** — width **and height** doubled |
docs/adr/0013-accuracysnes-framebuffer-oracle.md:155
- More instances of
256x224appear in the extraction summary; the rest of the docs use×for resolutions (e.g.docs/libretro.md:38). Switching to×here keeps resolution notation consistent.
- from Mesen2, additionally take the **even rows**, since its 478 is a line-double of 239;
- which yields a 256x224 sample again.
So the region size does not change at all. What a hi-res scene needs is a **declared per-scene
extraction** — a rule for turning whatever the host emits into the canonical 256x224 sample — and
| The `C5.15`, `C10.04` and `C9` hi-res rows were parked behind "widen the capture region past | ||
| 256x224". **That framing is wrong**, and the measurement that shows why is worth recording before | ||
| anyone builds to it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0013-accuracysnes-framebuffer-oracle.md`:
- Around line 151-152: Clarify the Mesen2 row-cropping description in the ADR by
specifying the exact 224-row window retained after selecting even rows from the
478-row frame, and relate that window to the FIRST_ROW and SCENE_H constants
used by the accuracysnes scene harness. Ensure the prose matches the
implementation so all hosts derive the same hash.
- Around line 160-164: Update the geometry-validation claim around
capture_scenes and hash_scene: reject framebuffers whose width or height differs
from the required geometry, including oversized heights such as 256×239, before
hashing. Implement the documented even-column extraction for wider inputs per
scene, or explicitly mark the paragraph as a pending requirement until
extraction and exact validation are implemented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cce03a72-c7f8-4a42-af28-c81fec723d46
📒 Files selected for processing (1)
docs/adr/0013-accuracysnes-framebuffer-oracle.md
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (7)
docs/**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Before changing a subsystem, consult
docs/architecture.md,docs/STATUS.md,CONTRIBUTING.md, the relevant subsystem documentation, and applicable ADRs.New subsystems must add documentation under
docs/.
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.md
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Chip-behavior changes must update both the chip implementation and the corresponding
docs/<subsystem>.mddocumentation.A chip change must update both the chip implementation and its corresponding
docs/<chip>.mddocumentation in the same change.
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.md
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Do not commit or vendor the generatedsnesdev_wiki/mirror; it is gitignored and intended only as a local reference.
Keep commits focused and use Conventional Commits:<type>(<scope>): <subject>, with an imperative subject of at most 72 characters.
Do not use emojis in code, comments, or commit messages.
Before opening a PR, ensure formatting, Clippy, workspace tests, the core embedded build, rustdoc with warnings denied, documentation coverage, and changelog requirements pass.
Ticket completion must be reflected in the relevantto-dos/sprint file.
**/*: Preserve the one-directional crate graph: chip crates must not depend on one another;rustysnes-coreties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keepdocs/STATUS.mdas the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNESv2.0orengine-lineageanchors as project releases.
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.md
docs/adr/**/*
📄 CodeRabbit inference engine (docs/adr/0004-determinism-contract.md)
docs/adr/**/*: The emulator must produce bit-identical framebuffer and audio for the same seed, ROM, and input sequence.
Power-on CPU, PPU, and SMP phase alignment must come from a seeded PRNG, and reset must preserve that alignment.
Track the SPC700 asynchronous domain using the integer relative-time accumulator defined by ADR 0001; do not use floating-point timing for this purpose.
The core must not use system time, thread scheduling, or OS RNG as sources of nondeterminism.
Use a fixed nominal 1.024 MHz SPC domain in the deterministic path; resonator drift must be excluded. Any hardware-accurate drift toggle belongs in the frontend, is off by default, and must remain outside the bit-identical path.
RTC chips such as S-RTC and SPC7110's RTC-4513 must use fixed or seeded time and must never read the host wall clock.
Rate control and run-ahead must be implemented in the frontend, never in core synthesis; netplay rollback must be frontend-orchestrated against the deterministic core.
Core code must not introduce hidden nondeterminism, including host time, thread scheduling, OS RNG, or HashMap iteration order.
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.md
docs/**/*
📄 CodeRabbit inference engine (docs/testing-strategy.md)
Chip crates should exceed 90% unit-test coverage, and each chip should be fuzzable in isolation.
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.md
docs/**
⚙️ CodeRabbit configuration file
docs/**: Docs are the spec, not a history log. Flag claims that contradict the code, counts that
contradict the generateddocs/accuracysnes-coverage.md, and any statement of coverage that
is broader than what the corresponding test actually asserts.
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Docs are the spec, not a changelog. Flag prose that has drifted from the code it describes
rather than style nits. The markdownlint gate is pinned to v0.39.0 via pre-commit —
do not report rules that version does not have (MD060 in particular).
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.md
🔇 Additional comments (2)
docs/adr/0013-accuracysnes-framebuffer-oracle.md (2)
127-150: LGTM!
153-159: LGTM!
| The prerequisite for any of this was that a host must *reject* an out-of-contract geometry rather | ||
| than silently hash it. Both hosts' checks were lower bounds until 2026-08-02 — they caught a frame | ||
| that was too small and passed one that was too large, hashing a diagonal slice (Mesen2's Lua, from a | ||
| 256 stride over a 512-wide buffer) or the leftmost 256 columns (the libretro host, which uses the | ||
| real pitch). That is fixed; the loud rejection above is what produced this table. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'fn hash_scene|fn capture_scenes|assert_eq!\(|fb\.len\(\)|visible_width|visible_height' \
crates/rustysnes-test-harness/tests/accuracysnes_scenes.rs
fd -t f -e rs -e lua . | xargs -r rg -n -C 6 \
'512|478|extract|framebuffer|hash_scene'Repository: doublegate/RustySNES
Length of output: 50377
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
target="crates/rustysnes-test-harness/tests/accuracysnes_scenes.rs"
printf '%s\n' '--- target outline ---'
ast-grep outline "$target" | rg 'hash_scene|capture_scenes|fn |const ' || true
printf '%s\n' '--- relevant definitions and call sites ---'
rg -n -C 18 'fn (hash_scene|capture_scenes)|hash_scene\(' "$target"
printf '%s\n' '--- geometry-related constants and framebuffer access ---'
rg -n -C 8 'SCENE_W|SCENE_H|VISIBLE|width|height|pitch|framebuffer|fb\.len' "$target"
printf '%s\n' '--- referenced ADR paragraph ---'
sed -n '145,175p' docs/adr/0013-accuracysnes-framebuffer-oracle.mdRepository: doublegate/RustySNES
Length of output: 14431
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- all harness files ---'
fd -t f -e rs . crates/rustysnes-test-harness
printf '%s\n' '--- exact symbols across the harness ---'
rg -n -C 12 'hash_scene|capture_scenes|SCENE_W|SCENE_H|visible_width|visible_height' crates/rustysnes-test-harness
printf '%s\n' '--- host-side geometry and extraction code ---'
rg -n -C 10 '256|224|239|512|448|478|pitch|stride|extract|slice|framebuffer' \
crates/rustysnes-test-harness --glob '*.rs' --glob '*.lua'Repository: doublegate/RustySNES
Length of output: 50377
Reject oversized heights and implement extraction before claiming geometry validation is fixed.
capture_scenes passes the raw framebuffer to hash_scene. A 256×239 framebuffer passes the width and length checks, so hash_scene hashes its first 224 rows without rejecting the oversized height. A 512-wide framebuffer triggers the width assertion before any documented even-column extraction. Add per-scene extraction and exact geometry validation, or mark this paragraph as a pending requirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adr/0013-accuracysnes-framebuffer-oracle.md` around lines 160 - 164,
Update the geometry-validation claim around capture_scenes and hash_scene:
reject framebuffers whose width or height differs from the required geometry,
including oversized heights such as 256×239, before hashing. Implement the
documented even-column extraction for wider inputs per scene, or explicitly mark
the paragraph as a pending requirement until extraction and exact validation are
implemented.
Source: Path instructions
Bot review, both suggestions accepted; no blocking issues. The extraction rule is now specified rather than gestured at: a closed `extract` enum on `Scene` (`Direct` | `HiResEven`), emitted as a COLUMN in build/scenes.tsv so the rule travels with the scene instead of three hosts each hard-coding their own -- the same reasoning that makes FIRST_ROW a declared per-host constant. A host meeting an `extract` value it does not implement must REJECT the scene, never fall back to `Direct`, which would silently hash the left half of a hi-res picture and is the exact failure this supplement exists to prevent. The "that is fixed" claim now names PR #320 and the two call sites, so the doc cannot drift from the code it depends on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both accepted; no blocking issues.
Worth adding explicitly, because it is the failure mode the whole supplement exists to prevent: a host that meets an |
Antigravity review (Gemini via Ultra)This PR adds a supplement to ADR-0013 documenting the decision to handle Mode 5 hi-res frames via a per-scene extraction rule ( Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0013-accuracysnes-framebuffer-oracle.md`:
- Around line 163-166: Clarify the `HiResEven` definition in the extraction
table by specifying the exact 512x478 source-row window and whether `FIRST_ROW`
and `SCENE_H` are applied before or after even-row decimation, yielding exactly
224 canonical rows. Align the host extraction implementations and
`build/scenes.tsv` semantics with this documented rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 70dd4441-4609-457e-8653-28ac57c88aa2
📒 Files selected for processing (1)
docs/adr/0013-accuracysnes-framebuffer-oracle.md
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (7)
docs/**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Before changing a subsystem, consult
docs/architecture.md,docs/STATUS.md,CONTRIBUTING.md, the relevant subsystem documentation, and applicable ADRs.New subsystems must add documentation under
docs/.
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.md
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Chip-behavior changes must update both the chip implementation and the corresponding
docs/<subsystem>.mddocumentation.A chip change must update both the chip implementation and its corresponding
docs/<chip>.mddocumentation in the same change.
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.md
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Do not commit or vendor the generatedsnesdev_wiki/mirror; it is gitignored and intended only as a local reference.
Keep commits focused and use Conventional Commits:<type>(<scope>): <subject>, with an imperative subject of at most 72 characters.
Do not use emojis in code, comments, or commit messages.
Before opening a PR, ensure formatting, Clippy, workspace tests, the core embedded build, rustdoc with warnings denied, documentation coverage, and changelog requirements pass.
Ticket completion must be reflected in the relevantto-dos/sprint file.
**/*: Preserve the one-directional crate graph: chip crates must not depend on one another;rustysnes-coreties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keepdocs/STATUS.mdas the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNESv2.0orengine-lineageanchors as project releases.
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.md
docs/adr/**/*
📄 CodeRabbit inference engine (docs/adr/0004-determinism-contract.md)
docs/adr/**/*: The emulator must produce bit-identical framebuffer and audio for the same seed, ROM, and input sequence.
Power-on CPU, PPU, and SMP phase alignment must come from a seeded PRNG, and reset must preserve that alignment.
Track the SPC700 asynchronous domain using the integer relative-time accumulator defined by ADR 0001; do not use floating-point timing for this purpose.
The core must not use system time, thread scheduling, or OS RNG as sources of nondeterminism.
Use a fixed nominal 1.024 MHz SPC domain in the deterministic path; resonator drift must be excluded. Any hardware-accurate drift toggle belongs in the frontend, is off by default, and must remain outside the bit-identical path.
RTC chips such as S-RTC and SPC7110's RTC-4513 must use fixed or seeded time and must never read the host wall clock.
Rate control and run-ahead must be implemented in the frontend, never in core synthesis; netplay rollback must be frontend-orchestrated against the deterministic core.
Core code must not introduce hidden nondeterminism, including host time, thread scheduling, OS RNG, or HashMap iteration order.
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.md
docs/**/*
📄 CodeRabbit inference engine (docs/testing-strategy.md)
Chip crates should exceed 90% unit-test coverage, and each chip should be fuzzable in isolation.
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.md
docs/**
⚙️ CodeRabbit configuration file
docs/**: Docs are the spec, not a history log. Flag claims that contradict the code, counts that
contradict the generateddocs/accuracysnes-coverage.md, and any statement of coverage that
is broader than what the corresponding test actually asserts.
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Docs are the spec, not a changelog. Flag prose that has drifted from the code it describes
rather than style nits. The markdownlint gate is pinned to v0.39.0 via pre-commit —
do not report rules that version does not have (MD060 in particular).
Files:
docs/adr/0013-accuracysnes-framebuffer-oracle.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: doublegate/RustySNES
Timestamp: 2026-08-02T04:17:41.094Z
Learning: Rendered-scene results must be reported separately as rendered-scene matches and must not alter the headline on-cart scoring figure.
Learnt from: CR
Repo: doublegate/RustySNES
Timestamp: 2026-08-02T04:17:41.094Z
Learning: Rendered tests should gate CI only after their scenes have been cross-validated and have committed goldens; informational, non-failing rendered checks are not sufficient.
🔇 Additional comments (1)
docs/adr/0013-accuracysnes-framebuffer-oracle.md (1)
155-161: LGTM!Also applies to: 168-172, 174-180
| | `extract` | meaning | | ||
| |---|---| | ||
| | `Direct` | today's behaviour — the frame is 256 wide, take `SCENE_H` rows from `FIRST_ROW`. Every existing scene, and the default, so no golden moves. | | ||
| | `HiResEven` | the frame is 512 wide: take even columns, and even rows as well on a host whose height also doubled. Yields the same canonical 256x224 sample. | |
There was a problem hiding this comment.
Define the Mesen2 row crop for HiResEven.
When Mesen2 emits a 512x478 frame, selecting even source rows produces 239 rows, not 224. This definition does not state which 224 rows remain or whether FIRST_ROW and SCENE_H apply before or after decimation. Different hosts can therefore produce different canonical hashes.
Specify the exact source-row window and indexing rule, then keep the host implementations and build/scenes.tsv semantics aligned.
As per path instructions: docs are the specification, so the crop rule must be explicit here.
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- scene geometry constants and extraction logic ---'
rg -n -C 12 \
'FIRST_ROW|SCENE_H|HiResEven|extract|512|478|224|239' \
crates/rustysnes-test-harness --glob '*.rs' --glob '*.lua' || true
printf '%s\n' '--- all serialized extraction contracts ---'
rg -n -C 12 \
'HiResEven|build/scenes.tsv|extract|FIRST_ROW|SCENE_H' \
. --glob '*.rs' --glob '*.c' --glob '*.lua' --glob '*.md' || true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adr/0013-accuracysnes-framebuffer-oracle.md` around lines 163 - 166,
Clarify the `HiResEven` definition in the extraction table by specifying the
exact 512x478 source-row window and whether `FIRST_ROW` and `SCENE_H` are
applied before or after even-row decimation, yielding exactly 224 canonical
rows. Align the host extraction implementations and `build/scenes.tsv` semantics
with this documented rule.
Source: Path instructions
The
C5.15,C10.04andC9hi-res rows have been parked behind "widen the capture region past 256×224" for some time. That framing is wrong, and the measurement showing why is worth recording before anyone builds to it.The measurement
A minimal Mode 5 frame was rendered as a throwaway scene and each host asked what it emits:
The two references do not agree on the shape of a hi-res frame, never mind its pixels: Mesen2 line-doubles and snes9x does not. A contract that simply said "512 wide now" would be comparing a 224-row picture against a 478-row one.
What that implies
Combined with the earlier empirical finding — even/subscreen columns agree within 0.4–3% across all three references while odd/mainscreen columns diverge 33–35% pairwise — the answer is:
The region size does not change at all. What a hi-res scene needs is a declared per-scene extraction — a rule for turning whatever the host emits into the canonical sample — and
Scenegaining a field that names it. That is a materially different (and smaller) piece of work than "widen the region", which is why recording it matters.The mainscreen halves of
C5.06/C5.07andC9.01/.02/.07/.08stay golden-blocked regardless: rule 4 forbids blessing a golden the references disagree about, and on those columns they do.This was only measurable because of #320
Both hosts' geometry checks were lower bounds until yesterday — they caught a frame that was too small and passed one that was too large, hashing a diagonal slice or the leftmost 256 columns. The loud rejection that #320 added is literally what printed the table above. The probe scene was reverted after measuring; no ROM or golden change here.
Docs only.
🤖 Generated with Claude Code
Documents ADR-0013 assertions that hi-res scenes require per-scene extraction rules. snes9x produces 512×224 frames. Mesen2 produces 512×478 frames through line doubling. The
HiResEvenrule selects even columns and, for Mesen2, even rows to produce 256×224 samples. TheDirect/HiResEvenvalue is emitted inbuild/scenes.tsv.Hosts must reject unsupported extraction values. They must not treat them as
Direct, because that defect hashes only the left half of a hi-res frame. Mainscreen golden rows remain blocked because reference pixels disagree. The coverage denominator does not move.The claim is false if either emulator produces different frame geometry, if the extraction does not produce the existing 256×224 sample, or if hosts accept unsupported extraction values or hash invalid frame sizes. It is also false if the mainscreen references agree.