[Bluez-devel] [PATCH] pcm_bluetooth : fix a2dp streaming

[Bluez-devel] [PATCH] pcm_bluetooth : fix a2dp streaming

[Fabien Chevalier]

[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]

Johan & All,

Following recent mailing list discussions, i decided to implement some 
of the ideas that have been suggested recently. :-)

So i heavily modified the A2DP streaming part of the pcm plugin

The user-visible features of this patch:
   * much reduced latency (You should love this one Frederic)
   * supports headsets that are unable to throttle the data flow and
     that play too fast or drop packets or whatever (Marcel, could you
     please confirm that fixes the issues you had with some headsets ?).

This patch is fairly intrusive, and removes some code that i judjed 
useless now, like the bandwidth measurement stuff, and has greatly 
simplifyed transfer loop for avdtp (see comments in the code for the 
justifications)

It brings in a thread that has for only reponsibility to increment the 
so called hardware pointer (which doesn't point to anything in our 
case), and notify the application trough writing in a pipe that some 
room is available (virtual room again), so that it could send more data.

The attached file details the tests i've run to check i didn't break 
anything. All is conclusive except playing with XMMS that goes underrun 
for a reason i haven spotted yet. Apart from that aplay, mplayer, 
gstreamer are fine.

I'm looking forward for any questions you have on the patch.

Nest step would be to fix the SCO part of the plugin, but this one is 
looks harder as i think it will require tweaking the kernel SCO part.

Regards,

Fabien

[-- Attachment #2: pcm_bluetooth.c.patch --]
[-- Type: text/x-patch, Size: 14534 bytes --]

diff -u -r1.31 pcm_bluetooth.c
--- pcm_bluetooth.c	15 Aug 2007 10:36:16 -0000	1.31
+++ pcm_bluetooth.c	19 Aug 2007 16:24:22 -0000
@@ -28,6 +28,7 @@
 #include <sys/socket.h>
 #include <sys/un.h>
 #include <sys/time.h>
+#include <pthread.h>
 
 #include <netinet/in.h>
 
@@ -93,13 +94,13 @@
 	uint16_t seq_num;		/* */
 	int frame_count;		/* */
 
-	int bandwithcount;
-	struct timeval bandwithtimestamp;
+	pthread_t hw_thread;            /* Makes virtual hw pointer move */	
+	int pipefd[2];			/* Inter thread communication */
 };
 
 struct bluetooth_data {
 	snd_pcm_ioplug_t io;
-	snd_pcm_sframes_t hw_ptr;
+	volatile snd_pcm_sframes_t hw_ptr;
 	struct ipc_data_cfg cfg;	/* Bluetooth device config */
 	int stream_fd;			/* Audio stream filedescriptor */
 	int sock;			/* Daemon unix socket */
@@ -108,6 +109,8 @@
 	struct bluetooth_a2dp a2dp;	/* a2dp data */
 };
 
+static void * a2dp_playback_hw_thread(void *param);
+
 void memcpy_changeendian(void *dst, const void *src, int size)
 {
 	int i;
@@ -132,6 +135,43 @@
 	return 0;
 }
 
+static int bluetooth_a2dp_playback_start(snd_pcm_ioplug_t *io)
+{
+	struct bluetooth_data *data = io->private_data;
+	struct bluetooth_a2dp *a2dp_data = &data->a2dp;	
+	int ret = 0;
+	DBG("%p", io);
+
+	assert(a2dp_data->hw_thread == 0);
+	ret = -pthread_create(&a2dp_data->hw_thread, 0, a2dp_playback_hw_thread, data);
+
+	DBG(" - return %d", ret);
+	return ret;
+}
+
+static int bluetooth_a2dp_playback_stop(snd_pcm_ioplug_t *io)
+{
+	DBG("%p", io);
+	struct bluetooth_data *data = io->private_data;
+	struct bluetooth_a2dp *a2dp_data = &data->a2dp;	
+	int ret = 0;
+
+	/* Beware - We can be called more than once */
+	if(a2dp_data->hw_thread != 0) {
+		ret = -pthread_cancel(a2dp_data->hw_thread);
+		if(ret != 0) {
+			goto end;
+		}
+
+		ret = -pthread_join(a2dp_data->hw_thread, 0);
+	}
+
+end:
+	a2dp_data->hw_thread = 0;
+	DBG(" - return %d", ret);
+	return ret;
+}
+
 static snd_pcm_sframes_t bluetooth_pointer(snd_pcm_ioplug_t *io)
 {
 	struct bluetooth_data *data = io->private_data;
@@ -154,6 +194,12 @@
 	if (data->cfg.codec == CFG_CODEC_SBC)
 		sbc_finish(&data->a2dp.sbc);
 
+	if(data->a2dp.pipefd[0] > 0)
+		close(data->a2dp.pipefd[0]);
+	
+	if(data->a2dp.pipefd[1] > 0)
+		close(data->a2dp.pipefd[1]);
+	
 	free(data);
 }
 
@@ -171,6 +217,8 @@
 static int bluetooth_prepare(snd_pcm_ioplug_t *io)
 {
 	struct bluetooth_data *data = io->private_data;
+	int ret;
+	char c;	
 
 	DBG("Preparing with io->period_size = %lu, io->buffer_size = %lu",
 			io->period_size, io->buffer_size);
@@ -184,7 +232,9 @@
 		 * If it is, capture won't start */
 		data->hw_ptr = io->period_size;
 
-	return 0;
+	/* a2dp : wake up any client polling at us */	
+	ret = write(data->a2dp.pipefd[1], &c, 1);
+	return ret;
 }
 
 static int bluetooth_hsp_hw_params(snd_pcm_ioplug_t *io,
@@ -239,6 +289,111 @@
 	return -err;
 }
 
+static int bluetooth_poll_descriptors(snd_pcm_ioplug_t *io, 
+					struct pollfd *pfd, 
+					unsigned int space)
+{
+	assert(io);
+	assert(space >= 1);
+
+	pfd[0].fd = ((struct bluetooth_data *)io->private_data)->stream_fd;
+	pfd[0].events = POLLIN;
+	pfd[0].revents = 0;
+
+	return 1;
+}
+
+static int bluetooth_poll_revents(snd_pcm_ioplug_t *io ATTRIBUTE_UNUSED,
+				     struct pollfd *pfds, unsigned int nfds,
+				     unsigned short *revents)
+{
+	assert(pfds && nfds == 1 && revents);
+
+	*revents = pfds[0].revents;
+	return 0;
+}
+
+static int bluetooth_a2dp_playback_poll_descriptors(snd_pcm_ioplug_t *io, 
+							struct pollfd *pfd, 
+							unsigned int space)
+{
+	struct bluetooth_data *data = io->private_data;
+
+	DBG("");
+
+	assert(io);
+	assert(space >= 1);
+	assert(data->a2dp.pipefd[0] != 0);
+
+	pfd[0].fd = data->a2dp.pipefd[0];
+	pfd[0].events = POLLIN;
+	pfd[0].revents = 0;
+
+	return 1;
+}
+
+static int bluetooth_a2dp_playback_poll_revents(snd_pcm_ioplug_t *io,
+				     struct pollfd *pfds, unsigned int nfds,
+				     unsigned short *revents)
+{
+	static char buf[1];
+	int ret = 0;	
+
+	DBG("");
+
+	assert(pfds);
+	assert(nfds == 1);
+	assert(revents);
+	assert(pfds[0].fd != 0);
+
+	if(io->state != SND_PCM_STATE_PREPARED) {	
+		ret = read(pfds[0].fd, buf, 1);
+	}	
+	*revents = (pfds[0].revents & ~POLLIN) | POLLOUT;
+	return 0;
+}
+
+static void * a2dp_playback_hw_thread(void *param)
+{
+	struct bluetooth_data* data = (struct bluetooth_data *)param;
+	unsigned int num_period_elapsed = 0;
+	unsigned long long starttime; /* in usecs */
+	unsigned long long curtime; /* in usecs */
+	struct timeval tv;
+	int ret;
+
+	gettimeofday(&tv, 0);
+	starttime = tv.tv_sec * 1000000 + tv.tv_usec;
+
+	#define PERIOD_TIME_USECS (1000000.0 * \
+		(data->io.period_size) / \
+		 data->io.rate)
+	for(;;) {
+		gettimeofday(&tv, 0);
+		curtime = tv.tv_sec * 1000000 + tv.tv_usec;
+		/* How much time period_time has elapsed since the thread started ? */
+		unsigned int ntimes = (1.0 * (curtime - starttime)) 
+					/ PERIOD_TIME_USECS;
+		if(ntimes > num_period_elapsed) {
+			char c;			
+			data->hw_ptr = (data->hw_ptr + 
+					(ntimes - num_period_elapsed)
+					* data->io.period_size)
+					% data->io.buffer_size;
+			DBG("pointer = %ld", data->hw_ptr);			
+			/* Notify user that hardware pointer has moved */
+			ret = write(data->a2dp.pipefd[1], &c, 1);
+			assert(ret == 1);			
+			num_period_elapsed = ntimes;
+		}		
+		/* Period time is usually no shorter that 1 ms, 
+		   no need to sleep for a shorter amount of time */
+		usleep(1000);
+		/* Offer opportunity to be canceled by main thread */		
+		pthread_testcancel();	
+	}
+}
+
 static snd_pcm_sframes_t bluetooth_hsp_read(snd_pcm_ioplug_t *io,
 						const snd_pcm_channel_area_t *areas,
 						snd_pcm_uframes_t offset,
@@ -364,19 +519,14 @@
 	return ret;
 }
 
-static int avdtp_write(struct bluetooth_data *data, unsigned int nonblock)
+static int avdtp_write(struct bluetooth_data *data)
 {
-	int count = 0, written = 0, ret = 0;
+	int ret = 0;
 	struct rtp_header *header;
 	struct rtp_payload *payload;
 	struct bluetooth_a2dp *a2dp = &data->a2dp;
-#ifdef ENABLE_DEBUG
-	static struct timeval send_date = { 0, 0 };
-	static struct timeval prev_date = { 0, 0 };
-	struct timeval send_delay = { 0, 0 };
-	struct timeval sendz_delay = { 0, 0 };
-#endif
 
+	DBG("");
 	header = (void *) a2dp->buffer;
 	payload = (void *) (a2dp->buffer + sizeof(*header));
 
@@ -389,98 +539,24 @@
 	header->timestamp = htonl(a2dp->nsamples);
 	header->ssrc = htonl(1);
 
-	while (count++ < 10) {
-#ifdef ENABLE_DEBUG
-		gettimeofday(&send_date, NULL);
-#endif
-		ret = send(data->stream_fd, a2dp->buffer, a2dp->count,
-				nonblock ? MSG_DONTWAIT : 0);
-		if (ret < 0) {
-			ret = -errno;
-			if (errno == EAGAIN)
-				goto retry;
-			fprintf(stderr, "send: %s (%d)\n", strerror(errno),
-					errno);
-			goto done;
-		}
-
-		written += ret;
-
-#ifdef ENABLE_DEBUG
-		if ((written >= 0 || errno == EAGAIN) && prev_date.tv_sec != 0) {
-			long delay, real, theo, delta;
+	ret = send(data->stream_fd, a2dp->buffer, a2dp->count,
+				MSG_DONTWAIT);
+	if(ret == -1)
+		ret = -errno;
+
+	/* Kernel side l2cap socket layer makes sure either everything 
+	   is buffered for sending, or nothing is buffered.
+	   This assertion is to remind people of this fact (and be noticed
+	   the day that changes)
+ 	   */
+	assert(ret < 0 || ret == a2dp->count);
 
-			delay = (long) (send_delay.tv_sec * 1000 +
-						send_delay.tv_usec / 1000),
-			real = (long) (sendz_delay.tv_sec * 1000 +
-						sendz_delay.tv_usec / 1000);
-			theo = (long) (((float) a2dp->nsamples) /
-						data->cfg.rate * 1000.0);
-			delta = (long) (sendz_delay.tv_sec * 1000 +
-						sendz_delay.tv_usec / 1000) -
-					(long) (((float) a2dp->nsamples) /
-							data->cfg.rate * 1000.0);
-
-			timersub(&send_date, &prev_date, &send_delay);
-			timersub(&send_date, &a2dp->ntimestamp, &sendz_delay);
-
-			printf("send %d (cumul=%d) samples (delay=%ld ms,"
-					" real=%ld ms, theo=%ld ms,"
-					" delta=%ld ms).\n", a2dp->samples,
-					a2dp->nsamples, delay, real, theo,
-					delta);
-		}
-#endif
-		if (written == a2dp->count)
-			break;
-
-		a2dp->count -= written;
-
-retry:
-		DBG("send (retry).");
-		usleep(150000);
-	}
-
-#ifdef ENABLE_DEBUG
-	prev_date = send_date;
-#endif
-
-	if (written != a2dp->count)
-		printf("Wrote %d not %d bytes\n", written, a2dp->count);
-
-#ifdef ENABLE_DEBUG
-	else {
-		/* Measure bandwith usage */
-		struct timeval now = { 0, 0 };
-		struct timeval interval = { 0, 0 };
-
-		if(a2dp->bandwithtimestamp.tv_sec == 0)
-			gettimeofday(&a2dp->bandwithtimestamp, NULL);
-
-		/* See if we must wait again */
-		gettimeofday(&now, NULL);
-		timersub(&now, &a2dp->bandwithtimestamp, &interval);
-		if(interval.tv_sec > 0)
-			printf("Bandwith: %d (%d kbps)\n", a2dp->bandwithcount,
-				a2dp->bandwithcount / 128);
-		a2dp->bandwithtimestamp = now;
-		a2dp->bandwithcount = 0;
-	}
-
-	a2dp->bandwithcount += written;
-
-#endif
-
-done:
 	/* Reset buffer of data to send */
 	a2dp->count = sizeof(struct rtp_header) + sizeof(struct rtp_payload);
 	a2dp->frame_count = 0;
 	a2dp->samples = 0;
 	a2dp->seq_num++;
 
-	if (written > 0)
-		return written;
-
 	return ret;
 }
 
@@ -497,9 +573,35 @@
 	uint8_t *buff;
 	static int codesize = 0;
 
-	DBG("areas->step=%u, areas->first=%u, offset=%lu, size=%lu,"
-			"io->nonblock=%u", areas->step, areas->first,
-			offset, size, io->nonblock);
+	DBG("areas->step=%u, areas->first=%u, offset=%lu, size=%lu"
+			, areas->step, areas->first, offset, size);
+	DBG("hw_ptr = %lu, appl_ptr = %lu"
+			, io->hw_ptr, io->appl_ptr);
+
+	if(io->hw_ptr > io->appl_ptr) {
+		ret = bluetooth_a2dp_playback_stop(io);
+		if(ret == 0)		
+			ret = -EPIPE;
+		goto done;	
+	}		
+
+	/* Check if we should autostart */
+	if(io->state == SND_PCM_STATE_PREPARED) {
+		snd_pcm_sw_params_t *swparams;
+		snd_pcm_uframes_t threshold;
+	
+		snd_pcm_sw_params_malloc(&swparams);
+		if(!snd_pcm_sw_params_current(io->pcm, swparams)
+		   && !snd_pcm_sw_params_get_start_threshold(swparams,
+		       &threshold) ) {
+			if(io->appl_ptr >= threshold) {
+				ret = snd_pcm_start(io->pcm);
+				if(ret != 0)
+					goto done;			
+			}
+		}
+		snd_pcm_sw_params_free(swparams);	
+	}
 
 	if (codesize == 0) {
 		/* How much data can be encoded by sbc at a time? */
@@ -548,7 +650,7 @@
 	DBG("encoded = %d  a2dp.sbc.len= %d", encoded, a2dp->sbc.len);
 
 	if (a2dp->count + a2dp->sbc.len >= data->cfg.pkt_len) {
-		ret = avdtp_write(data, io->nonblock);
+		ret = avdtp_write(data);
 		if (ret < 0) {
 			if (-ret == EPIPE)
 				ret = -EIO;
@@ -561,55 +663,60 @@
 	a2dp->frame_count++;
 	a2dp->samples += encoded / frame_size;
 	a2dp->nsamples += encoded / frame_size;
-	/* Increment hardware transmition pointer */
-	data->hw_ptr = (data->hw_ptr + encoded / frame_size)
-			% io->buffer_size;
 
 	ret = frames_to_read;
 
 done:
-	DBG("returning %lu", ret);
+	DBG("returning %ld", ret);
 	return ret;
 }
 
 static snd_pcm_ioplug_callback_t bluetooth_hsp_playback = {
-	.start		= bluetooth_start,
-	.stop		= bluetooth_stop,
-	.pointer	= bluetooth_pointer,
-	.close		= bluetooth_close,
-	.hw_params	= bluetooth_hsp_hw_params,
-	.prepare	= bluetooth_prepare,
-	.transfer	= bluetooth_hsp_write,
+	.start			= bluetooth_start,
+	.stop			= bluetooth_stop,
+	.pointer		= bluetooth_pointer,
+	.close			= bluetooth_close,
+	.hw_params		= bluetooth_hsp_hw_params,
+	.prepare		= bluetooth_prepare,
+	.transfer		= bluetooth_hsp_write,
+	.poll_descriptors	= bluetooth_poll_descriptors,
+	.poll_revents		= bluetooth_poll_revents,
 };
 
 static snd_pcm_ioplug_callback_t bluetooth_hsp_capture = {
-	.start		= bluetooth_start,
-	.stop		= bluetooth_stop,
-	.pointer	= bluetooth_pointer,
-	.close		= bluetooth_close,
-	.hw_params	= bluetooth_hsp_hw_params,
-	.prepare	= bluetooth_prepare,
-	.transfer	= bluetooth_hsp_read,
+	.start			= bluetooth_start,
+	.stop			= bluetooth_stop,
+	.pointer		= bluetooth_pointer,
+	.close			= bluetooth_close,
+	.hw_params		= bluetooth_hsp_hw_params,
+	.prepare		= bluetooth_prepare,
+	.transfer		= bluetooth_hsp_read,
+	.poll_descriptors	= bluetooth_poll_descriptors,
+	.poll_revents		= bluetooth_poll_revents,
 };
 
 static snd_pcm_ioplug_callback_t bluetooth_a2dp_playback = {
-	.start		= bluetooth_start,
-	.stop		= bluetooth_stop,
-	.pointer	= bluetooth_pointer,
-	.close		= bluetooth_close,
-	.hw_params	= bluetooth_a2dp_hw_params,
-	.prepare	= bluetooth_prepare,
-	.transfer	= bluetooth_a2dp_write,
+	.start			= bluetooth_a2dp_playback_start,
+	.stop			= bluetooth_a2dp_playback_stop,
+	.pointer		= bluetooth_pointer,
+	.close			= bluetooth_close,
+	.hw_params		= bluetooth_a2dp_hw_params,
+	.prepare		= bluetooth_prepare,
+	.transfer		= bluetooth_a2dp_write,
+	.poll_descriptors	= bluetooth_a2dp_playback_poll_descriptors,
+	.poll_revents		= bluetooth_a2dp_playback_poll_revents,
 };
 
 static snd_pcm_ioplug_callback_t bluetooth_a2dp_capture = {
-	.start		= bluetooth_start,
-	.stop		= bluetooth_stop,
-	.pointer	= bluetooth_pointer,
-	.close		= bluetooth_close,
-	.hw_params	= bluetooth_a2dp_hw_params,
-	.prepare	= bluetooth_prepare,
-	.transfer	= bluetooth_a2dp_read,
+	.start			= bluetooth_start,
+	.stop			= bluetooth_stop,
+	.pointer		= bluetooth_pointer,
+	.close			= bluetooth_close,
+	.hw_params		= bluetooth_a2dp_hw_params,
+	.prepare		= bluetooth_prepare,
+	.transfer		= bluetooth_a2dp_read,
+	.poll_descriptors	= bluetooth_poll_descriptors,
+	.poll_revents		= bluetooth_poll_revents,
 };
 
 #define ARRAY_NELEMS(a) (sizeof((a)) / sizeof((a)[0]))
@@ -661,7 +768,7 @@
 		return err;
 
 	err = snd_pcm_ioplug_set_param_minmax(io, SND_PCM_IOPLUG_HW_PERIODS,
-					2, 200);
+					2, 50);
 	if (err < 0)
 		return err;
 
@@ -738,6 +845,14 @@
 	a2dp->sbc.blocks = sbc->blocks;
 	a2dp->sbc.bitpool = sbc->bitpool;
 
+
+	if(pipe(a2dp->pipefd) != 0)
+		return -errno;
+	if(fcntl(a2dp->pipefd[0], F_SETFL, O_NONBLOCK) != 0)
+		return -errno;	
+	if(fcntl(a2dp->pipefd[1], F_SETFL, O_NONBLOCK) != 0)
+		return -errno;
+
 	return 0;
 }
 
@@ -888,7 +1003,7 @@
 	DBG("Bluetooth PCM plugin (%s)",
 		stream == SND_PCM_STREAM_PLAYBACK ? "Playback" : "Capture");
 
-	data = malloc(sizeof(struct bluetooth_data));
+	data = calloc(1, sizeof(struct bluetooth_data));
 	if (!data) {
 		err = -ENOMEM;
 		goto error;
@@ -901,9 +1016,6 @@
 	data->io.version = SND_PCM_IOPLUG_VERSION;
 	data->io.name = "Bluetooth Audio Device";
 	data->io.mmap_rw = 0; /* No direct mmap communication */
-	data->io.poll_fd = data->stream_fd;
-	data->io.poll_events = stream == SND_PCM_STREAM_PLAYBACK ?
-					POLLOUT : POLLIN;
 	data->io.private_data = data;
 
 	if (data->cfg.codec == CFG_CODEC_SBC)

[-- Attachment #3: plugin non reg cookbook.txt --]
[-- Type: text/plain, Size: 890 bytes --]

Bluetooth alsa a2dp non regression cookbook
-------------------------------------------

I) PCM test tool shipped with ALSA

Setup:
*Download the alsa-utils source for the binary that run on your machine
*build the pcm executable in the test directory

Run:
./pcm -c 2 -r 48000 -Dbluetooth -m write -b 10000
./pcm -c 2 -r 48000 -Dbluetooth -m write_and_poll -b 10000
./pcm -c 2 -r 48000 -Dbluetooth -m direct_write -b 10000

Result:
You should hear a clear sine in the headset.
pcm process should not eat 100% cpu (check with top)
Occasionnal sound drops are OK (due to quite short buffering of 10 ms)

II) Play a file from XMMS

III) Play file from totem
Setup:
* gconftool -t string -s /system/gstreamer/0.10/default/musicaudiosink "alsasink device=bluetooth"

Run:
Launch totem, then select a file for play

III) PLay a file from mplayer
mplayer -ao alsa:device=bluetooth filetoplay.wav

[-- Attachment #4: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

[-- Attachment #5: Type: text/plain, Size: 164 bytes --]

_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] pcm_bluetooth : fix a2dp streaming

[Marcel Holtmann]

Hi Fabien,

> Following recent mailing list discussions, i decided to implement some 
> of the ideas that have been suggested recently. :-)
> 
> So i heavily modified the A2DP streaming part of the pcm plugin
> 
> The user-visible features of this patch:
>    * much reduced latency (You should love this one Frederic)
>    * supports headsets that are unable to throttle the data flow and
>      that play too fast or drop packets or whatever (Marcel, could you
>      please confirm that fixes the issues you had with some headsets ?).
> 
> This patch is fairly intrusive, and removes some code that i judjed 
> useless now, like the bandwidth measurement stuff, and has greatly 
> simplifyed transfer loop for avdtp (see comments in the code for the 
> justifications)
> 
> It brings in a thread that has for only reponsibility to increment the 
> so called hardware pointer (which doesn't point to anything in our 
> case), and notify the application trough writing in a pipe that some 
> room is available (virtual room again), so that it could send more data.
> 
> The attached file details the tests i've run to check i didn't break 
> anything. All is conclusive except playing with XMMS that goes underrun 
> for a reason i haven spotted yet. Apart from that aplay, mplayer, 
> gstreamer are fine.
> 
> I'm looking forward for any questions you have on the patch.

I tested the patch with my Logitech headphone and it makes beep work
fine. Using xmms has serious problems. This keeps happening:

** WARNING **: alsa_write_audio(): write error: Resource temporarily unavailable

However I do have blanks with beep, too. It can also crash:

beep-media-player: pcm_bluetooth.c:386: a2dp_playback_hw_thread:
Assertion `ret == 1' failed.

This is triggered by a song end where the stream is still open, but beep
fails to enhance to the next track.

Using usleep() seems not precise enough to me. Also we might not wanna
use a pipe. Some other notification mechanism might be better. I don't
really know what solution is best here.

When using mplayer to watch a movie, I have sound blanks from time to
time and it also crashed:

mplayer: pcm_bluetooth.c:386: a2dp_playback_hw_thread: Assertion `ret == 1' failed.

MPlayer interrupted by signal 6 in module: sleep_timer

However it is a huge improvement and a big step in the right direction.
I know that Luiz was working on a similar patch. Try to get on #bluez on
freenode.org so the future work can be coordinated.

Besides that, please fix the coding style. I am serious about that. I am
picky about whitespace placements, but you already knew that:

Every "if" is following by a whitespace like "if (". No exceptions.

Also stuff like this is not acceptable:

pfd[0].fd = ((struct bluetooth_data *)io->private_data)->stream_fd;

If I have to twist my mind too much to understand why this code is
correct it is too complex. Do something like this:

struct bluetooth_data *data = io->private_data;
pfd[0].fd = data->stream_fd;

Don't invert the result of a function call:

ret = -pthread_create(&a2dp_data->hw_thread, 

However thought this is a good idea is wrong. This is big hole for
overlooking sign errors. Don't do it. Do this instead:

return -ret

And while we are at it. Don't initiate variables with a value:

int ret = 0;

This is only needed in a few rare cases and will hide errors in the code
flow that otherwise gcc might have caught.

And instead of "ret" use "err" which I prefer.

The assert() statements are nice for development, but we can't have them
in the final version. We have to survive as much errors as possible and
better play nothing than crash.

The "for(;;) {" statement are better replaced with "while (1) {".

For the "volatile snd_pcm_sframes_t hw_ptr;". Do you think that is
enough and we not better use a lock or a mutex?

Regards

Marcel



-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] pcm_bluetooth : fix a2dp streaming

[Fabien Chevalier]

Hi Marcel,

please find below some comments.

> 
> I tested the patch with my Logitech headphone and it makes beep work
> fine. Using xmms has serious problems. This keeps happening:
> 
> ** WARNING **: alsa_write_audio(): write error: Resource temporarily unavailable

Interesting... that didn't happen to me !

> 
> However I do have blanks with beep, too. It can also crash:

Looks like the same issue XMMS has, where underrun can occasionally happen.

> 
> beep-media-player: pcm_bluetooth.c:386: a2dp_playback_hw_thread:
> Assertion `ret == 1' failed.
> 
> This is triggered by a song end where the stream is still open, but beep
> fails to enhance to the next track.

That basically means beep forgots to close the stream in a timely maneer 
after those kind of issues. I should SNDERR() something rather than 
assert on this one however...

> 
> Using usleep() seems not precise enough to me. 
Well considering that you definetely *don't need* any kind of precise 
timing for streaming in general, and a2dp in particular, usleep is an 
order of magnitude too precise already. In fact 10 ms granularity would 
be more than enough.

Also we might not wanna
> use a pipe. Some other notification mechanism might be better. I don't
> really know what solution is best here.

Well, it has to be conveyed using a file descriptor due to ALSA using 
poll() to be notifyed of hardware wakeup.
So unless anybody has a better idea, i consider a pipe as the best way 
to go ;-)

> 
> When using mplayer to watch a movie, I have sound blanks from time to
> time and it also crashed:
> 
> mplayer: pcm_bluetooth.c:386: a2dp_playback_hw_thread: Assertion `ret == 1' failed.

Could you be more precise ? Which kind of codec ? For how long did you 
play ?

> MPlayer interrupted by signal 6 in module: sleep_timer
> 
> However it is a huge improvement and a big step in the right direction.
> I know that Luiz was working on a similar patch. Try to get on #bluez on
> freenode.org so the future work can be coordinated.

Will try, however ML is easier for me as when i work on bluez stuff i 
usually only have an old RTC line :-(

> 
> Besides that, please fix the coding style. I am serious about that. I am
> picky about whitespace placements, but you already knew that:
> 
> Every "if" is following by a whitespace like "if (". No exceptions.

Hmmm... is this rule only for if's are is it valid for any kind of 
reserved key word ??

> 
> Also stuff like this is not acceptable:
> 
> pfd[0].fd = ((struct bluetooth_data *)io->private_data)->stream_fd;
> 
> If I have to twist my mind too much to understand why this code is
> correct it is too complex. Do something like this:
> 
> struct bluetooth_data *data = io->private_data;
> pfd[0].fd = data->stream_fd;

As you wish...

> 
> Don't invert the result of a function call:
> 
> ret = -pthread_create(&a2dp_data->hw_thread, 
> 
> However thought this is a good idea is wrong. This is big hole for
> overlooking sign errors. Don't do it. Do this instead:
> 
> return -ret

You're right that wasn't the smartest piece of code i wrote recently
  :-)
> 
> And while we are at it. Don't initiate variables with a value:
> 
> int ret = 0;
> 
> This is only needed in a few rare cases and will hide errors in the code
> flow that otherwise gcc might have caught.

Hmmm... I'm quite surprised : good programming practices tend to 
recommend to always initialize variables, unless this is obvious enough 
that the variable gets initialized right after declaration : could you 
bring more arguments under the hood ? I'm far from being conviced...

> 
> And instead of "ret" use "err" which I prefer.

Makes sense as it is an error code anyway...

> 
> The assert() statements are nice for development, but we can't have them
> in the final version. 

We have to prevent asserts to be executed on final version, that doesn't 
mean we have to remove them from the code however.
The correct way to use asserts is to have them enabled only when 
configuring using --enable-debug
If it's not currently the case on bluez-utils i suggest we patch its 
build process... and keep the asserts :-)


> We have to survive as much errors as possible and
> better play nothing than crash.

Agree.


> 
> The "for(;;) {" statement are better replaced with "while (1) {".
> 
> For the "volatile snd_pcm_sframes_t hw_ptr;". Do you think that is
> enough and we not better use a lock or a mutex?

That's enough for my case, where i have a single writer. Volatile is 
just to prevent the compiler to optimize away by keeping a variable in 
registers while it's value in main memory is changed by another thread.

Regards,

Fabien

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] pcm_bluetooth : fix a2dp streaming

[Luiz Augusto von Dentz]

Hi Fabian,

I got almost the same idea implemented in my git
(git://git.infradead.org/users/vudentz/bluez-utils.git). There is
alot of simplification you already did, but I consider the
bitrate as a2dp spec says. I also implement a circular
buffer for encoded sbc frames to be consumed by a
thread.

On 8/19/07, Fabien Chevalier <fabchevalier@free.fr> wrote:
> Johan & All,
>
> Following recent mailing list discussions, i decided to implement some
> of the ideas that have been suggested recently. :-)
>
> So i heavily modified the A2DP streaming part of the pcm plugin
>
> The user-visible features of this patch:
>    * much reduced latency (You should love this one Frederic)
>    * supports headsets that are unable to throttle the data flow and
>      that play too fast or drop packets or whatever (Marcel, could you
>      please confirm that fixes the issues you had with some headsets ?).
>
> This patch is fairly intrusive, and removes some code that i judjed
> useless now, like the bandwidth measurement stuff, and has greatly
> simplifyed transfer loop for avdtp (see comments in the code for the
> justifications)
>
> It brings in a thread that has for only reponsibility to increment the
> so called hardware pointer (which doesn't point to anything in our
> case), and notify the application trough writing in a pipe that some
> room is available (virtual room again), so that it could send more data.
>
> The attached file details the tests i've run to check i didn't break
> anything. All is conclusive except playing with XMMS that goes underrun
> for a reason i haven spotted yet. Apart from that aplay, mplayer,
> gstreamer are fine.
>
> I'm looking forward for any questions you have on the patch.
>
> Nest step would be to fix the SCO part of the plugin, but this one is
> looks harder as i think it will require tweaking the kernel SCO part.
>
> Regards,
>
> Fabien
>
> Bluetooth alsa a2dp non regression cookbook
> -------------------------------------------
>
> I) PCM test tool shipped with ALSA
>
> Setup:
> *Download the alsa-utils source for the binary that run on your machine
> *build the pcm executable in the test directory
>
> Run:
> ./pcm -c 2 -r 48000 -Dbluetooth -m write -b 10000
> ./pcm -c 2 -r 48000 -Dbluetooth -m write_and_poll -b 10000
> ./pcm -c 2 -r 48000 -Dbluetooth -m direct_write -b 10000
>
> Result:
> You should hear a clear sine in the headset.
> pcm process should not eat 100% cpu (check with top)
> Occasionnal sound drops are OK (due to quite short buffering of 10 ms)
>
> II) Play a file from XMMS
>
> III) Play file from totem
> Setup:
> * gconftool -t string -s /system/gstreamer/0.10/default/musicaudiosink "a=
lsasink device=3Dbluetooth"
>
> Run:
> Launch totem, then select a file for play
>
> III) PLay a file from mplayer
> mplayer -ao alsa:device=3Dbluetooth filetoplay.wav
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >>  http://get.splunk.com/
> _______________________________________________
> Bluez-devel mailing list
> Bluez-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/bluez-devel
>
>
>


-- =

Luiz Augusto von Dentz
Engenheiro de Computa=E7=E3o

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] pcm_bluetooth : fix a2dp streaming

[Fabien Chevalier]

Hi Luiz,

Please find a few remarks below:

> I got almost the same idea implemented in my git
> (git://git.infradead.org/users/vudentz/bluez-utils.git). 

Should make sense then :-)

There is
> alot of simplification you already did, but I consider the
> bitrate as a2dp spec says.

Oh... about this bitrate stuff i must say i'm quite confused : where did 
you find anything in the spec ? (I have A2DP 1.0 but didn't find 
anything :-( )

> I also implement a circular
> buffer for encoded sbc frames to be consumed by a
> thread.

That was another option, however it was not that clear what the benefits 
from it would be, so i took the easiest to implement alternative, and 
just sent data as they arrived ;-)

Cheers,

Fabien

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] pcm_bluetooth : fix a2dp streaming

[Luiz Augusto von Dentz]

Hi Fabien,

On 8/20/07, Fabien Chevalier <fabchevalier@free.fr> wrote:
> Hi Luiz,
>
> Please find a few remarks below:
>
> > I got almost the same idea implemented in my git
> > (git://git.infradead.org/users/vudentz/bluez-utils.git).
>
> Should make sense then :-)
>
> There is
> > alot of simplification you already did, but I consider the
> > bitrate as a2dp spec says.
>
> Oh... about this bitrate stuff i must say i'm quite confused : where did
> you find anything in the spec ? (I have A2DP 1.0 but didn't find
> anything :-( )

Spec 1.2 is already available, but even in it is difficult to find as it is
in Appendix part 12.9 page 66. Im not sure if the rate is mandatory
or not, but as we have some headsets surfer from speed problem
it might be a rate issue.

> > I also implement a circular
> > buffer for encoded sbc frames to be consumed by a
> > thread.
>
> That was another option, however it was not that clear what the benefits
> from it would be, so i took the easiest to implement alternative, and
> just sent data as they arrived ;-)

It seems it is not always the case we are ready to send right away,
this is what Im trying to figure out since the buffer sometimes got
full (encoding process is too fast) or even (weird) I got no frames
to sent on time thread (encoding too slow).

-- =

Luiz Augusto von Dentz
Engenheiro de Computa=E7=E3o

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] pcm_bluetooth : fix a2dp streaming

[Fabien Chevalier]

>> Oh... about this bitrate stuff i must say i'm quite confused : where did
>> you find anything in the spec ? (I have A2DP 1.0 but didn't find
>> anything :-( )
> 
> Spec 1.2 is already available, but even in it is difficult to find as it is
> in Appendix part 12.9 page 66. Im not sure if the rate is mandatory
> or not, but as we have some headsets surfer from speed problem
> it might be a rate issue.

Ok thanks, i'm gonna have a closer look to it then. :-)

> 
>>> I also implement a circular
>>> buffer for encoded sbc frames to be consumed by a
>>> thread.
>> That was another option, however it was not that clear what the benefits
>> from it would be, so i took the easiest to implement alternative, and
>> just sent data as they arrived ;-)
> 
> It seems it is not always the case we are ready to send right away,
> this is what Im trying to figure out since the buffer sometimes got
> full (encoding process is too fast) or even (weird) I got no frames
> to sent on time thread (encoding too slow).

I think these issues you have (too fast, too slow) is because the poll() 
support in your version of pcm_bluetooth is broken.
I suggest you have a look at the pulse and jack plugin (available in 
lastest alsa-plugins), and also to my patch to see how it fits in the 
picture.

A correct implementation of poll is the only way you will be able to 
wake up the application only when you are ready to accept more data.
This means you must never block in the  bluetooth_a2dp_write functions, 
either explicitely (through pthread_mutex_lock for instance :-) ) or 
implicitely (using a blocking send call on a socket for instance).
The blocking is done by libasound itself, by polling on the fd's you 
give to it.

If you have any questions i'm gonna try to be o IRC tomorrow.

Cheers,

Fabien

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] pcm_bluetooth : fix a2dp streaming

[Luiz Augusto von Dentz]

Hi again,

On 8/20/07, Fabien Chevalier <fabchevalier@free.fr> wrote:
> I think these issues you have (too fast, too slow) is because the poll()
> support in your version of pcm_bluetooth is broken.
> I suggest you have a look at the pulse and jack plugin (available in
> lastest alsa-plugins), and also to my patch to see how it fits in the
> picture.

Yep, it seems you are right on this. My solution even  consume more
cpu and solves nothing, too bad I couldn't figure out this before.

> A correct implementation of poll is the only way you will be able to
> wake up the application only when you are ready to accept more data.
> This means you must never block in the  bluetooth_a2dp_write functions,
> either explicitely (through pthread_mutex_lock for instance :-) ) or
> implicitely (using a blocking send call on a socket for instance).
> The blocking is done by libasound itself, by polling on the fd's you
> give to it.

Not sure how we could avoid blocking on transfer callback, that was
my idea but as I need to protect the buffer from being concurrent
accessed I end up with almost the same solution we already had.
Apart from that it seems you already solve the rate problem by properly
handle hw_ptr on a thread, adding the rate constrait spec mention
whould probably only add cpu consumption.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel