summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorwm4 <wm4@nowhere>2014-11-09 15:22:00 +0100
committerwm4 <wm4@nowhere>2014-11-09 15:23:40 +0100
commite4403523131a69a92a8418bb3714090a408680c7 (patch)
treef57c7161e06ecdff1f16e788242c42506eaae79e
parent0025f0042f68b664ab9e62ebd0279e0485f3eaa8 (diff)
downloadmpv-e4403523131a69a92a8418bb3714090a408680c7.tar.bz2
mpv-e4403523131a69a92a8418bb3714090a408680c7.tar.xz
audio/out/pull: avoid deadlock if audio callback stops
If the audio callback suddenly stops, and the AO provides no "reset" callback, then reset() could deadlock by waiting on the audio callback forever. The waiting was needed to enter a consistent state, where the audio callback guarantees it won't access the ringbuffer. This in turn is needed because mp_ring_reset() is not concurrency-safe. This active waiting is unavoidable. But the way it was implemented, the audio callback had to call ao_read_data() at least once when reset() is called. Fix this by making ao_read_data() set a flag upon entering and leaving, which basically turns p->state into some sort of spinlock. The audio callback actually never needs to spin, because there are only 2 states: playing audio, or playing silence. This might be a bit surprising, because usually atomic_compare_exchange_strong() requires a retry-loop idiom for correct operation. This commit is needed because ao_wasapi can (or will in the future) randomly stop the audio callback in certain corner cases. Then the player would hang forever in reset().
-rw-r--r--audio/out/pull.c66
1 files changed, 40 insertions, 26 deletions
diff --git a/audio/out/pull.c b/audio/out/pull.c
index f559e7d7ea..951034dd78 100644
--- a/audio/out/pull.c
+++ b/audio/out/pull.c
@@ -43,8 +43,11 @@ enum {
// finished, but device is open)
AO_STATE_WAIT, // wait for callback to go into AO_STATE_NONE state
AO_STATE_PLAY, // play the buffer
+ AO_STATE_BUSY, // like AO_STATE_PLAY, but ao_read_data() is being called
};
+#define IS_PLAYING(st) ((st) == AO_STATE_PLAY || (st) == AO_STATE_BUSY)
+
struct ao_pull_state {
// Be very careful with the order when accessing planes.
struct mp_ring *buffers[MP_NUM_CHANNELS];
@@ -56,6 +59,21 @@ struct ao_pull_state {
atomic_llong end_time_us;
};
+static void set_state(struct ao *ao, int new_state)
+{
+ struct ao_pull_state *p = ao->api_priv;
+ while (1) {
+ int old = atomic_load(&p->state);
+ if (old == AO_STATE_BUSY) {
+ // A spinlock, because some audio APIs don't want us to use mutexes.
+ mp_sleep_us(1);
+ continue;
+ }
+ if (atomic_compare_exchange_strong(&p->state, &old, new_state))
+ break;
+ }
+}
+
static int get_space(struct ao *ao)
{
struct ao_pull_state *p = ao->api_priv;
@@ -79,8 +97,10 @@ static int play(struct ao *ao, void **data, int samples, int flags)
int r = mp_ring_write(p->buffers[n], data[n], write_bytes);
assert(r == write_bytes);
}
- if (atomic_load(&p->state) != AO_STATE_PLAY) {
- atomic_store(&p->state, AO_STATE_PLAY);
+
+ int state = atomic_load(&p->state);
+ if (!IS_PLAYING(state)) {
+ set_state(ao, AO_STATE_PLAY);
ao->driver->resume(ao);
}
@@ -101,14 +121,13 @@ int ao_read_data(struct ao *ao, void **data, int samples, int64_t out_time_us)
struct ao_pull_state *p = ao->api_priv;
int full_bytes = samples * ao->sstride;
- int state = atomic_load(&p->state);
+ bool need_wakeup = false;
int bytes = 0;
- if (state != AO_STATE_PLAY) {
- if (state == AO_STATE_WAIT)
- atomic_store(&p->state, AO_STATE_NONE);
+ // Play silence in states other than AO_STATE_PLAY.
+ if (!atomic_compare_exchange_strong(&p->state, &(int){AO_STATE_PLAY},
+ AO_STATE_BUSY))
goto end;
- }
// Since the writer will write the first plane last, its buffered amount
// of data is the minimum amount across all planes.
@@ -124,10 +143,16 @@ int ao_read_data(struct ao *ao, void **data, int samples, int64_t out_time_us)
}
// Half of the buffer played -> request more.
- if (buffered_bytes - bytes <= mp_ring_size(p->buffers[0]) / 2)
- mp_input_wakeup_nolock(ao->input_ctx);
+ need_wakeup = buffered_bytes - bytes <= mp_ring_size(p->buffers[0]) / 2;
+
+ // Should never fail.
+ atomic_compare_exchange_strong(&p->state, &(int){AO_STATE_BUSY}, AO_STATE_PLAY);
end:
+
+ if (need_wakeup)
+ mp_input_wakeup_nolock(ao->input_ctx);
+
// pad with silence (underflow/paused/eof)
for (int n = 0; n < ao->num_planes; n++)
af_fill_silence(data[n], full_bytes - bytes, ao->format);
@@ -160,19 +185,9 @@ static double get_delay(struct ao *ao)
static void reset(struct ao *ao)
{
struct ao_pull_state *p = ao->api_priv;
- if (ao->driver->reset) {
+ if (ao->driver->reset)
ao->driver->reset(ao); // assumes the audio callback thread is stopped
- atomic_store(&p->state, AO_STATE_NONE);
- } else {
- // The thread keeps running. Wait until the audio callback gets into
- // a defined state where it won't touch the ringbuffer. We must do
- // this, because emptying the ringbuffer is not an atomic operation.
- if (atomic_load(&p->state) != AO_STATE_NONE) {
- atomic_store(&p->state, AO_STATE_WAIT);
- while (atomic_load(&p->state) != AO_STATE_NONE)
- mp_sleep_us(1);
- }
- }
+ set_state(ao, AO_STATE_NONE);
for (int n = 0; n < ao->num_planes; n++)
mp_ring_reset(p->buffers[n]);
atomic_store(&p->end_time_us, 0);
@@ -180,23 +195,22 @@ static void reset(struct ao *ao)
static void pause(struct ao *ao)
{
- struct ao_pull_state *p = ao->api_priv;
if (ao->driver->reset)
ao->driver->reset(ao);
- atomic_store(&p->state, AO_STATE_NONE);
+ set_state(ao, AO_STATE_NONE);
}
static void resume(struct ao *ao)
{
- struct ao_pull_state *p = ao->api_priv;
- atomic_store(&p->state, AO_STATE_PLAY);
+ set_state(ao, AO_STATE_PLAY);
ao->driver->resume(ao);
}
static void drain(struct ao *ao)
{
struct ao_pull_state *p = ao->api_priv;
- if (atomic_load(&p->state) == AO_STATE_PLAY)
+ int state = atomic_load(&p->state);
+ if (IS_PLAYING(state))
mp_sleep_us(get_delay(ao) * 1000000);
reset(ao);
}