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

[Georg Chini]

On 07.05.2017 13:44, Tanu Kaskinen wrote:
> 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.

You are right, there has been a discussion about filter types a while
ago. So the kind of patch I was thinking of would have to wait until
we can identify the type of a filter (device based/stream based/
stream-group based), if that will ever be the case.

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

Why will it become obsolete? Is there anything else that will move
streams when a new device turns up?

>
>>>> 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.
>
Well, your patch prevents a move that should not happen, while my
patch ensures that it is not tried at all in the first place. Maybe we
should apply both? They share your is_filter_*_moving() functions
anyway.