[pulseaudio-discuss] [PATCH] bluetooth: MP3 passthrough over A2DP
Tanu Kaskinen
tanuk at iki.fi
Sun Nov 7 03:20:14 PST 2010
Hi,
Patch review below.
On Fri, 2010-10-22 at 18:48 -0500, Pierre-Louis Bossart wrote:
> This patch enables MP3 passthrough over A2DP, with the decoding
> taking place on the headset to reduce power consumption. This was
> only possible if PulseAudio was bypassed before with the a2dpsink
> element in gstreamer.
>
> This is a simpler version of an earlier patch. The MP3 parsing is
> now supposed to happen in the application, and the BT sink
> expects an IEC61937 format: 8byte header, mp3 frame and zero-padding
> to reach 4608 bytes. This format is a pseudo-PCM stream, which
> is compatible with the existing way of handling latency/timing.
>
> this patch can be tested with following command:
>
> gst-launch filesrc location=file.mp3 ! mp3parse ! mpegiec958 ! filesink location=file-be.iec
> dd if=if file-be.iec of=file-le.iec conv=swab
> paplay --raw --passthrough -v --format="s16le" --rate=44100 --device=bluez_sink.00_11_22_33_44_55 file-le.iec
>
> or more simply
> gst-launch filesrc location=file.mp3 ! mp3parse ! mpegiec958 ! pulsesink device=bluez_sink.00_11_22_33_44_55
>
> patches for mpegiec958 and pulsesink will be posted separately.
>
> TODO for later patches:
> - add type for MP3/AC3
> - expose sink capability to apps so that the pipeline can be reconfigured
> - handle dynamic routing
> - set sink sample rate to the rate of the stream
> ---
> src/modules/bluetooth/bluetooth-util.h | 1 +
> src/modules/bluetooth/module-bluetooth-device.c | 665 +++++++++++++++++++++--
> src/modules/bluetooth/rtp.h | 14 +-
> src/pulsecore/sink-input.c | 26 +-
> 4 files changed, 655 insertions(+), 51 deletions(-)
>
> diff --git a/src/modules/bluetooth/bluetooth-util.h b/src/modules/bluetooth/bluetooth-util.h
> index ce55196..214bf98 100644
> --- a/src/modules/bluetooth/bluetooth-util.h
> +++ b/src/modules/bluetooth/bluetooth-util.h
> @@ -58,6 +58,7 @@ struct pa_bluetooth_uuid {
> enum profile {
> PROFILE_A2DP,
> PROFILE_A2DP_SOURCE,
> + PROFILE_A2DP_PASSTHROUGH,
> PROFILE_HSP,
> PROFILE_HFGW,
> PROFILE_OFF
> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
> index c7ac7fa..7a60d5b 100644
> --- a/src/modules/bluetooth/module-bluetooth-device.c
> +++ b/src/modules/bluetooth/module-bluetooth-device.c
> @@ -110,6 +110,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 */
> @@ -178,6 +181,10 @@ struct userdata {
> int service_write_type, service_read_type;
>
> pa_bool_t filter_added;
> +
> + /* required for PASSTHROUGH profile */
> + size_t leftover_bytes;
> +
> };
>
> #define FIXED_LATENCY_PLAYBACK_A2DP (25*PA_USEC_PER_MSEC)
> @@ -300,6 +307,66 @@ 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_assert(u);
> + pa_assert(rsp);
> + pa_assert( u->profile == PROFILE_A2DP_PASSTHROUGH );
I already pointed out in the previous review that here's too much
spaces. Reading the patch further, it seems that you ignored most of the
review comments... I assume that was not intentional. I'll put the same
comments in this review.
> +
> + u->a2dp.has_mpeg = FALSE;
> +
> + 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? **/
> +
> + 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)) {
> + pa_log_error("Got capabilities for wrong codec.");
> + return -1;
> + }
> +
> + while (bytes_left > 0) {
> + if ((codec->type == BT_A2DP_MPEG12_SINK) && (!codec->lock)) {
> + break;
> + }
> + bytes_left -= codec->length;
> + codec = (const codec_capabilities_t*) ((const uint8_t*) codec + codec->length);
> + }
> +
> + 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;
> +
> + u->a2dp.has_mpeg = TRUE;
Should this be put a few lines earlier, before "if (codec->configured &&
seid == 0)"? If we return codec->seid, this function will be called
again once, but is it possible that also the second invocation will
return codec->seid? If it's possible, then u->a2dp.has_mpeg is left as
FALSE, which is probably wrong.
> + 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.
> +
> + 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 +447,89 @@ 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)
> + 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;
> +
> + /* 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;
>
> - return get_caps(u, ret);
> + ret = parse_mpeg_caps(u, ret, &msg.getcaps_rsp);
> + if (ret < 0)
> + return -1;
> + }
> + }
> + return 0;
> }
>
> /* Run from main thread */
> @@ -482,8 +594,54 @@ 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;
> + }
> +
> + mcap->mpf = 0; /* don't use optional IETF payload, send raw frames */
> + mcap->bitrate = 0x8000; /* set for vbr, this covers all cases */
>
> + return 0;
> + }
> + else {
> + pa_log("setup_a2dp: Trying to set-up A2DP Passthrough configuration but no MPEG endpoint available");
> + return -1; /* this should not happen */
Please use assertions for stuff that should not happen. That makes it
much easier to reason about the code - if there's no assertion, the
reader has to assume that it actually is possible that this code is
executed in some situation, and if the reader wants to know what
situation that would be, it may take a significant amount of work to
figure out that in the end the code can not really be ever executed. The
assertion is a promise that somebody else has already done that
checking. Having just a "this should not happen" comment looks like the
author of the code isn't quite sure whether proper error handling is
required or not.
> + }
> + }
> +
> + /* other profiles */
> cap = &u->a2dp.sbc_capabilities;
>
> /* Find the lowest freq that is at least as high as the requested
> @@ -683,8 +841,21 @@ 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) {
No need for if, just use pa_assert(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;
> @@ -692,7 +863,10 @@ static int set_conf(struct userdata *u) {
> if (service_expect(u, &msg.open_rsp.h, sizeof(msg), BT_OPEN, sizeof(msg.open_rsp)) < 0)
> return -1;
>
> - if (u->profile == PROFILE_A2DP || u->profile == PROFILE_A2DP_SOURCE) {
> + if (u->profile == PROFILE_A2DP || u->profile == PROFILE_A2DP_PASSTHROUGH || u->profile == PROFILE_A2DP_SOURCE ) {
> +
> + /* for passthrough we expect little-endian data to be compatible with the ALSA iec958
> + devices */
> u->sample_spec.format = PA_SAMPLE_S16LE;
>
> if (setup_a2dp(u) < 0)
> @@ -712,6 +886,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) {
No need for if, just use pa_assert(u->a2dp.has_mpeg).
> + 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;
> @@ -732,12 +913,18 @@ static int set_conf(struct userdata *u) {
> setup_sbc(&u->a2dp);
>
> u->block_size =
> - ((u->link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
> + ((u->link_mtu - sizeof(struct rtp_header) - sizeof(struct sbc_rtp_payload))
> / u->a2dp.frame_length
> * u->a2dp.codesize);
>
> 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 if (u->profile == PROFILE_A2DP_PASSTHROUGH) {
> +
> + /* available payload per packet */
> + u->block_size = 1152*4; /* this is the size of an IEC61937 frame for MPEG layer 3 */
> + u->leftover_bytes=0;
> +
> } else
> u->block_size = u->link_mtu;
>
> @@ -1248,7 +1435,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;
> @@ -1270,7 +1457,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;
>
> @@ -1378,6 +1565,382 @@ 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] = {
Time to revisit the sample vs frame discussion... I'm sorry I never
replied to your last mail concerning this. I'll reply now.
You wrote:
"It's actually number of samples. Frames only makes sense for PCM, for
compressed data what you need is the number of samples. Frames are
undefined for compressed data."
I'm fairly sure "pcm frame" means something else to you than to me. If
the sampling rate of a pcm stream is 48000 Hz and the stream has two
channels, then one second of that stream contains 48000 pcm frames and
96000 samples when using my definition of terms "pcm frame" and
"sample". I guess you would say that one second of such stream contains
48000 samples and some smaller number of pcm frames - I don't have an
idea what the definition of "pcm frame" would be in this case.
I'd like you to use the term "pcm frame" instead of "sample", because
that way we have separate terms for "sample" as a single number and
"sample" as the collection of numbers that are needed to represent all
channels at one point in time. It's important to have clear terminology
here, because we very much want to know how big one "sample" is in terms
of bytes - it's not nice if the reader has to guess which definition of
"sample" this particular line in code is using.
There already are functions pa_sample_size() and pa_frame_size() that
follow the terminology I'm advocating. The audio APIs that I'm familiar
with use the same terminology: alsa has types called snd_pcm_uframes_t
and snd_pcm_sframes_t and jack has jack_nframes_t.
It's possible that there was no misunderstaning, and you're really
claiming that compressed data doesn't care about pcm frames, but I very
much doubt that...
So, how about renaming this array to pcm_frames_per_mpeg_frame?
> + {0, 576, 1152, 384},
> + {0, 1152, 1152, 384}
> +};
> +
> +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.
> +{
Put the opening brace to the previous line.
> + 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]) /
Fix spaces. Also, you use constant index 1 - that's wrong for < 32kHz
streams, isn't it?
> + 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_iec958(uint8_t **p_src,uint32_t *input_bytes)
const uint8_t **p_src would avoid the need for some casting in the
calling code.
Also, fix spaces.
> +{
Put the opening brace to the previous line.
> + int sync_found=0;
Fix spaces, and use pa_bool_t instead of int.
> + uint32_t preambles;
> + uint8_t *src=*p_src;
Before dereferencing pointers, check that they're not null.
> +
> + 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)
> + break;
> +
> + /* we need 4 bytes to detect the Pa,Pb preambles */
> + preambles = src[0]<<24|src[1]<<16|src[2]<<8|src[3];
Put spaces around the operators.
> +
> + if (preambles == 0x72F81F4E) { /* little endian assumed */
Where is this magic number specified? What document should I read in
order to understand and verify this code?
> + sync_found = 1;
Use TRUE instead of 1.
> + break;
> + } else {
> + /* try to find a new syncword */
> + src += 1;
> + *input_bytes -= 1;
> + }
> + }
> + *p_src=src;
> +
> + return sync_found;
return sync_found ? 0 : -1;
> +} /* end do_sync_iec958() */
Redundant comment.
> +
> +
Extra line.
> +static int a2dp_passthrough_process_render(struct userdata *u) {
In the previous review I said:
"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."
That's still a valid comment, except that claiming that
a2dp_process_render() doesn't have the comment either is of course
wrong, because it does have it... I probably confused
a2dp_process_render() with a2dp_process_push(), which lacks the comment.
You can add the comment to _push() also if you want.
> + struct a2dp_info *a2dp;
> + struct rtp_header *header;
> + struct mpeg_rtp_payload *payload;
> + size_t nbytes;
> + uint8_t *d;
> + const uint8_t *p;
> + size_t bytes_remaining=0; /* in the sink buffer */
Fix spaces.
> + uint32_t sync_found = 0;
Use pa_bool_t.
> + uint32_t confirmed_sync;
Use pa_bool_t.
> + uint32_t frame_length;
> + uint32_t frag_offset;
> + uint32_t max_bytes;
> + 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);
> +
> + /* allocate memory if needed */
> + 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 no sync was found, handle the remaining part of the previous buffer */
> + if (u->leftover_bytes) {
> + 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);
> +
> + 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);
> +
> + bytes_remaining = u->block_size;
> +
> + /* try to find an IEC958 pattern in the buffer */
> + p = (const uint8_t*) pa_memblock_acquire(u->write_memchunk.memblock);
Why cast here?
> + sync_found = do_sync_iec958((uint8_t**)&p,&bytes_remaining);
Fix spaces.
> + pa_memblock_release(u->write_memchunk.memblock);
> +
> + /* update pointers */
> + u->write_memchunk.index = u->block_size-bytes_remaining;
Fix spaces.
> + u->write_memchunk.length = bytes_remaining;
> + u->leftover_bytes=bytes_remaining;
Fix spaces.
> +
> + if (sync_found && u->leftover_bytes!=u->block_size) {
Fix spaces.
> + /* only a partial frame found, handle in the next call */
> + sync_found = 0;
Use FALSE instead of 0.
> + }
> +
> + confirmed_sync = 0;
Use FALSE instead of 0.
> + if (sync_found) {
> + /* we are aligned on an IEC958 frame boundary */
> + p = (const uint8_t*) pa_memblock_acquire(u->write_memchunk.memblock);
Starting from this line, the code is indented one space too much.
Also, why cast here?
> +
> + if (p[4] == 0x05) { /* MPEG payload */
> + uint32_t frame_length2 = 0;
> + uint32_t sample_len = 0;
> +
> + /* valid MPEG payload, now extract frame length */
> +
> + frame_length = p[7] << 8 | p[6]; /* in bits */
> + frame_length >>= 3; /* in bytes */
> +
> + if (!mp3_synclength(p[9]<<24|p[8]<<16|p[11]<<8|p[10], &frame_length2, &sample_len)) {
Put spaces between the operators. Personally, I'd create a variable for
the first function parameter, but if you don't like that, then no need
to do that.
Also, usually success check is not done in PA code with the ! operator
but like this:
if (mp3_synclength(p[9] << 24 | p[8] << 16 | p[11] << 8 | p[10], &frame_length2, &sample_len) >= 0)
That of course doesn't work as long as mp3_synclength() returns a
positive number on failure, but that is easily fixed by making the
failure code negative.
> + if( frame_length == frame_length2) {
Fix spaces.
> + /* now we have verified that the IEC frame and MPEG frame are in
> + agreement, we should be good to go */
> + confirmed_sync = 1;
> + }
> + } else {
> + pa_log("IEC958 sync found but no MP3 sync found");
> + }
> + }
> + pa_memblock_release(u->write_memchunk.memblock);
> + }
> +
> + /* no valid sync found, clean-up and return */
> + if (!confirmed_sync) {
> + int toflush;
> +
> + toflush = u->block_size - bytes_remaining;
> + if (toflush == 0)
> + toflush = u->block_size;
> + u->write_index += (uint64_t)toflush;
> +
> + /* clean-up and move on */
> + if (u->leftover_bytes == u->block_size) {
> + pa_memblock_unref(u->write_memchunk.memblock);
> + pa_memchunk_reset(&u->write_memchunk);
> + u->leftover_bytes = 0;
> + }
> +
> + return 1;
> + }
> +
> + /* now push the MP3 data into the A2DP link */
> +
> + /* set pointers for RTP header and payload */
> + a2dp = &u->a2dp;
> + header = a2dp->buffer;
> + payload = (struct mpeg_rtp_payload*) ((uint8_t*) a2dp->buffer + sizeof(*header));
> +
> + /* set pointer for MP3 frame */
> + p = (const uint8_t*) pa_memblock_acquire(u->write_memchunk.memblock);
Why cast here?
> + p += 8;
> +
> + max_bytes = u->link_mtu - sizeof(*header) - sizeof(*payload);
> + max_bytes >>= 1;
> + max_bytes <<= 1; /* make sure we handle byte pairs */
> +
> + /* round to ceiling for frame length, required for byte swaps */
> + frame_length ++;
> + frame_length >>= 1;
> + frame_length <<= 1;
> +
> + frag_offset = 0;
> +
> + while (frame_length != 0) { /* several iterations if frame_length larger than mtu */
> + uint32_t length;
> + uint i;
Everywhere else plain unsigned ints are declared using "unsigned"
instead of "uint".
> +
> + length = frame_length;
> + if (length > max_bytes)
> + length = max_bytes;
> + frame_length -= length;
> +
> + d = (uint8_t*) a2dp->buffer + sizeof(*header) + sizeof(*payload);
> +
> + /* swap bytes, input is assumed 16-bit little endian */
> + pa_assert( !(length&1) );
Fix spaces.
> +
> + for (i=0;i<length/2;i++) {
Fix spaces.
> + *(d+1) = *p;
> + *d = *(p+1);
> + d += 2;
> + p += 2;
> + }
> +
> + nbytes = (uint8_t*) d - (uint8_t*) a2dp->buffer;
Starting from this line, the code is indented one space too much.
> + pa_assert(nbytes!=0);
Fix spaces.
> +
> + /* fill RTP header and payload */
> + memset(a2dp->buffer, 0, sizeof(*header) + sizeof(*payload));
> + header->v = 2; /* rtp packet v2 */
> + header->pt = 14; /* MPA payload type */
> + header->timestamp =
> + htonl(u->write_index / pa_frame_size(&u->sample_spec)*
Space missing before the * operator, but actually I would prefer
operators to go to the start of the new line when wrapping long lines.
> + 90000/u->sample_spec.rate); /* 90kHz timestamp */
Put spaces around /.
> + header->m = 1; /* talk_spurt is always set */
> + header->sequence_number = htons(a2dp->seq_num);
> + header->ssrc = htonl(1);
> +
> + payload->mbz = 0; /* always zero */
> + payload->frag_offset = frag_offset; /* non-zero only when frame is spread across multiple packets */
> +
> + pa_assert(nbytes != 0);
> + pa_assert(nbytes <= u->link_mtu);
> +
> + ret = 0;
> + 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;
> + }
> +
> + ret = 1;
> + break;
> + }
> +
> + if (ret != 1) {
> + /* stop this loop */
> + frame_length = 0;
> + }
> +
> + frag_offset += length;
> + }
> +
> + pa_memblock_release(u->write_memchunk.memblock);
> +
> + /* clean-up and move on */
> + if (u->leftover_bytes == u->block_size) {
> + pa_memblock_unref(u->write_memchunk.memblock);
> + pa_memchunk_reset(&u->write_memchunk);
> + u->leftover_bytes = 0;
> + }
> +
> + /* update counters */
> + u->write_index += (uint64_t) u->block_size;
> + a2dp->seq_num++;
> +
> + return ret;
> +}
> +
> static int a2dp_process_push(struct userdata *u) {
> int ret = 0;
> pa_memchunk memchunk;
> @@ -1395,7 +1958,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;
> @@ -1406,7 +1969,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);
>
> @@ -1598,6 +2161,9 @@ static void thread_func(void *userdata) {
> 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;
> @@ -1884,7 +2450,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;
> @@ -1910,7 +2476,9 @@ static int add_sink(struct userdata *u) {
>
> 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) +
> + (((u->profile == PROFILE_A2DP)||
> + (u->profile == PROFILE_A2DP_PASSTHROUGH)) ?
> + FIXED_LATENCY_PLAYBACK_A2DP : FIXED_LATENCY_PLAYBACK_HSP) +
> pa_bytes_to_usec(u->block_size, &u->sample_spec));
> }
>
> @@ -1919,6 +2487,9 @@ static int add_sink(struct userdata *u) {
> u->sink->n_volume_steps = 16;
> }
>
> + if (u->profile == PROFILE_A2DP_PASSTHROUGH) {
> + u->sink->flags |= PA_SINK_PASSTHROUGH;
> + }
No need for braces in simple ifs like this.
> return 0;
> }
>
> @@ -2103,7 +2674,7 @@ static int bt_transport_config_a2dp(struct userdata *u)
> a2dp->frame_length = sbc_get_frame_length(&a2dp->sbc);
>
> u->block_size =
> - ((u->link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
> + ((u->link_mtu - sizeof(struct rtp_header) - sizeof(struct sbc_rtp_payload))
> / a2dp->frame_length
> * a2dp->codesize);
>
> @@ -2298,6 +2869,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)
> @@ -2443,6 +3015,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;
> @@ -2551,6 +3127,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)) {
> @@ -2625,6 +3215,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
> diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
> index 065fd2d..cafc561 100644
> --- a/src/pulsecore/sink-input.c
> +++ b/src/pulsecore/sink-input.c
> @@ -296,18 +296,20 @@ int pa_sink_input_new(
> !pa_sample_spec_equal(&data->sample_spec, &data->sink->sample_spec) ||
> !pa_channel_map_equal(&data->channel_map, &data->sink->channel_map)) {
>
> - if (!(resampler = pa_resampler_new(
> - core->mempool,
> - &data->sample_spec, &data->channel_map,
> - &data->sink->sample_spec, &data->sink->channel_map,
> - data->resample_method,
> - ((data->flags & PA_SINK_INPUT_VARIABLE_RATE) ? PA_RESAMPLER_VARIABLE_RATE : 0) |
> - ((data->flags & PA_SINK_INPUT_NO_REMAP) ? PA_RESAMPLER_NO_REMAP : 0) |
> - (core->disable_remixing || (data->flags & PA_SINK_INPUT_NO_REMIX) ? PA_RESAMPLER_NO_REMIX : 0) |
> - (core->disable_lfe_remixing ? PA_RESAMPLER_NO_LFE : 0)))) {
> - pa_log_warn("Unsupported resampling operation.");
> - return -PA_ERR_NOTSUPPORTED;
> - }
> + /* FIXME: for passthrough content we need to adjust the output rate to that of the current sink-input */
> + if ( !(data->flags& PA_SINK_INPUT_PASSTHROUGH)) /* no resampler for passthrough content */
> + if (!(resampler = pa_resampler_new(
> + core->mempool,
> + &data->sample_spec, &data->channel_map,
> + &data->sink->sample_spec, &data->sink->channel_map,
> + data->resample_method,
> + ((data->flags & PA_SINK_INPUT_VARIABLE_RATE) ? PA_RESAMPLER_VARIABLE_RATE : 0) |
> + ((data->flags & PA_SINK_INPUT_NO_REMAP) ? PA_RESAMPLER_NO_REMAP : 0) |
> + (core->disable_remixing || (data->flags & PA_SINK_INPUT_NO_REMIX) ? PA_RESAMPLER_NO_REMIX : 0) |
> + (core->disable_lfe_remixing ? PA_RESAMPLER_NO_LFE : 0)))) {
> + pa_log_warn("Unsupported resampling operation.");
> + return -PA_ERR_NOTSUPPORTED;
> + }
> }
>
> i = pa_msgobject_new(pa_sink_input);
--
Tanu
More information about the pulseaudio-discuss
mailing list