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

[Luiz Augusto von Dentz]

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:
>>>>>>
>>>>>> +    pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path,
>>>>>> "org.ofono.HandsfreeAudioCard", "Acquire"));
>>>>>
>>>>> m is never unreffed.
>>>>
>>>> And it is never done with dbus_connection_send_with_reply_and_block
>>>> since that uses a pending call that takes ownership of the message.
>>>
>>> The documentation doesn't say that the ownership is transferred, and I
>>> also had a look at the libdbus code, and it looks to me that indeed no
>>> such transfer occurs.
>>
>> Nevermind, it seems my memory is failing me.
>>
>>>>>> +    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/

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.

> With this patch, the transport state is also set correctly when the
> acquire method is used.
>
>
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss



-- 
Luiz Augusto von Dentz