jamulussoftware / jamulussoftware/jamulus
Harden ASIO re-init: ignore stray bufferSwitch() when not running; narrow ASIOMutex scope
- Vorherrschende Sprache
- C
- Sterne
- 1.1k
- Forks
- 248
- Ø Merge
- 2 T. 3 Std.
- Gemergte PRs (30 T.)
- 9
Beschreibung
## 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.
Beitragsleitfaden
Rechercherichtung
Beginnen Sie in src/sound/asio/sound.cpp, indem Sie CSound::bufferSwitch() und CSound::Init() lesen, und verfolgen Sie anschließend bRun, ASIOMutex und die Stellen, an denen in die Audio-Puffer-Member geschrieben wird. Prüfen Sie, ob Callbacks während der Initialisierung oder des Teardowns auftreten können, ohne eine Data Race zu verursachen. Als erledigt gilt die Aufgabe, wenn eine vereinbarte Implementierung unter Windows mit ASIO4ALL validiert wurde oder der Vorschlag abgelehnt und die entsprechenden Sicherheitsbedenken dokumentiert wurden.
Vom Indexierungsmodell aus dem Issue-Text verfasst.
Bewertung
- Tech-Stack
- cpp
- Bereich
- audio-video-rtc
- Issue-Typ
- Bug
- Schwierigkeit
- 4/5
- Geschätzter Aufwand
- 3-5 Tage
- Aktivitätsstatus
- Ruhig
- Klarheit
- Muss geklärt werden
- Anfängerfreundlichkeit
- 42/100