[pulseaudio-discuss] [PATCH 3/7] device-manager, filter-apply: don't reroute streams that have a filter

[Tanu Kaskinen]

On Mon, 2016-04-25 at 15:47 +0530, Arun Raghavan wrote:
> On Mon, 2016-04-25 at 13:13 +0300, Tanu Kaskinen wrote:
> > On Mon, 2016-04-25 at 15:37 +0530, Arun Raghavan wrote:
> > > On Mon, 2016-04-25 at 12:36 +0300, Tanu Kaskinen wrote:
> > > > On Mon, 2016-04-25 at 15:02 +0530, Arun Raghavan wrote:
> > > > > On Tue, 2016-03-22 at 15:54 +0200, Tanu Kaskinen wrote:
> > > > > > On Tue, 2016-03-22 at 15:41 +0200, Tanu Kaskinen wrote:
> > > > > > > device-manager reroutes all streams whenever a new device appears.
> > > > > > > When filter-apply has loaded a filter for some stream, the filter
> > > > > > > device may not be what device-manager considers the best device for
> > > > > > > the stream, which means that when an unrelated device appears,
> > > > > > > device-manager may break the filtering that filter-apply had set up.
> > > > > > > 
> > > > > > > This patch changes filter-apply so that it saves the filter device
> > > > > > > name to the stream proplist when it sets up a filter. device-manager
> > > > > > > can then check the proplist when it does rerouting, and skip the
> > > > > > > rerouting for streams that have a filter applied to them.
> > > > > > > 
> > > > > > > The proplist isn't cleaned up when the stream moves away from the
> > > > > > > filter device, so before doing any decisions based on the
> > > > > > > filter_device property, it should be checked that the stream is
> > > > > > > currently routed to the filter device. It seemed simpler to do it this
> > > > > > > way compared to setting up stream move monitoring in filter-apply and
> > > > > > > removing the property when the stream moves away from the filter
> > > > > > > device.
> > > > > 
> > > > > My preference is to manage an ignore property that module-device-
> > > > > manager can check, and have that managed within module-filter-apply. In
> > > > > that way, all the behaviour related to module-filter-apply is
> > > > > effectively encapsulated in that module, rather than leaking into
> > > > > module-device-manager.
> > > > > 
> > > > > Thoughts?
> > > >  
> > > > I don't quite understand what you mean. With this patch, module-device-
> > > > manager doesn't have other logic than checking a property and ignoring
> > > > the stream if the property is set (and up to date). The property is
> > > > managed by module-filter-apply. Your proposal sounds exactly the same.
> > >  
> > > The difference is in where the policy lies. Your change adds policy
> > > regarding module-filter-apply to module-device-manager.
> > > 
> > > My suggestion adds mechanism in module-device-manager to skip streams,
> > > and then the policy on how filtered streams are dealt with lies only in
> > > module-filter-apply.
> > 
> > Ok, so the property would be named "module-device-manager.ignore" or
> > "m-d-m.dont_move" or something like that.
> > 
> > Still, module-device-manager is a routing policy module. I don't see
> > anything wrong with adding policy rules there. This is about two
> > routing modules collaborating, I don't see what difference it makes
> > which one implements the mechanism and which uses the mechanism
> > provided by the other module.
> 
> It just makes it harder to read and understand the code in the future,
> so I'd like to avoid too much policy that spans files.

I still don't see what difference it makes which module implements the
mechanism and which implements the policy. In either case the policy is
in only one file, but in order to implement the policy, some new code
is needed in another file.

> I'm okay to pick this patche set as-is and redo this bit the way I
> suggested myself, though.

Ok, feel free to do that. I'll add the comment that I mentioned in my
first reply to myself, and then push the patches. (For the record, I
got ack in irc from Arun for the other patches.)

-- 
Tanu