From 242d8a1dcd3f15d5d7c173f45cdbf820d7b173d6 Mon Sep 17 00:00:00 2001 From: George Kiagiadakis Date: Tue, 24 Dec 2019 11:36:06 +0200 Subject: audiomixer: rework event handling to avoid race conditions Previously there was a race condition that would happen when audiomixer was the first client to connect to pipewire. It would get a session first and then a default endpoint id from the session, but at the time the default endpoint id would be known, the WpProxyEndpoints would still not be ready. This would cause the populate_controls() algorithm to end up with no controls and the binding would therefore report that no controls were exposed. Now we also handle objects-changed from the endpoints object manager. This is an additional trigger, so that when we end up in the situation described above, the objects-changed from the endpoints om will trigger the controls to be exposed eventually. In order to avoid signalling control changes all the time, there are now some checks to change controls only under certain conditions. Change-Id: Ied705592f889a0262465ed5efa711233a66d579b Signed-off-by: George Kiagiadakis --- binding/audiomixer.c | 163 +++++++++++++++++++++++++++++++++++---------------- 1 file changed, 114 insertions(+), 49 deletions(-) diff --git a/binding/audiomixer.c b/binding/audiomixer.c index 6ef3210..aceaefe 100644 --- a/binding/audiomixer.c +++ b/binding/audiomixer.c @@ -19,11 +19,12 @@ struct audiomixer GCond cond; WpRemoteState state; - WpObjectManager *endpoints_om; GPtrArray *mixer_controls; + GWeakRef session; - /* just pointer, no ref */ - WpSession *session; + /* ref held in the thread function */ + WpObjectManager *sessions_om; + WpObjectManager *endpoints_om; const struct audiomixer_events *events; void *events_data; @@ -91,41 +92,95 @@ control_changed (WpEndpoint * ep, guint32 control_id, struct audiomixer * self) /* called with self->lock locked */ static void -populate_controls (struct audiomixer * self, WpSession * session) +check_and_populate_controls (struct audiomixer * self) { - guint32 def_id = wp_session_get_default_endpoint (session, - WP_DEFAULT_ENDPOINT_TYPE_AUDIO_SINK); - g_autoptr (GPtrArray) arr = wp_object_manager_get_objects ( - self->endpoints_om, 0); - - for (guint i = 0; i < arr->len; i++) { - WpEndpoint *ep = g_ptr_array_index (arr, i); - guint32 id = wp_proxy_get_global_id (WP_PROXY (ep)); + g_autoptr (WpSession) session = NULL; + g_autoptr (WpEndpoint) def_ep = NULL; + guint32 def_id = 0; + struct mixer_control_impl *master_ctl = NULL; + + /* find the session */ + session = g_weak_ref_get (&self->session); + + /* find the default audio sink endpoint */ + if (session) { + g_autoptr (GPtrArray) arr = + wp_object_manager_get_objects (self->endpoints_om, 0); + def_id = wp_session_get_default_endpoint (session, + WP_DEFAULT_ENDPOINT_TYPE_AUDIO_SINK); + + for (guint i = 0; i < arr->len; i++) { + WpEndpoint *ep = g_ptr_array_index (arr, i); + guint32 id = wp_proxy_get_global_id (WP_PROXY (ep)); + if (id != def_id) + continue; + def_ep = g_object_ref (ep); + } + } + + /* find the audio sink endpoint that was the default before */ + for (guint i = 0; i < self->mixer_controls->len; i++) { + struct mixer_control_impl *ctl = (struct mixer_control_impl *) + g_ptr_array_index (self->mixer_controls, i); + if (ctl->is_master) { + master_ctl = ctl; + break; + } + } + + g_debug ("check_and_populate: session:%p, def_ep:%p, def_id:%u, " + "master_ctl:%p, master_id:%u", session, def_ep, def_id, + master_ctl, master_ctl ? master_ctl->id : 0); + + /* case 1: there is a default endpoint but it doesn't match + what we currently expose -> clear and expose the new one */ + if (def_ep && (!master_ctl || master_ctl->id != def_id)) { struct mixer_control_impl *mixctl = NULL; gfloat volume; gboolean mute; - if (id != def_id) - continue; + /* clear previous */ + if (master_ctl) + g_signal_handlers_disconnect_by_data (master_ctl->endpoint, self); + g_ptr_array_set_size (self->mixer_controls, 0); - wp_endpoint_get_control_float (ep, WP_ENDPOINT_CONTROL_VOLUME, &volume); - wp_endpoint_get_control_boolean (ep, WP_ENDPOINT_CONTROL_MUTE, &mute); + /* get current master values */ + wp_endpoint_get_control_float (def_ep, WP_ENDPOINT_CONTROL_VOLUME, &volume); + wp_endpoint_get_control_boolean (def_ep, WP_ENDPOINT_CONTROL_MUTE, &mute); + /* create the Master control */ mixctl = g_new0 (struct mixer_control_impl, 1); strncpy (mixctl->pub.name, "Master", sizeof (mixctl->pub.name)); mixctl->pub.volume = volume; mixctl->pub.mute = mute; mixctl->is_master = TRUE; - mixctl->id = id; - mixctl->endpoint = g_object_ref (ep); + mixctl->id = def_id; + mixctl->endpoint = g_object_ref (def_ep); g_ptr_array_add (self->mixer_controls, mixctl); - g_signal_connect (ep, "control-changed", (GCallback) control_changed, + /* track changes */ + g_signal_connect (def_ep, "control-changed", (GCallback) control_changed, self); /* wake up audiomixer_ensure_controls() */ g_cond_broadcast (&self->cond); - break; + + g_debug ("controls changed"); + + /* notify subscribers */ + if (self->events && self->events->controls_changed) + self->events->controls_changed (self->events_data); + } + /* case 2: there is no default endpoint but something is exposed -> clear */ + else if (!def_ep && master_ctl) { + g_signal_handlers_disconnect_by_data (master_ctl->endpoint, self); + g_ptr_array_set_size (self->mixer_controls, 0); + + g_debug ("controls cleared"); + + /* notify subscribers */ + if (self->events && self->events->controls_changed) + self->events->controls_changed (self->events_data); } } @@ -133,44 +188,49 @@ static void default_endpoint_changed (WpSession * session, WpDefaultEndpointType type, guint32 new_id, struct audiomixer * self) { - if (session != self->session || type != WP_DEFAULT_ENDPOINT_TYPE_AUDIO_SINK) + if (type != WP_DEFAULT_ENDPOINT_TYPE_AUDIO_SINK) return; g_autoptr (GMutexLocker) locker = g_mutex_locker_new (&self->lock); - - g_ptr_array_set_size (self->mixer_controls, 0); - populate_controls (self, session); - - if (self->events && self->events->controls_changed) - self->events->controls_changed (self->events_data); + check_and_populate_controls (self); } static void -sessions_changed (WpObjectManager * om, struct audiomixer * self) +sessions_changed (WpObjectManager * sessions_om, struct audiomixer * self) { g_autoptr (GMutexLocker) locker = g_mutex_locker_new (&self->lock); + g_autoptr (WpSession) old_session = NULL; g_autoptr (WpSession) session = NULL; g_autoptr (GPtrArray) arr = NULL; - g_ptr_array_set_size (self->mixer_controls, 0); + old_session = g_weak_ref_get (&self->session); /* normally there is only 1 session */ - arr = wp_object_manager_get_objects (om, 0); + arr = wp_object_manager_get_objects (sessions_om, 0); if (arr->len > 0) session = WP_SESSION (g_object_ref (g_ptr_array_index (arr, 0))); - if (session) - populate_controls (self, session); + g_debug ("sessions changed, count:%d, session:%p, old_session:%p", + arr->len, session, old_session); - /* just to be safe in case there are more sessions around */ - if (session != self->session) { - g_signal_connect (session, "default-endpoint-changed", - (GCallback) default_endpoint_changed, self); - self->session = session; + if (session != old_session) { + if (old_session) + g_signal_handlers_disconnect_by_data (old_session, self); + if (session) + g_signal_connect (session, "default-endpoint-changed", + (GCallback) default_endpoint_changed, self); + + g_weak_ref_set (&self->session, session); } - if (self->events && self->events->controls_changed) - self->events->controls_changed (self->events_data); + check_and_populate_controls (self); +} + +static void +endpoints_changed (WpObjectManager * endpoints_om, struct audiomixer * self) +{ + g_autoptr (GMutexLocker) locker = g_mutex_locker_new (&self->lock); + check_and_populate_controls (self); } static void @@ -182,10 +242,18 @@ remote_state_changed (WpCore *core, WpRemoteState state, g_cond_broadcast (&self->cond); } +static gboolean +connect_in_idle (struct audiomixer * self) +{ + wp_core_connect (self->core); + return G_SOURCE_REMOVE; +} + static void * audiomixer_thread (struct audiomixer * self) { - g_autoptr (WpObjectManager) sessions_om = wp_object_manager_new (); + g_autoptr (WpObjectManager) sessions_om = + self->sessions_om = wp_object_manager_new (); g_autoptr (WpObjectManager) endpoints_om = self->endpoints_om = wp_object_manager_new (); GVariantBuilder b; @@ -210,11 +278,13 @@ audiomixer_thread (struct audiomixer * self) g_variant_new_string ("Audio/Sink")); g_variant_builder_close (&b); - wp_object_manager_add_proxy_interest (self->endpoints_om, + wp_object_manager_add_proxy_interest (endpoints_om, PW_TYPE_INTERFACE_Endpoint, g_variant_builder_end (&b), WP_PROXY_FEATURE_INFO | WP_PROXY_ENDPOINT_FEATURE_CONTROLS); - wp_core_install_object_manager (self->core, self->endpoints_om); + g_signal_connect (endpoints_om, "objects-changed", + (GCallback) endpoints_changed, self); + wp_core_install_object_manager (self->core, endpoints_om); g_signal_connect (self->core, "remote-state-changed", (GCallback) remote_state_changed, self); @@ -249,6 +319,7 @@ audiomixer_new (void) self->state = WP_REMOTE_STATE_UNCONNECTED; self->mixer_controls = g_ptr_array_new_with_free_func ( (GDestroyNotify) mixer_control_impl_free); + g_weak_ref_init (&self->session, NULL); self->thread = g_thread_new ("audiomixer", (GThreadFunc) audiomixer_thread, self); @@ -262,6 +333,7 @@ audiomixer_free(struct audiomixer *self) g_main_loop_quit (self->loop); g_thread_join (self->thread); + g_weak_ref_clear (&self->session); g_ptr_array_unref (self->mixer_controls); g_object_unref (self->core); g_main_loop_unref (self->loop); @@ -284,13 +356,6 @@ audiomixer_unlock(struct audiomixer *self) g_mutex_unlock (&self->lock); } -static gboolean -connect_in_idle (struct audiomixer * self) -{ - wp_core_connect (self->core); - return G_SOURCE_REMOVE; -} - int audiomixer_ensure_connected(struct audiomixer *self, int timeout_sec) { -- cgit 1.2.3-korg