[pulseaudio-discuss] [PATCH 1/4] A2DP api with multi-codec support
Huang-Huang Bao
eh5 at sokka.cn
Fri Dec 14 11:29:00 UTC 2018
On 12/14/18 7:01 PM, Pali Rohár wrote:
> On Friday 14 December 2018 18:48:05 Huang-Huang Bao wrote:
>> I don't want to debate too much about naming. In this state, we should pay attention to codes function.
>>
>> Broken codes and useless codes could be cleaned up after this patches set applied.
>>
>> If someone want to add another codec, he/she can just modify "a2dp-codeces.h" or others.
>>
>>
>> On 12/14/18 5:41 PM, Pali Rohár wrote:
>>> On Friday 14 December 2018 00:07:27 Huang-Huang Bao wrote:
>>>> "a2dp-codecs.h" copied from bluez, then added a2dp_aptxhd_t, a2dp_ldac_t.
>>> This does not excuse having broken code in pulseaudio. Atrac or big
>>> endian definition seems to be really wrong.
>>>
>>> Is not it stupid to copy+paste broken code from one project to another?
>>>
>>>> I don't want to debate too much about naming.
>>> Look, if other people comes up with something in the naming, then it
>>> should be also reflected.
>>>
>>> Bad naming just increase confusion for other people who start reading
>>> code later. This is the fact.
>>>
>>>> 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 .
>>> Then it is really needed to have "/VENDOR" string in the middle of the
>>> endpoint name? Because SBC codec does not have it. Is not
>>>
>>> A2DP_SOURCE_ENDPOINT "/" <codec_name>
>>>
>>> enough for endpoint name?
>> Because all vendor codecs share the single codec-id: 0xFF, which is at
>> the same level with SBC or AAC.
> But this does not answer my question. Why we need to have "VENDOR"
> string for every 0xFF codec in bluez dbus endpoint name? It does not
> bring any value. Bluez endpoint dbus name can be any unique string. So
> why not just use simple pattern /MediaEndpoint/A2DPSink/<my_codec_name>?
> Normally slash in dbus path is used for sub-resource (or something
> similar). But here SBC codec (at /MediaEndpoint/A2DPSink/SBC) has
> exactly same meaning as your (/MediaEndpoint/A2DPSink/VENDOR/APTX) aptX
> codec. So logically those two endpoints should be at same level
> /MediaEndpoint/A2DPSink/<--->.
>
> Bluetooth A2DP specification and its usage of 0xFF for other vendor
> codecs is just different layer which bluez/pulseaudio abstract. So it
> does not have to leak from one layer to another.
>
> Also pulseaudio source files do not have /.*vendor.*/ in codec names.
Bluez's media api:
Service unique name
Interface org.bluez.MediaEndpoint1
Object path freely definable
Methods array{byte} SelectConfiguration(array{byte} capabilities)
We can know codec type (SBC, AAC , ... , Vendor codecs) from object path
only.
Add "/VENDOR" make it simpler.
>
>>>> The result is, it support multi-codec, but can't switch to other codec.
>>> Yes, bluez is missing API for this switch.
>>>
>>>> Endpoint registered first has higher priority. (see enum
>>>> pa_a2dp_codec_index )
>>> Yes, but this priority takes effect only in the case computer creates
>>> connection to headset. NOT when headset initialize connection to
>>> computer.
>>>
>>> Therefore that priority list is not always enforced.
>> How do you know?
> Because I wrote my own implementation of aptX and FastStream codec
> support for pulseaudio. Also I played with bluez a lot. In summer I sent
> those my patches to pulseaudio mailing list, including proposal for
> pulseaudio codec API.
>
> And there was discussion about it. Also bluez developer confirmed that
> priority list is not applied when headset initialize connection.
>
> So for proper codec selection or fully working priority list, it is
> needed to implement this missing API in bluez first.
>
>> Have you try this patches set ?
> Note yet. But you are using exactly same bluez API and same logic for
> registering endpoints as I used. Therefore it behaves in same way.
>
>> Whatever, It works fine for me.
> This is not general argument or prove that something is working or not.
>
>>>>> Also currently bluez does not allows you to choose which endpoint will
>>>>> be used... As bluez does not provide API for it yet.
>>>>>
>>>> _______________________________________________
>>>> pulseaudio-discuss mailing list
>>>> pulseaudio-discuss at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
-------------- 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/6f65c5a1/attachment.sig>
More information about the pulseaudio-discuss
mailing list