Skip to content

fix(libSceAudioOut): return from a blocking AudioOut2 push once the queue has room - #12954

Merged
adriadam10 merged 9 commits into
boykopovar:mainfrom
hugodearaujo:fix/audioout2-blocking-push
Oct 30, 2026
Merged

adriadam10 merged 9 commits into
boykopovar:mainfrom
hugodearaujo:fix/audioout2-blocking-push

Conversation

@hugodearaujo

@hugodearaujo hugodearaujo commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

What

sceAudioOut2ContextPush with blocking 1 waited only before queueing its grain, while the queue was full. The push that fills the queue therefore returned at once, with the queue at its depth.

PRAGMATA (PPSA02530) feeds the context its sound engine renders into (queue depth 4) from three threads: a ticker polls sceAudioOut2ContextGetQueueLevel, calls sceAudioOut2ContextAdvance once per free slot and signals a semaphore per slot; a feeder thread waits on that semaphore, calls sceAudioOut2ContextPush(ctx, 1) and then sets the event flag that wakes the render thread; the render thread waits on that flag with no timeout. With the push returning as soon as the grain was queued, the fourth push left the queue full, the woken render thread found no free slot and slept, and nothing woke it until the title's next frame. Every call traced on one context (local diagnostic, not part of this PR):

t=300.478 GetQueueLevel ctx 0x7fb4fa30e6b0 tid 1431260 -> level 4, available 0 (queued 4)
t=301.262 GetQueueLevel ctx 0x7fb4fa30e6b0 tid 1431260 -> level 0, available 4 (queued 0)

So the mix advanced four 21.3 ms grains per rendered frame, and at the 1-3 fps the title renders at under AnyPS5 the context took 0.5-6 pushes per second instead of 46.9. The real-time scene after the opening film follows that audio: it crawls and then stops on one frame, on one of the opening credits, for the rest of the run (in every run that got that far, from about 485 s after start on), while rendering continues and the title still takes input.

A blocking push now also waits after queueing until fewer grains than the queue depth wait to be played, with the existing 200 ms limit, so the feeder is paced by playback and wakes the renderer once per grain. A non-blocking push is unchanged. When the console's blocking push returns is not measured; the debt entry says so.

Tested

Linux (Arch, kernel 7.2.11, GCC 16.2.1, Release), on main bb04a4f.

  • New TestBlockingPushWaitsForRoomWithoutADevice in core/libs/prx/libSceAudioOut/tests/AudioOut2Timing.cpp (run by audio_out2_timing): a blocking push that fills a context's queue must not return before the queue has room again. With only AudioOut2Context.cpp restored from main, audio_out2_timing fails (Subprocess aborted).

  • Real audio device, before and after with exact builds. Both builds are main bb04a4f + fix(libScePad): return an error for unsupported port probes #10935 6076b85 + fix(shader): load and store 16-bit data of converted packed images #11580 fedf3e9 + fix(agc): keep at most 4 GiB of the host heap from the host import budget #7844 89d6a7c + fix(libc): leave a range that a mutation waits for out of new leases #6565 f2029f9 + fix(libSceAvPlayer): keep synced video at the presentation clock when the title takes frames slowly #12898 d2d4331 + fix(agc): keep buffer owners when a staged address space dissolves #10304 31f895c + fix(agc): refuse writable buffer feedback with native depth #10497 8a0dcd5 + fix(agc): prepare a draw or dispatch again when the memory it captured changed before it was recorded #7446 a4a99c3 + fix(agc): enable independentBlend for color targets with different blend states #6782 4433a62 (0109385f31, the open PRs PRAGMATA needs to reach that scene on current main), one with this PR's head 1ed07de merged on top (ea561be1f7); nothing else differs (no local diagnostics; only libSceAudioOut.prx changes). Audio went through SDL's PulseAudio backend into a PipeWire null sink, and both AudioOut2 contexts opened a device (SDL device 2 opened, SDL device 3 opened). Same warmed shader cache, 1400 s each, APS5_TRACE_AUDIOOUT2=1 for the per-second summary:

    Sound engine context (depth 4) Underruns (primes) Drops Scene
    Without this PR 634 pushes by 1029 s (0.0-0.3 per second in the summary lines) 104 0 black from 763 s to the end of the run, no loading screen
    With this PR 46.9 per second throughout, 46735 pushes by 1000 s 0 0 the credits finish and the loading screen appears at 929 s; at 1314 s the next load stops on an unrelated Vulkan device loss (vkQueueSubmit recorder: Vulkan result -4; DISPATCH_INDIRECT on queue 0x0)
    without: [audioout2] t=1028.983 ctx 0x7f3135e95b40: 0.0 pushes/s, 0.0 advances/s, 0.1 queue polls/s, hw queue 0/4, unread 21 ms, mix peak 0.002; totals: pushes 634 (634 blocking, 0 queue-full rejects), primes 104, dropped 0
    with:    [audioout2] t=1000.490 ctx 0x7f1d7fbff370: 47.0 pushes/s, 47.0 advances/s, 187.9 queue polls/s, hw queue 3/4, unread 106 ms, mix peak 0.000; totals: pushes 46735 (46735 blocking, 0 queue-full rejects), primes 0, dropped 0
    
  • Deterministic blocking/release tests on a test clock (AudioOut2Timing.cpp): a device-less context can run its queue model on clockForTest, so playback and the 200 ms wait limit only advance when the test moves the clock.

    • TestBlockingPushStaysBlockedUntilPlaybackFreesASlot: with the clock frozen and depth - 1 grains queued, a blocking push from another thread is observed under the context lock with its grain queued (queued == depth) but not yet counted (pushes == depth - 1), its future still pending and the level at depth. Playing one grain on the test clock returns it with 0 and the level at depth - 1.
    • TestBlockingPushOnAFullQueueQueuesOnlyOnceASlotIsFree: with the queue filled by non-blocking pushes, a blocking push never takes queued past the depth, and returns once two grains are played.
    • Red controls, 3 of 3 each: main's AudioOut2Context.cpp (bb04a4f) fails pushes == depth - 1; the head without the post-queue wait fails the same assertion; the head without the pre-queue wait fails queued <= depth. On the old code, queueing and counting the push happen in one critical section, so the first assertion fails however the threads are scheduled. The head passes 3 of 3.
  • ctest -R audio_out: 52 of 52 pass, three times in a row.

  • main 102c4f9 merged in (56fd9ea): AudioOut2Context.cpp conflicted where main replaced the handle cast with its context registry (AudioOut2FindContext); the resolution keeps the registry and this PR's test clock. ctest -R audio_out: 56 of 56, three times in a row.

  • PRAGMATA (PPSA02530), 1000 s runs from boot through the menus into the opening film and the real-time scene after it, SDL dummy audio, with a local diagnostic that logs the context's pushes per second. Both builds are the same local integration build PRAGMATA needs to get there (main e61a7c7 plus open PRs, including fix(libc): leave a range that a mutation waits for out of new leases #6565 and fix(libSceAvPlayer): keep synced video at the presentation clock when the title takes frames slowly #12898) and differ only in this change to AudioOut2Context.cpp:

    Without this PR With this PR
    Pushes on the engine's context by 400 s 1081 (4.1 pushes/s at 400 s, 0.5 pushes/s at 543 s) 18615 and 19791 by 400-425 s (46.9 pushes/s for the rest of both runs, no underruns)
    The real-time scene stops on the "Executive Producer" credit at 515-545 s, as the earlier runs that got that far did 2 of 2 runs: the credits finish, a black loading screen follows for about 2.5 min, and the title plays the next scene ("Station AI: Initiating environmental optimization sequence" in one run, the hangar dialogue "All right, let's do it. Just get in and get out." in the other)
    without: [audioout2] t=400.479 ctx 0x7fb4fa30e6b0: 4.1 pushes/s, 4.1 advances/s, 15.5 queue polls/s, hw queue 1/4, ...; totals: pushes 1081 (1081 blocking, 0 queue-full rejects), primes 0, dropped 0
    with:    [audioout2] t=400.083 ctx 0x7f9abef447b0: 46.9 pushes/s, 46.9 advances/s, 189.6 queue polls/s, hw queue 3/4, ...; totals: pushes 18615 (18615 blocking, 0 queue-full rejects), primes 0, dropped 0
    

    Under the dummy driver only one AudioOut2 context gets a device (SDL_OpenAudioDevice failed: Audio device already open), so the engine's context runs on the device-less clock. A run with the SDL disk driver, where both contexts got a device, stopped at the same point without this change (6-10 pushes per second).

  • tools/check_conventions.py --base origin/main passes and git diff --check is clean.

Checklist

  • Based on current main; no other open PR implements the same functions (docs(libSceAudioOut): count the blocking values passed to sceAudioOut2ContextPush #12698 only documents the blocking values titles pass)
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: PPSA02530 (the real-time scene after the opening film, together with the other PRs above)
  • GPUs tested: n/a
  • AI-assisted: yes

…ueue has room

A blocking sceAudioOut2ContextPush waited only before queueing its grain, so the push that filled the queue returned at once. PRAGMATA's Wwise feeds its context from a thread that pushes and then wakes the renderer; the renderer sees a full queue, sleeps, and is woken again only by the game's next RenderAudio, so the mix advanced four grains per game frame. A blocking push now also waits after queueing until a grain finishes, bounded by the existing 200 ms limit.
…PPSA02530

The wait is described in the debt file instead of in code comments.
@github-actions github-actions Bot added the unblocks title Names a title ID on the Unblocks line label Oct 29, 2026

@rooklz rooklz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head 7a75ea95b3 against main bb04a4f1ea. The merge preview is clean, git diff --check is clean and tools/check_conventions.py --base upstream/main passes.

What I ran

I built a standalone harness on macOS arm64 (Apple clang, ASan+UBSan). It copies GrainDuration, Drain, the device-less QueueLevel branch, WaitForRoom and the blocking/non-blocking control flow of sceAudioOut2ContextPush verbatim from this head. SDL, rendering and tracing are left out. The new test's rows run against it unchanged (depth 2, 1024-frame grains).

Build PR rows Notes
this head pass (rounds 1-3 take 22-24 ms each, level 1 afterwards)
drop the post-queue WaitForRoom (equivalent to main) 3 rows fail (level 2 after the push) the test catches the fix
drop the pre-queue WaitForRoom pass survives, see 2

Other measurements from the same harness:

  • A free-running blocking feeder on depth 4 makes 50 pushes/s, which is the playback rate (46.9/s). The pacing works as the body describes.
  • Four blocking pushes into an empty depth-4 queue now take about 23 ms. On main they took 0 ms because the fourth push waits for the oldest grain. This is the observable change for titles that prime the queue before they start their clock. The debt line says the console behaviour is unmeasured, which is the right place for it.
  • If the queue never drains, one blocking push now takes about 404 ms: two 200 ms waits. On main it took 200 ms. The debt line already says "each wait ends after at most 200 ms".

Findings

  1. Comments in the test. The three // lines above TestBlockingPushWaitsForRoomWithoutADevice are not technical-debt comments. The CI checker accepts them only because the PR also touches docs/dev/debt/. AGENTS.md and CONVENTIONS.md still allow only debt comments, #endif and namespace ends, so please drop these lines and keep the explanation in the PR body.
  2. The pre-queue wait is not covered by a test. With the post-queue wait in place, a blocking push only finds the queue full on entry if the previous push gave up after 200 ms. Removing the first WaitForRoom passes every row. If it is meant to keep main's "wait before queueing" behaviour after a timeout, a row with a stalled queue would pin it down. Otherwise it could go, which would also bring the worst-case stall back to 200 ms. This is non-blocking.
  3. Non-blocking, wording: the debt line cites PPSA02530 as the inference. Since the change itself is generic, that is fine. "Inferred from one title's feeder pattern" is still the only evidence, so a console timing of sceAudioOut2ContextPush(ctx, 1) on a fresh depth-N context (when the Nth push returns) would settle it.

Not verified

I did not build the real audio_out2_timing target. It needs the SDL2 submodule, which is not initialised here. I did not run the device path (device != 0, DeviceQueueLevel), Windows or a console.

AI-assisted: yes (Claude Code)

@adriadam10 adriadam10 self-assigned this Oct 29, 2026
@JJenkx

JJenkx commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Windows run of this head (7a75ea95b3), merged onto main bb04a4f1ea (clean merge, local only). Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, WinLibs GCC 15.2.0 UCRT r7, Ninja, Release, BUILD_TESTING=ON, no Vulkan driver visible.

  • This PR: audio_out2_timing passes 3 of 3 (ctest --repeat until-fail:3).
  • Negative control: with this PR's source files (core/libs/prx/libSceAudioOut/src/AudioOut2Context.cpp) put back to main's version and its tests kept, audio_out2_timing fails.

AI-assisted: yes (Claude Code).

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@oneandonlydean

Copy link
Copy Markdown
Collaborator

@hugodearaujo thanks, the analysis of the title's ticker/feeder pattern is clear, the non-blocking path is untouched, and each new wait is bounded (200 ms), so a push can't hang when nothing calls Advance. Three things before this can land:

  1. The test needs to be deterministic. It checks that a full-queue push took at least half a grain. If the test thread is descheduled between pushes long enough for playback to drain a slot, correct code returns at once and the assertion fails; and old code can pass if it is descheduled after queueing, before the end timestamp. Please drive it with a handshake instead of elapsed time: playback/clock progress under the test's control, a check that the push is blocked while the queue is full, and that it returns once a slot frees, plus a negative control that fails on the old code.

  2. Evidence on a real audio device. The numbers in the description are from runs without an audio device (SDL dummy driver), and the one device-backed run was without this change. Since this changes when every blocking AudioOut2 push returns, please post a before/after run of that title on a real device (pushes per second, underruns/drops, whether the scene runs past the credits), with the raw logs and the exact build you ran (a build of main plus open PRs makes it hard to tell what changed). The debt entry rightly says the console's return point isn't measured; please keep that. We'll run a before/after on another AudioOut2 title that pushes from its own audio thread (queue level, drops, audible crackle) on our side.

  3. Destroy while a push is waiting. The new wait after queueing releases the context lock, so sceAudioOut2ContextDestroy can delete the context before the pusher re-locks it. The earlier wait already had this window, so it isn't new, but this adds another one. Please either make sure a waiting push is woken and leaves before the context is freed (with a deterministic test), or say in the debt entry that destroying a context with a push in flight isn't supported.

I checked #12863, which edits the same file: the two combine cleanly in either order.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests run and reviewed — console behavior unverified

The focused host test passes on the current head and the base-source negative control aborts, but the console’s blocking return timing remains unmeasured.

What I ran

Head bb6d79e52e3d8ec72a604af30d0e4429b193cf66; base bb04a4f1ea0175e9e4f149f0359a764d62ff3296. git merge-tree produced a clean preview tree (9f11fc6a2c604b83f18d0f53e115185ac6c13a9f); no merge was attempted. Environment: Debian Linux x86_64, kernel 6.1.0-54-amd64, GCC 12.2.0, QEMU x86_64 Haswell.

cmake --build /dev/shm/AnyPS5-build-12915-full --target audio_out2_timing_tests -j2
timeout 240s qemu-x86_64 -cpu Haswell /dev/shm/AnyPS5-build-12915-full/tests/audio_out2_timing_tests > /tmp/AnyPS5-12954-audio-out2-timing-pr-exact-final.log 2>&1
python3 tools/check_conventions.py --base upstream/main
git diff --check bb04a4f1ea0175e9e4f149f0359a764d62ff3296...HEAD
git merge-tree --write-tree upstream/main HEAD

Results

Check Result
audio_out2_timing_tests build Pass
Focused executable under QEMU Haswell Pass, exit 0
Conventions Pass
git diff --check Pass, no output
Hosted conventions, eligibility, labels, report Pass
Hosted Windows job In progress when checked
Hosted Linux and specialized jobs Skipped
Build output
[0/2] Re-checking globbed directories...
[1/3] Building CXX object core/libs/prx/libSceAudioOut/CMakeFiles/libSceAudioOut.dir/src/AudioOut2Context.cpp.o
[2/3] Linking CXX shared library core/libs/libs/unpatched/libSceAudioOut.prx
[3/3] Linking CXX executable tests/audio_out2_timing_tests
Focused test output
exit=0
qemu-x86_64: warning: TCG doesn't support requested feature: CPUID.01H:ECX.x2apic [bit 21]
qemu-x86_64: warning: TCG doesn't support requested feature: CPUID.01H:ECX.tsc-deadline [bit 24]
qemu-x86_64: warning: TCG doesn't support requested feature: CPUID.07H:EBX.hle [bit 4]
qemu-x86_64: warning: TCG doesn't support requested feature: CPUID.07H:EBX.invpcid [bit 10]
qemu-x86_64: warning: TCG doesn't support requested feature: CPUID.07H:EBX.rtm [bit 11]
Convention check output
All conventions checks passed.

Negative control

Assertion Change to code under test Did the new test fail?
The PR’s new blocking-push regression test Restored AudioOut2Context.cpp from base bb04a4f1ea0175e9e4f149f0359a764d62ff3296, keeping the PR test source Yes; the test process aborted with exit 134. Require does not print which assertion failed.
Negative-control output
negative-control-exit=134
qemu: uncaught target signal 6 (Aborted) - core dumped

Findings

Blocking

None found in the focused host implementation/test review.

Non-blocking

  • The code now waits both before and after enqueuing. If the queue does not drain, one call can spend nearly the 200 ms timeout in each loop (roughly 400 ms plus scheduling/processing); the debt note currently describes the per-wait limit. Please confirm that the combined bound is intended and document it clearly.
  • The debt note correctly says the console’s blocking return point is unmeasured. The local test verifies the chosen host behavior, not console ABI behavior, so I left a comment-only review instead of approving. A console trace of push return time against queue level would resolve this uncertainty.

Sources

Not verified

  • Console timing or PS5 ABI behavior.
  • Device-backed SDL audio path or native host execution.
  • Full audio_out CTest suite; the PR body reports it, but I did not rerun it.
  • Windows job was still running; hosted Linux and specialized jobs were skipped.

To get this to a good state

Please confirm the combined timeout is intended and provide a console timing trace if one becomes available; I will rerun these same commands on the new head.

@rooklz rooklz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at bb6d79e52e. The only change since 7a75ea95b3 is commit bb6d79e52e, which removes the three // lines above TestBlockingPushWaitsForRoomWithoutADevice. That settles my point 1. The production code (42beeaa468) and the debt line (7a75ea95b3) have not changed.

What I ran

  • python3 tools/check_conventions.py --base upstream/main: pass
  • git diff --check upstream/main...HEAD: clean
  • git merge-tree --write-tree upstream/main HEAD against bb04a4f1ea: clean
  • git diff upstream/main...HEAD | grep '^+.*//': no added // lines

My harness results from the first review still apply because the code under test is unchanged: the PR rows pass, dropping the post-queue wait turns 3 rows red, and dropping the pre-queue wait survives.

Still open

  • These are still optional and non-blocking: my earlier point 2 (a stalled-queue row to cover the pre-queue WaitForRoom, or drop that wait) and point 3 (a console timing of the Nth blocking push on a fresh depth-N context).
  • oneandonlydean's three points are not addressed yet: a handshake-driven deterministic test instead of the elapsed-time check, before/after evidence on a real device, and destroy while a push is waiting. I agree the elapsed-time assertion can flake both ways under scheduler preemption. A clock or Advance hook controlled by the test would fix that and would also give a negative control that fails on main.

AI-assisted: yes

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — changes requested

The elapsed-time regression test can fail with correct code under scheduler delay and can pass with the old code, so its current negative control is not reliable.

What I ran

Head bb6d79e52e3d8ec72a604af30d0e4429b193cf66; base bb04a4f1ea0175e9e4f149f0359a764d62ff3296. The merge preview is clean; no merge was attempted. Environment: Debian Linux x86_64, kernel 6.1.0-54-amd64, GCC 12.2.0, QEMU x86_64 Haswell.

cmake --build /dev/shm/AnyPS5-build-12915-full --target audio_out2_timing_tests -j2
timeout 240s qemu-x86_64 -cpu Haswell /dev/shm/AnyPS5-build-12915-full/tests/audio_out2_timing_tests > /tmp/AnyPS5-12954-audio-out2-timing-pr-exact-final.log 2>&1
python3 tools/check_conventions.py --base upstream/main
git diff --check bb04a4f1ea0175e9e4f149f0359a764d62ff3296...HEAD
git merge-tree --write-tree upstream/main HEAD

Results

Check Result
audio_out2_timing_tests build Pass
Focused executable under QEMU Haswell Pass, exit 0
Conventions Pass
git diff --check Pass, no output
Hosted Windows, conventions, eligibility, labels, report Pass
Hosted Linux and specialized jobs Skipped
Focused test output
exit=0
qemu-x86_64: warning: TCG doesn't support requested feature: CPUID.01H:ECX.x2apic [bit 21]
qemu-x86_64: warning: TCG doesn't support requested feature: CPUID.01H:ECX.tsc-deadline [bit 24]
qemu-x86_64: warning: TCG doesn't support requested feature: CPUID.07H:EBX.hle [bit 4]
qemu-x86_64: warning: TCG doesn't support requested feature: CPUID.07H:EBX.invpcid [bit 10]
qemu-x86_64: warning: TCG doesn't support requested feature: CPUID.07H:EBX.rtm [bit 11]
Convention output
All conventions checks passed.

Negative control

Assertion Change to code under test Did the new test fail?
New blocking-push regression test Restored AudioOut2Context.cpp from base bb04a4f1ea0175e9e4f149f0359a764d62ff3296, leaving the PR test source in place Yes in this run: test process aborted with exit 134; Require does not print the failing assertion. The timing-sensitive test can still pass under a different scheduling delay.
Negative-control output
negative-control-exit=134
qemu: uncaught target signal 6 (Aborted) - core dumped

Findings

Blocking

  • The regression test is scheduling-dependent. TestBlockingPushWaitsForRoomWithoutADevice asserts that each measured push takes at least half a grain and checks queue level after return. If the test thread is descheduled between pushes, playback can drain a slot before a correct call begins, making the elapsed-time assertion fail. If it is descheduled after recording start but before calling Push, that delay can satisfy the threshold on the old implementation; a pause after the old call returns can also let the queue-level check observe a drained slot. Please replace the elapsed-time oracle with a handshake and test-controlled playback clock/advance: establish a full queue, verify the push remains blocked, release one slot, and verify it then returns. Run the same test against the base implementation as a deterministic negative control.

Non-blocking

  • No changed-code before/after run on a real audio device or console was available to me. The PR itself says its title runs used SDL dummy/no-device audio, and its device-backed run was without this change; the device-backed path and console return condition remain unverified.
  • The post-enqueue wait unlocks the context while sleeping. The existing pre-enqueue wait also unlocks, but this adds another interval in which concurrent ContextDestroy could race with a waiting push. I could not establish whether the API permits destroy concurrent with push; please ensure the lifetime is safe or document that restriction.

Sources

Not verified

  • Console timing, PS5 ABI behavior, or device-backed SDL behavior.
  • Full audio_out CTest suite; the PR body reports it, but I did not rerun it.
  • Native host execution; the focused executable was run under QEMU.

To get this to a good state

Replace the timing-based test with a synchronized clock/advance test and confirm the push/context-destroy lifetime contract. I will rerun these same commands on the new head.

@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 5021 docs/dev/debt/libSceAudioOut.md
PR 9154 core/libs/prx/libSceAudioOut/src/AudioOut2Context.cpp
PR 12863 core/libs/prx/libSceAudioOut/src/AudioOut2Context.cpp
docs/dev/debt/libSceAudioOut.md

…e; note destroy during a push is unsupported

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@rooklz rooklz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at 1ed07de320. The production code (42beeaa468) has not changed. Commit 1ed07de320 replaces the elapsed-time check in TestBlockingPushWaitsForRoomWithoutADevice with a queue-level check after each push. It also adds one sentence to the debt line: destroying a context while a push waits on it is not supported.

What I ran

  • python3 tools/check_conventions.py --base upstream/main: pass
  • git diff --check upstream/main...HEAD: clean
  • git merge-tree --write-tree upstream/main HEAD against bb04a4f1ea: clean. No added // lines.
  • Standalone harness on macOS arm64 (Apple clang 21), built from slices copied verbatim from this head: FULL_WAIT_*, GrainDuration, Drain, the device-less QueueLevel branch and WaitForRoom. Push is reduced to pre-wait / queued++; playHead += grain / post-wait. The four-round loop and level < queueDepth && available > 0 match the new test (depth 2, 1024-frame grains).
variant -O2 -O2, all cores busy (yes × ncpu) ASan+UBSan TSan
head 0/100 fail 0/100 fail 0/50 fail 0/20 fail
post-queue wait dropped (= main) 100/100 fail 100/100 fail 50/50 fail
pre-queue wait dropped 0/100 fail

One run takes 0.07 s.

Why the new check is deterministic

Once a push returns, nothing else adds to the queue before GetQueueLevel, and Drain can only lower the level. So correct code cannot fail because of a scheduler delay. The only way it could fail is the 200 ms timeout expiring while the queue is still full, and a 21 ms grain always drains well before that. Old code returns at level 2 in rounds 1 to 3. It could only pass if the thread were descheduled for more than one grain between the push and the poll, in all three rounds, and that never happened even with every core busy. The flake that MarcZX10 and oneandonlydean pointed out (point 1) is fixed.

Still open

  • oneandonlydean's point 2: before/after evidence on a real audio device. This review cannot settle that.
  • Point 3 is now handled by the debt sentence, which was one of the two options offered.
  • Optional, same as before: the pre-queue WaitForRoom is still not covered by a test (the mutant survives). A stalled-queue row would cover it, or that wait could be dropped.

AI-assisted: yes

@hugodearaujo

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews, and @adriadam10 for the level-based test and the destroy note. Status:

  1. Deterministic test: done in 1ed07de.

  2. Real device, before and after with exact builds (PRAGMATA): both builds are main bb04a4f plus the open PRs the title needs to reach that scene on current main (fix(libScePad): return an error for unsupported port probes #10935, fix(shader): load and store 16-bit data of converted packed images #11580, fix(agc): keep at most 4 GiB of the host heap from the host import budget #7844, fix(libc): leave a range that a mutation waits for out of new leases #6565, fix(libSceAvPlayer): keep synced video at the presentation clock when the title takes frames slowly #12898, fix(agc): keep buffer owners when a staged address space dissolves #10304, fix(agc): refuse writable buffer feedback with native depth #10497, fix(agc): prepare a draw or dispatch again when the memory it captured changed before it was recorded #7446, fix(agc): enable independentBlend for color targets with different blend states #6782; heads in the description), with and without this PR's head; only libSceAudioOut.prx differs and there are no local diagnostics. SDL's PulseAudio backend into a PipeWire null sink, both AudioOut2 contexts device-backed, the same warmed shader cache, 1400 s each:

    Sound engine context Underruns (primes) Drops Scene
    without 634 pushes by 1029 s (0.0-0.3 per second) 104 0 black from 763 s to the end, no loading screen
    with 46.9 per second, 46735 pushes by 1000 s 0 0 the credits finish, loading screen from 929 s

    The run with this PR later stops on an unrelated Vulkan device loss in the next load (vkQueueSubmit recorder: Vulkan result -4; DISPATCH_INDIRECT), which I'm looking at separately. I can attach the raw logs. The console's return point is still unmeasured, and the debt entry keeps saying so.

  3. Destroy while a push waits: recorded as unsupported (1ed07de).

@MarcZX10 the combined bound is intended: 1f90599 adds to the debt entry that on a queue that never drains a blocking push returns after up to 400 ms (200 ms before queueing, 200 ms after).

@rooklz on the pre-queue wait: I'd keep it. Without it, a blocking push that finds the queue already full (filled by non-blocking pushes, by another thread, or after a previous timeout) would queue a grain beyond the depth, which main doesn't do. Its end state matches the post-queue wait's, so a level check after the push can't tell them apart; it can only be pinned while the push is still blocked. I have a deterministic row for that (playback stalled, the queue filled with non-blocking pushes, the blocking push must keep its grain out until a slot frees; removing the pre-queue wait fails it 3/3), but it needs the 200 ms limit to become a per-context field the test can raise. I left it out to keep this PR small; I can add it if you'd like.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked — new head, changes requested

The elapsed-time threshold is gone, but the new queue-level assertion can still pass the old behavior if the test thread is descheduled after Push returns and before it reads the queue.

What I ran

  • Exact PR head: 1f905993a3463cc9aa39460b77491bc9eb4405d7; recorded PR base: bb04a4f1ea0175e9e4f149f0359a764d62ff3296.
  • Current upstream/main: 1189ec3003672bb364dd9f3119ead7b3976e3c83; merge preview is clean, tree 60a6cfd16dca697446a9d58dc37cc99ca499ae43.
  • Environment: Linux workspace; no compiler, build or test command was run.
git merge-tree --write-tree upstream/main refs/review/pr-12954-current
git diff --check upstream/main...refs/review/pr-12954-current
python3 tools/check_conventions.py --base upstream/main --head refs/review/pr-12954-current

Results

Check Result
Current-main merge preview Clean
git diff --check Passed; no output
Conventions Passed
Hosted Windows job In progress when refreshed
Product Linux and specialized jobs Skipped by workflow policy
Focused tests and PRAGMATA run Author-reported in the latest report; not independently run here
Unedited local command output
$ git merge-tree --write-tree upstream/main refs/review/pr-12954-current
60a6cfd16dca697446a9d58dc37cc99ca499ae43
$ git diff --check upstream/main...refs/review/pr-12954-current
[no output; exit status 0]
$ python3 tools/check_conventions.py --base upstream/main --head refs/review/pr-12954-current
All conventions checks passed.

Negative control

Assertion Mutation Result
After a blocking push, queue level is below queueDepth Remove the post-enqueue WaitForRoom Not exercised in this review. The current test reads queue level after the call using the live clock, so a scheduling pause can let the old implementation pass.

Findings

Blocking

TestBlockingPushWaitsForRoomWithoutADevice checks the queue after each synchronous push, but does not control playback or observe that the full-queue push is blocked. QueueLevel advances from the live clock; if the test thread is paused for a grain after the old implementation returns, the queue drains before the assertion and the regression is missed. Please make the playback/clock progression test-controlled (or use an equivalent handshake), assert that the full-queue push remains blocked until a slot is released, and show that the old implementation fails the same test.

Non-blocking

The author records context destruction concurrent with a waiting push as unsupported; console return timing remains unmeasured. I did not find a new source-level blocker in those stated limitations.

Sources

Not verified

I did not build or run the focused test locally, run a negative control, or verify console behavior. The test and title results cited above are author-reported.

To get this to a good state

Add a controlled blocking/release test and a red control using the previous push behavior. I will rerun the same source checks and review the exact new head.

…back frees a slot

A device-less context can run its queue model on a clock the test sets. With the clock frozen, a blocking push that fills the queue is seen still waiting with its grain queued and not counted, and returns once the test plays one grain; a blocking push that meets a queue filled by non-blocking pushes does not queue past the depth.
@hugodearaujo

Copy link
Copy Markdown
Contributor Author

@MarcZX10 thanks. The new head adds a test clock for the device-less queue model (clockForTest on the context, used by the queue level and by both waits, including the 200 ms limit), so playback only advances when the test moves it.

  • TestBlockingPushStaysBlockedUntilPlaybackFreesASlot freezes the clock and fills the queue with a blocking push from another thread. Under the context lock it sees the grain queued but the push not yet counted (queued == depth, pushes == depth - 1), with the future still pending. It then plays one grain on the test clock, and the push returns 0 with the level at depth - 1.
  • TestBlockingPushOnAFullQueueQueuesOnlyOnceASlotIsFree covers the pre-queue wait (@rooklz's point 2): with the queue full of non-blocking pushes, a blocking push never queues past the depth, and it returns once grains are played.

Red controls, 3 of 3 each: main's AudioOut2Context.cpp (bb04a4f) fails pushes == depth - 1 deterministically, because on the old code queueing and counting the push are one critical section; dropping the post-queue wait fails the same assertion; dropping the pre-queue wait fails queued <= depth. The head passes 3 of 3, ctest -R audio_out passes 52 of 52 three times, and the level-based test from 1ed07de is unchanged.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked — new head, test concern addressed

The test-clock changes at head 3fc5485e6c6cc0b25e27d9db9e1b7c3504f56695 replace the scheduler-sensitive oracle with controlled blocking and playback checks, addressing the prior test blocker.

I inspected the new test and clock-injection diff against the prior reviewed head 1f905993a3463cc9aa39460b77491bc9eb4405d7. The author reports 52/52 audio_out tests passing three times and deterministic red controls for the base implementation and for removing either wait; I did not run them locally. The current-main change in the shared implementation file since the PR base is a pad-rumble call and does not touch this wait path. The hosted Windows job was still in progress when refreshed; Linux and specialized jobs are skipped. Console blocking-return timing remains unmeasured, so this is a comment-only recheck, not an approval.

@github-actions github-actions Bot added docs prx needs rebase Conflicts with main, rebase needed labels Oct 30, 2026
# Conflicts:
#	core/libs/prx/libSceAudioOut/src/AudioOut2Context.cpp
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 30, 2026
@MarcZX10

Copy link
Copy Markdown
Contributor

Re-checked — host tests pass, console behavior unverified

The test-clock regression removes the scheduler-sensitive assertion, and the exact head 56fd9eac87759199a6edb81586b236c99a5c2706 has a passing Windows job; Linux was skipped. The PR still describes the pre-queue and post-queue wait behavior as inferred and says console return timing is unmeasured (debt note at this head); the host test cannot confirm that target behavior, so I’m withholding approval pending evidence that establishes it.

@hugodearaujo

Copy link
Copy Markdown
Contributor Author

@MarcZX10 thanks for re-checking. A payload probe for exactly that is ready: a fresh depth-4 context with no ports, a non-blocking phase and then blocking pushes with the return time and queue level logged around each one, so it shows when the console's Nth blocking push returns. I'll run it on the console as soon as I have access to it again and post the raw lines here; the debt entry stays as it is until then.

@adriadam10
adriadam10 merged commit 01069f4 into boykopovar:main Oct 30, 2026
11 of 12 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked the blocking push's two waits against current main 96ff704: the merge is clean, the new AudioOut2FindContext registry keeps both waits, and the test-clock tests were checked against the red controls reported in this thread (rooklz, JJenkx). The console's blocking-push return time stays unmeasured, as the debt entry says; merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs prx unblocks title Names a title ID on the Unblocks line

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants