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! -- Tanu https://liberapay.com/tanuk https://www.patreon.com/tanuk
