[PATCH v2 1/2] Modular API for Bluetooth A2DP codec

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sunday 05 August 2018 11:07:23 Arun Raghavan wrote:
> On Sat, 28 Jul 2018, at 9:04 PM, Pali Rohár wrote:
> > Move current SBC codec implementation into separate source file and use
> > this new API for providing all needed functionality for Bluetooth A2DP.
> > 
> > Both bluez5-util and module-bluez5-device are changed to use this new
> > modular codec API.
> > ---
> >  src/Makefile.am                              |   9 +-
> >  src/modules/bluetooth/a2dp-codecs.h          |   5 +-
> >  src/modules/bluetooth/bluez5-util.c          | 331 +++++----------
> >  src/modules/bluetooth/bluez5-util.h          |  10 +-
> >  src/modules/bluetooth/module-bluez5-device.c | 487 ++++++----------------
> >  src/modules/bluetooth/pa-a2dp-codec-sbc.c    | 579 +++++++++++++++++++++++++++
> >  src/modules/bluetooth/pa-a2dp-codec.h        |  40 ++
> >  7 files changed, 851 insertions(+), 610 deletions(-)
> >  create mode 100644 src/modules/bluetooth/pa-a2dp-codec-sbc.c
> >  create mode 100644 src/modules/bluetooth/pa-a2dp-codec.h
> > 
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index f62e7d5c4..411b9e5e5 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -2117,6 +2117,7 @@ module_bluetooth_discover_la_CFLAGS = $(AM_CFLAGS) 
> > -DPA_MODULE_NAME=module_bluet
> >  libbluez5_util_la_SOURCES = \
> >  		modules/bluetooth/bluez5-util.c \
> >  		modules/bluetooth/bluez5-util.h \
> > +		modules/bluetooth/pa-a2dp-codec.h \
> >  		modules/bluetooth/a2dp-codecs.h
> >  if HAVE_BLUEZ_5_OFONO_HEADSET
> >  libbluez5_util_la_SOURCES += \
> > @@ -2131,6 +2132,10 @@ libbluez5_util_la_LDFLAGS = -avoid-version
> >  libbluez5_util_la_LIBADD = $(MODULE_LIBADD) $(DBUS_LIBS)
> >  libbluez5_util_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS)
> >  
> > +libbluez5_util_la_SOURCES += modules/bluetooth/pa-a2dp-codec-sbc.c
> > +libbluez5_util_la_LIBADD += $(SBC_LIBS)
> > +libbluez5_util_la_CFLAGS += $(SBC_CFLAGS)
> > +
> >  module_bluez5_discover_la_SOURCES = modules/bluetooth/module-bluez5-
> > discover.c
> >  module_bluez5_discover_la_LDFLAGS = $(MODULE_LDFLAGS)
> >  module_bluez5_discover_la_LIBADD = $(MODULE_LIBADD) $(DBUS_LIBS) 
> > libbluez5-util.la
> > @@ -2138,8 +2143,8 @@ module_bluez5_discover_la_CFLAGS = $(AM_CFLAGS) $
> > (DBUS_CFLAGS) -DPA_MODULE_NAME=
> >  
> >  module_bluez5_device_la_SOURCES = modules/bluetooth/module-bluez5-
> > device.c
> >  module_bluez5_device_la_LDFLAGS = $(MODULE_LDFLAGS)
> > -module_bluez5_device_la_LIBADD = $(MODULE_LIBADD) $(SBC_LIBS) 
> > libbluez5-util.la
> > -module_bluez5_device_la_CFLAGS = $(AM_CFLAGS) $(SBC_CFLAGS) -
> > DPA_MODULE_NAME=module_bluez5_device
> > +module_bluez5_device_la_LIBADD = $(MODULE_LIBADD) libbluez5-util.la
> > +module_bluez5_device_la_CFLAGS = $(AM_CFLAGS) -
> > DPA_MODULE_NAME=module_bluez5_device
> >  
> >  # Apple Airtunes/RAOP
> >  module_raop_sink_la_SOURCES = modules/raop/module-raop-sink.c
> > diff --git a/src/modules/bluetooth/a2dp-codecs.h b/src/modules/
> > bluetooth/a2dp-codecs.h
> > index 8afcfcb24..004975586 100644
> > --- a/src/modules/bluetooth/a2dp-codecs.h
> > +++ b/src/modules/bluetooth/a2dp-codecs.h
> > @@ -47,6 +47,9 @@
> >  #define SBC_ALLOCATION_SNR		(1 << 1)
> >  #define SBC_ALLOCATION_LOUDNESS		1
> >  
> > +#define SBC_MIN_BITPOOL 2
> > +#define SBC_MAX_BITPOOL 64
> > +
> >  #define MPEG_CHANNEL_MODE_MONO		(1 << 3)
> >  #define MPEG_CHANNEL_MODE_DUAL_CHANNEL	(1 << 2)
> >  #define MPEG_CHANNEL_MODE_STEREO	(1 << 1)
> > @@ -63,8 +66,6 @@
> >  #define MPEG_SAMPLING_FREQ_44100	(1 << 1)
> >  #define MPEG_SAMPLING_FREQ_48000	1
> >  
> > -#define MAX_BITPOOL 64
> > -#define MIN_BITPOOL 2
> >  
> >  #if __BYTE_ORDER == __LITTLE_ENDIAN
> >  
> > diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/
> > bluetooth/bluez5-util.c
> > index 2d8337317..9c4e3367b 100644
> > --- a/src/modules/bluetooth/bluez5-util.c
> > +++ b/src/modules/bluetooth/bluez5-util.c
> > @@ -2,6 +2,7 @@
> >    This file is part of PulseAudio.
> >  
> >    Copyright 2008-2013 João Paulo Rechi Vita
> > +  Copyrigth 2018      Pali Rohár <pali.rohar at gmail.com>
> >  
> >    PulseAudio is free software; you can redistribute it and/or modify
> >    it under the terms of the GNU Lesser General Public License as
> > @@ -33,7 +34,7 @@
> >  #include <pulsecore/refcnt.h>
> >  #include <pulsecore/shared.h>
> >  
> > -#include "a2dp-codecs.h"
> > +#include "pa-a2dp-codec.h"
> >  
> >  #include "bluez5-util.h"
> >  
> > @@ -48,8 +49,8 @@
> >  
> >  #define BLUEZ_ERROR_NOT_SUPPORTED "org.bluez.Error.NotSupported"
> >  
> > -#define A2DP_SOURCE_ENDPOINT "/MediaEndpoint/A2DPSource"
> > -#define A2DP_SINK_ENDPOINT "/MediaEndpoint/A2DPSink"
> > +#define A2DP_SOURCE_SBC_ENDPOINT "/MediaEndpoint/A2DPSourceSBC"
> > +#define A2DP_SINK_SBC_ENDPOINT "/MediaEndpoint/A2DPSinkSBC"
> >  
> >  #define ENDPOINT_INTROSPECT_XML                                         
> > \
> >      DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE                           
> > \
> > @@ -170,9 +171,9 @@ static const char 
> > *transport_state_to_string(pa_bluetooth_transport_state_t stat
> >  
> >  static bool device_supports_profile(pa_bluetooth_device *device, 
> > pa_bluetooth_profile_t profile) {
> >      switch (profile) {
> > -        case PA_BLUETOOTH_PROFILE_A2DP_SINK:
> > +        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
> >              return !!pa_hashmap_get(device->uuids, 
> > PA_BLUETOOTH_UUID_A2DP_SINK);
> > -        case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
> > +        case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
> >              return !!pa_hashmap_get(device->uuids, 
> 
> I haven't done a thorough review yet, but this one caught my eye. Tying the codec and the profile together here does not seem to be a good choice. Seems cleaner to separate out the codec choice into a separate field.

For each codec you need bluez endpoint, bound to bluez code which is in
bluez5-util.c

If codec should not be part of profile? How should be codec selected,
negotiated or switched by user?

And how to handle situation for bi-rectional A2DP codecs? Because now
PA_BLUETOOTH_PROFILE_A2DP_SINK and PA_BLUETOOTH_PROFILE_A2DP_SOURCE
support only one direction.

> There's a bunch of other stuff, but probably makes sense to discuss this up-front.
> 
> Also, we now have GitLab for patch submission, so feel free to use that for easier code reviews in the next iteration.
> 
> -- Arun

-- 
Pali Rohár
pali.rohar at gmail.com


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux