All of lore.kernel.org
 help / color / mirror / Atom feed
* [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints
@ 2007-10-12 12:25 Kai Vehmanen
  2007-10-14  3:36 ` Luiz Augusto von Dentz
  2007-10-14 11:19 ` Fabien Chevalier
  0 siblings, 2 replies; 10+ messages in thread
From: Kai Vehmanen @ 2007-10-12 12:25 UTC (permalink / raw)
  To: bluez-devel

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

Hello,

I've been having some problems in using the Bluez ALSA/A2DP plugin
with applications that use the ALSA API in event driven fashion (so
'write samples at last possible moment' instead of 'write samples when 
there is free room in the buffer'). One test application I've used is
Pulseaudio. 

Here're couple of patches (against CVS HEAD) that solve the 
problems for me (for instance pulseaudio starts working with
the plugin, less CPU usage with other apps). If you think these
changes are ok, please include them to the upstream codebase.

This first modifies the buffering constraints as exposed by
the A2DP ALSA plugin:
  - increased max overall buffer size to 16384 bytes (~93msecs
    of CD quality audio, so a reasonable max size)
  - allow applications to specify buffer size
  - allow applications to use bigger periods (less wakeup
    load -> the previous limit of 128 is heavy for apps)

I'll submit the other patch as a separate mail, so it's easier
to discuss about the patch contents separately.

This is my first patch to bluez-utils, so please be kind. :)

Br,
-- 
first.surname@nokia.com (Kai Vehmanen)

[-- Attachment #2: patch-kv-bluez-utils-fix-a2dp-buffer-constraints-20071012.txt --]
[-- Type: text/plain, Size: 2252 bytes --]

diff -ruN bluez-utils.orig/audio/pcm_bluetooth.c bluez-utils/audio/pcm_bluetooth.c
--- bluez-utils.orig/audio/pcm_bluetooth.c	2007-10-12 12:57:28.000000000 +0300
+++ bluez-utils/audio/pcm_bluetooth.c	2007-10-12 15:04:49.000000000 +0300
@@ -45,7 +45,8 @@
 
 #define MIN_PERIOD_TIME 1
 
-#define BUFFER_SIZE 2048
+#define MIN_BUFFER_SIZE 256    /* minimum size of buffer */
+#define MAX_BUFFER_SIZE 16384  /* allocated RAM for buffer */
 
 #ifdef ENABLE_DEBUG
 #define DBG(fmt, arg...)  printf("DEBUG: %s: " fmt "\n" , __FUNCTION__ , ## arg)
@@ -123,7 +124,7 @@
 	sbc_t sbc;			/* Codec data */
 	int codesize;			/* SBC codesize */
 	int samples;			/* Number of encoded samples */
-	uint8_t buffer[BUFFER_SIZE];	/* Codec transfer buffer */
+	uint8_t buffer[MAX_BUFFER_SIZE];/* Codec transfer buffer */
 	int count;			/* Codec transfer buffer counter */
 
 	int nsamples;			/* Cumulative number of codec samples */
@@ -137,7 +138,7 @@
 	struct ipc_data_cfg cfg;	/* Bluetooth device config */
 	struct pollfd stream;		/* Audio stream filedescriptor */
 	struct pollfd server;		/* Audio daemon filedescriptor */
-	uint8_t buffer[BUFFER_SIZE];	/* Encoded transfer buffer */
+	uint8_t buffer[MAX_BUFFER_SIZE];/* Encoded transfer buffer */
 	int count;			/* Transfer buffer counter */
 	struct bluetooth_a2dp a2dp;	/* A2DP data */
 
@@ -932,14 +933,24 @@
 	if (err < 0)
 		return err;
 
-	/* supported block size */
+	/* supported block sizes:
+	 *   - lower limit is A2DP codec size
+	 *   - total buffer size is the upper limit (with two periods) */
 	err = snd_pcm_ioplug_set_param_minmax(io, SND_PCM_IOPLUG_HW_PERIOD_BYTES,
-						a2dp->codesize, a2dp->codesize);
+					      a2dp->codesize, MAX_BUFFER_SIZE / 2);
+	if (err < 0)
+		return err;
+
+	/* supported buffer sizes */
+	err = snd_pcm_ioplug_set_param_minmax(io, SND_PCM_IOPLUG_HW_BUFFER_BYTES,
+					      MIN_BUFFER_SIZE, MAX_BUFFER_SIZE);
 	if (err < 0)
 		return err;
 
+	/* supported period count: 
+	 *   - derived from max buffer size and minimum period size */
 	err = snd_pcm_ioplug_set_param_minmax(io, SND_PCM_IOPLUG_HW_PERIODS,
-									2, 50);
+					      2, MAX_BUFFER_SIZE / a2dp->codesize);
 	if (err < 0)
 		return err;
 

[-- Attachment #3: Type: text/plain, Size: 314 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 #4: Type: text/plain, Size: 164 bytes --]

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints
  2007-10-12 12:25 [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints Kai Vehmanen
@ 2007-10-14  3:36 ` Luiz Augusto von Dentz
  2007-10-14 11:19 ` Fabien Chevalier
  1 sibling, 0 replies; 10+ messages in thread
From: Luiz Augusto von Dentz @ 2007-10-14  3:36 UTC (permalink / raw)
  To: BlueZ development

Hi Kai,

On 10/12/07, Kai Vehmanen <kai.vehmanen@nokia.com> wrote:
> This first modifies the buffering constraints as exposed by
> the A2DP ALSA plugin:
>   - increased max overall buffer size to 16384 bytes (~93msecs
>     of CD quality audio, so a reasonable max size)
>   - allow applications to specify buffer size
>   - allow applications to use bigger periods (less wakeup
>     load -> the previous limit of 128 is heavy for apps)

All seems to be good improvements, perhaps brad can comment
about it because he was working with pulse before.

-- =

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints
  2007-10-12 12:25 [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints Kai Vehmanen
  2007-10-14  3:36 ` Luiz Augusto von Dentz
@ 2007-10-14 11:19 ` Fabien Chevalier
  2007-10-15 10:54   ` [Bluez-devel] PATCH1/2: bluez-utils -fix-a2dp-buffer-constraints Kai Vehmanen
  1 sibling, 1 reply; 10+ messages in thread
From: Fabien Chevalier @ 2007-10-14 11:19 UTC (permalink / raw)
  To: BlueZ development

Hello Kai,

Please find a few comments below

> Hello,
> 
> I've been having some problems in using the Bluez ALSA/A2DP plugin
> with applications that use the ALSA API in event driven fashion (so
> 'write samples at last possible moment' instead of 'write samples when 
> there is free room in the buffer'). One test application I've used is
> Pulseaudio. 
> 
> Here're couple of patches (against CVS HEAD) that solve the 
> problems for me (for instance pulseaudio starts working with
> the plugin, less CPU usage with other apps).

Could you provide some details on the problems you experienced, as well 
the hardware platform you experienced them on ?

> diff -ruN bluez-utils.orig/audio/pcm_bluetooth.c bluez-utils/audio/pcm_bluetooth.c
> --- bluez-utils.orig/audio/pcm_bluetooth.c	2007-10-12 12:57:28.000000000 +0300
> +++ bluez-utils/audio/pcm_bluetooth.c	2007-10-12 15:04:49.000000000 +0300
> @@ -45,7 +45,8 @@
>  
>  #define MIN_PERIOD_TIME 1
>  
> -#define BUFFER_SIZE 2048
> +#define MIN_BUFFER_SIZE 256    /* minimum size of buffer */
> +#define MAX_BUFFER_SIZE 16384  /* allocated RAM for buffer */
>  
>  #ifdef ENABLE_DEBUG
>  #define DBG(fmt, arg...)  printf("DEBUG: %s: " fmt "\n" , __FUNCTION__ , ## arg)
> @@ -123,7 +124,7 @@
>  	sbc_t sbc;			/* Codec data */
>  	int codesize;			/* SBC codesize */
>  	int samples;			/* Number of encoded samples */
> -	uint8_t buffer[BUFFER_SIZE];	/* Codec transfer buffer */
> +	uint8_t buffer[MAX_BUFFER_SIZE];/* Codec transfer buffer */
>  	int count;			/* Codec transfer buffer counter */
>  
>  	int nsamples;			/* Cumulative number of codec samples */
> @@ -137,7 +138,7 @@
>  	struct ipc_data_cfg cfg;	/* Bluetooth device config */
>  	struct pollfd stream;		/* Audio stream filedescriptor */
>  	struct pollfd server;		/* Audio daemon filedescriptor */
> -	uint8_t buffer[BUFFER_SIZE];	/* Encoded transfer buffer */
> +	uint8_t buffer[MAX_BUFFER_SIZE];/* Encoded transfer buffer */
>  	int count;			/* Transfer buffer counter */
>  	struct bluetooth_a2dp a2dp;	/* A2DP data */
>  
> @@ -932,14 +933,24 @@
>  	if (err < 0)
>  		return err;
>  
> -	/* supported block size */
> +	/* supported block sizes:
> +	 *   - lower limit is A2DP codec size
> +	 *   - total buffer size is the upper limit (with two periods) */
>  	err = snd_pcm_ioplug_set_param_minmax(io, SND_PCM_IOPLUG_HW_PERIOD_BYTES,
> -						a2dp->codesize, a2dp->codesize);
> +					      a2dp->codesize, MAX_BUFFER_SIZE / 2);
> +	if (err < 0)
> +		return err;
> +
> +	/* supported buffer sizes */
> +	err = snd_pcm_ioplug_set_param_minmax(io, SND_PCM_IOPLUG_HW_BUFFER_BYTES,
> +					      MIN_BUFFER_SIZE, MAX_BUFFER_SIZE);
>  	if (err < 0)
>  		return err;

This part of the code is useless, you don't need to set 
SND_PCM_IOPLUG_HW_BUFFER_BYTES if you set SND_PCM_IOPLUG_HW_PERIODS, you 
just have to choose one of the two alternatives :-)

>  
> +	/* supported period count: 
> +	 *   - derived from max buffer size and minimum period size */
>  	err = snd_pcm_ioplug_set_param_minmax(io, SND_PCM_IOPLUG_HW_PERIODS,
> -									2, 50);
> +					      2, MAX_BUFFER_SIZE / a2dp->codesize);
>  	if (err < 0)
>  		return err;
>  
> 

Apart from that i'd say this is a foot step in the right direction :-)

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Bluez-devel] PATCH1/2: bluez-utils -fix-a2dp-buffer-constraints
  2007-10-14 11:19 ` Fabien Chevalier
@ 2007-10-15 10:54   ` Kai Vehmanen
  2007-10-17 10:23     ` Fabien Chevalier
  0 siblings, 1 reply; 10+ messages in thread
From: Kai Vehmanen @ 2007-10-15 10:54 UTC (permalink / raw)
  To: 'BlueZ development'

Hi,

On 14 Oct 2007,  Fabien Chevalier wrote:
>Could you provide some details on the problems you 
>experienced, as well the hardware platform you experienced them on ?

I've been testing with a low spec GNU/Debian desktop, and the problem has 
been too high CPU load leading to a continuous stream of XRUNs in 
the pcm_bluetooth.c code (i.e. no audio output at all). With the patch, 
on the same exact system, playback is flawless. And the problem only
occurs with apps that always poll/select before writing to the device (i.e.
not aplay). Relaxing the buffering constraints allows apps to feed audio 
to A2DP with less context switches and less system calls (bigger buffer
fragments at a time basicly).

>> -	/* supported block size */
>> +	/* supported block sizes:
>> +	 *   - lower limit is A2DP codec size
>> +	 *   - total buffer size is the upper limit (with two periods) */
>>  	err = snd_pcm_ioplug_set_param_minmax(io,
SND_PCM_IOPLUG_HW_PERIOD_BYTES,
[...]
>> +	/* supported buffer sizes */
>> +	err = snd_pcm_ioplug_set_param_minmax(io,
SND_PCM_IOPLUG_HW_BUFFER_BYTES,
>> +					      MIN_BUFFER_SIZE,
MAX_BUFFER_SIZE);
>>  	if (err < 0)
>>  		return err;
>
>
>This part of the code is useless, you don't need to set 
>SND_PCM_IOPLUG_HW_BUFFER_BYTES if you set 
>SND_PCM_IOPLUG_HW_PERIODS, you just have to choose one of the 
>two alternatives :-)

But if we'd drop setting mimax for BUFFER_BYTES, the application could 
request for 64 periods of size 'MAX_BUFFER_SIZE / 2' -> 524288 bytes.
Of course we could fix this by limiting the max period size, but then 
the application couldn't use a buffering setup of two max-sized periods
(good for robust playback). If we'd limit the max count of periods, 
application couldn't choose a buffering setup of lots of small 
buffer fragments (good for robust recording with minimal latency).

So defining a minmax range for all three params gives more
flexibility without requiring any added logic to the plugin
code.

-- 
first.surname@nokia.com (Kai Vehmanen)


-------------------------------------------------------------------------
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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Bluez-devel] PATCH1/2: bluez-utils -fix-a2dp-buffer-constraints
  2007-10-15 10:54   ` [Bluez-devel] PATCH1/2: bluez-utils -fix-a2dp-buffer-constraints Kai Vehmanen
@ 2007-10-17 10:23     ` Fabien Chevalier
  2007-10-17 14:36       ` Kai.Vehmanen
  0 siblings, 1 reply; 10+ messages in thread
From: Fabien Chevalier @ 2007-10-17 10:23 UTC (permalink / raw)
  To: BlueZ development

Hello Kai,

> I've been testing with a low spec GNU/Debian desktop, and the problem has 
> been too high CPU load leading to a continuous stream of XRUNs in 
> the pcm_bluetooth.c code (i.e. no audio output at all). With the patch, 
> on the same exact system, playback is flawless. And the problem only
> occurs with apps that always poll/select before writing to the device (i.e.
> not aplay). Relaxing the buffering constraints allows apps to feed audio 
> to A2DP with less context switches and less system calls (bigger buffer
> fragments at a time basicly).

I'm quite interested in optimizations for low end ARM boxes :-)
Could you provide CPU details and the player you used to performed your 
testing ? I'd like to be able to reproduce those on my Neo1973 box :-)

> 
>>> -	/* supported block size */
>>> +	/* supported block sizes:
>>> +	 *   - lower limit is A2DP codec size
>>> +	 *   - total buffer size is the upper limit (with two periods) */
>>>  	err = snd_pcm_ioplug_set_param_minmax(io,
> SND_PCM_IOPLUG_HW_PERIOD_BYTES,
> [...]
>>> +	/* supported buffer sizes */
>>> +	err = snd_pcm_ioplug_set_param_minmax(io,
> SND_PCM_IOPLUG_HW_BUFFER_BYTES,
>>> +					      MIN_BUFFER_SIZE,
> MAX_BUFFER_SIZE);
>>>  	if (err < 0)
>>>  		return err;
>>
>> This part of the code is useless, you don't need to set 
>> SND_PCM_IOPLUG_HW_BUFFER_BYTES if you set 
>> SND_PCM_IOPLUG_HW_PERIODS, you just have to choose one of the 
>> two alternatives :-)
> 
> But if we'd drop setting mimax for BUFFER_BYTES, the application could 
> request for 64 periods of size 'MAX_BUFFER_SIZE / 2' -> 524288 bytes.
> Of course we could fix this by limiting the max period size, but then 
> the application couldn't use a buffering setup of two max-sized periods
> (good for robust playback). If we'd limit the max count of periods, 
> application couldn't choose a buffering setup of lots of small 
> buffer fragments (good for robust recording with minimal latency).
> 
> So defining a minmax range for all three params gives more
> flexibility without requiring any added logic to the plugin
> code.

That's definately a good answer :-).
Maybe we should drop SND_PCM_IOPLUG_HW_PERIODS and keep 
SND_PCM_IOPLUG_HW_BUFFER_BYTES only then ?

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Bluez-devel] PATCH1/2: bluez-utils -fix-a2dp-buffer-constraints
  2007-10-17 10:23     ` Fabien Chevalier
@ 2007-10-17 14:36       ` Kai.Vehmanen
  0 siblings, 0 replies; 10+ messages in thread
From: Kai.Vehmanen @ 2007-10-17 14:36 UTC (permalink / raw)
  To: bluez-devel

Hi Fabien,

On 17 Oct 2007,  Fabien Chevalier wrote:
>I'm quite interested in optimizations for low end ARM boxes 
>:-) Could you provide CPU details and the player you used to 
>performed your testing ? I'd like to be able to reproduce 
>those on my Neo1973 box :-)

:) I'm using an x86 system myself (2.66GHz P4). Just try running 
pulseaudio plus for instance "aplay -D pulse foo.wav" (or paplay) and 
you should see similar results. With the above system and  
unpatched 3.19, I get 2x number of gettimeofday()s, plus audio play 
out is as described in my earlier mails (choppy, so not really working),

compared to running the same test with patched 3.19. And this with 
default settings (i.e. not optimizing for a larger period size which 
would further minimize the system call load).

Anyways, this is definitely not specific to any hardware architecture 
nor CPU processing power. You can simply calculate the number of
unnecessary 
system calls and context switches. Whether those are enough to degrade 
audio quality is another question, but nevertheless you are wasting CPU 
cycles, using more electricity, accelerating global warming, etc, etc :)


>Maybe we should drop SND_PCM_IOPLUG_HW_PERIODS and keep 
>SND_PCM_IOPLUG_HW_BUFFER_BYTES only then ?

Hmm, that should be ok. I guess alsa-lib won't allow applications
to use only one period anyways, so not specifying that in
pcm_bluetooth.c 
should be ok.

-- 
first.surname@nokia.com (Kai Vehmanen)

-------------------------------------------------------------------------
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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints
  2007-10-18 10:08 ` Fabien Chevalier
@ 2007-10-18 17:33   ` Jim Carter
  0 siblings, 0 replies; 10+ messages in thread
From: Jim Carter @ 2007-10-18 17:33 UTC (permalink / raw)
  To: BlueZ development

On Thu, 18 Oct 2007, Fabien Chevalier wrote:
> > On 12 Oct 2007, Kai Vehmanen wrote: 

> > Current code seems to gather samples-to-be-sent in a buffer,
> > and when a threshold (a2dp->codesize or cfg.pkt_len) is reached,
> > a packet is then sent right away. So in other words, application
> > cannot really prefill more data than one packet worth, and most of 
> > the allocated buffer space gets never used.
> 
> Well, application can prefill more data, the data gets send immediately 
> to the headset, and buffering will happen at headset side. The issue is 
> if the buffer size as seen by the application is bigger than the real 
> headset buffer size, and the application waits for the buffer to be 
> almost empty before to refill it, then audio cuts might happen. :-(

I remember a month or two back, someone said that the A2DP profile was weak 
on flow control, requiring the source (ALSA) to send data at an even rate.  
If the headset's buffer is full can it cork the stream?  Or will the 
additional data just overwrite data already at the headset?

With bluez-utils-3.19 (3.20 is latest) I'm having consistently good results 
sending to the Bluetooth ALSA device, with neither silent gaps nor lost 
chunks.  But I'm seeing corruption when going through PulseAudio, which is 
attributed to an unknown formatting or parameter issue and is being dealt 
with on their mailing list.

James F. Carter          Voice 310 825 2897    FAX 310 206 6673
UCLA-Mathnet;  6115 MSA; 405 Hilgard Ave.; Los Angeles, CA, USA  90095-1555
Email: jimc@math.ucla.edu    http://www.math.ucla.edu/~jimc (q.v. for PGP key)

-------------------------------------------------------------------------
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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints
  2007-10-17 16:31 [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints Kai.Vehmanen
  2007-10-18  8:42 ` Johan Hedberg
@ 2007-10-18 10:08 ` Fabien Chevalier
  2007-10-18 17:33   ` Jim Carter
  1 sibling, 1 reply; 10+ messages in thread
From: Fabien Chevalier @ 2007-10-18 10:08 UTC (permalink / raw)
  To: BlueZ development

Hello Kai,

Please find below some comments.

> On 12 Oct 2007, Kai Vehmanen wrote: 
>> Here're couple of patches (against CVS HEAD) that solve the 
>> problems for me (for instance pulseaudio starts working with 
>> the plugin, less CPU usage with other apps). If you think 
> [...]
>> This first modifies the buffering constraints as exposed by 
>> the A2DP ALSA plugin:
>>  - increased max overall buffer size to 16384 bytes (~93msecs
>>    of CD quality audio, so a reasonable max size)
> 
> hmm, looking at this more deeply, I realized this doesn't quite 
> work. I originally thought there is a real ringbuffer (imitating 
> the usual audio hardware ringbuffer) between the worker
> thread and the application, but this appears not to be the
> case. 

No this is not the case. :-)
I implemented it that way so that not to add another buffering layer.
As per ALSA semantic, the buffer is found in the remote device, but we 
have no way of knowing how big it is. :-(

Current code seems to gather samples-to-be-sent in a buffer,
> and when a threshold (a2dp->codesize or cfg.pkt_len) is reached,
> a packet is then sent right away. So in other words, application
> cannot really prefill more data than one packet worth, and most of 
> the allocated buffer space gets never used.

Well, application can prefill more data, the data gets send immediately 
to the headset, and buffering will happen at headset side. The issue is 
if the buffer size as seen by the application is bigger than the real 
headset buffer size, and the application waits for the buffer to be 
almost empty before to refill it, then audio cuts might happen. :-(

> 
> So I guess this specific patch should be dropped (it might fix
> some issues, but at least the assumptions I used to pick the 
> values are wrong ;)). 

Even if your assumptions were wrong, this part of the code is still very 
much work in progress, and didn't receive any tuning at all.
So Johan tested your patch, didn't noticed any regression, so we decided 
to apply it anyway. :-)


The other patch should still be valid.
> 
> What I intended with the patch was that you could...:
>   - prefill N*a2dp_codesize (less than buffersize) worth of 
>     PCM frames
>   - trigger playback 
>       -> oldest queued a2dp_codesize worth of samples is encoded 
>         and sent
>   - application is woken up every M*a2dp_codesize
>       - in case application is occasionally late, the worker
>         thread would still be encoding and sending packets
>         at a steady rate using the buffered samples
> 	- with M>1, application can reduce the amount of wakeups
>         needed, but will increase the playout latency (but
>         note: the HW thread still needs to wake up for every 
>         a2dp_codesize)
>   - an XRUN occurs only if application is stalled for 
>     duration of N*a2dp_codesize
>         

That is basically another alternative implementation. For now i'm still 
not convinced this is the right way to go ;-). This will work but add 
another layer of buffering thus a much increased latency :-(


> But yeah, that would seem to be a much bigger task to implement

Yes indeed :-)
  (and
> something you may have already discussed earlier).

Well, yes and no. a2dpd implementation was written the way you though we 
were working. Audio service ALSA plugin is written another way, that is 
known to work. However we didn't tune the implementation to improve its
behaviours regarding the number of application wakeups it generates, nor 
the buffer sizes or period sizes :-(
I think you were the first one to run those kind of tests, that's why 
i'm still interested in you giving us the details of your tests setup :-)

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints
  2007-10-17 16:31 [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints Kai.Vehmanen
@ 2007-10-18  8:42 ` Johan Hedberg
  2007-10-18 10:08 ` Fabien Chevalier
  1 sibling, 0 replies; 10+ messages in thread
From: Johan Hedberg @ 2007-10-18  8:42 UTC (permalink / raw)
  To: BlueZ development

Hi Kai,

Both of your patches are in CVS now since they don't at least seem to  
do any harm. Any further patches are very welcome since you seemed to  
have a quite good idea of things that need improvement :)

Johan

-------------------------------------------------------------------------
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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints
@ 2007-10-17 16:31 Kai.Vehmanen
  2007-10-18  8:42 ` Johan Hedberg
  2007-10-18 10:08 ` Fabien Chevalier
  0 siblings, 2 replies; 10+ messages in thread
From: Kai.Vehmanen @ 2007-10-17 16:31 UTC (permalink / raw)
  To: bluez-devel

Hi,

On 12 Oct 2007, Kai Vehmanen wrote: 
>Here're couple of patches (against CVS HEAD) that solve the 
>problems for me (for instance pulseaudio starts working with 
>the plugin, less CPU usage with other apps). If you think 
[...]
>This first modifies the buffering constraints as exposed by 
>the A2DP ALSA plugin:
>  - increased max overall buffer size to 16384 bytes (~93msecs
>    of CD quality audio, so a reasonable max size)

hmm, looking at this more deeply, I realized this doesn't quite 
work. I originally thought there is a real ringbuffer (imitating 
the usual audio hardware ringbuffer) between the worker
thread and the application, but this appears not to be the
case. Current code seems to gather samples-to-be-sent in a buffer,
and when a threshold (a2dp->codesize or cfg.pkt_len) is reached,
a packet is then sent right away. So in other words, application
cannot really prefill more data than one packet worth, and most of 
the allocated buffer space gets never used.

So I guess this specific patch should be dropped (it might fix
some issues, but at least the assumptions I used to pick the 
values are wrong ;)). The other patch should still be valid.

What I intended with the patch was that you could...:
  - prefill N*a2dp_codesize (less than buffersize) worth of 
    PCM frames
  - trigger playback 
      -> oldest queued a2dp_codesize worth of samples is encoded 
        and sent
  - application is woken up every M*a2dp_codesize
      - in case application is occasionally late, the worker
        thread would still be encoding and sending packets
        at a steady rate using the buffered samples
	- with M>1, application can reduce the amount of wakeups
        needed, but will increase the playout latency (but
        note: the HW thread still needs to wake up for every 
        a2dp_codesize)
  - an XRUN occurs only if application is stalled for 
    duration of N*a2dp_codesize
        
But yeah, that would seem to be a much bigger task to implement (and
something you may have already discussed earlier).

br,
-- 
first.surname@nokia.com (Kai Vehmanen)

-------------------------------------------------------------------------
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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2007-10-18 17:33 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-10-12 12:25 [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints Kai Vehmanen
2007-10-14  3:36 ` Luiz Augusto von Dentz
2007-10-14 11:19 ` Fabien Chevalier
2007-10-15 10:54   ` [Bluez-devel] PATCH1/2: bluez-utils -fix-a2dp-buffer-constraints Kai Vehmanen
2007-10-17 10:23     ` Fabien Chevalier
2007-10-17 14:36       ` Kai.Vehmanen
2007-10-17 16:31 [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints Kai.Vehmanen
2007-10-18  8:42 ` Johan Hedberg
2007-10-18 10:08 ` Fabien Chevalier
2007-10-18 17:33   ` Jim Carter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.