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

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

[Kai.Vehmanen]

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

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

[Johan Hedberg]

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

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

[Fabien Chevalier]

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

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

[Jim Carter]

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

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

[Kai.Vehmanen]

Hi,

On 18 Oct 2007,  Jim Carter wrote:
>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?

ok, I'm very new to A2DP, but my understanding is that timing 
is done similarly as in RTP-over-IP: the receiver adapts to
the clock of the sender. This means the sender just needs to 
send at a steady pace (not required, but makes life easier for
the receiver). And as with RTP, I assume implementations vary
a lot in quality, so improving the timing (reducing jitter 
and drift against nominal rate) will improve interoperability
between different devices.

Any ++agreed or --aggreed statements? :)

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

Have you tested with my patches (now in CVS)? Vanilla 3.19/3.20
didn't work at all with Pulseaudio, but with my patches the
playback is now flawless.

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

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

[Luiz Augusto von Dentz]

Hi Kai,

> ok, I'm very new to A2DP, but my understanding is that timing
> is done similarly as in RTP-over-IP: the receiver adapts to
> the clock of the sender. This means the sender just needs to
> send at a steady pace (not required, but makes life easier for
> the receiver). And as with RTP, I assume implementations vary
> a lot in quality, so improving the timing (reducing jitter
> and drift against nominal rate) will improve interoperability
> between different devices.
>
> Any ++agreed or --aggreed statements? :)

Yep, that is probably a good explanation why some devices
work better than others.

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

There is some misunderstanding about the buffer size we use, those
buffers are not intended to be used as ring buffers, it is just fill buffers
so when you got enough frames to send they are written in the socket
to device. The first buffer used (bluetooth_data) for pcm frames witch
should be the same size of the codesize, it is intend to buffer pcm
frames until it got enough data to encode a sbc frame than it should
be restarted. The second buffer (bluetooth_a2dp) is filled with sbc frames
it should have the mtu size of the l2cap, it stuff how many frames it can
in there until there is no space left for another frame. The pace should
be adjusted by the worker thread.

Making the buffer to be 16384 bytes long is nonsense we probably will never
use this, bluetooth_a2dp_write does not expect block sizes bigger than code=
size.
So we need to fix this as soon as possible.

One thing that may worth trying is to set the supported block sizes up to
the number of pcm frames it need to fill an entire packet, normally this
means 4-6 sbc frames so it would be 2kb to 3kb (each sbc frame
normally correspond to 512bytes).

-- =

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] PATCH1/2: bluez-utils -fix-a2dp-buffer-constraints

[Fabien Chevalier]

Hi Kai, please find below some comments
> 
> On 18 Oct 2007,  Jim Carter wrote:
>> 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?
> 
> ok, I'm very new to A2DP, but my understanding is that timing 
> is done similarly as in RTP-over-IP: the receiver adapts to
> the clock of the sender. This means the sender just needs to 
> send at a steady pace (not required, but makes life easier for
> the receiver). And as with RTP, I assume implementations vary
> a lot in quality, so improving the timing (reducing jitter 
> and drift against nominal rate) will improve interoperability
> between different devices.

I agree with you :-)
Except for the timing part, i'm not really sure the receiver is that 
clever. I guess at least some of them will expect to receive data with
at their local clock speed...

> 
> Any ++agreed or --aggreed statements? :)
> 
>> 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.
> 
> Have you tested with my patches (now in CVS)? Vanilla 3.19/3.20
> didn't work at all with Pulseaudio, but with my patches the
> playback is now flawless.

I didn't had time to test them, sorry :-(

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

[Bluez-devel] experimental ALSA/A2DP plugin patch (was: RE: PATCH1/2: bluez-utils -fix-a2dp-buffer-constraints)

[Kai.Vehmanen]

Hi,

On 23 Oct 2007,  Fabien Chevalier wrote:
>Except for the timing part, i'm not really sure the receiver 
>is that clever. I guess at least some of them will expect to 
>receive data with at their local clock speed...

hmm, based on further reading of the spec, I think the receivers
must be that clever. And this makes sense, as it's basicly RTP, 
so an off-the-shelf RTP receiver implementation would work
for the headsets.

And that assumption leads to an experimental patch (patch plus
patched pcm_bluetooth.c for convenience):

http://sofia-sip.org/~vehmanek/bluez-patches/patch-pcm_bluetooth-kv-2007
1025-works.txt
http://sofia-sip.org/~vehmanek/bluez-patches/pcm_bluetooth.c

 - send errors are now ignored (packets dropped), seems
   to have no bad side effects on the headsets I've tested
   with
     -> the positive impact is that we can more easily keep
	  up the correct timing even with bigger period sizes
 - a2dp_write() now loops'n'sends until all data submitted by
   the app is  encoded (needed to work with MMAP using clients)
 - allowed period sizes defined as set of values (vs range), 
   this works with for example pcm.c (seems to be an alsa-lib bug,
   but didn't have time to track it down)
 - period count fixed to 3 (seems to work best and still 
   doesn't confuse ALSA apps)

I've tested this with alsa-lib pcm.c, aplay, and pulseaudio, and all
seem to work better. But **NOTE**, I've only tested with limited
number of headsets, so it might be that ignoring send errors
causes audible glitches on some models. So feedback is very welcome!

If you can, please test this and report whether this improves (or 
degrades) the audio quality, and/or compatibility with ALSA apps 
you are using.

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

Re: [Bluez-devel] experimental ALSA/A2DP plugin patch

[Fabien Chevalier]

Hi Kai,


> Hi,
> 
> On 23 Oct 2007,  Fabien Chevalier wrote:
>> Except for the timing part, i'm not really sure the receiver 
>> is that clever. I guess at least some of them will expect to 
>> receive data with at their local clock speed...
> 
> hmm, based on further reading of the spec, I think the receivers
> must be that clever. And this makes sense, as it's basicly RTP, 
> so an off-the-shelf RTP receiver implementation would work
> for the headsets.

There is a major difference between Internet RTP and Bluetooth RTP.
AFAIK the senders and the receivers' clocks are not syncrhonized on the 
Internet, while using bluetooth technology all slaves devices have an 
estimate of the master clock.

As for off the shelf implementations, i guess everybody just rewrites an 
  RTP implementation for it's own device, as we have done for the Bluez 
project :-)

> 
> And that assumption leads to an experimental patch (patch plus
> patched pcm_bluetooth.c for convenience):
> 
> http://sofia-sip.org/~vehmanek/bluez-patches/patch-pcm_bluetooth-kv-2007
> 1025-works.txt
> http://sofia-sip.org/~vehmanek/bluez-patches/pcm_bluetooth.c
> 
>  - send errors are now ignored (packets dropped), seems
>    to have no bad side effects on the headsets I've tested
>    with
>      -> the positive impact is that we can more easily keep
> 	  up the correct timing even with bigger period sizes
>  - a2dp_write() now loops'n'sends until all data submitted by
>    the app is  encoded (needed to work with MMAP using clients)
>  - allowed period sizes defined as set of values (vs range), 
>    this works with for example pcm.c (seems to be an alsa-lib bug,
>    but didn't have time to track it down)
>  - period count fixed to 3 (seems to work best and still 
>    doesn't confuse ALSA apps)
> 


I'm gonna have a look to that :-)

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

[Bluez-devel] Kai's experimental ALSA/A2DP plugin patch review

[Fabien Chevalier]

Hi Kai,

I generated a patch from your latest pcm_bluetooth.c changes.
Please find some comments below (look for ==> sequence for my comments) :

--- pcm_bluetooth.c.orig	2007-11-01 14:48:34.000000000 +0100
+++ pcm_bluetooth.c	2007-10-30 13:31:14.000000000 +0100
@@ -609,8 +609,8 @@
  	header->timestamp = htonl(a2dp->nsamples);
  	header->ssrc = htonl(1);

-	ret = send(data->stream.fd, a2dp->buffer, a2dp->count, MSG_DONTWAIT);
-	if (ret == -1)
+        ret = send(data->stream.fd, a2dp->buffer, a2dp->count, 0);

==> MSG_DONTWAIT is there to prevent the l2cap socket to lock us up,
if for some (bad) reason the l2cap layer is not able to send data at the 
right speed. I see no reason to change that !!


+	if (ret < 0)
  		ret = -errno;

  	/* Reset buffer of data to send */
@@ -629,19 +629,20 @@
  	struct bluetooth_data *data = io->private_data;
  	struct bluetooth_a2dp *a2dp = &data->a2dp;
  	snd_pcm_sframes_t ret = 0;
-	snd_pcm_uframes_t frames_to_read;
+	snd_pcm_uframes_t frames_to_read, frames_left = size;
  	int frame_size, encoded;
  	uint8_t *buff;

  	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);
+	DBG("hw_ptr=%lu appl_ptr=%lu diff=%lu", io->hw_ptr, io->appl_ptr, 
io->appl_ptr - io->hw_ptr);

  	if (io->hw_ptr > io->appl_ptr) {
-		ret = bluetooth_playback_stop(io);
-		if (ret == 0)
-			ret = -EPIPE;
-		goto done;
+		/* Delegate XRUN handling to the headset. Here we just
+		 * reset the virtual hardware pointer. */
+		DBG("hw-ptr reset, hw_ptr=%lu appl_ptr=%lu diff=%lu.\n",
+		       io->hw_ptr, io->appl_ptr, io->appl_ptr - io->hw_ptr);
+		io->hw_ptr = io->appl_ptr;
  	}

==> Could you explain why this change is needed ? It seems to be pretty 
bas to me as it is not *at all* in the ALSA philosophy to play magics 
whith the hw_ptr, especially not bring it back in  the past !!

  	/* Check if we should autostart */
@@ -661,9 +662,11 @@
  		snd_pcm_sw_params_free(swparams);
  	}

+	while(frames_left > 0) {
+
  	frame_size = areas->step / 8;
-	if ((data->count + size * frame_size) <= a2dp->codesize)
-		frames_to_read = size;
+		if ((data->count + frames_left * frame_size) <= a2dp->codesize)
+			frames_to_read = frames_left;
  	else
  		frames_to_read = (a2dp->codesize - data->count) / frame_size;

@@ -674,7 +677,7 @@

  	/* Ready for more data */
  	buff = (uint8_t *) areas->addr +
-				(areas->first + areas->step * offset) / 8;
+			(areas->first + areas->step * (offset + ret)) / 8;
  	memcpy(data->buffer + data->count, buff, frame_size * frames_to_read);

  	/* Remember we have some frames in the pipe now */
@@ -693,15 +696,16 @@

  	data->count -= encoded;

-	DBG("encoded=%d  a2dp.sbc.len=%d", encoded, a2dp->sbc.len);
+		DBG("encoded=%d  a2dp.sbc.len=%d count=%d", encoded, a2dp->sbc.len, 
a2dp->count);

+		/* Send previously encoded buffer */
  	if (a2dp->count + a2dp->sbc.len >= data->cfg.pkt_len) {
-		ret = avdtp_write(data);
-		if (ret < 0) {
-			if (-ret == EPIPE)
-				ret = -EIO;
-			goto done;
-		}
+#ifdef ENABLE_DEBUG
+			int old_count = a2dp->count;
+			int c =
+#endif
+			avdtp_write(data);
+			DBG("sending packet %d, count %d, pkt_len %u", c, old_count, 
data->cfg.pkt_len);
  	}

  	memcpy(a2dp->buffer + a2dp->count, a2dp->sbc.data, a2dp->sbc.len);
@@ -710,7 +714,15 @@
  	a2dp->samples += encoded / frame_size;
  	a2dp->nsamples += encoded / frame_size;

-	ret = frames_to_read;
+		/* (io->buffer_size + io->appl_ptr - io->period_size) * % 
io->buffer_size */
+
+		ret += frames_to_read;
+		frames_left -= frames_to_read;
+	}
+
+	/* note: some ALSA apps will get confused otherwise */
+	if (ret > size)
+		ret = size;

  done:
  	DBG("returning %ld", ret);
@@ -847,7 +859,6 @@
  static int bluetooth_a2dp_hw_constraint(snd_pcm_ioplug_t *io)
  {
  	struct bluetooth_data *data = io->private_data;
-	struct bluetooth_a2dp *a2dp = &data->a2dp;
  	struct ipc_data_cfg cfg = data->cfg;
  	snd_pcm_access_t access_list[] = {
  		SND_PCM_ACCESS_RW_INTERLEAVED,
@@ -886,18 +897,26 @@
  	if (err < 0)
  		return err;

-	/* 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);
+	/* supported block sizes: */
+	{
+		const unsigned int period_list[] = {
+			2048,
+			4096, /* 23/46ms (mono/stereo 16bit at 44.1kHz) */
+#if 0
+			8192, /* 46/93ms */
+			16384 /* 93/185ms */
+#endif
+		};
+		
+		err = snd_pcm_ioplug_set_param_list(io, SND_PCM_IOPLUG_HW_PERIOD_BYTES,
+						    ARRAY_NELEMS(period_list), period_list);
  	if (err < 0)
  		return err;
+	}

+	/* period count fixed to 3 as we don't support prefilling */
  	err = snd_pcm_ioplug_set_param_minmax(io, SND_PCM_IOPLUG_HW_PERIODS,
-						2, 50);
+						3, 3);
  	if (err < 0)
  		return err;


>  - send errors are now ignored (packets dropped), seems
>    to have no bad side effects on the headsets I've tested
>    with
>      -> the positive impact is that we can more easily keep
> 	  up the correct timing even with bigger period sizes

I agree with that.

>  - a2dp_write() now loops'n'sends until all data submitted by
>    the app is  encoded (needed to work with MMAP using clients)

Ok for me :-)

>  - allowed period sizes defined as set of values (vs range), 
>    this works with for example pcm.c (seems to be an alsa-lib bug,
>    but didn't have time to track it down)

Ok with that too.

>  - period count fixed to 3 (seems to work best and still 
>    doesn't confuse ALSA apps)

OK with that too.

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] Kai's experimental ALSA/A2DP plugin patch review

[Kai.Vehmanen]

Hi Fabien,

thanks for reviewing the patch! 

On 01 Nov 2007,  Fabien Chevalier wrote:
>I generated a patch from your latest pcm_bluetooth.c changes.
>Please find some comments below (look for ==> sequence for my 
>comments) :

btw, I've continued to test the patch and found out...
 - poll()'ing in current CVS code doesn't really work as
   'events' fields are not correctly set in hw_thread
 - but if you do enable the polling (of the L2CAP socket fd),
   the wakeups are far too frequent (and eat too much CPU time)
     - audio playback is OK, but spending 99% of CPU time is
       not really worth it ;)
     -> so unless there is a way to configure the L2CAP to 
        wake up only when "at least X bytes can be written", 
        the only sensible solution is a) blocking writes to
        the L2CAP socket, b) timer paced writes to L2CAP (which
        causes problems with some headsets, e.g. iMuff)

If noone comes up with a way to set a wakeup threshold for 
the L2CAP sockets, I'd propose the following steps forward:

(i) Merge some version (not ready yet) of my patch -> makes the plugin 
    compatible with apps such as Pulseaudio. Currently I'm not
    aware of any regressions over the CVS version. The only known 
    problematic headset is iMuff, but that doesn't work with
    CVS version of pcm_bluetooth.c either (but does work 
    with gsta2dpsink). So not a full solution, but at a step forward.
(ii) Write a version of the plugin in which a worker thread 
     does blocking writes to the L2CAP socket (implementation
     is then identical to gsta2dpsink). In this approach, hw_ptr
     is updated based on the blocking writes to L2CAP, not based on 
     system-timer.

I'm not sure yet, whether I have time to write (ii), but I'll
keep working on (i) for now.

Then some comments to your comments below...:

>-	ret = send(data->stream.fd, a2dp->buffer, a2dp->count,
MSG_DONTWAIT);
>
>==> MSG_DONTWAIT is there to prevent the l2cap socket to lock 
>us up, if for some (bad) reason the l2cap layer is not able to 
>send data at the right speed. I see no reason to change that !!

But if we use MSG_DONTWAIT, the send() will return an error every now 
as the L2CAP buffers are full (as our send()'s are paced by a system
timer, 
and are in no way synchronized to the actual L2CAP buffer status). And 
with our current plugin design, we have no way to handle this error
scenario without messing up the client timing (I've tried a lot
of things, but none of them work). :(

So until we implement (ii) approach (see above), I don't 
see any other solution than to drop MSG_DONTWAIT.

>  	if (io->hw_ptr > io->appl_ptr) {
>-		ret = bluetooth_playback_stop(io);
>-		if (ret == 0)
>-			ret = -EPIPE;
>-		goto done;
>+		/* Delegate XRUN handling to the headset. Here we just
>+		 * reset the virtual hardware pointer. */
>+		DBG("hw-ptr reset, hw_ptr=%lu appl_ptr=%lu diff=%lu.\n",
>+		       io->hw_ptr, io->appl_ptr, io->appl_ptr - 
>io->hw_ptr);
>+		io->hw_ptr = io->appl_ptr;
>  	}
>
>==> Could you explain why this change is needed ? It seems to 
>be pretty 
>bas to me as it is not *at all* in the ALSA philosophy to play magics 
>whith the hw_ptr, especially not bring it back in  the past !!

Well, the hw_ptr is really just a guesstimate in our plugin, as we
don't know how the headset is operating. We try to guess (and maintain
a system-timer paced hw_ptr of our own), but if we are wrong, 
the above happens. It doesn't make sense to stick to our interpretation
of hw_ptr as that will result in audible hickups in the headset.

If you revert the above segment of the patch, you'll get an XRUN
couple of times per minute on many headsets. And this is not 
process scheduling related, so not an actual XRUN, but instead is 
caused by drift against our guess of hw_ptr and the actual hw_ptr 
of the headset.

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

Re: [Bluez-devel] Kai's experimental ALSA/A2DP plugin patch review

[Fabien Chevalier]

Hi Kai,

Please find below some comments.

For those too lazy to read this whole stuff : here is a quick status

  * i noticed audible audio clicks while using rhythmbox and my HBH 
DS970 headset: from my point of view the audio quality is clearly 
inferior to what is in CVS today.
  * as such this patch is not ready for inclusion, furthermore it still 
has some design flaws (see below).
  * if Kai goes on with he's good work, i have no doubt he's gonna come 
up with something quite good in the future :-) ... but for now we're not 
there yet.

> Hi Fabien,
> 
> thanks for reviewing the patch! 
> 
> On 01 Nov 2007,  Fabien Chevalier wrote:
>> I generated a patch from your latest pcm_bluetooth.c changes.
>> Please find some comments below (look for ==> sequence for my 
>> comments) :
> 
> btw, I've continued to test the patch and found out...
>  - poll()'ing in current CVS code doesn't really work as
>    'events' fields are not correctly set in hw_thread

Could you be more specific on what doesn't work ? If this is the poll()
call in hw_thread, it is unused for now (i.e never exits due to fd 
activity, but always on timeout).So that wouldn't surprise me if it was 
  kind of broken. ;-)


>  - but if you do enable the polling (of the L2CAP socket fd),
>    the wakeups are far too frequent (and eat too much CPU time)
>      - audio playback is OK, but spending 99% of CPU time is
>        not really worth it ;)
>      -> so unless there is a way to configure the L2CAP to 
>         wake up only when "at least X bytes can be written", 
>         the only sensible solution is a) blocking writes to
>         the L2CAP socket, b) timer paced writes to L2CAP (which
>         causes problems with some headsets, e.g. iMuff)

You CAN'T throttle yourself by using the l2cap socket blocking call or 
poll() syscall. That causes some headsets to play too fast as they have 
no mean to throttle the speed at which we send data to them. In fact 
this is the first implementation people do, until they discover that it
breaks with some headsets (Luiz started with that, as well as people at 
my company who wrote an A2DP implementation)

> 
> If noone comes up with a way to set a wakeup threshold for 
> the L2CAP sockets, I'd propose the following steps forward:
> 
> (i) Merge some version (not ready yet) of my patch -> makes the plugin 
>     compatible with apps such as Pulseaudio. Currently I'm not
>     aware of any regressions over the CVS version. The only known 
>     problematic headset is iMuff, but that doesn't work with
>     CVS version of pcm_bluetooth.c either (but does work 
>     with gsta2dpsink). So not a full solution, but at a step forward.
> (ii) Write a version of the plugin in which a worker thread 
>      does blocking writes to the L2CAP socket (implementation
>      is then identical to gsta2dpsink). 

As explaned above, this is not possible. If gsta2dpsink is written that 
way today, then it needs to be fixed, not the other way around. :-)

In this approach, hw_ptr
>      is updated based on the blocking writes to L2CAP, not based on 
>      system-timer.
> 
> I'm not sure yet, whether I have time to write (ii), but I'll
> keep working on (i) for now.

Go for (i) then. Have you given up on rewriting the worker thread so 
that it wakes up at regular intervals to send data over the network ?

> 
> Then some comments to your comments below...:
> 
>> -	ret = send(data->stream.fd, a2dp->buffer, a2dp->count,
> MSG_DONTWAIT);
>> ==> MSG_DONTWAIT is there to prevent the l2cap socket to lock 
>> us up, if for some (bad) reason the l2cap layer is not able to 
>> send data at the right speed. I see no reason to change that !!
> 
> But if we use MSG_DONTWAIT, the send() will return an error every now 
> as the L2CAP buffers are full (as our send()'s are paced by a system
> timer, 

This is exactly the right thing to do. We just have to ignore this error 
and go forward. In no way we should block ourself on the system call.

> and are in no way synchronized to the actual L2CAP buffer status). And 
> with our current plugin design, we have no way to handle this error
> scenario without messing up the client timing (I've tried a lot
> of things, but none of them work). :(

See above, what you propose is not possible.

> 
> So until we implement (ii) approach (see above), I don't 
> see any other solution than to drop MSG_DONTWAIT.
> 
>>  	if (io->hw_ptr > io->appl_ptr) {
>> -		ret = bluetooth_playback_stop(io);
>> -		if (ret == 0)
>> -			ret = -EPIPE;
>> -		goto done;
>> +		/* Delegate XRUN handling to the headset. Here we just
>> +		 * reset the virtual hardware pointer. */
>> +		DBG("hw-ptr reset, hw_ptr=%lu appl_ptr=%lu diff=%lu.\n",
>> +		       io->hw_ptr, io->appl_ptr, io->appl_ptr - 
>> io->hw_ptr);
>> +		io->hw_ptr = io->appl_ptr;
>>  	}
>>
>> ==> Could you explain why this change is needed ? It seems to 
>> be pretty 
>> bas to me as it is not *at all* in the ALSA philosophy to play magics 
>> whith the hw_ptr, especially not bring it back in  the past !!
> 
> Well, the hw_ptr is really just a guesstimate in our plugin, as we
> don't know how the headset is operating. We try to guess (and maintain
> a system-timer paced hw_ptr of our own), but if we are wrong, 
> the above happens. It doesn't make sense to stick to our interpretation
> of hw_ptr as that will result in audible hickups in the headset.

Kai, i think you're messing up your pointers ;-)
The story between hw_ptr and appl_ptr is purely an application<-->alsa 
plugin interface issue, it has very little to do with the actual headset 
hardware. And this is in no way (as least today - i proposed an approach 
but until we actually find a headset that breaks due to the lack of 
synchronisation i don't think i'm gonna manage to convince anybody that 
this is really usefull ;-) ) an estimate of the 'hw_ptr' for the headset.


> 
> If you revert the above segment of the patch, you'll get an XRUN
> couple of times per minute on many headsets. And this is not 
> process scheduling related, so not an actual XRUN, but instead is 
> caused by drift against our guess of hw_ptr and the actual hw_ptr 
> of the headset.

See above, that doesn't make sense.
I think you must be somewhat mixing up different issues at the same time 
  :-(

I've suffered real pain having this part of the code to work (even if 
it's not perfect as you've noticed it), so has frederic dalleau : if you 
want some tips to undersand why things are as they are today and what 
other people have tryed before you, feel free to ask me on IRC

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] Kai's experimental ALSA/A2DP plugin patch review

[Kai.Vehmanen]

Hi,

On 01 Nov 2007,  Fabien Chevalier wrote:
>For those too lazy to read this whole stuff : here is a quick status
>
>  * i noticed audible audio clicks while using rhythmbox and 
>my HBH DS970 headset: from my point of view the audio quality 
>is clearly inferior to what is in CVS today.

hmm, so that's with gstalsasink using pcm_bluetooth.c, right?
But yeah, one regression is enough, so back to the drawing 
board.

>>  - poll()'ing in current CVS code doesn't really work as
>>    'events' fields are not correctly set in hw_thread
>
>Could you be more specific on what doesn't work ? If this is 
>the poll() call in hw_thread, it is unused for now (i.e never 
>exits due to fd activity, but always on timeout).So that 
>wouldn't surprise me if it was kind of broken. ;-)

Yes, exactly that (they are not really polled).

>>  - but if you do enable the polling (of the L2CAP socket fd),
>>    the wakeups are far too frequent (and eat too much CPU time)
>
>You CAN'T throttle yourself by using the l2cap socket blocking call or
>poll() syscall. That causes some headsets to play too fast as 
>they have no mean to throttle the speed at which we send data 

Well, that's what I've thought so far, but then when I checked
gsta2dpsink (which seems to work better with more headsets than
pcm_bluetooth.c), it would seem to do just that. In

http://bluez.cvs.sourceforge.net/bluez/utils/audio/gsta2dpsink.c?revisio
n=1.13&view=markup

- 'self->stream' is set to blocking mode
- gst_a2dp_sink_render() just writes to the socket with seemingly
  blocking writes

... unless I misread the code (possible ;)), this seems to do 
exactly what I describe above (and unlike current pcm_bluetooth.c).

With which headsets the playback is too fast?

>> (ii) Write a version of the plugin in which a worker thread 
>>      does blocking writes to the L2CAP socket (implementation
>>      is then identical to gsta2dpsink). 
>
>As explaned above, this is not possible. If gsta2dpsink is 
>written that way today, then it needs to be fixed, not the 
>other way around. :-)

Hmm, but for instance gsta2dpsink works with the iMuff headset,
while the system-timer paced pcm_bluetooth.c doesn't.

>> I'm not sure yet, whether I have time to write (ii), but I'll keep 
>> working on (i) for now.
>
>Go for (i) then. Have you given up on rewriting the worker 
>thread so that it wakes up at regular intervals to send data 
>over the network ?

Not totally, but it would seem that starting any large-scale work is
kind of pointless without a truckload of different headsets available
for testing. Oh well, such is life. :(

>> But if we use MSG_DONTWAIT, the send() will return an error every now

>> as the L2CAP buffers are full (as our send()'s are paced by a system 
>> timer,
>
>This is exactly the right thing to do. We just have to ignore 
>this error and go forward. In no way we should block ourself 
>on the system call.

But as we don't have a real ringbuffer yet, we can't wait and
resend later (as by that time the application has provided us
with yet more data). We can of course stop the application, but
that will cause an XRUN (and audible glitch). Without MSG_DONTWAIT,
the write will block momentarily every now and then (every few minuts
with the headsets I've been testing with), and audio is glitch-free.

Oh well, but I guess we agree that a real ringbuffer would be
needed. 

>> and are in no way synchronized to the actual L2CAP buffer status).
And 
>> with our current plugin design, we have no way to handle this error 
>> scenario without messing up the client timing (I've tried a lot of 
>> things, but none of them work). :(
>
>See above, what you propose is not possible.

But it works with the headsets I've been testing with! Of course that
doesn't prove anything in the general case, just explaining how I
ended up with the proposal.

>> Well, the hw_ptr is really just a guesstimate in our plugin, as we 
>> don't know how the headset is operating. We try to guess (and
maintain 
>
>Kai, i think you're messing up your pointers ;-) The story 
>between hw_ptr and appl_ptr is purely an application<-->alsa 
>plugin interface issue, it has very little to do with the 
>actual headset hardware. And this is in no way (as least today 

Yes and no. If the hw_ptr is not synced to the headset, 
the application will write data too fast (leading
to either a send() error, or blocking send(), depending on
whether MSG_DONTWAIT is set or not).

>- i proposed an approach but until we actually find a headset 
>that breaks due to the lack of synchronisation i don't think 
>i'm gonna manage to convince anybody that this is really 
>usefull ;-) ) an estimate of the 'hw_ptr' for the headset.

Well, the problem realizes when you try to reduce the application
wakeups with pcm_bluetooth.c. The audio _does_ glitch for instance 
with Plantronics P590 (and fixed by "hw_ptr adjusting").

>> If you revert the above segment of the patch, you'll get an XRUN 
>> couple of times per minute on many headsets. And this is not process 
>> scheduling related, so not an actual XRUN, but instead is caused by 
>> drift against our guess of hw_ptr and the actual hw_ptr of the 
>> headset.
>
>See above, that doesn't make sense.

Well it does. When call to send() blocks (or returns EAGAIN), we've 
been writing data at an incorrect rate, and also updated hw_ptr at 
a wrong rate (w.r.t. to the L2CAP-socket/headset). Thus we need to
either
a) adjust hw_ptr (what my patch does), b) drop the packet (causes
an audible glitch).

>I've suffered real pain having this part of the code to work 
>(even if it's not perfect as you've noticed it), so has 
>frederic dalleau : if you want some tips to undersand why 

I can believe that! :) As I have very little experience with
Bluetooth (my background is with audio apps), I'm sure I'd 
have even more painful road ahead of me unless I follow your
footsteps here. It's definitely clear to me now, that I need to
get more headsets and/or spend more time reading the core
BT specs in order to propose solutions that work in the general
case. But I still want to emphasize that my patches are addressing 
real problems I can reproduce with real-life headsets and apps.
IOW, pulseaudio is still not working with CVS-HEAD of 
pcm_bluetooth.c. :(

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

Re: [Bluez-devel] Kai's experimental ALSA/A2DP plugin patch review

[Kai.Vehmanen]

Hi,

quick reply to myself:

On 02 November 2007, Vehmanen Kai wrote:
>Hmm, but for instance gsta2dpsink works with the iMuff 
>headset, while the system-timer paced pcm_bluetooth.c doesn't.

oops, I mixed the headset models, it was something else
than iMuff. Anyways, it seems that some headsets take advantage
of L2CAP flow control (-> blocking writes to L2CAP will work 
more reliably as we don't need to guesstimate hw_ptr), while 
some don't (-> we have to use a system timer). Now my patch tries 
to work with both (it uses a system timer, but can adjust the hw_ptr 
if needed as well), but as Fabien pointed out, some cases are still 
not handled. Now I guess a good follow-up question is that should
the plugin try to verify whether L2CAP flow-control is used
or not, and then adjust its behaviour...?

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

[Bluez-devel] new not-so-experimental ALSA plugin patch (was: RE: Kai's experimental ALSA/A2DP plugin patch review)

[Kai.Vehmanen]

Hi Fabien and others,

On 01 Nov 2007, Fabien Chevalier wrote:
>  * i noticed audible audio clicks while using rhythmbox and 
>my HBH DS970 headset: from my point of view the audio quality 
>is clearly inferior to what is in CVS today.
>  * as such this patch is not ready for inclusion, furthermore 
>it still has some design flaws (see below).

so there (see above) were problems with my last attempt. But in 
the spririt of...

>  * if Kai goes on with he's good work, i have no doubt he's 
>gonna come up with something quite good in the future :-) ... 
>but for now we're not there yet.

... ;), here's another attempt. I now got a Logitech Freepulse 
headset, which triggered quite a few issues I hadn't seen
before (some of which happen also with CVS-HEAD of pcm_bluetooth.c):

Delta to last patch:
  - brought MSG_DONTWAIT back (and dropped hw_ptr "adaptation" as
    it's not needed if MSG_DONTWAIT is used)
	-> rely on XRUN mechanism as before
  - dropped 16384 byte period size -> broken playback with
    Logitech Freepulse
  - modified polling mechanism to queue wakeups (write to internal
    pipe) for each hw_ptr increment -> this fixes plugin's poll() 
    behaviour in cases where hw_ptr jumps multiple periods in
    one go -> fixes a problem with Logitech Freepulse
  - added a simple mechanism to reset the hw_thread state 
    when XRUNs occur

General ideas I'm trying to accomplish:
  - allow use of bigger blocksizes to reduce wakeup load
  - fix playback with apps that rely on accurate poll
    behaviour: alsa-lib/test/pcm.c and 'write_and_poll' access
    method, Pulseaudio 0.9.[67], ...
  - make a2dp_write consume all bytes in one go (needed to
    make MMAP emulation work and still allow bigger period sizes)

If this is ok to the reviewers, I'm proposing to merge this
into CVS-HEAD.

Patched pcm_bluetooth.c, and a patch against CVS-HEAD are 
available at:
http://sofia-sip.org/~vehmanek/bluez-patches/pcm_bluetooth.c-worksgood20
071108-clean
http://sofia-sip.org/~vehmanek/bluez-patches/patch-20071108-a2dp-wakeup-
rectoring-kv.txt

Here's the patch inlined for commenting:
--cut--
--- pcm_bluetooth.c?revision=1.77	2007-11-06 13:37:25.000000000
+0200
+++ pcm_bluetooth.c-worksgood20071108-clean	2007-11-08
11:53:56.000000000 +0200
@@ -3,6 +3,7 @@
  *  BlueZ - Bluetooth protocol stack for Linux
  *
  *  Copyright (C) 2004-2007  Marcel Holtmann <marcel@holtmann.org>
+ *  Copyright (C) 2007 Nokia Corporation
  *
  *
  *  This library is free software; you can redistribute it and/or
@@ -30,6 +31,7 @@
 #include <sys/un.h>
 #include <sys/time.h>
 #include <pthread.h>
+#include <signal.h>
 
 #include <netinet/in.h>
 
@@ -91,6 +93,7 @@
 	pthread_t hw_thread;		/* Makes virtual hw pointer move
*/
 	int pipefd[2];			/* Inter thread communication */
 	int stopped;
+	sig_atomic_t reset;             /* Request XRUN handling */
 };
 
 static int bluetooth_start(snd_pcm_ioplug_t *io)
@@ -116,6 +119,9 @@
 	struct pollfd fds[2];
 	int poll_timeout;
 
+	data->server.events = POLLIN;
+	/* note: only errors for data->stream.events */
+
 	fds[0] = data->server;
 	fds[1] = data->stream;
 
@@ -136,6 +142,13 @@
 		if (data->stopped)
 			goto iter_sleep;
 
+		if (data->reset) {
+			DBG("Handle XRUN in hw-thread.");
+			data->reset = 0;
+			gettimeofday(&start, 0);
+			prev_periods = 0;
+		}
+
 		gettimeofday(&cur, 0);
 
 		timersub(&cur, &start, &delta);
@@ -145,14 +158,17 @@
 
 		if (periods > prev_periods) {
 			char c = 'w';
+			int frags = periods - prev_periods, n;
 
-			data->hw_ptr += (periods - prev_periods) *
-
data->io.period_size;
+			data->hw_ptr += frags *	data->io.period_size;
 			data->hw_ptr %= data->io.buffer_size;
 
-			/* Notify user that hardware pointer has moved
*/
-			if (write(data->pipefd[1], &c, 1) < 0)
-				pthread_testcancel();
+			for (n = 0; n < frags; n++) {
+				/* Notify user that hardware pointer
+				 * has moved * */
+				if (write(data->pipefd[1], &c, 1) < 0)
+					pthread_testcancel();
+			}
 
 			/* Reset point of reference to avoid too big
values
 			 * that wont fit an unsigned int */
@@ -167,6 +183,7 @@
 iter_sleep:
 		/* sleep up to one period interval */
 		ret = poll(fds, 2, poll_timeout);
+		
 		if (ret < 0) {
 			SNDERR("poll error: %s (%d)", strerror(errno),
errno);
 			if (errno != EINTR)
@@ -329,6 +346,8 @@
 	DBG("Preparing with io->period_size=%lu io->buffer_size=%lu",
 					io->period_size,
io->buffer_size);
 
+	data->reset = 0;
+
 	if (io->stream == SND_PCM_STREAM_PLAYBACK)
 		/* If not null for playback, xmms doesn't display time
 		 * correctly */
@@ -609,9 +628,11 @@
 	header->timestamp = htonl(a2dp->nsamples);
 	header->ssrc = htonl(1);
 
-	ret = send(data->stream.fd, a2dp->buffer, a2dp->count,
MSG_DONTWAIT);
-	if (ret == -1)
+        ret = send(data->stream.fd, a2dp->buffer, a2dp->count,
MSG_DONTWAIT);
+	if (ret < 0) {
+		DBG("send returned %d errno %s.", ret, strerror(errno));
 		ret = -errno;
+	}
 
 	/* Reset buffer of data to send */
 	a2dp->count = sizeof(struct rtp_header) + sizeof(struct
rtp_payload);
@@ -629,18 +650,19 @@
 	struct bluetooth_data *data = io->private_data;
 	struct bluetooth_a2dp *a2dp = &data->a2dp;
 	snd_pcm_sframes_t ret = 0;
-	snd_pcm_uframes_t frames_to_read;
+	snd_pcm_uframes_t frames_to_read, frames_left = size;
 	int frame_size, encoded;
 	uint8_t *buff;
 
 	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);
+	DBG("hw_ptr=%lu appl_ptr=%lu diff=%lu", io->hw_ptr,
io->appl_ptr, io->appl_ptr - io->hw_ptr);
 
 	if (io->hw_ptr > io->appl_ptr) {
 		ret = bluetooth_playback_stop(io);
 		if (ret == 0)
 			ret = -EPIPE;
+		data->reset = 1;
 		goto done;
 	}
 
@@ -648,69 +670,82 @@
 	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)) {
+		    !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);
 	}
 
-	frame_size = areas->step / 8;
-	if ((data->count + size * frame_size) <= a2dp->codesize)
-		frames_to_read = size;
-	else
-		frames_to_read = (a2dp->codesize - data->count) /
frame_size;
+	while(frames_left > 0) {
 
-	DBG("count=%d frames_to_read=%lu", data->count, frames_to_read);
-	DBG("a2dp.count=%d cfg.pkt_len=%d", a2dp->count,
data->cfg.pkt_len);
+		frame_size = areas->step / 8;
+		if ((data->count + frames_left * frame_size) <=
a2dp->codesize)
+			frames_to_read = frames_left;
+		else
+			frames_to_read = (a2dp->codesize - data->count)
/ frame_size;
+
+		DBG("count=%d frames_to_read=%lu", data->count,
frames_to_read);
+		DBG("a2dp.count=%d cfg.pkt_len=%d", a2dp->count,
data->cfg.pkt_len);
+
+		/* FIXME: If state is not streaming then return */
+
+		/* Ready for more data */
+		buff = (uint8_t *) areas->addr +
+			(areas->first + areas->step * (offset + ret)) /
8;
+		memcpy(data->buffer + data->count, buff, frame_size *
frames_to_read);
+
+		/* Remember we have some frames in the pipe now */
+		data->count += frames_to_read * frame_size;
+		if (data->count != a2dp->codesize) {
+			ret = frames_to_read;
+			goto done;
+		}
 
-	/* FIXME: If state is not streaming then return */
+		/* Enough data to encode (sbc wants 1k blocks) */
+		encoded = sbc_encode(&(a2dp->sbc), data->buffer,
a2dp->codesize);
+		if (encoded <= 0) {
+			DBG("Encoding error %d", encoded);
+			goto done;
+		}
 
-	/* Ready for more data */
-	buff = (uint8_t *) areas->addr +
-				(areas->first + areas->step * offset) /
8;
-	memcpy(data->buffer + data->count, buff, frame_size *
frames_to_read);
+		data->count -= encoded;
 
-	/* Remember we have some frames in the pipe now */
-	data->count += frames_to_read * frame_size;
-	if (data->count != a2dp->codesize) {
-		ret = frames_to_read;
-		goto done;
-	}
+		DBG("encoded=%d  a2dp.sbc.len=%d count=%d", encoded,
a2dp->sbc.len, a2dp->count);
 
-	/* Enough data to encode (sbc wants 1k blocks) */
-	encoded = sbc_encode(&(a2dp->sbc), data->buffer,
a2dp->codesize);
-	if (encoded <= 0) {
-		DBG("Encoding error %d", encoded);
-		goto done;
-	}
+		/* Send previously encoded buffer */
+		if (a2dp->count + a2dp->sbc.len >= data->cfg.pkt_len) {
+#ifdef ENABLE_DEBUG
+			int old_count = a2dp->count;
+			int c =
+#endif
 
-	data->count -= encoded;
+			avdtp_write(data);
+			DBG("sending packet %d, count %d, pkt_len %u",
c, old_count, data->cfg.pkt_len);
+		}
 
-	DBG("encoded=%d  a2dp.sbc.len=%d", encoded, a2dp->sbc.len);
+		memcpy(a2dp->buffer + a2dp->count, a2dp->sbc.data,
a2dp->sbc.len);
+		a2dp->count += a2dp->sbc.len;
+		a2dp->frame_count++;
+		a2dp->samples += encoded / frame_size;
+		a2dp->nsamples += encoded / frame_size;
 
-	if (a2dp->count + a2dp->sbc.len >= data->cfg.pkt_len) {
-		ret = avdtp_write(data);
-		if (ret < 0) {
-			if (-ret == EPIPE)
-				ret = -EIO;
-			goto done;
-		}
-	}
+		/* (io->buffer_size + io->appl_ptr - io->period_size) *
% io->buffer_size */
 
-	memcpy(a2dp->buffer + a2dp->count, a2dp->sbc.data,
a2dp->sbc.len);
-	a2dp->count += a2dp->sbc.len;
-	a2dp->frame_count++;
-	a2dp->samples += encoded / frame_size;
-	a2dp->nsamples += encoded / frame_size;
+		ret += frames_to_read;
+		frames_left -= frames_to_read;
+	}
 
-	ret = frames_to_read;
+	/* note: some ALSA apps will get confused otherwise */
+	if (ret > size)
+		ret = size;
 
 done:
 	DBG("returning %ld", ret);
@@ -847,7 +882,6 @@
 static int bluetooth_a2dp_hw_constraint(snd_pcm_ioplug_t *io)
 {
 	struct bluetooth_data *data = io->private_data;
-	struct bluetooth_a2dp *a2dp = &data->a2dp;
 	struct ipc_data_cfg cfg = data->cfg;
 	snd_pcm_access_t access_list[] = {
 		SND_PCM_ACCESS_RW_INTERLEAVED,
@@ -886,18 +920,25 @@
 	if (err < 0)
 		return err;
 
-	/* 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);
-	if (err < 0)
-		return err;
+	/* supported block sizes: */
+	{ 
+		const unsigned int period_list[] = {
+			512,  /*  3/6ms (mono/stereo 16bit at 44.1kHz)
*/
+			1024, /*  6/12ms */
+			2048, /* 12/23ms */
+			4096, /* 23/46ms */
+			8192, /* 46/93ms */
+		};
+		
+		err = snd_pcm_ioplug_set_param_list(io,
SND_PCM_IOPLUG_HW_PERIOD_BYTES,
+
ARRAY_NELEMS(period_list), period_list);
+		if (err < 0)
+			return err;
+	}
 
+	/* period count fixed to 3 as we don't support prefilling */
 	err = snd_pcm_ioplug_set_param_minmax(io,
SND_PCM_IOPLUG_HW_PERIODS,
-						2, 50);
+					      3, 3);
 	if (err < 0)
 		return err;
--cut--

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

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

[Kai.Vehmanen]

Hi Fabian, Luiz, and others,

On 18 Oct 2007,  Fabien Chevalier wrote:
>>>  - increased max overall buffer size to 16384 bytes (~93msecs
>> 
>> 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. :-(

ok, this indeed affects the buffering constraints we should
expose to the client (and my original buffer-constraints patch
should be reverted).

One drawback of this design is that the timing of packet transmission 
is not totally controlled by the plugin. Instead, whenever the
application
decides to write to the ALSA device, a packet might be sent. If
the overall buffer-size is not constrained, application can send
bursts of data, which will lead to burts of RTP packets sent to 
headset, which will probably confuse it. Of course, if the bursts
stay within the limits of headset's receive buffer, we are ok, but
that's seems somewhat fragile as we don't the buffer size.

To force application _not_ to do this, the plugin can put a strict
limit on buffer size and period count (-> application cannot write
bursts of data to the plugin, but instead it will have to sleep until
the sufficient amount of time has elapsed). But as a consequency, the
timing
requirements become really strict for the application, which will
lead to XRUNs on loaded systems (and POSIX realtime scheduling might
not help as that might mess interactions with the worker and the
application thread).

Plus the following, quite common design in ALSA apps, doesn't 
work as expected:
  - preload X frames of audio to the plugin
  - ... do something else (that takes time)
  - use snd_pcm_start() to trigger playback
... as the headset might start playback already before snd_pcm_start().

But I do agree that it makes sense to avoid adding another
layer of buffering in the plugin.

Hmm, one compromise solution might be to use the existing buffer
setup, but use the worker thread to actually send the packets (at
intervals 
defined by the system timer, not by the application calls to
the plugin). If we set the buffer constraint to match the number
of bytes needed by the SBC encoder per packet, and allow use of
multiple periods, we'd 1) have no duplicated buffering, 2) timing
would be more regular, 3) application would have slightly more
relaxed timing requirements (hopefully leading to less XRUNs).
Comments?

>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. :-(

Yeps, I guess that's precisely the risk with sending bursts of 
packets. OTOH, I guess it's not so clear cut. I guess some headsets 
may require an initial burst before they start playback.

>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 

Aa, ok, thanks, this is good to know.

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

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

[Fabien Chevalier]

Hi Kai,

> Hi Fabian, Luiz, and others,
> 
> On 18 Oct 2007,  Fabien Chevalier wrote:
>>>>  - increased max overall buffer size to 16384 bytes (~93msecs
>>> 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. :-(
> 
> ok, this indeed affects the buffering constraints we should
> expose to the client (and my original buffer-constraints patch
> should be reverted).
> 
> One drawback of this design is that the timing of packet transmission 
> is not totally controlled by the plugin. Instead, whenever the
> application
> decides to write to the ALSA device, a packet might be sent. If
> the overall buffer-size is not constrained, application can send
> bursts of data, which will lead to burts of RTP packets sent to 
> headset, which will probably confuse it. Of course, if the bursts
> stay within the limits of headset's receive buffer, we are ok, but
> that's seems somewhat fragile as we don't the buffer size.

Yes you've spotted the main drawback of this implementation :-)

> 
> To force application _not_ to do this, the plugin can put a strict
> limit on buffer size and period count (-> application cannot write
> bursts of data to the plugin, but instead it will have to sleep until
> the sufficient amount of time has elapsed). But as a consequency, the
> timing
> requirements become really strict for the application, which will
> lead to XRUNs on loaded systems (and POSIX realtime scheduling might
> not help as that might mess interactions with the worker and the
> application thread).

Agree :-)

> 
> Plus the following, quite common design in ALSA apps, doesn't 
> work as expected:
>   - preload X frames of audio to the plugin
>   - ... do something else (that takes time)
>   - use snd_pcm_start() to trigger playback
> ... as the headset might start playback already before snd_pcm_start().
> 
> But I do agree that it makes sense to avoid adding another
> layer of buffering in the plugin.
> 
> Hmm, one compromise solution might be to use the existing buffer
> setup, but use the worker thread to actually send the packets (at
> intervals 
> defined by the system timer, not by the application calls to
> the plugin). If we set the buffer constraint to match the number
> of bytes needed by the SBC encoder per packet, and allow use of
> multiple periods, we'd 1) have no duplicated buffering, 2) timing
> would be more regular, 3) application would have slightly more
> relaxed timing requirements (hopefully leading to less XRUNs).
> Comments?

Well, given the issues you've spotted with our current implementation,
i'd say i'm 100% for it :-) [Though i don't have time to work on it 
myself :-( ]
Btw This is the way Frederic Dalleau made his plugin implementation.



> 
>> 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. :-(
> 
> Yeps, I guess that's precisely the risk with sending bursts of 
> packets. OTOH, I guess it's not so clear cut. I guess some headsets 
> may require an initial burst before they start playback.

Yes sure they do :-)

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