[pulseaudio-discuss] [PATCH 3/5] alsa-sink: Add locking to protect concurrent mixer and mixer_path access

[Jyri Sarha]

From: Jyri Sarha <jyri.sarha at nokia.com>

---
 src/modules/alsa/alsa-sink.c |   55 ++++++++++++++++++++++++++++++++++++++---
 1 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
index 45ba24a..43b0b09 100644
--- a/src/modules/alsa/alsa-sink.c
+++ b/src/modules/alsa/alsa-sink.c
@@ -98,6 +98,12 @@ struct userdata {
     snd_mixer_t *mixer_handle;
     pa_alsa_path_set *mixer_path_set;
     pa_alsa_path *mixer_path;
+    /* NOTE: This lock is a crude way of protecting both mixer_path and mixer_element
+     *       when accessing them from main and IO-thread concurrently. Everything
+     *       seem to work fine even without this lock when stress testing the sync
+     *       volume with multiple concurrent transient streams, but better safe than
+     *       sorry. */
+    pa_mutex *mixer_mutex;
 
     pa_cvolume hardware_volume;
 
@@ -1133,12 +1139,16 @@ static void sink_get_volume_cb(pa_sink *s) {
     struct userdata *u = s->userdata;
     pa_cvolume r;
     char t[PA_CVOLUME_SNPRINT_MAX];
+    int status;
 
     pa_assert(u);
     pa_assert(u->mixer_path);
     pa_assert(u->mixer_handle);
 
-    if (pa_alsa_path_get_volume(u->mixer_path, u->mixer_handle, &s->channel_map, &r) < 0)
+    pa_mutex_lock(u->mixer_mutex);
+    status = pa_alsa_path_get_volume(u->mixer_path, u->mixer_handle, &s->channel_map, &r);
+    pa_mutex_unlock(u->mixer_mutex);
+    if (status < 0)
         return;
 
     /* Shift down by the base volume, so that 0dB becomes maximum volume */
@@ -1161,6 +1171,7 @@ static void sink_set_volume_cb(pa_sink *s) {
     pa_cvolume r;
     char t[PA_CVOLUME_SNPRINT_MAX];
     pa_bool_t write_to_hw = (s->flags & PA_SINK_SYNC_VOLUME) ? FALSE : TRUE;
+    int status;
 
     pa_assert(u);
     pa_assert(u->mixer_path);
@@ -1169,7 +1180,10 @@ static void sink_set_volume_cb(pa_sink *s) {
     /* Shift up by the base volume */
     pa_sw_cvolume_divide_scalar(&r, &s->real_volume, s->base_volume);
 
-    if (pa_alsa_path_set_volume(u->mixer_path, u->mixer_handle, &s->channel_map, &r, write_to_hw) < 0)
+    pa_mutex_lock(u->mixer_mutex);
+    status = pa_alsa_path_set_volume(u->mixer_path, u->mixer_handle, &s->channel_map, &r, write_to_hw);
+    pa_mutex_unlock(u->mixer_mutex);
+    if (status < 0)
         return;
 
     /* Shift down by the base volume, so that 0dB becomes maximum volume */
@@ -1212,13 +1226,18 @@ static void sink_set_volume_cb(pa_sink *s) {
 static void sink_write_volume_cb(pa_sink *s) {
     struct userdata *u = s->userdata;
     pa_cvolume hw_vol = s->thread_info.current_hw_volume;
+    int status;
 
     pa_assert(u);
     pa_assert(u->mixer_path);
     pa_assert(u->mixer_handle);
     pa_assert(s->flags & PA_SINK_SYNC_VOLUME);
 
-    if (pa_alsa_path_set_volume(u->mixer_path, u->mixer_handle, &s->channel_map, &hw_vol, TRUE) < 0)
+    pa_mutex_lock(u->mixer_mutex);
+    status = pa_alsa_path_set_volume(u->mixer_path, u->mixer_handle, &s->channel_map, &hw_vol, TRUE);
+    pa_mutex_unlock(u->mixer_mutex);
+
+    if (status < 0)
         pa_log_error("Writting HW volume failed");
     else {
         pa_cvolume tmp_vol;
@@ -1239,12 +1258,16 @@ static void sink_write_volume_cb(pa_sink *s) {
 static void sink_get_mute_cb(pa_sink *s) {
     struct userdata *u = s->userdata;
     pa_bool_t b;
+    int status;
 
     pa_assert(u);
     pa_assert(u->mixer_path);
     pa_assert(u->mixer_handle);
 
-    if (pa_alsa_path_get_mute(u->mixer_path, u->mixer_handle, &b) < 0)
+    pa_mutex_lock(u->mixer_mutex);
+    status = pa_alsa_path_get_mute(u->mixer_path, u->mixer_handle, &b);
+    pa_mutex_unlock(u->mixer_mutex);
+    if (status < 0)
         return;
 
     s->muted = b;
@@ -1257,7 +1280,9 @@ static void sink_set_mute_cb(pa_sink *s) {
     pa_assert(u->mixer_path);
     pa_assert(u->mixer_handle);
 
+    pa_mutex_lock(u->mixer_mutex);
     pa_alsa_path_set_mute(u->mixer_path, u->mixer_handle, s->muted);
+    pa_mutex_unlock(u->mixer_mutex);
 }
 
 static int sink_set_port_cb(pa_sink *s, pa_device_port *p) {
@@ -1271,6 +1296,13 @@ static int sink_set_port_cb(pa_sink *s, pa_device_port *p) {
     data = PA_DEVICE_PORT_DATA(p);
 
     pa_assert_se(u->mixer_path = data->path);
+
+    /* This forces flushing of pending volume change events */
+    if (u->sink->flags & PA_SINK_SYNC_VOLUME)
+        pa_assert_se(pa_asyncmsgq_send(s->asyncmsgq, PA_MSGOBJECT(s), PA_SINK_MESSAGE_GET_VOLUME, NULL, 0, NULL) == 0);
+
+    pa_mutex_lock(u->mixer_mutex);
+
     pa_alsa_path_select(u->mixer_path, u->mixer_handle);
 
     if (u->mixer_path->has_volume && u->mixer_path->has_dB) {
@@ -1289,10 +1321,16 @@ static int sink_set_port_cb(pa_sink *s, pa_device_port *p) {
     if (data->setting)
         pa_alsa_setting_select(data->setting, u->mixer_handle);
 
+    pa_mutex_unlock(u->mixer_mutex);
+
     if (s->set_mute)
         s->set_mute(s);
     if (s->set_volume)
         s->set_volume(s);
+    /* NOTE: This is a bit unorthodox to call this from main thread,
+     *       but everything is mutex protected anyway. */
+    if (s->write_volume)
+        s->write_volume(s);
 
     return 0;
 }
@@ -1796,6 +1834,11 @@ pa_sink *pa_alsa_sink_new(pa_module *m, pa_modargs *ma, const char*driver, pa_ca
             TRUE);
     u->smoother_interval = SMOOTHER_MIN_INTERVAL;
 
+    if (!(u->mixer_mutex = pa_mutex_new(TRUE, FALSE))) {
+        pa_log_error("Could not allocate pa_mutex");
+        goto fail;
+    }
+
     dev_id = pa_modargs_get_value(
             ma, "device_id",
             pa_modargs_get_value(ma, "device", DEFAULT_DEVICE));
@@ -2064,6 +2107,10 @@ static void userdata_free(struct userdata *u) {
         snd_pcm_close(u->pcm_handle);
     }
 
+    if (u->mixer_mutex) {
+        pa_mutex_free(u->mixer_mutex);
+    }
+
     if (u->mixer_fdl)
         pa_alsa_fdlist_free(u->mixer_fdl);
 
-- 
1.7.0