[Bug 28413] Sound play in games and programs causes brief "pauses"

http://bugs.winehq.org/show_bug.cgi?id=28413

--- Comment #23 from Jörg Höhle  2012-02-09 06:31:02 CST ---
Your change to PlaySound_Free is incorrect. There are error paths that go there
with last_playsound == wps.
> EnterCriticalSection(&WINMM_cs);

Every time I see a critical section, my reviews are delayed by at least one
hour because I must visit and think about all users and uses of the CS.  Even
more when it's a big lock.

I recommend you look into Interlocked* instead, where applicable.

if(last_playsound == wps)
        last_playsound = NULL;
InterlockedCompareExchangePointer
> EnterCriticalSection(&WINMM_cs);
>    if (waveOutOpen(&wps->hWave, ...

You should not hold such a big lock for an operation like waveOutOpen that can
take ages loading all msacm codecs.

Instead the pattern is:
. verify preconditions hold
. start an operation locally
. before changing global state, check whether preconditions still hold
. if not, either abort or retry transaction
  -- With playsound you'll abort, not retry:
if (...) /* are we still owners */
    if(waveOutOpen(&hwave, ...
        if (...) /* are we still owners */
            wps->hWave = hwave;
        else /* superseded by another PlaySound */
            waveOutClose(hwave) + goto CleanUp;

Well, that's concurrency theory.  With winmm, you'll prefer to serialize calls
so as to avoid ERROR_DEVICE_IN_USE in the superseding PlaySound.  How to
serialize?  That's tough without the psLastEvent that you removed.

Can we get rid of critical sections entirely?  No, because waveOutReset depends
on mutual exclusion.  The aborting player must be sure that the other player is
still running and not amid waveOutClose.

Your waveOutReset patch part is correct.  My paranoia would make me cache
last_playsound (and use InterlockedExchange):
EnterCS;
if ((running = last_playsound) != NULL) {
    running->bPlaySoundStop = TRUE;
    if (running->hwave)
        waveOutReset(running->hwave);
} or
if ((running = last_playsound) != NULL && !running->bPlaySoundStop) {
    running->bPlaySoundStop = TRUE;
    if (running->hwave)
        waveOutReset(running->hwave);
}
last_playsound = wps;
LeaveCS;

What must the other side do?
CleanUp:
hwave = wps->hwave;
EnterCS;
wps->hwave = 0; /* tell others to keep off */
if (last_playsound == wps) {
    last_playsound = NULL;
}
LeaveCS;
if (hwave)
    waveOutClose(hwave);

It appears that the CS we need is a pure local one.  Hijacking WINMM_cs may
have unforeseen effects.

Do we need a CS when starting up? I don't think so, but you'll have to use
InterLockedExchange() above and be careful about the condition after
waveOutOpen.  A short CS after waveOutOpen may be easier to reason about until
you get acquainted to Interlocked*.  Please give them a try.  I find the
Interlocked* functions easier to reason about than critical sections, esp. when
it comes to liveness properties in concurency.