From 8e5448fd0cb597b6bc2e5994c2d4f5bb7246203b Mon Sep 17 00:00:00 2001 From: George Kiagiadakis Date: Fri, 18 Jun 2021 12:21:29 +0300 Subject: [PATCH] si-audio-adapter/endpoint: do not sync() in loops, use ports-changed instead Calling sync() in loops may end up looping forever if the operating system's scheduling allows pipewire to reply to this sync() before wireplumber's event loop has a chance to become idle. In order for the new ports to appear, the object manager that monitors them needs to emit "objects-changed", which only ever happens in an idle callback. If the reply to sync() arrives before the idle callback, it gets prioritized and processed, which causes another sync(), and so on... Since setting PortFormat in the adapter always changes the ports, watching for "ports-changed" feels like a better solution. Still, there is more room for improvement. --- modules/module-si-audio-adapter.c | 63 ++++++++++++------------------ modules/module-si-audio-endpoint.c | 62 ++++++++++++----------------- 2 files changed, 48 insertions(+), 77 deletions(-) diff --git a/modules/module-si-audio-adapter.c b/modules/module-si-audio-adapter.c index 305898c6..3a1cae6b 100644 --- a/modules/module-si-audio-adapter.c +++ b/modules/module-si-audio-adapter.c @@ -261,12 +261,21 @@ on_node_enum_format_done (WpPipewireObject * proxy, GAsyncResult * res, "dsp", on_format_set, transition); } +static void +on_node_ports_changed (WpObject * node, WpSiAudioAdapter *self) +{ + /* finish the task started by _set_ports_format() */ + if (self->format_task && wp_node_get_n_ports (self->node) > 0) { + g_autoptr (GTask) t = g_steal_pointer (&self->format_task); + g_task_return_boolean (t, TRUE); + } +} + static void on_feature_ports_ready (WpObject * node, GAsyncResult * res, WpTransition * transition) { WpSiAudioAdapter *self = wp_transition_get_source_object (transition); - g_autoptr (WpCore) core = wp_object_get_core (WP_OBJECT (self)); g_autoptr (GError) error = NULL; if (!wp_object_activate_finish (node, res, &error)) { @@ -274,6 +283,9 @@ on_feature_ports_ready (WpObject * node, GAsyncResult * res, return; } + g_signal_connect_object (node, "ports-changed", + (GCallback) on_node_ports_changed, self, 0); + /* If device node, enum available formats and set one of them */ if (self->is_device || self->dont_remix || !self->is_autoconnect) wp_pipewire_object_enum_params (WP_PIPEWIRE_OBJECT (self->node), @@ -342,42 +354,6 @@ si_audio_adapter_get_ports_format (WpSiAdapter * item, const gchar **mode) return self->format ? wp_spa_pod_ref (self->format) : NULL; } -static void -on_sync_done (WpCore * core, GAsyncResult * res, WpSiAudioAdapter *self) -{ - g_autoptr (GError) error = NULL; - guint32 active = 0; - - if (!wp_core_sync_finish (core, res, &error)) { - g_autoptr (GTask) t = g_steal_pointer (&self->format_task); - g_task_return_error (t, g_steal_pointer (&error)); - return; - } - - active = wp_object_get_active_features (WP_OBJECT (self->node)); - if (!(active & WP_NODE_FEATURE_PORTS)) { - g_autoptr (GTask) t = g_steal_pointer (&self->format_task); - g_task_return_new_error (t, WP_DOMAIN_LIBRARY, - WP_LIBRARY_ERROR_OPERATION_FAILED, - "node feature ports is not enabled, aborting set format operation"); - return; - } - - /* The task might be destroyed by set_ports_format before sync is finished. - * The set_ports_format API returns a task error if there is a pending task - * so we don't need to do anything here */ - if (!self->format_task) - return; - - /* make sure ports are available */ - if (wp_node_get_n_ports (self->node) > 0) { - g_autoptr (GTask) t = g_steal_pointer (&self->format_task); - g_task_return_boolean (t, TRUE); - } else { - wp_core_sync (core, NULL, (GAsyncReadyCallback) on_sync_done, self); - } -} - static gboolean parse_adapter_format (WpSpaPod *format, gint *channels, WpSpaPod **position) @@ -461,6 +437,7 @@ si_audio_adapter_set_ports_format (WpSiAdapter * item, WpSpaPod *f, g_autoptr (WpCore) core = wp_object_get_core (WP_OBJECT (self)); g_autoptr (WpSpaPod) format = f; g_autoptr (WpSpaPod) new_format = NULL; + guint32 active = 0; g_return_if_fail (core); @@ -484,6 +461,15 @@ si_audio_adapter_set_ports_format (WpSiAdapter * item, WpSpaPod *f, return; } + active = wp_object_get_active_features (WP_OBJECT (self->node)); + if (G_UNLIKELY (!(active & WP_NODE_FEATURE_PORTS))) { + g_autoptr (GTask) t = g_steal_pointer (&self->format_task); + g_task_return_new_error (t, WP_DOMAIN_LIBRARY, + WP_LIBRARY_ERROR_OPERATION_FAILED, + "node feature ports is not enabled, aborting set format operation"); + return; + } + /* set format and mode */ g_clear_pointer (&self->format, wp_spa_pod_unref); self->format = g_steal_pointer (&new_format); @@ -500,8 +486,7 @@ si_audio_adapter_set_ports_format (WpSiAdapter * item, WpSpaPod *f, "format", "P", self->format, NULL)); - /* sync until new ports are available */ - wp_core_sync (core, NULL, (GAsyncReadyCallback) on_sync_done, self); + /* the task finishes with new ports being emitted -> on_node_ports_changed */ } static gboolean diff --git a/modules/module-si-audio-endpoint.c b/modules/module-si-audio-endpoint.c index 9f74ce4c..f17c0490 100644 --- a/modules/module-si-audio-endpoint.c +++ b/modules/module-si-audio-endpoint.c @@ -158,6 +158,16 @@ si_audio_endpoint_disable_exported (WpSessionItem *si) WP_SESSION_ITEM_FEATURE_EXPORTED); } +static void +on_node_ports_changed (WpObject * node, WpSiAudioEndpoint *self) +{ + /* finish the task started by _set_ports_format() */ + if (self->format_task && wp_node_get_n_ports (self->node) > 0) { + g_autoptr (GTask) t = g_steal_pointer (&self->format_task); + g_task_return_boolean (t, TRUE); + } +} + static void on_node_activate_done (WpObject * node, GAsyncResult * res, WpTransition * transition) @@ -170,6 +180,9 @@ on_node_activate_done (WpObject * node, GAsyncResult * res, return; } + g_signal_connect_object (node, "ports-changed", + (GCallback) on_node_ports_changed, self, 0); + wp_object_update_features (WP_OBJECT (self), WP_SESSION_ITEM_FEATURE_ACTIVE, 0); } @@ -372,42 +385,6 @@ si_audio_endpoint_get_ports_format (WpSiAdapter * item, const gchar **mode) return self->format ? wp_spa_pod_ref (self->format) : NULL; } -static void -on_sync_done (WpCore * core, GAsyncResult * res, WpSiAudioEndpoint *self) -{ - g_autoptr (GError) error = NULL; - guint32 active = 0; - - if (!wp_core_sync_finish (core, res, &error)) { - g_autoptr (GTask) t = g_steal_pointer (&self->format_task); - g_task_return_error (t, g_steal_pointer (&error)); - return; - } - - active = wp_object_get_active_features (WP_OBJECT (self->node)); - if (!(active & WP_NODE_FEATURE_PORTS)) { - g_autoptr (GTask) t = g_steal_pointer (&self->format_task); - g_task_return_new_error (t, WP_DOMAIN_LIBRARY, - WP_LIBRARY_ERROR_OPERATION_FAILED, - "node feature ports is not enabled, aborting set format operation"); - return; - } - - /* The task might be destroyed by set_ports_format before sync is finished. - * The set_ports_format API returns a task error if there is a pending task - * so we don't need to do anything here */ - if (!self->format_task) - return; - - /* make sure ports are available */ - if (wp_node_get_n_ports (self->node) > 0) { - g_autoptr (GTask) t = g_steal_pointer (&self->format_task); - g_task_return_boolean (t, TRUE); - } else { - wp_core_sync (core, NULL, (GAsyncReadyCallback) on_sync_done, self); - } -} - static gboolean parse_adapter_format (WpSpaPod *format, gint *channels, WpSpaPod **position) @@ -491,6 +468,7 @@ si_audio_endpoint_set_ports_format (WpSiAdapter * item, WpSpaPod *f, g_autoptr (WpCore) core = wp_object_get_core (WP_OBJECT (self)); g_autoptr (WpSpaPod) format = f; g_autoptr (WpSpaPod) new_format = NULL; + guint32 active = 0; g_return_if_fail (core); @@ -514,6 +492,15 @@ si_audio_endpoint_set_ports_format (WpSiAdapter * item, WpSpaPod *f, return; } + active = wp_object_get_active_features (WP_OBJECT (self->node)); + if (G_UNLIKELY (!(active & WP_NODE_FEATURE_PORTS))) { + g_autoptr (GTask) t = g_steal_pointer (&self->format_task); + g_task_return_new_error (t, WP_DOMAIN_LIBRARY, + WP_LIBRARY_ERROR_OPERATION_FAILED, + "node feature ports is not enabled, aborting set format operation"); + return; + } + /* set format and mode */ g_clear_pointer (&self->format, wp_spa_pod_unref); self->format = g_steal_pointer (&new_format); @@ -529,8 +516,7 @@ si_audio_endpoint_set_ports_format (WpSiAdapter * item, WpSpaPod *f, "format", "P", self->format, NULL)); - /* sync until new ports are available */ - wp_core_sync (core, NULL, (GAsyncReadyCallback) on_sync_done, self); + /* the task finishes with new ports being emitted -> on_node_ports_changed */ } static gboolean