[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.
>
> 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?
>
>> 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?
>
Hi,

while testing your patch, I found an example where the streams get killed.

If I do the following:
1) unload module-stream-restore
2) load module switch-on-connect
3) add some streams to the default sink
4) load module-echo-cancel so that the streams are moved to it
5) unload module-echo-cancel -> streams get killed

Not sure where exactly that happens, but it looks like during unload of
the echo-cancel module the streams are moved away at a point in time
when the master sink is already invalid. Your patch denies the move, so
the streams get killed when the echo-cancel sink vanishes.

I'll send a new version of my patch which sets the default sink/source to
NULL during the move (and also fixes a bug in module-switch-on-connect
which assumes that default_sink/source is always set).