[pulseaudio-discuss] [PATCH] bluetooth: Poll BT SCO socket for checking remote disconnect.

[Tanu Kaskinen]

On Fri, 2011-12-16 at 14:06 +0200, Tanu Kaskinen wrote:
> From: Marko Ollonen <marko.ollonen at digia.com>
> 
> Additional comments from Tanu Kaskinen:
> 
> There's this comment in start_thread() for the SCO over PCM
> case: "FIXME: monitor stream_fd error". I guess this patch
> solves that FIXME item at least partially.
> 
> The "u->sink->state == PA_SINK_RUNNING" condition looks a
> bit strange to me - why is this only checked when the sink
> is RUNNING, why not also when IDLE?
> 
> I think it would be better to poll stream_fd in the IO
> thread (which is currently not started for the SCO over PCM
> case) so that the stream disconnection could be handled
> immediately when it happens. I'm not willing to do that
> work now, however.
> 
> ---
> 
> This is an old patch that has been made for Harmattan. I'd
> like to get this patch to upstream, since it (probably)
> does something useful and very likely doesn't break
> anything, but if this is not considered the right way to
> fix the problem, I can also understand if the patch gets
> rejected.

No comments received, but nevertheless, I'm not going to push this
patch. I've learned that this can actually break things: let's say
pulseaudio is running on a phone, and the user is having a call using a
hands-free bluetooth device. One of the features of the HFP profile is
that the user can transfer the audio from the hands-free device to the
phone. So the user does that. This causes bluetoothd to close the audio
stream between bluetoothd and Pulseaudio, and some policy thing moves
the phone streams from the SCO sink and source to somewhere else. As a
result, the sink and source become idle, and these state changes trigger
calls to this sco_over_pcm_state_update() function.

In the described scenario, this patch may or may not notice that the
stream fd has been closed by bluetoothd, and will release the transport.
Whether that will happen, depends on which became idle first: the sink
or the source. If the sink becomes idle first, the "(u->sink->state ==
PA_SINK_RUNNING) && (u->stream_fd >= 0)" condition will evaluate to
false, and the transport will not get released and the u->stream_fd will
remain non-negative. But if the source becomes idle first, then the sink
will still be in the running state when this function is called for the
first time. So the transport gets released and u->stream_fd becomes -1.

Later in the function, it is checked whether u->stream_fd is negative.
If it is, the bluetooth stream gets reinitialized, which isn't quite
what we want in this scenario! At least on Harmattan, the policy thing
will interpret this reinitialization so that the audio should get routed
to the bluetooth sink and source again, so the user's attempt to move
the audio from the hands-free device to the phone fails.

This shows that if the stream gets closed by bluetoothd, immediately
reopening the stream is not the right reaction, at least not always.
Maybe sometimes it is - otherwise I don't think this patch would have
been ever created (the original motivation for the patch unfortunately
hasn't been documented).

It's not clear to me how this kind of situation should be handled. What
to do when the user requests moving the audio away from the bluetooth
device and bluetoothd closes the stream? My proposal is to mark the HSP
ports as unavailable when the stream gets closed by bluetoothd. The
expectation is that some policy module will detect that and move the
phone streams elsewhere. If there isn't a suitable policy module loaded,
the HSP sink and source shall just sit there without processing any
audio.

When would the HSP ports become available again? Well, the ports became
unavailable as a result from the user asking the audio to be moved away
from the bluetooth device, so I think it would be logical if the ports
became available when the user asks the audio to be moved back to the
bluetooth device. I'm not familiar with the details, but I'd expect
bluetoothd to send a D-Bus signal when that request is made, and
Pulseaudio could act based on that signal.

-- 
Tanu