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
