[pulseaudio-discuss] [PATCH] switch-on-connect: Don't move a stream already moving

[Tanu Kaskinen]

On Thu, 2012-07-05 at 15:28 +0200, Dalleau, Frederic wrote:
> Tanu,
> 
> On Wed, Jul 4, 2012 at 10:08 PM, Dalleau, Frederic
> <frederic.dalleau at intel.com> wrote:
> > Hi Tanu,
> >
> >> Can you explain how o->source can be NULL? It can't be just because it's
> >> "already in the process of being moved". The only places that I found
> >> where pa_source_output.source is set to NULL are
> >> pa_source_output_start_move() and pa_source_output_unlink(). Of those,
> >> pa_source_output_start_move() is the interesting one, but prior to
> >> setting o->source to NULL, the source output is removed from the
> >> source's list of outputs, so PA_IDXSET_FOREACH(o, def->outputs, idx)
> >> shouldn't touch any outputs that are being moved.
> >
> > This comment makes sense. I'll reproduce the issue to clarify what's missing,
> > I come back to you ASAP.
> >
> > Frédéric
> 
> Hi Tanu,
> 
> I have some news, I haven't reproduce the exact crash yet, but a similar one.
> 
> # First, how to reproduce : ~/.pulse/default.pa contains only the
> following (note that I have an alsa card)
> load-module module-udev-detect # or load-module module-alsa-card  device_id=0
> load-module module-native-protocol-unix
> load-module module-bluetooth-discover
> load-module module-switch-on-connect
> 
> # Run the following commands, replace the address as needed
> test-audio -f AudioSink connect 00:1E:DE:21:7E:77
> test-audio -f Headset connect 00:1E:DE:21:7E:77
> pacmd set-card-profile bluez_card.00_1E_DE_21_7E_77 a2dp
> pacmd load-module module-loopback sink=bluez_sink.00_1E_DE_21_7E_77
> # loopback should be running from alsa to a2dp.
> 
> # Now run :
> pacmd set-card-profile bluez_card.00_1E_DE_21_7E_77 hsp
> 
> # And you get this :
> #0  0xb7e967f4 in source_output_may_move_to_cb (o=0x80d95e0,
> dest=0x80f55c8) at modules/module-loopback.c:366
> #1  0xb7c7fa55 in pa_source_output_may_move_to (o=0x80d95e0,
> dest=0x80f55c8) at pulsecore/source-output.c:1199
> #2  0xb7c80f69 in pa_source_output_move_to (o=0x80d95e0,
> dest=0x80f55c8, save=false) at pulsecore/source-output.c:1498
> #3  0xb7e761ca in source_put_hook_callback (c=0x80831f8,
> source=0x80f55c8, userdata=0x809f4e8) at
> modules/module-switch-on-connect.c:146
> #4  0xb7c3edd1 in pa_hook_fire (hook=0x8083400, data=0x80f55c8) at
> pulsecore/hook-list.c:106
> #5  0xb7c84dc2 in pa_source_put (s=0x80f55c8) at pulsecore/source.c:601
> #6  0xb7ce562e in start_thread (u=0x80925c8) at
> modules/bluetooth/module-bluetooth-device.c:2635
> #7  0xb7ce5dd8 in card_set_profile (c=0x808e8e0,
> new_profile=0x80cb610) at
> modules/bluetooth/module-bluetooth-device.c:2735
> #8  0xb7c384e1 in pa_card_set_profile (c=0x808e8e0, name=0x809bf30
> "hsp", save=true) at pulsecore/card.c:246
> 
> In pa_card_set_profile, o->source is put to NULL between
> pa_source_move_all_start and pa_sink_move_all_finish.
> When switching from a2dp to hsp profile, start_thread creates the new
> sink, then the new source.
> Since a new source is created, module-switch-on-connect tries to move
> the loopback source-output from the default source (alsa), to the
> newly created source.
> To do that, it asks the loopback if the move is possible.
> module-loopback would like to checks that the destination is not the
> monitor_source of the loopback sink.

Right, so a stream move has been started by A, and B would like to move
the same stream. This could happen with any two routing policy modules.
Is it a bug if two routing policy modules conflict with each other? I'd
guess no, because it's pretty hard to automatically detect whether two
routing policies conflict. It's up to the user to configure a consistent
set of modules.

In any case, I think pa_source_output_may_move_to() should fail if the
source output is already being moved. Additionally, a warning should be
logged, because the situation should only happen if the user has made a
configuration mistake, or if the code is buggy like in this case
(module-bluetooth-device shouldn't be moving anything). The problem with
that is that pa_source_output_may_move_to() is used also internally by
pa_source_output_finish_move() in a situation where the source output is
already being moved (and there may be also other code that uses
pa_source_output_may_move_to() when a move has already been started, I
haven't checked this). I think there should be a separate
pa_source_output_may_attach_to() function that is used instead of
pa_source_output_may_move_to() when the move is already in process.

-- 
Tanu