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

[Georg Chini]

On 09.02.2018 09:08, Tanu Kaskinen wrote:
> On Thu, 2018-02-08 at 18:40 +0100, Georg Chini wrote:
>> 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.
> Ok, I now agree with your original patch. Thanks for working on this!
>
Thanks, pushed it now.