Order of setting audio Play/Pause state and device opening

Ken,

I was please to see your recent changes to winecoreaudio.  In my
quest for audio bugs, when I reviewed the code, I found the
following which I believe is wrong or I don't understand.
Maybe you could look at it?

BTW, the code and comments are the same in both wineoss.drv and
winecoreaudio.drv.

In audio.c/wodPause:
     /* The order of the following operations is important since we
can't hold
      * the mutex while we make an Audio Unit call.  Stop the Audio Unit
before
      * setting the PAUSED state.
That's indeed what the code below does.

* Although we can be in PAUSED
      * state with the Audio Unit still running,
Quite to the contrary, PAUSED state is only set after the unit stopped.

* that's harmless because the
      * render callback will just produce silence.
I can't comment on that.

The code is (error checking not shown):
     status = AudioOutputUnitStop(wwo->audioUnit);
     OSSpinLockLock(&wwo->lock);
     if (wwo->state == WINE_WS_PLAYING)
         wwo->state = WINE_WS_PAUSED;
     OSSpinLockUnlock(&wwo->lock);

In audio.c/wodRestart:
      * In wodRestart, the order is reversed.
Indeed.

* This guarantees that we can't get into a situation where the
state is
      * PLAYING but the Audio Unit isn't running.
Quite to the contrary, PLAYING state is reached yet
AudioOutputUnitStart is only called a few instructions afterwards.
Likewise above, it's stopped and PLAYING state is only unset afterwards.

OSSpinLockLock(&wwo->lock);
     if (wwo->state == WINE_WS_PAUSED)
         wwo->state = WINE_WS_PLAYING;
     OSSpinLockUnlock(&wwo->lock);
     status = AudioOutputUnitStart(wwo->audioUnit);

Is my understanding wrong?
Does the code need be changed to reflect the comments?
Do the comments need correction?

Thanks,
    Jorg Hohle

[Ken Thomases]

On Oct 29, 2009, at 9:18 AM,  <[e-mail]
> wrote:
> In audio.c/wodPause:
>     /* The order of the following operations is important since we
> can't hold
>      * the mutex while we make an Audio Unit call.  Stop the Audio  
> Unit
> before
>      * setting the PAUSED state.
> That's indeed what the code below does.
>  * Although we can be in PAUSED
>  * state with the Audio Unit still running,
>
> Quite to the contrary, PAUSED state is only set after the unit  
> stopped.
>  * that's harmless because the
>  * render callback will just produce silence.
>
> I can't comment on that.
>
> The code is (error checking not shown):
>     status = AudioOutputUnitStop(wwo->audioUnit);
>     OSSpinLockLock(&wwo->lock);
>     if (wwo->state == WINE_WS_PLAYING)
>         wwo->state = WINE_WS_PAUSED;
>     OSSpinLockUnlock(&wwo->lock);
>
> In audio.c/wodRestart:
>      * In wodRestart, the order is reversed.
> Indeed.
>  * This guarantees that we can't get into a situation where the
>
> state is
>      * PLAYING but the Audio Unit isn't running.
> Quite to the contrary, PLAYING state is reached yet
> AudioOutputUnitStart is only called a few instructions afterwards.
> Likewise above, it's stopped and PLAYING state is only unset  
> afterwards.
> OSSpinLockLock(&wwo->lock);
> if (wwo->state == WINE_WS_PAUSED)
>     wwo->state = WINE_WS_PLAYING;
> OSSpinLockUnlock(&wwo->lock);
> status = AudioOutputUnitStart(wwo->audioUnit);
>
> Is my understanding wrong?
> Does the code need be changed to reflect the comments?
> Do the comments need correction?

The comments could probably use clarification.  The guarantee is meant  
to hold with respect to two separate threads racing -- one is doing  
wodPause, the other wodRestart.  We want it to be impossible for that  
scenario to leave the audio unit stopped but the state as PLAYING.   
It's acceptable for the audio unit to be left running but the state as  
PAUSED.

The difficulty is that we can't hold the lock on the wave-out instance  
while we call AudioOutputUnitStop/Start.  The Audio Unit calls have  
their own mutex, so those two calls are guaranteed to be atomic with  
respect to each other.  And we use wwo->lock to make sure that the  
state updates are atomic with respect to each other.  But we can't  
make the entirety of wodPause atomic with respect to wodRestart, or  
vice versa, because it would deadlock.  So, the solution is to use  
careful ordering of actions.

The possible sequences are:

1)
Thread A: au stop
Thread A: state -> PAUSED
Thread B: state -> PLAYING
Thread B: au start
Result: PLAYING and au running

2)
Thread A: au stop
Thread B: state -> PLAYING
Thread A: state -> PAUSED
Thread B: au start
Result: PAUSED and au running (acceptable, though not ideal)

3)
Thread A: au stop
Thread B: state -> PLAYING
Thread B: au start
Thread A: state -> PAUSED
Result: PAUSED and au running (ditto)

4)
Thread B: state -> PLAYING
Thread A: au stop
Thread A: state -> PAUSED
Thread B: au start
Result: PAUSED and au running (ditto)

5)
Thread B: state -> PLAYING
Thread A: au stop
Thread B: au start
Thread A: state -> PAUSED
Result: PAUSED and au running (ditto)

6)
Thread B: state -> PLAYING
Thread B: au start
Thread A: au stop
Thread A: state -> PAUSED
Result: PAUSED and au not running

Regards,
Ken