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. > >> 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 > > > -- Pali Rohár pali.rohar@xxxxxxxxx
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
