[pulseaudio-discuss] [PATCH 2/3] module-filter-apply: Don't implement policy in module-device-manager

[Tanu Kaskinen]

On Fri, 2016-05-06 at 13:43 +0530, arun at accosted.net wrote:
> From: Arun Raghavan <git at arunraghavan.net>
> 
> This adds an ignore mechanism to module-device-manager and uses that
> from within module-filter-apply, rather than having m-d-m have knowledge
> of anything related to m-f-a.
> 
> This does make m-f-a know about m-d-m policy, but it should be possible
> to completely remove this in the future, but having m-d-m be more
> intelligent about dealing with filters.

You probably meant to write "by" instead of the second "but".

Out of curiosity, what would that "more intelligent" mean in practice?
How can you make m-d-m and m-f-a cooperate without one of them knowing
something about the other?

> @@ -664,13 +664,8 @@ static void route_sink_input(struct userdata *u, pa_sink_input *si) {
>      if (!si->sink)
>          return;
>  
> -    /* If module-filter-apply has loaded a filter for the stream, let's not
> -     * break the filtering. The pa_streq() check is needed, because
> -     * module-filter-apply doesn't unset the property when the stream moves
> -     * away from the filter device for whatever reason (this is ugly, but
> -     * easier to do this way than appropriately unsetting the property). */
> -    filter_device = pa_proplist_gets(si->proplist, "module-filter-apply.filter_device");
> -    if (filter_device && pa_streq(filter_device, si->sink->name))
> +    ignore = pa_proplist_gets(si->proplist, "module-device-manager.ignore");
> +    if (ignore && pa_parse_boolean(ignore))

If pa_parse_boolean() fails, I think the code should act as if the
property was not set at all. (Same for the source output code.)

> @@ -65,6 +66,7 @@ struct filter {
>  struct userdata {
>      pa_core *core;
>      pa_hashmap *filters;
> +    pa_hashmap *mdm_ignored_inputs, *mdm_ignored_outputs;

A comment about the key and value types would be nice.

>      bool autoclean;
>      pa_time_event *housekeeping_time_event;
>  };
> @@ -270,17 +272,20 @@ static void trigger_housekeeping(struct userdata *u) {
>      u->housekeeping_time_event = pa_core_rttime_new(u->core, pa_rtclock_now() + HOUSEKEEPING_INTERVAL, housekeeping_time_callback, u);
>  }
>  
> -static int do_move(pa_object *obj, pa_object *parent, bool is_input) {
> +static int do_move(struct userdata *u, pa_object *obj, pa_object *parent, bool is_input) {
> +    /* Keep track of objects that we've marked for module-device-manager to ignore */
> +    pa_hashmap_put(is_input ? u->mdm_ignored_inputs : u->mdm_ignored_outputs, obj, NULL);

I think it's bad style to store NULL values in pa_hashmap, because that
makes it impossible to use pa_hashmap_get(), since it will return NULL
on both success and failure. I suggest using the same value for both
key and value when using a hashmap as a set.

> @@ -520,6 +525,9 @@ static pa_hook_result_t sink_input_move_finish_cb(pa_core *core, pa_sink_input *
>      if (pa_proplist_gets(i->proplist, PA_PROP_FILTER_APPLY_MOVING))
>          return PA_HOOK_OK;
>  
> +    /* If we're managing m-d-m.ignore on this, remove and re-add if we're continuing to manage it */

This comment gets out of date after you rename the property in the next
patch. (Same for the source output code.)

-- 
Tanu