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

[Huang-Huang Bao]

"a2dp-codecs.h" copied from bluez, then added a2dp_aptxhd_t, a2dp_ldac_t.

I don't want to debate too much about naming.

On 12/13/18 11:35 PM, Pali Rohár wrote:
> 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.

Actually, there do have multiple endpoints .

The result is, it support multi-codec, but can't switch to other codec.

Endpoint registered first has higher priority. (see enum
pa_a2dp_codec_index )

>
> Also currently bluez does not allows you to choose which endpoint will
> be used... As bluez does not provide API for it yet.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20181214/dbac0be6/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20181214/dbac0be6/attachment-0001.sig>