On Jul 18, 2013 12:31 PM, "Tanu Kaskinen" <tanu.kaskinen at linux.intel.com>
wrote:
>
> On Fri, 2013-07-12 at 15:06 -0300, jprvita at gmail.com wrote:
> > From: Jo?o Paulo Rechi Vita <jprvita at openbossa.org>
> >
> > Create the pa_bluetooth_transport structure to store information about
> > the bluetooth transport and utility functions to manipulate this
> > structure. The acquire() and release() operations are function pointers
> > in the pa_bluetooth_transport structure to make possible for different
> > transport backends to provide different implementations of these
> > operations. Thre is also a userdata field for the transport backend
> > provide data for the acquire/release functions.
> > ---
> > src/modules/bluetooth/bluez5-util.c | 130
++++++++++++++++++++++++++++++++++++
> > src/modules/bluetooth/bluez5-util.h | 44 ++++++++++++
> > 2 files changed, 174 insertions(+)
> >
> > diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
> > index 87aaa37..6a94d50 100644
> > --- a/src/modules/bluetooth/bluez5-util.c
> > +++ b/src/modules/bluetooth/bluez5-util.c
> > @@ -36,6 +36,7 @@
> > #include "bluez5-util.h"
> >
> > #define BLUEZ_SERVICE "org.bluez"
> > +#define BLUEZ_MEDIA_TRANSPORT_INTERFACE BLUEZ_SERVICE
".MediaTransport1"
> >
> > struct pa_bluetooth_discovery {
> > PA_REFCNT_DECLARE;
> > @@ -45,8 +46,117 @@ struct pa_bluetooth_discovery {
> > bool filter_added;
> > pa_hook hooks[PA_BLUETOOTH_HOOK_MAX];
> > pa_hashmap *devices;
> > + pa_hashmap *transports;
> > };
> >
> > +pa_bluetooth_transport *pa_bluetooth_transport_new(pa_bluetooth_device
*d, const char *owner, const char *path,
> > +
pa_bluetooth_profile_t p, const uint8_t *config, int size,
> > +
pa_bluetooth_transport_acquire_cb acquire,
> > +
pa_bluetooth_transport_release_cb release, void *userdata) {
> > + pa_bluetooth_transport *t;
> > +
> > + t = pa_xnew0(pa_bluetooth_transport, 1);
> > + t->device = d;
> > + t->owner = pa_xstrdup(owner);
> > + t->path = pa_xstrdup(path);
> > + t->profile = p;
> > + t->config_size = size;
> > + t->acquire = acquire;
> > + t->release = release;
> > + t->userdata = userdata;
> > +
> > + if (size > 0) {
> > + t->config = pa_xnew(uint8_t, size);
> > + memcpy(t->config, config, size);
> > + }
> > +
> > + t->state = PA_BLUETOOTH_TRANSPORT_STATE_IDLE;
> > +
> > + pa_assert_se(pa_hashmap_put(d->discovery->transports, t->path, t)
>= 0);
> > +
> > + return t;
> > +}
> > +
> > +void pa_bluetooth_transport_free(pa_bluetooth_transport *t) {
> > + pa_assert(t);
> > +
> > + pa_hashmap_remove(t->device->discovery->transports, t->path);
> > + pa_xfree(t->owner);
> > + pa_xfree(t->path);
> > + pa_xfree(t->config);
> > + pa_xfree(t);
> > +}
> > +
> > +static int bluez5_transport_acquire(pa_bluetooth_transport *t, bool
optional, size_t *imtu, size_t *omtu) {
>
> This is meant to be used as an acquire callback, right? Please put "_cb"
> at the end of the function name. Same for bluez5_transport_release.
>
Ok.
> > + DBusMessage *m, *r;
> > + DBusError err;
> > + int ret;
> > + uint16_t i, o;
> > + const char *method = optional ? "TryAcquire" : "Acquire";
> > +
> > + pa_assert(t);
> > + pa_assert(t->device);
> > + pa_assert(t->device->discovery);
> > +
> > + pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path,
BLUEZ_MEDIA_TRANSPORT_INTERFACE, method));
> > +
> > + dbus_error_init(&err);
> > +
> > + r =
dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t->device->discovery->connection),
m, -1, &err);
> > + if (!r) {
> > + if (optional && pa_streq(err.name,
"org.bluez.Error.NotAvailable"))
> > + pa_log_info("Failed optional acquire of unavailable
transport %s", t->path);
> > + else
> > + pa_log_error("Transport %s() failed for transport %s
(%s)", method, t->path, err.message);
> > +
> > + dbus_error_free(&err);
> > + return -1;
> > + }
> > +
> > + if (!dbus_message_get_args(r, &err, DBUS_TYPE_UNIX_FD, &ret,
DBUS_TYPE_UINT16, &i, DBUS_TYPE_UINT16, &o,
> > + DBUS_TYPE_INVALID)) {
> > + pa_log_error("Failed to parse %s() reply: %s", method,
err.message);
> > + dbus_error_free(&err);
> > + ret = -1;
> > + goto finish;
> > + }
> > +
> > + if (imtu)
> > + *imtu = i;
> > +
> > + if (omtu)
> > + *omtu = o;
> > +
> > +finish:
> > + dbus_message_unref(r);
> > + return ret;
> > +}
> > +
> > +static void bluez5_transport_release(pa_bluetooth_transport *t) {
> > + DBusMessage *m;
> > + DBusError err;
> > +
> > + pa_assert(t);
> > + pa_assert(t->device);
> > + pa_assert(t->device->discovery);
> > +
> > + dbus_error_init(&err);
> > +
> > + if (t->state <= PA_BLUETOOTH_TRANSPORT_STATE_IDLE) {
> > + pa_log_info("Transport %s auto-released by BlueZ or already
released", t->path);
> > + return;
> > + }
> > +
> > + pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path,
BLUEZ_MEDIA_TRANSPORT_INTERFACE, "Release"));
> > +
dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t->device->discovery->connection),
m, -1, &err);
> > +
> > + if (dbus_error_is_set(&err)) {
> > + pa_log_error("Failed to release transport %s: %s", t->path,
err.message);
>
> In an earlier log message there is a mention of auto-releasing by BlueZ.
> I don't know exactly what it means, but it sounds like BlueZ can release
> the transport too, in which case there is a race condition: if BlueZ
> releases a transport at the same time when this code is running, our
> Release() call will be received when the transport is already released,
> which I presume will result in an error. If that is not a real error
> condition, perhaps the log level should be info instead of error.
>
If the transport is auto-released by BlueZ is not an error condition, but
other errors can occur on release, so I think we should keep the log level
here as error. FWIW possible errors on release are "not authorized", if the
process which calls Release() is not the same which acquired it before, and
"in progress" if Release() has already been called by the same process but
not returned yet.
> > + dbus_error_free(&err);
> > + } else
> > + pa_log_info("Transport %s released", t->path);
> > +}
> > +
> > static pa_bluetooth_device*
pa_bluetooth_discovery_create_device(pa_bluetooth_discovery *y, const char
*path) {
> > pa_bluetooth_device *d;
> >
> > @@ -92,8 +202,22 @@ pa_bluetooth_device*
pa_bluetooth_discovery_get_device_by_address(pa_bluetooth_d
> > }
> >
> > static void pa_bluetooth_device_free(pa_bluetooth_device *d) {
> > + unsigned i;
> > +
> > pa_assert(d);
> >
> > + for (i = 0; i < PA_BLUETOOTH_PROFILE_COUNT; i++) {
> > + pa_bluetooth_transport *t;
> > +
> > + if (!(t = d->transports[i]))
> > + continue;
> > +
> > + d->transports[i] = NULL;
> > + t->state = PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED;
> > +
pa_hook_fire(&d->discovery->hooks[PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED],
t);
>
> Can the transport state be DISCONNECTED already before we set it here?
> If it can, please fire the hook only if the state actually changes.
>
> > + pa_bluetooth_transport_free(t);
> > + }
> > +
> > d->discovery = NULL;
> > pa_xfree(d->path);
> > pa_xfree(d->alias);
> > @@ -194,6 +318,7 @@ pa_bluetooth_discovery*
pa_bluetooth_discovery_get(pa_core *c) {
> > PA_REFCNT_INIT(y);
> > y->core = c;
> > y->devices = pa_hashmap_new(pa_idxset_string_hash_func,
pa_idxset_string_compare_func);
> > + y->transports = pa_hashmap_new(pa_idxset_string_hash_func,
pa_idxset_string_compare_func);
> >
> > for (i = 0; i < PA_BLUETOOTH_HOOK_MAX; i++)
> > pa_hook_init(&y->hooks[i], y);
> > @@ -254,6 +379,11 @@ void
pa_bluetooth_discovery_unref(pa_bluetooth_discovery *y) {
> > pa_hashmap_free(y->devices, NULL);
> > }
> >
> > + if (y->transports) {
> > + pa_assert(pa_hashmap_isempty(y->transports));
> > + pa_hashmap_free(y->transports, NULL);
> > + }
> > +
> > if (y->connection) {
> > pa_dbus_remove_matches(pa_dbus_connection_get(y->connection),
> >
"type='signal',sender='org.freedesktop.DBus',interface='org.freedesktop.DBus',member='NameOwnerChanged'"
> > diff --git a/src/modules/bluetooth/bluez5-util.h
b/src/modules/bluetooth/bluez5-util.h
> > index a205b45..82953a7 100644
> > --- a/src/modules/bluetooth/bluez5-util.h
> > +++ b/src/modules/bluetooth/bluez5-util.h
> > @@ -24,14 +24,50 @@
> >
> > #include <pulsecore/core.h>
> >
> > +typedef struct pa_bluetooth_transport pa_bluetooth_transport;
> > typedef struct pa_bluetooth_device pa_bluetooth_device;
> > typedef struct pa_bluetooth_discovery pa_bluetooth_discovery;
> >
> > typedef enum pa_bluetooth_hook {
> > PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED, /* Call
data: pa_bluetooth_device */
> > + PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED, /* Call
data: pa_bluetooth_transport */
> > PA_BLUETOOTH_HOOK_MAX
> > } pa_bluetooth_hook_t;
> >
> > +typedef enum profile {
> > + PROFILE_A2DP_SINK,
> > + PROFILE_A2DP_SOURCE,
> > + PROFILE_OFF
> > +} pa_bluetooth_profile_t;
> > +#define PA_BLUETOOTH_PROFILE_COUNT PROFILE_OFF
> > +
> > +typedef enum pa_bluetooth_transport_state {
> > + PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED,
> > + PA_BLUETOOTH_TRANSPORT_STATE_IDLE,
> > + PA_BLUETOOTH_TRANSPORT_STATE_PLAYING
> > +} pa_bluetooth_transport_state_t;
> > +
> > +typedef int
(*pa_bluetooth_transport_acquire_cb)(pa_bluetooth_transport *t, bool
optional, size_t *imtu, size_t *omtu);
> > +typedef void
(*pa_bluetooth_transport_release_cb)(pa_bluetooth_transport *t);
>
> If these typedefs are only used in the pa_bluetooth_transport
> definition, I don't think they bring any benefit, and they also make it
> harder to check what the function signature is (the first place to look
> is the struct definition, and from there you have to go to the typedef -
> it would be easier if the signature was visible already in the struct
> definition).
>
> Hmm, now I see that the typedefs are used also in the transport_new()
> function parameters, where they certainly are very useful. However, I
> would prefer you to follow the usual subclassing pattern in PulseAudio:
> the backend calls foo_new() and then sets the callbacks and the userdata
> pointer (and then calls foo_put(), if such function exists).
>
Can you point me for an example of this pattern that I could follow? It's
not clear to me from the text what exactly you mean.
> > +
> > +struct pa_bluetooth_transport {
> > + pa_bluetooth_device *device;
> > +
> > + char *owner;
> > + char *path;
> > + pa_bluetooth_profile_t profile;
> > +
> > + uint8_t codec;
> > + uint8_t *config;
> > + int config_size;
>
> I think the convention is to use size_t for byte array sizes.
>
Ok.
> > +
> > + pa_bluetooth_transport_state_t state;
> > +
> > + pa_bluetooth_transport_acquire_cb acquire;
> > + pa_bluetooth_transport_release_cb release;
> > + void *userdata;
> > +};
> > +
> > struct pa_bluetooth_device {
> > pa_bluetooth_discovery *discovery;
> >
> > @@ -43,8 +79,16 @@ struct pa_bluetooth_device {
> > char *alias;
> > char *address;
> > int class_of_device;
> > +
> > + pa_bluetooth_transport *transports[PA_BLUETOOTH_PROFILE_COUNT];
> > };
> >
> > +pa_bluetooth_transport *pa_bluetooth_transport_new(pa_bluetooth_device
*d, const char *owner, const char *path,
> > +
pa_bluetooth_profile_t p, const uint8_t *config, int size,
>
> size_t here also.
>
Ok.
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20130725/68ba6f51/attachment-0001.html>
