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

[Arun Raghavan]

On Fri, 2016-05-06 at 15:40 +0300, Tanu Kaskinen wrote:
> 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?

That comment is stale, from when I wanted to just disallow all moves
within the filter hierarchy. As you pointed out, in the case of
manually loaded filters, it is possible that relative priorities of the
sink and its filter are managed by the user.

Will just drop that.

> > 
> > @@ -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.)

Ah, yes. I'll fix this.

> > 
> > @@ -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.

Fair enough.

> > 
> > @@ -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.)

Out-of-date in what sense? We do want to continue the same behaviour of
only managing the property on the stream if we actually loaded a filter
for it.

-- Arun