feat(ps1): complete Phase 2 GPU capture for #418#645
Conversation
Move GP0 opcode dispatch into Gp0HookDispatch linked by the libretro plugin, add full A0/C0 VRAM hook handling, Sentry breadcrumb ps1.rip.capture.frame_armed, parser tests for all seven primitive flavors, and a disarmed-overhead regression test. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR refactors the PS1 GPU command capture pipeline by introducing ChangesGPU Command Dispatch Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/PS1/runtime/Gp0HookDispatch_test.cpp`:
- Around line 39-42: The two brittle timing assertions using EXPECT_LT on
disarmedNs and armedNs should be replaced: instead of relying on wall-clock
ratios (the EXPECT_LT comparisons), change the test to a deterministic check
that validates the early-return/disarmed path behavior (for example, assert that
the disarmed path did not invoke the scan handler or increment a side-effect
counter) or, if timing must be used, relax and stabilize the criterion (e.g.,
assert disarmedNs <= armedNs and set a much looser ratio threshold). Update the
assertions around the variables disarmedNs and armedNs (and any EXPECT_LT
usages) to implement the deterministic side-effect verification or the relaxed
threshold so CI noise cannot flake the test.
In `@src/PS1/runtime/Gp0HookDispatch.cpp`:
- Around line 59-69: primDedupeKey currently omits per-vertex UVs (and colors),
causing distinct textured primitives with identical geometry to collapse; update
primDedupeKey (and use PrimRecord fields) to incorporate UV coordinates for each
vertex (e.g., prim.uvs or prim.verts[v].u/ .v) into the dedupe string and also
include primitive color/pixel color fields if present (e.g., prim.color or
prim.verts[v].color); restrict adding UVs to textured primitive kinds (check
prim.kind for textured types) to avoid bloating keys for non-textured kinds, and
format/arg them consistently with the existing vertex/attribute concatenation so
the key remains stable.
In `@src/PS1/runtime/PS1RipManager.cpp`:
- Around line 342-346: In PS1RipManager::armCapture replace the current
SentryReporter::addBreadcrumb calls that use the specific category
"ps1.rip.capture.frame_armed" with the required category "ui.action" for both
the armed and disarmed branches, and keep the action detail in the message (e.g.
include "ps1.rip.capture.frame_armed: armed" / "ps1.rip.capture.frame_armed:
disarmed" or otherwise preserve the "armed"/"disarmed" detail) so the breadcrumb
uses SentryReporter::addBreadcrumb("ui.action", <message>) while preserving the
action context.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 381a2284-e5d1-4a7d-85c0-551766f789dd
📒 Files selected for processing (18)
plugins/ps1core_libretro/CMakeLists.txtplugins/ps1core_libretro/LibretroEmuCore.cppplugins/ps1core_libretro/LibretroEmuCore.hsrc/CMakeLists.txtsrc/PS1/CMakeLists.txtsrc/PS1/runtime/EmuHooks.hsrc/PS1/runtime/Gp0HookDispatch.cppsrc/PS1/runtime/Gp0HookDispatch.hsrc/PS1/runtime/Gp0HookDispatch_test.cppsrc/PS1/runtime/GpuCommandParser.cppsrc/PS1/runtime/GpuCommandParser.hsrc/PS1/runtime/GpuCommandParser_test.cppsrc/PS1/runtime/PS1RipManager.cppsrc/PS1/runtime/PsxGpuRamScanner.cppsrc/PS1/runtime/PsxGpuRamScanner.hsrc/PS1/runtime/RipperHooks.cppsrc/PS1/runtime/RipperHooks.htests/CMakeLists.txt
💤 Files with no reviewable changes (2)
- plugins/ps1core_libretro/LibretroEmuCore.h
- src/CMakeLists.txt
| QString primDedupeKey(const PrimRecord &prim) | ||
| { | ||
| QString key = QStringLiteral("%1|%2").arg(static_cast<int>(prim.kind)).arg(prim.vertexCount); | ||
| for (int v = 0; v < 4; ++v) | ||
| key += QStringLiteral("|%1,%2").arg(prim.verts[v].x).arg(prim.verts[v].y); | ||
| key += QStringLiteral("|%1,%2|%3|%4") | ||
| .arg(prim.tpage) | ||
| .arg(prim.clut) | ||
| .arg(prim.semiTrans) | ||
| .arg(prim.matrixId); | ||
| return key; |
There was a problem hiding this comment.
Dedupe key is too coarse and can drop valid textured primitives.
primDedupeKey ignores UVs (and color), so two different primitives with identical geometry can collapse into one capture entry. Include UVs (at least for textured kinds) in the key to avoid false-positive deduplication.
Proposed patch
QString primDedupeKey(const PrimRecord &prim)
{
QString key = QStringLiteral("%1|%2").arg(static_cast<int>(prim.kind)).arg(prim.vertexCount);
- for (int v = 0; v < 4; ++v)
- key += QStringLiteral("|%1,%2").arg(prim.verts[v].x).arg(prim.verts[v].y);
+ for (int v = 0; v < 4; ++v) {
+ key += QStringLiteral("|%1,%2").arg(prim.verts[v].x).arg(prim.verts[v].y);
+ key += QStringLiteral("|%1,%2").arg(prim.verts[v].u).arg(prim.verts[v].v);
+ }
key += QStringLiteral("|%1,%2|%3|%4")
.arg(prim.tpage)
.arg(prim.clut)
.arg(prim.semiTrans)
.arg(prim.matrixId);
return key;
}🤖 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 `@src/PS1/runtime/Gp0HookDispatch.cpp` around lines 59 - 69, primDedupeKey
currently omits per-vertex UVs (and colors), causing distinct textured
primitives with identical geometry to collapse; update primDedupeKey (and use
PrimRecord fields) to incorporate UV coordinates for each vertex (e.g., prim.uvs
or prim.verts[v].u/ .v) into the dedupe string and also include primitive
color/pixel color fields if present (e.g., prim.color or prim.verts[v].color);
restrict adding UVs to textured primitive kinds (check prim.kind for textured
types) to avoid bloating keys for non-textured kinds, and format/arg them
consistently with the existing vertex/attribute concatenation so the key remains
stable.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 949b6c7a2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const int xEnd = qMin(static_cast<int>(x) + static_cast<int>(w), VramSnapshot::kWidth); | ||
| const int yEnd = qMin(static_cast<int>(y) + static_cast<int>(h), VramSnapshot::kHeight); | ||
| for (int yy = static_cast<int>(y); yy < yEnd; ++yy) { | ||
| for (int xx = static_cast<int>(x); xx < xEnd; ++xx) |
There was a problem hiding this comment.
onVramRead now iterates every pixel in the requested rectangle but discards the value, so it has no observable effect on capture state while adding O(w*h) work per GP0 0xC0 packet. In this codebase the RAM scanner is heuristic (looksLikeGp0Opcode only checks opcode bytes), so false-positive read-back packets are expected; each one can trigger up to a full 1024x512 walk here, which can significantly degrade armed-capture performance without improving correctness.
Useful? React with 👍 / 👎.
Use ui.action breadcrumbs with frame_armed detail, replace flaky timing benchmark with deterministic disarmed-capture test, and drop O(w*h) VRAM read-back loop. Co-authored-by: Cursor <cursoragent@cursor.com>
|
1 participant
Summary
Gp0HookDispatchwith GP0 opcode dispatch linked directly by the libretro plugin (GPL boundary clean)RipperHooksps1.rip.capture.frame_armed, parser tests for all 7 primitive flavors + CSV round-trip, and disarmed-overhead regression testX11Extraslink that broke Qt6 Linux buildsTest plan
xvfb-run ./build_local/bin/UnitTests --gtest_filter="GpuCommandParser*:Gp0HookDispatch*:PsxGpuRamScanner*"— 14/14 passCloses remaining acceptance criteria for #418.
Made with Cursor
Summary by CodeRabbit
Improvements
Tests