[pulseaudio-discuss] [PATCH] filter-apply: Ignore monitor source of filter in find_paired_master()

[Georg Chini]

On 08.02.2018 17:09, Tanu Kaskinen wrote:
> On Wed, 2018-02-07 at 15:28 +0100, Georg Chini wrote:
>> On 07.02.2018 13:21, Tanu Kaskinen wrote:
>>> On Tue, 2018-02-06 at 20:26 +0100, Georg Chini wrote:
>>>> When module-filter-apply tries to find a matching source-output for
>>>> a given sink-input and a stream with the same role
>>> Shouldn't this be "with the same group", not "with the same role"?
>> Yes, you are right. I was thinking of the special case where
>> the streams specify media.role. In that case, streams that
>> have the same role are in the same group.
>>
>>>> exists on the
>>>> monitor source of the filter, module-filter apply falsely assumes
>>>> that the source belongs to another instance of the filter and tries
>>>> to access source->output_from_master->source, which leads to a
>>>> segmentation fault.
>>>>
>>>> This patch fixes the issue by ignoring the stream if the source is
>>>> the monitor source of the filter.
>>> Is ignoring the stream really the right solution? Previously any source
>>> output in the same group as the sink input would be accepted, no matter
>>> how it was routed. If the source output was already routed to the
>>> correct filter, then we'd pick the master source through
>>> "output_from_master", and if the source output was not correctly routed
>>> yet, we'd pick the current source as the master.
>>>
>>> In this case the source output is wrongly determined as "already
>>> correctly routed", so shouldn't we fix the issue by setting the current
>>> source as the master source, like in all other cases where the source
>>> output is not yet correctly routed? No, because we can't make the
>>> filter source use the filter sink's monitor as the master source. It
>>> seems that the correct solution would be to find the filter source that
>>> corresponds to the filter sink, and then use "output_from_master" to
>>> set the master source. In case you don't follow, I'll put some code
>>> below:
>>>
>>>> Bug link: https://bugs.freedesktop.org/show_bug.cgi?id=104958
>>>> ---
>>>>    src/modules/module-filter-apply.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/src/modules/module-filter-apply.c b/src/modules/module-filter-apply.c
>>>> index 783d85ed..163d52a2 100644
>>>> --- a/src/modules/module-filter-apply.c
>>>> +++ b/src/modules/module-filter-apply.c
>>>> @@ -259,6 +259,12 @@ static bool find_paired_master(struct userdata *u, struct filter *filter, pa_obj
>>>>    
>>>>                    if (pa_streq(g, group)) {
>>>>                        if (pa_streq(module_name, so->source->module->name)) {
>>>> +                        /* Make sure we are not routing to the monitor source
>>>> +                         * of the same filter */
>>>> +                        if (so->source->monitor_of) {
>>>> +                            pa_xfree(g);
>>>> +                            continue;
>>>> +                        }
>>> So I'd do this instead:
>>>
>>> /* The source output is currently routed to the monitor
>>>    * source of the filter. We can't use the monitor source
>>>    * of the same filter as the master source. Let's find
>>>    * the real source of the filter, and then use its master
>>>    * source. */
>>> if (so->source->monitor_of) {
>>>       pa_source *source;
>>>       PA_IDXSET_FOREACH(source, u->core->sources, idx) {
>>>           if (source->module == so->source->monitor_of->module && source->output_from_master) {
>>>               filter->source_master = source->output_from_master->source;
>>>               break;
>>>           }
>>>       }
>>> }
>>>
>> I was looking for some way to figure out what the right source might be,
>> but I do not think the information is sufficient. Consider you have a
>> loopback
>> running from the monitor source to some other sink and have specified
>> media.role=phone for the loopback. Then the stream is completely unrelated
>> to what you are trying to filter. Also, specifying the monitor source of a
>> filter as the master source does not make a lot of sense from the user
>> perspective, so I concluded that in this case there must be another reason
>> why the monitor stream is in the same group and therefore decided to ignore
>> it. Another possibility would be to return false if the monitor source is in
>> the same group, but that seems too restrictive.
>>
>> I am however not sure what is the correct way to solve it, so if you still
>> believe that your way is best, I can change the patch accordingly.
> The loopback stream in your example shouldn't be in any filter group,
> so it shouldn't cause any confusion when resolving the master source.
> Is this loopback example something that was actually observed in the
> bug?
>
No, it wasn't. In that case the cause was an application (zoiper) which
seems to be testing different combinations of source and sink if I read
the log right. So the bug report falls into the category "user error"
because the application is trying a combination of source/sink which
does not make sense.

In fact I read the code wrong - my loopback example does not work because
the loopback would also need to have the filter.apply property set to the
same filter in addition to having the same role. So forget the example.

Anyway, I decided to ignore the stream because I could only see two
possible reasons why the monitor stream can be in the same group:

1) User error: Then, when the stream is ignored, find_paired_master()
will return false because it does not find a matching stream. In my
opinion this is OK because the user specified a sink/source combination
which cannot work (and we cannot really figure out, which source the
user actually wanted).

2) A weird setup, so that the monitor stream is in the same group more
or less by accident. Then, when the stream is ignored, find_paired_master()
should find the correct stream later.