Skip to contentSkip to conversation

First try

Bluez-devel Frédéric DALLEAU 3 participants 6 messages May 30, 2006 – Jun 19, 2006 Bluetooth drivers

lore
Share & tools
lore
#0 lore raw

Hi brad,

I just tried to move global variables to pcm_a2dp struct... However, I seem to experience a loss in quality. I'm really unsure about it. I don't understand why as it is a straight forward fix. Could you please review it? It's based on the latest cvs.

I also tried a "make dist", but I got a btsco-0.2.tar.gz, which makes me say configure.in is not up to date !

Thank you,
Frédéric

Index: pcm_a2dp.c
===================================================================
RCS file: /cvsroot/bluetooth-alsa/btsco/alsa-plugins/pcm_a2dp.c,v retrieving revision 1.9
diff -r1.9 pcm_a2dp.c
52,54d51
< time_t timestamp=0;
< uint16_t seq_num=1;
< int frame_count=0;
56,66d52
< static char bufe[BUFS];
< int lenbufe=0;
<
< unsigned long nbytes=0;
< unsigned long total_time=0;
< struct timeval t0;
< struct itimerval itimer;
< struct sigaction sa;
< struct timeval tsend;
< int fpr=0;
<
563a550,563

Quoted 8 lines
   char bufe[BUFS];
  int lenbufe;//=0;
  unsigned long nbytes;//=0;
Quoted 6 lines

struct timeval tsend;

  time_t timestamp;//=0;
  uint16_t seq_num;//=1;
  int frame_count;//=0;

671,673c671,673
< if(lenbufe<codesize && lenbufe+datatoread<sizeof(bufe)){ // if not enough data in bufe to encode and there is space in bufe
< memcpy(bufe+lenbufe,buf,datatoread);// we read data to bufe
< lenbufe+=datatoread;
---

Quoted 3 lines
    	if(a2dp->lenbufe<codesize && a2dp->lenbufe+datatoread<sizeof(a2dp->bufe)){ // if not enough data in bufe to encode and there is space in bufe
		memcpy(a2dp->bufe+a2dp->lenbufe,buf,datatoread);// we read data to bufe
		a2dp->lenbufe+=datatoread;

677,682c677,682
< if(lenbufe>=codesize && a2dp->len + a2dp->sbc.len < 678){ // if enough data in bufe to encode and not enough frame to fill up mtu: encoding
< change_endian(bufe,codesize); // changing the endianness
< len = sbc_encode(&(a2dp->sbc), bufe, codesize); //encode
< memmove(bufe, bufe + len, lenbufe - len); //shift the bufe
< lenbufe-=len;
< nbytes+=len;
---

Quoted 6 lines
	if(a2dp->lenbufe>=codesize && a2dp->len + a2dp->sbc.len < 678){ // if enough data in bufe to encode and not enough frame to fill up mtu: encoding
		change_endian(a2dp->bufe,codesize); // changing the endianness
		len = sbc_encode(&(a2dp->sbc), a2dp->bufe, codesize); //encode
		memmove(a2dp->bufe, a2dp->bufe + len, a2dp->lenbufe - len); //shift the bufe                                 
		a2dp->lenbufe-=len;
		a2dp->nbytes+=len;

686c686
< frame_count++;
---

Quoted 1 line
		a2dp->frame_count++;

695c695
< dt.tv_usec=1000000*a2dp->sbc.subbands*a2dp->sbc.blocks*frame_count/io->rate; // time interval between transmitions
---

Quoted 1 line
		dt.tv_usec=1000000*a2dp->sbc.subbands*a2dp->sbc.blocks*a2dp->frame_count/io->rate; // time interval between transmitions

697c697
< if(time_to_wait(&tsend, &dt)==0){ // time to send data
---

Quoted 1 line
		if(time_to_wait(&a2dp->tsend, &dt)==0){ // time to send data

700c700
< payload_header.frame_count=frame_count;
---

Quoted 1 line
			payload_header.frame_count=a2dp->frame_count;

703,704c703,704
< packet_header.sequence_number = htons(seq_num);
< packet_header.timestamp = htonl(timestamp);
---

Quoted 2 lines
			packet_header.sequence_number = htons(a2dp->seq_num);
			packet_header.timestamp = htonl(a2dp->timestamp);

706c706
< timestamp += (a2dp->sbc.blocks + 1)*4 * (a2dp->sbc.subbands + 1)*4;
---

Quoted 1 line
			a2dp->timestamp += (a2dp->sbc.blocks + 1)*4 * (a2dp->sbc.subbands + 1)*4;

711c711
< frame_count=0;
---

Quoted 1 line
			a2dp->frame_count=0;

713c713
< seq_num++;
---

Quoted 1 line
			a2dp->seq_num++;

734d733
<
737a737,754

Quoted 10 lines
	struct timeval timeofday;
	gettimeofday(&timeofday, NULL);
	/*
	DBG("(%2d%3d): %d + %d * %d (=%d)",
	    (timeofday.tv_sec      )%100,  // s
	    (timeofday.tv_usec/1000)%1000, // ms
	    (int)areas->first, (int)areas->step, offset, (areas->first + areas->step * offset));
	/*
	DBG("(%2d%3d): %d * %d * %d * 2 (=%d) x %d",
Quoted 8 lines
	    (timeofday.tv_sec      )%100,  // s
	    (timeofday.tv_usec/1000)%1000, // ms
	    a2dp->sbc.subbands, // 8
	    a2dp->sbc.blocks, // 16
	    a2dp->sbc.channels, // 2
	    codesize, // 512
	    datatoread); // 512
	*/

739,741c756,758
< if(lenbufe<codesize){
< memcpy(bufe+lenbufe,buf,datatoread);
< lenbufe+=datatoread;
---

Quoted 3 lines
    	if(a2dp->lenbufe<codesize){
		memcpy(a2dp->bufe+a2dp->lenbufe,buf,datatoread);
		a2dp->lenbufe+=datatoread;

745,750c762,767
< if(lenbufe>=codesize){ //enough data to encode
< change_endian(bufe,codesize); // changing the endianness
< len = sbc_encode(&(a2dp->sbc), bufe, codesize); //encode
< memmove(bufe, bufe + len, lenbufe - len); //shift the bufe
< lenbufe-=len;
< nbytes+=len;
---

Quoted 6 lines
	if(a2dp->lenbufe>=codesize){ //enough data to encode
		change_endian(a2dp->bufe,codesize); // changing the endianness
		len = sbc_encode(&(a2dp->sbc), a2dp->bufe, codesize); //encode
		memmove(a2dp->bufe, a2dp->bufe + len, a2dp->lenbufe - len); //shift the bufe                                 
		a2dp->lenbufe-=len;
		a2dp->nbytes+=len;

756c773
< dt.tv_usec=1000000*a2dp->sbc.subbands*a2dp->sbc.blocks*frame_count/io->rate;
---

Quoted 1 line
			dt.tv_usec=1000000*a2dp->sbc.subbands*a2dp->sbc.blocks*a2dp->frame_count/io->rate;

759c776
< payload_header.frame_count=frame_count;
---

Quoted 1 line
			payload_header.frame_count=a2dp->frame_count;

762,763c779,780
< packet_header.sequence_number = htons(seq_num);
< packet_header.timestamp = htonl(timestamp);
---

Quoted 2 lines
			packet_header.sequence_number = htons(a2dp->seq_num);
			packet_header.timestamp = htonl(a2dp->timestamp);

765c782
< timestamp += (a2dp->sbc.blocks + 1)*4 * (a2dp->sbc.subbands + 1)*4;
---

Quoted 1 line
			a2dp->timestamp += (a2dp->sbc.blocks + 1)*4 * (a2dp->sbc.subbands + 1)*4;

768c785
< sleeptill(&tsend, &dt);
---

Quoted 1 line
			sleeptill(&a2dp->tsend, &dt);

774c791
< frame_count=0;
---

Quoted 1 line
			a2dp->frame_count=0;

776c793
< seq_num++;
---

Quoted 1 line
			a2dp->seq_num++;

778c795
< frame_count++;
---

Quoted 1 line
		a2dp->frame_count++;

974c991
< DBG("a2dp %p", a2dp);
---

Quoted 1 line
	DBG("TEST a2dp %p", a2dp);

1042a1060

Quoted 1 line
	a2dp->seq_num = 1;

_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

#1 lore raw

Fr=E9d=E9ric

Quoted 5 lines

I just tried to move global variables to pcm_a2dp struct... However, I seem to experience a loss in quality. I'm really unsure about it. I don't understand why as it is a straight forward fix. Could you please review it?
It's based on the latest cvs.

could you resend as a 'diff -u' output?

thanks for your work on this...

brad

_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

#2 lore raw

Hi brad,

Quoted 3 lines

could you resend as a 'diff -u' output?

this is the command line I use is that correct?

cvs diff -u pcm_a2dp.c > pcm_a2dp.c.diff-u
Now the output has +- instead of ><

Frederic

Index: alsa-plugins/pcm_a2dp.c
===================================================================
RCS file: /cvsroot/bluetooth-alsa/btsco/alsa-plugins/pcm_a2dp.c,v retrieving revision 1.9

pcm_a2dp.c +70 -52
diff -u -r1.9 pcm_a2dp.c
@@ -49,21 +49,7 @@
 #include "../a2dp.h"
 
 #define NONSPECAUDIO 1
-time_t timestamp=0;
-uint16_t seq_num=1;
-int frame_count=0;
 #define BUFS 1024
-static char bufe[BUFS];
-int lenbufe=0;
-
-unsigned long nbytes=0;
-unsigned long total_time=0;
-struct timeval t0;
-struct itimerval itimer;
-struct sigaction sa;
-struct timeval tsend;
-int fpr=0;
-
 
 #define min(X, Y)  ((X) < (Y) ? (X) : (Y))
 #define DBG(fmt, arg...)  printf("DEBUG: %s: " fmt "\n" , __FUNCTION__ , ## arg)
@@ -561,6 +547,20 @@
 	unsigned int len;
 	unsigned int frame_bytes;
 	int use_rfcomm;
+
+   char bufe[BUFS];
+  int lenbufe;//=0;
+
+  unsigned long nbytes;//=0;
+
+
+
+  struct timeval tsend;
+
+  time_t timestamp;//=0;
+  uint16_t seq_num;//=1;
+  int frame_count;//=0;
+
 } snd_pcm_a2dp_t;
 
 static void inline a2dp_get(snd_pcm_a2dp_t *a2dp)
@@ -668,22 +668,22 @@
 	codesize=a2dp->sbc.subbands*a2dp->sbc.blocks*a2dp->sbc.channels*2; // size of data encoded by sbc_encode in one call
 	datatoread=min(codesize,size*a2dp->frame_bytes); // amount of data to read
 	buf = (char *) areas->addr + (areas->first + areas->step * offset) / 8;
-    	if(lenbufe<codesize && lenbufe+datatoread<sizeof(bufe)){ // if not enough data in bufe to encode and there is space in bufe
-		memcpy(bufe+lenbufe,buf,datatoread);// we read data to bufe
-		lenbufe+=datatoread;
+    	if(a2dp->lenbufe<codesize && a2dp->lenbufe+datatoread<sizeof(a2dp->bufe)){ // if not enough data in bufe to encode and there is space in bufe
+		memcpy(a2dp->bufe+a2dp->lenbufe,buf,datatoread);// we read data to bufe
+		a2dp->lenbufe+=datatoread;
 	}
 	else{datatoread=0;}//nothing has been read
 
-	if(lenbufe>=codesize && a2dp->len + a2dp->sbc.len < 678){ // if enough data in bufe to encode and not enough frame to fill up mtu: encoding
-		change_endian(bufe,codesize); // changing the endianness
-		len = sbc_encode(&(a2dp->sbc), bufe, codesize); //encode
-		memmove(bufe, bufe + len, lenbufe - len); //shift the bufe                                 
-		lenbufe-=len;
-		nbytes+=len;
+	if(a2dp->lenbufe>=codesize && a2dp->len + a2dp->sbc.len < 678){ // if enough data in bufe to encode and not enough frame to fill up mtu: encoding
+		change_endian(a2dp->bufe,codesize); // changing the endianness
+		len = sbc_encode(&(a2dp->sbc), a2dp->bufe, codesize); //encode
+		memmove(a2dp->bufe, a2dp->bufe + len, a2dp->lenbufe - len); //shift the bufe                                 
+		a2dp->lenbufe-=len;
+		a2dp->nbytes+=len;
 		sleeptime += a2dp->sbc.duration;
 		if (len <= 0)
 			return len;
-		frame_count++;
+		a2dp->frame_count++;
 		memcpy(a2dp->buf + a2dp->len, a2dp->sbc.data, a2dp->sbc.len); // copy encoded frames into a2dp->buf
 		a2dp->len+=a2dp->sbc.len;
 		if (a2dp->state == BT_CONNECTED)
@@ -692,25 +692,25 @@
 	}		
 
 	if(a2dp->len + a2dp->sbc.len > 678){ // if packet is formed
-		dt.tv_usec=1000000*a2dp->sbc.subbands*a2dp->sbc.blocks*frame_count/io->rate; // time interval between transmitions
+		dt.tv_usec=1000000*a2dp->sbc.subbands*a2dp->sbc.blocks*a2dp->frame_count/io->rate; // time interval between transmitions
 		dt.tv_sec=0;
-		if(time_to_wait(&tsend, &dt)==0){ // time to send data
+		if(time_to_wait(&a2dp->tsend, &dt)==0){ // time to send data
 			memset(&payload_header, 0, sizeof(payload_header)); // fill up the headers
 			memset(&packet_header, 0, sizeof(packet_header)); //---
-			payload_header.frame_count=frame_count;
+			payload_header.frame_count=a2dp->frame_count;
 			packet_header.v = 2;
 			packet_header.pt = 1;
-			packet_header.sequence_number = htons(seq_num);
-			packet_header.timestamp = htonl(timestamp);
+			packet_header.sequence_number = htons(a2dp->seq_num);
+			packet_header.timestamp = htonl(a2dp->timestamp);
 			packet_header.ssrc = htonl(1);
-			timestamp += (a2dp->sbc.blocks + 1)*4 * (a2dp->sbc.subbands + 1)*4;
+			a2dp->timestamp += (a2dp->sbc.blocks + 1)*4 * (a2dp->sbc.subbands + 1)*4;
 			memcpy(a2dp->buf, &packet_header, sizeof(packet_header)); //copy the headers to buf
 			memcpy(a2dp->buf + sizeof(packet_header), &payload_header, sizeof(payload_header));//---
 			write(a2dp->sk,a2dp->buf,a2dp->len); // sending the packet
 			a2dp->len = sizeof(packet_header)+sizeof(payload_header); //inital position in buf, just after headers
-			frame_count=0;
+			a2dp->frame_count=0;
 			sleeptime=0;
-			seq_num++;
+			a2dp->seq_num++;
 		}else{usleep(1);}
 	}
 	return datatoread / a2dp->frame_bytes;
@@ -731,51 +731,68 @@
 	int written;
 	
 	struct timeval dt;
-	
 
 	codesize=a2dp->sbc.subbands*a2dp->sbc.blocks*a2dp->sbc.channels*2;
 	datatoread=min(codesize,size*a2dp->frame_bytes);
+
+	struct timeval timeofday;
+	gettimeofday(&timeofday, NULL);
+	/*
+	DBG("(%2d%3d): %d + %d * %d (=%d)",
+	    (timeofday.tv_sec      )%100,  // s
+	    (timeofday.tv_usec/1000)%1000, // ms
+	    (int)areas->first, (int)areas->step, offset, (areas->first + areas->step * offset));
+	/*
+	DBG("(%2d%3d): %d * %d * %d * 2 (=%d) x %d",
+	    (timeofday.tv_sec      )%100,  // s
+	    (timeofday.tv_usec/1000)%1000, // ms
+	    a2dp->sbc.subbands, // 8
+	    a2dp->sbc.blocks, // 16
+	    a2dp->sbc.channels, // 2
+	    codesize, // 512
+	    datatoread); // 512
+	*/
 	buf = (char *) areas->addr + (areas->first + areas->step * offset) / 8;
-    	if(lenbufe<codesize){
-		memcpy(bufe+lenbufe,buf,datatoread);
-		lenbufe+=datatoread;
+    	if(a2dp->lenbufe<codesize){
+		memcpy(a2dp->bufe+a2dp->lenbufe,buf,datatoread);
+		a2dp->lenbufe+=datatoread;
 	}  
 	else{datatoread=0;}
 
-	if(lenbufe>=codesize){ //enough data to encode
-		change_endian(bufe,codesize); // changing the endianness
-		len = sbc_encode(&(a2dp->sbc), bufe, codesize); //encode
-		memmove(bufe, bufe + len, lenbufe - len); //shift the bufe                                 
-		lenbufe-=len;
-		nbytes+=len;
+	if(a2dp->lenbufe>=codesize){ //enough data to encode
+		change_endian(a2dp->bufe,codesize); // changing the endianness
+		len = sbc_encode(&(a2dp->sbc), a2dp->bufe, codesize); //encode
+		memmove(a2dp->bufe, a2dp->bufe + len, a2dp->lenbufe - len); //shift the bufe                                 
+		a2dp->lenbufe-=len;
+		a2dp->nbytes+=len;
 		sleeptime += a2dp->sbc.duration;
 		if (len <= 0)
 			return len;
 		if(a2dp->len + a2dp->sbc.len > 678)	{ // time to prepare and send the packet
 			dt.tv_sec=0;
-			dt.tv_usec=1000000*a2dp->sbc.subbands*a2dp->sbc.blocks*frame_count/io->rate;
+			dt.tv_usec=1000000*a2dp->sbc.subbands*a2dp->sbc.blocks*a2dp->frame_count/io->rate;
 			memset(&payload_header, 0, sizeof(payload_header));
 			memset(&packet_header, 0, sizeof(packet_header));
-			payload_header.frame_count=frame_count;
+			payload_header.frame_count=a2dp->frame_count;
 			packet_header.v = 2;
 			packet_header.pt = 1;
-			packet_header.sequence_number = htons(seq_num);
-			packet_header.timestamp = htonl(timestamp);
+			packet_header.sequence_number = htons(a2dp->seq_num);
+			packet_header.timestamp = htonl(a2dp->timestamp);
 			packet_header.ssrc = htonl(1);
-			timestamp += (a2dp->sbc.blocks + 1)*4 * (a2dp->sbc.subbands + 1)*4;
+			a2dp->timestamp += (a2dp->sbc.blocks + 1)*4 * (a2dp->sbc.subbands + 1)*4;
 			memcpy(a2dp->buf, &packet_header, sizeof(packet_header));
 			memcpy(a2dp->buf + sizeof(packet_header), &payload_header, sizeof(payload_header));
-			sleeptill(&tsend, &dt);
+			sleeptill(&a2dp->tsend, &dt);
 			if((written = write(a2dp->sk,a2dp->buf,a2dp->len)) != a2dp->len) {
 				DBG("Wrote %d not %d bytes; errno %s(%d)", written, a2dp->len,
 					strerror(errno), errno);
 			}
 			a2dp->len = sizeof(packet_header)+sizeof(payload_header);
-			frame_count=0;
+			a2dp->frame_count=0;
 			sleeptime=0;
-			seq_num++;
+			a2dp->seq_num++;
 		}
-		frame_count++;
+		a2dp->frame_count++;
 		memcpy(a2dp->buf + a2dp->len, a2dp->sbc.data, a2dp->sbc.len);
 		a2dp->len+=a2dp->sbc.len;
 		if (a2dp->state == BT_CONNECTED)
@@ -971,7 +988,7 @@
 	unsigned int format[2], channel[2], rate[2];
 	int err;
 
-	DBG("a2dp %p", a2dp);
+	DBG("TEST a2dp %p", a2dp);
 
 	err = snd_pcm_ioplug_set_param_list(io, SND_PCM_IOPLUG_HW_ACCESS, 2, access_list);
 	if (err < 0)
@@ -1040,6 +1057,7 @@
 	memset(a2dp, 0, sizeof(*a2dp));
 
 	a2dp->refcnt = 1;
+	a2dp->seq_num = 1;
 
 	a2dp->state = BT_OPEN;
 

_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

maintainer
#3 lore raw

Hi Frederic,

Quoted 5 lines
Quoted 1 line

could you resend as a 'diff -u' output?

this is the command line I use is that correct?

cvs diff -u pcm_a2dp.c > pcm_a2dp.c.diff-u

you should have a .cvsrc file in $HOME that looks like this:

cvs -z3
diff -u
update -dP
checkout -P

Quoted 1 line

Now the output has +- instead of ><

It is called unified diff and it is the preferred format around any open source project. I don't know of anybody who ever has reviewed a context diff.

Regards

Marcel

_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

#4 lore raw
In reply to Frédéric DALLEAU

Fr=E9d=E9ric

I had a look at your a2dp patches to move globals into the per-connection struct. It all looks like a clear improvement. I didn't have any problems with changes in quality after the change.

Would you like to check things out of cvs and verify it works for you?

thanks
Brad

_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

#5 lore raw
Quoted 9 lines

I had a look at your a2dp patches to move globals into the per-connection struct. It all looks like a clear improvement. I didn't have any problems with changes in quality after the change.

Would you like to check things out of cvs and verify it works for you?
=

Sorry for the long silence, I'm working on something else. I'm pleased =

you integrated my previous patch, it works well.

I tried two headset at a time. It does'nt crash, but I have bad sound. I =

believe this is due to bandwith limit. My bluetooth dongle is USB so =

there is enough bandwith. And bluetooth is 700 kb/s so I thought it =

should be enough to pass sbc data. I believed sbc was 128 kb/s. Maybe it =

is per channel... Can you confirm?

Thank you!
Fr=E9d=E9ric.

_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel