[pulseaudio-discuss] [PATCH] sink-input, source-output: don't allow moving of streams connected to moving filter devices

[Arun Raghavan]

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