[pulseaudio-discuss] [PATCH 0/7] Misc cleanups

[João Paulo Rechi Vita]

I've been refactoring and expanding the audio-routing patches we carry
downstream at Endless and ended-up with a few cleanup patches in the
process. Basically they make the code a bit easier to read and
understand (hopefully not only IMO) by either breaking up big
conditionals into smaller ones or adding debug logs to help follow
things in runtime.

I'm also re-sending an older patch that adds logging to the initial card
profile selection that has been lingering on the mailing list since it
is falls in the same category as the rest of this series.

I'll soon send another series with some of our downstream audio routing
patches as a RFC, since I believe some of them would be beneficial
upstream.

João Paulo Rechi Vita (7):
card: Log availability status when choosing initial profile
switch-on-port-available: Improve readability
switch-on-connect: Improve readability
switch-on-connect: Clean-up tabs used for identation
switch-on-connect: Add debug logs
module-rescue-streams: Fix tab used for identation
card: Fix typo in comments

src/modules/module-rescue-streams.c | 2 +-
src/modules/module-switch-on-connect.c | 54 ++++++++++++-------
src/modules/module-switch-on-port-available.c | 17 ++++--
src/pulsecore/card.c | 5 ++
src/pulsecore/card.h | 2 +-
5 files changed, 54 insertions(+), 26 deletions(-)

[João Paulo Rechi Vita]

---
src/pulsecore/card.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
index bc5b75b04..c785f00a7 100644
--- a/src/pulsecore/card.c
+++ b/src/pulsecore/card.c
@@ -186,7 +186,10 @@ void pa_card_choose_initial_profile(pa_card *card) {
* or if all profiles are unavailable, pick the profile with the highest
* priority regardless of its availability. */

+ pa_log_debug("Looking for initial profile for card %s", card->name);
PA_HASHMAP_FOREACH(profile, card->profiles, state) {
+ pa_log_debug("profile %s availability %s", profile->name, profile->available == PA_AVAILABLE_YES ? "yes" :
+ profile->available == PA_AVAILABLE_NO ? "no" : "unknown");
if (profile->available == PA_AVAILABLE_NO)
continue;

@@ -195,6 +198,7 @@ void pa_card_choose_initial_profile(pa_card *card) {
}

if (!best) {
+ pa_log_info("No profile with availability status 'yes' or 'unknown' found for card %s", card->name);
PA_HASHMAP_FOREACH(profile, card->profiles, state) {
if (!best || profile->priority > best->priority)
best = profile;
@@ -202,6 +206,7 @@ void pa_card_choose_initial_profile(pa_card *card) {
}
pa_assert(best);

+ pa_log_debug("Initial profile %s chosen for card %s", best->name, card->name);
card->active_profile = best;
card->save_profile = false;

[João Paulo Rechi Vita]

Make code easier to read by moving pa_proplist_gets out of the if
condition and using pa_safe_streq.
---
src/modules/module-switch-on-connect.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/src/modules/module-switch-on-connect.c b/src/modules/module-switch-on-connect.c
index 34fffafec..7b82ff1a6 100644
--- a/src/modules/module-switch-on-connect.c
+++ b/src/modules/module-switch-on-connect.c
@@ -69,12 +69,9 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void*
return PA_HOOK_OK;

/* Don't switch to any internal devices */
- if ((s = pa_proplist_gets(sink->proplist, PA_PROP_DEVICE_BUS))) {
- if (pa_streq(s, "pci"))
- return PA_HOOK_OK;
- else if (pa_streq(s, "isa"))
- return PA_HOOK_OK;
- }
+ s = pa_proplist_gets(sink->proplist, PA_PROP_DEVICE_BUS);
+ if (pa_safe_streq(s, "pci") || pa_safe_streq(s, "isa"))
+ return PA_HOOK_OK;

/* Ignore virtual sinks if not configured otherwise on the command line */
if (u->ignore_virtual && !(sink->flags & PA_SINK_HARDWARE))
@@ -139,12 +136,9 @@ static pa_hook_result_t source_put_hook_callback(pa_core *c, pa_source *source,
return PA_HOOK_OK;

/* Don't switch to any internal devices */
- if ((s = pa_proplist_gets(source->proplist, PA_PROP_DEVICE_BUS))) {
- if (pa_streq(s, "pci"))
- return PA_HOOK_OK;
- else if (pa_streq(s, "isa"))
- return PA_HOOK_OK;
- }
+ s = pa_proplist_gets(source->proplist, PA_PROP_DEVICE_BUS);
+ if (pa_safe_streq(s, "pci") || pa_safe_streq(s, "isa"))
+ return PA_HOOK_OK;

/* Ignore virtual sources if not configured otherwise on the command line */
if (u->ignore_virtual && !(source->flags & PA_SOURCE_HARDWARE))

[João Paulo Rechi Vita]

Split a big conditional into separate checks and use pa_safe_streq
instead of checking if a pointer is valid and calling pa_streq inside a
conditional.
---
src/modules/module-switch-on-port-available.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/modules/module-switch-on-port-available.c b/src/modules/module-switch-on-port-available.c
index 321db361f..385c2f693 100644
--- a/src/modules/module-switch-on-port-available.c
+++ b/src/modules/module-switch-on-port-available.c
@@ -156,7 +156,7 @@ static int try_to_switch_profile(pa_device_port *port) {
continue;

/* Give a high bonus in case this is the preferred profile */
- if (port->preferred_profile && pa_streq(name ? name : profile->name, port->preferred_profile))
+ if (pa_safe_streq(name ? name : profile->name, port->preferred_profile))
prio += 1000000;

if (best_profile && best_prio >= prio)
@@ -261,10 +261,19 @@ static void switch_from_port(pa_device_port *port) {
return; /* Already deselected */

/* Try to find a good enough port to switch to */
- PA_HASHMAP_FOREACH(p, port->card->ports, state)
- if (p->direction == port->direction && p != port && p->available != PA_AVAILABLE_NO &&
- (!best_port || best_port->priority < p->priority))
+ PA_HASHMAP_FOREACH(p, port->card->ports, state) {
+ if (p == port)
+ continue;
+
+ if (p->available == PA_AVAILABLE_NO)
+ continue;
+
+ if (p->direction != port->direction)
+ continue;
+
+ if (!best_port || best_port->priority < p->priority)
best_port = p;
+ }

pa_log_debug("Trying to switch away from port %s, found %s", port->name, best_port ? best_port->name : "no better option");

[João Paulo Rechi Vita]

Log when module-switch-on-connect tries to change the default sink or
source.
---
src/modules/module-switch-on-connect.c | 36 ++++++++++++++++++++------
1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/src/modules/module-switch-on-connect.c b/src/modules/module-switch-on-connect.c
index f277c7945..ebdd6aad0 100644
--- a/src/modules/module-switch-on-connect.c
+++ b/src/modules/module-switch-on-connect.c
@@ -68,14 +68,20 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void*
if (c->state != PA_CORE_RUNNING)
return PA_HOOK_OK;

+ pa_log_debug("Trying to switch to new sink %s", sink->name);
+
/* Don't switch to any internal devices */
s = pa_proplist_gets(sink->proplist, PA_PROP_DEVICE_BUS);
- if (pa_safe_streq(s, "pci") || pa_safe_streq(s, "isa"))
+ if (pa_safe_streq(s, "pci") || pa_safe_streq(s, "isa")) {
+ pa_log_debug("Refusing to switch to sink on %s bus", s);
return PA_HOOK_OK;
+ }

/* Ignore virtual sinks if not configured otherwise on the command line */
- if (u->ignore_virtual && !(sink->flags & PA_SINK_HARDWARE))
+ if (u->ignore_virtual && !(sink->flags & PA_SINK_HARDWARE)) {
+ pa_log_debug("Refusing to switch to virtual sink");
return PA_HOOK_OK;
+ }

/* No default sink, nothing to move away, just set the new default */
if (!c->default_sink) {
@@ -83,12 +89,16 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void*
return PA_HOOK_OK;
}

- if (c->default_sink == sink)
+ if (c->default_sink == sink) {
+ pa_log_debug("%s already is the default sink", sink->name);
return PA_HOOK_OK;
+ }

if (u->only_from_unavailable)
- if (!c->default_sink->active_port || c->default_sink->active_port->available != PA_AVAILABLE_NO)
+ if (!c->default_sink->active_port || c->default_sink->active_port->available != PA_AVAILABLE_NO) {
+ pa_log_debug("Current default sink is available and module argument only_from_unavailable was set");
return PA_HOOK_OK;
+ }

old_default_sink = c->default_sink;

@@ -135,14 +145,20 @@ static pa_hook_result_t source_put_hook_callback(pa_core *c, pa_source *source,
if (source->monitor_of)
return PA_HOOK_OK;

+ pa_log_debug("Trying to switch to new source %s", source->name);
+
/* Don't switch to any internal devices */
s = pa_proplist_gets(source->proplist, PA_PROP_DEVICE_BUS);
- if (pa_safe_streq(s, "pci") || pa_safe_streq(s, "isa"))
+ if (pa_safe_streq(s, "pci") || pa_safe_streq(s, "isa")) {
+ pa_log_debug("Refusing to switch to source on %s bus", s);
return PA_HOOK_OK;
+ }

/* Ignore virtual sources if not configured otherwise on the command line */
- if (u->ignore_virtual && !(source->flags & PA_SOURCE_HARDWARE))
+ if (u->ignore_virtual && !(source->flags & PA_SOURCE_HARDWARE)) {
+ pa_log_debug("Refusing to switch to virtual source");
return PA_HOOK_OK;
+ }

/* No default source, nothing to move away, just set the new default */
if (!c->default_source) {
@@ -150,12 +166,16 @@ static pa_hook_result_t source_put_hook_callback(pa_core *c, pa_source *source,
return PA_HOOK_OK;
}

- if (c->default_source == source)
+ if (c->default_source == source) {
+ pa_log_debug("%s already is the default source", source->name);
return PA_HOOK_OK;
+ }

if (u->only_from_unavailable)
- if (!c->default_source->active_port || c->default_source->active_port->available != PA_AVAILABLE_NO)
+ if (!c->default_source->active_port || c->default_source->active_port->available != PA_AVAILABLE_NO) {
+ pa_log_debug("Current default source is available and module argument only_from_unavailable was set");
return PA_HOOK_OK;
+ }

old_default_source = c->default_source;

[João Paulo Rechi Vita]

---
src/modules/module-switch-on-connect.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/modules/module-switch-on-connect.c b/src/modules/module-switch-on-connect.c
index 7b82ff1a6..f277c7945 100644
--- a/src/modules/module-switch-on-connect.c
+++ b/src/modules/module-switch-on-connect.c
@@ -201,14 +201,14 @@ int pa__init(pa_module*m) {
pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SOURCE_PUT], PA_HOOK_LATE+20, (pa_hook_cb_t) source_put_hook_callback, u);

if (pa_modargs_get_value_boolean(ma, "only_from_unavailable", &u->only_from_unavailable) < 0) {
- pa_log("Failed to get a boolean value for only_from_unavailable.");
- goto fail;
+ pa_log("Failed to get a boolean value for only_from_unavailable.");
+ goto fail;
}

u->ignore_virtual = true;
if (pa_modargs_get_value_boolean(ma, "ignore_virtual", &u->ignore_virtual) < 0) {
- pa_log("Failed to get a boolean value for ignore_virtual.");
- goto fail;
+ pa_log("Failed to get a boolean value for ignore_virtual.");
+ goto fail;
}

pa_modargs_free(ma);

[João Paulo Rechi Vita]

---
src/modules/module-rescue-streams.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/modules/module-rescue-streams.c b/src/modules/module-rescue-streams.c
index 52675ecf7..0d8b1be8b 100644
--- a/src/modules/module-rescue-streams.c
+++ b/src/modules/module-rescue-streams.c
@@ -136,7 +136,7 @@ static pa_sink* find_evacuation_sink(pa_core *c, pa_sink_input *i, pa_sink *skip
target = fb_sink;

if (!target)
- pa_log_debug("No evacuation sink found.");
+ pa_log_debug("No evacuation sink found.");

return target;
}

[João Paulo Rechi Vita]

---
src/pulsecore/card.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/pulsecore/card.h b/src/pulsecore/card.h
index 5699475d8..09284f6ff 100644
--- a/src/pulsecore/card.h
+++ b/src/pulsecore/card.h
@@ -128,7 +128,7 @@ pa_card *pa_card_new(pa_core *c, pa_card_new_data *data);

/* Select the initial card profile according to the configured policies. This
* must be called between pa_card_new() and pa_card_put(), after the port and
- * profile availablities have been initialized. */
+ * profile availabilities have been initialized. */
void pa_card_choose_initial_profile(pa_card *card);

void pa_card_put(pa_card *c);

[Tanu Kaskinen]

Post by João Paulo Rechi Vita
I've been refactoring and expanding the audio-routing patches we carry
downstream at Endless and ended-up with a few cleanup patches in the
process. Basically they make the code a bit easier to read and
understand (hopefully not only IMO) by either breaking up big
conditionals into smaller ones or adding debug logs to help follow
things in runtime.
I'm also re-sending an older patch that adds logging to the initial card
profile selection that has been lingering on the mailing list since it
is falls in the same category as the rest of this series.
I'll soon send another series with some of our downstream audio routing
patches as a RFC, since I believe some of them would be beneficial
upstream.
card: Log availability status when choosing initial profile
switch-on-port-available: Improve readability
switch-on-connect: Improve readability
switch-on-connect: Clean-up tabs used for identation
switch-on-connect: Add debug logs
module-rescue-streams: Fix tab used for identation
card: Fix typo in comments
src/modules/module-rescue-streams.c | 2 +-
src/modules/module-switch-on-connect.c | 54 ++++++++++++-------
src/modules/module-switch-on-port-available.c | 17 ++++--
src/pulsecore/card.c | 5 ++
src/pulsecore/card.h | 2 +-
5 files changed, 54 insertions(+), 26 deletions(-)

Thanks for the patches! I applied all except patch 1, which has an
updated version in GitLab.

[João Paulo Rechi Vita]

Post by Tanu Kaskinen

Post by João Paulo Rechi Vita
I've been refactoring and expanding the audio-routing patches we carry
downstream at Endless and ended-up with a few cleanup patches in the
process. Basically they make the code a bit easier to read and
understand (hopefully not only IMO) by either breaking up big
conditionals into smaller ones or adding debug logs to help follow
things in runtime.
I'm also re-sending an older patch that adds logging to the initial card
profile selection that has been lingering on the mailing list since it
is falls in the same category as the rest of this series.
I'll soon send another series with some of our downstream audio routing
patches as a RFC, since I believe some of them would be beneficial
upstream.
card: Log availability status when choosing initial profile
switch-on-port-available: Improve readability
switch-on-connect: Improve readability
switch-on-connect: Clean-up tabs used for identation
switch-on-connect: Add debug logs
module-rescue-streams: Fix tab used for identation
card: Fix typo in comments
src/modules/module-rescue-streams.c | 2 +-
src/modules/module-switch-on-connect.c | 54 ++++++++++++-------
src/modules/module-switch-on-port-available.c | 17 ++++--
src/pulsecore/card.c | 5 ++
src/pulsecore/card.h | 2 +-
5 files changed, 54 insertions(+), 26 deletions(-)

Thanks for the patches! I applied all except patch 1, which has an
updated version in GitLab.

Thanks! I have now addressed your points on the MR.

--
João Paulo Rechi Vita
http://about.me/jprvita