[pulseaudio-discuss] [RFC - MP3 passthrough over A2DP 2/2] mp3 passthrough: Bluetooth changes
Tanu Kaskinen
tanuk at iki.fi
Tue Oct 5 10:16:23 PDT 2010
Hi,
Patch review below:
On Tue, 2010-09-21 at 18:00 -0500, bossart.nospam at gmail.com wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart at intel.com>
>
>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at intel.com>
>
> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
> index 61fe369..d12c4e3 100644
> --- a/src/modules/bluetooth/module-bluetooth-device.c
> +++ b/src/modules/bluetooth/module-bluetooth-device.c
> @@ -109,6 +109,9 @@ static const char* const valid_modargs[] = {
>
> struct a2dp_info {
> sbc_capabilities_t sbc_capabilities;
> + mpeg_capabilities_t mpeg_capabilities;
> + pa_bool_t has_mpeg;
> +
> sbc_t sbc; /* Codec data */
> pa_bool_t sbc_initialized; /* Keep track if the encoder is initialized */
> size_t codesize, frame_length; /* SBC Codesize, frame_length. We simply cache those values here */
> @@ -131,6 +134,7 @@ struct hsp_info {
>
> enum profile {
> PROFILE_A2DP,
> + PROFILE_A2DP_PASSTHROUGH,
> PROFILE_A2DP_SOURCE,
> PROFILE_HSP,
> PROFILE_HFGW,
> @@ -157,7 +161,7 @@ struct userdata {
> pa_rtpoll_item *rtpoll_item;
> pa_thread *thread;
>
> - uint64_t read_index, write_index;
> + uint64_t s_read_index, s_write_index; /* count in samples instead of bytes */
The unit seems to be actually pcm frames, not samples.
> pa_usec_t started_at;
> pa_smoother *read_smoother;
>
> @@ -169,7 +173,8 @@ struct userdata {
> int stream_fd;
>
> size_t link_mtu;
> - size_t block_size;
> + size_t block_size; /* in bytes */
> + size_t samples_per_block; /* in samples */
Apparently samples_per_block is not in samples but in frames. Please
rename this variable (I suggest "pcm_frames_per_block" to distinguish
from all other frame types).
>
> struct a2dp_info a2dp;
> struct hsp_info hsp;
> @@ -182,8 +187,22 @@ struct userdata {
> int service_write_type, service_read_type;
>
> pa_bool_t filter_added;
> +
> + /* required for PASSTHROUGH profile */
> + size_t leftover_bytes;
> + pa_memchunk leftover_memchunk; /* storage for bytes that could not be sent
> + in the last packet */
> +
> +
> };
>
> +static pa_usec_t samples_to_usec(uint64_t length, const pa_sample_spec *spec) {
The function name is incorrect, should be frames_to_usec (or
pcm_frames_to_usec to distinguish from other frame types).
> + pa_assert(spec);
> + pa_return_val_if_fail(pa_sample_spec_valid(spec), 0);
Why do you want to accept (i.e. not use assertion) invalid sample specs?
> +
> + return ((pa_usec_t) (length * PA_USEC_PER_SEC) / spec->rate);
> +}
> +
> #define FIXED_LATENCY_PLAYBACK_A2DP (25*PA_USEC_PER_MSEC)
> #define FIXED_LATENCY_RECORD_A2DP (25*PA_USEC_PER_MSEC)
> #define FIXED_LATENCY_PLAYBACK_HSP (125*PA_USEC_PER_MSEC)
> @@ -300,6 +319,68 @@ static ssize_t service_expect(struct userdata*u, bt_audio_msg_header_t *rsp, siz
> }
>
> /* Run from main thread */
> +static int parse_mpeg_caps(struct userdata *u, uint8_t seid, const struct bt_get_capabilities_rsp *rsp) {
> + uint16_t bytes_left;
> + const codec_capabilities_t *codec;
> + pa_bool_t has_mpeg=FALSE;
> +
> + pa_assert(u);
> + pa_assert(rsp);
> + pa_assert( u->profile == PROFILE_A2DP_PASSTHROUGH );
No spaces after "(" and before ")".
> +
> + bytes_left = rsp->h.length - sizeof(*rsp);
> +
> + if (bytes_left < sizeof(codec_capabilities_t)) {
> + pa_log_error("Packet too small to store codec information.");
> + return -1;
> + }
> +
> + codec = (codec_capabilities_t *) rsp->data; /** ALIGNMENT? **/
codec_capabilities_t contains only 8-bit variables, which to my
understanding guarantees that it doesn't matter where the data happens
to be located in the memory. If you agree (I don't consider myself an
alignment expert), then I suggest you remove the "ALIGNMENT?" comment
from here and from parse_caps.
> +
> + pa_log_debug("Payload size is %lu %lu", (unsigned long) bytes_left, (unsigned long) sizeof(*codec));
> +
> + if ((u->profile == PROFILE_A2DP_PASSTHROUGH) && codec->transport != BT_CAPABILITIES_TRANSPORT_A2DP) {
Illogical parentheses use: both conditions test equality, but only one
of them has extra parentheses. I don't care if you add parentheses to
the latter condition or remove them from the first :)
> + pa_log_error("Got capabilities for wrong codec.");
> + return -1;
> + }
> +
> + while (bytes_left > 0) {
> + if ((codec->type == BT_A2DP_MPEG12_SINK) && !codec->lock) {
> + has_mpeg = TRUE;
> + break;
> + }
> + bytes_left -= codec->length;
> + codec = (const codec_capabilities_t*) ((const uint8_t*) codec + codec->length);
If the previous alignment comment did have a point, then surely this is
broken too. So, if you don't remove the "ALIGNMENT?" comment above, I
suggest you add such comment here also (or change the code so that there
is no chance of alignment problems). If you do any changes here, do them
also in parse_caps.
> + }
> +
> + if (bytes_left <= 0 || codec->length != sizeof(u->a2dp.mpeg_capabilities))
> + return -1;
> +
> + pa_assert(codec->type == BT_A2DP_MPEG12_SINK);
> +
> + if (codec->configured && seid == 0)
> + return codec->seid;
> +
> + has_mpeg = TRUE;
> + memcpy(&u->a2dp.mpeg_capabilities, codec, sizeof(u->a2dp.mpeg_capabilities));
> +
> + pa_log_info("MPEG caps detected");
> + pa_log_info("channel_mode %d crc %d layer %d frequency %d mpf %d bitrate %d",
> + u->a2dp.mpeg_capabilities.channel_mode,
> + u->a2dp.mpeg_capabilities.crc,
> + u->a2dp.mpeg_capabilities.layer,
> + u->a2dp.mpeg_capabilities.frequency,
> + u->a2dp.mpeg_capabilities.mpf,
> + u->a2dp.mpeg_capabilities. bitrate);
Extra space in "u->a2dp.mpeg_capabilities. bitrate".
> +
> + u->a2dp.has_mpeg = has_mpeg;
As far as I can see, the local has_mpeg variable is redundant; you can
use "u->a2dp.has_mpeg = TRUE" here.
> +
> + return 0;
> +}
> +
> +
> +
Extra lines.
> +/* Run from main thread */
> static int parse_caps(struct userdata *u, uint8_t seid, const struct bt_get_capabilities_rsp *rsp) {
> uint16_t bytes_left;
> const codec_capabilities_t *codec;
> @@ -380,44 +461,91 @@ static int parse_caps(struct userdata *u, uint8_t seid, const struct bt_get_capa
> return 0;
> }
>
> -/* Run from main thread */
> -static int get_caps(struct userdata *u, uint8_t seid) {
> - union {
> - struct bt_get_capabilities_req getcaps_req;
> - struct bt_get_capabilities_rsp getcaps_rsp;
> - bt_audio_error_t error;
> - uint8_t buf[BT_SUGGESTED_BUFFER_SIZE];
> - } msg;
> - int ret;
> +typedef union {
> + struct bt_get_capabilities_req getcaps_req;
> + struct bt_get_capabilities_rsp getcaps_rsp;
> + bt_audio_error_t error;
> + uint8_t buf[BT_SUGGESTED_BUFFER_SIZE];
> +} bt_getcaps_msg_t;
> +
> +
Extra line.
> +static int get_caps_msg(struct userdata *u, uint8_t seid, bt_getcaps_msg_t *msg) {
The "Run from main thread" comment is dropped.
>
> pa_assert(u);
>
> - memset(&msg, 0, sizeof(msg));
> - msg.getcaps_req.h.type = BT_REQUEST;
> - msg.getcaps_req.h.name = BT_GET_CAPABILITIES;
> - msg.getcaps_req.h.length = sizeof(msg.getcaps_req);
> - msg.getcaps_req.seid = seid;
> -
> - pa_strlcpy(msg.getcaps_req.object, u->path, sizeof(msg.getcaps_req.object));
> - if (u->profile == PROFILE_A2DP || u->profile == PROFILE_A2DP_SOURCE)
> - msg.getcaps_req.transport = BT_CAPABILITIES_TRANSPORT_A2DP;
> + memset(msg, 0, sizeof(bt_getcaps_msg_t));
> + msg->getcaps_req.h.type = BT_REQUEST;
> + msg->getcaps_req.h.name = BT_GET_CAPABILITIES;
> + msg->getcaps_req.h.length = sizeof(struct bt_get_capabilities_req);
> + msg->getcaps_req.seid = seid;
> +
> + pa_strlcpy(msg->getcaps_req.object, u->path, sizeof(msg->getcaps_req.object));
> + if (u->profile == PROFILE_A2DP || u->profile == PROFILE_A2DP_PASSTHROUGH ||
> + u->profile == PROFILE_A2DP_SOURCE)
> + msg->getcaps_req.transport = BT_CAPABILITIES_TRANSPORT_A2DP;
> else {
> pa_assert(u->profile == PROFILE_HSP || u->profile == PROFILE_HFGW);
> - msg.getcaps_req.transport = BT_CAPABILITIES_TRANSPORT_SCO;
> + msg->getcaps_req.transport = BT_CAPABILITIES_TRANSPORT_SCO;
> }
> - msg.getcaps_req.flags = u->auto_connect ? BT_FLAG_AUTOCONNECT : 0;
> + msg->getcaps_req.flags = u->auto_connect ? BT_FLAG_AUTOCONNECT : 0;
>
> - if (service_send(u, &msg.getcaps_req.h) < 0)
> + if (service_send(u, &msg->getcaps_req.h) < 0)
> return -1;
>
> - if (service_expect(u, &msg.getcaps_rsp.h, sizeof(msg), BT_GET_CAPABILITIES, 0) < 0)
> + if (service_expect(u, &msg->getcaps_rsp.h, sizeof(bt_getcaps_msg_t), BT_GET_CAPABILITIES, 0) < 0) {
> + pa_log("BT_GET_CAPABILITIES expect failed");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +/* Run from main thread */
> +static int get_caps(struct userdata *u, uint8_t seid) {
> + bt_getcaps_msg_t msg;
> + int ret;
> +
> + pa_assert(u);
> +
> + if( get_caps_msg(u,seid,&msg) < 0)
Put space after "if" but not after "(". Put spaces after commas.
> return -1;
>
> ret = parse_caps(u, seid, &msg.getcaps_rsp);
> - if (ret <= 0)
> - return ret;
> + if (ret < 0)
> + return -1;
> +
> + if( ret > 0 ) {
Fix spaces.
> + /* refine seid caps */
> + if( get_caps_msg(u,ret,&msg) < 0)
Fix spaces.
> + return -1;
> +
> + ret = parse_caps(u, ret, &msg.getcaps_rsp);
> + if (ret < 0)
> + return -1;
> + }
> +
> + if (u->profile == PROFILE_A2DP_PASSTHROUGH ) {
Fix spaces.
> + seid = 0;
>
> - return get_caps(u, ret);
> + /* try to find mpeg end-point */
> + if( get_caps_msg(u,seid,&msg) < 0)
Fix spaces.
> + return -1;
> +
> + ret = parse_mpeg_caps(u, seid, &msg.getcaps_rsp);
> + if (ret < 0)
> + return -1;
> +
> + if( ret > 0 ) {
Fix spaces
> + /* refine seid caps */
> + if( get_caps_msg(u,ret,&msg) < 0)
Fix spaces.
> + return -1;
> +
> + ret = parse_mpeg_caps(u, ret, &msg.getcaps_rsp);
> + if (ret < 0)
> + return -1;
> + }
> + }
> + return 0;
> }
>
> /* Run from main thread */
> @@ -482,95 +610,142 @@ static int setup_a2dp(struct userdata *u) {
> };
>
> pa_assert(u);
> - pa_assert(u->profile == PROFILE_A2DP || u->profile == PROFILE_A2DP_SOURCE);
> + pa_assert(u->profile == PROFILE_A2DP || u->profile == PROFILE_A2DP_PASSTHROUGH || u->profile == PROFILE_A2DP_SOURCE);
> +
> +
> + if (u->profile == PROFILE_A2DP_PASSTHROUGH) {
> + mpeg_capabilities_t *mcap;
> +
> + if (u->a2dp.has_mpeg) {
> + int rate;
> +
> + mcap = &u->a2dp.mpeg_capabilities;
> + rate = u->sample_spec.rate;
> +
> + if( u->sample_spec.channels == 1)
Fix spaces.
> + mcap->channel_mode = BT_A2DP_CHANNEL_MODE_MONO;
> + else
> + mcap->channel_mode = BT_A2DP_CHANNEL_MODE_STEREO;
> +
> + mcap->crc = 0; /* CRC is broken in some encoders */
> + mcap->layer = BT_MPEG_LAYER_3;
> +
> + if (rate == 44100)
> + mcap->frequency = BT_MPEG_SAMPLING_FREQ_44100;
> + else if (rate == 48000)
> + mcap->frequency = BT_MPEG_SAMPLING_FREQ_48000;
> + else if (rate == 32000)
> + mcap->frequency = BT_MPEG_SAMPLING_FREQ_32000;
> + else if (rate == 24000)
> + mcap->frequency = BT_MPEG_SAMPLING_FREQ_24000;
> + else if (rate == 22050)
> + mcap->frequency = BT_MPEG_SAMPLING_FREQ_22050;
> + else if (rate == 16000)
> + mcap->frequency = BT_MPEG_SAMPLING_FREQ_16000;
> + else {
> + pa_log("unsupported sampling frequency");
> + return -1;
> + }
>
> - cap = &u->a2dp.sbc_capabilities;
> + mcap->mpf = 0; /* mpf - we will only use the mandatory one */
>
> - /* Find the lowest freq that is at least as high as the requested
> - * sampling rate */
> - for (i = 0; (unsigned) i < PA_ELEMENTSOF(freq_table); i++)
> - if (freq_table[i].rate >= u->sample_spec.rate && (cap->frequency & freq_table[i].cap)) {
> - u->sample_spec.rate = freq_table[i].rate;
> - cap->frequency = freq_table[i].cap;
> - break;
> + /* vbr - we always say its vbr, we don't have how to know it */
> + mcap->bitrate = 0x8000;
I'm not sure what "we don't have how to know it" tries to say.
Also, does every BT device that supports MP3 support VBR?
> +
> + }
> + else {
> + pa_log("setup_a2dp: Trying to set-up A2DP Passthrough configuration but no MPEG endpoint available");
> + return -1; /* this should not happen */
If it happens anyway, can the bug be anywhere else than in Pulseaudio's
code? If not, use an assertion.
> }
> + } else {
Like the mcap variable is defined inside the if block, I would move the
variables i and cap inside this else block, since they're not used
elsewhere.
>
> - if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
> - for (--i; i >= 0; i--) {
> - if (cap->frequency & freq_table[i].cap) {
> + cap = &u->a2dp.sbc_capabilities;
> +
> + /* Find the lowest freq that is at least as high as the requested
> + * sampling rate */
> + for (i = 0; (unsigned) i < PA_ELEMENTSOF(freq_table); i++)
> + if (freq_table[i].rate >= u->sample_spec.rate && (cap->frequency & freq_table[i].cap)) {
> u->sample_spec.rate = freq_table[i].rate;
> cap->frequency = freq_table[i].cap;
> break;
> }
> - }
>
> - if (i < 0) {
> - pa_log("Not suitable sample rate");
> - return -1;
> + if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
> + for (--i; i >= 0; i--) {
> + if (cap->frequency & freq_table[i].cap) {
> + u->sample_spec.rate = freq_table[i].rate;
> + cap->frequency = freq_table[i].cap;
> + break;
> + }
> + }
> +
> + if (i < 0) {
> + pa_log("Not suitable sample rate");
> + return -1;
> + }
> }
> - }
>
> - pa_assert((unsigned) i < PA_ELEMENTSOF(freq_table));
> + pa_assert((unsigned) i < PA_ELEMENTSOF(freq_table));
>
> - if (cap->capability.configured)
> - return 0;
> + if (cap->capability.configured)
> + return 0;
>
> - if (u->sample_spec.channels <= 1) {
> - if (cap->channel_mode & BT_A2DP_CHANNEL_MODE_MONO) {
> - cap->channel_mode = BT_A2DP_CHANNEL_MODE_MONO;
> - u->sample_spec.channels = 1;
> - } else
> + if (u->sample_spec.channels <= 1) {
> + if (cap->channel_mode & BT_A2DP_CHANNEL_MODE_MONO) {
> + cap->channel_mode = BT_A2DP_CHANNEL_MODE_MONO;
> + u->sample_spec.channels = 1;
> + } else
> + u->sample_spec.channels = 2;
> + }
> +
> + if (u->sample_spec.channels >= 2) {
> u->sample_spec.channels = 2;
> - }
>
> - if (u->sample_spec.channels >= 2) {
> - u->sample_spec.channels = 2;
> + if (cap->channel_mode & BT_A2DP_CHANNEL_MODE_JOINT_STEREO)
> + cap->channel_mode = BT_A2DP_CHANNEL_MODE_JOINT_STEREO;
> + else if (cap->channel_mode & BT_A2DP_CHANNEL_MODE_STEREO)
> + cap->channel_mode = BT_A2DP_CHANNEL_MODE_STEREO;
> + else if (cap->channel_mode & BT_A2DP_CHANNEL_MODE_DUAL_CHANNEL)
> + cap->channel_mode = BT_A2DP_CHANNEL_MODE_DUAL_CHANNEL;
> + else if (cap->channel_mode & BT_A2DP_CHANNEL_MODE_MONO) {
> + cap->channel_mode = BT_A2DP_CHANNEL_MODE_MONO;
> + u->sample_spec.channels = 1;
> + } else {
> + pa_log("No supported channel modes");
> + return -1;
> + }
> + }
>
> - if (cap->channel_mode & BT_A2DP_CHANNEL_MODE_JOINT_STEREO)
> - cap->channel_mode = BT_A2DP_CHANNEL_MODE_JOINT_STEREO;
> - else if (cap->channel_mode & BT_A2DP_CHANNEL_MODE_STEREO)
> - cap->channel_mode = BT_A2DP_CHANNEL_MODE_STEREO;
> - else if (cap->channel_mode & BT_A2DP_CHANNEL_MODE_DUAL_CHANNEL)
> - cap->channel_mode = BT_A2DP_CHANNEL_MODE_DUAL_CHANNEL;
> - else if (cap->channel_mode & BT_A2DP_CHANNEL_MODE_MONO) {
> - cap->channel_mode = BT_A2DP_CHANNEL_MODE_MONO;
> - u->sample_spec.channels = 1;
> - } else {
> - pa_log("No supported channel modes");
> + if (cap->block_length & BT_A2DP_BLOCK_LENGTH_16)
> + cap->block_length = BT_A2DP_BLOCK_LENGTH_16;
> + else if (cap->block_length & BT_A2DP_BLOCK_LENGTH_12)
> + cap->block_length = BT_A2DP_BLOCK_LENGTH_12;
> + else if (cap->block_length & BT_A2DP_BLOCK_LENGTH_8)
> + cap->block_length = BT_A2DP_BLOCK_LENGTH_8;
> + else if (cap->block_length & BT_A2DP_BLOCK_LENGTH_4)
> + cap->block_length = BT_A2DP_BLOCK_LENGTH_4;
> + else {
> + pa_log_error("No supported block lengths");
> return -1;
> }
> - }
>
> - if (cap->block_length & BT_A2DP_BLOCK_LENGTH_16)
> - cap->block_length = BT_A2DP_BLOCK_LENGTH_16;
> - else if (cap->block_length & BT_A2DP_BLOCK_LENGTH_12)
> - cap->block_length = BT_A2DP_BLOCK_LENGTH_12;
> - else if (cap->block_length & BT_A2DP_BLOCK_LENGTH_8)
> - cap->block_length = BT_A2DP_BLOCK_LENGTH_8;
> - else if (cap->block_length & BT_A2DP_BLOCK_LENGTH_4)
> - cap->block_length = BT_A2DP_BLOCK_LENGTH_4;
> - else {
> - pa_log_error("No supported block lengths");
> - return -1;
> - }
> -
> - if (cap->subbands & BT_A2DP_SUBBANDS_8)
> - cap->subbands = BT_A2DP_SUBBANDS_8;
> - else if (cap->subbands & BT_A2DP_SUBBANDS_4)
> - cap->subbands = BT_A2DP_SUBBANDS_4;
> - else {
> - pa_log_error("No supported subbands");
> - return -1;
> - }
> -
> - if (cap->allocation_method & BT_A2DP_ALLOCATION_LOUDNESS)
> - cap->allocation_method = BT_A2DP_ALLOCATION_LOUDNESS;
> - else if (cap->allocation_method & BT_A2DP_ALLOCATION_SNR)
> - cap->allocation_method = BT_A2DP_ALLOCATION_SNR;
> + if (cap->subbands & BT_A2DP_SUBBANDS_8)
> + cap->subbands = BT_A2DP_SUBBANDS_8;
> + else if (cap->subbands & BT_A2DP_SUBBANDS_4)
> + cap->subbands = BT_A2DP_SUBBANDS_4;
> + else {
> + pa_log_error("No supported subbands");
> + return -1;
> + }
>
> - cap->min_bitpool = (uint8_t) PA_MAX(MIN_BITPOOL, cap->min_bitpool);
> - cap->max_bitpool = (uint8_t) PA_MIN(a2dp_default_bitpool(cap->frequency, cap->channel_mode), cap->max_bitpool);
> + if (cap->allocation_method & BT_A2DP_ALLOCATION_LOUDNESS)
> + cap->allocation_method = BT_A2DP_ALLOCATION_LOUDNESS;
> + else if (cap->allocation_method & BT_A2DP_ALLOCATION_SNR)
> + cap->allocation_method = BT_A2DP_ALLOCATION_SNR;
>
> + cap->min_bitpool = (uint8_t) PA_MAX(MIN_BITPOOL, cap->min_bitpool);
> + cap->max_bitpool = (uint8_t) PA_MIN(a2dp_default_bitpool(cap->frequency, cap->channel_mode), cap->max_bitpool);
> + }
> return 0;
> }
>
> @@ -683,17 +858,37 @@ static int set_conf(struct userdata *u) {
> msg.open_req.h.length = sizeof(msg.open_req);
>
> pa_strlcpy(msg.open_req.object, u->path, sizeof(msg.open_req.object));
> - msg.open_req.seid = (u->profile == PROFILE_A2DP || u->profile == PROFILE_A2DP_SOURCE) ? u->a2dp.sbc_capabilities.capability.seid : BT_A2DP_SEID_RANGE + 1;
> - msg.open_req.lock = (u->profile == PROFILE_A2DP) ? BT_WRITE_LOCK : BT_READ_LOCK | BT_WRITE_LOCK;
> +
> + if (u->profile == PROFILE_A2DP || u->profile == PROFILE_A2DP_SOURCE) {
> + msg.open_req.seid = u->a2dp.sbc_capabilities.capability.seid;
> + } else if (u->profile == PROFILE_A2DP_PASSTHROUGH) {
> + if (u->a2dp.has_mpeg) {
> + msg.open_req.seid = u->a2dp.mpeg_capabilities.capability.seid;
> + } else {
> + pa_log("set_conf(): Trying to open MPEG endpoint but no MPEG endpoint available");
> + return -1;
> + }
> + } else {
> + msg.open_req.seid = BT_A2DP_SEID_RANGE + 1;
> + }
> +
> + msg.open_req.lock = (u->profile == PROFILE_A2DP || u->profile == PROFILE_A2DP_PASSTHROUGH) ? BT_WRITE_LOCK : BT_READ_LOCK | BT_WRITE_LOCK;
>
> if (service_send(u, &msg.open_req.h) < 0)
> return -1;
>
> - if (service_expect(u, &msg.open_rsp.h, sizeof(msg), BT_OPEN, sizeof(msg.open_rsp)) < 0)
> + if (service_expect(u, &msg.open_rsp.h, sizeof(msg), BT_OPEN, sizeof(msg.open_rsp)) < 0) {
> + pa_log("BT_OPEN expect failed");
> return -1;
> + }
>
> - if (u->profile == PROFILE_A2DP || u->profile == PROFILE_A2DP_SOURCE) {
> - u->sample_spec.format = PA_SAMPLE_S16LE;
> + if (u->profile == PROFILE_A2DP || u->profile == PROFILE_A2DP_PASSTHROUGH || u->profile == PROFILE_A2DP_SOURCE ) {
> +
> + if (u->profile == PROFILE_A2DP_PASSTHROUGH) {
> + u->sample_spec.format = PA_MP3;
> + } else {
> + u->sample_spec.format = PA_SAMPLE_S16LE;
> + }
Less curly brackets would suffice.
>
> if (setup_a2dp(u) < 0)
> return -1;
> @@ -712,6 +907,13 @@ static int set_conf(struct userdata *u) {
>
> if (u->profile == PROFILE_A2DP || u->profile == PROFILE_A2DP_SOURCE) {
> memcpy(&msg.setconf_req.codec, &u->a2dp.sbc_capabilities, sizeof(u->a2dp.sbc_capabilities));
> + } else if (u->profile == PROFILE_A2DP_PASSTHROUGH) {
> + if(u->a2dp.has_mpeg) {
Fix spaces.
> + memcpy(&msg.setconf_req.codec, &u->a2dp.mpeg_capabilities, sizeof(u->a2dp.mpeg_capabilities));
> + } else {
> + pa_log("set_conf(): Trying to configure MPEG endpoint but no MPEG endpoint available");
> + return -1;
> + }
> } else {
> msg.setconf_req.codec.transport = BT_CAPABILITIES_TRANSPORT_SCO;
> msg.setconf_req.codec.seid = BT_A2DP_SEID_RANGE + 1;
> @@ -719,28 +921,50 @@ static int set_conf(struct userdata *u) {
> }
> msg.setconf_req.h.length += msg.setconf_req.codec.length - sizeof(msg.setconf_req.codec);
>
> - if (service_send(u, &msg.setconf_req.h) < 0)
> + if (service_send(u, &msg.setconf_req.h) < 0) {
> + pa_log("BT_SET_CONFIGURATION send failed");
> return -1;
> -
> - if (service_expect(u, &msg.setconf_rsp.h, sizeof(msg), BT_SET_CONFIGURATION, sizeof(msg.setconf_rsp)) < 0)
> + }
> + if (service_expect(u, &msg.setconf_rsp.h, sizeof(msg), BT_SET_CONFIGURATION, sizeof(msg.setconf_rsp)) < 0) {
> + pa_log("BT_SET_CONFIGURATION expect failed");
> return -1;
> -
> + }
> u->link_mtu = msg.setconf_rsp.link_mtu;
>
> /* setup SBC encoder now we agree on parameters */
> if (u->profile == PROFILE_A2DP || u->profile == PROFILE_A2DP_SOURCE) {
> +
> setup_sbc(&u->a2dp);
>
> + /* number of input bytes per packet */
> u->block_size =
> - ((u->link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
> - / u->a2dp.frame_length
> - * u->a2dp.codesize);
> + (u->link_mtu - sizeof(struct rtp_header) - sizeof(struct sbc_rtp_payload))
> + / (u->a2dp.frame_length)
> + * (u->a2dp.codesize);
There were completely redundant parentheses before, which you removed,
but then you added other completely redundant parentheses. Why are
frame_length and codesize inside parentheses?
> +
> + /* number of samples per packet */
> + u->samples_per_block =
> + (u->link_mtu - sizeof(struct rtp_header) - sizeof(struct sbc_rtp_payload))
> + / (u->a2dp.frame_length)
> + * (u->a2dp.sbc.subbands ? 8 : 4) * (4 + (u->a2dp.sbc.blocks * 4));
(u->a2dp.sbc.subbands ? 8 : 4) relies on the fact that SBC_SB_4 happens
to be defined as 0x00. I think it would be better to have
(u->a2dp.sbc.subbands == SBC_SB_8 ? 8 : 4).
But anyway, why make this so complicated? I don't have any idea what sbc
subbands and sbc blocks mean, so it's very hard to verify that this code
is correct. Why not simply
u->samples_per_block = u->block_size / pa_frame_size(&u->sample_spec);
?
Also, redundant parentheses around frame_length.
>
> pa_log_info("SBC parameters:\n\tallocation=%u\n\tsubbands=%u\n\tblocks=%u\n\tbitpool=%u\n",
> u->a2dp.sbc.allocation, u->a2dp.sbc.subbands, u->a2dp.sbc.blocks, u->a2dp.sbc.bitpool);
> - } else
> - u->block_size = u->link_mtu;
> + } else if (u->profile == PROFILE_A2DP_PASSTHROUGH) {
>
> + /* available payload per packet */
> + u->block_size =
> + ((u->link_mtu - sizeof(struct rtp_header) - sizeof(struct mpeg_rtp_payload)) /4 ) * 4; /* force 32-bit length to simplify synchronization */
Fix spaces around "/4": add one between "/" and "4" and remove one
between "4" and ")".
> +
> + /* FIXME : number of samples per packet */
> + u->samples_per_block = 1152;
So is this assuming certain link_mtu and certain constant (or average)
bitrate? Since the samples per block ratio doesn't stay constant with
vbr streams, is this variable going to become unneeded when you
implement the timing logic correctly?
> +
> + u->leftover_bytes=0;
> +
> + } else {
> + u->block_size = u->link_mtu;
> + u->samples_per_block = u->link_mtu/pa_frame_size(&u->sample_spec);
Put spaces around "/".
> + }
> return 0;
> }
>
> @@ -795,7 +1019,7 @@ static int start_stream_fd(struct userdata *u) {
> pollfd->fd = u->stream_fd;
> pollfd->events = pollfd->revents = 0;
>
> - u->read_index = u->write_index = 0;
> + u->s_read_index = u->s_write_index = 0;
> u->started_at = 0;
>
> if (u->source)
> @@ -902,14 +1126,14 @@ static int sink_process_msg(pa_msgobject *o, int code, void *data, int64_t offse
> pa_usec_t wi, ri;
>
> ri = pa_smoother_get(u->read_smoother, pa_rtclock_now());
> - wi = pa_bytes_to_usec(u->write_index + u->block_size, &u->sample_spec);
> + wi = samples_to_usec(u->s_write_index + u->samples_per_block, &u->sample_spec);
>
> *((pa_usec_t*) data) = wi > ri ? wi - ri : 0;
> } else {
> pa_usec_t ri, wi;
>
> ri = pa_rtclock_now() - u->started_at;
> - wi = pa_bytes_to_usec(u->write_index, &u->sample_spec);
> + wi = samples_to_usec(u->s_write_index, &u->sample_spec);
>
> *((pa_usec_t*) data) = wi > ri ? wi - ri : 0;
> }
> @@ -975,7 +1199,7 @@ static int source_process_msg(pa_msgobject *o, int code, void *data, int64_t off
>
> if (u->read_smoother) {
> wi = pa_smoother_get(u->read_smoother, pa_rtclock_now());
> - ri = pa_bytes_to_usec(u->read_index, &u->sample_spec);
> + ri = samples_to_usec(u->s_read_index, &u->sample_spec);
>
> *((pa_usec_t*) data) = (wi > ri ? wi - ri : 0) + u->source->thread_info.fixed_latency;
> } else
> @@ -1043,8 +1267,7 @@ static int hsp_process_render(struct userdata *u) {
> ret = -1;
> break;
> }
> -
> - u->write_index += (uint64_t) u->write_memchunk.length;
> + u->s_write_index += (uint64_t) u->write_memchunk.length/pa_frame_size(&u->sample_spec);
Put spaces around "/".
> pa_memblock_unref(u->write_memchunk.memblock);
> pa_memchunk_reset(&u->write_memchunk);
>
> @@ -1111,7 +1334,7 @@ static int hsp_process_push(struct userdata *u) {
> pa_assert((size_t) l <= pa_memblock_get_length(memchunk.memblock));
>
> memchunk.length = (size_t) l;
> - u->read_index += (uint64_t) l;
> + u->s_read_index += (uint64_t) l/pa_frame_size(&u->sample_spec);
Put spaces around "/".
>
> for (cm = CMSG_FIRSTHDR(&m); cm; cm = CMSG_NXTHDR(&m, cm))
> if (cm->cmsg_level == SOL_SOCKET && cm->cmsg_type == SO_TIMESTAMP) {
> @@ -1127,7 +1350,7 @@ static int hsp_process_push(struct userdata *u) {
> tstamp = pa_rtclock_now();
> }
>
> - pa_smoother_put(u->read_smoother, tstamp, pa_bytes_to_usec(u->read_index, &u->sample_spec));
> + pa_smoother_put(u->read_smoother, tstamp, samples_to_usec(u->s_read_index, &u->sample_spec));
> pa_smoother_resume(u->read_smoother, tstamp, TRUE);
>
> pa_source_post(u->source, &memchunk);
> @@ -1157,7 +1380,7 @@ static void a2dp_prepare_buffer(struct userdata *u) {
> static int a2dp_process_render(struct userdata *u) {
> struct a2dp_info *a2dp;
> struct rtp_header *header;
> - struct rtp_payload *payload;
> + struct sbc_rtp_payload *payload;
> size_t nbytes;
> void *d;
> const void *p;
> @@ -1179,7 +1402,7 @@ static int a2dp_process_render(struct userdata *u) {
>
> a2dp = &u->a2dp;
> header = a2dp->buffer;
> - payload = (struct rtp_payload*) ((uint8_t*) a2dp->buffer + sizeof(*header));
> + payload = (struct sbc_rtp_payload*) ((uint8_t*) a2dp->buffer + sizeof(*header));
>
> frame_count = 0;
>
> @@ -1237,7 +1460,7 @@ static int a2dp_process_render(struct userdata *u) {
> header->v = 2;
> header->pt = 1;
> header->sequence_number = htons(a2dp->seq_num++);
> - header->timestamp = htonl(u->write_index / pa_frame_size(&u->sample_spec));
> + header->timestamp = htonl(u->s_write_index); /* sample count */
Not sample, but pcm frame count.
> header->ssrc = htonl(1);
> payload->frame_count = frame_count;
>
> @@ -1275,7 +1498,7 @@ static int a2dp_process_render(struct userdata *u) {
> break;
> }
>
> - u->write_index += (uint64_t) u->write_memchunk.length;
> + u->s_write_index += (uint64_t) u->samples_per_block;
> pa_memblock_unref(u->write_memchunk.memblock);
> pa_memchunk_reset(&u->write_memchunk);
>
> @@ -1287,6 +1510,334 @@ static int a2dp_process_render(struct userdata *u) {
> return ret;
> }
>
> +
> +static uint32_t uextract(uint32_t x, uint32_t position, uint32_t nbits)
> +{
> + int mask;
> +
> + pa_assert(position <= 31);
> + pa_assert(nbits <= 31);
> +
> + x = x >> position;
> + mask = (1<<nbits)-1;
> + x = x & mask;
> +
> + return x;
> +}
> +
> +
> +#define MPEG_LAYER_INDEX 4
> +#define MPEG_BITRATE_INDEX 16
> +#define MPEG_SAMPFREQ_INDEX 4
> +#define MPEG_INDEX 2
> +
> +unsigned short const mpeg_sampling_frequencies[MPEG_INDEX][MPEG_SAMPFREQ_INDEX] = {
> + {22050, 24000, 16000, 0},
> + {44100, 48000, 32000, 0}
Tab indentation.
> +};
> +
> +short const mpeg_layer_bitrates[MPEG_INDEX][MPEG_BITRATE_INDEX] = {
> + { 0, 8, 16, 24, 32, 40, 48, 56, 64, 80, 96, 112, 128, 144, 160, -1},
> + { 0, 32, 40, 48, 56, 64, 80, 96, 112, 128, 160, 192, 224, 256, 320, -1}
Tab indentation.
> +};
> +
> +short const mpeg_samples_per_frame[MPEG_INDEX][MPEG_LAYER_INDEX] = {
> + {0, 576, 1152, 384},
> + {0, 1152, 1152, 384}
> +};
If these are actually pcm frames and not samples, rename the array. (If
they actually are samples, then mp3_synclength() is broken.)
> +
> +static int mp3_synclength(uint32_t hi, uint32_t *len, uint32_t *sample_len)
This function has messed up indentation (mixed tabs and spaces).
Also, you should assert that len and sample_len are not null.
> +{
> + unsigned int tmp;
> + unsigned int idex;
> + unsigned int id;
> + unsigned int bitrate;
> + unsigned int freq;
> + unsigned int bits;
> +
> + tmp = uextract(hi, 21, 11);
> + if (tmp == 0x7ff) { /* valid sync word */
> + tmp = uextract(hi,19,2);
Fix spaces.
> + if (tmp != 1) { /* valid IDex */
> +
> + idex = tmp >> 1;
> + if (idex != 0) { /* MP3 2.5, not supported by A2DP */
> +
> + id = tmp & 1;
> + tmp = uextract(hi,17,2);
Fix spaces.
> +
> + if (tmp == 0x1) { /* layer 3 */
> +
> + bitrate = uextract(hi,12,4);
Fix spaces.
> + if ((bitrate != 0) && /* not free format */
> + (bitrate != 0xf)) { /* not reserved */
> +
> + freq = uextract(hi,10, 2);
Fix spaces.
> +
> + if (freq != 3) { /* valid sampling frequency */
> +
> + tmp = uextract(hi,9,1);
Fix spaces.
> +
> + bitrate = mpeg_layer_bitrates[id][bitrate] * 1000;
> +
> + bits =
> + (unsigned int) ((bitrate * mpeg_samples_per_frame[id][1]) /
Here's two spaces between "bitrate" and "*". Also, you use constant
index 1 - is that really correct? Why is there an array of values, if
only one of them is ever used?
> + mpeg_sampling_frequencies[id][freq]);
> +
> +
> + bits /= 8; /* # of bytes */
> + if (tmp) { /* padding */
> + bits += 1;
> + }
> +
> + /* sanity check */
> + if (bits > 4 && bits <= 1728) { /* max frame length */
> + *len = bits;
> + *sample_len = mpeg_samples_per_frame[id][1];
Constant index 1 again.
> + return 0;
> + }
> + }
> + }
> + }
> + }
> + }
> + } else {
> + if(hi!=0)
Fix spaces.
> + pa_log("no sync word found %x",hi);
Fix spaces.
> + }
> + return 1;
-1 is the canonical failure return value.
> +}
> +
> +static int do_sync(uint8_t **p_src,uint8_t **p_dest, uint32_t *input_bytes, uint32_t *output_bytes,
> + uint32_t *frame_count, uint32_t *sample_count)
If function arguments need wrapping, I think it's a convention in PA
sources to put each argument on its own line with 8-space indentation:
static int do_sync(
uint8_t **p_src,
uint8_t **p_dest,
uint32_t *input_bytes,
uint32_t *output_bytes,
uint32_t *frame_count,
uint32_t *sample_count)
Also, use assertions to check that any pointer isn't null.
> +{
> + int sync_found=0;
Fix spaces, and use pa_bool_t instead of int.
> + uint32_t len, sample_len;
> + uint8_t *src=*p_src;
> + uint8_t *dest=*p_dest;
Don't dereference pointers until you've made sure they're not null.
> +
> + len = 0; /* remove compiler warning */
Unnecessary comment; it's better to initialize len when it's declared.
> + while (1) {
For maximum consistency you could use "for (;;)", as that's what's used
for this kind of loops virtually everywhere in PA's source, but "while
(1)" of course isn't any less readable.
> +
> + if ( *input_bytes< 4 )
Fix spaces.
> + break;
> +
> + /* we need 4 bytes to detect the frame length */
> + if (!mp3_synclength(src[0]<<24|src[1]<<16|src[2]<<8|src[3],
> + &len, &sample_len) ) {
Fix spaces, and instead of using the not operator, use mp3_synclength()
>= 0 (requires changing the failure return value to -1).
> +
> + sync_found = 1;
Use TRUE istead of 1.
> +
> + if (len>*input_bytes) {
Fix spaces.
> + /* we don't have a full frame in the input buffer */
> + break;
> + }
> +
> + /* make sure there's enough room in the output buffer */
> + if (len > *output_bytes)
> + break;
> +
> + /* copy complete frame */
> + memcpy(dest,src,len);
Fix spaces.
> + dest = dest + len;
> + src = src + len;
> +
> + *input_bytes -= len;
> + *output_bytes -= len;
> +
> + *frame_count += 1;
> + *sample_count += sample_len;
> +
> + } else {
> +
> + sync_found = 0;
> +
> + /* try to find a new syncword */
> + src += 1;
> + *input_bytes -= 1;
> + }
> + }
> + *p_src=src;
> + *p_dest=dest;
Fix spaces.
> +
> + return sync_found;
return sync_found ? 0 : -1;
> +} /* end do_sync() */
Redundant comment.
> +
> +
Extra line.
> +static int a2dp_passthrough_process_render(struct userdata *u) {
I think you should add "Run from IO thread" comment here. I know
a2dp_process_render() doesn't have the comment either, but I think it
should.
> + struct a2dp_info *a2dp;
> + struct rtp_header *header;
> + struct mpeg_rtp_payload *payload;
> + size_t nbytes;
> + void *d;
> + const void *p;
Declaring d as "uint8_t *" and p as "const uint8_t *" would get rid of
some casts without any adverse effects, as far as I can see.
> + size_t dest_bytes; /* in the destination buffer */
> + size_t orig_bytes=0; /* in the sink buffer */
Fix spaces.
> + uint32_t frame_count;
> + uint32_t sample_count=0;
Fix spaces. Also, I suggest you rename frame_count to mp3_frame_count
and sample_count to pcm_frame_count.
> + uint32_t sync_found = 0;
Use pa_bool_t.
> + uint32_t talk_spurt;
> +
> + int ret = 0;
> +
> + pa_assert(u);
> + pa_assert(u->profile == PROFILE_A2DP_PASSTHROUGH);
> + pa_assert(u->sink);
> +
> + /* inits for output buffer */
> + a2dp_prepare_buffer(u);
Not really related to this patch, but I checked a2dp_prepare_buffer
implementation, and it seemed broken: it sometimes calls pa_xmalloc()
from the IO thread. Fixing that doesn't belong this patch, though. I
just thought I'd mention. Someone should fix that, but it's a completely
separate task, so there's no reason that someone should be you...
> +
> + a2dp = &u->a2dp;
> + header = a2dp->buffer;
> + payload = (struct mpeg_rtp_payload*) ((uint8_t*) a2dp->buffer + sizeof(*header));
> + d = (uint8_t*) a2dp->buffer + sizeof(*header) + sizeof(*payload);
> + dest_bytes = a2dp->buffer_size - sizeof(*header) - sizeof(*payload);
> +
> + if (dest_bytes > u->block_size) {
> + dest_bytes = u->block_size;
> + }
Don't use curly brackets for simple ifs like this.
> +
> + frame_count = 0;
> +
> +
Extra line.
> + /* render some data */
> + if (!u->write_memchunk.memblock) {
> + pa_memchunk *c;
> +
> + pa_assert(u->leftover_bytes == 0);
> + c = &u->write_memchunk;
> + c->memblock = pa_memblock_new(u->sink->core->mempool,u->block_size);
Fix spaces.
> + c->index = 0;
> + c->length = u->block_size;
> + }
> +
> + if (u->leftover_bytes) { /* incomplete frame that wasn't handled in the previous call */
> + pa_memchunk *c;
> + pa_memblock *n;
> + void *tdata, *sdata;
> + size_t l;
> +
> + c = &u->write_memchunk;
> + n = pa_memblock_new(u->sink->core->mempool,u->block_size);
Fix spaces.
> +
> + sdata = pa_memblock_acquire(c->memblock);
> + tdata = pa_memblock_acquire(n);
> +
> + l = c->length;
> + memcpy(tdata, (uint8_t*) sdata + c->index, l);
sdata could be declared as uint8_t*, removing the need to do this cast.
> +
> + pa_memblock_release(c->memblock);
> + pa_memblock_release(n);
> +
> + pa_memblock_unref(c->memblock);
> +
> + c->memblock = n;
> + c->index = l;
> + c->length = u->block_size - l;
> + }
> +
> + /* fill memchunck, previous leftover bytes have been copied into beginning of frame already */
> + pa_sink_render_into_full(u->sink, &u->write_memchunk);
> +
> + orig_bytes = u->block_size;
> +
> + p = (const uint8_t*) pa_memblock_acquire(u->write_memchunk.memblock);
Is the cast really needed?
> +
> + sync_found = do_sync((uint8_t**)&p,(uint8_t**)&d, &orig_bytes, &dest_bytes,
> + &frame_count, &sample_count);
If p had type "const uint8_t *", like I propose, and do_sync's first
parameter type would be changed to "const uint8_t **", then p wouldn't
need any casting. Similarly, if d had type "uint8_t *", no cast would be
needed for it either.
> +
> + u->write_memchunk.index = u->block_size-orig_bytes;
Fix spaces.
> + u->write_memchunk.length = orig_bytes;
> +
> + pa_memblock_release(u->write_memchunk.memblock);
> +
> + if (sync_found) {
> + nbytes = (uint8_t*) d - (uint8_t*) a2dp->buffer;
> +
> + u->leftover_bytes=orig_bytes;
> +
> + if (u->leftover_bytes == 0) {
> + pa_memblock_unref(u->write_memchunk.memblock);
> + pa_memchunk_reset(&u->write_memchunk);
> + }
> +
> + talk_spurt = 1;
> + } else {
> + /* we lost the sync here, zero out rest of buffer */
I suggest you do the parsing of the mp3 frames as soon as possible, i.e.
in pa_stream_write(), and drop the data that you can't parse
immediately. That way there will never be gaps when the data is
processed in the server (the server must still not crash if such gaps
are found, in case the client is using something else than libpulse to
communicate with the server).
I also suggest that you make sure that memchunks contain only whole
frames. That way it's always possible to map a memchunk's data to time,
and also map a memblockq's data to time. I would guess that those
mappings will be needed.
I also have a feeling that you don't need to copy the audio data so
much, but even if that's true, fixing it can be left to later time.
> + memset(d, 0, dest_bytes);
> +
> + u->leftover_bytes=0;
Fix spaces.
> + pa_memblock_unref(u->write_memchunk.memblock);
> + pa_memchunk_reset(&u->write_memchunk);
> +
> + /* FIXME: this is completely broken, force the write index to some value */
> + u->s_write_index += (uint64_t) 1152;
> + ret = 1;
> +
> + return ret;
> + }
> +
> + pa_assert(nbytes!=0);
Fix spaces.
> + pa_assert(nbytes<=(u->block_size+sizeof(*header)+sizeof(*payload)));
Fix spaces.
> +
> + /* write it to the fifo */
> +
> + memset(a2dp->buffer, 0, sizeof(*header) + sizeof(*payload));
> + header->v = 2; /* rtp packet v2 */
> + header->pt = 14; /* MPA payload type */
> + header->timestamp = htonl((u->s_write_index*90000)/u->sample_spec.rate); /* 90kHz timestamp */
Fix spaces.
> + header->m = talk_spurt;
talk_spurt is a redundant variable. You can use simply header->m = 1
here.
> + header->sequence_number = htons(a2dp->seq_num++);
> + header->ssrc = htonl(1); /* should in theory be random */
Why should it? And why is it not? What are the consequences of this
choice?
> +
> + payload->mbz = 0;
> + payload->frag_offset = 0;
> +
> + pa_assert(nbytes != 0);
> + pa_assert(nbytes <= u->link_mtu);
> +
> + for (;;) {
> + ssize_t l;
> +
> + l = pa_write(u->stream_fd, a2dp->buffer, nbytes, &u->stream_write_type);
> +
> + pa_assert(l != 0);
> +
> + if (l < 0) {
> +
> + if (errno == EINTR)
> + /* Retry right away if we got interrupted */
> + continue;
> +
> + else if (errno == EAGAIN)
> + /* Hmm, apparently the socket was not writable, give up for now */
> + break;
> +
> + pa_log_error("Failed to write data to socket: %s", pa_cstrerror(errno));
> + ret = -1;
> + break;
> + }
> +
> + pa_assert((size_t) l <= nbytes);
> +
> + if ((size_t) l != nbytes) {
> + pa_log_warn("Wrote memory block to socket only partially! %llu written, wanted to write %llu.",
> + (unsigned long long) l,
> + (unsigned long long) nbytes);
> + ret = -1;
> + break;
> + }
> +
> + u->s_write_index += (uint64_t) sample_count;
> +
> + ret = 1;
> +
> + break;
> + }
> +
> + return ret;
> +}
> +
> static int a2dp_process_push(struct userdata *u) {
> int ret = 0;
> pa_memchunk memchunk;
> @@ -1304,7 +1855,7 @@ static int a2dp_process_push(struct userdata *u) {
> pa_usec_t tstamp;
> struct a2dp_info *a2dp;
> struct rtp_header *header;
> - struct rtp_payload *payload;
> + struct sbc_rtp_payload *payload;
> const void *p;
> void *d;
> ssize_t l;
> @@ -1315,7 +1866,7 @@ static int a2dp_process_push(struct userdata *u) {
>
> a2dp = &u->a2dp;
> header = a2dp->buffer;
> - payload = (struct rtp_payload*) ((uint8_t*) a2dp->buffer + sizeof(*header));
> + payload = (struct sbc_rtp_payload*) ((uint8_t*) a2dp->buffer + sizeof(*header));
>
> l = pa_read(u->stream_fd, a2dp->buffer, a2dp->buffer_size, &u->stream_write_type);
>
> @@ -1336,7 +1887,7 @@ static int a2dp_process_push(struct userdata *u) {
>
> pa_assert((size_t) l <= a2dp->buffer_size);
>
> - u->read_index += (uint64_t) l;
> + u->s_read_index += (uint64_t) l/pa_frame_size(&u->sample_spec);
Fix spaces.
>
> /* TODO: get timestamp from rtp */
> if (!found_tstamp) {
> @@ -1344,7 +1895,7 @@ static int a2dp_process_push(struct userdata *u) {
> tstamp = pa_rtclock_now();
> }
>
> - pa_smoother_put(u->read_smoother, tstamp, pa_bytes_to_usec(u->read_index, &u->sample_spec));
> + pa_smoother_put(u->read_smoother, tstamp, samples_to_usec(u->s_read_index, &u->sample_spec));
> pa_smoother_resume(u->read_smoother, tstamp, TRUE);
>
> p = (uint8_t*) a2dp->buffer + sizeof(*header) + sizeof(*payload);
> @@ -1428,8 +1979,7 @@ static void thread_func(void *userdata) {
>
> /* We should send two blocks to the device before we expect
> * a response. */
> -
> - if (u->write_index == 0 && u->read_index <= 0)
> + if (u->s_write_index == 0 && u->s_read_index <= 0)
> do_write = 2;
>
> if (pollfd && (pollfd->revents & POLLIN)) {
> @@ -1465,19 +2015,20 @@ static void thread_func(void *userdata) {
> * to. So let's do things by time */
>
> time_passed = pa_rtclock_now() - u->started_at;
> - audio_sent = pa_bytes_to_usec(u->write_index, &u->sample_spec);
> + audio_sent = samples_to_usec(u->s_write_index, &u->sample_spec);
>
> if (audio_sent <= time_passed) {
> pa_usec_t audio_to_send = time_passed - audio_sent;
>
> /* Never try to catch up for more than 100ms */
> - if (u->write_index > 0 && audio_to_send > MAX_PLAYBACK_CATCH_UP_USEC) {
> + if (u->s_write_index > 0 && audio_to_send > MAX_PLAYBACK_CATCH_UP_USEC) {
> pa_usec_t skip_usec;
> uint64_t skip_bytes;
>
> skip_usec = audio_to_send - MAX_PLAYBACK_CATCH_UP_USEC;
> skip_bytes = pa_usec_to_bytes(skip_usec, &u->sample_spec);
>
> + /* FIXME: this doesn't work for PASSTHROUGH */
Any ideas how to fix this? Would moving u->started_at forward by
skip_bytes make sense for passthrough?
> if (skip_bytes > 0) {
> pa_memchunk tmp;
>
> @@ -1487,9 +2038,10 @@ static void thread_func(void *userdata) {
>
> pa_sink_render_full(u->sink, skip_bytes, &tmp);
> pa_memblock_unref(tmp.memblock);
> - u->write_index += skip_bytes;
> - }
> - }
> + pa_assert(u->profile != PROFILE_A2DP_PASSTHROUGH);
> + u->s_write_index += skip_bytes/pa_frame_size(&u->sample_spec);
Fix spaces.
> + }
> + }
>
> do_write = 1;
> }
> @@ -1498,12 +2050,15 @@ static void thread_func(void *userdata) {
> if (writable && do_write > 0) {
> int n_written;
>
> - if (u->write_index <= 0)
> + if (u->s_write_index <= 0)
> u->started_at = pa_rtclock_now();
>
> if (u->profile == PROFILE_A2DP) {
> if ((n_written = a2dp_process_render(u)) < 0)
> goto fail;
> + } else if (u->profile == PROFILE_A2DP_PASSTHROUGH) {
> + if ((n_written = a2dp_passthrough_process_render(u)) < 0)
> + goto fail;
> } else {
> if ((n_written = hsp_process_render(u)) < 0)
> goto fail;
> @@ -1523,7 +2078,7 @@ static void thread_func(void *userdata) {
> * to. So let's estimate when we need to wake up the latest */
>
> time_passed = pa_rtclock_now() - u->started_at;
> - next_write_at = pa_bytes_to_usec(u->write_index, &u->sample_spec);
> + next_write_at = samples_to_usec(u->s_write_index, &u->sample_spec);
> sleep_for = time_passed < next_write_at ? next_write_at - time_passed : 0;
>
> /* pa_log("Sleeping for %lu; time passed %lu, next write at %lu", (unsigned long) sleep_for, (unsigned long) time_passed, (unsigned long)next_write_at); */
> @@ -1782,7 +2337,7 @@ static int add_sink(struct userdata *u) {
> data.driver = __FILE__;
> data.module = u->module;
> pa_sink_new_data_set_sample_spec(&data, &u->sample_spec);
> - pa_proplist_sets(data.proplist, "bluetooth.protocol", u->profile == PROFILE_A2DP ? "a2dp" : "sco");
> + pa_proplist_sets(data.proplist, "bluetooth.protocol", (u->profile == PROFILE_A2DP || u->profile == PROFILE_A2DP_PASSTHROUGH) ? "a2dp" : "sco");
> if (u->profile == PROFILE_HSP)
> pa_proplist_sets(data.proplist, PA_PROP_DEVICE_INTENDED_ROLES, "phone");
> data.card = u->card;
> @@ -1807,16 +2362,24 @@ static int add_sink(struct userdata *u) {
> u->sink->parent.process_msg = sink_process_msg;
>
> pa_sink_set_max_request(u->sink, u->block_size);
> - pa_sink_set_fixed_latency(u->sink,
> - (u->profile == PROFILE_A2DP ? FIXED_LATENCY_PLAYBACK_A2DP : FIXED_LATENCY_PLAYBACK_HSP) +
> - pa_bytes_to_usec(u->block_size, &u->sample_spec));
> - }
>
> + if (u->profile == PROFILE_A2DP_PASSTHROUGH) {
> + /* FIXME: latency is broken for now */
> + pa_sink_set_fixed_latency(u->sink,FIXED_LATENCY_PLAYBACK_A2DP);
> + } else {
> + pa_sink_set_fixed_latency(u->sink,
> + ((u->profile == PROFILE_A2DP)? FIXED_LATENCY_PLAYBACK_A2DP : FIXED_LATENCY_PLAYBACK_HSP) +
> + pa_bytes_to_usec(u->block_size, &u->sample_spec));
> + }
> + }
> if (u->profile == PROFILE_HSP) {
> u->sink->set_volume = sink_set_volume_cb;
> u->sink->n_volume_steps = 16;
> }
>
> + if (u->profile == PROFILE_A2DP_PASSTHROUGH) {
> + u->sink->flags |= PA_SINK_PASSTHROUGH;
> + }
> return 0;
> }
>
> @@ -1962,6 +2525,7 @@ static int init_profile(struct userdata *u) {
> return -1;
>
> if (u->profile == PROFILE_A2DP ||
> + u->profile == PROFILE_A2DP_PASSTHROUGH ||
> u->profile == PROFILE_HSP ||
> u->profile == PROFILE_HFGW)
> if (add_sink(u) < 0)
> @@ -2104,6 +2668,10 @@ static int card_set_profile(pa_card *c, pa_card_profile *new_profile) {
> pa_log_warn("A2DP is not connected, refused to switch profile");
> return -PA_ERR_IO;
> }
> + else if (device->audio_sink_state < PA_BT_AUDIO_STATE_CONNECTED && *d == PROFILE_A2DP_PASSTHROUGH) {
> + pa_log_warn("A2DP Passthrough is not connected, refused to switch profile");
> + return -PA_ERR_IO;
> + }
> else if (device->hfgw_state <= PA_BT_AUDIO_STATE_CONNECTED && *d == PROFILE_HFGW) {
> pa_log_warn("HandsfreeGateway is not connected, refused to switch profile");
> return -PA_ERR_IO;
> @@ -2212,6 +2780,20 @@ static int add_card(struct userdata *u, const pa_bluetooth_device *device) {
> *d = PROFILE_A2DP;
>
> pa_hashmap_put(data.profiles, p->name, p);
> +
> + /* add passthrough profile */
> + p = pa_card_profile_new("passthrough", _("MP3 passthrough (A2DP)"), sizeof(enum profile));
> + p->priority = 5;
> + p->n_sinks = 1;
> + p->n_sources = 0;
> + p->max_sink_channels = 2;
> + p->max_source_channels = 0;
> +
> + d = PA_CARD_PROFILE_DATA(p);
> + *d = PROFILE_A2DP_PASSTHROUGH;
> +
> + pa_hashmap_put(data.profiles, p->name, p);
> +
> }
>
> if (pa_bluetooth_uuid_has(device->uuids, A2DP_SOURCE_UUID)) {
> @@ -2286,6 +2868,7 @@ static int add_card(struct userdata *u, const pa_bluetooth_device *device) {
>
> if ((device->headset_state < PA_BT_AUDIO_STATE_CONNECTED && *d == PROFILE_HSP) ||
> (device->audio_sink_state < PA_BT_AUDIO_STATE_CONNECTED && *d == PROFILE_A2DP) ||
> + (device->audio_sink_state < PA_BT_AUDIO_STATE_CONNECTED && *d == PROFILE_A2DP_PASSTHROUGH) ||
> (device->hfgw_state < PA_BT_AUDIO_STATE_CONNECTED && *d == PROFILE_HFGW)) {
> pa_log_warn("Default profile not connected, selecting off profile");
> u->card->active_profile = pa_hashmap_get(u->card->profiles, "off");
> diff --git a/src/modules/bluetooth/rtp.h b/src/modules/bluetooth/rtp.h
> index 1457362..19307ac 100644
> --- a/src/modules/bluetooth/rtp.h
> +++ b/src/modules/bluetooth/rtp.h
> @@ -38,7 +38,7 @@ struct rtp_header {
> uint32_t csrc[0];
> } __attribute__ ((packed));
>
> -struct rtp_payload {
> +struct sbc_rtp_payload {
> unsigned frame_count:4;
> unsigned rfa0:1;
> unsigned is_last_fragment:1;
> @@ -46,6 +46,11 @@ struct rtp_payload {
> unsigned is_fragmented:1;
> } __attribute__ ((packed));
>
> +struct mpeg_rtp_payload {
> + unsigned mbz:16;
> + unsigned frag_offset:16;
> +} __attribute__ ((packed));
> +
> #elif __BYTE_ORDER == __BIG_ENDIAN
>
> struct rtp_header {
> @@ -63,7 +68,7 @@ struct rtp_header {
> uint32_t csrc[0];
> } __attribute__ ((packed));
>
> -struct rtp_payload {
> +struct sbc_rtp_payload {
> unsigned is_fragmented:1;
> unsigned is_first_fragment:1;
> unsigned is_last_fragment:1;
> @@ -71,6 +76,11 @@ struct rtp_payload {
> unsigned frame_count:4;
> } __attribute__ ((packed));
>
> +struct mpeg_rtp_payload {
> + unsigned frag_offset:16;
> + unsigned mbz:16;
> +} __attribute__ ((packed));
> +
> #else
> #error "Unknown byte order"
> #endif
--
Tanu Kaskinen
More information about the pulseaudio-discuss
mailing list