Re: [pulseaudio-discuss] [PATCH v2 1/2] Modular API for Bluetooth A2DP codec
On Monday 17 September 2018 15:02:18 Tanu Kaskinen wrote: > > > > +sbc_info->min_bitpool = config->min_bitpool; > > > > +sbc_info->max_bitpool = config->max_bitpool; > > > > + > > > > +/* Set minimum bitpool for source to get the maximum possible > > > > block_size */ > > > > +sbc_info->sbc.bitpool = is_a2dp_sink ? sbc_info->max_bitpool : > > > > sbc_info->min_bitpool; > > > > > > Do you understand the logic here? > > > > I have not looked deeply at this code. So I'm not fully sure. I just > > moved existing code into new file. > > Ok, so this remains a mystery to all of us. Now I figured out, why that logic is there. Is is because of how block size in a2dp_prepare_decoder_buffer() function is calculated. Thanks for your email about while loops, checking processed sizes in faststream decoder function; this helped me. It is mess. I'm going to cleanup this code. But it would need some changes in codec API... That minimal bitpool value is there really needed and I will add comment with detailed explanation to code. Bitpool is controlled by sender so for source by other side -- not pulseaudio. Frame length depends on bitpool value and buffer size which is calculated from MTU is inversely proportional to frame length. To prevent decoding failures due to small buffer, we set bitpool value for source to the smallest value, so a2dp_prepare_decoder_buffer() calculates and allocates the largest possible buffer size. sbc_info->sbc.bitpool is not used by SBC decoder itself. Moreover after decoding each SBC frame, libsbc decoder updates this value to bitpool of decoded SBC frame. -- Pali Rohár pali.ro...@gmail.com signature.asc Description: PGP signature ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v2 1/2] Modular API for Bluetooth A2DP codec
Hi Tanu! I'm working on a new version of this patch series.
Below are comments, I'm fixing problems which you pointed. Thank you for
review.
Also, on bluez mailing list are patches which add support for profile
switching, so I'm implementing it for this patch series.
Once I have implemented it, I will send a new version to pulseaudio
mailing list.
Most important change is removal of all codec specific enums, ifdefs,
etc... from bluez5-util.c and module-bluez5-device.c files. So for
adding new codec it would not be needed to touch these files!
On Tuesday 04 September 2018 11:44:10 Tanu Kaskinen wrote:
> On Sat, 2018-07-28 at 17:34 +0200, 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 ++
>
> The "pa-" prefix doesn't look very good to me. Maybe you didn't want to
> add a2dp-codec.h, because it looks so similar to the existing a2dp-
> codecs.h header? I think we can get rid of a2dp-codecs.h: the SBC stuff
> can be moved to a2dp-codec-sbc.c, and the MPEG stuff can be dropped
> since it's not used anywhere.
I'm going to change/fix it.
a2dp-codec-api.h --> structure definitions for implementing codecs
a2dp-codec-.c --> particular codec implementation
a2dp-codecs.h--> upstream bluez header file for A2DP definitions
a2dp-codec-util.h--> header file for utility functions for working
with codecs (e.g. listing all codecs, etc.)
a2dp-codec-util.c--> implementation of a2dp-codec-util.h
> > @@ -888,10 +889,21 @@ finish:
> > static void register_endpoint(pa_bluetooth_discovery *y, const char *path,
> > const char *endpoint, const char *uuid) {
> > DBusMessage *m;
> > DBusMessageIter i, d;
> > -uint8_t codec = 0;
> > +uint8_t capabilities[1024];
> > +size_t capabilities_size;
> > +uint8_t codec_id;
> > +const pa_a2dp_codec_t *codec;
> > +
> > +codec = pa_a2dp_endpoint_to_a2dp_codec(endpoint);
>
> I think it would be better to change the function parameters so that
> instead of an endpoint path the function would take a codec id.
That is not possible. endpoint is bound to pair (codec + direction).
I'm changing function parameters, so pulseaudio codec structure is
passed too (together with endpoint).
This simplify lot of other things and removal of
pa_a2dp_endpoint_to_a2dp_codec function call from there.
> > +if (!codec)
> > +return;
>
> As far as I can tell, this should never happen, so an assertion would
> be better (and it could be in the lookup function so that every caller
> doesn't need to add a check).
After above change, yes, so removing it.
> > @@ -1316,6 +1271,38 @@ const char
> > *pa_bluetooth_profile_to_string(pa_bluetooth_profile_t profile) {
> > return NULL;
> > }
> >
> > +const char *pa_bluetooth_profile_to_a2dp_endpoint(pa_bluetooth_profile_t
> > profile) {
>
> This function isn't used outside bluez5-util.c, so it can be made
> static and removed from bluez5-util.h. Then the pa_bluetooth_ prefix
> should be dropped.
Yes, doing it.
> > +switch (profile) {
> > +case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
> > +return A2DP_SOURCE_SBC_ENDPOINT;
> > +case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
> > +return A2DP_SINK_SBC_ENDPOINT;
> > +default:
> > +return NULL;
>
> I think it would be good to use pa_assert_not_reached() here. I assume
> this won't be used in a context where a non-a2dp profile would be
> passed to the function.
The whole bluez5-util.c file does not have any codec specific enums, so
this above profile switch was removed.
> > +}
> > +
> > +return NULL;
>
> This is redundant.
>
> > +}
> > +
> > +const pa_a2dp_codec_t
> > *pa_bluetooth_profile_to_a2dp_codec(pa_bluetooth_profile_t profile) {
> > +switch (profile) {
> > +case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
> > +case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
> > +return _a2dp_codec_sbc;
> > +default:
> > +return NULL;
> > +}
> > +
> > +return NULL;
> > +}
>
> This function seems to be used also for checking whether a profile is
> an a2dp profile. I think it would be clearer to have a separate
> pa_bluetooth_profile_is_a2dp() function. Then this function could also
> have an assertion rather than returning NULL
Re: [pulseaudio-discuss] [PATCH v2 1/2] Modular API for Bluetooth A2DP codec
On Monday 17 September 2018 15:02:18 Tanu Kaskinen wrote: > (I sent my review both to you and to the list, but you replied only > privately - probably not on purpose. I'll send this mail only to the > list.) Sorry for that. I was not my intension. > On Thu, 2018-09-13 at 10:54 +0200, Pali Rohár wrote: > > On Tuesday 04 September 2018 11:44:10 Tanu Kaskinen wrote: > > > Using libopenaptx directly > > > doesn't prevent us from switching to GStreamer later if someone writes > > > the code. > > > > Yes, that is truth. > > > > > Another point that was raised that the codec choice shouldn't be bound > > > to the card profile. I tend to agree with that. There are situations > > > where module-bluetooth-policy or the user only wants to switch from HSP > > > to A2DP and not care about the codec. You asked how the codec should be > > > negotiated or switched by the user if it's not bound to the profile. > > > Regarding negotiation, we can add a hook that module-bluetooth-policy > > > can use to make the codec decision (if module-bluetooth-policy isn't > > > loaded, SBC seems like a sensible default). > > > > > > Is there a need for the user to be able to choose the codec? Shouldn't > > > we always automatically pick the highest-quality one? > > > > What is "highest-quality" codec? How you would compare e.g. our SBC > > implementation which can dynamically change bitpool and therefore > > quality? > > > > Also how you can compare... has SBC higher quality as MPEG1? Or has aptX > > HD higher quality as MPEG/AAC? > > Fair point. > > > Until now everybody said that aptX "is better" then SBC. But today it > > does not look like it is truth. > > > > Also if you have MP3 files on disk, then the best quality which you can > > achieve is to passthru it without any reencoding. In other cases AAC can > > be better. > > Passthrough is a separate thing. If someone implements passthrough > support for bluetooth and a client requests it, we should always switch > to the requested codec regardless of other user preferences. > > > So answer to this question depends on lot of things and either user > > itself or user's application would better it. > > > > > I'm not against > > > adding the functionality, it would be useful for testing if nothing > > > else. It just doesn't seem very important. > > > > > > We could have a "preferred-a2dp-codec" option in bluetooth.conf (that > > > file doesn't exist, but can be added). A per-card option would be > > > possible too, as would be having both a global option that could be > > > overridden with a per-card option. > > > > Preferred codec is not helpful. The only mandatory codec is SBC, all > > other are optional. And basically every manufactor supports different > > set of codecs. So at least some kind of list or partially ordered set of > > codec is needed. > > Sure, a priority list can better capture the user preferences, but > surely a "preferred codec" option is better than nothing. A priority > list of codecs isn't an obvious ultimate solution either, since some > codecs have adjustable parameters that can affect the codec preference > ranking. For example, someone's preference might be: > > SBC (high bitrate) > aptX > SBC (medium bitrate) > > It's not clear to me how fine-grained control is optimal. A solution > that allows specifying a priority list with an arbitrary number of > codec parameters would obviously be sufficient, but it's unlikely we > want that complex system. The best thing is allowing user to choose codec itself (in some GUI under section additional settings, or similar) and ideally remember it for every device separately. > Real use cases are needed. I can't provide any, since I don't use > bluetooth (and if I did, I probably wouldn't bother with the codec > settings at all - SBC and aptX seem both good enough for me). Now that > you found out that aptX isn't that great after all, do you have > personal use for aptX either? This is interesting question. For more then month I'm using aptX codec and I have not hear difference or something which would disturb me. So I probably really do not need aptX at all. > > > For runtime configuration, we can add a command to set the codec > > > preference. This should be done with the message API that Georg Chini > > > has been working on (not yet finished). There's then need for saving > > > the choice too. We can either introduce a new database for bluetooth > > > stuff, or we can add some API to module-card-restore so that other > > > modules (module-bluetooth-discover in this case) can save arbitrary > > > parameters for cards. > > > > > > I recall there was some lack of relevant APIs in bluetoothd for > > > choosing the codec from PulseAudio. Can you remind me, what are the > > > limitations, and how did you plan to deal with them? > > > > Currently when bluetooth headset initialize connection, it is up to > > headset which codec would choose. I did some testing and my headset > > choose codec (sbc
Re: [pulseaudio-discuss] [PATCH v2 1/2] Modular API for Bluetooth A2DP codec
On Tue, 4 Sep 2018, at 2:14 PM, Tanu Kaskinen wrote: > Hi Pali! > > Thanks a lot for working on this! Arun has been strongly advocating > using GStreamer, and I don't know if his position is that "AptX must be > implemented with GStreamer or not implemented at all". I hope not. To > me the library choice is not important. Using libopenaptx directly > doesn't prevent us from switching to GStreamer later if someone writes > the code. A couple of notes. Firstly, I really do appreciate all the work that Pali has put into this (it is clearly a lot). Secondly, while I am dead against adding a codec as a dep, once we get things in shape where this is the only issue, I volunteer to help with the GStreamer bits so as to not block this. Cheers, Arun ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v2 1/2] Modular API for Bluetooth A2DP codec
(I sent my review both to you and to the list, but you replied only privately - probably not on purpose. I'll send this mail only to the list.) On Thu, 2018-09-13 at 10:54 +0200, Pali Rohár wrote: > On Tuesday 04 September 2018 11:44:10 Tanu Kaskinen wrote: > > Using libopenaptx directly > > doesn't prevent us from switching to GStreamer later if someone writes > > the code. > > Yes, that is truth. > > > Another point that was raised that the codec choice shouldn't be bound > > to the card profile. I tend to agree with that. There are situations > > where module-bluetooth-policy or the user only wants to switch from HSP > > to A2DP and not care about the codec. You asked how the codec should be > > negotiated or switched by the user if it's not bound to the profile. > > Regarding negotiation, we can add a hook that module-bluetooth-policy > > can use to make the codec decision (if module-bluetooth-policy isn't > > loaded, SBC seems like a sensible default). > > > > Is there a need for the user to be able to choose the codec? Shouldn't > > we always automatically pick the highest-quality one? > > What is "highest-quality" codec? How you would compare e.g. our SBC > implementation which can dynamically change bitpool and therefore > quality? > > Also how you can compare... has SBC higher quality as MPEG1? Or has aptX > HD higher quality as MPEG/AAC? Fair point. > Until now everybody said that aptX "is better" then SBC. But today it > does not look like it is truth. > > Also if you have MP3 files on disk, then the best quality which you can > achieve is to passthru it without any reencoding. In other cases AAC can > be better. Passthrough is a separate thing. If someone implements passthrough support for bluetooth and a client requests it, we should always switch to the requested codec regardless of other user preferences. > So answer to this question depends on lot of things and either user > itself or user's application would better it. > > > I'm not against > > adding the functionality, it would be useful for testing if nothing > > else. It just doesn't seem very important. > > > > We could have a "preferred-a2dp-codec" option in bluetooth.conf (that > > file doesn't exist, but can be added). A per-card option would be > > possible too, as would be having both a global option that could be > > overridden with a per-card option. > > Preferred codec is not helpful. The only mandatory codec is SBC, all > other are optional. And basically every manufactor supports different > set of codecs. So at least some kind of list or partially ordered set of > codec is needed. Sure, a priority list can better capture the user preferences, but surely a "preferred codec" option is better than nothing. A priority list of codecs isn't an obvious ultimate solution either, since some codecs have adjustable parameters that can affect the codec preference ranking. For example, someone's preference might be: SBC (high bitrate) aptX SBC (medium bitrate) It's not clear to me how fine-grained control is optimal. A solution that allows specifying a priority list with an arbitrary number of codec parameters would obviously be sufficient, but it's unlikely we want that complex system. Real use cases are needed. I can't provide any, since I don't use bluetooth (and if I did, I probably wouldn't bother with the codec settings at all - SBC and aptX seem both good enough for me). Now that you found out that aptX isn't that great after all, do you have personal use for aptX either? > > For runtime configuration, we can add a command to set the codec > > preference. This should be done with the message API that Georg Chini > > has been working on (not yet finished). There's then need for saving > > the choice too. We can either introduce a new database for bluetooth > > stuff, or we can add some API to module-card-restore so that other > > modules (module-bluetooth-discover in this case) can save arbitrary > > parameters for cards. > > > > I recall there was some lack of relevant APIs in bluetoothd for > > choosing the codec from PulseAudio. Can you remind me, what are the > > limitations, and how did you plan to deal with them? > > Currently when bluetooth headset initialize connection, it is up to > headset which codec would choose. I did some testing and my headset > choose codec (sbc vs aptx) just randomly. > > When bluez initialize connection, then it prefer codecs in order in > which pulseaudio did dbus endpoint registration. Order is global and > hardcoded in source code. > > Once connection is established there is no way to change codec. bluez > does not provide API for it. > > So everything which you wrote above would apply only when pulseaudio > starts a2dp connection, not headset. > > And I think it is very bad if we cannot achieve stable codec selection. So what do you propose? If your point was that my suggestions result in unstable codec selection, I don't see how your original proposal of having a
Re: [pulseaudio-discuss] [PATCH v2 1/2] Modular API for Bluetooth A2DP codec
On Tue, 2018-09-04 at 11:44 +0300, Tanu Kaskinen wrote:
> On Sat, 2018-07-28 at 17:34 +0200, Pali Rohár wrote:
> > +static size_t pa_sbc_select_configuration(const pa_sample_spec
> > *sample_spec, const uint8_t *capabilities_buffer, size_t capabilities_size,
> > uint8_t *config_buffer, size_t config_size) {
> > +a2dp_sbc_t *config = (a2dp_sbc_t *) config_buffer;
> > +a2dp_sbc_t *capabilities = (a2dp_sbc_t *) capabilities_buffer;
>
> This looks likely to cause alignment errors, since the data in the
> uint8_t arrays doesn't have any kind of alignment guarantees. However,
> a2dp_sbc_t happens to only contain uint8_t values, so the values can't
> be badly aligned. It would be good to have a comment that reassures the
> reader that there won't be any alignment errors. Suggestion: "We cast a
> byte array to struct here, which can often cause alignment errors, but
> in this case it's safe, because a2dp_sbc_t contains only single-byte
> values."
a2dp_aptx_t has variables that are bigger than just one byte, so I
thought that we're going to have alignment issues with it. However, I
found out now that packed structs don't have alignment restrictions,
because the compiler will always be extra careful when dealing with
them. Therefore a better comment would be: "We cast a byte array to
struct here, which can often cause alignment errors, but in this case
it's safe, because a2dp_sbc_t uses the packed attribute, which makes
the compiler extra careful."
Copying that comment everywhere where we do the casting may not be such
a great idea after all, though. So much repetition. Maybe just add to
the struct definition a comment that explains what the packed attribute
does.
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v2 1/2] Modular API for Bluetooth A2DP codec
Hi Pali! Thanks a lot for working on this! Arun has been strongly advocating using GStreamer, and I don't know if his position is that "AptX must be implemented with GStreamer or not implemented at all". I hope not. To me the library choice is not important. Using libopenaptx directly doesn't prevent us from switching to GStreamer later if someone writes the code. Another point that was raised that the codec choice shouldn't be bound to the card profile. I tend to agree with that. There are situations where module-bluetooth-policy or the user only wants to switch from HSP to A2DP and not care about the codec. You asked how the codec should be negotiated or switched by the user if it's not bound to the profile. Regarding negotiation, we can add a hook that module-bluetooth-policy can use to make the codec decision (if module-bluetooth-policy isn't loaded, SBC seems like a sensible default). Is there a need for the user to be able to choose the codec? Shouldn't we always automatically pick the highest-quality one? I'm not against adding the functionality, it would be useful for testing if nothing else. It just doesn't seem very important. We could have a "preferred-a2dp-codec" option in bluetooth.conf (that file doesn't exist, but can be added). A per-card option would be possible too, as would be having both a global option that could be overridden with a per-card option. For runtime configuration, we can add a command to set the codec preference. This should be done with the message API that Georg Chini has been working on (not yet finished). There's then need for saving the choice too. We can either introduce a new database for bluetooth stuff, or we can add some API to module-card-restore so that other modules (module-bluetooth-discover in this case) can save arbitrary parameters for cards. I recall there was some lack of relevant APIs in bluetoothd for choosing the codec from PulseAudio. Can you remind me, what are the limitations, and how did you plan to deal with them? Comments on the code below. This review was done over several days, sorry for any inconsistencies or strange repetition (I noticed I explain some things multiple times). On Sat, 2018-07-28 at 17:34 +0200, 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 ++ The "pa-" prefix doesn't look very good to me. Maybe you didn't want to add a2dp-codec.h, because it looks so similar to the existing a2dp- codecs.h header? I think we can get rid of a2dp-codecs.h: the SBC stuff can be moved to a2dp-codec-sbc.c, and the MPEG stuff can be dropped since it's not used anywhere. > 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) Cosmetic nitpicking: these can be merged with the earlier variable assignemnts. Maybe you're aiming for some kind of "keep the SBC stuff separate" modularization, but that's not really in line of how the rest of the file is written. > + > 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 >
Re: [pulseaudio-discuss] [PATCH v2 1/2] Modular API for Bluetooth A2DP codec
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_LOUDNESS1 > > > > +#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 > > > >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 > > #include > > > > -#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
Re: [pulseaudio-discuss] [PATCH v2 1/2] Modular API for Bluetooth A2DP codec
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
>
>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
> #include
>
> -#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
[pulseaudio-discuss] [PATCH v2 1/2] Modular API for Bluetooth A2DP codec
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_LOUDNESS1
+#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
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
#include
-#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,
PA_BLUETOOTH_UUID_A2DP_SOURCE);
