[pulseaudio-discuss] [PATCH] Update stream-interaction logic (module-role-{cork, ducking}).

[Juho Hämäläinen]

After reconsidering the stream-interaction patch[1] I opted to refactor/rewrite
the implementation instead. First patch refactors module argument parsing and
uses LLIST for groups. Second patch updates how the interaction is handled.
Now both trigger roles (streams that when active trigger corking or ducking)
and interaction roles (streams that are ducked when trigger roles are active)
are tracked individually which hopefully makes the logic a bit more easy to
follow.

Third patch is of convenience and probably not widely needed but as the change
to functionality is minimal it would be nice to have upstream.

[1] https://lists.freedesktop.org/archives/pulseaudio-discuss/2018-July/030158.html

[Juho Hämäläinen]

Use LLIST instead of array for interaction groups, and refactor parsing.
Side effect from refactoring is that module-role-cork also now supports
interaction groups.

Signed-off-by: Juho Hämäläinen <***@hilvi.org>
---
src/modules/stream-interaction.c | 337 +++++++++++++++++++++------------------
1 file changed, 180 insertions(+), 157 deletions(-)

diff --git a/src/modules/stream-interaction.c b/src/modules/stream-interaction.c
index bf7134a..d538e88 100644
--- a/src/modules/stream-interaction.c
+++ b/src/modules/stream-interaction.c
@@ -31,21 +31,28 @@
#include <pulsecore/core-util.h>
#include <pulsecore/sink-input.h>
#include <pulsecore/modargs.h>
+#include <pulsecore/llist.h>
+#include <pulsecore/idxset.h>

#include "stream-interaction.h"

+#define STREAM_INTERACTION_DEFAULT_DUCK_VOLUME_DB (-20)
+#define STREAM_INTERACTION_ANY_ROLE "any_role"
+#define STREAM_INTERACTION_NO_ROLE "no_role"
+
struct group {
char *name;
pa_idxset *trigger_roles;
pa_idxset *interaction_roles;
pa_hashmap *interaction_state;
pa_volume_t volume;
+
+ PA_LLIST_FIELDS(struct group);
};

struct userdata {
pa_core *core;
- uint32_t n_groups;
- struct group **groups;
+ PA_LLIST_HEAD(struct group, groups);
bool global:1;
bool duck:1;
pa_hook_slot
@@ -63,13 +70,12 @@ static const char *get_trigger_role(struct userdata *u, pa_sink_input *i, struct
uint32_t role_idx;

if (!(role = pa_proplist_gets(i->proplist, PA_PROP_MEDIA_ROLE)))
- role = "no_role";
+ role = STREAM_INTERACTION_NO_ROLE;

if (g == NULL) {
/* get it from all groups */
- uint32_t j;
- for (j = 0; j < u->n_groups; j++) {
- PA_IDXSET_FOREACH(trigger_role, u->groups[j]->trigger_roles, role_idx) {
+ PA_LLIST_FOREACH(g, u->groups) {
+ PA_IDXSET_FOREACH(trigger_role, g->trigger_roles, role_idx) {
if (pa_streq(role, trigger_role))
return trigger_role;
}
@@ -170,12 +176,13 @@ static inline void apply_interaction_to_sink(struct userdata *u, pa_sink *s, con
continue;

if (!(role = pa_proplist_gets(j->proplist, PA_PROP_MEDIA_ROLE)))
- role = "no_role";
+ role = STREAM_INTERACTION_NO_ROLE;

PA_IDXSET_FOREACH(interaction_role, g->interaction_roles, role_idx) {
if ((trigger = pa_streq(role, interaction_role)))
break;
- if ((trigger = (pa_streq(interaction_role, "any_role") && !get_trigger_role(u, j, g))))
+ if ((trigger = (pa_streq(interaction_role, STREAM_INTERACTION_ANY_ROLE) &&
+ !get_trigger_role(u, j, g))))
break;
}
if (!trigger)
@@ -229,7 +236,7 @@ static void remove_interactions(struct userdata *u, struct group *g) {
if(!!pa_hashmap_get(g->interaction_state, j)) {
corked = (j->state == PA_SINK_INPUT_CORKED);
if (!(role = pa_proplist_gets(j->proplist, PA_PROP_MEDIA_ROLE)))
- role = "no_role";
+ role = STREAM_INTERACTION_NO_ROLE;
uncork_or_unduck(u, j, role, corked, g);
}
}
@@ -238,21 +245,21 @@ static void remove_interactions(struct userdata *u, struct group *g) {

static pa_hook_result_t process(struct userdata *u, pa_sink_input *i, bool create, bool new_stream) {
const char *trigger_role;
- uint32_t j;
+ struct group *g;

pa_assert(u);
pa_sink_input_assert_ref(i);

if (!create)
- for (j = 0; j < u->n_groups; j++)
- pa_hashmap_remove(u->groups[j]->interaction_state, i);
+ PA_LLIST_FOREACH(g, u->groups)
+ pa_hashmap_remove(g->interaction_state, i);

if (!i->sink)
return PA_HOOK_OK;

- for (j = 0; j < u->n_groups; j++) {
- trigger_role = find_global_trigger_stream(u, i->sink, create ? NULL : i, u->groups[j]);
- apply_interaction(u, i->sink, trigger_role, create ? NULL : i, new_stream, u->groups[j]);
+ PA_LLIST_FOREACH(g, u->groups) {
+ trigger_role = find_global_trigger_stream(u, i->sink, create ? NULL : i, g);
+ apply_interaction(u, i->sink, trigger_role, create ? NULL : i, new_stream, g);
}

return PA_HOOK_OK;
@@ -315,13 +322,127 @@ static pa_hook_result_t sink_input_proplist_changed_cb(pa_core *core, pa_sink_in
return PA_HOOK_OK;
}

+static struct group *group_new(const char *prefix, uint32_t id) {
+ struct group *g = pa_xnew0(struct group, 1);
+ PA_LLIST_INIT(struct group, g);
+ g->trigger_roles = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func,
+ pa_xfree, NULL);
+ g->interaction_roles = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func,
+ pa_xfree, NULL);
+ g->interaction_state = pa_hashmap_new(pa_idxset_trivial_hash_func, pa_idxset_trivial_compare_func);
+ g->volume = pa_sw_volume_from_dB(STREAM_INTERACTION_DEFAULT_DUCK_VOLUME_DB);
+ g->name = pa_sprintf_malloc("%s_group_%u", prefix, id);
+
+ return g;
+}
+
+static void group_free(struct group *g) {
+ pa_idxset_free(g->trigger_roles, pa_xfree);
+ pa_idxset_free(g->interaction_roles, pa_xfree);
+ pa_hashmap_free(g->interaction_state);
+ pa_xfree(g->name);
+ pa_xfree(g);
+}
+
+typedef bool (*group_value_add_t)(struct group *g, const char *data);
+
+static bool group_value_add_trigger_roles(struct group *g, const char *data) {
+ pa_idxset_put(g->trigger_roles, pa_xstrdup(data), NULL);
+ return true;
+}
+
+static bool group_value_add_interaction_roles(struct group *g, const char *data) {
+ pa_idxset_put(g->interaction_roles, pa_xstrdup(data), NULL);
+ return true;
+}
+
+static bool group_value_add_volume(struct group *g, const char *data) {
+ if (pa_parse_volume(data, &(g->volume)) < 0) {
+ pa_log("Failed to parse volume");
+ return false;
+ }
+ return true;
+}
+
+static bool group_parse_roles(pa_modargs *ma, const char *name, group_value_add_t add_cb, struct group *groups) {
+ const char *roles;
+ char *roles_in_group;
+ struct group *g;
+
+ pa_assert(ma);
+ pa_assert(name);
+ pa_assert(add_cb);
+ pa_assert(groups);
+
+ g = groups;
+ roles = pa_modargs_get_value(ma, name, NULL);
+
+ if (roles) {
+ const char *group_split_state = NULL;
+
+ while ((roles_in_group = pa_split(roles, "/", &group_split_state))) {
+ pa_assert(g);
+
+ if (roles_in_group[0] != '\0') {
+ const char *split_state = NULL;
+ char *n = NULL;
+ while ((n = pa_split(roles_in_group, ",", &split_state))) {
+ bool ret = true;
+
+ if (n[0] != '\0')
+ ret = add_cb(g, n);
+ else {
+ ret = false;
+ pa_log("Empty %s", name);
+ }
+
+ pa_xfree(n);
+ if (!ret)
+ goto fail;
+ }
+ } else {
+ pa_log("Empty %s", name);
+ goto fail;
+ }
+
+ g = g->next;
+ pa_xfree(roles_in_group);
+ }
+ }
+
+ return true;
+
+fail:
+ return false;
+}
+
+static int count_groups(pa_modargs *ma, const char *module_argument) {
+ const char *val;
+ int count = 0;
+
+ pa_assert(ma);
+ pa_assert(module_argument);
+
+ val = pa_modargs_get_value(ma, module_argument, NULL);
+ if (val) {
+ const char *split_state = NULL;
+ int len = 0;
+ /* Count empty ones as well, empty groups will fail later
+ * when parsing the groups. */
+ while (pa_split_in_place(val, "/", &len, &split_state))
+ count++;
+ }
+
+ return count;
+}
+
int pa_stream_interaction_init(pa_module *m, const char* const v_modargs[]) {
pa_modargs *ma = NULL;
struct userdata *u;
- const char *roles;
- char *roles_in_group = NULL;
bool global = false;
uint32_t i = 0;
+ uint32_t group_count_tr = 0;
+ struct group *last = NULL;

pa_assert(m);

@@ -333,155 +454,66 @@ int pa_stream_interaction_init(pa_module *m, const char* const v_modargs[]) {
m->userdata = u = pa_xnew0(struct userdata, 1);

u->core = m->core;
+ u->duck = pa_streq(m->name, "module-role-ducking");
+ PA_LLIST_HEAD_INIT(struct group, u->groups);

- u->duck = false;
- if (pa_streq(m->name, "module-role-ducking"))
- u->duck = true;
-
- u->n_groups = 1;
+ group_count_tr = count_groups(ma, "trigger_roles");

if (u->duck) {
- const char *volumes;
- uint32_t group_count_tr = 0;
uint32_t group_count_du = 0;
uint32_t group_count_vol = 0;
-
- roles = pa_modargs_get_value(ma, "trigger_roles", NULL);
- if (roles) {
- const char *split_state = NULL;
- char *n = NULL;
- while ((n = pa_split(roles, "/", &split_state))) {
- group_count_tr++;
- pa_xfree(n);
- }
- }
- roles = pa_modargs_get_value(ma, "ducking_roles", NULL);
- if (roles) {
- const char *split_state = NULL;
- char *n = NULL;
- while ((n = pa_split(roles, "/", &split_state))) {
- group_count_du++;
- pa_xfree(n);
- }
- }
- volumes = pa_modargs_get_value(ma, "volume", NULL);
- if (volumes) {
- const char *split_state = NULL;
- char *n = NULL;
- while ((n = pa_split(volumes, "/", &split_state))) {
- group_count_vol++;
- pa_xfree(n);
- }
- }
+ group_count_du = count_groups(ma, "ducking_roles");
+ group_count_vol = count_groups(ma, "volume");

if ((group_count_tr > 1 || group_count_du > 1 || group_count_vol > 1) &&
((group_count_tr != group_count_du) || (group_count_tr != group_count_vol))) {
pa_log("Invalid number of groups");
goto fail;
}
+ } else {
+ uint32_t group_count_co;
+ group_count_co = count_groups(ma, "cork_roles");

- if (group_count_tr > 0)
- u->n_groups = group_count_tr;
+ if ((group_count_tr > 1 || group_count_co > 1) &&
+ (group_count_tr != group_count_co)) {
+ pa_log("Invalid number of groups");
+ goto fail;
+ }
}

- u->groups = pa_xnew0(struct group*, u->n_groups);
- for (i = 0; i < u->n_groups; i++) {
- u->groups[i] = pa_xnew0(struct group, 1);
- u->groups[i]->trigger_roles = pa_idxset_new(NULL, NULL);
- u->groups[i]->interaction_roles = pa_idxset_new(NULL, NULL);
- u->groups[i]->interaction_state = pa_hashmap_new(pa_idxset_trivial_hash_func, pa_idxset_trivial_compare_func);
- if (u->duck)
- u->groups[i]->name = pa_sprintf_malloc("ducking_group_%u", i);
- }
+ /* create at least one group */
+ if (group_count_tr == 0)
+ group_count_tr = 1;

- roles = pa_modargs_get_value(ma, "trigger_roles", NULL);
- if (roles) {
- const char *group_split_state = NULL;
- i = 0;
+ for (i = 0; i < group_count_tr; i++) {
+ struct group *g = group_new(u->duck ? "ducking" : "cork", i);
+ PA_LLIST_INSERT_AFTER(struct group, u->groups, last, g);
+ last = g;
+ }

- while ((roles_in_group = pa_split(roles, "/", &group_split_state))) {
- if (roles_in_group[0] != '\0') {
- const char *split_state = NULL;
- char *n = NULL;
- while ((n = pa_split(roles_in_group, ",", &split_state))) {
- if (n[0] != '\0')
- pa_idxset_put(u->groups[i]->trigger_roles, n, NULL);
- else {
- pa_log("empty trigger role");
- pa_xfree(n);
- goto fail;
- }
- }
- i++;
- } else {
- pa_log("empty trigger roles");
- goto fail;
- }
+ if (!group_parse_roles(ma, "trigger_roles", group_value_add_trigger_roles, u->groups))
+ goto fail;

- pa_xfree(roles_in_group);
- }
- }
- if (pa_idxset_isempty(u->groups[0]->trigger_roles)) {
+ if (pa_idxset_isempty(u->groups->trigger_roles)) {
pa_log_debug("Using role 'phone' as trigger role.");
- pa_idxset_put(u->groups[0]->trigger_roles, pa_xstrdup("phone"), NULL);
+ pa_idxset_put(u->groups->trigger_roles, pa_xstrdup("phone"), NULL);
}

- roles = pa_modargs_get_value(ma, u->duck ? "ducking_roles" : "cork_roles", NULL);
- if (roles) {
- const char *group_split_state = NULL;
- i = 0;
-
- while ((roles_in_group = pa_split(roles, "/", &group_split_state))) {
- if (roles_in_group[0] != '\0') {
- const char *split_state = NULL;
- char *n = NULL;
- while ((n = pa_split(roles_in_group, ",", &split_state))) {
- if (n[0] != '\0')
- pa_idxset_put(u->groups[i]->interaction_roles, n, NULL);
- else {
- pa_log("empty ducking role");
- pa_xfree(n);
- goto fail;
- }
- }
- i++;
- } else {
- pa_log("empty ducking roles");
- goto fail;
- }
+ if (!group_parse_roles(ma,
+ u->duck ? "ducking_roles" : "cork_roles",
+ group_value_add_interaction_roles,
+ u->groups))
+ goto fail;

- pa_xfree(roles_in_group);
- }
- }
- if (pa_idxset_isempty(u->groups[0]->interaction_roles)) {
- pa_log_debug("Using roles 'music' and 'video' as %s roles.", u->duck ? "ducking" : "cork");
- pa_idxset_put(u->groups[0]->interaction_roles, pa_xstrdup("music"), NULL);
- pa_idxset_put(u->groups[0]->interaction_roles, pa_xstrdup("video"), NULL);
+ if (pa_idxset_isempty(u->groups->interaction_roles)) {
+ pa_log_debug("Using roles 'music' and 'video' as %s_roles.", u->duck ? "ducking" : "cork");
+ pa_idxset_put(u->groups->interaction_roles, pa_xstrdup("music"), NULL);
+ pa_idxset_put(u->groups->interaction_roles, pa_xstrdup("video"), NULL);
}

- if (u->duck) {
- const char *volumes;
- u->groups[0]->volume = pa_sw_volume_from_dB(-20);
- if ((volumes = pa_modargs_get_value(ma, "volume", NULL))) {
- const char *group_split_state = NULL;
- char *n = NULL;
- i = 0;
- while ((n = pa_split(volumes, "/", &group_split_state))) {
- if (n[0] != '\0') {
- if (pa_parse_volume(n, &(u->groups[i++]->volume)) < 0) {
- pa_log("Failed to parse volume");
- pa_xfree(n);
- goto fail;
- }
- } else {
- pa_log("empty volume");
- pa_xfree(n);
- goto fail;
- }
- pa_xfree(n);
- }
- }
- }
+ if (u->duck)
+ if (!group_parse_roles(ma, "volume", group_value_add_volume, u->groups))
+ goto fail;

if (pa_modargs_get_value_boolean(ma, "global", &global) < 0) {
pa_log("Invalid boolean parameter: global");
@@ -506,8 +538,6 @@ fail:

if (ma)
pa_modargs_free(ma);
- if (roles_in_group)
- pa_xfree(roles_in_group);

return -1;

@@ -521,18 +551,11 @@ void pa_stream_interaction_done(pa_module *m) {
if (!(u = m->userdata))
return;

- if (u->groups) {
- uint32_t j;
- for (j = 0; j < u->n_groups; j++) {
- remove_interactions(u, u->groups[j]);
- pa_idxset_free(u->groups[j]->trigger_roles, pa_xfree);
- pa_idxset_free(u->groups[j]->interaction_roles, pa_xfree);
- pa_hashmap_free(u->groups[j]->interaction_state);
- if (u->duck)
- pa_xfree(u->groups[j]->name);
- pa_xfree(u->groups[j]);
- }
- pa_xfree(u->groups);
+ while (u->groups) {
+ struct group *g = u->groups;
+ PA_LLIST_REMOVE(struct group, u->groups, g);
+ remove_interactions(u, g);
+ group_free(g);
}

if (u->sink_input_put_slot)

[Juho Hämäläinen]

Stream interaction groups now have hashmaps for all trigger and interaction
states and roles. We track both trigger streams and interaction streams
separately, so that applying interaction (ducking or cork and muting) is
relatively simple.

Signed-off-by: Juho Hämäläinen <***@hilvi.org>
---
src/modules/stream-interaction.c | 388 ++++++++++++++++++++-------------------
1 file changed, 200 insertions(+), 188 deletions(-)

diff --git a/src/modules/stream-interaction.c b/src/modules/stream-interaction.c
index d538e88..c30a8c0 100644
--- a/src/modules/stream-interaction.c
+++ b/src/modules/stream-interaction.c
@@ -42,10 +42,13 @@

struct group {
char *name;
- pa_idxset *trigger_roles;
- pa_idxset *interaction_roles;
+ pa_hashmap *trigger_roles;
+ pa_hashmap *interaction_roles;
+ pa_hashmap *trigger_state;
pa_hashmap *interaction_state;
pa_volume_t volume;
+ bool any_role;
+ bool active;

PA_LLIST_FIELDS(struct group);
};
@@ -55,211 +58,228 @@ struct userdata {
PA_LLIST_HEAD(struct group, groups);
bool global:1;
bool duck:1;
- pa_hook_slot
- *sink_input_put_slot,
- *sink_input_unlink_slot,
- *sink_input_move_start_slot,
- *sink_input_move_finish_slot,
- *sink_input_state_changed_slot,
- *sink_input_mute_changed_slot,
- *sink_input_proplist_changed_slot;
};

-static const char *get_trigger_role(struct userdata *u, pa_sink_input *i, struct group *g) {
- const char *role, *trigger_role;
- uint32_t role_idx;
+static const char *sink_input_role(pa_sink_input *sink_input) {
+ const char *role;

- if (!(role = pa_proplist_gets(i->proplist, PA_PROP_MEDIA_ROLE)))
- role = STREAM_INTERACTION_NO_ROLE;
+ pa_assert(sink_input);

- if (g == NULL) {
- /* get it from all groups */
- PA_LLIST_FOREACH(g, u->groups) {
- PA_IDXSET_FOREACH(trigger_role, g->trigger_roles, role_idx) {
- if (pa_streq(role, trigger_role))
- return trigger_role;
- }
- }
- } else {
- PA_IDXSET_FOREACH(trigger_role, g->trigger_roles, role_idx) {
- if (pa_streq(role, trigger_role))
- return trigger_role;
- }
- }
+ if (!(role = pa_proplist_gets(sink_input->proplist, PA_PROP_MEDIA_ROLE)))
+ role = STREAM_INTERACTION_NO_ROLE;

- return NULL;
+ return role;
}

-static const char *find_trigger_stream(struct userdata *u, pa_sink *s, pa_sink_input *ignore, struct group *g) {
- pa_sink_input *j;
- uint32_t idx;
- const char *trigger_role;
+/* Return true if group state changes between
+ * active and inactive. */
+static bool update_group_active(struct userdata *u, struct group *g) {
+ void *state = NULL;
+ bool new_active = false;
+ pa_sink_input *sink_input = NULL;
+ void *value;

pa_assert(u);
- pa_sink_assert_ref(s);
-
- for (j = PA_SINK_INPUT(pa_idxset_first(s->inputs, &idx)); j; j = PA_SINK_INPUT(pa_idxset_next(s->inputs, &idx))) {
+ pa_assert(g);

- if (j == ignore)
- continue;
+ if (pa_hashmap_size(g->trigger_state) > 0) {
+ PA_HASHMAP_FOREACH_KV(sink_input, value, g->trigger_state, state) {
+ if (!sink_input->muted &&
+ sink_input->state != PA_SINK_INPUT_CORKED) {
+ new_active = true;
+ break;
+ }
+ }
+ }

- trigger_role = get_trigger_role(u, j, g);
- if (trigger_role && !j->muted && j->state != PA_SINK_INPUT_CORKED)
- return trigger_role;
+ if (new_active != g->active) {
+ pa_log_debug("Group '%s' is now %sactive.", g->name, new_active ? "" : "in");
+ g->active = new_active;
+ return true;
}

- return NULL;
+ return false;
}

-static const char *find_global_trigger_stream(struct userdata *u, pa_sink *s, pa_sink_input *ignore, struct group *g) {
- const char *trigger_role = NULL;
+/* Identify trigger streams and add or remove the streams from
+ * state hashmap. Proplist change when changing media.role may
+ * result in already existing stream to gain or lose trigger role
+ * status. Returns true if the handled sink-input should be ignored
+ * in interaction applying phase. */
+static bool update_trigger_streams(struct userdata *u, struct group *g, pa_sink_input *sink_input,
+ bool put, bool unlink, bool proplist) {
+ const char *role = NULL;
+ bool proplist_changed = false;
+ bool can_ignore = false;

pa_assert(u);
+ pa_assert(g);
+ pa_assert(sink_input);
+
+ if (proplist) {
+ bool in_trigger_state;
+ bool in_trigger_roles;
+ role = sink_input_role(sink_input);
+
+ in_trigger_state = !!pa_hashmap_get(g->trigger_state, sink_input);
+ in_trigger_roles = !!pa_hashmap_get(g->trigger_roles, role);
+
+ /* If the sink-input is already both pointer in trigger_state hashmap
+ * and role in trigger_roles, or neither, proplist value important to
+ * us (media.role) didn't change, so no need to update anything. */
+ proplist_changed = ((in_trigger_state && in_trigger_roles) ||
+ (!in_trigger_state && !in_trigger_roles));
+ }

- if (u->global) {
- uint32_t idx;
- PA_IDXSET_FOREACH(s, u->core->sinks, idx)
- if ((trigger_role = find_trigger_stream(u, s, ignore, g)))
- break;
- } else
- trigger_role = find_trigger_stream(u, s, ignore, g);
+ if (proplist_changed || unlink) {
+ if (pa_hashmap_remove(g->trigger_state, sink_input)) {
+ can_ignore = true;
+ pa_log_debug("Stream with role '%s' removed from group '%s'", sink_input_role(sink_input), g->name);
+ }
+ }
+
+ if (proplist_changed || put) {
+ if (!role)
+ role = sink_input_role(sink_input);

- return trigger_role;
+ if (pa_hashmap_get(g->trigger_roles, role)) {
+ pa_hashmap_put(g->trigger_state, sink_input, PA_INT_TO_PTR(1));
+ can_ignore = true;
+ pa_log_debug("Stream with role '%s' added to group '%s'", role, g->name);
+ }
+ }
+
+ /* If proplist changed we need to include this stream into consideration
+ * when applying interaction roles, as the sink-input may have gained or
+ * lost trigger or interaction role. */
+ if (proplist_changed)
+ can_ignore = false;
+
+ return can_ignore;
}

-static void cork_or_duck(struct userdata *u, pa_sink_input *i, const char *interaction_role, const char *trigger_role, bool interaction_applied, struct group *g) {
+static void cork_or_duck(struct userdata *u, struct group *g,
+ pa_sink_input *sink_input, const char *interaction_role) {

- if (u->duck && !interaction_applied) {
+ if (u->duck) {
pa_cvolume vol;
- vol.channels = 1;
- vol.values[0] = g->volume;
+ pa_cvolume_set(&vol, 1, g->volume);

- pa_log_debug("Found a '%s' stream of '%s' that ducks a '%s' stream.", trigger_role, g->name, interaction_role);
- pa_sink_input_add_volume_factor(i, g->name, &vol);
+ pa_log_debug("Group '%s' ducks a '%s' stream.", g->name, interaction_role);
+ pa_sink_input_add_volume_factor(sink_input, g->name, &vol);

- } else if (!u->duck) {
- pa_log_debug("Found a '%s' stream that corks/mutes a '%s' stream.", trigger_role, interaction_role);
- pa_sink_input_set_mute(i, true, false);
- pa_sink_input_send_event(i, PA_STREAM_EVENT_REQUEST_CORK, NULL);
+ } else {
+ pa_log_debug("Group '%s' corks/mutes a '%s' stream.", g->name, interaction_role);
+ if (!sink_input->muted)
+ pa_sink_input_set_mute(sink_input, true, false);
+ if (sink_input->state != PA_SINK_INPUT_CORKED)
+ pa_sink_input_send_event(sink_input, PA_STREAM_EVENT_REQUEST_CORK, NULL);
}
}

-static void uncork_or_unduck(struct userdata *u, pa_sink_input *i, const char *interaction_role, bool corked, struct group *g) {
+static void uncork_or_unduck(struct userdata *u, struct group *g,
+ pa_sink_input *sink_input, const char *interaction_role) {

if (u->duck) {
- pa_log_debug("In '%s', found a '%s' stream that should be unducked", g->name, interaction_role);
- pa_sink_input_remove_volume_factor(i, g->name);
- }
- else if (corked || i->muted) {
- pa_log_debug("Found a '%s' stream that should be uncorked/unmuted.", interaction_role);
- if (i->muted)
- pa_sink_input_set_mute(i, false, false);
- if (corked)
- pa_sink_input_send_event(i, PA_STREAM_EVENT_REQUEST_UNCORK, NULL);
+ pa_log_debug("In '%s', found a '%s' stream that should be unducked", g->name, interaction_role);
+ pa_sink_input_remove_volume_factor(sink_input, g->name);
+ } else {
+ pa_log_debug("In '%s' found a '%s' stream that should be uncorked/unmuted.", g->name, interaction_role);
+ if (sink_input->muted)
+ pa_sink_input_set_mute(sink_input, false, false);
+ if (sink_input->state == PA_SINK_INPUT_CORKED)
+ pa_sink_input_send_event(sink_input, PA_STREAM_EVENT_REQUEST_UNCORK, NULL);
}
}

-static inline void apply_interaction_to_sink(struct userdata *u, pa_sink *s, const char *new_trigger, pa_sink_input *ignore, bool new_stream, struct group *g) {
- pa_sink_input *j;
- uint32_t idx, role_idx;
- const char *interaction_role;
+static void apply_interaction_to_sink_input(struct userdata *u, struct group *g,
+ pa_sink_input *sink_input, bool unlink) {
bool trigger = false;
+ bool interaction_applied;
+ const char *role = STREAM_INTERACTION_ANY_ROLE;

pa_assert(u);
- pa_sink_assert_ref(s);
-
- for (j = PA_SINK_INPUT(pa_idxset_first(s->inputs, &idx)); j; j = PA_SINK_INPUT(pa_idxset_next(s->inputs, &idx))) {
- bool corked, interaction_applied;
- const char *role;

- if (j == ignore)
- continue;
-
- if (!(role = pa_proplist_gets(j->proplist, PA_PROP_MEDIA_ROLE)))
- role = STREAM_INTERACTION_NO_ROLE;
-
- PA_IDXSET_FOREACH(interaction_role, g->interaction_roles, role_idx) {
- if ((trigger = pa_streq(role, interaction_role)))
- break;
- if ((trigger = (pa_streq(interaction_role, STREAM_INTERACTION_ANY_ROLE) &&
- !get_trigger_role(u, j, g))))
- break;
- }
- if (!trigger)
- continue;
-
- /* Some applications start their streams corked, so the stream is uncorked by */
- /* the application only after sink_input_put() was called. If a new stream turns */
- /* up, act as if it was not corked. In the case of module-role-cork this will */
- /* only mute the stream because corking is reverted later by the application */
- corked = (j->state == PA_SINK_INPUT_CORKED);
- if (new_stream && corked)
- corked = false;
- interaction_applied = !!pa_hashmap_get(g->interaction_state, j);
+ if (pa_hashmap_get(g->trigger_state, sink_input))
+ return;

- if (new_trigger && ((!corked && !j->muted) || u->duck)) {
- if (!interaction_applied)
- pa_hashmap_put(g->interaction_state, j, PA_INT_TO_PTR(1));
+ if (!g->any_role) {
+ role = sink_input_role(sink_input);
+ trigger = !!pa_hashmap_get(g->interaction_roles, role);
+ }

- cork_or_duck(u, j, role, new_trigger, interaction_applied, g);
+ if (!g->any_role && !trigger)
+ return;

- } else if (!new_trigger && interaction_applied) {
- pa_hashmap_remove(g->interaction_state, j);
+ interaction_applied = !!pa_hashmap_get(g->interaction_state, sink_input);

- uncork_or_unduck(u, j, role, corked, g);
- }
+ if (g->active && !interaction_applied) {
+ pa_hashmap_put(g->interaction_state, sink_input, PA_INT_TO_PTR(1));
+ cork_or_duck(u, g, sink_input, role);
+ } else if ((unlink || !g->active) && interaction_applied) {
+ pa_hashmap_remove(g->interaction_state, sink_input);
+ uncork_or_unduck(u, g, sink_input, role);
}
}

-static void apply_interaction(struct userdata *u, pa_sink *s, const char *trigger_role, pa_sink_input *ignore, bool new_stream, struct group *g) {
+static void update_interactions(struct userdata *u, struct group *g,
+ pa_sink_input *sink_input, pa_sink_input *ignore,
+ bool unlink) {
+ pa_sink_input *si;
+ pa_idxset *idxset;
+ uint32_t idx;
+
pa_assert(u);
+ pa_assert(g);
+ pa_assert(sink_input);
+ pa_assert(sink_input->sink);
+
+ if (u->global)
+ idxset = u->core->sink_inputs;
+ else
+ idxset = sink_input->sink->inputs;

- if (u->global) {
- uint32_t idx;
- PA_IDXSET_FOREACH(s, u->core->sinks, idx)
- apply_interaction_to_sink(u, s, trigger_role, ignore, new_stream, g);
- } else
- apply_interaction_to_sink(u, s, trigger_role, ignore, new_stream, g);
+ PA_IDXSET_FOREACH(si, idxset, idx) {
+ if (si == ignore)
+ continue;
+ apply_interaction_to_sink_input(u, g, si, unlink);
+ }
}

static void remove_interactions(struct userdata *u, struct group *g) {
- uint32_t idx, idx_input;
- pa_sink *s;
- pa_sink_input *j;
- bool corked;
- const char *role;
+ pa_sink_input *sink_input;
+ void *value;
+ void *state = NULL;

- PA_IDXSET_FOREACH(s, u->core->sinks, idx) {
+ pa_assert(u);
+ pa_assert(g);

- for (j = PA_SINK_INPUT(pa_idxset_first(s->inputs, &idx_input)); j; j = PA_SINK_INPUT(pa_idxset_next(s->inputs, &idx_input))) {
+ PA_HASHMAP_FOREACH_KV(sink_input, value, g->interaction_state, state)
+ uncork_or_unduck(u, g, sink_input, sink_input_role(sink_input));

- if(!!pa_hashmap_get(g->interaction_state, j)) {
- corked = (j->state == PA_SINK_INPUT_CORKED);
- if (!(role = pa_proplist_gets(j->proplist, PA_PROP_MEDIA_ROLE)))
- role = STREAM_INTERACTION_NO_ROLE;
- uncork_or_unduck(u, j, role, corked, g);
- }
- }
- }
+ pa_hashmap_remove_all(g->trigger_state);
+ pa_hashmap_remove_all(g->interaction_state);
}

-static pa_hook_result_t process(struct userdata *u, pa_sink_input *i, bool create, bool new_stream) {
- const char *trigger_role;
+static pa_hook_result_t process(struct userdata *u, pa_sink_input *sink_input,
+ bool put, bool unlink, bool proplist) {
struct group *g;
+ bool ignore = false;

pa_assert(u);
- pa_sink_input_assert_ref(i);
-
- if (!create)
- PA_LLIST_FOREACH(g, u->groups)
- pa_hashmap_remove(g->interaction_state, i);
+ pa_sink_input_assert_ref(sink_input);

- if (!i->sink)
+ if (!sink_input->sink)
return PA_HOOK_OK;

PA_LLIST_FOREACH(g, u->groups) {
- trigger_role = find_global_trigger_stream(u, i->sink, create ? NULL : i, g);
- apply_interaction(u, i->sink, trigger_role, create ? NULL : i, new_stream, g);
+ if (put || unlink || proplist)
+ ignore = update_trigger_streams(u, g, sink_input, put, unlink, proplist);
+
+ /* Update interactions when group state changes, or if there is new
+ * stream added or removed while the group is already active. */
+ if (update_group_active(u, g) || ((put || unlink) && g->active))
+ update_interactions(u, g, sink_input, ignore ? sink_input : NULL, unlink);
}

return PA_HOOK_OK;
@@ -269,35 +289,35 @@ static pa_hook_result_t sink_input_put_cb(pa_core *core, pa_sink_input *i, struc
pa_core_assert_ref(core);
pa_sink_input_assert_ref(i);

- return process(u, i, true, true);
+ return process(u, i, true, false, false);
}

static pa_hook_result_t sink_input_unlink_cb(pa_core *core, pa_sink_input *i, struct userdata *u) {
pa_sink_input_assert_ref(i);

- return process(u, i, false, false);
+ return process(u, i, false, true, false);
}

static pa_hook_result_t sink_input_move_start_cb(pa_core *core, pa_sink_input *i, struct userdata *u) {
pa_core_assert_ref(core);
pa_sink_input_assert_ref(i);

- return process(u, i, false, false);
+ return process(u, i, false, true, false);
}

static pa_hook_result_t sink_input_move_finish_cb(pa_core *core, pa_sink_input *i, struct userdata *u) {
pa_core_assert_ref(core);
pa_sink_input_assert_ref(i);

- return process(u, i, true, false);
+ return process(u, i, true, false, false);
}

static pa_hook_result_t sink_input_state_changed_cb(pa_core *core, pa_sink_input *i, struct userdata *u) {
pa_core_assert_ref(core);
pa_sink_input_assert_ref(i);

- if (PA_SINK_INPUT_IS_LINKED(i->state) && get_trigger_role(u, i, NULL))
- return process(u, i, true, false);
+ if (PA_SINK_INPUT_IS_LINKED(i->state))
+ process(u, i, false, false, false);

return PA_HOOK_OK;
}
@@ -306,8 +326,8 @@ static pa_hook_result_t sink_input_mute_changed_cb(pa_core *core, pa_sink_input
pa_core_assert_ref(core);
pa_sink_input_assert_ref(i);

- if (PA_SINK_INPUT_IS_LINKED(i->state) && get_trigger_role(u, i, NULL))
- return process(u, i, true, false);
+ if (PA_SINK_INPUT_IS_LINKED(i->state))
+ process(u, i, false, false, false);

return PA_HOOK_OK;
}
@@ -317,7 +337,7 @@ static pa_hook_result_t sink_input_proplist_changed_cb(pa_core *core, pa_sink_in
pa_sink_input_assert_ref(i);

if (PA_SINK_INPUT_IS_LINKED(i->state))
- return process(u, i, true, false);
+ process(u, i, false, false, true);

return PA_HOOK_OK;
}
@@ -329,6 +349,7 @@ static struct group *group_new(const char *prefix, uint32_t id) {
pa_xfree, NULL);
g->interaction_roles = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func,
pa_xfree, NULL);
+ g->trigger_state = pa_hashmap_new(pa_idxset_trivial_hash_func, pa_idxset_trivial_compare_func);
g->interaction_state = pa_hashmap_new(pa_idxset_trivial_hash_func, pa_idxset_trivial_compare_func);
g->volume = pa_sw_volume_from_dB(STREAM_INTERACTION_DEFAULT_DUCK_VOLUME_DB);
g->name = pa_sprintf_malloc("%s_group_%u", prefix, id);
@@ -337,8 +358,9 @@ static struct group *group_new(const char *prefix, uint32_t id) {
}

static void group_free(struct group *g) {
- pa_idxset_free(g->trigger_roles, pa_xfree);
- pa_idxset_free(g->interaction_roles, pa_xfree);
+ pa_hashmap_free(g->trigger_roles);
+ pa_hashmap_free(g->interaction_roles);
+ pa_hashmap_free(g->trigger_state);
pa_hashmap_free(g->interaction_state);
pa_xfree(g->name);
pa_xfree(g);
@@ -347,12 +369,14 @@ static void group_free(struct group *g) {
typedef bool (*group_value_add_t)(struct group *g, const char *data);

static bool group_value_add_trigger_roles(struct group *g, const char *data) {
- pa_idxset_put(g->trigger_roles, pa_xstrdup(data), NULL);
+ pa_hashmap_put(g->trigger_roles, pa_xstrdup(data), PA_INT_TO_PTR(1));
return true;
}

static bool group_value_add_interaction_roles(struct group *g, const char *data) {
- pa_idxset_put(g->interaction_roles, pa_xstrdup(data), NULL);
+ pa_hashmap_put(g->interaction_roles, pa_xstrdup(data), PA_INT_TO_PTR(1));
+ if (pa_streq(data, STREAM_INTERACTION_ANY_ROLE))
+ g->any_role = true;
return true;
}

@@ -494,9 +518,9 @@ int pa_stream_interaction_init(pa_module *m, const char* const v_modargs[]) {
if (!group_parse_roles(ma, "trigger_roles", group_value_add_trigger_roles, u->groups))
goto fail;

- if (pa_idxset_isempty(u->groups->trigger_roles)) {
+ if (pa_hashmap_isempty(u->groups->trigger_roles)) {
pa_log_debug("Using role 'phone' as trigger role.");
- pa_idxset_put(u->groups->trigger_roles, pa_xstrdup("phone"), NULL);
+ pa_hashmap_put(u->groups->trigger_roles, pa_xstrdup("phone"), PA_INT_TO_PTR(1));
}

if (!group_parse_roles(ma,
@@ -505,10 +529,10 @@ int pa_stream_interaction_init(pa_module *m, const char* const v_modargs[]) {
u->groups))
goto fail;

- if (pa_idxset_isempty(u->groups->interaction_roles)) {
+ if (pa_hashmap_isempty(u->groups->interaction_roles)) {
pa_log_debug("Using roles 'music' and 'video' as %s_roles.", u->duck ? "ducking" : "cork");
- pa_idxset_put(u->groups->interaction_roles, pa_xstrdup("music"), NULL);
- pa_idxset_put(u->groups->interaction_roles, pa_xstrdup("video"), NULL);
+ pa_hashmap_put(u->groups->interaction_roles, pa_xstrdup("music"), PA_INT_TO_PTR(1));
+ pa_hashmap_put(u->groups->interaction_roles, pa_xstrdup("video"), PA_INT_TO_PTR(1));
}

if (u->duck)
@@ -521,13 +545,18 @@ int pa_stream_interaction_init(pa_module *m, const char* const v_modargs[]) {
}
u->global = global;

- u->sink_input_put_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SINK_INPUT_PUT], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_put_cb, u);
- u->sink_input_unlink_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SINK_INPUT_UNLINK], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_unlink_cb, u);
- u->sink_input_move_start_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SINK_INPUT_MOVE_START], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_move_start_cb, u);
- u->sink_input_move_finish_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SINK_INPUT_MOVE_FINISH], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_move_finish_cb, u);
- u->sink_input_state_changed_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SINK_INPUT_STATE_CHANGED], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_state_changed_cb, u);
- u->sink_input_mute_changed_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SINK_INPUT_MUTE_CHANGED], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_mute_changed_cb, u);
- u->sink_input_proplist_changed_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SINK_INPUT_PROPLIST_CHANGED], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_proplist_changed_cb, u);
+ pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_INPUT_PUT], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_put_cb, u);
+ pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_INPUT_UNLINK], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_unlink_cb, u);
+ pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_INPUT_STATE_CHANGED], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_state_changed_cb, u);
+ pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_INPUT_MUTE_CHANGED], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_mute_changed_cb, u);
+ pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_INPUT_PROPLIST_CHANGED], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_proplist_changed_cb, u);
+
+ /* When global interaction is enabled we don't need to know which sink our sink-inputs
+ * are connected to. */
+ if (!u->global) {
+ pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_INPUT_MOVE_START], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_move_start_cb, u);
+ pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_INPUT_MOVE_FINISH], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_move_finish_cb, u);
+ }

pa_modargs_free(ma);

@@ -540,7 +569,6 @@ fail:
pa_modargs_free(ma);

return -1;
-
}

void pa_stream_interaction_done(pa_module *m) {
@@ -558,21 +586,5 @@ void pa_stream_interaction_done(pa_module *m) {
group_free(g);
}

- if (u->sink_input_put_slot)
- pa_hook_slot_free(u->sink_input_put_slot);
- if (u->sink_input_unlink_slot)
- pa_hook_slot_free(u->sink_input_unlink_slot);
- if (u->sink_input_move_start_slot)
- pa_hook_slot_free(u->sink_input_move_start_slot);
- if (u->sink_input_move_finish_slot)
- pa_hook_slot_free(u->sink_input_move_finish_slot);
- if (u->sink_input_state_changed_slot)
- pa_hook_slot_free(u->sink_input_state_changed_slot);
- if (u->sink_input_mute_changed_slot)
- pa_hook_slot_free(u->sink_input_mute_changed_slot);
- if (u->sink_input_proplist_changed_slot)
- pa_hook_slot_free(u->sink_input_proplist_changed_slot);
-
pa_xfree(u);
-
}

[Juho Hämäläinen]

With this enabled when trigger stream is available but
in corked state trigger is already applied to interaction
streams. This is useful when trigger stream pauses for
a while before playing, so that the trigger happens
slightly earlier.

Signed-off-by: Juho Hämäläinen <***@hilvi.org>
---
src/modules/module-role-ducking.c | 2 ++
src/modules/stream-interaction.c | 16 ++++++++++------
2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/src/modules/module-role-ducking.c b/src/modules/module-role-ducking.c
index 1b2ecd7..50c23b7 100644
--- a/src/modules/module-role-ducking.c
+++ b/src/modules/module-role-ducking.c
@@ -34,6 +34,7 @@ PA_MODULE_USAGE(
"ducking_roles=<Comma(and slash) separated list of roles which will be ducked. Slash can divide the roles into groups>"
"global=<Should we operate globally or only inside the same device?>"
"volume=<Volume for the attenuated streams. Default: -20dB. If trigger_roles and ducking_roles are separated by slash, use slash for dividing volume group>"
+ "duck_while_corked=<Duck ducking_roles even if trigger_roles are corked. Default false>"
);

static const char* const valid_modargs[] = {
@@ -41,6 +42,7 @@ static const char* const valid_modargs[] = {
"ducking_roles",
"global",
"volume",
+ "duck_while_corked",
NULL
};

diff --git a/src/modules/stream-interaction.c b/src/modules/stream-interaction.c
index c30a8c0..dc727dc 100644
--- a/src/modules/stream-interaction.c
+++ b/src/modules/stream-interaction.c
@@ -56,8 +56,9 @@ struct group {
struct userdata {
pa_core *core;
PA_LLIST_HEAD(struct group, groups);
- bool global:1;
- bool duck:1;
+ bool global;
+ bool duck;
+ bool duck_while_corked;
};

static const char *sink_input_role(pa_sink_input *sink_input) {
@@ -85,7 +86,7 @@ static bool update_group_active(struct userdata *u, struct group *g) {
if (pa_hashmap_size(g->trigger_state) > 0) {
PA_HASHMAP_FOREACH_KV(sink_input, value, g->trigger_state, state) {
if (!sink_input->muted &&
- sink_input->state != PA_SINK_INPUT_CORKED) {
+ (u->duck_while_corked || sink_input->state != PA_SINK_INPUT_CORKED)) {
new_active = true;
break;
}
@@ -463,7 +464,6 @@ static int count_groups(pa_modargs *ma, const char *module_argument) {
int pa_stream_interaction_init(pa_module *m, const char* const v_modargs[]) {
pa_modargs *ma = NULL;
struct userdata *u;
- bool global = false;
uint32_t i = 0;
uint32_t group_count_tr = 0;
struct group *last = NULL;
@@ -539,11 +539,15 @@ int pa_stream_interaction_init(pa_module *m, const char* const v_modargs[]) {
if (!group_parse_roles(ma, "volume", group_value_add_volume, u->groups))
goto fail;

- if (pa_modargs_get_value_boolean(ma, "global", &global) < 0) {
+ if (pa_modargs_get_value_boolean(ma, "global", &u->global) < 0) {
pa_log("Invalid boolean parameter: global");
goto fail;
}
- u->global = global;
+
+ if (pa_modargs_get_value_boolean(ma, "duck_while_corked", &u->duck_while_corked) < 0) {
+ pa_log("Invalid boolean parameter: duck_while_corked");
+ goto fail;
+ }

pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_INPUT_PUT], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_put_cb, u);
pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_INPUT_UNLINK], PA_HOOK_LATE, (pa_hook_cb_t) sink_input_unlink_cb, u);

[Tanu Kaskinen]

Post by Juho Hämäläinen
With this enabled when trigger stream is available but
in corked state trigger is already applied to interaction
streams. This is useful when trigger stream pauses for
a while before playing, so that the trigger happens
slightly earlier.

Is this option something that you actually use (or want to use)? Can
you give a concrete example of where this is useful?

If a new option is added, I think it would be good to have it also for
module-role-cork, and I would suggest "ignore_corked_triggers" for the
option name (the meaning is inverted, so the default should be true).

[Tanu Kaskinen]

Post by Juho Hämäläinen
Stream interaction groups now have hashmaps for all trigger and interaction
states and roles. We track both trigger streams and interaction streams
separately, so that applying interaction (ducking or cork and muting) is
relatively simple.

You forgot the "why" part from the commit message. (Based on our
earlier discussion the goal is to make the code easier to understand.)

Post by Juho Hämäläinen
---
src/modules/stream-interaction.c | 388 ++++++++++++++++++++-------------------
1 file changed, 200 insertions(+), 188 deletions(-)
diff --git a/src/modules/stream-interaction.c b/src/modules/stream-interaction.c
index d538e88..c30a8c0 100644
--- a/src/modules/stream-interaction.c
+++ b/src/modules/stream-interaction.c
@@ -42,10 +42,13 @@
struct group {
char *name;
- pa_idxset *trigger_roles;
- pa_idxset *interaction_roles;
+ pa_hashmap *trigger_roles;
+ pa_hashmap *interaction_roles;
+ pa_hashmap *trigger_state;
pa_hashmap *interaction_state;

Could you add comments about what these hashmaps contain and maybe
something about how they are used? I'm reading
update_trigger_streams(), and I have some trouble understanding it. One
obstacle to understanding is that I don't know what trigger_state
contains, except that the keys are sink inputs. I could look elsewhere
in the code to figure it out, but it would be nice to have a reference
here in the struct definition.

The hashmaps seem to be used as sets, i.e. you don't care about the
values, you just want to be able to look up the keys. I suggest using
the key as the value, that way you can use PA_HASHMAP_FOREACH instead
of PA_HASHMAP_FOREACH_KV.

Post by Juho Hämäläinen
pa_volume_t volume;
+ bool any_role;
+ bool active;
PA_LLIST_FIELDS(struct group);
};
@@ -55,211 +58,228 @@ struct userdata {
PA_LLIST_HEAD(struct group, groups);
bool global:1;
bool duck:1;
- pa_hook_slot
- *sink_input_put_slot,
- *sink_input_unlink_slot,
- *sink_input_move_start_slot,
- *sink_input_move_finish_slot,
- *sink_input_state_changed_slot,
- *sink_input_mute_changed_slot,
- *sink_input_proplist_changed_slot;
};
-static const char *get_trigger_role(struct userdata *u, pa_sink_input *i, struct group *g) {
- const char *role, *trigger_role;
- uint32_t role_idx;
+static const char *sink_input_role(pa_sink_input *sink_input) {
+ const char *role;
- if (!(role = pa_proplist_gets(i->proplist, PA_PROP_MEDIA_ROLE)))
- role = STREAM_INTERACTION_NO_ROLE;
+ pa_assert(sink_input);
- if (g == NULL) {
- /* get it from all groups */
- PA_LLIST_FOREACH(g, u->groups) {
- PA_IDXSET_FOREACH(trigger_role, g->trigger_roles, role_idx) {
- if (pa_streq(role, trigger_role))
- return trigger_role;
- }
- }
- } else {
- PA_IDXSET_FOREACH(trigger_role, g->trigger_roles, role_idx) {
- if (pa_streq(role, trigger_role))
- return trigger_role;
- }
- }
+ if (!(role = pa_proplist_gets(sink_input->proplist, PA_PROP_MEDIA_ROLE)))
+ role = STREAM_INTERACTION_NO_ROLE;
- return NULL;
+ return role;
}
-static const char *find_trigger_stream(struct userdata *u, pa_sink *s, pa_sink_input *ignore, struct group *g) {
- pa_sink_input *j;
- uint32_t idx;
- const char *trigger_role;
+/* Return true if group state changes between
+ * active and inactive. */
+static bool update_group_active(struct userdata *u, struct group *g) {
+ void *state = NULL;
+ bool new_active = false;
+ pa_sink_input *sink_input = NULL;
+ void *value;
pa_assert(u);
- pa_sink_assert_ref(s);
-
- for (j = PA_SINK_INPUT(pa_idxset_first(s->inputs, &idx)); j; j = PA_SINK_INPUT(pa_idxset_next(s->inputs, &idx))) {
+ pa_assert(g);
- if (j == ignore)
- continue;
+ if (pa_hashmap_size(g->trigger_state) > 0) {
+ PA_HASHMAP_FOREACH_KV(sink_input, value, g->trigger_state, state) {
+ if (!sink_input->muted &&
+ sink_input->state != PA_SINK_INPUT_CORKED) {
+ new_active = true;
+ break;
+ }
+ }
+ }

The pa_hashmap_size() check doesn't seem to do anything useful. If the
hashmap is empty, the iteration loop won't run anyway.

Post by Juho Hämäläinen
- trigger_role = get_trigger_role(u, j, g);
- if (trigger_role && !j->muted && j->state != PA_SINK_INPUT_CORKED)
- return trigger_role;
+ if (new_active != g->active) {
+ pa_log_debug("Group '%s' is now %sactive.", g->name, new_active ? "" : "in");
+ g->active = new_active;
+ return true;
}
- return NULL;
+ return false;
}
-static const char *find_global_trigger_stream(struct userdata *u, pa_sink *s, pa_sink_input *ignore, struct group *g) {
- const char *trigger_role = NULL;
+/* Identify trigger streams and add or remove the streams from
+ * state hashmap. Proplist change when changing media.role may
+ * result in already existing stream to gain or lose trigger role
+ * status. Returns true if the handled sink-input should be ignored
+ * in interaction applying phase. */
+static bool update_trigger_streams(struct userdata *u, struct group *g, pa_sink_input *sink_input,
+ bool put, bool unlink, bool proplist) {
+ const char *role = NULL;
+ bool proplist_changed = false;

I think renaming "proplist" to "proplist_changed" and
"proplist_changed" to "trigger_status_changed" would improve the
readability.

Post by Juho Hämäläinen
+ bool can_ignore = false;
pa_assert(u);
+ pa_assert(g);
+ pa_assert(sink_input);
+
+ if (proplist) {
+ bool in_trigger_state;
+ bool in_trigger_roles;
+ role = sink_input_role(sink_input);
+
+ in_trigger_state = !!pa_hashmap_get(g->trigger_state, sink_input);
+ in_trigger_roles = !!pa_hashmap_get(g->trigger_roles, role);
+
+ /* If the sink-input is already both pointer in trigger_state hashmap
+ * and role in trigger_roles, or neither, proplist value important to
+ * us (media.role) didn't change, so no need to update anything. */
+ proplist_changed = ((in_trigger_state && in_trigger_roles) ||
+ (!in_trigger_state && !in_trigger_roles));
+ }
- if (u->global) {
- uint32_t idx;
- PA_IDXSET_FOREACH(s, u->core->sinks, idx)
- if ((trigger_role = find_trigger_stream(u, s, ignore, g)))
- break;
- } else
- trigger_role = find_trigger_stream(u, s, ignore, g);
+ if (proplist_changed || unlink) {
+ if (pa_hashmap_remove(g->trigger_state, sink_input)) {
+ can_ignore = true;
+ pa_log_debug("Stream with role '%s' removed from group '%s'", sink_input_role(sink_input), g->name);

You could set the role variable unconditionally in the beginning of the
function, then sink_input_role() wouldn't have to be called multiple
times.

I suggest adding "triggers" to the end of the log message to make it
clear that a trigger stream was removed, not a target stream. That is:

"Stream with role '%s' removed from group '%s' triggers."

Post by Juho Hämäläinen
+ }
+ }
+
+ if (proplist_changed || put) {
+ if (!role)
+ role = sink_input_role(sink_input);
- return trigger_role;
+ if (pa_hashmap_get(g->trigger_roles, role)) {
+ pa_hashmap_put(g->trigger_state, sink_input, PA_INT_TO_PTR(1));
+ can_ignore = true;
+ pa_log_debug("Stream with role '%s' added to group '%s'", role, g->name);
+ }
+ }
+
+ /* If proplist changed we need to include this stream into consideration
+ * when applying interaction roles, as the sink-input may have gained or
+ * lost trigger or interaction role. */
+ if (proplist_changed)
+ can_ignore = false;

I have trouble fully understanding what streams should be ignored and
why.

When something happens, the interactions are updated for each group. A
trigger stream can't be a target stream at the same time (within the
group, that is - being a trigger in one group and target in another is
probably possible), so ignoring makes sense for those, and indeed, you
set can_ignore to true only for new and removed trigger streams.
However, if a stream changes its role from a target to a trigger, then
its interaction need to be updated. Therefore, not all trigger streams
are ignored. Since not all trigger streams are ignored, what's the
point of having the complexity of ignoring any streams? Why not just
run the update process for all streams?

Post by Juho Hämäläinen
+
+ return can_ignore;
}
-static void cork_or_duck(struct userdata *u, pa_sink_input *i, const char *interaction_role, const char *trigger_role, bool interaction_applied, struct group *g) {
+static void cork_or_duck(struct userdata *u, struct group *g,
+ pa_sink_input *sink_input, const char *interaction_role) {
- if (u->duck && !interaction_applied) {
+ if (u->duck) {
pa_cvolume vol;
- vol.channels = 1;
- vol.values[0] = g->volume;
+ pa_cvolume_set(&vol, 1, g->volume);
- pa_log_debug("Found a '%s' stream of '%s' that ducks a '%s' stream.", trigger_role, g->name, interaction_role);
- pa_sink_input_add_volume_factor(i, g->name, &vol);
+ pa_log_debug("Group '%s' ducks a '%s' stream.", g->name, interaction_role);
+ pa_sink_input_add_volume_factor(sink_input, g->name, &vol);
- } else if (!u->duck) {
- pa_log_debug("Found a '%s' stream that corks/mutes a '%s' stream.", trigger_role, interaction_role);
- pa_sink_input_set_mute(i, true, false);
- pa_sink_input_send_event(i, PA_STREAM_EVENT_REQUEST_CORK, NULL);
+ } else {
+ pa_log_debug("Group '%s' corks/mutes a '%s' stream.", g->name, interaction_role);
+ if (!sink_input->muted)
+ pa_sink_input_set_mute(sink_input, true, false);
+ if (sink_input->state != PA_SINK_INPUT_CORKED)
+ pa_sink_input_send_event(sink_input, PA_STREAM_EVENT_REQUEST_CORK, NULL);
}
}
-static void uncork_or_unduck(struct userdata *u, pa_sink_input *i, const char *interaction_role, bool corked, struct group *g) {
+static void uncork_or_unduck(struct userdata *u, struct group *g,
+ pa_sink_input *sink_input, const char *interaction_role) {
if (u->duck) {
- pa_log_debug("In '%s', found a '%s' stream that should be unducked", g->name, interaction_role);
- pa_sink_input_remove_volume_factor(i, g->name);
- }
- else if (corked || i->muted) {
- pa_log_debug("Found a '%s' stream that should be uncorked/unmuted.", interaction_role);
- if (i->muted)
- pa_sink_input_set_mute(i, false, false);
- if (corked)
- pa_sink_input_send_event(i, PA_STREAM_EVENT_REQUEST_UNCORK, NULL);
+ pa_log_debug("In '%s', found a '%s' stream that should be unducked", g->name, interaction_role);
+ pa_sink_input_remove_volume_factor(sink_input, g->name);
+ } else {
+ pa_log_debug("In '%s' found a '%s' stream that should be uncorked/unmuted.", g->name, interaction_role);
+ if (sink_input->muted)
+ pa_sink_input_set_mute(sink_input, false, false);
+ if (sink_input->state == PA_SINK_INPUT_CORKED)
+ pa_sink_input_send_event(sink_input, PA_STREAM_EVENT_REQUEST_UNCORK, NULL);
}
}
-static inline void apply_interaction_to_sink(struct userdata *u, pa_sink *s, const char *new_trigger, pa_sink_input *ignore, bool new_stream, struct group *g) {
- pa_sink_input *j;
- uint32_t idx, role_idx;
- const char *interaction_role;
+static void apply_interaction_to_sink_input(struct userdata *u, struct group *g,
+ pa_sink_input *sink_input, bool unlink) {
bool trigger = false;
+ bool interaction_applied;
+ const char *role = STREAM_INTERACTION_ANY_ROLE;
pa_assert(u);
- pa_sink_assert_ref(s);
-
- for (j = PA_SINK_INPUT(pa_idxset_first(s->inputs, &idx)); j; j = PA_SINK_INPUT(pa_idxset_next(s->inputs, &idx))) {
- bool corked, interaction_applied;
- const char *role;
- if (j == ignore)
- continue;
-
- if (!(role = pa_proplist_gets(j->proplist, PA_PROP_MEDIA_ROLE)))
- role = STREAM_INTERACTION_NO_ROLE;
-
- PA_IDXSET_FOREACH(interaction_role, g->interaction_roles, role_idx) {
- if ((trigger = pa_streq(role, interaction_role)))
- break;
- if ((trigger = (pa_streq(interaction_role, STREAM_INTERACTION_ANY_ROLE) &&
- !get_trigger_role(u, j, g))))
- break;
- }
- if (!trigger)
- continue;
-
- /* Some applications start their streams corked, so the stream is uncorked by */
- /* the application only after sink_input_put() was called. If a new stream turns */
- /* up, act as if it was not corked. In the case of module-role-cork this will */
- /* only mute the stream because corking is reverted later by the application */
- corked = (j->state == PA_SINK_INPUT_CORKED);
- if (new_stream && corked)
- corked = false;
- interaction_applied = !!pa_hashmap_get(g->interaction_state, j);
+ if (pa_hashmap_get(g->trigger_state, sink_input))
+ return;

This means "don't do anything on trigger streams", right? What if the
stream was previously a target stream and changed its role to a
trigger, shouldn't it be uncorked/unducked?

Post by Juho Hämäläinen
- if (new_trigger && ((!corked && !j->muted) || u->duck)) {
- if (!interaction_applied)
- pa_hashmap_put(g->interaction_state, j, PA_INT_TO_PTR(1));
+ if (!g->any_role) {
+ role = sink_input_role(sink_input);
+ trigger = !!pa_hashmap_get(g->interaction_roles, role);
+ }
- cork_or_duck(u, j, role, new_trigger, interaction_applied, g);
+ if (!g->any_role && !trigger)
+ return;

I think this would be easier to read:

if (g->any_role)
trigger = true;
else {
role = sink_input_role(sink_input);
trigger = !!pa_hashmap_get(g->interaction_roles, role);
}

if (!trigger)
return;

[Tanu Kaskinen]

Post by Juho Hämäläinen
Use LLIST instead of array for interaction groups, and refactor parsing.
Side effect from refactoring is that module-role-cork also now supports
interaction groups.

The commit message could be clearer on *why* this change is made. Is
the support for interaction groups the goal? Or is dealing with llist
easier than plain arrays? Why is parsing refactored - for better
readability perhaps?

Also, what does it mean that module-role-cork now supports interaction
groups? Did module-role-ducking already support interaction groups, and
if so, was there a reason for that asymmetry?

I guess I'll find the answers to these questions when reviewing the
code, but it would have been good to have this information in the
commit message.

...

I found some answers:

LLIST indeed makes it easier to read the code than plain arrays (I
believe pa_dynarray would have been even better). The llist conversion
could easily have been a separate patch (and I hope you'll submit it as
a separate patch in v2).

Supporting groups in module-role-cork means that it's possible to have
different triggers for different to-be-corked streams. I don't know if
there's any practical use for this, but it's good to have more symmetry
between the two modules. Maybe the reason why this was not supported
before was that with ducking the benefit is maybe more clear: grouping
allows different volumes for different ducked streams.

Post by Juho Hämäläinen
---
src/modules/stream-interaction.c | 337 +++++++++++++++++++++------------------
1 file changed, 180 insertions(+), 157 deletions(-)
diff --git a/src/modules/stream-interaction.c b/src/modules/stream-interaction.c
index bf7134a..d538e88 100644
--- a/src/modules/stream-interaction.c
+++ b/src/modules/stream-interaction.c
@@ -31,21 +31,28 @@
#include <pulsecore/core-util.h>
#include <pulsecore/sink-input.h>
#include <pulsecore/modargs.h>
+#include <pulsecore/llist.h>
+#include <pulsecore/idxset.h>
#include "stream-interaction.h"
+#define STREAM_INTERACTION_DEFAULT_DUCK_VOLUME_DB (-20)
+#define STREAM_INTERACTION_ANY_ROLE "any_role"
+#define STREAM_INTERACTION_NO_ROLE "no_role"
+
struct group {
char *name;
pa_idxset *trigger_roles;
pa_idxset *interaction_roles;
pa_hashmap *interaction_state;
pa_volume_t volume;
+
+ PA_LLIST_FIELDS(struct group);
};
struct userdata {
pa_core *core;
- uint32_t n_groups;
- struct group **groups;
+ PA_LLIST_HEAD(struct group, groups);
bool global:1;
bool duck:1;
pa_hook_slot
@@ -63,13 +70,12 @@ static const char *get_trigger_role(struct userdata *u, pa_sink_input *i, struct
uint32_t role_idx;
if (!(role = pa_proplist_gets(i->proplist, PA_PROP_MEDIA_ROLE)))
- role = "no_role";
+ role = STREAM_INTERACTION_NO_ROLE;
if (g == NULL) {
/* get it from all groups */
- uint32_t j;
- for (j = 0; j < u->n_groups; j++) {
- PA_IDXSET_FOREACH(trigger_role, u->groups[j]->trigger_roles, role_idx) {
+ PA_LLIST_FOREACH(g, u->groups) {
+ PA_IDXSET_FOREACH(trigger_role, g->trigger_roles, role_idx) {
if (pa_streq(role, trigger_role))
return trigger_role;
}
@@ -170,12 +176,13 @@ static inline void apply_interaction_to_sink(struct userdata *u, pa_sink *s, con
continue;
if (!(role = pa_proplist_gets(j->proplist, PA_PROP_MEDIA_ROLE)))
- role = "no_role";
+ role = STREAM_INTERACTION_NO_ROLE;
PA_IDXSET_FOREACH(interaction_role, g->interaction_roles, role_idx) {
if ((trigger = pa_streq(role, interaction_role)))
break;
- if ((trigger = (pa_streq(interaction_role, "any_role") && !get_trigger_role(u, j, g))))
+ if ((trigger = (pa_streq(interaction_role, STREAM_INTERACTION_ANY_ROLE) &&
+ !get_trigger_role(u, j, g))))
break;
}
if (!trigger)
@@ -229,7 +236,7 @@ static void remove_interactions(struct userdata *u, struct group *g) {
if(!!pa_hashmap_get(g->interaction_state, j)) {
corked = (j->state == PA_SINK_INPUT_CORKED);
if (!(role = pa_proplist_gets(j->proplist, PA_PROP_MEDIA_ROLE)))
- role = "no_role";
+ role = STREAM_INTERACTION_NO_ROLE;
uncork_or_unduck(u, j, role, corked, g);
}
}
@@ -238,21 +245,21 @@ static void remove_interactions(struct userdata *u, struct group *g) {
static pa_hook_result_t process(struct userdata *u, pa_sink_input *i, bool create, bool new_stream) {
const char *trigger_role;
- uint32_t j;
+ struct group *g;
pa_assert(u);
pa_sink_input_assert_ref(i);
if (!create)
- for (j = 0; j < u->n_groups; j++)
- pa_hashmap_remove(u->groups[j]->interaction_state, i);
+ PA_LLIST_FOREACH(g, u->groups)
+ pa_hashmap_remove(g->interaction_state, i);
if (!i->sink)
return PA_HOOK_OK;
- for (j = 0; j < u->n_groups; j++) {
- trigger_role = find_global_trigger_stream(u, i->sink, create ? NULL : i, u->groups[j]);
- apply_interaction(u, i->sink, trigger_role, create ? NULL : i, new_stream, u->groups[j]);
+ PA_LLIST_FOREACH(g, u->groups) {
+ trigger_role = find_global_trigger_stream(u, i->sink, create ? NULL : i, g);
+ apply_interaction(u, i->sink, trigger_role, create ? NULL : i, new_stream, g);
}
return PA_HOOK_OK;
@@ -315,13 +322,127 @@ static pa_hook_result_t sink_input_proplist_changed_cb(pa_core *core, pa_sink_in
return PA_HOOK_OK;
}
+static struct group *group_new(const char *prefix, uint32_t id) {
+ struct group *g = pa_xnew0(struct group, 1);
+ PA_LLIST_INIT(struct group, g);
+ g->trigger_roles = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func,
+ pa_xfree, NULL);
+ g->interaction_roles = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func,
+ pa_xfree, NULL);

Compiler warnings:

modules/stream-interaction.c: In function ‘group_new’:
modules/stream-interaction.c:328:22: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types]
g->trigger_roles = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func,
^
modules/stream-interaction.c:330:26: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types]
g->interaction_roles = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func,
^

You probably didn't mean to change the container type in this patch.

Post by Juho Hämäläinen
+ g->interaction_state = pa_hashmap_new(pa_idxset_trivial_hash_func, pa_idxset_trivial_compare_func);
+ g->volume = pa_sw_volume_from_dB(STREAM_INTERACTION_DEFAULT_DUCK_VOLUME_DB);
+ g->name = pa_sprintf_malloc("%s_group_%u", prefix, id);
+
+ return g;
+}
+
+static void group_free(struct group *g) {
+ pa_idxset_free(g->trigger_roles, pa_xfree);
+ pa_idxset_free(g->interaction_roles, pa_xfree);
+ pa_hashmap_free(g->interaction_state);
+ pa_xfree(g->name);
+ pa_xfree(g);
+}
+
+typedef bool (*group_value_add_t)(struct group *g, const char *data);
+
+static bool group_value_add_trigger_roles(struct group *g, const char *data) {
+ pa_idxset_put(g->trigger_roles, pa_xstrdup(data), NULL);
+ return true;
+}
+
+static bool group_value_add_interaction_roles(struct group *g, const char *data) {
+ pa_idxset_put(g->interaction_roles, pa_xstrdup(data), NULL);
+ return true;
+}
+
+static bool group_value_add_volume(struct group *g, const char *data) {
+ if (pa_parse_volume(data, &(g->volume)) < 0) {
+ pa_log("Failed to parse volume");

I suggest pa_log("Failed to parse volume: %s", data)

Post by Juho Hämäläinen
+ return false;
+ }
+ return true;
+}
+
+static bool group_parse_roles(pa_modargs *ma, const char *name, group_value_add_t add_cb, struct group *groups) {

Since this is used also for the "volume" modarg, I think the function
name is misleading. I think "group_parse_modarg" would be a better
name, and the "name" variable would be clearer if it was "arg_name" or
"modarg_name". I first thought it would be the group name.

The coding style dictates that ints should be used for error reporting
(for simple success/failure, return 0 on success and -1 on failure).

The function should take an "allow_lists" flag that could be set to
false when parsing the "volume" modarg. Now "-2dB,-10dB" doesn't result
in an error.

Post by Juho Hämäläinen
+ const char *roles;

I suggest renaming this to "arg_value" or "modarg_value".

Post by Juho Hämäläinen
+ char *roles_in_group;

...and this to "value_in_group".

Post by Juho Hämäläinen
+ struct group *g;
+
+ pa_assert(ma);
+ pa_assert(name);
+ pa_assert(add_cb);
+ pa_assert(groups);
+
+ g = groups;
+ roles = pa_modargs_get_value(ma, name, NULL);
+
+ if (roles) {
+ const char *group_split_state = NULL;
+
+ while ((roles_in_group = pa_split(roles, "/", &group_split_state))) {
+ pa_assert(g);
+
+ if (roles_in_group[0] != '\0') {
+ const char *split_state = NULL;
+ char *n = NULL;
+ while ((n = pa_split(roles_in_group, ",", &split_state))) {
+ bool ret = true;
+
+ if (n[0] != '\0')
+ ret = add_cb(g, n);
+ else {
+ ret = false;
+ pa_log("Empty %s", name);

name is the modarg name, so this error message suggests that the modarg
was empty, even though it is not. I suggest "Empty value in %s."

Post by Juho Hämäläinen
+ }
+
+ pa_xfree(n);
+ if (!ret)
+ goto fail;

This leaks roles_in_group. I suggest "break;" instead, and check ret
after the pa_xfree(roles_in_group) call.

Post by Juho Hämäläinen
+ }
+ } else {
+ pa_log("Empty %s", name);

I suggest "Empty value in %s." here too.

Post by Juho Hämäläinen
+ goto fail;

This too leaks roles_in_group.

Post by Juho Hämäläinen
+ }
+
+ g = g->next;
+ pa_xfree(roles_in_group);
+ }
+ }
+
+ return true;
+
+ return false;
+}
+
+static int count_groups(pa_modargs *ma, const char *module_argument) {
+ const char *val;
+ int count = 0;
+
+ pa_assert(ma);
+ pa_assert(module_argument);
+
+ val = pa_modargs_get_value(ma, module_argument, NULL);
+ if (val) {
+ const char *split_state = NULL;
+ int len = 0;
+ /* Count empty ones as well, empty groups will fail later
+ * when parsing the groups. */
+ while (pa_split_in_place(val, "/", &len, &split_state))
+ count++;
+ }
+
+ return count;

If the module argument is empty, that counts as one group. I think
fixing that here would be better than doing it later in
pa_stream_interaction_init().

Post by Juho Hämäläinen
+}
+
int pa_stream_interaction_init(pa_module *m, const char* const v_modargs[]) {
pa_modargs *ma = NULL;
struct userdata *u;
- const char *roles;
- char *roles_in_group = NULL;
bool global = false;
uint32_t i = 0;
+ uint32_t group_count_tr = 0;
+ struct group *last = NULL;
pa_assert(m);
@@ -333,155 +454,66 @@ int pa_stream_interaction_init(pa_module *m, const char* const v_modargs[]) {
m->userdata = u = pa_xnew0(struct userdata, 1);
u->core = m->core;
+ u->duck = pa_streq(m->name, "module-role-ducking");
+ PA_LLIST_HEAD_INIT(struct group, u->groups);
- u->duck = false;
- if (pa_streq(m->name, "module-role-ducking"))
- u->duck = true;
-
- u->n_groups = 1;
+ group_count_tr = count_groups(ma, "trigger_roles");
if (u->duck) {
- const char *volumes;
- uint32_t group_count_tr = 0;
uint32_t group_count_du = 0;
uint32_t group_count_vol = 0;

These variables don't need to be initialized to 0 any more, since they
are immediately assigned a new value.

Post by Juho Hämäläinen
-
- roles = pa_modargs_get_value(ma, "trigger_roles", NULL);
- if (roles) {
- const char *split_state = NULL;
- char *n = NULL;
- while ((n = pa_split(roles, "/", &split_state))) {
- group_count_tr++;
- pa_xfree(n);
- }
- }
- roles = pa_modargs_get_value(ma, "ducking_roles", NULL);
- if (roles) {
- const char *split_state = NULL;
- char *n = NULL;
- while ((n = pa_split(roles, "/", &split_state))) {
- group_count_du++;
- pa_xfree(n);
- }
- }
- volumes = pa_modargs_get_value(ma, "volume", NULL);
- if (volumes) {
- const char *split_state = NULL;
- char *n = NULL;
- while ((n = pa_split(volumes, "/", &split_state))) {
- group_count_vol++;
- pa_xfree(n);
- }
- }
+ group_count_du = count_groups(ma, "ducking_roles");
+ group_count_vol = count_groups(ma, "volume");
if ((group_count_tr > 1 || group_count_du > 1 || group_count_vol > 1) &&

If count_groups() were to return 1 for empty modargs, this check
wouldn't be needed any more.

Post by Juho Hämäläinen
((group_count_tr != group_count_du) || (group_count_tr != group_count_vol))) {
pa_log("Invalid number of groups");
goto fail;
}
+ } else {
+ uint32_t group_count_co;
+ group_count_co = count_groups(ma, "cork_roles");
- if (group_count_tr > 0)
- u->n_groups = group_count_tr;
+ if ((group_count_tr > 1 || group_count_co > 1) &&

Same comment as above.

Post by Juho Hämäläinen
+ (group_count_tr != group_count_co)) {
+ pa_log("Invalid number of groups");
+ goto fail;
+ }
}
- u->groups = pa_xnew0(struct group*, u->n_groups);
- for (i = 0; i < u->n_groups; i++) {
- u->groups[i] = pa_xnew0(struct group, 1);
- u->groups[i]->trigger_roles = pa_idxset_new(NULL, NULL);
- u->groups[i]->interaction_roles = pa_idxset_new(NULL, NULL);
- u->groups[i]->interaction_state = pa_hashmap_new(pa_idxset_trivial_hash_func, pa_idxset_trivial_compare_func);
- if (u->duck)
- u->groups[i]->name = pa_sprintf_malloc("ducking_group_%u", i);
- }
+ /* create at least one group */
+ if (group_count_tr == 0)
+ group_count_tr = 1;

If count_groups() were to return 1 for empty modargs, this wouldn't be
needed.

Thanks for the refactoring, it definitely makes the code easier to
follow!