Re: [PATCH] Gateway profile

[Marcel Holtmann <marcel@holtmann.org>]

Hi,

> >> >> >> I've implemented gateway profile. I've tested basic things, like
> >> >> >> place/cancel/answer call. Others are in development. Some could not be
> >> >> >> tested as my carrier doesn't provide corresponding services (like
> >> >> >> 3-way call, etc.) so any help welcome.
> >> >> >
> >> >> > thanks for the works, but can you please base the patch against the
> >> >> > latest GIT tree. It is kinda hard to review things that might already
> >> >> > have been implemented like sco_listen.
> >> >> >
> >> >> >  audio/audio-api.txt  |   94 +++++
> >> >> >  audio/device.h       |    7
> >> >> >  audio/gateway.c      |  938 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >> >  audio/gateway.h      |   11
> >> >> >  audio/manager.c      |  124 ++++--
> >> >> >  common/glib-helper.c |   85 +++-
> >> >> >  common/glib-helper.h |    1
> >> >> >  7 files changed, 1205 insertions(+), 55 deletions(-)
> >> >> >
> >> >> > So any changes to glib-helper.[ch] have to be in a separate patch and
> >> >> > need to be discussed independent from the gateway implementation.
> >> >> >
> >> >> > Any audio-api.txt stuff should also go separately since that has to be
> >> >> > discussed. Also we can't send PCM data over D-Bus. It just doesn't work
> >> >> > like that. We do have the internal IPC for that and plugins for ALSA,
> >> >> > GStreamer and PulseAudio that should be used.
> >> >> >
> >> >> > However the most important part is that you follow the coding style and
> >> >> > that is the kernel coding style. You make it really hard for us to
> >> >> > review the code like this and it can't be applied. I really want you to
> >> >> > add support for the gateway role to BlueZ, but the overall code in the
> >> >> > project needs to follow the same rules.
> >> >> >
> >> >> > So please fix these issues first and then we do a deep review of it.
> >> >>
> >> >> Here is reworked and improved patches as you suggested with IPC support.
> >> >>
> >> >> But I have some doubts and questions:
> >> >> 1. to distinguish between headset and gateway I've added one more alsa
> >> >> config option "role" which could be master (for gateway and probably
> >> >> a2dp source) or slave (which is default and works for headset and a2dp
> >> >> sink). I don't really like this approach so if you have any other idea
> >> >> it would be great.
> >> >
> >> > we should use the terms "headset", "gateway", "sink" and "source" as
> >> > these are used through the specs.
> >> >
> >>
> >> But current configuration contains "type" which is either voice or
> >> hifi. So should I set 4 mentioned above for type field? I mean this
> >> will break backward compatibility.
> >
> > you could actually and still have "sink" == "hifi" and then also
> > "headset" == "voice" for example.
> >
> >> >> 2. my cell phone closes SCO connection when it doesn't need one,
> >> >> probably others act like this as well. SCO close results in
> >> >> bluetooth_hsp_write returning an error. What would be the best way to
> >> >> overcome this?
> >> >
> >> > No idea what's the problem here. You should already get a notice of the
> >> > IPC that the channel is closed. On closed channels we have to discard
> >> > any kind of PCM data from the PC.
> >> >
> >>
> >> That is how it should work with current implementation but it would
> >> not be very nice for application developers as e.g. pause of the
> >> player on the phone will result in snd_pcm_read (or how is it named)
> >> returning an error. I've tested using pyalsaaudio which raises an
> >> exception in this case.
> >> If I would develop an application over such api I would say several bad words :)
> >
> > Obviously the ALSA plugin (or GStreamer or PluseAudio for that matter)
> > need to hide that fact and make it return a proper value instead of an
> > error. While for playback we can just discard the audio data, for
> > capture we might have to produce silence.
> >
> >> >> 3. I've noticed that ipc interface duplicate dbus interface to some
> >> >> extent. Why can't pcm_bluetooth work over dbus directly?
> >> >
> >> > D-Bus can't handle massive amount of PCM data payload. Also the ALSA
> >> > plugins don't really like dealing with a D-Bus mainloop. Hence we do
> >> > have the IPC as an alternate way of dealing with audio. We don't like to
> >> > do it, but we have to.
> >> >
> >>
> >> You can add one more DBus call which will create socket and send audio
> >> connection over it.
> >
> > It just doesn't work that way. You will be killing your performance and
> > creating memory pressure. Especially on small and embedded systems. Also
> > the latency is pretty bad. Trust me here.
> 
> I didn't meant to send audio data over dbus. My idea was like this:
> 
>      +-------+            +------+
>      | bluez |            | alsa |
>      +-------+            +------+
>          |  get unix socket  |
>          | <-----------------|
>       create                 |
>     unix socket              |
>          |                   |
>          |    send unix      |
>          |   socket name     |
>          |------------------>|
>          |                 listen
>          |                 for fd
>          |                   |
>          | send sco fd       |
>          |------------------>|
> 
> first call is over Dbus and socket is sent over domain socket.

then you still have to handle the integration of D-Bus mainloop (or call
it message parsing/handling) into an ALSA plugin. ALSA is just a total
broken concept when it comes to virtual sound cards. It works great for
actual physical devices, but that is it.

Regards

Marcel