On 03.07.2017 17:51, Luiz Augusto von Dentz wrote:
Hi Tanu,
On Mon, Jul 3, 2017 at 5:05 PM, Tanu Kaskinen <ta...@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 <ta...@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/
With this patch, the transport state is also set correctly when the
acquire method is used.
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
