Fix Pulseaudio buffering#220
Conversation
Long Newton sounds were choppy because the PulseAudio backend wrote directly from the Newton sound callback and could not reliably handle cases where the stream could not accept a full Newton buffer immediately. Replace the direct-write path with a local ring buffer drained by PulseAudio's write callback. This keeps queued sound data intact across partial writable-space windows. Throttle output interrupt requests so audio refill does not starve emulator input, and unregister PulseAudio callbacks during teardown to avoid callback use after shutdown.
Initialize PulseAudio pointer members before setup so constructor error paths can safely check them. Unref PulseAudio stream and context during normal teardown after disconnecting them. Use std::min instead of the non-portable MIN macro when sizing queued PulseAudio writes.
📝 WalkthroughWalkthrough
ChangesPulseAudio queued output
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Emulator
participant TPulseAudioSoundManager
participant TCircleBuffer
participant PulseAudio
Emulator->>TPulseAudioSoundManager: ScheduleOutput audio
TPulseAudioSoundManager->>TCircleBuffer: buffer audio bytes
PulseAudio->>TPulseAudioSoundManager: write callback with requested bytes
TPulseAudioSoundManager->>TCircleBuffer: peek and discard queued bytes
TPulseAudioSoundManager->>PulseAudio: pa_stream_write
PulseAudio->>TPulseAudioSoundManager: underflow notification
TPulseAudioSoundManager->>Emulator: RequestOutputInterrupt
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| mDataMutex->Lock(); | ||
| mOutputIsRunning = false; | ||
| mOutputRequestPending = false; | ||
| mDataMutex->Unlock(); |
There was a problem hiding this comment.
StopOutput() now takes mDataMutex while it already holds the PulseAudio mainloop lock, but producer paths take the same locks in the opposite order. A zero-size output buffer can enter this path while another output call is waiting on the mainloop lock, which can deadlock the stop/drain sequence.
| // disconnect the stream | ||
| pa_stream_disconnect(mOutputStream); | ||
| pa_context_disconnect(mPAContext); | ||
| // pa_threaded_mainloop_stop | ||
| pa_stream_unref(mOutputStream); | ||
| if (mPAContext) | ||
| { | ||
| pa_context_disconnect(mPAContext); | ||
| pa_context_unref(mPAContext); |
There was a problem hiding this comment.
The new context unref is nested under if (mOutputStream). If construction creates mPAContext but fails before a stream is assigned, destruction skips pa_context_disconnect() and pa_context_unref(), leaving the PulseAudio context alive after the manager is torn down.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Emulator/Sound/TPulseAudioSoundManager.cpp (1)
375-390: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStopOutput leaves queued samples behind.
pa_stream_drain()only waits for PulseAudio’s server-side buffer;mOutputBufferis never cleared, so leftover audio can play after stop or on the next start. Clear or discard the local queue before draining, or switch to flush/cork semantics if this is meant to abort playback.🤖 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 `@Emulator/Sound/TPulseAudioSoundManager.cpp` around lines 375 - 390, Update StopOutput around mOutputBuffer and the pa_stream_drain call so stopping playback also discards any locally queued samples; clear the queue while holding mDataMutex before initiating the PulseAudio operation, or use flush/cork semantics if StopOutput is intended to abort playback rather than drain it.
🤖 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 `@Emulator/Sound/TPulseAudioSoundManager.cpp`:
- Around line 518-543: Make the output-buffer write in the sound callback
transactional: update the logic around mOutputBuffer->Consume() and
pa_stream_write() so queued bytes are not permanently removed before a
successful PulseAudio write. Check pa_stream_write()’s return value, restore or
otherwise retain consumed data when it fails, and route both
pa_stream_begin_write() and pa_stream_write() failures through the existing
recovery/request-output handling.
---
Outside diff comments:
In `@Emulator/Sound/TPulseAudioSoundManager.cpp`:
- Around line 375-390: Update StopOutput around mOutputBuffer and the
pa_stream_drain call so stopping playback also discards any locally queued
samples; clear the queue while holding mDataMutex before initiating the
PulseAudio operation, or use flush/cork semantics if StopOutput is intended to
abort playback rather than drain it.
🪄 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: 44f6c605-40b8-4c1c-bdd9-efd85c496360
📒 Files selected for processing (2)
Emulator/Sound/TPulseAudioSoundManager.cppEmulator/Sound/TPulseAudioSoundManager.h
Follow-up to the PulseAudio buffering fix for choppy long sounds. The write callback copied data out of the Newton-side queue with Consume() before checking whether PulseAudio accepted the write. If pa_stream_write() failed, those queued bytes were lost. Add Peek() and Discard() to TCircleBuffer and use them so the PulseAudio callback only advances the output queue after a successful pa_stream_write(). Request more output on begin/write failure through the existing recovery path.
| if (mPAContext) | ||
| { | ||
| pa_context_disconnect(mPAContext); | ||
| pa_context_unref(mPAContext); | ||
| } |
There was a problem hiding this comment.
pa_context_new() succeeds but stream creation fails before mOutputStream is assigned, the destructor skips this block entirely. That leaves the PulseAudio context connected and referenced after the manager is destroyed. Move the context disconnect and unref out of the stream-only branch so partial construction is cleaned up correctly.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
K/Misc/TCircleBuffer.cpp (1)
238-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRefactor
Consumeto usePeek+Discardto eliminate duplication and fix a pre-existing bug.
PeekandDiscardare correct and well-implemented. However,Peekis nearly a line-for-line copy ofConsume's copy logic.Consumecould be refactored asPeek+Discard, which would:
- Eliminate the duplication between
PeekandConsume.- Fix a pre-existing bug in
Consume(lines 215 and 230 useinAmountinstead of the clampedamount), whichPeekcorrectly avoids.♻️ Proposed refactor of Consume
KUIntPtr TCircleBuffer::Consume(void* outBuffer, KUIntPtr inAmount) { - KUIntPtr amount = inAmount; - - // Copy data. - if (mConsumerCrsr <= mProducerCrsr) - { - // ---C123P--- - KUIntPtr max = mProducerCrsr - mConsumerCrsr; - if (amount > max) - { - amount = max; - } - - (void) ::memcpy( - outBuffer, - (const void*) (mBuffer + mConsumerCrsr), - amount); - - mConsumerCrsr += amount; - } else - { - // 456P---C123 - KUIntPtr max = mBufferSize + mProducerCrsr - mConsumerCrsr; - if (amount > max) - { - amount = max; - } - - if (amount + mConsumerCrsr > mBufferSize) - { - KUIntPtr toEnd = mBufferSize - mConsumerCrsr; - KUIntPtr fromBeginning = inAmount - toEnd; - (void) ::memcpy( - outBuffer, - (const void*) (mBuffer + mConsumerCrsr), - toEnd); - (void) ::memcpy( - ((KUInt8*) outBuffer) + toEnd, - (const void*) mBuffer, - fromBeginning); - mConsumerCrsr = fromBeginning; - } else - { - (void) ::memcpy( - outBuffer, - (const void*) (mBuffer + mConsumerCrsr), - inAmount); - mConsumerCrsr += inAmount; - } - } - - return amount; + KUIntPtr copied = Peek(outBuffer, inAmount); + Discard(copied); + return copied; }🤖 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 `@K/Misc/TCircleBuffer.cpp` around lines 238 - 315, Refactor TCircleBuffer::Consume to delegate copying and cursor advancement to the existing TCircleBuffer::Peek and TCircleBuffer::Discard methods. Replace its duplicated copy logic with a Peek(outBuffer, inAmount) call, pass the returned clamped byte count to Discard, and return that count; do not use the original inAmount when advancing or copying.
🤖 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.
Nitpick comments:
In `@K/Misc/TCircleBuffer.cpp`:
- Around line 238-315: Refactor TCircleBuffer::Consume to delegate copying and
cursor advancement to the existing TCircleBuffer::Peek and
TCircleBuffer::Discard methods. Replace its duplicated copy logic with a
Peek(outBuffer, inAmount) call, pass the returned clamped byte count to Discard,
and return that count; do not use the original inAmount when advancing or
copying.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e19efa58-76d0-4b94-90e4-26e4574ee4c7
📒 Files selected for processing (3)
Emulator/Sound/TPulseAudioSoundManager.cppK/Misc/TCircleBuffer.cppK/Misc/TCircleBuffer.h
🚧 Files skipped from review as they are similar to previous changes (1)
- Emulator/Sound/TPulseAudioSoundManager.cpp
|
Thanks for setting up this PR. I'll try to get my Linux environment running so I can check the PR and eventually merge it. |
2 participants
Long sounds were choppy. This fixes it.
See commit messages for details.
I accidentally pushed these changes to another PR that I had open, but they are separate. I did take into account the AI code review from that initial PR though, and addressed them.
Summary by CodeRabbit
Greptile Summary
This PR changes PulseAudio output to buffer audio locally before writing it to the stream. The main changes are:
PeekandDiscardhelpers onTCircleBuffer.Confidence Score: 4/5
This is close, but one cleanup path should be fixed before merging.
Emulator/Sound/TPulseAudioSoundManager.cpp
Important Files Changed
Reviews (2): Last reviewed commit: "Make PulseAudio queued writes transactio..." | Re-trigger Greptile