[pulseaudio-discuss] [PATCH 26/56] bluetooth: Create pa_bluetooth_transport for BlueZ 5 support

[Tanu Kaskinen]

On Tue, 2013-08-06 at 20:08 -0300, João Paulo Rechi Vita wrote:
> On Mon, Jul 29, 2013 at 12:29 PM, Tanu Kaskinen
> <tanu.kaskinen at linux.intel.com> wrote:
> > On Thu, 2013-07-25 at 00:31 -0300, João Paulo Rechi Vita wrote:
> >> 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:
> >> > > 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.
> >
> > Well, have a look at how sinks, for example, are created.
> > module-bluetooth-device calls pa_sink_new(), sets pa_sink.userdata and
> > some callbacks, and then calls pa_sink_put(). Similarly, the a2dp
> > transport code can call pa_bluez5_transport_new(), then set
> > pa_bluez5_transport.userdata and the callbacks, and then, if necessary,
> > call pa_bluez5_transport_put().
> >
> 
> If I understood correctly your suggestion is to have acquire_cb,
> release_cb and userdata set by the caller function instead of being
> parameters of pa_bluetooth_transport_new(). I don't see how a _put()
> function would fit here.

If you fire a hook (I don't remember if you do) to inform others about
the new transport, then that should be done in _put() instead of _new(),
because the transport isn't yet fully initialized in _new().

> But even with this pattern I prefer to keep
> the typedefs instead of having the function signature in the struct.

Why? I gave a reason for why I want the signature in the struct, but you
haven't provided any counter argument.

-- 
Tanu