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

[Tanu Kaskinen]

On Sat, 2016-03-26 at 00:07 +0900, Sangchul Lee wrote:
> Now, trigger_roles, ducking_roles and volume can be divided into several groups by slash.
> That means each group can be affected by its own volume policy.
> 
> If we need to apply ducking volume level differently that is triggered
> from each trigger role(s), this feature would be useful for this purpose.
> 
> For example, let's assume that tts should take music and video's volume down to 40%
> whereas voice_recognition should take those and tts's volume down to 20%.
> In this case, the configuration can be written as below.
>   trigger_roles=tts/voice_recognition ducking_roles=music,video/music,video,tts volume=40%/20%
> 
> If one of ducking role is affected by more than two trigger roles simultaneously,
> volume of the ducking role will be applied by method of multiplication.
> And it works in the same way as before without any slash.
> 
> Signed-off-by: Sangchul Lee <sc11.lee at samsung.com>
> ---
> 
> Notes:
>  v2 changelog: revise codes according to Tanu's comments
>   - commit message enhancement
>   - definition of group structure
>   - rename variable and revise logs
>   - handle error case of empty parsed string
> 
>  it would be integrated with the latest upstream codes in the next update(v3)
> 
>  src/modules/module-role-ducking.c | 241 +++++++++++++++++++++++++++-----------
>  1 file changed, 175 insertions(+), 66 deletions(-)

Looks pretty good. A few comments still below.

> +typedef struct group {
> +    char *name;
>      pa_idxset *trigger_roles;
>      pa_idxset *ducking_roles;
>      pa_idxset *ducked_inputs;
> -    bool global;
>      pa_volume_t volume;
> +} group;

The convention in PulseAudio is to not use typedefs on private structs.
See how "struct userdata" is defined: the definition doesn't use a
typedef.

> -static bool sink_has_trigger_streams(struct userdata *u, pa_sink *s, pa_sink_input *ignore) {
> +static bool sink_has_trigger_streams(struct userdata *u, pa_sink *s, pa_sink_input *ignore, uint32_t group_idx) {

Rather than passing the group index as a parameter, you can just give a
direct pointer to the group (this same comment applies to all functions
that you pass group_idx as a parameter).

> @@ -230,41 +248,137 @@ 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);
> +        }
> +    }
> +    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_groups = group_count_tr;
> +    else
> +        u->n_groups = 1;
> +
> +    u->groups = pa_xmalloc0(u->n_groups * sizeof(group*));
> +    for (i = 0; i < u->n_groups; i++) {
> +        u->groups[i] = pa_xmalloc0(sizeof(group));
> +        u->groups[i]->name = pa_sprintf_malloc("%s_group_%u", u->name, i);
> +        u->groups[i]->trigger_roles = pa_idxset_new(NULL, NULL);
> +        u->groups[i]->ducking_roles = pa_idxset_new(NULL, NULL);
> +        u->groups[i]->ducked_inputs = 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_groups = NULL;

In v1 this variable was called "roles_in_group". That was a better
name, because the string contains roles for only one group.

-- 
Tanu