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
