OpenPSF: Corrections for PS2 playback - #211
Open
Wedge009 wants to merge 17 commits into
Open
Conversation
MAINThread()'s silence check counted zero samples across the whole interleaved L+R buffer, so a single fully-silent channel (a mono voice, or one that simply has no more streamed data) was enough on its own to discard the entire block, including a still-playing other channel's audio. Check each channel's silence independently instead, and only drop the block if both are quiet. Separately, a channel's ADPCM read pointer (pCurr) had no bounds check against the 2MB SPU2 memory buffer. A channel whose stream never produces a real stop/loop flag would walk pCurr straight past the end of the buffer and read whatever memory happened to follow it in the process. Treat running outside the buffer the same as the existing explicit stop sentinel. Neither fix is a complete solution for tracks using Neill Corlett's generic 'streaming PSF2 driver' (eg the FFIV PSX extraction): the driver's read-ahead buffer still never refills past its first ~256KB per channel, for reasons not yet discovered. These fixes stop two real, independent bugs from making that existing problem worse (silently losing already-buffered audio, and reading out of bounds), without fixing the underlying streaming stall.
…annel The out-of-bounds guard on the per-voice ADPCM decode pointer treated a pointer reaching exactly the end of the 2MB spuMem buffer as an error and permanently disabled the channel. spuMem is a power-of-two sized address space, and real SPU2 hardware naturally wraps a voice's address register at that boundary rather than faulting, so mirror that instead: wrap the pointer back to the start of the buffer. This also avoids a regression for streaming drivers whose read-ahead logic polls a voice's address to decide when to request more data - halting the channel froze that reported position, which in turn stalled the driver's own refill logic indefinitely.
mips_commit_delayed_load() indexed mipscpu.r[] with delayr without excluding the REGPC (32) sentinel value, which marks a pending delayed branch rather than a pending register load. Three of its four call sites already guard against this, but mips_delayed_branch() calls it unconditionally, so a branch instruction sitting in another branch's delay slot could write one past the end of the 32-entry register array, landing on the adjacent cp0r[0] (INDEX) register. Found via UndefinedBehaviorSanitizer while investigating an unrelated playback issue; confirmed the fix eliminates the finding.
Refines 0b63849: wrapping at the full 2MB spuMem array (rather than stopping the channel) avoided the immediate hang, but let a voice's reported playback address grow far beyond the ~128KB window this streaming driver's own read-ahead accounting expects. That accounting saturates a clamped calculation once the reported address exceeds its assumed range, permanently zeroing the driver's computed "room available" and stalling its refill logic for tens to hundreds of seconds at a time. Wrapping within the channel's own pStart- anchored window instead keeps the reported address within the range the driver was built to handle. WIP: this measurably improves playback (confirmed via reference comparison: continuous audio across most of the track's length, versus a few seconds before) but does not fully resolve it - the right channel still drops out intermittently before the file's later section. The 0x20000 window size is derived from this file's observed behaviour, not a known hardware or driver constant, and may need revisiting. Marking as a checkpoint before further attempts.
MAINThread() only delivered a decoded block if fewer than 20 of its 735 samples were exactly zero in each channel. Real audio crosses zero constantly even when quiet, so this threshold triggered on ordinary passages, not just silence - and a dropped block isn't muted, it's erased from the output, since playback progress is tracked by how much audio has been delivered rather than a fixed timer. This caused a systematic, cumulative duration shortfall on any track with real pauses (confirmed exactly on one title: 617 dropped blocks accounted for the entire ~10.3s gap between the rendered and expected length), and explains audible skips landing at rhythmic quiet points elsewhere in a track. Deliver every block unconditionally instead. A genuinely silent block costs nothing to deliver correctly.
ced284f's pStart-anchored wraparound assumes a channel's own 128KB window always fits inside the real 2MB spuMem array, but pStart is set from a 20-bit register field and can legitimately be placed as little as 2 bytes from the buffer's end. When that happens, wrapping start to "pStart + (offset within 0x20000)" can still land tens of KB past the real allocation, reading unmapped memory - a reliably reproducible segfault confirmed via gdb backtrace (peops2/spu.cc:490, out-of-bounds channel decode pointer). Treat that case the same as the existing sentinel-stop condition instead of computing another guessed address: there's no sane data to continue decoding from a pStart placed that close to the end of the buffer, and trying anyway (eg wrapping within the full 2MB array) just trades the crash for a channel that can read unrelated bytes indefinitely without ever hitting a real stop flag. Verified via gdb that the original crash (reproducible ~00:37 into Final Fantasy X-2's "Yuna's Theme") no longer occurs.
call_irq_routine() (and its two inline duplicates in psx_bios_exception() for the VBlank and root-counter IRQ paths) hand the interpreter to a driver-registered handler address and spin until that routine executes a synthetic return trap. If the handler ever derails - runs into non-code memory and hits an unrecognized opcode, which has nowhere sane to land since there's no real BIOS/kernel here - it never reaches the trap and the loop spins at 100% CPU forever, with no recovery short of killing the process. Reproduced for real on FFX-2's Crimson Squad.psf2. Cap the wait to a fixed cycle budget and abandon a misbehaving handler instead of hanging indefinitely.
30ba81a's 10000-cycle budget was too tight: FFX-2's Game Over and Good Night each have a driver handler that finishes correctly but takes longer than that to reach its return trap, so it was getting abandoned mid-execution - which left both tracks completely silent for the rest of playback (previously they just played, with an unrelated early-restart bug). Raising the budget to 40960 cycles lets these legitimately-slow handlers finish while still bounding Crimson Squad's genuinely-broken one (confirmed: still completes instead of hanging, same 43264 abandoned attempts as before).
…ices ced284f wrapped a channel's decode pointer within a 128KB window to keep a streaming driver's read-ahead accounting happy (see the ffiv_streaming_driver_stall investigation) - but that window is arbitrary, and FFX-2's "Game Over"/"Good Night" jingles and FFXI's "Dash de Chocobo (Variation)" all have more real content than 128KB, so the wrap fires well before the track's real end and replays already-played material - a premature loop. The real distinguishing signal is whether a channel's data is still being actively streamed in (fresh DMA writes landing) or was fully preloaded once, up front (the common case for a normal sequenced driver). Track a high-water mark of the furthest point any SPU2 DMA write has ever reached (g_spuMem_write_high), and branch cleanly on recent DMA activity instead of blending the two signals into one condition (an earlier attempt at this did blend them and broke both cases in different ways - see early_loop_regression notes): - While DMA has written data recently, keep the existing small pStart-relative window wrap completely unchanged - this is what an actively-streamed voice (eg FFIV's hacked Corlett streaming driver) needs to keep its read-ahead accounting alive. - Once DMA has been idle for a couple of seconds, there's nothing left to stream in, so fall back to the write high-water mark instead: real data below it is read straight through with no intervention, and only genuinely never-written memory past it ends the channel cleanly. Verified: - FFX-2 Game Over and Good Night: no more early loop, real audio restored with a natural fade instead of an abrupt cut-off. - FFXI Dash de Chocobo (Variation): no more early stop/silence gap, plays its full length including the fade out.
…reeze Every blocking IOP HLE syscall (DelayThread, WaitSema, SleepThread, etc.) is only ever reached through psx_iop_call(), which is only ever invoked from the interpreter's "addiu $0,N" special case - and by this code's own generated-code convention, that instruction is always the delay slot of an immediately-preceding "jr $ra" (the return half of every IOP export-table stub: "jr $ra; addiu $0,callnum"). So whenever one of these syscalls freezes its own thread, mipscpu.delayr is still REGPC (a not-yet-committed pending branch from that jr) and mipscpu.delayv already equals $ra - the same value FreezeThread() separately captures as the thread's resume PC. FreezeThread() was snapshotting delayr/delayv unconditionally, saving this pending branch as if it was independent state to restore later. It isn't: on thaw, PC gets set correctly to the saved resume point, but the redundant "still pending" branch also gets restored and wrongly re-fires on the very next instruction's delay-slot commit, silently diverting control flow - sometimes on an entirely different thread - into whatever memory that stale target happens to reference, including non-code data. Clear the saved delayr/delayv in this specific, provably-safe case (flag=1 freeze with a pending REGPC branch) - the resume PC already captures its effect, so nothing is lost by dropping the duplicate. Verified: FFXI's "Dash de Chocobo" previously played ~47 seconds of silence before any real content, long attributed to a genuine driver-authored rest in the track's own sequence data. It wasn't - it was this bug corrupting a thread's control flow for the whole duration. With the fix, the file plays real audio from t=1s through its full length, matching a Highly Experimental reference render (waveform-compared by ear and by eye).
…ll() psx.cc's OP_ADDIU case (the interpreter's HLE-dispatch marker) always called mips_advance_pc() right after psx_iop_call(), regardless of what that call did. Most HLE dispatches rely on exactly this: they set PC to (target - 4) and depend on this same advance to complete the jump with its final +4. But when the call triggers a blocking syscall or any other handler that reschedules on to a different thread, mipscpu no longer belongs to the thread that made the call - the unconditional advance then silently corrupts whichever thread is now active by skipping one of its instructions, unevaluated. This is what permanently truncated FFX-2 Vegnagun Starting's per-song instrument-descriptor table: the copy loop's own loop-continuation branch got skipped this way, mid-copy, and the thread doing the copy never got scheduled back in to finish it, leaving roughly half the table blank for the rest of playback. Track which kind of freeze last suspended a thread (already-correct $ra based vs. plain PC-based, see FreezeThread()) so psx_iop_call() can tell its caller whether it's still safe to advance, and - when it isn't - complete a still-pending "target - 4" jump directly on the affected thread's own saved state instead of on whatever thread is now live.
WaitSema unconditionally set $v0 after its if/else block, even when the else branch just froze the calling thread and rescheduled - which can switch mipscpu to a completely different thread before that write runs, silently stomping whatever that thread was doing instead. Confirmed causing FFX-2 Crimson Squad's runaway copy loop (a corrupted loop-exit register never terminates, eventually overwriting a live IRQ handler) and FFX-2 Last Mission's Wind Crest silence (the same mechanism, different thread perpetually blocked on the same semaphore). Move the return-value write into the immediate-success branch only. SignalSema/iSignalSema now set the woken thread's own saved $v0 directly when they ready it, since removing the old unconditional write means the resuming thread otherwise never gets its correct return value.
CreateThread()'s priority argument was read into an unused "refCon" field and never consulted, so ps2_reschedule() picked whichever ready thread came up next in a pure round-robin scan - treating the music sequencer thread the same as low-priority housekeeping work. It now tracks the lowest-priority-value (most urgent) ready thread across a full wrap-around scan, with a guard so a thread that hasn't yielded on its own only gets pre-empted by something strictly more urgent. Deliberately narrow: does not include the separate DelayThread-rate fix, which has its own regression history and is not yet re-attempted.
DelayThread() sets waitparm in raw IOP clock cycles at the true 36864000 Hz rate, but the per-sample countdown was decrementing it by CLOCK_DIV (8) instead of IOP_CYCLES_PER_SAMPLE (836) - every delay- based wait took roughly 104x longer than the driver actually requested. This was previously reverted after combining it with an early, less careful priority-scheduling fix caused severe regressions (FFX-2 Mission Start/Mission Complete going silent); those turned out to be interactions with separate thread-corruption bugs since fixed (WaitSema, FreezeThread, mips_advance_pc-after-psx_iop_call). Shrinks the start-up silence gap measurably (~0.3s earlier first audible content on both FFX-2 Chocobo and Yuna's Theme) but not fully - most of that gap is scheduling granularity, not wait-rate, and needs the separate sub-slice scheduling work to close further.
Some streaming drivers, such as the generic PS2 streaming driver used for Final Fantasy IV, kick a single 32KB DMA transfer per channel but only refill the first 16KB with genuinely fresh data each cycle - the second half is whatever was last written there, which for a freshly allocated channel ring is permanently zero. SPU2writeDMA4Mem wrote that stale zero straight into spuMem, producing recurring silent ADPCM blocks on playback. Past the first 8192 words of a transfer landing in a channel's own ring, treat an exact-zero source sample as 'not actually new data' and leave the existing spuMem content alone instead of overwriting real audio with a spurious zero - the correct data for that position typically arrives one refill cycle later via the next kick's own first half. Gated on value (exact zero) rather than transfer size, since some titles legitimately use >16KB single-channel transfers whose tail carries real data; confirmed via direct instrumentation that those titles' tails are never exactly zero and are unaffected. This does not fully fix FFIV: the left channel is still substantially misaligned from the reference, and this fix trades the original silence for an audible splice on the right channel at the substitution points. Root cause for both remains open.
Member
Author
|
While OpenPSF is still the official PSF plug-in, it's horribly broken. At least these mitigate some of the problems I ran into. Replacing it with upse-ng would be a better long-term position, but I published these anyway in the hope that it will improve the OpenPSF case while it's still the official implementation. (If I knew about upse-ng I would have never attempted this as this work took a few weeks of trial and error investigation.) On the macOS build failure - I'm not sure, but is that necessarily related to anything I've done? It looks like a download error. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The full explanation for this is in https://github.com/orgs/audacious-media-player/discussions/1867.
But essentially, I found these LLM-assisted changes were necessary to resolve serious issues I found with OpenPSF:
Unsolved:
Only the PS2 portion is affected by this, PS1 portion is untouched.
All of this may become moot if a better implementation is brought in to replace OpenPSF, as discussed in the thread linked at the start.