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@xxxxxxxxx> 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.
> > +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?
> > + /* 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.
> > + /* 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.
> > + 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.
> > + /* 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.
> > + 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.
> > + } 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.
> 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.
> > + 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@xxxxxxxxx
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
