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 at 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!
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
