On Sat, 2016-03-26 at 00:07 +0900, Sangchul Lee wrote:
> Now, trigger_roles, ducking_roles and volume can be divided into several groups by slash.
> That means each group can be affected by its own volume policy.
>
> If we need to apply ducking volume level differently that is triggered
> from each trigger role(s), this feature would be useful for this purpose.
>
> For example, let's assume that tts should take music and video's volume down to 40%
> whereas voice_recognition should take those and tts's volume down to 20%.
> In this case, the configuration can be written as below.
> Â trigger_roles=tts/voice_recognition ducking_roles=music,video/music,video,tts volume=40%/20%
>
> If one of ducking role is affected by more than two trigger roles simultaneously,
> volume of the ducking role will be applied by method of multiplication.
> And it works in the same way as before without any slash.
>
> Signed-off-by: Sangchul Lee <sc11.lee at samsung.com>
> ---
>
> Notes:
> Â v2 changelog: revise codes according to Tanu's comments
> Â - commit message enhancement
> Â - definition of group structure
> Â - rename variable and revise logs
> Â - handle error case of empty parsed string
>
> Â it would be integrated with the latest upstream codes in the next update(v3)
>
> Â src/modules/module-role-ducking.c | 241 +++++++++++++++++++++++++++-----------
> Â 1 file changed, 175 insertions(+), 66 deletions(-)
Looks pretty good. A few comments still below.
> +typedef struct group {
> +Â Â Â Â char *name;
> Â Â Â Â Â pa_idxset *trigger_roles;
> Â Â Â Â Â pa_idxset *ducking_roles;
> Â Â Â Â Â pa_idxset *ducked_inputs;
> -Â Â Â Â bool global;
> Â Â Â Â Â pa_volume_t volume;
> +} group;
The convention in PulseAudio is to not use typedefs on private structs.
See how "struct userdata" is defined: the definition doesn't use a
typedef.
> -static bool sink_has_trigger_streams(struct userdata *u, pa_sink *s, pa_sink_input *ignore) {
> +static bool sink_has_trigger_streams(struct userdata *u, pa_sink *s, pa_sink_input *ignore, uint32_t group_idx) {
Rather than passing the group index as a parameter, you can just give a
direct pointer to the group (this same comment applies to all functions
that you pass group_idx as a parameter).
> @@ -230,41 +248,137 @@ int pa__init(pa_module *m) {
> Â Â Â Â Â u->core = m->core;
> Â Â Â Â Â u->name = m->name;
> Â
> -Â Â Â Â u->ducked_inputs = pa_idxset_new(NULL, NULL);
> -
> -Â Â Â Â 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);
> +Â Â Â Â Â Â Â Â while ((n = pa_split(roles, "/", &split_state))) {
> +Â Â Â Â Â Â Â Â Â Â Â Â group_count_tr++;
> +Â Â Â Â Â Â Â Â Â Â Â Â pa_xfree(n);
> +Â Â Â Â Â Â Â Â }
> +Â Â Â Â }
> +Â Â Â Â 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))) {
> +Â Â Â Â Â Â Â Â Â Â Â Â group_count_du++;
> +Â Â Â Â Â Â Â Â Â Â Â Â pa_xfree(n);
> +Â Â Â Â Â Â Â Â }
> +Â Â Â Â }
> +Â Â Â Â volumes = pa_modargs_get_value(ma, "volume", NULL);
> +Â Â Â Â if (volumes) {
> +Â Â Â Â Â Â Â Â const char *split_state = NULL;
> +Â Â Â Â Â Â Â Â char *n = NULL;
> +Â Â Â Â Â Â Â Â while ((n = pa_split(volumes, "/", &split_state))) {
> +Â Â Â Â Â Â Â Â Â Â Â Â group_count_vol++;
> +Â Â Â Â Â Â Â Â Â Â Â Â pa_xfree(n);
> +Â Â Â Â Â Â Â Â }
> +Â Â Â Â }
> +
> +Â Â Â Â if ((group_count_tr > 1 || group_count_du > 1 || group_count_vol > 1) &&
> +Â Â Â Â Â Â Â Â ((group_count_tr != group_count_du) || (group_count_tr != group_count_vol))) {
> +Â Â Â Â Â Â Â Â pa_log("Invalid number of groups");
> +Â Â Â Â Â Â Â Â goto fail;
> +Â Â Â Â }
> +
> +Â Â Â Â if (group_count_tr > 0)
> +Â Â Â Â Â Â Â Â u->n_groups = group_count_tr;
> +Â Â Â Â else
> +Â Â Â Â Â Â Â Â u->n_groups = 1;
> +
> +Â Â Â Â u->groups = pa_xmalloc0(u->n_groups * sizeof(group*));
> +Â Â Â Â for (i = 0; i < u->n_groups; i++) {
> +Â Â Â Â Â Â Â Â u->groups[i] = pa_xmalloc0(sizeof(group));
> +Â Â Â Â Â Â Â Â u->groups[i]->name = pa_sprintf_malloc("%s_group_%u", u->name, i);
> +Â Â Â Â Â Â Â Â u->groups[i]->trigger_roles = pa_idxset_new(NULL, NULL);
> +Â Â Â Â Â Â Â Â u->groups[i]->ducking_roles = pa_idxset_new(NULL, NULL);
> +Â Â Â Â Â Â Â Â u->groups[i]->ducked_inputs = pa_idxset_new(NULL, NULL);
> +Â Â Â Â }
> +
> +Â Â Â Â roles = pa_modargs_get_value(ma, "trigger_roles", NULL);
> +Â Â Â Â if (roles) {
> +Â Â Â Â Â Â Â Â const char *group_split_state = NULL;
> +Â Â Â Â Â Â Â Â char *roles_in_groups = NULL;
In v1 this variable was called "roles_in_group". That was a better
name, because the string contains roles for only one group.
--
Tanu
