[pulseaudio-discuss] [PATCH v2 3/3] module-ducking: Applying the ducking effect for specified streams

[Tanu Kaskinen]

On Thu, 2012-11-29 at 11:04 -0200, Flavio Ceolin wrote:
> This module works pretty similar to the module-role-cork.
> It should be used as an alternative to that module. Basically
> it decreases the volume of the streams specified in ducking_roles
> in the presence of at least one stream specified in trigger_roles.
> Also, it's possible to choice the volume that will be used in the
> ducking streams and if it should operates in all devices or not.
> 
> For basic reference: http://en.wikipedia.org/wiki/Ducking

About the commit title: "module-ducking: Applying the ducking effect for
specified streams"... The module name is module-role-ducking, not
module-ducking, and the commit labels don't usually have the "module-"
prefix, so that should be changed to "role-ducking". Also, I think
"Apply a ducking effect based on stream roles" would be better than
"Applying the ducking effect for specified streams".

> +PA_MODULE_USAGE(
> +        "trigger_roles=<Comma separated list of roles which will trigger a ducking> "
> +        "ducking_roles=<Comma separated list of roles which will be ducked> "
> +        "global=<Should we operate globally or only inside the same device?>"
> +        "volume=<It's allowed to use three formats, the value in percentage or in decibel or as integer>"

The documentation for the volume option isn't very useful. Instead of
trying to specify the format, it would be better to state what the
volume will be used for. A more suitable place for the format
specification would be the wiki, since a proper format specification
would probably be too long to be here.

> +struct userdata {
> +    pa_core *core;
> +    const char *name;
> +    pa_idxset *trigger_roles;
> +    pa_idxset *ducking_roles;
> +    pa_idxset *ducked_inputs;
> +    pa_bool_t global:1;

pa_bool_t is deprecated, use bool instead. There are more places where
you use pa_bool_t, change them all.

> +    pa_volume_t volume;
> +    pa_hook_slot
> +        *sink_input_put_slot,
> +        *sink_input_unlink_slot,
> +        *sink_input_move_start_slot,
> +        *sink_input_move_finish_slot;
> +};
> +
> +static pa_bool_t shall_ducking(struct userdata *u, pa_sink *s, pa_sink_input *ignore) {

"shall_duck" would be more correct grammatically. Another suggestion
would be "sink_has_trigger_streams" - I think that would be more clear
about what the function actually does.

> +    pa_sink_input *j;
> +    uint32_t idx, role_idx;
> +    const char *trigger_role;
> +
> +    pa_assert(u);
> +    pa_sink_assert_ref(s);
> +
> +    PA_IDXSET_FOREACH(j, s->inputs, idx) {
> +        const char *role;
> +
> +        if (j == ignore)
> +            continue;
> +
> +        if (!(role = pa_proplist_gets(j->proplist, PA_PROP_MEDIA_ROLE)))
> +            continue;
> +
> +        PA_IDXSET_FOREACH(trigger_role, u->trigger_roles, role_idx) {
> +            if (pa_streq(role, trigger_role)) {
> +                pa_log_debug("Found a '%s' stream that will trigger the ducking.", trigger_role);
> +                return TRUE;
> +            }
> +        }
> +    }
> +
> +    return FALSE;
> +}
> +
> +static void apply_ducking_to_sink(struct userdata *u, pa_sink *s, pa_sink_input *ignore, pa_bool_t ducking) {

If a boolean variable is used to tell whether something should be done,
"do_foo" makes more sense than "doing_foo". Therefore, "duck" would be
better than "ducking". This is not the only instance, please fix all of
them.

> +    pa_cvolume aux;

Wouldn't "vol" or "volume" be a more obvious name for this variable?
Also, this is only used inside one if block, so this could be moved
there.

> +    pa_sink_input *j;
> +    uint32_t idx, role_idx;
> +    const char *ducking_role;
> +    pa_bool_t trigger = FALSE;
> +
> +    pa_assert(u);
> +    pa_sink_assert_ref(s);
> +
> +    PA_IDXSET_FOREACH(j, s->inputs, idx) {
> +        const char *role;
> +        pa_sink_input *i;
> +
> +        if (j == ignore)
> +            continue;
> +
> +        if (!(role = pa_proplist_gets(j->proplist, PA_PROP_MEDIA_ROLE)))
> +            continue;
> +
> +        PA_IDXSET_FOREACH(ducking_role, u->ducking_roles, role_idx) {
> +            if ((trigger = pa_streq(role, ducking_role)))
> +                break;
> +        }
> +        if (!trigger)
> +            continue;
> +
> +        i = pa_idxset_get_by_data(u->ducked_inputs, j, NULL);
> +        if (ducking && !i) {
> +            pa_log_debug("Found a '%s' stream that should be ducked.", ducking_role);
> +            pa_sink_input_get_volume(j, &aux, FALSE);

This call seems to be here only to get the channel count of the input.
Using j->sample_spec.channels would be better, but since
pa_sink_input_add_volume_factor() accepts mono volumes, even better is
to just ignore the sink input channels and initialize the volume with
only one channel.

> +            pa_cvolume_set(&aux, aux.channels, u->volume);
> +            pa_sink_input_add_volume_factor(j, u->name, &aux);
> +            pa_idxset_put(u->ducked_inputs, j, NULL);
> +        } else if (!ducking && i) { /* This stream should not longer be ducked */
> +            pa_idxset_remove_by_data(u->ducked_inputs, j, NULL);
> +            pa_sink_input_remove_volume_factor(j, u->name);

I think there should be a log message about unducking to match the
message about ducking.

> +        }
> +    }
> +}
> +
> +static void apply_ducking(struct userdata *u, pa_sink *s, pa_sink_input *ignore, pa_bool_t ducking) {
> +    pa_assert(u);
> +
> +    if (u->global) {
> +        uint32_t idx;
> +        PA_IDXSET_FOREACH(s, u->core->sinks, idx)
> +            apply_ducking_to_sink(u, s, ignore, ducking);
> +    } else
> +        apply_ducking_to_sink(u, s, ignore, ducking);
> +}
> +
> +static pa_hook_result_t process(struct userdata *u, pa_sink_input *i, pa_bool_t ducking) {
> +    pa_bool_t should_ducking = FALSE;

"should_duck"

> +    if (pa_modargs_get_value_boolean(ma, "global", &global) < 0) {
> +        pa_log("Invalid boolean parameter: global");

I think the error message can be understood so that the module doesn't
recognize the option name. I think "Failed to parse a boolean parameter:
global" would be a better message.

> +        goto fail;
> +    }
> +    u->global = global;
> +
> +    if (pa_modargs_get_value_volume(ma, "volume", &volume) < 0) {
> +        pa_log("Invalid parameter: volume");

Same as above.

-- 
Tanu