[1/3] stream-interaction: Use LLIST for groups.

Submitted by Juho Hämäläinen on July 13, 2018, 8:21 a.m.

Details

Message ID 1531470065-6571-2-git-send-email-jusa@hilvi.org
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Juho Hämäläinen July 13, 2018, 8:21 a.m.
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 <jusa@hilvi.org>
---
 src/modules/stream-interaction.c | 337 +++++++++++++++++++++------------------
 1 file changed, 180 insertions(+), 157 deletions(-)

Patch hide | download patch | download mbox

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)

Comments

On Fri, 2018-07-13 at 11:21 +0300, Juho Hämäläinen wrote:
> 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.

> Signed-off-by: Juho Hämäläinen <jusa@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);

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.

> +    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)

> +        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.

> +    const char *roles;

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

> +    char *roles_in_group;

...and this to "value_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);

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."

> +                    }
> +
> +                    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.

> +                }
> +            } else {
> +                pa_log("Empty %s", name);

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

> +                goto fail;

This too leaks roles_in_group.

> +            }
> +
> +            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;

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().

> +}
> +
>  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.

> -
> -        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.

>              ((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.

> +            (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!