Repository navigation
fix(libSceVideoOut): refuse to unregister the buffer set on screen as the console does - #11196
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
The two independent catalog observations and the regression check give useful evidence for refusing the on-screen set, and I found no contradiction in the checked path. However, this is a VideoOut change, and CONTRIBUTING.md requires the focused tests on both main and the change with VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, ANYPS5_NO_SHADER_CACHE=1 and ANYPS5_REQUIRE_VULKAN=1, plus a comparison for newly introduced VUID- errors. The report describes the Windows flip-test run and a failing test without the code change, but not the required validation-layer comparison. Please add those same-machine/GPU results before approval; hosted product builds are skipped.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
Thanks. I've added the validation-layer comparison to the description:
|
|
@MarcZX10 could you take another look when you have time? The validation-layer comparison you asked for is in the description and in this comment: all 33 libSceVideoOut tests pass with |
adriadam10
left a comment
There was a problem hiding this comment.
Re-checked against the current head: the validation-layer comparison @MarcZX10 asked for is in the description (33 libSceVideoOut tests on main and on this head with VK_LAYER_KHRONOS_validation, ANYPS5_NO_SHADER_CACHE=1, ANYPS5_REQUIRE_VULKAN=1, no VUID- errors, same machine and GPU), and the code, the Flip.cpp checks and the debt entry are fine. Nothing is left for the author.
@MarcZX10 the only thing holding the merge is your CHANGES_REQUESTED on this head; could you re-review or dismiss it?
511b2b6 to
2188fef
Compare
|
Rebased onto
Rerun on
@MarcZX10 @adriadam10 Both your reviews on this PR are still recorded as changes requested; adriadam10's from 2026-10-27 said nothing was left for the author. Could you re-review with the new commit, or dismiss them if it's fine? |
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, changes still requested
The overlay-bus adjustment addresses the reported regression, but the rebase and validation evidence are stale against current main.
What I ran
- Reviewed exact head
2188fef0aab9cebb9bbc2b61767d7f76cb07e4d6against currentmainedf6804cf52c62eacafc592dd6142cb4efe5bd05. - Ran
git merge-base,git merge-tree --write-tree,git diff --name-status, andgit diff --check. - Checked the author’s rebase report and current hosted checks. I did not run the GPU tests locally.
Results
| Check | Result |
|---|---|
| PR merge base | 31646b6e17e95e9124953d4008f0ce2e50ed153d |
| Current-main merge preview | Conflicts in docs/dev/debt/libSceVideoOut.md |
| VideoOut paths changed on main since the reported test base | Output.hpp, Output.cpp, Flip.cpp, and OutputModePolicy.cpp |
| PR paths | Buffer.cpp, Flip.cpp, and the VideoOut debt file |
| Diff check | Passed; no output |
| Hosted Windows / conventions / eligibility / labels / report | Passed; Linux and specialized jobs skipped |
| Focused GPU validation | Author reports 36 tests pass on main 31646b6e17 and head 2188fef0aa; that main is no longer current |
Unedited command output
$ git merge-base upstream/main refs/review/upstream-11196
31646b6e17e95e9124953d4008f0ce2e50ed153d
$ git merge-tree --write-tree upstream/main refs/review/upstream-11196
8e74cb366dbd8ac5a73812ca24e09fe1f2238ebc
100644 d57e8243d19ae5d88698f18fd5ac247585de24fb 1 docs/dev/debt/libSceVideoOut.md
100644 527bc73b1251c6912063838d141437a0944bac80 2 docs/dev/debt/libSceVideoOut.md
100644 a0c42367343794db17238837f2179c395754692d 3 docs/dev/debt/libSceVideoOut.md
Auto-merging core/libs/prx/libSceVideoOut/tests/Flip.cpp
Auto-merging docs/dev/debt/libSceVideoOut.md
CONFLICT (content): Merge conflict in docs/dev/debt/libSceVideoOut.md
$ git diff --name-status 31646b6e17e95e9124953d4008f0ce2e50ed153d..upstream/main -- core/libs/prx/libSceVideoOut
M core/libs/prx/libSceVideoOut/include/Output.hpp
M core/libs/prx/libSceVideoOut/src/Output.cpp
M core/libs/prx/libSceVideoOut/tests/Flip.cpp
M core/libs/prx/libSceVideoOut/tests/OutputModePolicy.cpp
$ git diff --name-status upstream/main...refs/review/upstream-11196
M core/libs/prx/libSceVideoOut/src/Buffer.cpp
M core/libs/prx/libSceVideoOut/tests/Flip.cpp
M docs/dev/debt/libSceVideoOut.md
$ git diff --check upstream/main...refs/review/upstream-11196
Negative control
| Reported control | Outcome |
|---|---|
Before the bus-type guard, video_out_flip_overlay on the rebased tree |
Author reports failure because the overlay buffer stayed composited; not independently reproduced here |
Findings
Blocking
- Please rebase onto current
main, resolve the debt-file conflict, then rerun the focused VideoOut tests on both that exactmainand the resulting PR head with the Vulkan validation settings in the report. The current main changed VideoOut implementation and test paths since31646b6e17, so the posted 36-test comparison does not validate the current integration.
Non-blocking
- The PR correctly limits the measured
RESOURCE_BUSYbehavior to the main bus and records the overlay bus as unmeasured.
Sources
- Author's rebase and test report
- Current-main VideoOut output path
- Current-main VideoOut flip tests
- PR-head buffer behavior
Not verified
- Current-main behavior after the VideoOut changes since
31646b6e17, and the author-reported test outcomes were not independently reproduced.
To get this to a good state
- Rebase and resolve the debt overlap, then publish the same-scene validation-layer comparison with exact current-main and PR-head commits and all focused test results.
2188fef to
040acf7
Compare
|
@MarcZX10 Rebased onto current Rerun on that head and on
|
MarcZX10
left a comment
There was a problem hiding this comment.
Re-check — changes requested\n\nThe new head addresses the overlay-bus regression and the reported 36-test validation-layer run on main/head 4c7d275 is useful. Current upstream main is now aaccd0b, and this head is 44 commits behind it; main has also changed the same VideoOut Flip.cpp test file since the reported base. Please merge current main, preserve the main-bus refusal and overlay regression coverage, resolve the overlapping test hunk, and rerun the focused VideoOut tests on current main and the exact updated head with validation enabled. I did not run the GPU tests locally.
The console answers RESOURCE_BUSY (0x80290009) when the set holding the buffer on screen is unregistered, and the set stays registered: EVO Player measured it with a re-registration then failing SLOT_OCCUPIED, and Prospero Win on firmware 12.02 after its flips went idle. The set was released and 0 returned instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…only EVO Player and Prospero Win measured RESOURCE_BUSY on ports opened on bus type 0. The overlay bus is unmeasured, so its on-screen set is still released as before, which video_out_flip_overlay relies on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
040acf7 to
ecadb60
Compare
|
@MarcZX10 Rebased onto current
Rerun on that exact head and on
|
|
Re-checked on |
What
sceVideoOutUnregisterBuffersreleased the buffer set holding the buffer on screen and returned 0. Two catalog titles measured that the console refuses this:0x80290010(SLOT_OCCUPIED), so the set stays registered.sceVideoOutClosethen succeeds.The change:
flipStatus.currentBuffer, the call now returnsRESOURCE_BUSY(0x80290009) and keeps the set and its buffers registered. Closing the port releases them, as before. Both titles open bus type 0 (sceVideoOutOpen(0xff, 0, 0, NULL)in EVO Player and Prospero Win). The overlay bus is unmeasured and still releases its on-screen set, whichvideo_out_flip_overlayrelies on.video_out_flip_cpuflipandvideo_out_flip_cpuflip_in_ordernow check the refusal after their flips of buffer 0 have been shown. They also check that the set is still registered.Tested
Current head
2188fef0aaonmain31646b6e17(Windows 11 Pro 26300, WinLibs GCC 15.2.0, Release, full builds, NVIDIA GeForce RTX 5070):main, with the Windows driver on a separate desktop andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1,ANYPS5_REQUIRE_VULKAN=1andVK_LOADER_DEBUG=layer. In each log the loader inserts the layer (23Insert instance layerlines), and neither haswas not found but was requestedor anyVUID-error.guest_video_out_wqhd_detection_no_vulkanskips by design.video_out_flip_overlayfailed on this rebase ("an unregistered overlay buffer was composited"). That test, added tomainon 2026-10-24, unregisters the overlay set while it is on screen.tools/check_conventions.py --base origin/mainpasses.Earlier runs, Windows 11 Pro (26300), WinLibs GCC 15.2.0, Release, NVIDIA, full build on
main1f68b1f:ANYPS5_VIDEO_OUT_TEST_DRIVER=windowson a separate desktop, so the flip tests run instead of skipping on the offscreen driver;guest_video_out_wqhd_detection_no_vulkanskips by design.Buffer.cppchange:video_out_flip_cpuflipfails with "the set on screen was unregistered".ctest -VandVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1andANYPS5_REQUIRE_VULKAN=1, onmain1f68b1f and on this change, on the same machine and GPU (Vulkan SDK 1.4.363.0).VK_LOADER_DEBUG=layerthe loader inserts the layer.was not found but was requested by env var VK_INSTANCE_LAYERS, and neither has anyVUID-error.guest_video_out_high_frame_ratefail its timing check ("high-frame-rate mode did not double vblank frequency") while the machine was busy. That test doesn't unregister buffers. Three more runs each onmainand the change pass, and so does a second full run.tools/check_conventions.py --base origin/mainpasses.tools/pr_overlap.pyfinds no overlap; of the open PRs it couldn't compare, feat: run PS4 games on the AGC driver — Limbo (CUSA01664) in game (integration branch) #6977 and feat(libSceGnmDriver): build PS4 command buffers and submit them #9432 touchBuffer.cppbut not this function.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code