[pulseaudio-discuss] [PATCH v3 1/3] bluetooth: ofono: Use Acquire method if available

[Tanu Kaskinen]

On Mon, 2017-07-03 at 21:01 +0300, Luiz Augusto von Dentz wrote:
> Hi Georg,
> 
> On Mon, Jul 3, 2017 at 8:32 PM, Georg Chini <georg at chini.tk> wrote:
> > On 03.07.2017 17:51, Luiz Augusto von Dentz wrote:
> > > 
> > > Hi Tanu,
> > > 
> > > On Mon, Jul 3, 2017 at 5:05 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> > > > 
> > > > On Mon, 2017-07-03 at 16:09 +0300, Luiz Augusto von Dentz wrote:
> > > > > 
> > > > > Hi Tanu,
> > > > > 
> > > > > On Mon, Jun 19, 2017 at 4:29 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> > > > > > 
> > > > > > On Thu, 2017-05-25 at 11:36 +0300, Luiz Augusto von Dentz wrote:
> > > > > > > +    if (!r) {
> > > > > > > +        if (!pa_streq(err.name, DBUS_ERROR_UNKNOWN_METHOD)) {
> > > > > > > +            pa_log_error("Failed to acquire %s: %s", err.name,
> > > > > > > err.message);
> > > > > > > +            return -1;
> > > > > > > +        }
> > > > > > > +    } else if ((dbus_message_get_args(r, NULL,
> > > > > > > +                         DBUS_TYPE_UNIX_FD, &card->fd,
> > > > > > > +                         DBUS_TYPE_BYTE, &card->codec,
> > > > > > 
> > > > > > I don't know how dbus_message_get_args() works, but it seems likely
> > > > > > that it can fail between setting card->fd and card->codec. We don't
> > > > > > want to set card->fd if the operation fails, so I think we should read
> > > > > > the values into local variables, and only update the card if the
> > > > > > operation is successful.
> > > > > 
> > > > > Will add separate variables just to be safe.
> > > > > 
> > > > > > The codec value should be checked (it has to be CVSD).
> > > > > > 
> > > > > > hf_audio_agent_new_connection() sets the transport state to PLAYING,
> > > > > > but that's not done here. This was pointed out by Georg as well. In the
> > > > > > previous discussion, he said that he'll send a patch related to this,
> > > > > > but that hasn't happened...
> > > > > 
> > > > > hf_audio_agent_new_connection is called for the main thread thus why
> > > > > it can change the transport state, on the IO thread this would cause a
> > > > > crash, in fact up to now all backends including A2DP do not set the
> > > > > transport state from acquire callback directly for this very reason
> > > > > which is why I decided to leave it be like this since perhaps we want
> > > > > the caller to directly set the state which might make sense to change
> > > > > in a separate patch.
> > > > 
> > > > Is a separate patch forthcoming? Without such patch it looks like
> > > > you're introducing a regression here.
> > > 
> > > Don't follow your logic, regression to what? _None_ of the the
> > > backends do set the transport state from the io thread, this is not
> > > changing anything either with Connect or Acquire the behavior is
> > > exactly the same in this regard. Im not saying we should not fix it,
> > > but considering the current behavior is to wait for NewConnection
> > > forever that is probably a much worse offender then having out of sync
> > > transport state.
> > > 
> > > A2DP also does a similar thing, though it return the fd in place it
> > > only changes the transport state when D-Bus indicate so:
> > > 
> > >              if (pa_streq(key, "State")) {
> > >                  pa_bluetooth_transport_state_t state;
> > > 
> > >                  if (transport_state_from_string(value, &state) < 0) {
> > >                      pa_log_error("Invalid state received: %s", value);
> > >                      return;
> > >                  }
> > > 
> > >                  pa_bluetooth_transport_set_state(t, state);
> > > 
> > > Unfortunately the Connect + NewConnection is racy, which should be
> > > clear by now, this stayed for as long I remembered the ofono backend
> > > exists, so I fixing things because they are not very reliable,
> > > including ofono which got patched as well.
> > > 
> > > 
> > > 
> > 
> > Hi, I don't understand the discussion. I think we don't want to set the
> > state
> > from the I/O thread. The transport state is set correctly from the main
> > thread in all cases except for the AG role when using the native
> > backend. The patch Tanu is referring to is here:
> > 
> > https://patchwork.freedesktop.org/patch/155661/

Oh, so you did send a patch for this already. Sorry, I don't know how I
missed it.

> Great, that should work regardless of the backend. Btw is there a
> chance the BLUETOOTH_MESSAGE_SET_TRANSPORT_PLAYING set but in the
> meantime we disconnect/hup? It seems to me this is still racy, though
> we can probably ensure setting to playing is valid by checking if
> transport is active, but if the profile changes in the meantime this
> might not work since the transport may have changed.

It looks racy indeed, so some check should be added as you say. When
shutting down or changing the profile, stop_thread() is always called.
stop_thread() calls pa_thread_mq_done(), and this is where queued
messages are processed. The transport hasn't been released yet at this
point, but I think the transport release can be moved to happen before
pa_thread_mq_done(), then you can check if u->transport is NULL.

-- 
Tanu

https://www.patreon.com/tanuk