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
