[pulseaudio-discuss] [PATCH v4 5/7] bluetooth: Modular API for A2DP codecs
Luiz Augusto von Dentz
luiz.dentz at gmail.com
Thu Jan 24 16:54:18 UTC 2019
Hi Pali,
On Thu, Jan 24, 2019 at 6:00 PM Pali Rohár <pali.rohar at gmail.com> wrote:
>
> Hi!
>
> On Thursday 24 January 2019 16:03:17 Luiz Augusto von Dentz wrote:
> > On Tue, Jan 15, 2019 at 1:24 AM Pali Rohár <pali.rohar at gmail.com> wrote:
> > > +typedef struct pa_a2dp_codec_capabilities {
> > > + uint8_t size;
> > > + uint8_t buffer[]; /* max size is 254 bytes */
> > > +} pa_a2dp_codec_capabilities;
> > > +
> > > +typedef struct pa_a2dp_codec_id {
> > > + uint8_t codec_id;
> >
> > Can we not duplicate terms? The struct already is contain codec on its name.
> >
> > > + uint32_t vendor_id;
> > > + uint16_t vendor_codec_id;
> > > +} pa_a2dp_codec_id;
>
> So which names do you suggest? For identification of A2DP codec we need
> 3 values: A2DP codec id, A2DP vendor id, A2DP codec id of specific to
> vendor id.
>
> I thought that (codec_id, vendor_id, vendor_codec_id) is the best what
> can be invented.
You could actually just use vendor, codec, if the vendor is ff then
just use the codec, either way having to construct something like
codec_id->codec_id seems pretty long for no reason, codec_id->id is
not as bad.
> > > +typedef struct pa_a2dp_codec {
> > > + /* Unique name of the codec, lowercase and without whitespaces, used for constructing identifier, D-Bus paths, ... */
> > > + const char *codec_name;
> > > + /* Human readable codec description */
> > > + const char *codec_description;
> >
> > Ditto, using name and description here should be just as informative.
>
> So you want to have just "name" and "description" without "codec_"
> prefix?
Yep, or you thin codec->codec_name is not a bad pattern?
> > > + /* A2DP codec id */
> > > + pa_a2dp_codec_id codec_id;
> > > +
> > > + /* True if codec is bi-directional and supports backchannel */
> > > + bool support_backchannel;
> >
> > Id prefer using the term bidir here.
>
> "bidir" does not sounds like something obvious nor widely used. And in
> these cases I rather used more descriptive names.
Well other codecs may not call it a backchannel, but if you insist at
least drop the support term, this should be obvious for this type of
flag.
> > > + /* Returns true if codec accepts capabilities, for_encoding is true when capabilities are used for encoding */
> > > + bool (*accept_capabilities)(const uint8_t *capabilities_buffer, uint8_t capabilities_size, bool for_encoding);
> > > + /* Choose preferred capabilities from hash map (const char * -> const pa_a2dp_codec_capabilities *) and returns corresponding key, for encoder is true when capabilities hash map is used for encoding */
> >
> > accept perhaps is enough here as there are nothing else to accept.
>
> Better name could be "can_accept_capabilites". As it really does not
> accept.
You really like long names, Im more for short and consistent name
specially for structs.
> > > + const char *(*choose_capabilities)(const pa_hashmap *capabilities_hashmap, bool for_encoding);
> >
> > Id perfer using the same terminology as used in D-Bus, thus select
> > probably fits better here.
>
> I thought that names "fill_capabilities" or "validate_configuration"
> better describe what functions are doing.
These are used locally only, anyway there are so many duplicates
terms, like sample_spec, buffer, not sure what is the matter with
that.
> > > + /* Fill codec capabilities, returns size of filled buffer */
> > > + uint8_t (*fill_capabilities)(uint8_t capabilities_buffer[254]);
> > > + /* Validate codec configuration, returns true on success */
> > > + bool (*validate_configuration)(const uint8_t *config_buffer, uint8_t config_size);
> > > + /* Fill preferred codec configuration, returns size of filled buffer */
> > > + uint8_t (*fill_preferred_configuration)(const pa_sample_spec *default_sample_spec, const uint8_t *capabilities_buffer, uint8_t capabilities_size, uint8_t config_buffer[254]);
> > > +
> > > + /* Initialize codec, returns codec info data and set sample_spec, for_encoding is true when codec_info is used for encoding, for_backchannel is true when codec_info is used for backchannel */
> > > + void *(*init_codec)(bool for_encoding, bool for_backchannel, const uint8_t *config_buffer, uint8_t config_size, pa_sample_spec *sample_spec);
> > > + /* Deinitialize and release codec info data in codec_info */
> > > + void (*finish_codec)(void *codec_info);
> > > + /* Reset internal state of codec info data in codec_info */
> > > + void (*reset_codec)(void *codec_info);
> >
> > init, deinit and reset.
> >
> > > + /* Get read block size for codec */
> > > + size_t (*get_read_block_size)(void *codec_info, size_t read_link_mtu);
> > > + /* Get write block size for codec */
> > > + size_t (*get_write_block_size)(void *codec_info, size_t write_link_mtu);
> > > +
> > > + /* Reduce encoder bitrate for codec, returns new write block size or zero if not changed, called when socket is not accepting encoded data fast enough */
> > > + size_t (*reduce_encoder_bitrate)(void *codec_info, size_t write_link_mtu);
> >
> > Perhaps a set_bitrate would be better instead of just calling it
> > reduce,
>
> Usage of name "reduce" was suggested by Tanu in V2.
>
> > but then we actually need to provide the current bitrate, the
> > codec can then check if it attend the current configuration and if not
> > adjust the parameters. This may change the latency as well but I far I
> > remember we do adjust it on device module but we may need to
> > incorporate a call to get the algorithm latency here as well.
> >
> > > + /* Encode input_buffer of input_size to output_buffer of output_size, returns size of filled ouput_buffer and set processed to size of processed input_buffer */
> > > + size_t (*encode_buffer)(void *codec_info, uint32_t timestamp, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size, size_t *processed);
> > > + /* Decode input_buffer of input_size to output_buffer of output_size, returns size of filled ouput_buffer and set processed to size of processed input_buffer */
> > > + size_t (*decode_buffer)(void *codec_info, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size, size_t *processed);
> >
> > encode, decode?
> >
> > > +} pa_a2dp_codec;
> > > +
> > > +#endif
> > > diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c b/src/modules/bluetooth/a2dp-codec-sbc.c
> > > new file mode 100644
> > > index 000000000..d84ad3c9f
> > > --- /dev/null
> > > +++ b/src/modules/bluetooth/a2dp-codec-sbc.c
> > > +static uint8_t fill_capabilities(uint8_t capabilities_buffer[254]) {
> > > + a2dp_sbc_t *capabilities = (a2dp_sbc_t *) capabilities_buffer;
> > > +
> > > + pa_zero(*capabilities);
> > > +
> > > + capabilities->channel_mode = SBC_CHANNEL_MODE_MONO | SBC_CHANNEL_MODE_DUAL_CHANNEL | SBC_CHANNEL_MODE_STEREO |
> > > + SBC_CHANNEL_MODE_JOINT_STEREO;
> > > + capabilities->frequency = SBC_SAMPLING_FREQ_16000 | SBC_SAMPLING_FREQ_32000 | SBC_SAMPLING_FREQ_44100 |
> > > + SBC_SAMPLING_FREQ_48000;
> > > + capabilities->allocation_method = SBC_ALLOCATION_SNR | SBC_ALLOCATION_LOUDNESS;
> > > + capabilities->subbands = SBC_SUBBANDS_4 | SBC_SUBBANDS_8;
> > > + capabilities->block_length = SBC_BLOCK_LENGTH_4 | SBC_BLOCK_LENGTH_8 | SBC_BLOCK_LENGTH_12 | SBC_BLOCK_LENGTH_16;
> > > + capabilities->min_bitpool = SBC_MIN_BITPOOL;
> > > + capabilities->max_bitpool = SBC_MAX_BITPOOL;
> >
> > SBC_MAX_BITPOOL is 64 but we only select 53, we should probably fix this.
>
> This code is already in pulseaudio tree. I just moved functions to new
> file name.
>
> So I would rather not change existing codec parameters in patch which
> just introduce new API.
>
> Improvements to SBC codec can be done later after this patch series.
Sure thing, I can send a patch for that as well was just pointing out
it is inconsistent regardless where it came from.
> > > + return sizeof(*capabilities);
> > > +}
> ...
> > > +static uint8_t default_bitpool(uint8_t freq, uint8_t mode) {
> > > + /* These bitpool values were chosen based on the A2DP spec recommendation */
> > > + switch (freq) {
> > > + case SBC_SAMPLING_FREQ_16000:
> > > + case SBC_SAMPLING_FREQ_32000:
> > > + return 53;
> > > +
> > > + case SBC_SAMPLING_FREQ_44100:
> > > +
> > > + switch (mode) {
> > > + case SBC_CHANNEL_MODE_MONO:
> > > + case SBC_CHANNEL_MODE_DUAL_CHANNEL:
> > > + return 31;
> > > +
> > > + case SBC_CHANNEL_MODE_STEREO:
> > > + case SBC_CHANNEL_MODE_JOINT_STEREO:
> > > + return 53;
> >
> > Change to return SBC_MAX_BITPOOL.
> >
> > > + }
> > > +
> > > + pa_log_warn("Invalid channel mode %u", mode);
> > > + return 53;
> >
> > Ditto.
> >
> > > + case SBC_SAMPLING_FREQ_48000:
> > > +
> > > + switch (mode) {
> > > + case SBC_CHANNEL_MODE_MONO:
> > > + case SBC_CHANNEL_MODE_DUAL_CHANNEL:
> > > + return 29;
> > > +
> > > + case SBC_CHANNEL_MODE_STEREO:
> > > + case SBC_CHANNEL_MODE_JOINT_STEREO:
> > > + return 51;
> >
> > Ditto.
> >
> > > + }
> > > +
> > > + pa_log_warn("Invalid channel mode %u", mode);
> > > + return 51;
> >
> > Ditto.
> >
> > > + }
> > > +
> > > + pa_log_warn("Invalid sampling freq %u", freq);
> > > + return 53;
> >
> > Ditto.
>
> Same there. This is just moving function from one file to another. I
> would not expect that moving code changes also logic of parameters in
> codec.
>
> > > +}
> ...
> > > + set_params(sbc_info);
> > > +
> > > + PA_ONCE_BEGIN {
> > > + pa_log_debug("Using SBC codec implementation: %s", pa_strnull(sbc_get_implementation_info(&sbc_info->sbc)));
> >
> > This was printing NULL to me which I suspect is the reason why you
> > have pa_strnull(?), anyway it is not enough to just call sbc_init
> > since that doesn't call sbc_init_primitives. We could in theory change
> > that to be part of sbc_init instead of on the first call to
> > sbc_encode.
>
> Also this code block is just moving from one file to another.
>
> I do not know why pa_strnull was there before.
Seems this has been broken for a while.
> > > + } PA_ONCE_END;
> > > +
> > > + pa_log_info("SBC parameters: allocation=%u, subbands=%u, blocks=%u, bitpool=%u",
> > > + sbc_info->sbc.allocation, sbc_info->sbc.subbands ? 8 : 4, sbc_info->sbc.blocks, sbc_info->sbc.bitpool);
> > > +
> > > + return sbc_info;
> > > +}
> ...
> > > +const pa_a2dp_codec pa_a2dp_codec_sbc = {
> > > + .codec_name = "sbc",
> > > + .codec_description = "SBC",
> > > + .codec_id = { A2DP_CODEC_SBC, 0, 0 },
> >
> > You can declare without the 0, 0, by default the compiler should fill
> > the remaining fields as 0.
>
> I just wanted to be explicit. But I can remove them.
>
> > > + .support_backchannel = false,
> > > + .accept_capabilities = accept_capabilities,
> > > + .choose_capabilities = choose_capabilities,
> > > + .fill_capabilities = fill_capabilities,
> > > + .validate_configuration = validate_configuration,
> > > + .fill_preferred_configuration = fill_preferred_configuration,
> > > + .init_codec = init_codec,
> > > + .finish_codec = finish_codec,
> > > + .reset_codec = reset_codec,
> > > + .get_read_block_size = get_block_size,
> > > + .get_write_block_size = get_block_size,
> > > + .reduce_encoder_bitrate = reduce_encoder_bitrate,
> > > + .encode_buffer = encode_buffer,
> > > + .decode_buffer = decode_buffer,
> > > +};
> ...
> > > diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
> > > index 48b147eed..9885ba461 100644
> > > --- a/src/modules/bluetooth/bluez5-util.c
> > > +++ b/src/modules/bluetooth/bluez5-util.c
> > > @@ -508,6 +540,42 @@ static int parse_transport_properties(pa_bluetooth_transport *t, DBusMessageIter
> > > return 0;
> > > }
> > >
> > > +static unsigned pa_a2dp_codec_id_hash_func(const void *_p) {
> > > + unsigned hash;
> > > + const pa_a2dp_codec_id *p = _p;
> > > +
> > > + hash = p->codec_id;
> > > + hash = 31 * hash + ((p->vendor_id >> 0) & 0xFF);
> > > + hash = 31 * hash + ((p->vendor_id >> 8) & 0xFF);
> > > + hash = 31 * hash + ((p->vendor_id >> 16) & 0xFF);
> > > + hash = 31 * hash + ((p->vendor_id >> 24) & 0xFF);
> > > + hash = 31 * hash + ((p->vendor_codec_id >> 0) & 0xFF);
> > > + hash = 31 * hash + ((p->vendor_codec_id >> 8) & 0xFF);
> > > + return hash;
> > > +}
> > > +
> > > +static int pa_a2dp_codec_id_compare_func(const void *_a, const void *_b) {
> > > + const pa_a2dp_codec_id *a = _a;
> > > + const pa_a2dp_codec_id *b = _b;
> > > +
> > > + if (a->codec_id < b->codec_id)
> > > + return -1;
> > > + if (a->codec_id > b->codec_id)
> > > + return 1;
> > > +
> > > + if (a->vendor_id < b->vendor_id)
> > > + return -1;
> > > + if (a->vendor_id > b->vendor_id)
> > > + return 1;
> > > +
> > > + if (a->vendor_codec_id < b->vendor_codec_id)
> > > + return -1;
> > > + if (a->vendor_codec_id > b->vendor_codec_id)
> > > + return 1;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static pa_bluetooth_device* device_create(pa_bluetooth_discovery *y, const char *path) {
> > > pa_bluetooth_device *d;
> > >
> > > @@ -518,6 +586,11 @@ static pa_bluetooth_device* device_create(pa_bluetooth_discovery *y, const char
> > > d->discovery = y;
> > > d->path = pa_xstrdup(path);
> > > d->uuids = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, NULL, pa_xfree);
> > > + d->a2dp_sink_endpoints = pa_hashmap_new_full(pa_a2dp_codec_id_hash_func, pa_a2dp_codec_id_compare_func, pa_xfree, (pa_free_cb_t)pa_hashmap_free);
> > > + d->a2dp_source_endpoints = pa_hashmap_new_full(pa_a2dp_codec_id_hash_func, pa_a2dp_codec_id_compare_func, pa_xfree, (pa_free_cb_t)pa_hashmap_free);
> >
> > This sounds a bit weird, you have a hash function which creates a hash
> > with the codec and vendor details, but the compare function don't use
> > it?
>
> Why you think it does not use it? Look few lines above. Both hash and
> compare function use codec and vendor information.
>
> > Perhaps it would have been better to just use the endpoint path
> > instead?
>
> I cannot. Endpoint path does not say for which codec it belongs. This is
> reason why hash table which has key of codec/vendor information is
> needed.
Not sure what do mean that you cannot, the element data do contain
that information perhaps that is just pa_hashmap, anyway it now makes
sense it just much more complex to do this way since you also have to
do lookups when and endpoint is removed, actually many lookups one for
each codec and it doesn't even stop when it find the endpoint, you
should probably fix that.
> > Actually I think you must do that in order to able to address
> > endpoint separately as it is valid to have the same codec in multiple
> > endpoints but for your hash function they would be identical.
>
> This is used on second level of structures.
>
> Seems you have not caught complete structure. Look into header file
> where is comment for these hash tables. It is two level hash table.
>
> Basically it is hash table with function:
>
> (codec_id, vendor_id, codec_vendor_id) -> hash_table_2
>
> Where hash_table_2 is with function:
>
> endpoint_path -> codec_capabilities
>
> This allows me to find all endpoints which belongs to specific A2DP
> codec. And then for each endpoint I have A2DP codec capabilities.
>
> When user want to switch to new A2DP profile, I get information which
> register pulseaudio codec should be used. From it I receive A2DP codec
> information (codec_id, vendor_id, codec_vendor_id) and identify all
> endpoints which are compatible. After that I ask pulseuadio codec to
> choose from second level hash table which endpoint to use, based on
> capabilities (as second level is hash table endpoint --> capability).
> Pulseaudio codec tells me endpoint and then I instruct bluez to switch
> to that endpoint.
So you want to switch by codec not by endpoint? How do you know if the
user want to switch to SBC 1 with bitpool 64 or SBC 2 with bitpool 32?
So headsets do that actually and Im not really sure how we would be
able to identify what the user wants to use, having it duplicated in
the list of profiles is also bad but at least the user is able to tell
there are 2 endpoints mapped to the same codec otherwise we would have
to do some guessing on which endpoint shall be used, best match, first
match?
> > > + d->a2dp_sink_codecs = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
> > > + d->a2dp_source_codecs = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
> > > + d->transports = pa_xnew0(pa_bluetooth_transport *, pa_bluetooth_profile_count());
> > >
> > > pa_hashmap_put(y->devices, d->path, d);
> > >
> > > @@ -553,14 +626,53 @@ pa_bluetooth_device* pa_bluetooth_discovery_get_device_by_address(pa_bluetooth_d
> > > return NULL;
> > > }
> > >
> > > +static char *remote_endpoint_path_to_device_path(const char *remote_endpoint_path) {
> > > + char *endptr;
> > > +
> > > + endptr = strrchr(remote_endpoint_path, '/');
> >
> > There exists a property called Device exactly to avoid this kind of assumption.
>
> Ok, I will look at it.
>
> > > + if (!endptr) {
> > > + pa_log_error("Invalid remote endpoint %s", remote_endpoint_path);
> > > + return NULL;
> > > + }
> > > +
> > > + return pa_xstrndup(remote_endpoint_path, endptr-remote_endpoint_path);
> > > +}
> > > +
> > > +static void remote_endpoint_remove(pa_bluetooth_discovery *y, const char *path) {
> > > + pa_bluetooth_device *device;
> > > + pa_hashmap *endpoints;
> > > + char *device_path;
> > > + void *state;
> > > +
> > > + device_path = remote_endpoint_path_to_device_path(path);
> > > + if (!device_path)
> > > + return;
> > > +
> > > + device = pa_hashmap_get(y->devices, device_path);
> > > + if (!device)
> > > + pa_log_warn("Remote endpoint %s for unknown device removed %s", path, device_path);
> > > +
> > > + pa_xfree(device_path);
> > > +
> > > + if (!device)
> > > + return;
> >
> > You can probably rework these checks with the device once you actually
> > resolve the device when creating the endpoint object.
> >
> > > + PA_HASHMAP_FOREACH(endpoints, device->a2dp_sink_endpoints, state)
> > > + pa_hashmap_remove_and_free(endpoints, path);
> >
> > Now I totally lost, does the hash keys are path or a hash of codec id
> > and vendor details?
>
> Look information above. Keys in first level hash table are codec/vendor
> details and keys in second level hash table are paths.
>
> > > + PA_HASHMAP_FOREACH(endpoints, device->a2dp_source_endpoints, state)
> > > + pa_hashmap_remove_and_free(endpoints, path);
> > > +}
> > > +
> ...
> > > + endpoints = pa_hashmap_get(codec_endpoints, a2dp_codec_id);
> > > + if (!endpoints) {
> > > + endpoints = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, pa_xfree, pa_xfree);
> > > + pa_hashmap_put(codec_endpoints, a2dp_codec_id, endpoints);
> > > + }
> > > +
> > > + pa_hashmap_put(endpoints, pa_xstrdup(endpoint), a2dp_codec_capabilities);
> >
> > Just to make sure Ive checked what pa_hashmap_put do, it does call the
> > hash_func which in this case is pa_a2dp_codec_id_hash_func
>
> No. It is pa_idxset_string_hash_func function. See 4 lines above.
>
> > which
> > expects a struct pa_a2dp_codec_id * not a string, you were probably
> > lucky that the string contents is big enough.
> >
> > > +}
>
> --
> Pali Rohár
> pali.rohar at gmail.com
--
Luiz Augusto von Dentz
More information about the pulseaudio-discuss
mailing list