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-<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 &pa_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 for non-a2dp profiles.
Yes, I added new functions pa_bluetooth_profile_is_a2dp(),
pa_bluetooth_profile_is_a2dp_sink() and
pa_bluetooth_profile_is_a2dp_source().
They helped in other parts of code.
> If we don't tie the codec and the profile together, however, then
> there's no need for an is_a2dp() function.
>
> > +
> > +const pa_a2dp_codec_t *pa_a2dp_endpoint_to_a2dp_codec(const char *endpoint) {
> > + if (pa_streq(endpoint, A2DP_SOURCE_SBC_ENDPOINT) || pa_streq(endpoint, A2DP_SINK_SBC_ENDPOINT))
> > + return &pa_a2dp_codec_sbc;
> > + else
> > + return NULL;
>
> pa_assert_not_reached() could be used here.
In endpoint_handler() I'm getting untrusted bluez dbus data, and above
function is calling on endpoint from bluez. So it should not throw
assert error, but in endpoint_handler() returns
DBUS_HANDLER_RESULT_NOT_YET_HANDLED.
So pa_assert_not_reached() cannot be used there.
> > + config_size = codec->select_configuration(&y->core->default_sample_spec, cap, size, config, sizeof(config));
> > + pa_assert(config_size != 0);
>
> We don't trust input from bluetoothd to be always valid, so
> select_configuration() can fail. Proper error handling is needed here.
codec->select_configuration is not from bluetoothd, but from pulseaudio
codec. And we should trust to our pulseaudio code.
> > @@ -1709,38 +1578,22 @@ static void endpoint_init(pa_bluetooth_discovery *y, pa_bluetooth_profile_t prof
> > static const DBusObjectPathVTable vtable_endpoint = {
> > .message_function = endpoint_handler,
> > };
> > + const char *endpoint = pa_bluetooth_profile_to_a2dp_endpoint(profile);
>
> The function could just take the endpoint path as an argument.
> pa_bluetooth_profile_t is not needed by this function.
Yes, I changed function argument from profile to endpoint.
> > static void endpoint_done(pa_bluetooth_discovery *y, pa_bluetooth_profile_t profile) {
> > + const char *endpoint = pa_bluetooth_profile_to_a2dp_endpoint(profile);
>
> Same comment as above.
Fixed too.
> > struct userdata {
> > pa_module *module;
> > pa_core *core;
> > @@ -146,7 +132,11 @@ struct userdata {
> > pa_smoother *read_smoother;
> > pa_memchunk write_memchunk;
> > pa_sample_spec sample_spec;
> > - struct sbc_info sbc_info;
> > + void *encoder_info;
> > + void *decoder_info;
>
> I think it would be better to put these inside the pa_a2dp_codec
> struct, preferably behind a single void pointer named "userdata" (as
> that would follow the usual PA code conventions). The various callbacks
> in pa_a2dp_codec that now take a void** parameter as their first
> argument would take a pa_a2dp_codec pointer instead.
These two pointers are user data passed to function callbacks in
pa_a2dp_codec strctures. So they need to be in module/stream user data.
Anyway, I was thinking about putting encoder_info and decoder_info into
one structure, but that decreased complexity in codec implementation. So
in current state it is easier to implement and a new codec.
So I would rather leave it as is.
> > +
> > + void* buffer; /* Codec transfer buffer */
> > + size_t buffer_size; /* Size of the buffer */
> > };
> > @@ -1078,7 +924,7 @@ static int add_source(struct userdata *u) {
> >
> > if (!u->transport_acquired)
> > switch (u->profile) {
> > - case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
> > + case PA_BLUETOOTH_PROFILE_A2DP_SBC_SOURCE:
> > case PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY:
> > data.suspend_cause = PA_SUSPEND_USER;
> > break;
> > @@ -1090,8 +936,9 @@ static int add_source(struct userdata *u) {
> > else
> > pa_assert_not_reached();
> > break;
> > - case PA_BLUETOOTH_PROFILE_A2DP_SINK:
> > + case PA_BLUETOOTH_PROFILE_A2DP_SBC_SINK:
> > case PA_BLUETOOTH_PROFILE_OFF:
> > + default:
>
> If a switch statement has cases for all members of an enumeration, as
> is the case here, then it's better to leave the default case out,
> because the default case removes compiler warnings about unhandled
> cases. If we add a new profile, it's good to get a warning if it's not
> handled here.
All codec specific enums were removed from module-bluez5-device.c file.
So above comment is not relevant for new patch version.
> > +typedef struct sbc_info {
> > + sbc_t sbc; /* Codec data */
> > + bool sbc_initialized; /* Keep track if the encoder is initialized */
> > + size_t codesize, frame_length; /* SBC Codesize, frame_length. We simply cache those values here */
> > + uint16_t seq_num; /* Cumulative packet sequence */
> > + uint8_t min_bitpool;
> > + uint8_t max_bitpool;
> > +} sbc_info_t;
>
> According to our coding style, structs that aren't exported in any
> headers should not be typedef'd. And if structs are typedef'd, they
> shouldn't have a "_t" suffix. (Yes, the typedef existed in the old code
> too, but I thought I'd mention this anyway.)
Fixing it.
> > +
> > +static size_t pa_sbc_fill_capabilities(uint8_t *capabilities_buffer, size_t capabilities_size) {
>
> Static functions shouldn't have the "pa_" prefix. The "sbc_" prefix
> feels a bit redundant too. This comment applies to all functions in
> this file.
Fixing it.
> > + if (capabilities->allocation_method & SBC_ALLOCATION_LOUDNESS)
> > + config->allocation_method = SBC_ALLOCATION_LOUDNESS;
> > + else if (capabilities->allocation_method & SBC_ALLOCATION_SNR)
> > + config->allocation_method = SBC_ALLOCATION_SNR;
>
> Error handling for unsupported allocation method is missing. Not your
> fault, and this doesn't look serious (because the final configuration
> is validated anyway), but you can fix this if you want.
Fixing it.
> > diff --git a/src/modules/bluetooth/pa-a2dp-codec.h b/src/modules/bluetooth/pa-a2dp-codec.h
> > new file mode 100644
> > index 000000000..68b1619c2
> > --- /dev/null
> > +++ b/src/modules/bluetooth/pa-a2dp-codec.h
> > @@ -0,0 +1,40 @@
> > +#ifndef foopaa2dpcodechfoo
> > +#define foopaa2dpcodechfoo
> > +
> > +/***
> > + This file is part of PulseAudio.
> > +
> > + Copyright 2018 Pali Rohár <pali.rohar@xxxxxxxxx>
> > +
> > + PulseAudio is free software; you can redistribute it and/or modify
> > + it under the terms of the GNU Lesser General Public License as
> > + published by the Free Software Foundation; either version 2.1 of the
> > + License, or (at your option) any later version.
> > +
> > + PulseAudio is distributed in the hope that it will be useful, but
> > + WITHOUT ANY WARRANTY; without even the implied warranty of
> > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + General Public License for more details.
> > +
> > + You should have received a copy of the GNU Lesser General Public
> > + License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
> > +***/
> > +
> > +typedef struct pa_a2dp_codec {
> > + const char *codec_name;
>
> This is currently unused, so it should be removed unless you come up
> with some use for it.
In new patch version it is used.
> > + uint8_t codec_id;
> > + size_t (*fill_capabilities)(uint8_t *, size_t);
>
> It would be good to have some documentation for the callbacks. Naming
> the function parameters also makes it easier to understand the code.
> Imagine being someone who's not that familiar with the code but has
> decided to add support for a new codec and needs to write
> implementations for the callbacks.
I know. I'm adding documentation comments for all callback functions and
all members. So adding new codec would be easier.
Also in patch version, I'm changing function names so they would better
describe what they are doing, plus I'm changing API...
> > + bool (*validate_configuration)(const uint8_t *, size_t);
>
> The bool return value is used for reporting failures, but according to
> our coding style failures should be reported using a negative int.
It is really needed to use integer return value for true/false?
> > + size_t (*select_configuration)(const pa_sample_spec *, const uint8_t *, size_t, uint8_t *, size_t);
> > + void (*init)(void **, pa_sample_spec *, bool, const uint8_t *, size_t);
> > + void (*finish)(void **);
> > + void (*setup)(void **);
> > + void (*fill_blocksize)(void **, size_t, size_t, size_t *, size_t *);
> > + bool (*fix_latency)(void **);
>
> I feel this callback name is not very good. The callback reduces the
> bitpool (in case of SBC), it doesn't fix the latency. Fixing the
> latency is left to the caller.
>
> I suggest changing the name to "reduce_bitrate" (sufficiently generic
> to not be specific to SBC).
That is better name! I will use reduce_encoder_bitrate.
> I also suggest some restructuring, because
> the logic behind calling transport_config_mtu() and
> update_buffer_size() after fix_latency() wasn't obvious. Restructuring
> ideas:
>
> 1) Allow the reduce_bitrate() callback to directly update the block
> size. The callback should return true only if the block size was
> changed.
>
> This will require access to the MTU sizes from reduce_bitrate(), so
> fill_blocksize() should save the MTU sizes in the codec private data
> (or the MTU could be passed as arguments to reduce_bitrate(), but I
> think it's cleaner to save the previously set MTU in the codec private
> data). It might make sense to rename fill_blocksize() to set_mtu().
>
> 2) Add a handle_block_size_change() function and move there the sink
> latency update code from transport_config_mtu() and the code from
> update_buffer_size(). Call handle_block_size_change() from
> transport_configu_mtu() and after the reduce_bitrate() call. Remove the
> transport_mtu_config() and update_buffer_size() calls from
> thread_function(). Remove the update_buffer_size() call from
> setup_stream(), it's not needed because transport_mtu_config() will
> update the kernel buffer size via handle_block_size_change().
> update_buffer_size() can be removed altogheter.
>
> As a result the code in thread_func() will look like this, which I find
> much more self-explanatory:
>
> bool block_size_changed = codec->reduce_bitrate(&u->encoder_info, &u->write_block_size);
>
> if (block_size_changed)
> handle_block_size_change(u);
That is nice code cleanup. I'm doing it.
> > + size_t (*encode)(void **, uint32_t, const uint8_t *, size_t, uint8_t *, size_t);
> > + size_t (*decode)(void **, const uint8_t *, size_t, uint8_t *, size_t, size_t *);
> > +} pa_a2dp_codec_t;
>
> According to our coding style, struct names shouldn't contain the _t
> suffix. The _t suffix is only used with basic types like ints.
Fixed.
--
Pali Rohár
pali.rohar@xxxxxxxxx
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
