2007-10-17 14:36:58

by Kai Vehmanen

[permalink] [raw]
Subject: Re: [Bluez-devel] PATCH1/2: bluez-utils -fix-a2dp-buffer-constraints

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.

--
[email protected] (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
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

2007-10-17 10:23:06

by Fabien Chevalier

[permalink] [raw]
Subject: Re: [Bluez-devel] PATCH1/2: bluez-utils -fix-a2dp-buffer-constraints

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
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

2007-10-15 10:54:13

by Kai Vehmanen

[permalink] [raw]
Subject: Re: [Bluez-devel] PATCH1/2: bluez-utils -fix-a2dp-buffer-constraints

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.

--
[email protected] (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
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

2007-10-14 11:19:41

by Fabien Chevalier

[permalink] [raw]
Subject: Re: [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints

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
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

2007-10-14 03:36:19

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [Bluez-devel] PATCH1/2: bluez-utils - fix-a2dp-buffer-constraints

Hi Kai,

On 10/12/07, Kai Vehmanen <[email protected]> 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
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel