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 <jusa at 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
