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

[João Paulo Rechi Vita]

On Wed, Aug 7, 2013 at 4:07 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> 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().
>

Ok, even tho it's the only thing that is done in that function, I
think it still makes sense since this will also be used by the other
transport backends.

>> 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.
>

I think your reason was pretty much a matter of preference. As of my
personal preference, I think it's more clean to have a typedef to a
function signature when using it as a function pointer as part of a
struct. For me this is a kind of type in the same sense as most other
typedefs all around the code. Additionally, I found a bunch of other
occurrences of typedef'ing function signatures (although I didn't
inspect them deeply to check if all of them are exceptions of a rule)
in PA's codebase, so it didn't seem to go against the current coding
style.

Anyway, I don't think there is much point in this discussion. I stated
my personal preference, and even tho I'm one of the responsibles for
the Bluetooth audio support, you're the one currently responsible for
reviewing and integrating this code. So whatever you accepts is fine
to me. Let's get this done and move on.

-- 
João Paulo Rechi Vita
http://about.me/jprvita