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.
