[pulseaudio-discuss] [PATCH] role-ducking: Add support for ducking group

[Tanu Kaskinen]

On Fri, 2016-02-19 at 22:22 +0900, Sangchul Lee wrote:
>  struct userdata {
>      pa_core *core;
>      const char *name;
> -    pa_idxset *trigger_roles;
> -    pa_idxset *ducking_roles;
> -    pa_idxset *ducked_inputs;
> +    uint32_t n_group;

I'd prefer "n_groups".

> +    pa_idxset **trigger_roles;
> +    pa_idxset **ducking_roles;
> +    pa_idxset **ducked_inputs;
> +    char **volumes;
>      bool global;
> -    pa_volume_t volume;

Why is the type for volumes changed from pa_volume_t to char*? It's
better to parse the volumes just once during the initialization rather
than every time the volume values are needed.

Rather than having four arrays containing the various attributes of a
group, it's better to define a struct for a group and have just one
array for storing the group structs.

You could also add the volume factor name to the group struct so that
it doesn't have to be constructed every time it's needed.

> @@ -230,41 +249,127 @@ int pa__init(pa_module *m) {
>      u->core = m->core;
>      u->name = m->name;
>  
> -    u->ducked_inputs = pa_idxset_new(NULL, NULL);
> -
> -    u->trigger_roles = pa_idxset_new(NULL, NULL);
>      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))) {
> -            if (n[0] != '\0')
> -                pa_idxset_put(u->trigger_roles, n, NULL);
> -            else
> -                pa_xfree(n);
> +        while ((n = pa_split(roles, "/", &split_state))) {
> +            group_count_tr++;
> +            pa_xfree(n);

This code makes "foo/" being counted as one group and "foo//" as two
groups. It would be more consistent to consider "foo/" to be two groups
and "foo//" to be three groups. However, the best way of fixing this
would be to fix pa_split() to not ignore the empty string after a
trailing delimiter, and I don't want to require you to do that change
(it would also require reviewing all other code that is currently using
pa_split() to make sure that nothing breaks by the change). If you want
to fix pa_split(), then that's awesome, but if not, then let's just
ignore this small problem.

>          }
>      }
> -    if (pa_idxset_isempty(u->trigger_roles)) {
> +    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);
> +        }
> +    }
> +
> +    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;
> +    }
> +
> +    if (group_count_tr > 0)
> +        u->n_group = group_count_tr;
> +    else
> +        u->n_group = 1;
> +
> +    u->trigger_roles = pa_xmalloc0(u->n_group * sizeof(pa_idxset*));
> +    u->ducking_roles = pa_xmalloc0(u->n_group * sizeof(pa_idxset*));
> +    u->ducked_inputs = pa_xmalloc0(u->n_group * sizeof(pa_idxset*));
> +    u->volumes = pa_xmalloc0(u->n_group * sizeof(char*));
> +    while (i < u->n_group) {

Since you're going to loop exactly u->n_group times, a for loop would
be the idiomatic choice.

> +        u->trigger_roles[i] = pa_idxset_new(NULL, NULL);
> +        u->ducking_roles[i] = pa_idxset_new(NULL, NULL);
> +        u->ducked_inputs[i++] = pa_idxset_new(NULL, NULL);
> +    }
> +
> +    roles = pa_modargs_get_value(ma, "trigger_roles", NULL);
> +    if (roles) {
> +        const char *group_split_state = NULL;
> +        char *roles_in_group = 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->trigger_roles[i], n, NULL);
> +                    else
> +                        pa_xfree(n);

If an empty string is given as a role name, I think that should be an
error.

> +                }
> +                i++;
> +            } else
> +                pa_xfree(roles_in_group);

Likewise, an empty group string should be interpreted as an empty role
name, which should be an error.

> +        }
> +    }
> +    if (pa_idxset_isempty(u->trigger_roles[0])) {
>          pa_log_debug("Using role 'phone' as trigger role.");
> -        pa_idxset_put(u->trigger_roles, pa_xstrdup("phone"), NULL);
> +        pa_idxset_put(u->trigger_roles[0], pa_xstrdup("phone"), NULL);
>      }
>  
> -    u->ducking_roles = pa_idxset_new(NULL, NULL);
>      roles = pa_modargs_get_value(ma, "ducking_roles", NULL);
>      if (roles) {
> -        const char *split_state = NULL;
> +        const char *group_split_state = NULL;
> +        char *roles_in_group = 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->ducking_roles[i], n, NULL);
> +                    else
> +                        pa_xfree(n);
> +                }
> +                i++;
> +            } else
> +                pa_xfree(roles_in_group);

Same comments as above regarding empty strings.

> +        }
> +    }
> +    if (pa_idxset_isempty(u->ducking_roles[0])) {
> +        pa_log_debug("Using roles 'music' and 'video' as ducking roles.");
> +        pa_idxset_put(u->ducking_roles[0], pa_xstrdup("music"), NULL);
> +        pa_idxset_put(u->ducking_roles[0], pa_xstrdup("video"), NULL);
> +    }
> +
> +    volumes = pa_modargs_get_value(ma, "volume", NULL);
> +    if (volumes) {
> +        const char *group_split_state = NULL;
>          char *n = NULL;
> -        while ((n = pa_split(roles, ",", &split_state))) {
> +        i = 0;
> +        while ((n = pa_split(volumes, "/", &group_split_state))) {
> +            pa_log_debug("%s", n);

This log message seems like something that you meant to remove from the
final version.

>              if (n[0] != '\0')
> -                pa_idxset_put(u->ducking_roles, n, NULL);
> +                u->volumes[i++] = n;
>              else
>                  pa_xfree(n);
>          }
>      }
> -    if (pa_idxset_isempty(u->ducking_roles)) {
> -        pa_log_debug("Using roles 'music' and 'video' as ducking roles.");
> -        pa_idxset_put(u->ducking_roles, pa_xstrdup("music"), NULL);
> -        pa_idxset_put(u->ducking_roles, pa_xstrdup("video"), NULL);
> +    if (!u->volumes[0])
> +        u->volumes[0] = pa_xstrdup("-20db");
> +
> +    for (i = 0; i < u->n_group; i++) {
> +        if (pa_parse_volume(u->volumes[i], &volume) == -1) {
> +            pa_log("Failed to parse a volume parameter: volume");

This message looks strange to me (I know it was there already earlier).
I think "Failed to parse volume" would be better.

-- 
Tanu