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

[Georg Chini]

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:
>>>> On Sat, 2017-05-06 at 20:06 +0200, Georg Chini wrote:
>>>>> On 06.05.2017 18:36, Tanu Kaskinen wrote:
>>>>>> On Fri, 2017-05-05 at 16:21 +0200, Georg Chini wrote:
>>>>>>> I agree with you that my patch is wrong, but I think there is a
>>>>>>> simpler
>>>>>>> solution than your patch. The reason why the move is triggered at all
>>>>>>> is because the virtual sink becomes the default sink when no other
>>>>>>> sink is available. Then a null-sink turns up and the streams are
>>>>>>> moved
>>>>>>> away from the default sink. This does not make sense anyway for a
>>>>>>> virtual sink, we don't want the streams to move away from there.
>>>>>>> So the simple fix is to prevent the virtual sink from being the
>>>>>>> default
>>>>>>> sink when it is not connected to a master, then the move will not
>>>>>>> occur.
>>>>>>> If you are OK with that approach, I would send a patch.
>>>>>> By "the virtual sink", do you mean the null sink? Would you want it to
>>>>>> be impossible to make any null sink the default sink? That doesn't
>>>>>> seem
>>>>>> like a good idea, so do you propose some more nuanced policy?
>>>>> No, by virtual sink I mean the filter sink. I think you did not get
>>>>> what I
>>>>> wanted to say. When there is no other sink, the filter sink becomes
>>>>> the default before the null sink turns up. Then streams are moved
>>>>> from the moving filter sink to the new null sink. If we prevent the
>>>>> filter sink from becoming the default while its sink input is moving,
>>>>> the problem will not happen in the first place.
>>>>> I'll send a patch, maybe you understand better what I mean then.
>>>> 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.

>
>> 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?