Review: bound OpenGL CPU readback to GPU-written regions - #23
Conversation
|
@cubic-dev-ai review this PR |
@Alexbeav I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/CMakeLists.txt">
<violation number="1" location="runtime/CMakeLists.txt:31">
P3: The test resolves both `gcc.exe` and `g++.exe` inside the directory of `CMAKE_C_COMPILER` (run_gl_readback_region.py links the probe with `T/'g++.exe'`). When the configured C++ toolchain lives in a different directory or uses a different binary name than the C compiler, the link either fails or falls back to an unrelated g++ from PATH, so the registered ctest result no longer reflects the configured toolchain. Derive the compiler bin directory from the actual C++ compiler (which drives the link) or pass the resolved compiler executables, not just the C compiler's directory.</violation>
</file>
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
| set(PSX_GL_READBACK_SDL_ROOT "" CACHE PATH "SDL3 static build root for GL readback test") | ||
| if(MINGW AND PSX_GL_READBACK_SDL_ROOT) | ||
| find_package(Python3 REQUIRED COMPONENTS Interpreter) | ||
| get_filename_component(_gl_test_compiler_bin "${CMAKE_C_COMPILER}" DIRECTORY) |
There was a problem hiding this comment.
P3: The test resolves both gcc.exe and g++.exe inside the directory of CMAKE_C_COMPILER (run_gl_readback_region.py links the probe with T/'g++.exe'). When the configured C++ toolchain lives in a different directory or uses a different binary name than the C compiler, the link either fails or falls back to an unrelated g++ from PATH, so the registered ctest result no longer reflects the configured toolchain. Derive the compiler bin directory from the actual C++ compiler (which drives the link) or pass the resolved compiler executables, not just the C compiler's directory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/CMakeLists.txt, line 31:
<comment>The test resolves both `gcc.exe` and `g++.exe` inside the directory of `CMAKE_C_COMPILER` (run_gl_readback_region.py links the probe with `T/'g++.exe'`). When the configured C++ toolchain lives in a different directory or uses a different binary name than the C compiler, the link either fails or falls back to an unrelated g++ from PATH, so the registered ctest result no longer reflects the configured toolchain. Derive the compiler bin directory from the actual C++ compiler (which drives the link) or pass the resolved compiler executables, not just the C compiler's directory.</comment>
<file context>
@@ -23,6 +23,23 @@ psxrecomp_add_runtime_target(psx-runtime
+ set(PSX_GL_READBACK_SDL_ROOT "" CACHE PATH "SDL3 static build root for GL readback test")
+ if(MINGW AND PSX_GL_READBACK_SDL_ROOT)
+ find_package(Python3 REQUIRED COMPONENTS Interpreter)
+ get_filename_component(_gl_test_compiler_bin "${CMAKE_C_COMPILER}" DIRECTORY)
+ add_test(NAME gl_readback_region_test
+ COMMAND "${Python3_EXECUTABLE}"
</file context>
| get_filename_component(_gl_test_compiler_bin "${CMAKE_C_COMPILER}" DIRECTORY) | |
| get_filename_component(_gl_test_compiler_bin "${CMAKE_CXX_COMPILER}" DIRECTORY) |
There was a problem hiding this comment.
The underlying unsafe compiler assumption was valid and is resolved by the explicit PSX_GL_READBACK_COMPILER_BIN cache option and --compiler-bin argument introduced in 4c35d05. Both gcc.exe and g++.exe must exist in that selected directory; the runner uses their absolute paths, so it cannot fall back to PATH. This is an explicitly selected GCC/MinGW hardware fixture, not a claim to test a differently configured Clang C++ runtime compiler. A Clang project can supply that separate GCC fixture directory or omit the hardware test. Missing/restored compiler paths were tested; current e57d9b9 also checks the SDL headers and library at configure and runner entry. The documentation and PR body state this scope.
|
Both findings are valid and fixed in 4c35d05. PSX_GL_READBACK_COMPILER_BIN explicitly selects a GCC/MinGW directory; CMake verifies gcc.exe and g++.exe before hardware registration. Missing and restored compiler cases were checked. The runner strictly parses the final complete summary and accepts the negative only for exactly one failure named single-pixel bounded transfer. Six parser tests reject counts 10/11/100/199, wrong failures and malformed results. Updated runtime CTest passes 70 enabled tests; real GL native/4x and strict pristine-upstream negative pass. The exact updated GL game head repeats table-state player continuation and exits normally at frame 599. @cubic-dev-ai review this PR |
@Alexbeav I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
All findings from reviews 5125059735, 5125075872 and 5125087765 are addressed on e57d9b9.
The branch update requests the automatic exact-head Cubic review; no second manual review run is requested. |
1 participant
A tiny GPU write followed by a CPU read currently copies all 524,288 native words. Keep a separate conservative rectangle for GPU writes that have not reached the CPU array, then read that region with an explicit full-image row stride. Uploads, queued draws and packing still complete in order. Texture packing must not erase CPU readback debt. Raster values, clocks, precision and GPU ownership remain unchanged.
This is the required fork review; do not merge into fork main. Intended upstream: mstan/psxrecomp.
Base
155e269ba5d50fe36523846f908cf124dc1561a0; review heade57d9b9f4fd220be6f65825282329215f8109a6f; four focused commits; 6 changed files:docs/GPU_GL_READBACK_REGIONS.mdruntime/CMakeLists.txtruntime/src/gpu_gl_renderer.cruntime/tests/run_gl_readback_region.pyruntime/tests/test_gl_readback_region.cruntime/tests/test_gl_readback_runner.pyThe renderer change owns coherence; the C fixture verifies native pixels and transfer size; the Python wrapper runs the real hidden GL context; CMake registers the optional hardware test; documentation states the invariant and limitations. No title configuration, private route tooling or retail payload is included.
Validation on this review head, Windows x64 / WinLibs GCC 16.1 / RTX 4070 Ti, NVIDIA 610.88:
-DBUILD_TESTING=ON -DPSX_GL_READBACK_SDL_ROOT=<SDL3-deps>on MinGW, thenctest --test-dir <runtime-build> -R gl_readback_region_test --output-on-failure. Six parser unit tests reject malformed summaries, wrong exits and unexpected negative failures. The dependency root containssdl3-src/includeandsdl3-build/libSDL3.a.PSX_GL_READBACK_COMPILER_BINcan select GCC for a Clang/MinGW project; configure checks both compiler executables before hardware registration. Missing/restored compiler cases were verified. Both configure and runner reject missing SDL headers/static library before building; a bad dependency root is not counted as a hardware pass. The wrapper also accepts--compiler-bin,--sdl-root,--output; it has no fixed CPU affinity or priority and retains a fresh receipt directory on repeated runs. Other platforms are not qualified by this hardware runner.dirty_text_continuation_guardsstale source fragment,release_zipWindows LF expectation,aot_overlay_discoveryhardcoded MSYS gcc subprocess. Python tests use PYTHONUTF8=1.96928d118bdcce574947052b4053153717eb28accontains the interlace correction plus this exact GL runtime change. Its fresh native OpenGL game-frontend route starts a player game, launches the ball, demonstrates flipper/ball play and closes normally at frame 6925. It is not this standalone review head. Interlace review Review: preserve active display field during interlaced drawing #22 continues independently; further combined results do not change the standalone GL evidence. Current runs establish route correctness only under host GPU load, not a new speed result.The game is a private local port. Owned generated game/SCPH5552 code is reused unchanged; no emitter-regeneration claim. Current public recomp-ui/master 773155ae lacks lobby/spectator fields used by upstream runtime, so these GPU retail tests use supported
PSX_RECOMP_UI=OFFdirect launch. No launcher compatibility, broad save/load, netplay, full depth24 playback, Vulkan, full-game or audio-wide acceptance is implied. Consumer runtime pin update is required; this correction does not require code regeneration. The two corrections can merge independently, but Timeshock normal startup needs both.Developed with AI assistance; validated as described (test evidence in PR body). AI writes the code and the PR, but I always test before I send something up. Happy to iterate on this process with your feedback.