On 20 December 2015 at 15:48, Tanu Kaskinen <tanuk at iki.fi> wrote: > This fixes a crash that was observed in the following scenario: > > A phone applications creates playback and recording streams with > filter.want=echo-cancel. An echo-cancel sink gets created. The alsa > card profile has enabled a 4.0 sink, and the user changes the profile > to have a stereo sink instead. A complex sequence of events happens: > the echo-cancel sink input gets detached from the alsa sink, the 4.0 > sink gets removed, which triggers loading of a null sink by > module-always-sink, and that in turn triggers rerouting in > module-device-manager, which decides to move the phone stream to the > null sink. Moving a sink input involves sending a message to the IO > thread of the old sink, but in this case the old sink is the > echo-cancel sink, which was detached from the now-dead alsa sink. > Therefore, the echo-cancel sink doesn't have an IO thread associated > with it, and as can be expected, sending a message to a non-existing > thread results in a crash. > > The crash can be avoided by disallowing moving streams that are > connected to filter devices that themselves are being moved. The > profile switch still doesn't work smoothly, though, because the > echo-cancel streams don't support moving, so when the old alsa sink > goes away, module-echo-cancel gets unloaded, and since the > echo-cancel sink input got detached before that, the phone sink input > isn't movable either and gets killed. But that's a separate issue. > > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=93443 > --- > src/pulsecore/sink-input.c | 26 ++++++++++++++++++++++++++ > src/pulsecore/source-output.c | 27 +++++++++++++++++++++++++++ > 2 files changed, 53 insertions(+) > > diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c > index 539ae17..0e01893 100644 > --- a/src/pulsecore/sink-input.c > +++ b/src/pulsecore/sink-input.c > @@ -1528,6 +1528,22 @@ static bool find_filter_sink_input(pa_sink_input *target, pa_sink *s) { > return false; > } > > +static bool is_filter_sink_moving(pa_sink_input *i) { > + pa_sink *sink = i->sink; > + > + if (!sink) > + return false; > + > + while (sink->input_to_master) { > + sink = sink->input_to_master->sink; > + > + if (!sink) > + return true; > + } > + > + return false; > +} > + > /* Called from main context */ > bool pa_sink_input_may_move_to(pa_sink_input *i, pa_sink *dest) { > pa_sink_input_assert_ref(i); > @@ -1547,6 +1563,16 @@ bool pa_sink_input_may_move_to(pa_sink_input *i, pa_sink *dest) { > return false; > } > > + /* If this sink input is connected to a filter sink that itself is moving, > + * then don't allow the move. Moving requires sending a message to the IO > + * thread of the old sink, and if the old sink is a filter sink that is > + * moving, there's no IO thread associated to the old sink. */ > + if (is_filter_sink_moving(i)) { > + pa_log_debug("Can't move input from filter sink %s, because the filter sink itself is currently moving.", > + i->sink->name); > + return false; > + } > + > if (pa_idxset_size(dest->inputs) >= PA_MAX_INPUTS_PER_SINK) { > pa_log_warn("Failed to move sink input: too many inputs per sink."); > return false; > diff --git a/src/pulsecore/source-output.c b/src/pulsecore/source-output.c > index 9000972..24e9df4 100644 > --- a/src/pulsecore/source-output.c > +++ b/src/pulsecore/source-output.c > @@ -1184,6 +1184,22 @@ static bool find_filter_source_output(pa_source_output *target, pa_source *s) { > return false; > } > > +static bool is_filter_source_moving(pa_source_output *o) { > + pa_source *source = o->source; > + > + if (!source) > + return false; > + > + while (source->output_from_master) { > + source = source->output_from_master->source; > + > + if (!source) > + return true; > + } > + > + return false; > +} > + > /* Called from main context */ > bool pa_source_output_may_move_to(pa_source_output *o, pa_source *dest) { > pa_source_output_assert_ref(o); > @@ -1202,6 +1218,17 @@ bool pa_source_output_may_move_to(pa_source_output *o, pa_source *dest) { > return false; > } > > + /* If this source output is connected to a filter source that itself is > + * moving, then don't allow the move. Moving requires sending a message to > + * the IO thread of the old source, and if the old source is a filter > + * source that is moving, there's no IO thread associated to the old > + * source. */ > + if (is_filter_source_moving(o)) { > + pa_log_debug("Can't move output from filter source %s, because the filter source itself is currently moving.", > + o->source->name); > + return false; > + } > + > if (pa_idxset_size(dest->outputs) >= PA_MAX_OUTPUTS_PER_SOURCE) { > pa_log_warn("Failed to move source output: too many outputs per source."); > return false; > -- I don't think we should push this out -- it effectively makes using filters unsupported, imo. We've hit a similar problem in the past, which I applied a band-aid around as well [1]. I'm going to try to dig in a bit more and find some alternatives. Cheers, Arun [1] https://bugs.freedesktop.org/show_bug.cgi?id=90416
