[pulseaudio-discuss] [PATCH] echo-cancel: Avoid segfaults due to invalid master sink or source

[Tanu Kaskinen]

On Sun, 2017-05-07 at 12:32 +0200, Georg Chini wrote:
> On 07.05.2017 09:33, Tanu Kaskinen wrote:
> > On Sat, 2017-05-06 at 22:38 +0200, Georg Chini wrote:
> > > On 06.05.2017 22:15, Georg Chini wrote:
> > > > On 06.05.2017 22:06, Tanu Kaskinen wrote:
> > > > > I made a strange interpretation, because what you really meant seemed
> > > > > even more crazy. I forgot that module-switch-on-connect only moves
> > > > > streams if they are routed to the default sink, and because of that
> > > > > missing piece your explanation made no sense to me at all. I understand
> > > > > the logic now.
> > > > > 
> > > > > Implementing your proposal doesn't seem that simple: what if the user
> > > > > has set a filter sink as the default sink? Would you change the default
> > > > >    sink just for the duration of the filter sink move?
> > > > > 
> > > > 
> > > > I already implemented it and it is quite simple (using part of your
> > > > patch).
> > > > 
> > > > The default sink will be set to the new sink by switch-on-connect
> > > > anyway (even the configured default sink), so the previous user
> > > > choice is irrelevant. I think however, that the simple logic of
> > > > switch-on-connect should at least be changed so that in the case
> > > > that the previous default sink was a filter then the streams should
> > > > not be moved away from it (unless the new sink also is a filter).
> > > > But this would be a matter of a different patch.
> > 
> > So you agree that the previous default sink choice isn't irrelevant
> > after all, since you propose another patch that changes
> > switch-on-
> > connect logic based on the previous default sink choice.
> 
> The only situation where a filter sink may be moving and may be
> excluded from the default is when there is an alsa profile switch.
> In this situation, my patch does exactly what I propose below,
> because the disappearing sink must have been the master of
> the filter (otherwise the filter would not be moving) and so the
> master will get attached to the new sink by the profile switch.
> In all other situations, the filter sink will not be moving, so you
> still have the correct information about the previous default
> sink.
> 
> And again: The only code path were a move within a move can
> happen at all is during an alsa card profile switch. In all other places,
> move_to() is used, which ensures that start_move() and finish_move()
> are executed without any hook calls in between.
> 
> > Preventing all moves from a filter sink to a non-filter sink in switch-
> > on-connect doesn't make much sense to me. If the current default sink
> > is a filter sink, why shouldn't module-switch-on-connect move streams
> > from it to a newly plugged in usb sink?
> 
> When a user sets the default sink to a filter sink, the streams should be
> filtered. When now a new device turns up, the logical action would be to
> move the master sink of the filter to the new device. Moving all streams
> away from the filter instead makes the filter temporarily unused and the
> user has to move all streams back manually if filtering is intended.

You seem to be thinking that filters are always tied to streams, so the
filters should move when the streams move, but filters can also be tied
to devices. For example, equalizers and remap sinks are usually per-
device things. Such filters should kept in place when a new device is
plugged in.

That said, I don't really care about what module-switch-on-connect
does, because it will be largely obsolete starting from the next
release. It just shouldn't crash.

> > > Another advantage of my patch is that you will not end up with failed moves
> > > that might kill the streams.
> > 
> > Can you give an example of a case where my patch would cause streams
> > getting killed?
> > 
> 
> You are right, this won't happen.
> 
> As already said, I am not completely against using your patch. It just
> seems more invasive than necessary. Maybe Arun should comment
> as well?

I don't understand what's invasive about the patch. It only blocks
moves that would otherwise lead to crashing. Comments from Arun would
of course be welcome.

-- 
Tanu

https://www.patreon.com/tanuk