[pulseaudio-discuss] [PATCH v4 5/7] bluetooth: Modular API for A2DP codecs
Pali Rohár
pali.rohar at gmail.com
Sun Feb 3 14:34:20 UTC 2019
Hi! I fixed some problems in V6.
About naming conventions: I have not changed nothing right now. But
because function names and parameters does not change code itself, it
can be done later if more people agree that it is needed to change.
Basically for testing functionality it does not matter if function is
named A or B.
On Thursday 24 January 2019 18:17:08 Pali Rohár wrote:
> > > > > 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.
>
> There are lot of other improvements for SBC. I would let for next step.
Fixed in V6 in separate patch.
> > > > > + 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.
Fixed in v6 too.
> > 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?
>
> SBC 1 and SBC 2 would be two different entries in "pa_a2dp_codecs"
> table. So basically there are two different pulseaudio codecs. Therefore
> for each one is defined own profile. And user would see two profiles SBC
> 1 and SBC 2. Name depends on what is filled in codec_description.
>
> So user choose if he wants SBC 1 or SBC 2.
>
> I think they would be named "SBC low quality", "SBC high quality", "SBC
> auto quality". Also based on accept_capabilities() API function,
> pulseaudio will filter unsupported codecs, e.g. when advertised bitpool
> value is too low for pulseaudio's "SBC high quality".
Something is implemented in V6.
> > > > > + 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.
Fixed in V6.
> > > > > + 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.
Fixed in V6.
--
Pali Rohár
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20190203/5f2bdc7f/attachment-0001.sig>
More information about the pulseaudio-discuss
mailing list