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? -- Tanu
