jamulussoftware / jamulussoftware/jamulus
Harden ASIO re-init: ignore stray bufferSwitch() when not running; narrow ASIOMutex scope
- Lingua principale
- C
- Stelle
- 1.1k
- Fork
- 248
- Merge medio
- 2g 3h
- PR unite (30g)
- 9
Descrizione
## Summary
**Possible hardening: make `CSound::bufferSwitch()` ignore stray callbacks when
not running, and narrow the `ASIOMutex` scope in `CSound::Init()`.** This is a
proposed improvement only — it is **not** the fix for the #3779 hang (that is
PR #3867, a `Qt::QueuedConnection` change). It is a separate, optional
measure for a distinct `ASIOMutex` race that #3867 does not touch.
## Background
- #3779 (Windows hang with ASIO4ALL) turned out to be a `MutexDriverReinit`
deadlock, fixed by #3867. There is also an `ASIOMutex`-related race which
this issue covers.
- A similar narrow-lock approach was tried in #3862 and closed as insufficient
on its own; the current proposal is a *secondary* measure, not a replacement.
## Proposed change (idea — may not be correct)
### 1. `CSound::bufferSwitch()` — early-return when not running
src/sound/asio/sound.cpp:
```diff
void CSound::bufferSwitch ( long index, ASIOBool )
{
+ // if not running, ignore stray callbacks (e.g. from ASIO4ALL's thread
+ // during Init()/teardown) so they cannot block on ASIOMutex
+ if ( !pSound->bRun )
+ {
+ return;
+ }
+
int iCurSample;
```
### 2. `CSound::Init()` — narrow the `ASIOMutex` scope
Keep the lock only around the `vecsMultChanAudioSndCrd.Init()` reallocation and
move the opaque driver calls (`ASIOSetSampleRate`, `ASIODisposeBuffers`,
`ASIOCreateBuffers`, `ASIOGetLatencies`, `ASIOOutputReady`) outside it:
```diff
int CSound::Init ( const int iNewPrefMonoBufferSize )
{
- ASIOMutex.lock(); // get mutex lock
- {
- // get the actual sound card buffer size which is supported
- // by the audio hardware
- iASIOBufferSizeMono = GetActualBufferSize ( iNewPrefMonoBufferSize );
+ // get the actual sound card buffer size which is supported
+ // by the audio hardware
+ iASIOBufferSizeMono = GetActualBufferSize ( iNewPrefMonoBufferSize );
- // init base class
- CSoundBase::Init ( iASIOBufferSizeMono );
+ // init base class
+ CSoundBase::Init ( iASIOBufferSizeMono );
- // set internal buffer size value and calculate stereo buffer size
- iASIOBufferSizeStereo = 2 * iASIOBufferSizeMono;
+ // set internal buffer size value and calculate stereo buffer size
+ iASIOBufferSizeStereo = 2 * iASIOBufferSizeMono;
- // set the sample rate
- ASIOSetSampleRate ( SYSTEM_SAMPLE_RATE_HZ );
+ // set the sample rate
+ ASIOSetSampleRate ( SYSTEM_SAMPLE_RATE_HZ );
- // create memory for intermediate audio buffer
+ // create memory for intermediate audio buffer. This reallocation is the
+ // only part which must not race with a running audio callback, so only it
+ // is guarded by the mutex. The opaque driver calls are deliberately kept
+ // outside the mutex: a driver may block inside them until its audio thread
+ // returns from bufferSwitch(), which would deadlock if the mutex were held
+ // here (see issue #3779)
+ ASIOMutex.lock(); // get mutex lock
+ {
vecsMultChanAudioSndCrd.Init ( iASIOBufferSizeStereo );
+ }
+ ASIOMutex.unlock();
- // create and activate ASIO buffers (buffer size in samples),
- // dispose old buffers (if any)
- ASIODisposeBuffers();
-
- // prepare input channels
- for ( int i = 0; i < lNumInChan; i++ )
- {
- bufferInfos[i].isInput = ASIOTrue;
- bufferInfos[i].channelNum = i;
- bufferInfos[i].buffers[0] = 0;
- bufferInfos[i].buffers[1] = 0;
- }
+ // create and activate ASIO buffers (buffer size in samples),
+ // dispose old buffers (if any)
+ ASIODisposeBuffers();
- // prepare output channels
- for ( int i = 0; i < lNumOutChan; i++ )
- {
- bufferInfos[lNumInChan + i].isInput = ASIOFalse;
- bufferInfos[lNumInChan + i].channelNum = i;
- bufferInfos[lNumInChan + i].buffers[0] = 0;
- bufferInfos[lNumInChan + i].buffers[1] = 0;
- }
+ // prepare input channels
+ for ( int i = 0; i < lNumInChan; i++ )
+ {
+ bufferInfos[i].isInput = ASIOTrue;
+ bufferInfos[i].channelNum = i;
+ bufferInfos[i].buffers[0] = 0;
+ bufferInfos[i].buffers[1] = 0;
+ }
- ASIOCreateBuffers ( bufferInfos, lNumInChan + lNumOutChan, iASIOBufferSizeMono, &asioCallbacks );
+ // prepare output channels
+ for ( int i = 0; i < lNumOutChan; i++ )
+ {
+ bufferInfos[lNumInChan + i].isInput = ASIOFalse;
+ bufferInfos[lNumInChan + i].channelNum = i;
+ bufferInfos[lNumInChan + i].buffers[0] = 0;
+ bufferInfos[lNumInChan + i].buffers[1] = 0;
+ }
- // query the latency of the driver
- long lInputLatency = 0;
- long lOutputLatency = 0;
+ ASIOCreateBuffers ( bufferInfos, lNumInChan + lNumOutChan, iASIOBufferSizeMono, &asioCallbacks );
- if ( ASIOGetLatencies ( &lInputLatency, &lOutputLatency ) != ASE_NotPresent )
- {
- // add the input and output latencies (returned in number of
- // samples) and calculate the time in ms
- fInOutLatencyMs = ( static_cast ( lInputLatency ) + lOutputLatency ) * 1000 / SYSTEM_SAMPLE_RATE_HZ;
- }
- else
- {
- // no latency available
- fInOutLatencyMs = 0.0f;
- }
+ // query the latency of the driver
+ long lInputLatency = 0;
+ long lOutputLatency = 0;
- // check whether the driver requires the ASIOOutputReady() optimization
- // (can be used by the driver to reduce output latency by one block)
- bASIOPostOutput = ( ASIOOutputReady() == ASE_OK );
+ if ( ASIOGetLatencies ( &lInputLatency, &lOutputLatency ) != ASE_NotPresent )
+ {
+ // add the input and output latencies (returned in number of
+ // samples) and calculate the time in ms
+ fInOutLatencyMs = ( static_cast ( lInputLatency ) + lOutputLatency ) * 1000 / SYSTEM_SAMPLE_RATE_HZ;
+ }
+ else
+ {
+ // no latency available
+ fInOutLatencyMs = 0.0f;
}
- ASIOMutex.unlock();
+
+ // check whether the driver requires the ASIOOutputReady() optimization
+ // (can be used by the driver to reduce output latency by one block)
+ bASIOPostOutput = ( ASIOOutputReady() == ASE_OK );
return iASIOBufferSizeMono;
}
```
## Why the narrowing should be safe (and what to double-check)
Everything `bufferSwitch()` reads once the lock is dropped — `iASIOBufferSizeMono`,
`iASIOBufferSizeStereo`, `vecsMultChanAudioSndCrd` — is written *before* the
mutex is taken and is not touched again during the unlocked driver calls. So
shrinking the lock should not expose torn state: a stray callback sees either
the old vector (fully written, from the previous Init) or the new one (fully
written, under the lock), never a half-reallocated one. That said, this is an
argument from reading the code, not from instrumentation — I'd want a reviewer
to confirm there's no other writer to those members on the re-init path.
## Caveats (why this may be wrong)
- `bRun` semantics and the thread-safety of reading it from the driver callback
thread need review (it is `std::atomic` on `main`).
- Whether a stray callback can actually fire during the `Init()`/teardown
window is not proven on all drivers; ASIO4ALL keeps its thread alive while
not streaming, which is why it is suspected here.
- The narrowing could in principle introduce a data race on
`vecsMultChanAudioSndCrd` in a path not considered here.
## Requested actions
- Review whether the `!pSound->bRun` early-return and/or the `ASIOMutex`
narrowing are correct and worth adding.
- If agreed, implement as a small change on `main` and test on Windows with
ASIO4ALL.
## ⚠️ AI-generated issue — please verify
- This text is AI generated, may be wrong, and may contain inaccuracies.
Guida per i contributori
Apri la guida per i contributori
Direzione di ricerca
Inizia in src/sound/asio/sound.cpp leggendo CSound::bufferSwitch() e CSound::Init(), quindi segui bRun, ASIOMutex e i punti in cui vengono scritti i membri del buffer audio. Verifica se i callback possono verificarsi durante l’inizializzazione o il teardown senza esporre una data race. Il lavoro è completato quando un’implementazione concordata viene validata su Windows con ASIO4ALL, oppure quando la proposta viene respinta e i relativi problemi di sicurezza vengono documentati.
Scritto dal modello di indicizzazione a partire dal testo della issue.
Valutazione
- Stack tecnologico
- cpp
- Ambito
- audio-video-rtc
- Tipo di issue
- Bug
- Difficoltà
- 4/5
- Tempo stimato
- 3-5 giorni
- Stato di attività
- Tranquilla
- Chiarezza
- Da chiarire
- Idoneità per principianti
- 42/100