[pulseaudio-discuss] [PATCH 1/4] A2DP api with multi-codec support

[Pali Rohár]

On Thursday 13 December 2018 19:43:36 EHfive wrote:
> +#define A2DP_CODEC_SBC			0x00
> +#define A2DP_CODEC_MPEG12		0x01
> +#define A2DP_CODEC_MPEG24		0x02
> +#define A2DP_CODEC_ATRAC		0x03

This is incorrect, Atrac codec has A2DP ID 0x04, see:
https://www.bluetooth.com/specifications/assigned-numbers/audio-video

> +#define MAX_BITPOOL 64
> +#define MIN_BITPOOL 2

These two constants are SBC specific, so make them with SBC_ prefix.

> +#define APTX_VENDOR_ID			    0x0000004f

This ID is allocated for APT Ltd. company. Not just for aptX codec. So
maybe better name is A2DP_VENDOR_APT_LIC_LTD?

> +#define APTX_CODEC_ID			    0x0001
> +
> +#define APTX_CHANNEL_MODE_MONO		0x01
> +#define APTX_CHANNEL_MODE_STEREO	0x02
> +
> +#define APTX_SAMPLING_FREQ_16000	0x08
> +#define APTX_SAMPLING_FREQ_32000	0x04
> +#define APTX_SAMPLING_FREQ_44100	0x02
> +#define APTX_SAMPLING_FREQ_48000	0x01
> +
> +#define LDAC_VENDOR_ID			0x0000012d

This ID is allocated for Sony Corporation company. Not just for LDAC
codec.

See: https://www.bluetooth.com/specifications/assigned-numbers/company-identifiers

> +#elif __BYTE_ORDER == __BIG_ENDIAN
> +
> +typedef struct {
> +	uint8_t frequency:4;
> +	uint8_t channel_mode:4;
> +	uint8_t block_length:4;
> +	uint8_t subbands:2;
> +	uint8_t allocation_method:2;
> +	uint8_t min_bitpool;
> +	uint8_t max_bitpool;
> +} __attribute__ ((packed)) a2dp_sbc_t;
> +
> +typedef struct {
> +	uint8_t layer:3;
> +	uint8_t crc:1;
> +	uint8_t channel_mode:4;
> +	uint8_t rfa:1;
> +	uint8_t mpf:1;
> +	uint8_t frequency:6;
> +	uint16_t bitrate;

This value for big endian systems seems to be broken. As you need to
store value in little endian. So maybe define as?

uint8_t bitrate_low;
uint8_t bitrate_high;

Or as

uint8_t bitrate[2];

Or better drop big endian support? Broken big endian support is not
useful at all.

> +static size_t
> +pa_sbc_decode(const void *read_buf, size_t read_buf_size, void *write_buf, size_t write_buf_size, size_t *_decoded,
> +              uint32_t *timestamp, void **codec_data) {
> +    const struct rtp_header *header;
> +    const struct rtp_payload *payload;
> +    const void *p;
> +    void *d;
> +    size_t to_write, to_decode;
> +    size_t total_written = 0;
> +    sbc_info_t *sbc_info = *codec_data;
> +    pa_assert(sbc_info);
> +
> +    header = read_buf;
> +    payload = (struct rtp_payload *) ((uint8_t *) read_buf + sizeof(*header));
> +
> +    *timestamp = ntohl(header->timestamp);
> +
> +    p = (uint8_t *) read_buf + sizeof(*header) + sizeof(*payload);
> +    to_decode = read_buf_size - sizeof(*header) - sizeof(*payload);
> +
> +    d = write_buf;
> +    to_write = write_buf_size;
> +
> +    *_decoded = 0;
> +    while (PA_LIKELY(to_decode > 0)) {
> +        size_t written;
> +        ssize_t decoded;
> +
> +        decoded = sbc_decode(&sbc_info->sbc,
> +                             p, to_decode,
> +                             d, to_write,
> +                             &written);
> +
> +        if (PA_UNLIKELY(decoded <= 0)) {
> +            pa_log_error("SBC decoding error (%li)", (long) decoded);
> +            *_decoded = 0;
> +            return 0;
> +        }
> +
> +        total_written += written;
> +
> +        /* Reset frame length, it can be changed due to bitpool change */
> +        sbc_info->frame_length = sbc_get_frame_length(&sbc_info->sbc);
> +
> +        pa_assert_fp((size_t) decoded <= to_decode);
> +        pa_assert_fp((size_t) decoded == sbc_info->frame_length);
> +
> +        pa_assert_fp((size_t) written == sbc_info->codesize);
> +
> +        *_decoded += decoded;
> +        p = (const uint8_t *) p + decoded;
> +        to_decode -= decoded;
> +
> +        d = (uint8_t *) d + written;
> +        to_write -= written;
> +    }
> +
> +    return total_written;
> +}

Seems that this decode procedure with while loop is similar/same for all
codecs. Months ago I sent to this mailing list different proposal for
codec API which tries to "fix" also this problem. Look into mailing list
archive for "[PATCH v2 1/2] Modular API for Bluetooth A2DP codec".

> +#define A2DP_SOURCE_ENDPOINT "/MediaEndpoint/A2DPSource"
> +#define A2DP_SINK_ENDPOINT "/MediaEndpoint/A2DPSink"
> +
> +#define A2DP_SBC_SRC_ENDPOINT A2DP_SOURCE_ENDPOINT "/SBC"
> +#define A2DP_SBC_SNK_ENDPOINT A2DP_SINK_ENDPOINT "/SBC"
> +
> +#define A2DP_VENDOR_SRC_ENDPOINT A2DP_SOURCE_ENDPOINT "/VENDOR"
> +#define A2DP_VENDOR_SNK_ENDPOINT A2DP_SINK_ENDPOINT "/VENDOR"

This would not work correctly. You need for each codec different
endpoint.

Also currently bluez does not allows you to choose which endpoint will
be used... As bluez does not provide API for it yet.

-- 
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/20181213/2867b355/attachment.sig>