On Fri, 2018-07-13 at 11:21 +0300, Juho Hämäläinen wrote: > Stream interaction groups now have hashmaps for all trigger and interaction > states and roles. We track both trigger streams and interaction streams > separately, so that applying interaction (ducking or cork and muting) is > relatively simple.
You forgot the "why" part from the commit message. (Based on our earlier discussion the goal is to make the code easier to understand.) > Signed-off-by: Juho Hämäläinen <j...@hilvi.org> > --- > src/modules/stream-interaction.c | 388 > ++++++++++++++++++++------------------- > 1 file changed, 200 insertions(+), 188 deletions(-) > > diff --git a/src/modules/stream-interaction.c > b/src/modules/stream-interaction.c > index d538e88..c30a8c0 100644 > --- a/src/modules/stream-interaction.c > +++ b/src/modules/stream-interaction.c > @@ -42,10 +42,13 @@ > > struct group { > char *name; > - pa_idxset *trigger_roles; > - pa_idxset *interaction_roles; > + pa_hashmap *trigger_roles; > + pa_hashmap *interaction_roles; > + pa_hashmap *trigger_state; > pa_hashmap *interaction_state; Could you add comments about what these hashmaps contain and maybe something about how they are used? I'm reading update_trigger_streams(), and I have some trouble understanding it. One obstacle to understanding is that I don't know what trigger_state contains, except that the keys are sink inputs. I could look elsewhere in the code to figure it out, but it would be nice to have a reference here in the struct definition. The hashmaps seem to be used as sets, i.e. you don't care about the values, you just want to be able to look up the keys. I suggest using the key as the value, that way you can use PA_HASHMAP_FOREACH instead of PA_HASHMAP_FOREACH_KV. > pa_volume_t volume; > + bool any_role; > + bool active; > > PA_LLIST_FIELDS(struct group); > }; > @@ -55,211 +58,228 @@ struct userdata { > PA_LLIST_HEAD(struct group, groups); > bool global:1; > bool duck:1; > - pa_hook_slot > - *sink_input_put_slot, > - *sink_input_unlink_slot, > - *sink_input_move_start_slot, > - *sink_input_move_finish_slot, > - *sink_input_state_changed_slot, > - *sink_input_mute_changed_slot, > - *sink_input_proplist_changed_slot; > }; > > -static const char *get_trigger_role(struct userdata *u, pa_sink_input *i, > struct group *g) { > - const char *role, *trigger_role; > - uint32_t role_idx; > +static const char *sink_input_role(pa_sink_input *sink_input) { > + const char *role; > > - if (!(role = pa_proplist_gets(i->proplist, PA_PROP_MEDIA_ROLE))) > - role = STREAM_INTERACTION_NO_ROLE; > + pa_assert(sink_input); > > - if (g == NULL) { > - /* get it from all groups */ > - PA_LLIST_FOREACH(g, u->groups) { > - PA_IDXSET_FOREACH(trigger_role, g->trigger_roles, role_idx) { > - if (pa_streq(role, trigger_role)) > - return trigger_role; > - } > - } > - } else { > - PA_IDXSET_FOREACH(trigger_role, g->trigger_roles, role_idx) { > - if (pa_streq(role, trigger_role)) > - return trigger_role; > - } > - } > + if (!(role = pa_proplist_gets(sink_input->proplist, PA_PROP_MEDIA_ROLE))) > + role = STREAM_INTERACTION_NO_ROLE; > > - return NULL; > + return role; > } > > -static const char *find_trigger_stream(struct userdata *u, pa_sink *s, > pa_sink_input *ignore, struct group *g) { > - pa_sink_input *j; > - uint32_t idx; > - const char *trigger_role; > +/* Return true if group state changes between > + * active and inactive. */ > +static bool update_group_active(struct userdata *u, struct group *g) { > + void *state = NULL; > + bool new_active = false; > + pa_sink_input *sink_input = NULL; > + void *value; > > 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))) { > + pa_assert(g); > > - if (j == ignore) > - continue; > + if (pa_hashmap_size(g->trigger_state) > 0) { > + PA_HASHMAP_FOREACH_KV(sink_input, value, g->trigger_state, state) { > + if (!sink_input->muted && > + sink_input->state != PA_SINK_INPUT_CORKED) { > + new_active = true; > + break; > + } > + } > + } The pa_hashmap_size() check doesn't seem to do anything useful. If the hashmap is empty, the iteration loop won't run anyway. > > - trigger_role = get_trigger_role(u, j, g); > - if (trigger_role && !j->muted && j->state != PA_SINK_INPUT_CORKED) > - return trigger_role; > + if (new_active != g->active) { > + pa_log_debug("Group '%s' is now %sactive.", g->name, new_active ? "" > : "in"); > + g->active = new_active; > + return true; > } > > - return NULL; > + return false; > } > > -static const char *find_global_trigger_stream(struct userdata *u, pa_sink > *s, pa_sink_input *ignore, struct group *g) { > - const char *trigger_role = NULL; > +/* Identify trigger streams and add or remove the streams from > + * state hashmap. Proplist change when changing media.role may > + * result in already existing stream to gain or lose trigger role > + * status. Returns true if the handled sink-input should be ignored > + * in interaction applying phase. */ > +static bool update_trigger_streams(struct userdata *u, struct group *g, > pa_sink_input *sink_input, > + bool put, bool unlink, bool proplist) { > + const char *role = NULL; > + bool proplist_changed = false; I think renaming "proplist" to "proplist_changed" and "proplist_changed" to "trigger_status_changed" would improve the readability. > + bool can_ignore = false; > > pa_assert(u); > + pa_assert(g); > + pa_assert(sink_input); > + > + if (proplist) { > + bool in_trigger_state; > + bool in_trigger_roles; > + role = sink_input_role(sink_input); > + > + in_trigger_state = !!pa_hashmap_get(g->trigger_state, sink_input); > + in_trigger_roles = !!pa_hashmap_get(g->trigger_roles, role); > + > + /* If the sink-input is already both pointer in trigger_state hashmap > + * and role in trigger_roles, or neither, proplist value important to > + * us (media.role) didn't change, so no need to update anything. */ > + proplist_changed = ((in_trigger_state && in_trigger_roles) || > + (!in_trigger_state && !in_trigger_roles)); > + } > > - if (u->global) { > - uint32_t idx; > - PA_IDXSET_FOREACH(s, u->core->sinks, idx) > - if ((trigger_role = find_trigger_stream(u, s, ignore, g))) > - break; > - } else > - trigger_role = find_trigger_stream(u, s, ignore, g); > + if (proplist_changed || unlink) { > + if (pa_hashmap_remove(g->trigger_state, sink_input)) { > + can_ignore = true; > + pa_log_debug("Stream with role '%s' removed from group '%s'", > sink_input_role(sink_input), g->name); You could set the role variable unconditionally in the beginning of the function, then sink_input_role() wouldn't have to be called multiple times. I suggest adding "triggers" to the end of the log message to make it clear that a trigger stream was removed, not a target stream. That is: "Stream with role '%s' removed from group '%s' triggers." > + } > + } > + > + if (proplist_changed || put) { > + if (!role) > + role = sink_input_role(sink_input); > > - return trigger_role; > + if (pa_hashmap_get(g->trigger_roles, role)) { > + pa_hashmap_put(g->trigger_state, sink_input, PA_INT_TO_PTR(1)); > + can_ignore = true; > + pa_log_debug("Stream with role '%s' added to group '%s'", role, > g->name); > + } > + } > + > + /* If proplist changed we need to include this stream into consideration > + * when applying interaction roles, as the sink-input may have gained or > + * lost trigger or interaction role. */ > + if (proplist_changed) > + can_ignore = false; I have trouble fully understanding what streams should be ignored and why. When something happens, the interactions are updated for each group. A trigger stream can't be a target stream at the same time (within the group, that is - being a trigger in one group and target in another is probably possible), so ignoring makes sense for those, and indeed, you set can_ignore to true only for new and removed trigger streams. However, if a stream changes its role from a target to a trigger, then its interaction need to be updated. Therefore, not all trigger streams are ignored. Since not all trigger streams are ignored, what's the point of having the complexity of ignoring any streams? Why not just run the update process for all streams? > + > + return can_ignore; > } > > -static void cork_or_duck(struct userdata *u, pa_sink_input *i, const char > *interaction_role, const char *trigger_role, bool interaction_applied, > struct group *g) { > +static void cork_or_duck(struct userdata *u, struct group *g, > + pa_sink_input *sink_input, const char > *interaction_role) { > > - if (u->duck && !interaction_applied) { > + if (u->duck) { > pa_cvolume vol; > - vol.channels = 1; > - vol.values[0] = g->volume; > + pa_cvolume_set(&vol, 1, g->volume); > > - pa_log_debug("Found a '%s' stream of '%s' that ducks a '%s' > stream.", trigger_role, g->name, interaction_role); > - pa_sink_input_add_volume_factor(i, g->name, &vol); > + pa_log_debug("Group '%s' ducks a '%s' stream.", g->name, > interaction_role); > + pa_sink_input_add_volume_factor(sink_input, g->name, &vol); > > - } else if (!u->duck) { > - pa_log_debug("Found a '%s' stream that corks/mutes a '%s' stream.", > trigger_role, interaction_role); > - pa_sink_input_set_mute(i, true, false); > - pa_sink_input_send_event(i, PA_STREAM_EVENT_REQUEST_CORK, NULL); > + } else { > + pa_log_debug("Group '%s' corks/mutes a '%s' stream.", g->name, > interaction_role); > + if (!sink_input->muted) > + pa_sink_input_set_mute(sink_input, true, false); > + if (sink_input->state != PA_SINK_INPUT_CORKED) > + pa_sink_input_send_event(sink_input, > PA_STREAM_EVENT_REQUEST_CORK, NULL); > } > } > > -static void uncork_or_unduck(struct userdata *u, pa_sink_input *i, const > char *interaction_role, bool corked, struct group *g) { > +static void uncork_or_unduck(struct userdata *u, struct group *g, > + pa_sink_input *sink_input, const char > *interaction_role) { > > if (u->duck) { > - pa_log_debug("In '%s', found a '%s' stream that should be unducked", > g->name, interaction_role); > - pa_sink_input_remove_volume_factor(i, g->name); > - } > - else if (corked || i->muted) { > - pa_log_debug("Found a '%s' stream that should be uncorked/unmuted.", > interaction_role); > - if (i->muted) > - pa_sink_input_set_mute(i, false, false); > - if (corked) > - pa_sink_input_send_event(i, PA_STREAM_EVENT_REQUEST_UNCORK, NULL); > + pa_log_debug("In '%s', found a '%s' stream that should be unducked", > g->name, interaction_role); > + pa_sink_input_remove_volume_factor(sink_input, g->name); > + } else { > + pa_log_debug("In '%s' found a '%s' stream that should be > uncorked/unmuted.", g->name, interaction_role); > + if (sink_input->muted) > + pa_sink_input_set_mute(sink_input, false, false); > + if (sink_input->state == PA_SINK_INPUT_CORKED) > + pa_sink_input_send_event(sink_input, > PA_STREAM_EVENT_REQUEST_UNCORK, NULL); > } > } > > -static inline void apply_interaction_to_sink(struct userdata *u, pa_sink *s, > const char *new_trigger, pa_sink_input *ignore, bool new_stream, struct group > *g) { > - pa_sink_input *j; > - uint32_t idx, role_idx; > - const char *interaction_role; > +static void apply_interaction_to_sink_input(struct userdata *u, struct group > *g, > + pa_sink_input *sink_input, bool > unlink) { > bool trigger = false; > + bool interaction_applied; > + const char *role = STREAM_INTERACTION_ANY_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))) { > - bool corked, interaction_applied; > - const char *role; > > - if (j == ignore) > - continue; > - > - if (!(role = pa_proplist_gets(j->proplist, PA_PROP_MEDIA_ROLE))) > - role = STREAM_INTERACTION_NO_ROLE; > - > - PA_IDXSET_FOREACH(interaction_role, g->interaction_roles, role_idx) { > - if ((trigger = pa_streq(role, interaction_role))) > - break; > - if ((trigger = (pa_streq(interaction_role, > STREAM_INTERACTION_ANY_ROLE) && > - !get_trigger_role(u, j, g)))) > - break; > - } > - if (!trigger) > - continue; > - > - /* Some applications start their streams corked, so the stream is > uncorked by */ > - /* the application only after sink_input_put() was called. If a new > stream turns */ > - /* up, act as if it was not corked. In the case of module-role-cork > this will */ > - /* only mute the stream because corking is reverted later by the > application */ > - corked = (j->state == PA_SINK_INPUT_CORKED); > - if (new_stream && corked) > - corked = false; > - interaction_applied = !!pa_hashmap_get(g->interaction_state, j); > + if (pa_hashmap_get(g->trigger_state, sink_input)) > + return; This means "don't do anything on trigger streams", right? What if the stream was previously a target stream and changed its role to a trigger, shouldn't it be uncorked/unducked? > > - if (new_trigger && ((!corked && !j->muted) || u->duck)) { > - if (!interaction_applied) > - pa_hashmap_put(g->interaction_state, j, PA_INT_TO_PTR(1)); > + if (!g->any_role) { > + role = sink_input_role(sink_input); > + trigger = !!pa_hashmap_get(g->interaction_roles, role); > + } > > - cork_or_duck(u, j, role, new_trigger, interaction_applied, g); > + if (!g->any_role && !trigger) > + return; I think this would be easier to read: if (g->any_role) trigger = true; else { role = sink_input_role(sink_input); trigger = !!pa_hashmap_get(g->interaction_roles, role); } if (!trigger) return; -- Tanu https://www.patreon.com/tanuk https://liberapay.com/tanuk _______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss