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

[Tanu Kaskinen]

On Wed, 2016-03-30 at 22:20 +0900, Sangchul Lee wrote:
> +struct userdata {
> +    pa_core *core;
> +    const char *name;

userdata doesn't need the name field any more. It's only referenced
once in pa_stream_interaction_init(), and that function can equally
well use m->name in place of u->name.

> -static void uncork_or_unduck(struct userdata *u, pa_sink_input *i, const char *interaction_role, bool corked) {
> +static void uncork_or_unduck(struct userdata *u, pa_sink_input *i, const char *interaction_role, bool corked, struct group *g) {
>  
>      if (u->duck) {
> -       pa_log_debug("Found a '%s' stream that should be unducked", interaction_role);
> -       pa_sink_input_remove_volume_factor(i, u->name);
> +       pa_log_debug("Found a '%s' stream that should be unducked by '%s'", interaction_role, g->name);

Groups don't unduck streams, so the log message is not good language.
"In '%s', found a '%s' stream that should be unducked" would be better
(the first %s is the group name).

>  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;
>  
>      pa_assert(u);
>      pa_sink_input_assert_ref(i);
>  
> -    if (!create)
> -        pa_hashmap_remove(u->interaction_state, i);
> -
>      if (!i->sink)
>          return PA_HOOK_OK;
>  
> -    trigger_role = find_global_trigger_stream(u, i->sink, create ? NULL : i);
> -    apply_interaction(u, i->sink, trigger_role, create ? NULL : i, new_stream);
> +    for (j = 0; j < u->n_groups; j++) {
> +        if (!create)
> +            pa_hashmap_remove(u->groups[j]->interaction_state, i);
> +        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]);
> +    }

The pa_hashmap_remove() call was moved after the if (!i->sink) check,
which means that if i->sink is not set, pa_hashmap_remove() doesn't any
more get called even if it should.

> +                    if (pa_parse_volume(n, &(u->groups[i++]->volume)) == -1) {

Use "< 0" instead of "== -1". Any negative number indicates a failure,
so comparing to only -1 makes it look like -1 would be somehow special.
The reader can't be expected to know that the function never returns
other error codes than -1.

-- 
Tanu