feat(vi): framebuffer scan-out to RGBA8 (T-31-004 part 2)

[doublegate]

Motivation

Phase 3 (v0.4.0 "Rasteriser"), Sprint 1 — T-31-004 part 2, the framebuffer
scan-out. Part 1 (#59) landed the VI register file; this converts the
framebuffer the FILL pipeline writes into a presentable RGBA8 frame — the
conversion that makes the pixels a picture.

Changes

• Bus::scanout(&self, out) -> (w, h) reads VI_ORIGIN/VI_WIDTH/VI_CTRL
  and the active height from VI_V_VIDEO, converting the framebuffer to RGBA8:
  16-bit RGBA5551 (5→8-bit channel expansion, 1-bit alpha to 0/255) and
  32-bit RGBA8888 (direct copy). TYPE 0/1 (blank) returns (0, 0).

Scope (honest)

A 1:1 scan — VI_X_SCALE/VI_Y_SCALE resampling and the AA/divot/de-dither
post-filters are deferred (open residual R-5); the pixel conversion itself is
exact and cited. No per-frame driver yet — the scheduler tick that calls
scanout lands with the V_CURRENT advance (part 3). The harness golden-frame
comparison (T-31-005) is the first real consumer.

Verification

• Byte-for-byte 16-/32-bit conversion (white/black RGBA5551, direct RGBA8888) and
  the blank-VI case. The 5→8-bit expansion is mutation-checked (dropping the
  low-bit replication makes 0xFFFF map to 0xF8F8F8 instead of white).
• Gates green: cargo fmt --check, clippy --workspace --all-targets -- -D warnings, cargo test --workspace, RUSTDOCFLAGS="-D warnings" cargo doc, the
  thumbv7em-none-eabihf no_std build, pre-commit run markdownlint.

🤖 Generated with Claude Code

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: bfe576f6-de89-4d53-bfcd-5e7ee4aa9862

📥 Commits

Reviewing files that changed from the base of the PR and between b1eccc0 and be0153f.

📒 Files selected for processing (1)

• docs/accuracy-ledger.md

---

📝 Walkthrough

Summary by CodeRabbit

• New Features
  • Added VI framebuffer scan-out that exports video memory to RGBA8 using active VI register state.
  • Supports direct RGBA8888 copying and RGBA5551 5→8 channel expansion with 0/255 alpha mapping; blank modes produce no output (returns zero dimensions).
  • If the destination is too small (or the VI format is not supported), scan-out is skipped and the output buffer remains unchanged.
• Tests
  • Added unit tests covering RGBA8888/5551 paths, alpha mapping, blank behaviour, and undersized destinations.
• Documentation
  • Updated accuracy notes/ledger and VI scan-out documentation, including current 1:1, unfiltered scope and remaining residuals.
• Chores
  • Documented the work in the changelog.

Walkthrough

Bus::scanout converts VI-selected RDRAM framebuffer data to RGBA8 for 16-bit and 32-bit formats, handles blank or undersized output, and derives dimensions from VI state. Tests cover conversion and validation. Documentation records the implementation and deferred scaling and filtering work.

Changes

VI framebuffer scan-out

Layer / File(s)	Summary
Scan-out conversion and validation crates/rustyn64-core/src/bus.rs	Adds Bus::scanout, RGBA5551 channel expansion, RGBA8888 copying, blank and undersized-output handling, and tests for these paths.
Scan-out status and residuals docs/rdp.md, docs/accuracy-ledger.md, CHANGELOG.md	Documents the implemented 1:1 conversion, scheduler status, deferred scaling and filters, and residual R-5.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant VI
  participant Scanout
  participant RDRAM
  participant Output
  VI->>Scanout: provide framebuffer configuration
  Scanout->>RDRAM: read framebuffer pixels
  RDRAM-->>Scanout: return pixel data
  Scanout->>Output: copy or convert to RGBA8

Loading

Possibly related PRs

• doublegate/RustyN64#59: Wires VI register state into the CPU bus, which Bus::scanout reads.

🚥 Pre-merge checks | ✅ 9 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title matches Conventional Commits format, but the subject is not in imperative mood as required.	Rewrite the subject in imperative form, e.g. "feat(vi): add framebuffer scan-out to RGBA8".

✅ Passed checks (9 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description is clearly about VI framebuffer scan-out and matches the change set.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Oracle Number Is Stated	✅ Passed	docs/accuracy-ledger.md R-5 and docs/rdp.md both state the n64-systemtest effect was not measured; docs/STATUS.md gives the current oracle count as 93.
Docs-As-Spec Sync	✅ Passed	PASS: rustyn64-core’s scan-out change is documented in docs/rdp.md, and R-5 in docs/accuracy-ledger.md records the deferred parts; the PR body explains the scope.
Changelog Entry For User-Visible Changes	✅ Passed	PASS — CHANGELOG.md has a new [Unreleased] entry for the user-visible VI framebuffer scan-out feature.
Measured, Never Tuned	✅ Passed	PASS: the scan-out values are covered by docs/accuracy-ledger.md R-5 and docs/rdp.md; no uncited hardware constant or timing value was added.
Unsafe Stays Out Of The Chip Crates	✅ Passed	No unsafe syntax appears in non-frontend Rust code, and the six non-frontend crate roots still #![forbid(unsafe_code)]; frontend has no unsafe blocks.

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Antigravity review (Gemini via Ultra)

This PR implements 1:1 VI framebuffer scan-out (Bus::scanout) to convert 16-bit RGBA5551 and 32-bit RGBA8888 framebuffers into RGBA8 pixel buffers.

Blocking issues

• Silent failure on undersized output buffer (bus.rs): When out.len() is smaller than the required width * height * 4 bytes, scanout aborts mid-frame and returns (width, height). Returning full dimensions when scan-out was incomplete creates a silent failure path where the caller cannot detect that out contains truncated, partial frame data. Validate out.len() upfront and return (0, 0) if the provided buffer is insufficient.

Suggestions

• Validate required buffer length upfront (bus.rs): Evaluating dst + 4 > out.len() inside the per-pixel inner loop adds unnecessary bounds checks to every pixel iteration. Validate out.len() < (width as usize) * (height as usize) * 4 once before starting the loop.
• Mask input in expand5 to guard against overflow panics (bus.rs): expand5 performs v5 << 3 on u8. If called with any value greater than 31, debug builds will panic due to shift overflow. Apply (v5 & 0x1F) inside expand5 to ensure domain safety.

Nitpicks

• Redundant integer casting (bus.rs): Iterate over 0..4usize to avoid casting i as usize inside the byte copy loop.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

[doublegate]

Adjudication of the Antigravity review

Fixes in the latest commit.

Blocking

• Silent failure on undersized buffer — ADOPTED. scanout now validates
  out.len() >= width * height * 4 up front and returns (0, 0) writing
  nothing when it can't hold the frame, so a non-zero return means the whole
  frame was written. New test scanout_refuses_an_undersized_buffer;
  mutation-checked (removing the guard panics on the out-of-bounds write).

Suggestions

• Validate length up front (perf) — ADOPTED, same change: the per-pixel
  bounds check is gone.
• Mask expand5 input — ADOPTED. expand5 masks to 5 bits first, so an
  out-of-range argument can't overflow << 3 in debug. (All call sites already
  pass & 0x1F values; this is belt-and-suspenders.)

Nitpicks

• Redundant cast in the byte loop — ADOPTED with modification. Rather than
  swap i as usize for i as u32, the 32-bit path now does
  out[dst..dst+4].copy_from_slice(&self.rdram_read_u32(src).to_be_bytes()) —
  no loop, no cast, and one word read instead of four byte reads.

[coderabbitai]

Actionable comments posted: 5

🤖 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 `@crates/rustyn64-core/src/bus.rs`:
- Around line 447-449: Update the frame-writing logic around the dst + 4 bounds
check to preflight the complete required RGBA output length before modifying
out. When the buffer is insufficient, return the established explicit
insufficient-buffer result rather than (width, height), and ensure no partial
frame data is written.
- Around line 1209-1215: Strengthen scanout_is_blank_when_the_vi_is_off to
pre-fill the destination with a nonzero sentinel, then verify it remains
unchanged after blank scan-out. Extend the test to cover both blank VI types,
TYPE == 0 and TYPE == 1, while preserving the expected (0, 0) result and
confirming neither path writes to the output.
- Around line 1198-1206: Update the scanout fixture in the RGBA5551 test around
bus.scanout to include a non-uniform pixel such as 0x0887, replacing or
supplementing the uniform white/black cases. Assert its observable RGBA output
is [0x08, 0x10, 0x18, 0xFF] so component ordering and field shifts are covered.

In `@docs/rdp.md`:
- Around line 280-282: Correct the scheduler description near the “VI_V_CURRENT
firing / the scheduler tick” heading: describe the scheduler advancing
VI_V_CURRENT and raising the VI interrupt at VI_V_INTR, rather than saying
VI_V_CURRENT fires. Preserve the surrounding scan-out scheduling and conversion
context.
- Around line 296-314: The scan-out verification record needs explicit
validation status. In docs/rdp.md lines 296-314, state the measured
n64-systemtest failing-assertion count, or explicitly document that it was not
measured and why; in docs/accuracy-ledger.md line 188, replace “are validated”
with wording that describes validation as planned while keeping R-5 open.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a6c3c18f-90de-4c56-ac09-0cf346028202

📥 Commits

Reviewing files that changed from the base of the PR and between 10efb38 and a3673a9.

📒 Files selected for processing (4)

• CHANGELOG.md
• crates/rustyn64-core/src/bus.rs
• docs/accuracy-ledger.md
• docs/rdp.md

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

crates/rustyn64-core/src/bus.rs (3)

1191-1204: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the complete RGBA8888 frame.

The 2×2 fixture checks only the first and last pixels, so an implementation that skips or corrupts either middle pixel still passes. Assert all four output pixels to cover the row/column traversal.

As per coding guidelines, tests must verify observable effects and choose cases that fail when the implementation is broken.

🤖 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 `@crates/rustyn64-core/src/bus.rs` around lines 1191 - 1204, Update the scanout
test around the 2×2 framebuffer fixture and its existing output assertions to
validate all four RGBA8888 pixels in row-major order. Replace the partial
first/last-pixel checks with complete coverage of the output buffer, preserving
the expected px32 values for both middle pixels.

Source: Coding guidelines

---

29-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make the expand5 guard mutation-testable or remove it.

Every current caller already masks its extracted channel with 0x1F, so deleting v5 & 0x1F does not change any scanout result and the tests will still pass. Either remove the redundant guard or add a focused test for its out-of-range contract.

As per coding guidelines, tests must mutation-check guards by proving that they fail without the fix.

🤖 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 `@crates/rustyn64-core/src/bus.rs` around lines 29 - 34, The `expand5` mask
guard is currently redundant and untested. In `expand5`, either remove the `v5 &
0x1F` masking or add a focused test that passes an out-of-range value and
verifies the documented masked result, ensuring the test fails if the guard is
removed.

Source: Coding guidelines

---

444-445: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Handle reversed VI_V_VIDEO ranges explicitly (crates/rustyn64-core/src/bus.rs:444). docs/rdp.md and accuracy ledger R-5 only define height as (V_END − V_START) half-lines; saturating_sub adds an unlisted zero-height fallback for V_START > V_END. Either reject that state or record the measured policy in docs/accuracy-ledger.md.

🤖 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 `@crates/rustyn64-core/src/bus.rs` around lines 444 - 445, Update the height
calculation near the VI timing validation in bus.rs to handle V_START > V_END
explicitly instead of silently using saturating_sub; reject the reversed
VI_V_VIDEO range through the existing invalid-timing path, or document the
chosen measured behavior in docs/accuracy-ledger.md while preserving the defined
(V_END − V_START) half-line calculation.

Source: Path instructions

🤖 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/accuracy-ledger.md`:
- Line 188: The R-5 accuracy-ledger entry lacks precise, falsifiable provenance
for its conversion and deferred VI behavior claims. Update the R-5 row to cite
exact N64brew manual/wiki sections or named measured test matrices covering
RGBA5551 5→8-bit widening, RGBA8888 channel ordering, VI_X_SCALE/VI_Y_SCALE
sampling, and AA/divot/de-dither filtering, while preserving the existing
open-status and scope.

---

Outside diff comments:
In `@crates/rustyn64-core/src/bus.rs`:
- Around line 1191-1204: Update the scanout test around the 2×2 framebuffer
fixture and its existing output assertions to validate all four RGBA8888 pixels
in row-major order. Replace the partial first/last-pixel checks with complete
coverage of the output buffer, preserving the expected px32 values for both
middle pixels.
- Around line 29-34: The `expand5` mask guard is currently redundant and
untested. In `expand5`, either remove the `v5 & 0x1F` masking or add a focused
test that passes an out-of-range value and verifies the documented masked
result, ensuring the test fails if the guard is removed.
- Around line 444-445: Update the height calculation near the VI timing
validation in bus.rs to handle V_START > V_END explicitly instead of silently
using saturating_sub; reject the reversed VI_V_VIDEO range through the existing
invalid-timing path, or document the chosen measured behavior in
docs/accuracy-ledger.md while preserving the defined (V_END − V_START) half-line
calculation.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c85bd60c-3303-4536-89f3-df3b573be2f9

📥 Commits

Reviewing files that changed from the base of the PR and between 8770c75 and c339f4e.

📒 Files selected for processing (3)

• crates/rustyn64-core/src/bus.rs
• docs/accuracy-ledger.md
• docs/rdp.md

[coderabbitai]

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/accuracy-ledger.md`:
- Line 188: Update the R-5 entry in the accuracy ledger to include an explicit
n64-systemtest impact statement, indicating whether the failing-assertion count
was measured; if not, state that it was not measured because the timing set has
not been run.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6f3150bf-b9cc-4680-8c9f-f55d722d01e7

📥 Commits

Reviewing files that changed from the base of the PR and between c339f4e and b1eccc0.

📒 Files selected for processing (1)

• docs/accuracy-ledger.md