[pulseaudio-discuss] [PATCH 2/2] module-ducking: Applying the ducking effect for specified streams
Flávio Ceolin
flavio.ceolin at profusion.mobi
Mon Oct 22 07:22:44 PDT 2012
Hi Tanu,
On Fri, Oct 19, 2012 at 3:33 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Tue, 2012-10-09 at 15:16 -0300, 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
>> ---
>> src/Makefile.am | 10 +-
>> src/modules/module-role-ducking.c | 330 ++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 339 insertions(+), 1 deletion(-)
>> create mode 100644 src/modules/module-role-ducking.c
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 6212c74..a12df63 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -1053,7 +1053,8 @@ modlibexec_LTLIBRARIES += \
>> module-switch-on-connect.la \
>> module-switch-on-port-available.la \
>> module-filter-apply.la \
>> - module-filter-heuristics.la
>> + module-filter-heuristics.la \
>> + module-role-ducking.la
>>
>> if HAVE_ESOUND
>> modlibexec_LTLIBRARIES += \
>> @@ -1361,6 +1362,7 @@ SYMDEF_FILES = \
>> module-raop-discover-symdef.h \
>> module-gconf-symdef.h \
>> module-position-event-sounds-symdef.h \
>> + module-role-ducking-symdef.h \
>> module-augment-properties-symdef.h \
>> module-role-cork-symdef.h \
>> module-console-kit-symdef.h \
>> @@ -1758,6 +1760,12 @@ module_position_event_sounds_la_LDFLAGS = $(MODULE_LDFLAGS)
>> module_position_event_sounds_la_LIBADD = $(MODULE_LIBADD)
>> module_position_event_sounds_la_CFLAGS = $(AM_CFLAGS)
>>
>> +# Ducking effect in presence of a DUCKING stream
>
> I think "Ducking effect based on stream roles" would be a more
> descriptive comment.
>
>> +module_role_ducking_la_SOURCES = modules/module-role-ducking.c
>> +module_role_ducking_la_LDFLAGS = $(MODULE_LDFLAGS)
>> +module_role_ducking_la_LIBADD = $(MODULE_LIBADD)
>> +module_role_ducking_la_CFLAGS = $(AM_CFLAGS)
>> +
>> # Augment properties from XDG .desktop files
>> module_augment_properties_la_SOURCES = modules/module-augment-properties.c
>> module_augment_properties_la_LDFLAGS = $(MODULE_LDFLAGS)
>> diff --git a/src/modules/module-role-ducking.c b/src/modules/module-role-ducking.c
>> new file mode 100644
>> index 0000000..428dc37
>> --- /dev/null
>> +++ b/src/modules/module-role-ducking.c
>> @@ -0,0 +1,330 @@
>> +/***
>> + This file is part of PulseAudio.
>> +
>> + Copyright 2012 Flavio Ceolin <flavio.ceolin at profusion.mobi>
>> +
>> + PulseAudio is free software; you can redistribute it and/or modify
>> + it under the terms of the GNU Lesser General Public License as published
>> + by the Free Software Foundation; either version 2.1 of the License,
>> + or (at your option) any later version.
>> +
>> + PulseAudio is distributed in the hope that it will be useful, but
>> + WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + General Public License for more details.
>> +
>> + You should have received a copy of the GNU Lesser General Public License
>> + along with PulseAudio; if not, write to the Free Software
>> + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
>> + USA.
>> +***/
>> +
>> +#ifdef HAVE_CONFIG_H
>> +#include <config.h>
>> +#endif
>> +
>> +#include <pulse/volume.h>
>> +#include <pulse/xmalloc.h>
>> +
>> +#include <pulsecore/macro.h>
>> +#include <pulsecore/hashmap.h>
>> +#include <pulsecore/hook-list.h>
>> +#include <pulsecore/core.h>
>> +#include <pulsecore/core-util.h>
>> +#include <pulsecore/sink-input.h>
>> +#include <pulsecore/modargs.h>
>> +
>> +#include "module-role-ducking-symdef.h"
>> +
>> +
>> +PA_MODULE_AUTHOR("Flavio Ceolin <flavio.ceolin at profusion.mobi>");
>> +PA_MODULE_DESCRIPTION("Apply ducking effect on streams with certain roles while others exist");
>> +PA_MODULE_VERSION(PACKAGE_VERSION);
>> +PA_MODULE_LOAD_ONCE(TRUE);
>> +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 duckinged> "
>
> I don't think "duckinged" is a proper word. Use "ducked" instead (this
> is not the only occurrence, so please replace all instances of
> "duckinged").
>
No problem, just fixed.
>> + "global=<Should we operate globally or only inside the same device?>"
>> + "volume=<linear factor for the volume. 0.0 and less is muted while 1.0 is PA_VOLUME_NORM>");
>
> I don't like the choice of using linear factor as the parameter type.
> The "native" volume type is pa_volume_t. So, I'd like you to use either
> that type as the parameter type, or if that's too un-user-friendly (a
> linear factor isn't very user-friendly either, btw), then accept any of
> these: a plain integer (pa_volume_t), a number followed by a percentage
> sign or a number followed by "dB". There could be
> pa_modargs_get_value_volume() that does the parsing (it could be useful
> elsewhere too).
>
So, instead of accept a linear value, i'll give to user three options
dB, % and integer, right ?
I'm ok with this solution, i liked the idea.
>> +
>> +static const char* const valid_modargs[] = {
>> + "trigger_roles",
>> + "ducking_roles",
>> + "global",
>> + "volume",
>> + NULL
>> +};
>> +
>> +struct userdata {
>> + pa_core *core;
>> + pa_hashmap *ducking_state;
>
> I'd like a comment here about the key and value types.
>
>> + pa_idxset *trigger_roles;
>> + pa_idxset *ducking_roles;
>> + pa_bool_t global:1;
>> + 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) {
>> + pa_sink_input *j;
>> + uint32_t idx, role_idx;
>> + const char *trigger_role;
>> +
>> + pa_assert(u);
>> + pa_sink_assert_ref(s);
>> +
>> + for (j = PA_SINK_INPUT(pa_idxset_first(s->inputs, &idx)); j; j = PA_SINK_INPUT(pa_idxset_next(s->inputs, &idx))) {
>
> The PA_IDXSET_FOREACH macro would be prettier.
>
>> + 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 inline void apply_ducking_to_sink(struct userdata *u, pa_sink *s, pa_sink_input *ignore, pa_bool_t ducking) {
>
> The "inline" specifier seems out of place. The compiler knows probably
> better than you whether it makes sense to inline this function or not,
> and this is not performance critical code anyway.
>
>> + pa_sink_input *j;
>> + uint32_t idx, role_idx;
>> + const char *ducking_role;
>> + pa_bool_t trigger = FALSE;
>> + pa_cvolume *v;
>> +
>> + pa_assert(u);
>> + pa_sink_assert_ref(s);
>> +
>> + for (j = PA_SINK_INPUT(pa_idxset_first(s->inputs, &idx)); j; j = PA_SINK_INPUT(pa_idxset_next(s->inputs, &idx))) {
>
> The PA_IDXSET_FOREACH macro would be prettier.
>
>> + const char *role;
>> +
>> + 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;
>> +
>> + if (ducking) {
>> + pa_cvolume aux;
>> + v = pa_hashmap_get(u->ducking_state, j);
>> +
>> + pa_log_debug("Found a '%s' stream that should be duckinged.", ducking_role);
>> + if (!v) {
>> + v = pa_xmalloc(sizeof (pa_cvolume));
>
> The convention is to use pa_xnew(1, pa_cvolume).
>
>> + pa_hashmap_put(u->ducking_state, j, pa_sink_input_get_volume(j, v, FALSE));
>> + }
>> +
>> + aux = *v;
>> + pa_cvolume_set(&aux, aux.channels, u->volume);
>> + pa_sink_input_set_volume(j, &aux, FALSE, FALSE);
>
> I don't think the volume change should be visible outside, so
> pa_sink_input_set_volume() shouldn't be called. Instead,
> j->volume_factor should be set (it's a sort of "invisible" volume
> modifier). There's no good way to do that, however. The volume factor
> feature is used very little, and there's currently no code that needs to
> update it on the fly - currently it's only set once during stream
> creation and it stays the same for the lifetime of the stream. I think
> pa_sink_input_set_volume_factor() needs to be created, which would also
> update j->soft_volume (and j->thread_info.soft_volume).
>
> Or even better, pa_sink_input.volume_factor could be replaced with a
> list (or whatever container makes most sense) of volume factors. There
> can be multiple modules, each independently applying volume factors. If
> there is only one volume variable, those modules will overwrite each
> other's volumes. Therefore, it's better to give each module its own
> volume factor object.
If multiple modules can apply volume factors, should exists some kind
of priority between them ?
is there a similar case in pulseaudio that i can take a look ?
>
>> + } else {
>> + v = pa_hashmap_remove(u->ducking_state, j);
>> + if (v) {
>> + pa_log_debug("Found a '%s' stream that should be unduckinged.", ducking_role);
>> + pa_sink_input_set_volume(j, v, FALSE, FALSE);
>> + pa_xfree(v);
>> + }
>> + }
>> + }
>> +}
>> +
>> +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 create) {
>
> "create" is a very confusing variable name, at least for me. It's not
> clear at all what is being created, even after reading the code.
>
>> + pa_bool_t ducking = FALSE;
>> + const char *role;
>> + pa_cvolume *v;
>> +
>> + pa_assert(u);
>> + pa_sink_input_assert_ref(i);
>> +
>> + if (!create) {
>> + v = pa_hashmap_remove(u->ducking_state, i);
>> + pa_xfree(v);
>> + }
>> +
>> + if (!(role = pa_proplist_gets(i->proplist, PA_PROP_MEDIA_ROLE)))
>> + return PA_HOOK_OK;
>> +
>> + if (!i->sink)
>> + return PA_HOOK_OK;
>> +
>> + ducking = shall_ducking(u, i->sink, create ? NULL : i);
>> + apply_ducking(u, i->sink, create ? NULL : i, ducking);
>> +
>> + return PA_HOOK_OK;
>> +}
>> +
>> +static pa_hook_result_t sink_input_put_cb(pa_core *core, pa_sink_input *i, struct userdata *u) {
>> + pa_core_assert_ref(core);
>> + pa_sink_input_assert_ref(i);
>> +
>> + return process(u, i, TRUE);
>> +}
>> +
>> +static pa_hook_result_t sink_input_unlink_cb(pa_core *core, pa_sink_input *i, struct userdata *u) {
>> + pa_sink_input_assert_ref(i);
>> +
>> + return process(u, i, FALSE);
>> +}
>> +
>> +static pa_hook_result_t sink_input_move_start_cb(pa_core *core, pa_sink_input *i, struct userdata *u) {
>> + pa_core_assert_ref(core);
>> + pa_sink_input_assert_ref(i);
>> +
>> + return process(u, i, FALSE);
>> +}
>> +
>> +static pa_hook_result_t sink_input_move_finish_cb(pa_core *core, pa_sink_input *i, struct userdata *u) {
>> + pa_core_assert_ref(core);
>> + pa_sink_input_assert_ref(i);
>> +
>> + return process(u, i, TRUE);
>> +}
>> +
>> +int pa__init(pa_module *m) {
>> + pa_modargs *ma = NULL;
>> + struct userdata *u;
>> + const char *roles;
>> + pa_bool_t global = FALSE;
>> + double volume = 0.15;
>> +
>> + pa_assert(m);
>> +
>> + if (!(ma = pa_modargs_new(m->argument, valid_modargs))) {
>> + pa_log("Failed to parse module arguments");
>> + goto fail;
>> + }
>> +
>> + m->userdata = u = pa_xnew(struct userdata, 1);
>> +
>> + u->core = m->core;
>> + u->ducking_state = pa_hashmap_new(pa_idxset_trivial_hash_func, pa_idxset_trivial_compare_func);
>> +
>> + 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);
>> + }
>> + }
>> + if (pa_idxset_isempty(u->trigger_roles)) {
>> + pa_log_debug("Using role 'phone' as trigger role.");
>> + pa_idxset_put(u->trigger_roles, 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;
>> + char *n = NULL;
>> + while ((n = pa_split(roles, ",", &split_state))) {
>> + if (n[0] != '\0')
>> + pa_idxset_put(u->ducking_roles, n, NULL);
>> + 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 (pa_modargs_get_value_boolean(ma, "global", &global) < 0) {
>> + pa_log("Invalid boolean parameter: global");
>> + goto fail;
>> + }
>> + u->global = global;
>> +
>> + if (pa_modargs_get_value_double(ma, "volume", &volume) < 0) {
>> + pa_log("Invalid unsigned integer parameter: volume");
>
> The log message talks about unsigned integers, while the variable is a
> double.
>
> --
> Tanu
>
Thanks for reviewing, i'm already fixing the spotted problems.
Best Regards,
Flávio Ceolin
More information about the pulseaudio-discuss
mailing list