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

Fri Feb 9 08:08:01 UTC 2018

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