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@xxxxxxxxxxxxxxxxxxxxx >>>> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
