[pulseaudio-discuss] [PATCH v0] bluetooth: Fix unacquired transports during sink resume

[Mikel Astiz]

Hi Tanu,

On Sun, Dec 2, 2012 at 4:05 AM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Thu, 2012-11-29 at 14:33 +0100, Mikel Astiz wrote:
>> Hi Tanu,
>>
>> On Thu, Nov 29, 2012 at 4:55 AM, Tanu Kaskinen <tanuk at iki.fi> wrote:
>> > On Wed, 2012-11-28 at 19:20 +0100, Mikel Astiz wrote:
>> >> From: Mikel Astiz <mikel.astiz at bmw-carit.de>
>> >>
>> >> The sink can be resumed while the source is still in PA_SOURCE_INIT.
>> >> This is the case if a module such as module-stream-restore routes the
>> >> audio to the sink during pa_sink_put(), leading to an inconsistent
>> >> state: the sink stays RUNNING but the transport is not actually
>> >> acquired.
>> >
>> > Ack from me. I think Arun wants to check this too, so not pushing yet,
>> > but to me the patch makes perfect sense.
>> >
>> > Slightly related comment, maybe this
>> >
>> >     if (u->sink->thread_info.state != PA_SINK_SUSPENDED)
>> >         break;
>> >
>> > just before the changed part should be changed to
>> >
>> >     if (PA_SINK_IS_OPENED(u->sink->thread_info.state))
>> >         break;
>> >
>>
>> I would even argue that the whole condition can be dropped. After all,
>> bt_transport_acquire() should handle nicely the case where the
>> transport was already acquired.
>>
>> Having said this, I would avoid changing this code for the upcoming
>> release, unless there is some associated issue. The reason is that
>> I've been testing this recently and I have the feeling that it's quite
>> tricky to get this right, perhaps because of lack of knowledge from my
>> side.
>
> Ok.
>
>> > so that INIT->IDLE transitions will work too, if we decide some day that
>> > it's not a good idea to start the devices suspended. (I think it makes
>> > sense to start them suspended, but relying on module-suspend-on-idle for
>> > unsuspending is not good.)
>>
>> INIT->IDLE should already be working, since in this case the transport
>> is acquired before the thread is started. I just sent two patches
>> that would do this for headsets, since Arun was
>> also concerned about relying on the presence of
>> module-suspend-on-idle.
>
> Would the acquire in setup_transport() be necessary at all if the
> transport would be acquired in the PA_SINK_MESSAGE_SET_STATE handler
> also for the INIT->IDLE transition?

I believe that the acquire would always be necessary in
setup_transport(), since this is the only way to find out which the
initial state of the sink/sources should be.

What you propose would be possible only if the
PA_SINK_MESSAGE_SET_STATE handler would be allowed to decide the state
being set, something like "PA_SINK_IDLE was requested but I have to
set PA_SINK_SUSPENDED". But I thought this was not possible.

In any case you're right that the existing code could be simplified.

Cheers,
Mikel