Fix Bluetooth audio artifacts and idle queue noise#107
Conversation
|
I was able to reproduce this consistently on Bluetooth headphones. For the stock build, the noise came back when:
As a control, setting With this patch:
|
Greptile SummaryThis PR addresses real Bluetooth audio issues by: (1) moving the The approach is sound and the new helpers (
Confidence Score: 4/5Safe to merge with minor attention to the data-race in fillBuffer logging The core audio fixes are well-motivated and logically correct. Lock usage for sound library swaps is a clear improvement. The main concern (activeSounds.count read after lock release) is a pre-existing race made more visible by moving the callback off the main thread, and only affects a debug log line — not actual audio output. The redundant timer ordering is harmless. No correctness-breaking regressions were found. Thock/Managers/SoundManager.swift — specifically fillBuffer (line 693) and the resetIdleTimer/restartQueue ordering in play() and playMouseSound() Important Files Changed
|
|
|
||
| resetIdleTimer() | ||
| restartQueue() |
There was a problem hiding this comment.
Redundant
resetIdleTimer() before restartQueue()
When the queue is stopped (e.g. after an idle timeout), the sequence is:
resetIdleTimer()is called here — starts a new countdown timer immediately.restartQueue()starts the queue and — on success — callsresetIdleTimer()again (line 396), cancelling and recreating that timer.
This always produces two timer allocations and one cancellation when the queue was stopped. The original code only called resetIdleTimer() when the queue was already running. Swapping the order avoids starting an idle countdown before the queue is actually running:
| resetIdleTimer() | |
| restartQueue() | |
| restartQueue() | |
| resetIdleTimer() |
| activeSoundsLock.unlock() | ||
|
|
||
| resetIdleTimer() | ||
| restartQueue() |
There was a problem hiding this comment.
Same redundant
resetIdleTimer() ordering in playMouseSound
Identical issue as in play() (lines 810-811). resetIdleTimer() starts the idle countdown before the queue has been (re)started; restartQueue() already calls resetIdleTimer() on success, so the first call is redundant. Swapping the order is the minimal fix.
| private func configurePlaybackProfile(for deviceID: AudioDeviceID) -> Bool { | ||
| let isBluetoothOutput = isBluetoothDevice(deviceID) | ||
| let effectiveBufferSize = isBluetoothOutput | ||
| ? max(currentBufferSize, Self.bluetoothMinimumBufferSize) | ||
| : currentBufferSize |
There was a problem hiding this comment.
currentBufferSize read outside queueStateLock
The lock comment on line 27 declares queueStateLock protects several fields; framesPerBuffer is also written under that lock (line 1162). Here currentBufferSize — which feeds directly into framesPerBuffer — is read without holding the lock. This is safe today because configurePlaybackProfile is only called from within reinitializeAudioQueue after currentBufferSize has already been written and the lock released, but it silently breaks the ownership contract and could be misused if the call-site changes.
Consider either acquiring queueStateLock around the read, or passing the effective buffer size as a parameter.
Summary
Testing