SBC big endian issues?

SBC big endian issues?

[Siarhei Siamashka]

Hello all,

SBC encoder contains the following fragment:

> #if __BYTE_ORDER == __LITTLE_ENDIAN
> 			if (sbc->endian == SBC_BE)
> #elif __BYTE_ORDER == __BIG_ENDIAN
> 			if (sbc->endian == SBC_LE)
> #else
> #error "Unknown byte order"
> #endif
> 				s = (ptr[0] & 0xff) << 8 | (ptr[1] & 0xff);
> 			else
> 				s = (ptr[0] & 0xff) | (ptr[1] & 0xff) << 8;

This does not look right. Because as far as I can see, it will work
differently on big and little endian systems (though I did not test it
on real hardware). Is anybody using SBC on big endian systems?
Does it actually work as expected?

Best regards,
Siarhei Siamashka

Re: SBC big endian issues?

[Brad Midgley]

Siarhei

> SBC encoder contains the following fragment:
>
>> #if __BYTE_ORDER == __LITTLE_ENDIAN
>>                       if (sbc->endian == SBC_BE)
>> #elif __BYTE_ORDER == __BIG_ENDIAN
>>                       if (sbc->endian == SBC_LE)
>> #else
>> #error "Unknown byte order"
>> #endif
>>                               s = (ptr[0] & 0xff) << 8 | (ptr[1] & 0xff);
>>                       else
>>                               s = (ptr[0] & 0xff) | (ptr[1] & 0xff) << 8;
>
> This does not look right. Because as far as I can see, it will work
> differently on big and little endian systems

I tested it some time ago on powerpc and I know Marcel also has a
powerpc for testing. I'll need to update my machine for bluez 4.x.

The logic here seems to be to swap bytes if the endian set in the sbc
struct does not match host endian.

-- 
Brad Midgley

Re: SBC big endian issues?

[Siarhei Siamashka]

On Monday 05 January 2009 17:36:57 ext Brad Midgley wrote:
> Siarhei
>
> > SBC encoder contains the following fragment:
> >> #if __BYTE_ORDER == __LITTLE_ENDIAN
> >>                       if (sbc->endian == SBC_BE)
> >> #elif __BYTE_ORDER == __BIG_ENDIAN
> >>                       if (sbc->endian == SBC_LE)
> >> #else
> >> #error "Unknown byte order"
> >> #endif
> >>                               s = (ptr[0] & 0xff) << 8 | (ptr[1] &
> >> 0xff); else
> >>                               s = (ptr[0] & 0xff) | (ptr[1] & 0xff) <<
> >> 8;
> >
> > This does not look right. Because as far as I can see, it will work
> > differently on big and little endian systems
>
> I tested it some time ago on powerpc and I know Marcel also has a
> powerpc for testing. I'll need to update my machine for bluez 4.x.
>
> The logic here seems to be to swap bytes if the endian set in the sbc
> struct does not match host endian.

The logic is that the first line contains a portable endian neutral read of
big endian data into native format:
>                               s = (ptr[0] & 0xff) << 8 | (ptr[1] & 0xff);
The second line contains a portable endian neutral read of little endian
data into native format:
>                               s = (ptr[0] & 0xff) | (ptr[1] & 0xff) <<8;

If __BYTE_ORDER is defined as __LITTLE_ENDIAN, the first line is used when
sbc->endian == SBC_BE. But if __BYTE_ORDER is set to __BIG_ENDIAN, the
logic is reversed.

The behaviour and the end result seems to be different for the big endian
and little endian systems. This can be probably easily tested by encoding the
same file by sbcenc on big and little endian systems and comparing results.

-- 
Best regards,
Siarhei Siamashka

Re: SBC big endian issues?

[Brad Midgley]

Siarhei

> The logic is that the first line contains a portable endian neutral read of
> big endian data into native format:
>>                               s = (ptr[0] & 0xff) << 8 | (ptr[1] & 0xff);

the intent is to swap bytes using this first statement if either

- host order is little endian and the ptr array is stored big endian
- host order is big endian and the ptr array is stored little endian

the second case could be done with a cast to a 16-bit int rather than
bit arithmetic.

We don't exercise all the various cases.

-- 
Brad Midgley

Re: SBC big endian issues?

[Brad Midgley]

Siarhei

and, yes, it does look fishy to me too.

Brad

Re: SBC big endian issues?

[Siarhei Siamashka]

On Monday 05 January 2009 18:25:24 ext Brad Midgley wrote:
> Siarhei
>
> > The logic is that the first line contains a portable endian neutral read
> > of
> >
> > big endian data into native format:
> >>                               s = (ptr[0] & 0xff) << 8 | (ptr[1] &
> >> 0xff);
>
> the intent is to swap bytes using this first statement if either
>
> - host order is little endian and the ptr array is stored big endian
> - host order is big endian and the ptr array is stored little endian
>
> the second case could be done with a cast to a 16-bit int rather than
> bit arithmetic.
>
> We don't exercise all the various cases.

No, it's a bit different. There is no 'swap byte' operation here, but just
portable reading of data in big endian format.

Let's suppose that we have the following two bytes in memory:

0x12 0x34

This equals to 0x1234 for big endian systems or 0x3412 for little endian
systems if you read data via int16_t * pointer.

But in the case of SBC, data is read one byte at a time and results in the
same value regardless of the endianness of the system. It will be always
0x1234 and this is generally a portable, but slow way of reading big endian
data (host system endian order does not matter).

So unless __BYTE_ORDER is somehow defined to be __LITTLE_ENDIAN on big
endian systems (two bugs which cancel each other ;-) ),  the code from SBC is
supposed to work incorrectly and sbcenc should produce broken files
with distorted sound (though somewhat resembling the original file). I just
want somebody to verify this on real big endian hardware.

PS. I'm interested in this code because it is very slow and needs to be
optimized. In the case of single channel and no endian conversion, just
memcpy can be used. Other cases can be improved too.

-- 
Best regards,
Siarhei Siamashka

Re: SBC big endian issues?

[Brad Midgley]

Siarhei

The copy is inefficient but it would be even better if we didn't have
to do it at all. I was investigating zero copy here and came up with a
patch but it was too complicated to be accepted.

The messy part here is we let the caller specify the byte order of the
array. It would simplify a lot to standardize on host endian. I don't
remember what the reasoning was against this.

> Let's suppose that we have the following two bytes in memory:
>
> 0x12 0x34
>
> This equals to 0x1234 for big endian systems or 0x3412 for little endian
> systems if you read data via int16_t * pointer.

if sbc->endian is set to the same as the host endian, then this could
be done with a memcopy or skipped (zero copy).

I believe sbc->endian is always set to little endian the way it works
now, meaning the array is storing data little endian regardless of the
architecture. The patch I wrote made a copy of the memory, swapping
bytes, if sbc->endian didn't match host endian. The way we use the
code, this swapping only happens on big endian machines.

-- 
Brad Midgley

Re: SBC big endian issues?

[Siarhei Siamashka]

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

On Monday 05 January 2009 21:26:17 ext Brad Midgley wrote:
> Siarhei
>
> The copy is inefficient but it would be even better if we didn't have
> to do it at all. I was investigating zero copy here and came up with a
> patch but it was too complicated to be accepted.

Yes, it is possible to reduce the number of data copies. Having
both 'sbc_encoder_state->X' and 'sbc_frame->pcm_sample' arrays
is obviously redundant and only one of them should remain.

But eliminating any kind of data copies completely is hardly possible. The
encoder needs to always have data for the 72 or 36 previous samples, so
they need to be stored somewhere between the calls to frame encode
function. And frame encode function will probably get data in small chunks
if having low latency is desired. So at least this part of data will need to
be moved around. Additionally, SIMD optimizations require input data
permutation, so they can't work directly with the input buffer.

Preserving the input samples 'history' is currently achieved by having 
'sbc_encoder_state->X' array which works as some kind of ring buffer.
If this buffer had infinite size, we would not have to duplicate information
in the lower and higher halves of this buffer. But this is of course not
possible due to the need to keep memory use reasonable and also in order to
efficiently use data cache. Anyway, increasing 'sbc_encoder_state->X' size
to some reasonable value may help to reduce extra overhead. One of the
variants is to have space in X buffer for all the input data of a frame plus
these 72/36 samples from the previous frame, copy the previous 72/36 samples
to it, and then perform "endian conversion + channels deinterleaving + do SIMD
permutation" in one pass directly into the X buffer from the buffer provided
by the user at the entry of the frame packing function. Increasing X buffer
more may allow to copy the previous 72/36 samples only once per 2 frames, once
per 3 frames or whatever.

This is not complicated at all, but is indeed a bit intrusive in the sense
that it will touch quite a large part of code. But we are ready to do it now,
am I right?

> The messy part here is we let the caller specify the byte order of the
> array. It would simplify a lot to standardize on host endian. I don't
> remember what the reasoning was against this.
>
> > Let's suppose that we have the following two bytes in memory:
> >
> > 0x12 0x34
> >
> > This equals to 0x1234 for big endian systems or 0x3412 for little endian
> > systems if you read data via int16_t * pointer.
>
> if sbc->endian is set to the same as the host endian, then this could
> be done with a memcopy or skipped (zero copy).
>
> I believe sbc->endian is always set to little endian the way it works
> now, meaning the array is storing data little endian regardless of the
> architecture. The patch I wrote made a copy of the memory, swapping
> bytes, if sbc->endian didn't match host endian. The way we use the
> code, this swapping only happens on big endian machines.

Well, having no other option to verify this big endian bug, I used this MIPS
big endian QEMU image for testing: 
http://people.debian.org/~aurel32/qemu/mips/

The current code is indeed broken on big endian systems. The attached
patch makes it work correct. Of course this part needs heavy performance
optimizations (as discussed above), but even just fixing it may be a good
idea at the moment.

There is one more suspicious part of code which may cause problems:
> 	unsigned char input[2048], output[2048];
> ...
> 	au_hdr = (struct au_header *) input;
> 	if (au_hdr->magic != AU_MAGIC ||
> 			BE_INT(au_hdr->hdr_size) > 128 ||
> 			BE_INT(au_hdr->hdr_size) < 24 ||
> 			BE_INT(au_hdr->encoding) != AU_FMT_LIN16) {
> 		fprintf(stderr, "Not in Sun/NeXT audio S16_BE format\n");
> 		goto done;
> 	}

As 'input' array is only guaranteed to have byte alignment and the code later
accesses 32-bit au_hdr data, it may cause problems and the compiler
rightfully issues a warning about it.


Best regards,
Siarhei Siamashka

[-- Attachment #2: 0001-Fix-for-big-endian-problems-in-SBC-codec.patch --]
[-- Type: text/x-diff, Size: 1336 bytes --]

>From 0ee38e8976bd728e16fa4523f53d7b8400754294 Mon Sep 17 00:00:00 2001
From: Siarhei Siamashka <siarhei.siamashka@nokia.com>
Date: Wed, 7 Jan 2009 14:28:48 +0200
Subject: [PATCH] Fix for big endian problems in SBC codec

---
 sbc/sbc.c |   12 ------------
 1 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/sbc/sbc.c b/sbc/sbc.c
index 8fff277..651981f 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -1157,13 +1157,7 @@ int sbc_decode(sbc_t *sbc, void *input, int input_len, void *output,
 			int16_t s;
 			s = priv->frame.pcm_sample[ch][i];
 
-#if __BYTE_ORDER == __LITTLE_ENDIAN
 			if (sbc->endian == SBC_BE) {
-#elif __BYTE_ORDER == __BIG_ENDIAN
-			if (sbc->endian == SBC_LE) {
-#else
-#error "Unknown byte order"
-#endif
 				*ptr++ = (s & 0xff00) >> 8;
 				*ptr++ = (s & 0x00ff);
 			} else {
@@ -1224,13 +1218,7 @@ int sbc_encode(sbc_t *sbc, void *input, int input_len, void *output,
 	for (i = 0; i < priv->frame.subbands * priv->frame.blocks; i++) {
 		for (ch = 0; ch < priv->frame.channels; ch++) {
 			int16_t s;
-#if __BYTE_ORDER == __LITTLE_ENDIAN
 			if (sbc->endian == SBC_BE)
-#elif __BYTE_ORDER == __BIG_ENDIAN
-			if (sbc->endian == SBC_LE)
-#else
-#error "Unknown byte order"
-#endif
 				s = (ptr[0] & 0xff) << 8 | (ptr[1] & 0xff);
 			else
 				s = (ptr[0] & 0xff) | (ptr[1] & 0xff) << 8;
-- 
1.5.6.5

Re: SBC big endian issues?

[Marcel Holtmann]

Hi Siarhei,

> > The messy part here is we let the caller specify the byte order of the
> > array. It would simplify a lot to standardize on host endian. I don't
> > remember what the reasoning was against this.
> >
> > > Let's suppose that we have the following two bytes in memory:
> > >
> > > 0x12 0x34
> > >
> > > This equals to 0x1234 for big endian systems or 0x3412 for little endian
> > > systems if you read data via int16_t * pointer.
> >
> > if sbc->endian is set to the same as the host endian, then this could
> > be done with a memcopy or skipped (zero copy).
> >
> > I believe sbc->endian is always set to little endian the way it works
> > now, meaning the array is storing data little endian regardless of the
> > architecture. The patch I wrote made a copy of the memory, swapping
> > bytes, if sbc->endian didn't match host endian. The way we use the
> > code, this swapping only happens on big endian machines.
> 
> Well, having no other option to verify this big endian bug, I used this MIPS
> big endian QEMU image for testing: 
> http://people.debian.org/~aurel32/qemu/mips/
> 
> The current code is indeed broken on big endian systems. The attached
> patch makes it work correct. Of course this part needs heavy performance
> optimizations (as discussed above), but even just fixing it may be a good
> idea at the moment.

patch has been applied. Thanks.

Regards

Marcel

Re: SBC big endian issues?

[Siarhei Siamashka]

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

On Wednesday 07 January 2009 15:43:57 ext Marcel Holtmann wrote:
> Hi Siarhei,
>
> > > The messy part here is we let the caller specify the byte order of the
> > > array. It would simplify a lot to standardize on host endian. I don't
> > > remember what the reasoning was against this.
> > >
> > > > Let's suppose that we have the following two bytes in memory:
> > > >
> > > > 0x12 0x34
> > > >
> > > > This equals to 0x1234 for big endian systems or 0x3412 for little
> > > > endian systems if you read data via int16_t * pointer.
> > >
> > > if sbc->endian is set to the same as the host endian, then this could
> > > be done with a memcopy or skipped (zero copy).
> > >
> > > I believe sbc->endian is always set to little endian the way it works
> > > now, meaning the array is storing data little endian regardless of the
> > > architecture. The patch I wrote made a copy of the memory, swapping
> > > bytes, if sbc->endian didn't match host endian. The way we use the
> > > code, this swapping only happens on big endian machines.
> >
> > Well, having no other option to verify this big endian bug, I used this
> > MIPS big endian QEMU image for testing:
> > http://people.debian.org/~aurel32/qemu/mips/
> >
> > The current code is indeed broken on big endian systems. The attached
> > patch makes it work correct. Of course this part needs heavy performance
> > optimizations (as discussed above), but even just fixing it may be a good
> > idea at the moment.
>
> patch has been applied. Thanks.

Thanks, though seems like there are some problems.

By default, SBC codec uses native byte order now (configured
by 'sbc_set_defaults' function, which is called from 'sbc_init').

But SBC gstreamer elements ('gstsbcdec.c', 'gstsbcenc.c') specify
endianness as little endian: "endianness = (int) LITTLE_ENDIAN, ".
So now after the patch got applied, "sbcenc/sbcdec" utilites got fixed,
but gstreamer elements may have problems on big endian systems
instead. Another concern is about 'pcm_bluetooth.c'. It does not specifically
configure endianness in any way, so SBC codec is used with the native byte
order as well. And there the supported format is also specified as
SND_PCM_FORMAT_S16_LE.

Looks like this was the case of double errors which were cancelling each other
or misleading constant names. In any case, SBC has been always treating data
by default as little endian before regardless of the host native byte order.

So in order to get gstreamer and pcm_bluetooth working right again, the
default configuration of SBC codec can be changed to SBC_LE (this way all the
old clients which did not change the default configuration will continue to
work just like before). A variant of patch with this kind of solution is
attached.

Alternatively, gstreamer elements and ALSA plugin could try to use native byte
order (and this can be probably good for the performance as with the strictly
little endian support for the data, big endian systems may actually have to do
the conversion twice - first in the gstreamer in order to provide the data to
the element in little endian format, and then in SBC codec itself to convert
it back to native format to use it in number crunching). This variant of patch
is also attached (but untested because I don't have a big endian system).


Some kind of fix needs to be used (and the attached patches are mutually
exclusive), as apparently 4.26 release does not work right on big endian
systems :(


Best regards,
Siarhei Siamashka

[-- Attachment #2: 0001-Change-of-SBC-default-configutation-to-little-endian.patch --]
[-- Type: text/x-diff, Size: 1056 bytes --]

>From 4a4669b0e6cf75e4c36802b8e90b3c369804aaf0 Mon Sep 17 00:00:00 2001
From: Siarhei Siamashka <siarhei.siamashka@nokia.com>
Date: Fri, 16 Jan 2009 18:54:31 +0200
Subject: [PATCH] Change of SBC default configutation to little endian

ALSA plugin and gstreamer elements assume little endian byte order
for the audio data. The problem is that they also rely on the
default SBC configuration to be "right", which is not what they
expect after commit 8bbfdf782dd1633a1f78a26584ff81b858df4a61
---
 sbc/sbc.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/sbc/sbc.c b/sbc/sbc.c
index 0699ae0..4da130a 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -927,13 +927,7 @@ static void sbc_set_defaults(sbc_t *sbc, unsigned long flags)
 	sbc->subbands = SBC_SB_8;
 	sbc->blocks = SBC_BLK_16;
 	sbc->bitpool = 32;
-#if __BYTE_ORDER == __LITTLE_ENDIAN
 	sbc->endian = SBC_LE;
-#elif __BYTE_ORDER == __BIG_ENDIAN
-	sbc->endian = SBC_BE;
-#else
-#error "Unknown byte order"
-#endif
 }
 
 int sbc_init(sbc_t *sbc, unsigned long flags)
-- 
1.5.6.5


[-- Attachment #3: 0001-Use-advertise-native-byte-order-for-audio-in-gstream.patch --]
[-- Type: text/x-diff, Size: 2538 bytes --]

>From 1132592f98a919f22534657fef63a06880472293 Mon Sep 17 00:00:00 2001
From: Siarhei Siamashka <siarhei.siamashka@nokia.com>
Date: Fri, 16 Jan 2009 19:15:34 +0200
Subject: [PATCH] Use/advertise native byte order for audio in gstreamer and ALSA plugins

---
 audio/gstsbcdec.c     |    6 ++++++
 audio/gstsbcenc.c     |    6 ++++++
 audio/pcm_bluetooth.c |   12 ++++++++++++
 3 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/audio/gstsbcdec.c b/audio/gstsbcdec.c
index fedc129..35a3297 100644
--- a/audio/gstsbcdec.c
+++ b/audio/gstsbcdec.c
@@ -50,7 +50,13 @@ static GstStaticPadTemplate sbc_dec_src_factory =
 		GST_STATIC_CAPS("audio/x-raw-int, "
 				"rate = (int) { 16000, 32000, 44100, 48000 }, "
 				"channels = (int) [ 1, 2 ], "
+#if __BYTE_ORDER == __LITTLE_ENDIAN
 				"endianness = (int) LITTLE_ENDIAN, "
+#elif __BYTE_ORDER == __BIG_ENDIAN
+				"endianness = (int) BIG_ENDIAN, "
+#else
+#error "Unknown byte order"
+#endif				
 				"signed = (boolean) true, "
 				"width = (int) 16, "
 				"depth = (int) 16"));
diff --git a/audio/gstsbcenc.c b/audio/gstsbcenc.c
index 3ecaacf..f083a9b 100644
--- a/audio/gstsbcenc.c
+++ b/audio/gstsbcenc.c
@@ -147,7 +147,13 @@ static GstStaticPadTemplate sbc_enc_sink_factory =
 		GST_STATIC_CAPS("audio/x-raw-int, "
 				"rate = (int) { 16000, 32000, 44100, 48000 }, "
 				"channels = (int) [ 1, 2 ], "
+#if __BYTE_ORDER == __LITTLE_ENDIAN
 				"endianness = (int) LITTLE_ENDIAN, "
+#elif __BYTE_ORDER == __BIG_ENDIAN
+				"endianness = (int) BIG_ENDIAN, "
+#else
+#error "Unknown byte order"
+#endif
 				"signed = (boolean) true, "
 				"width = (int) 16, "
 				"depth = (int) 16"));
diff --git a/audio/pcm_bluetooth.c b/audio/pcm_bluetooth.c
index bf24206..43b648e 100644
--- a/audio/pcm_bluetooth.c
+++ b/audio/pcm_bluetooth.c
@@ -1182,7 +1182,13 @@ static int bluetooth_hsp_hw_constraint(snd_pcm_ioplug_t *io)
 		SND_PCM_ACCESS_MMAP_INTERLEAVED
 	};
 	unsigned int format_list[] = {
+#if __BYTE_ORDER == __LITTLE_ENDIAN
 		SND_PCM_FORMAT_S16_LE
+#elif __BYTE_ORDER == __BIG_ENDIAN
+		SND_PCM_FORMAT_S16_BE
+#else
+#error "Unknown byte order"
+#endif
 	};
 	int err;
 
@@ -1237,7 +1243,13 @@ static int bluetooth_a2dp_hw_constraint(snd_pcm_ioplug_t *io)
 		SND_PCM_ACCESS_MMAP_INTERLEAVED
 	};
 	unsigned int format_list[] = {
+#if __BYTE_ORDER == __LITTLE_ENDIAN
 		SND_PCM_FORMAT_S16_LE
+#elif __BYTE_ORDER == __BIG_ENDIAN
+		SND_PCM_FORMAT_S16_BE
+#else
+#error "Unknown byte order"
+#endif
 	};
 	unsigned int rate_list[4];
 	unsigned int rate_count;
-- 
1.5.6.5

Re: SBC big endian issues?

[Luiz Augusto von Dentz]

Hi Siarhei,

It seems gstreamer does have G_BYTE_ORDER, so you probably don't need
those #ifdef. I also notice some unnecessary white spaces in the of a
line.

-- 
Luiz Augusto von Dentz
Engenheiro de Computação

Re: SBC big endian issues?

[Siarhei Siamashka]

On Saturday 17 January 2009 00:02:09 ext Luiz Augusto von Dentz wrote:
> Hi Siarhei,
>
> It seems gstreamer does have G_BYTE_ORDER, so you probably don't need
> those #ifdef. I also notice some unnecessary white spaces in the of a
> line.

Hi,

I did not verify that patch thoroughly (I only checked that it compiles) and
also seems like I somehow forgot to pass it through 'checkpatch.pl'. Thanks
for a good catch.

Regarding G_BYTE_ORDER, you are probably right.  I'm not very familiar with
gstreamer yet, so somebody else could probably fix this stuff much faster
than me.

In any case, it probably makes sense to relay all the endian conversion
related things to gstreamer and ALSA, and use only native byte order in SBC.
This will simplify the code a bit and will make optimizing it easier.

I'm quite worried about big endian systems, but I have plans to buy a PS3 home
in about a month timeframe unless something extraordinary happens  :)


Best regards,
Siarhei Siamashka

Re: SBC big endian issues?

[Siarhei Siamashka]

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

On Saturday 17 January 2009 20:10:04 ext Siarhei Siamashka wrote:
> On Saturday 17 January 2009 00:02:09 ext Luiz Augusto von Dentz wrote:
> > Hi Siarhei,
> >
> > It seems gstreamer does have G_BYTE_ORDER, so you probably don't need
> > those #ifdef. I also notice some unnecessary white spaces in the of a
> > line.
>
> Hi,
>
> I did not verify that patch thoroughly (I only checked that it compiles)
> and also seems like I somehow forgot to pass it through 'checkpatch.pl'.
> Thanks for a good catch.
>
> Regarding G_BYTE_ORDER, you are probably right.  I'm not very familiar with
> gstreamer yet, so somebody else could probably fix this stuff much faster
> than me.
>
> In any case, it probably makes sense to relay all the endian conversion
> related things to gstreamer and ALSA, and use only native byte order in
> SBC. This will simplify the code a bit and will make optimizing it easier.
>
> I'm quite worried about big endian systems, but I have plans to buy a PS3
> home in about a month timeframe unless something extraordinary happens  :)

OK, here is an updated patch.

Now I also tested GStreamer plugin on qemu mips big endian image. It works
fine with this change. Having native byte order and not strictly little endian
is a good idea because all the other gstreamer elements (vorbisdec,
flacdec, ...) produce output in native byte order. Unless sbcenc uses native
byte order on this big endian system, audioconvert element is needed to be
added to gstreamer pipeline and it most likely introduces some extra overhead.

I did not check ALSA part and probably can't even do this with qemu, but
removing _LE suffix should also do the job there (according to ALSA
documentation).

Also an interesting observation is that gstreamer is very slow. Using
gst-launch to encode audio file to SBC is something like 1.5 times slower
than sbcenc utility when benchmarked on x86 system. I wonder if
something could be done about this.


Best regards,
Siarhei Siamashka

[-- Attachment #2: 0001-Use-native-byte-order-for-audio-in-GStreamer-and-ALS.patch --]
[-- Type: text/x-diff, Size: 2119 bytes --]

>From f9afd1641cb22ed93b9f0d0992e9a78810ff8159 Mon Sep 17 00:00:00 2001
From: Siarhei Siamashka <siarhei.siamashka@gmail.com>
Date: Mon, 19 Jan 2009 10:26:28 +0200
Subject: [PATCH] Use native byte order for audio in GStreamer and ALSA plugins

This fixes endianness inconsistency between default SBC
configuration and GStreamer/ALSA.
---
 audio/gstsbcdec.c     |    2 +-
 audio/gstsbcenc.c     |    2 +-
 audio/pcm_bluetooth.c |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/audio/gstsbcdec.c b/audio/gstsbcdec.c
index fedc129..00af4fc 100644
--- a/audio/gstsbcdec.c
+++ b/audio/gstsbcdec.c
@@ -50,7 +50,7 @@ static GstStaticPadTemplate sbc_dec_src_factory =
 		GST_STATIC_CAPS("audio/x-raw-int, "
 				"rate = (int) { 16000, 32000, 44100, 48000 }, "
 				"channels = (int) [ 1, 2 ], "
-				"endianness = (int) LITTLE_ENDIAN, "
+				"endianness = (int) BYTE_ORDER, "
 				"signed = (boolean) true, "
 				"width = (int) 16, "
 				"depth = (int) 16"));
diff --git a/audio/gstsbcenc.c b/audio/gstsbcenc.c
index 3ecaacf..789bf78 100644
--- a/audio/gstsbcenc.c
+++ b/audio/gstsbcenc.c
@@ -147,7 +147,7 @@ static GstStaticPadTemplate sbc_enc_sink_factory =
 		GST_STATIC_CAPS("audio/x-raw-int, "
 				"rate = (int) { 16000, 32000, 44100, 48000 }, "
 				"channels = (int) [ 1, 2 ], "
-				"endianness = (int) LITTLE_ENDIAN, "
+				"endianness = (int) BYTE_ORDER, "
 				"signed = (boolean) true, "
 				"width = (int) 16, "
 				"depth = (int) 16"));
diff --git a/audio/pcm_bluetooth.c b/audio/pcm_bluetooth.c
index bf24206..5dfd778 100644
--- a/audio/pcm_bluetooth.c
+++ b/audio/pcm_bluetooth.c
@@ -1182,7 +1182,7 @@ static int bluetooth_hsp_hw_constraint(snd_pcm_ioplug_t *io)
 		SND_PCM_ACCESS_MMAP_INTERLEAVED
 	};
 	unsigned int format_list[] = {
-		SND_PCM_FORMAT_S16_LE
+		SND_PCM_FORMAT_S16
 	};
 	int err;
 
@@ -1237,7 +1237,7 @@ static int bluetooth_a2dp_hw_constraint(snd_pcm_ioplug_t *io)
 		SND_PCM_ACCESS_MMAP_INTERLEAVED
 	};
 	unsigned int format_list[] = {
-		SND_PCM_FORMAT_S16_LE
+		SND_PCM_FORMAT_S16
 	};
 	unsigned int rate_list[4];
 	unsigned int rate_count;
-- 
1.5.6.5

Re: SBC big endian issues?

[Marcel Holtmann]

Hi Siarhei,

> > I did not verify that patch thoroughly (I only checked that it compiles)
> > and also seems like I somehow forgot to pass it through 'checkpatch.pl'.
> > Thanks for a good catch.
> >
> > Regarding G_BYTE_ORDER, you are probably right.  I'm not very familiar with
> > gstreamer yet, so somebody else could probably fix this stuff much faster
> > than me.
> >
> > In any case, it probably makes sense to relay all the endian conversion
> > related things to gstreamer and ALSA, and use only native byte order in
> > SBC. This will simplify the code a bit and will make optimizing it easier.
> >
> > I'm quite worried about big endian systems, but I have plans to buy a PS3
> > home in about a month timeframe unless something extraordinary happens  :)
> 
> OK, here is an updated patch.
> 
> Now I also tested GStreamer plugin on qemu mips big endian image. It works
> fine with this change. Having native byte order and not strictly little endian
> is a good idea because all the other gstreamer elements (vorbisdec,
> flacdec, ...) produce output in native byte order. Unless sbcenc uses native
> byte order on this big endian system, audioconvert element is needed to be
> added to gstreamer pipeline and it most likely introduces some extra overhead.
> 
> I did not check ALSA part and probably can't even do this with qemu, but
> removing _LE suffix should also do the job there (according to ALSA
> documentation).

patch has been applied. Thanks.

> Also an interesting observation is that gstreamer is very slow. Using
> gst-launch to encode audio file to SBC is something like 1.5 times slower
> than sbcenc utility when benchmarked on x86 system. I wonder if
> something could be done about this.

I suspected GStreamer to be slower because of its GObject overhead, but
1.5 times is a lot. Not sure if GStreamer or GObject is to blame here.

Regards

Marcel

[Patch] SAP client plugin framework

[Liu, Raymond]

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

Holtmann, Johan

	The attach patch is for the SAP DBUS API doc and the initial framework code for SAP plugin.

	Could you help to have a review.	Thanks!

Best Regards,
Raymond Liu

[-- Attachment #2: 0001-Add-sap-api.txt-for-SAP-Client.patch --]
[-- Type: application/octet-stream, Size: 2143 bytes --]

From e43b988a05e8b5a755c55c889b7a15f1fc5c79c7 Mon Sep 17 00:00:00 2001
From: Raymond Liu <raymond.liu@intel.com>
Date: Fri, 16 Jan 2009 15:59:01 +0800
Subject: [PATCH] Add sap-api.txt for SAP Client

---
 doc/sap-api.txt |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 69 insertions(+), 0 deletions(-)
 create mode 100644 doc/sap-api.txt

diff --git a/doc/sap-api.txt b/doc/sap-api.txt
new file mode 100644
index 0000000..00eb28a
--- /dev/null
+++ b/doc/sap-api.txt
@@ -0,0 +1,69 @@
+BlueZ D-Bus SAP API description
+*********************************
+
+Copyright (C) 2008-2009 Intel Corporation
+
+
+SIM Access Profile Client hierarchy
+===============
+
+Service		org.bluez
+Interface	org.bluez.SimAccess
+Object path	[variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
+
+Methods		void Connect()
+
+			Connect to the SAP service on the remote device.
+
+		void Disconnect()
+
+			Disconnect from the SAP service on the remote device.
+
+		array{uint8} SendAPDU(array{uint8})
+
+			Send Command-APDU to the remote device.
+			The return array contains Response-APDU
+
+		void PowerOff()
+
+			Power Off the remote subscription module.
+
+		void PowerOn()
+
+			Power On the remote subscription module.
+
+		void Reset()
+
+			Reset the remote subscription module.
+
+		void SetProtocol(uint8 T)
+
+			Select the transport protocol with the remote subscription module.
+			Only T = 0, T = 1 is supported by the current spec.
+
+		dict GetProperties()
+
+			Returns all properties for the interface. See the
+			properties section for available properties.
+
+Signals		PropertyChanged(string name, variant value)
+
+			This signal indicates a changed value of the given
+			property.
+
+Properties	boolean Connected [readonly]
+
+			Indicates if there is an active connection on SAP service
+			with the remote device.
+
+		boolean Powered [readonly]
+
+			Indicates whether the remote subscription module is powered.
+
+		uint8 ModuleStatus [readonly]
+
+			Indicates the status of the subscription module.
+
+		uint8 CardReaderStatus [readonly]
+
+			Indicates the status of the CardReader.
-- 
1.5.6.3


[-- Attachment #3: 0002-Add-framework-code-for-SAP.patch --]
[-- Type: application/octet-stream, Size: 16555 bytes --]

From a518536785ff3f96234ce8bfdfe002d6eee330d5 Mon Sep 17 00:00:00 2001
From: Raymond Liu <raymond.liu@intel.com>
Date: Tue, 20 Jan 2009 13:47:12 +0800
Subject: [PATCH] Add framework code for SAP

---
 Makefile.am         |    2 +-
 acinclude.m4        |    6 ++
 configure.ac        |    1 +
 plugins/Makefile.am |    2 +
 sap/Makefile.am     |   21 +++++
 sap/device.c        |  219 +++++++++++++++++++++++++++++++++++++++++++++++++++
 sap/device.h        |   28 +++++++
 sap/main.c          |   59 ++++++++++++++
 sap/manager.c       |  130 ++++++++++++++++++++++++++++++
 sap/manager.h       |   26 ++++++
 10 files changed, 493 insertions(+), 1 deletions(-)
 create mode 100644 sap/Makefile.am
 create mode 100644 sap/device.c
 create mode 100644 sap/device.h
 create mode 100644 sap/main.c
 create mode 100644 sap/manager.c
 create mode 100644 sap/manager.h

diff --git a/Makefile.am b/Makefile.am
index f2d1ca7..c6899d2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,6 +1,6 @@
 
 SUBDIRS = include lib sbc gdbus common src client\
-	plugins network serial input audio \
+	plugins network serial input audio sap \
 	tools rfcomm compat cups test scripts doc
 
 EXTRA_DIST = bluez.m4
diff --git a/acinclude.m4 b/acinclude.m4
index 372833e..704c9ee 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -186,6 +186,7 @@ AC_DEFUN([AC_ARG_BLUEZ], [
 	input_enable=yes
 	serial_enable=yes
 	network_enable=yes
+	sap_enable=yes
 	service_enable=yes
 	tools_enable=yes
 	hidd_enable=no
@@ -226,6 +227,10 @@ AC_DEFUN([AC_ARG_BLUEZ], [
 		audio_enable=${enableval}
 	])
 
+	AC_ARG_ENABLE(sap, AC_HELP_STRING([--disable-sap], [disable sap plugin]), [
+		sap_enable=${enableval}
+	])
+
 	AC_ARG_ENABLE(service, AC_HELP_STRING([--disable-service], [disable service plugin]), [
 		service_enable=${enableval}
 	])
@@ -344,6 +349,7 @@ AC_DEFUN([AC_ARG_BLUEZ], [
 	AM_CONDITIONAL(INPUTPLUGIN, test "${input_enable}" = "yes")
 	AM_CONDITIONAL(SERIALPLUGIN, test "${serial_enable}" = "yes")
 	AM_CONDITIONAL(NETWORKPLUGIN, test "${network_enable}" = "yes")
+	AM_CONDITIONAL(SAPPLUGIN, test "${sap_enable}" = "yes")
 	AM_CONDITIONAL(SERVICEPLUGIN, test "${service_enable}" = "yes")
 	AM_CONDITIONAL(HIDD, test "${hidd_enable}" = "yes")
 	AM_CONDITIONAL(PAND, test "${pand_enable}" = "yes")
diff --git a/configure.ac b/configure.ac
index 84a6234..bc7cf6b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -55,6 +55,7 @@ AC_OUTPUT([
 	serial/Makefile
 	input/Makefile
 	audio/Makefile
+	sap/Makefile
 	tools/Makefile
 	rfcomm/Makefile
 	compat/Makefile
diff --git a/plugins/Makefile.am b/plugins/Makefile.am
index 3b258a3..7ad76fe 100644
--- a/plugins/Makefile.am
+++ b/plugins/Makefile.am
@@ -46,6 +46,7 @@ all-local:
 	@$(LN_S) -f $(top_srcdir)/audio/.libs/audio.so
 	@$(LN_S) -f $(top_srcdir)/serial/.libs/serial.so
 	@$(LN_S) -f $(top_srcdir)/network/.libs/network.so
+	@$(LN_S) -f $(top_srcdir)/sap/.libs/sap.so
 	@$(LN_S) -f .libs/service.so
 	@$(LN_S) -f .libs/hal.so
 
@@ -56,3 +57,4 @@ clean-local:
 	@rm -f serial.so
 	@rm -f audio.so
 	@rm -f input.so
+	@rm -f sap.so
diff --git a/sap/Makefile.am b/sap/Makefile.am
new file mode 100644
index 0000000..aad95f3
--- /dev/null
+++ b/sap/Makefile.am
@@ -0,0 +1,21 @@
+
+if SAPPLUGIN
+plugindir = $(libdir)/bluetooth/plugins
+
+plugin_LTLIBRARIES = sap.la
+
+sap_la_SOURCES = main.c manager.h manager.c \
+		device.h device.c
+
+LDADD = $(top_builddir)/common/libhelper.a \
+		@GDBUS_LIBS@ @GLIB_LIBS@ @DBUS_LIBS@ @BLUEZ_LIBS@
+endif
+
+AM_LDFLAGS = -module -avoid-version -no-undefined \
+			-export-symbols-regex bluetooth_plugin_desc
+
+AM_CFLAGS = @BLUEZ_CFLAGS@ @DBUS_CFLAGS@ @GLIB_CFLAGS@ @GDBUS_CFLAGS@
+
+INCLUDES = -I$(top_srcdir)/common -I$(top_srcdir)/src
+
+MAINTAINERCLEANFILES = Makefile.in
diff --git a/sap/device.c b/sap/device.c
new file mode 100644
index 0000000..b3c4a0a
--- /dev/null
+++ b/sap/device.c
@@ -0,0 +1,219 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2009 Intel Corporation
+ *  Copyright (C) 2004-2009  Marcel Holtmann <marcel@holtmann.org>
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+
+#include <bluetooth/bluetooth.h>
+#include <bluetooth/hci.h>
+#include <bluetooth/sdp.h>
+#include <bluetooth/sdp_lib.h>
+
+#include <glib.h>
+#include <gdbus.h>
+
+#include "../src/dbus-common.h"
+
+#include "logging.h"
+#include "glib-helper.h"
+
+#include "device.h"
+
+#define SAP_CLIENT_INTERFACE	"org.bluez.SimAccess"
+
+struct sap_device {
+	DBusConnection	*conn;
+	bdaddr_t	src;		/* Source (local) address */
+	bdaddr_t	dst;		/* Destination address */
+	char		*path;		/* Device path */
+	uint8_t		channel;	/* SAP server's rfcomm channel */
+};
+
+static GSList *devices = NULL;
+
+static struct sap_device *find_device(GSList *devices, const char *path)
+{
+	GSList *l;
+
+	for (l = devices; l != NULL; l = l->next) {
+		struct sap_device *device = l->data;
+
+		if (!strcmp(device->path, path))
+			return device;
+	}
+
+	return NULL;
+}
+
+static void sap_device_free(struct sap_device *device)
+{
+	g_free(device->path);
+	if (device->conn)
+		dbus_connection_unref(device->conn);
+	g_free(device);
+}
+
+static void path_unregister(void *data)
+{
+	struct sap_device *device = data;
+
+	info("Unregistered interface %s on path %s", SAP_CLIENT_INTERFACE,
+		device->path);
+
+	devices = g_slist_remove(devices, device);
+	sap_device_free(device);
+}
+
+static DBusMessage *sap_connect(DBusConnection *conn,
+				DBusMessage *msg, void *data)
+{
+	return NULL;
+}
+
+static DBusMessage *sap_disconnect(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	return NULL;
+}
+
+static DBusMessage *sap_send_apdu(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	return NULL;
+}
+
+static DBusMessage *sap_power_on(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	return NULL;
+}
+
+static DBusMessage *sap_power_off(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	return NULL;
+}
+
+static DBusMessage *sap_reset(DBusConnection *conn,
+				DBusMessage *msg, void *data)
+{
+	return NULL;
+}
+
+static DBusMessage *sap_set_protocol(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	return NULL;
+}
+
+static DBusMessage *sap_get_properties(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	return NULL;
+}
+
+static GDBusMethodTable sap_methods[] = {
+	{ "Connect",		"",	"",	sap_connect,
+						G_DBUS_METHOD_FLAG_ASYNC },
+	{ "Disconnect",		"",	"",	sap_disconnect,
+						G_DBUS_METHOD_FLAG_ASYNC },
+	{ "SendAPDU",		"ay",	"ay",	sap_send_apdu,
+						G_DBUS_METHOD_FLAG_ASYNC },
+	{ "PowerOff",		"",	"",	sap_power_off,
+						G_DBUS_METHOD_FLAG_DEPRECATED },
+	{ "PowerOn",		"",	"",	sap_power_on,
+						G_DBUS_METHOD_FLAG_DEPRECATED },
+	{ "Reset",		"",	"",	sap_reset,
+						G_DBUS_METHOD_FLAG_DEPRECATED },
+	{ "SetProtocol",	"y",	"",	sap_set_protocol,
+						G_DBUS_METHOD_FLAG_DEPRECATED },
+	{ "GetProperties",	"",	"a{sv}",sap_get_properties },
+	{ NULL, NULL, NULL, NULL }
+};
+
+static GDBusSignalTable sap_signals[] = {
+	{ "PropertyChanged",		"sv"	},
+	{ NULL, NULL }
+};
+
+static struct sap_device *create_sap_device(DBusConnection *conn,
+					const char *path, bdaddr_t *src,
+					bdaddr_t *dst, uint8_t channel)
+{
+	struct sap_device *device;
+
+	device = g_new0(struct sap_device, 1);
+	device->conn = dbus_connection_ref(conn);
+	bacpy(&device->dst, dst);
+	bacpy(&device->src, src);
+	device->path = g_strdup(path);
+	device->channel = channel;
+
+	if (!g_dbus_register_interface(conn, path,
+				SAP_CLIENT_INTERFACE,
+				sap_methods, sap_signals, NULL,
+				device, path_unregister)) {
+		error("D-Bus failed to register %s interface",
+				SAP_CLIENT_INTERFACE);
+		sap_device_free(device);
+		return NULL;
+	}
+
+	info("Registered interface %s on path %s",
+		SAP_CLIENT_INTERFACE, path);
+
+	return device;
+}
+
+int sap_device_register(DBusConnection *conn, const char *path, bdaddr_t *src,
+			  bdaddr_t *dst, const char *uuid, uint8_t channel)
+{
+	struct sap_device *device;
+
+	device = find_device(devices, path);
+	if (!device) {
+		device = create_sap_device(conn, path, src, dst, channel);
+		if (!device)
+			return -1;
+		devices = g_slist_append(devices, device);
+	}
+
+	return 0;
+}
+
+int sap_device_unregister(const char *path)
+{
+	struct sap_device *device;
+
+	device = find_device(devices, path);
+	if (!device)
+		return -ENOENT;
+
+	g_dbus_unregister_interface(device->conn, path, SAP_CLIENT_INTERFACE);
+
+	return 0;
+}
diff --git a/sap/device.h b/sap/device.h
new file mode 100644
index 0000000..e9f95b1
--- /dev/null
+++ b/sap/device.h
@@ -0,0 +1,28 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2009 Intel Corporation
+ *  Copyright (C) 2004-2009  Marcel Holtmann <marcel@holtmann.org>
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+int sap_device_register(DBusConnection *conn, const char *path, bdaddr_t *src,
+		  bdaddr_t *dst, const char *name, uint8_t channel);
+
+int sap_device_unregister(const char *path);
diff --git a/sap/main.c b/sap/main.c
new file mode 100644
index 0000000..f7b5857
--- /dev/null
+++ b/sap/main.c
@@ -0,0 +1,59 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2009 Intel Corporation
+ *  Copyright (C) 2004-2009  Marcel Holtmann <marcel@holtmann.org>
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+
+#include <gdbus.h>
+
+#include "plugin.h"
+#include "manager.h"
+
+static DBusConnection *connection;
+
+static int sap_init(void)
+{
+	connection = dbus_bus_get(DBUS_BUS_SYSTEM, NULL);
+	if (connection == NULL)
+		return -EIO;
+
+	if (sap_manager_init(connection) < 0) {
+		dbus_connection_unref(connection);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void sap_exit(void)
+{
+	sap_manager_exit();
+
+	dbus_connection_unref(connection);
+}
+
+BLUETOOTH_PLUGIN_DEFINE("sap", sap_init, sap_exit)
diff --git a/sap/manager.c b/sap/manager.c
new file mode 100644
index 0000000..90ff789
--- /dev/null
+++ b/sap/manager.c
@@ -0,0 +1,130 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2009 Intel Corporation
+ *  Copyright (C) 2004-2009  Marcel Holtmann <marcel@holtmann.org>
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+
+#include <bluetooth/bluetooth.h>
+#include <bluetooth/hci.h>
+#include <bluetooth/sdp.h>
+#include <bluetooth/sdp_lib.h>
+
+#include <gdbus.h>
+
+#include "logging.h"
+#include "../src/adapter.h"
+#include "../src/device.h"
+
+#include "manager.h"
+#include "device.h"
+
+
+#define SAP_UUID_STR		"0000112D-0000-1000-8000-00805F9B34FB"
+
+static DBusConnection *connection = NULL;
+
+static int sap_client_probe(struct btd_device *device, const char *uuid)
+{
+	struct btd_adapter *adapter = device_get_adapter(device);
+	const gchar *path = device_get_path(device);
+	sdp_list_t *protos;
+	int ch;
+	bdaddr_t src, dst;
+	const sdp_record_t *rec;
+
+	DBG("path %s: %s", path, uuid);
+
+	rec = btd_device_get_record(device, uuid);
+	if (!rec)
+		return -EINVAL;
+
+	if (sdp_get_access_protos(rec, &protos) < 0)
+		return -EINVAL;
+
+	ch = sdp_get_proto_port(protos, RFCOMM_UUID);
+	sdp_list_foreach(protos, (sdp_list_func_t) sdp_list_free, NULL);
+	sdp_list_free(protos, NULL);
+
+	if (ch < 1 || ch > 30) {
+		error("Channel out of range: %d", ch);
+		return -EINVAL;
+	}
+
+	adapter_get_address(adapter, &src);
+	device_get_address(device, &dst);
+
+	return sap_device_register(connection, path, &src, &dst, uuid, ch);
+}
+
+static void sap_client_remove(struct btd_device *device)
+{
+	const gchar *path = device_get_path(device);
+
+	DBG("path %s", path);
+
+	sap_device_unregister(path);
+}
+
+
+static int client_probe(struct btd_device *device, GSList *uuids)
+{
+	while (uuids) {
+		sap_client_probe(device, uuids->data);
+		uuids = uuids->next;
+	}
+
+	return 0;
+}
+
+static void client_remove(struct btd_device *device)
+{
+	return sap_client_remove(device);
+}
+
+static struct btd_device_driver sap_client_driver = {
+	.name	= "sap-client",
+	.uuids	= BTD_UUIDS(SAP_UUID_STR),
+	.probe	= client_probe,
+	.remove	= client_remove,
+};
+
+int sap_manager_init(DBusConnection *conn)
+{
+	connection = dbus_connection_ref(conn);
+
+	btd_register_device_driver(&sap_client_driver);
+
+	return 0;
+}
+
+void sap_manager_exit(void)
+{
+	btd_unregister_device_driver(&sap_client_driver);
+
+	dbus_connection_unref(connection);
+	connection = NULL;
+}
diff --git a/sap/manager.h b/sap/manager.h
new file mode 100644
index 0000000..f9162a3
--- /dev/null
+++ b/sap/manager.h
@@ -0,0 +1,26 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2009 Intel Corporation
+ *  Copyright (C) 2004-2009  Marcel Holtmann <marcel@holtmann.org>
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+int sap_manager_init(DBusConnection *conn);
+void sap_manager_exit(void);
-- 
1.5.6.3

RE: [Patch] SAP client plugin framework

[Liu, Raymond]

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

Holtmann, Johan

>
>Holtmann, Johan
>
>	The attach patch is for the SAP DBUS API doc and the initial framework code
>for SAP plugin.
>
>	Could you help to have a review.	Thanks!

	Resend the patch according to your comments.  

Best Regards,
Raymond Liu

[-- Attachment #2: 0001-Add-sap-api.txt-for-SAP-Client.patch --]
[-- Type: application/octet-stream, Size: 2299 bytes --]

From a1e32b2783bb9d2e0984b353ba5286d08ca34338 Mon Sep 17 00:00:00 2001
From: Raymond Liu <raymond.liu@intel.com>
Date: Fri, 16 Jan 2009 15:59:01 +0800
Subject: [PATCH] Add sap-api.txt for SAP Client

---
 doc/sap-api.txt |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 70 insertions(+), 0 deletions(-)
 create mode 100644 doc/sap-api.txt

diff --git a/doc/sap-api.txt b/doc/sap-api.txt
new file mode 100644
index 0000000..93ad21e
--- /dev/null
+++ b/doc/sap-api.txt
@@ -0,0 +1,70 @@
+BlueZ D-Bus SAP API description
+*********************************
+
+Copyright (C) 2008-2009 Intel Corporation
+
+
+SIM Access Profile Client hierarchy
+===============
+
+Service		org.bluez
+Interface	org.bluez.SimAccess
+Object path	[variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
+
+Methods		void Connect()
+
+			Connect to the SAP service on the remote device.
+
+		void Disconnect()
+
+			Disconnect from the SAP service on the remote device.
+
+		array{uint8} RunCmd(array{uint8})
+
+			Send Command-APDU to the remote device.
+			The return array contains Response-APDU
+
+		void Reset()
+
+			Reset the remote subscription module.
+
+		dict GetProperties()
+
+			Returns all properties for the interface. See the
+			properties section for available properties.
+
+		void SetProperty(string name, variant value)
+
+			Changes the value of the specified property. Only
+			properties that are listed a read-write are changeable.
+			On success this will emit a PropertyChanged signal.
+
+			Possible Errors: org.bluez.Error.DoesNotExist
+					 org.bluez.Error.InvalidArguments
+
+Signals		PropertyChanged(string name, variant value)
+
+			This signal indicates a changed value of the given
+			property.
+
+Properties	boolean Connected [readonly]
+
+			Indicates if there is an active connection on SAP service
+			with the remote device.
+
+		boolean Power [readwrite]
+
+			The power status of the remote subscription module.
+
+		uint8 Protocol [readwrite]
+
+			The transport protocol with the remote subscription module.
+			Only 0 and 1 is supported by the current spec.
+
+		string ModuleStatus [readonly]
+
+			Indicates the status of the subscription module.
+
+		uint8 CardReaderStatus [readonly]
+
+			Indicates the status of the CardReader.
-- 
1.5.6.3


[-- Attachment #3: 0002-Add-framework-code-for-SAP.patch --]
[-- Type: application/octet-stream, Size: 16165 bytes --]

From 7e4d1e6ffdb2324478c5512bcd61bc78e5566eaf Mon Sep 17 00:00:00 2001
From: Raymond Liu <raymond.liu@intel.com>
Date: Tue, 20 Jan 2009 13:47:12 +0800
Subject: [PATCH] Add framework code for SAP

---
 Makefile.am         |    2 +-
 acinclude.m4        |    6 ++
 configure.ac        |    1 +
 plugins/Makefile.am |    2 +
 sap/Makefile.am     |   21 +++++
 sap/device.c        |  203 +++++++++++++++++++++++++++++++++++++++++++++++++++
 sap/device.h        |   28 +++++++
 sap/main.c          |   59 +++++++++++++++
 sap/manager.c       |  130 ++++++++++++++++++++++++++++++++
 sap/manager.h       |   26 +++++++
 10 files changed, 477 insertions(+), 1 deletions(-)
 create mode 100644 sap/Makefile.am
 create mode 100644 sap/device.c
 create mode 100644 sap/device.h
 create mode 100644 sap/main.c
 create mode 100644 sap/manager.c
 create mode 100644 sap/manager.h

diff --git a/Makefile.am b/Makefile.am
index f2d1ca7..c6899d2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,6 +1,6 @@
 
 SUBDIRS = include lib sbc gdbus common src client\
-	plugins network serial input audio \
+	plugins network serial input audio sap \
 	tools rfcomm compat cups test scripts doc
 
 EXTRA_DIST = bluez.m4
diff --git a/acinclude.m4 b/acinclude.m4
index 372833e..704c9ee 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -186,6 +186,7 @@ AC_DEFUN([AC_ARG_BLUEZ], [
 	input_enable=yes
 	serial_enable=yes
 	network_enable=yes
+	sap_enable=yes
 	service_enable=yes
 	tools_enable=yes
 	hidd_enable=no
@@ -226,6 +227,10 @@ AC_DEFUN([AC_ARG_BLUEZ], [
 		audio_enable=${enableval}
 	])
 
+	AC_ARG_ENABLE(sap, AC_HELP_STRING([--disable-sap], [disable sap plugin]), [
+		sap_enable=${enableval}
+	])
+
 	AC_ARG_ENABLE(service, AC_HELP_STRING([--disable-service], [disable service plugin]), [
 		service_enable=${enableval}
 	])
@@ -344,6 +349,7 @@ AC_DEFUN([AC_ARG_BLUEZ], [
 	AM_CONDITIONAL(INPUTPLUGIN, test "${input_enable}" = "yes")
 	AM_CONDITIONAL(SERIALPLUGIN, test "${serial_enable}" = "yes")
 	AM_CONDITIONAL(NETWORKPLUGIN, test "${network_enable}" = "yes")
+	AM_CONDITIONAL(SAPPLUGIN, test "${sap_enable}" = "yes")
 	AM_CONDITIONAL(SERVICEPLUGIN, test "${service_enable}" = "yes")
 	AM_CONDITIONAL(HIDD, test "${hidd_enable}" = "yes")
 	AM_CONDITIONAL(PAND, test "${pand_enable}" = "yes")
diff --git a/configure.ac b/configure.ac
index 84a6234..bc7cf6b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -55,6 +55,7 @@ AC_OUTPUT([
 	serial/Makefile
 	input/Makefile
 	audio/Makefile
+	sap/Makefile
 	tools/Makefile
 	rfcomm/Makefile
 	compat/Makefile
diff --git a/plugins/Makefile.am b/plugins/Makefile.am
index 3b258a3..7ad76fe 100644
--- a/plugins/Makefile.am
+++ b/plugins/Makefile.am
@@ -46,6 +46,7 @@ all-local:
 	@$(LN_S) -f $(top_srcdir)/audio/.libs/audio.so
 	@$(LN_S) -f $(top_srcdir)/serial/.libs/serial.so
 	@$(LN_S) -f $(top_srcdir)/network/.libs/network.so
+	@$(LN_S) -f $(top_srcdir)/sap/.libs/sap.so
 	@$(LN_S) -f .libs/service.so
 	@$(LN_S) -f .libs/hal.so
 
@@ -56,3 +57,4 @@ clean-local:
 	@rm -f serial.so
 	@rm -f audio.so
 	@rm -f input.so
+	@rm -f sap.so
diff --git a/sap/Makefile.am b/sap/Makefile.am
new file mode 100644
index 0000000..aad95f3
--- /dev/null
+++ b/sap/Makefile.am
@@ -0,0 +1,21 @@
+
+if SAPPLUGIN
+plugindir = $(libdir)/bluetooth/plugins
+
+plugin_LTLIBRARIES = sap.la
+
+sap_la_SOURCES = main.c manager.h manager.c \
+		device.h device.c
+
+LDADD = $(top_builddir)/common/libhelper.a \
+		@GDBUS_LIBS@ @GLIB_LIBS@ @DBUS_LIBS@ @BLUEZ_LIBS@
+endif
+
+AM_LDFLAGS = -module -avoid-version -no-undefined \
+			-export-symbols-regex bluetooth_plugin_desc
+
+AM_CFLAGS = @BLUEZ_CFLAGS@ @DBUS_CFLAGS@ @GLIB_CFLAGS@ @GDBUS_CFLAGS@
+
+INCLUDES = -I$(top_srcdir)/common -I$(top_srcdir)/src
+
+MAINTAINERCLEANFILES = Makefile.in
diff --git a/sap/device.c b/sap/device.c
new file mode 100644
index 0000000..e826d94
--- /dev/null
+++ b/sap/device.c
@@ -0,0 +1,203 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2009 Intel Corporation
+ *  Copyright (C) 2004-2009  Marcel Holtmann <marcel@holtmann.org>
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+
+#include <bluetooth/bluetooth.h>
+#include <bluetooth/hci.h>
+#include <bluetooth/sdp.h>
+#include <bluetooth/sdp_lib.h>
+
+#include <glib.h>
+#include <gdbus.h>
+
+#include "../src/dbus-common.h"
+
+#include "logging.h"
+#include "glib-helper.h"
+
+#include "device.h"
+
+#define SAP_CLIENT_INTERFACE	"org.bluez.SimAccess"
+
+struct sap_device {
+	DBusConnection	*conn;
+	bdaddr_t	src;		/* Source (local) address */
+	bdaddr_t	dst;		/* Destination address */
+	char		*path;		/* Device path */
+	uint8_t		channel;	/* SAP server's rfcomm channel */
+};
+
+static GSList *devices = NULL;
+
+static struct sap_device *find_device(GSList *devices, const char *path)
+{
+	GSList *l;
+
+	for (l = devices; l != NULL; l = l->next) {
+		struct sap_device *device = l->data;
+
+		if (!strcmp(device->path, path))
+			return device;
+	}
+
+	return NULL;
+}
+
+static void sap_device_free(struct sap_device *device)
+{
+	g_free(device->path);
+	if (device->conn)
+		dbus_connection_unref(device->conn);
+	g_free(device);
+}
+
+static void path_unregister(void *data)
+{
+	struct sap_device *device = data;
+
+	info("Unregistered interface %s on path %s", SAP_CLIENT_INTERFACE,
+		device->path);
+
+	devices = g_slist_remove(devices, device);
+	sap_device_free(device);
+}
+
+static DBusMessage *sap_connect(DBusConnection *conn,
+				DBusMessage *msg, void *data)
+{
+	return NULL;
+}
+
+static DBusMessage *sap_disconnect(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	return NULL;
+}
+
+static DBusMessage *sap_run_cmd(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	return NULL;
+}
+
+static DBusMessage *sap_reset(DBusConnection *conn,
+				DBusMessage *msg, void *data)
+{
+	return NULL;
+}
+
+static DBusMessage *sap_set_property(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	return NULL;
+}
+
+static DBusMessage *sap_get_properties(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	return NULL;
+}
+
+static GDBusMethodTable sap_methods[] = {
+	{ "Connect",		"",	"",	sap_connect,
+						G_DBUS_METHOD_FLAG_ASYNC },
+	{ "Disconnect",		"",	"",	sap_disconnect,
+						G_DBUS_METHOD_FLAG_ASYNC },
+	{ "RunCmd",		"ay",	"ay",	sap_run_cmd,
+						G_DBUS_METHOD_FLAG_ASYNC },
+	{ "Reset",		"",	"",	sap_reset,
+						G_DBUS_METHOD_FLAG_DEPRECATED },
+	{ "SetProperty",	"sv",	"",	sap_set_property,
+						G_DBUS_METHOD_FLAG_DEPRECATED },
+	{ "GetProperties",	"",	"a{sv}",sap_get_properties },
+	{ NULL, NULL, NULL, NULL }
+};
+
+static GDBusSignalTable sap_signals[] = {
+	{ "PropertyChanged",		"sv"	},
+	{ NULL, NULL }
+};
+
+static struct sap_device *create_sap_device(DBusConnection *conn,
+					const char *path, bdaddr_t *src,
+					bdaddr_t *dst, uint8_t channel)
+{
+	struct sap_device *device;
+
+	device = g_new0(struct sap_device, 1);
+	device->conn = dbus_connection_ref(conn);
+	bacpy(&device->dst, dst);
+	bacpy(&device->src, src);
+	device->path = g_strdup(path);
+	device->channel = channel;
+
+	if (!g_dbus_register_interface(conn, path,
+				SAP_CLIENT_INTERFACE,
+				sap_methods, sap_signals, NULL,
+				device, path_unregister)) {
+		error("D-Bus failed to register %s interface",
+				SAP_CLIENT_INTERFACE);
+		sap_device_free(device);
+		return NULL;
+	}
+
+	info("Registered interface %s on path %s",
+		SAP_CLIENT_INTERFACE, path);
+
+	return device;
+}
+
+int sap_device_register(DBusConnection *conn, const char *path, bdaddr_t *src,
+			  bdaddr_t *dst, const char *uuid, uint8_t channel)
+{
+	struct sap_device *device;
+
+	device = find_device(devices, path);
+	if (!device) {
+		device = create_sap_device(conn, path, src, dst, channel);
+		if (!device)
+			return -1;
+		devices = g_slist_append(devices, device);
+	}
+
+	return 0;
+}
+
+int sap_device_unregister(const char *path)
+{
+	struct sap_device *device;
+
+	device = find_device(devices, path);
+	if (!device)
+		return -ENOENT;
+
+	g_dbus_unregister_interface(device->conn, path, SAP_CLIENT_INTERFACE);
+
+	return 0;
+}
diff --git a/sap/device.h b/sap/device.h
new file mode 100644
index 0000000..e9f95b1
--- /dev/null
+++ b/sap/device.h
@@ -0,0 +1,28 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2009 Intel Corporation
+ *  Copyright (C) 2004-2009  Marcel Holtmann <marcel@holtmann.org>
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+int sap_device_register(DBusConnection *conn, const char *path, bdaddr_t *src,
+		  bdaddr_t *dst, const char *name, uint8_t channel);
+
+int sap_device_unregister(const char *path);
diff --git a/sap/main.c b/sap/main.c
new file mode 100644
index 0000000..f7b5857
--- /dev/null
+++ b/sap/main.c
@@ -0,0 +1,59 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2009 Intel Corporation
+ *  Copyright (C) 2004-2009  Marcel Holtmann <marcel@holtmann.org>
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+
+#include <gdbus.h>
+
+#include "plugin.h"
+#include "manager.h"
+
+static DBusConnection *connection;
+
+static int sap_init(void)
+{
+	connection = dbus_bus_get(DBUS_BUS_SYSTEM, NULL);
+	if (connection == NULL)
+		return -EIO;
+
+	if (sap_manager_init(connection) < 0) {
+		dbus_connection_unref(connection);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void sap_exit(void)
+{
+	sap_manager_exit();
+
+	dbus_connection_unref(connection);
+}
+
+BLUETOOTH_PLUGIN_DEFINE("sap", sap_init, sap_exit)
diff --git a/sap/manager.c b/sap/manager.c
new file mode 100644
index 0000000..90ff789
--- /dev/null
+++ b/sap/manager.c
@@ -0,0 +1,130 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2009 Intel Corporation
+ *  Copyright (C) 2004-2009  Marcel Holtmann <marcel@holtmann.org>
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+
+#include <bluetooth/bluetooth.h>
+#include <bluetooth/hci.h>
+#include <bluetooth/sdp.h>
+#include <bluetooth/sdp_lib.h>
+
+#include <gdbus.h>
+
+#include "logging.h"
+#include "../src/adapter.h"
+#include "../src/device.h"
+
+#include "manager.h"
+#include "device.h"
+
+
+#define SAP_UUID_STR		"0000112D-0000-1000-8000-00805F9B34FB"
+
+static DBusConnection *connection = NULL;
+
+static int sap_client_probe(struct btd_device *device, const char *uuid)
+{
+	struct btd_adapter *adapter = device_get_adapter(device);
+	const gchar *path = device_get_path(device);
+	sdp_list_t *protos;
+	int ch;
+	bdaddr_t src, dst;
+	const sdp_record_t *rec;
+
+	DBG("path %s: %s", path, uuid);
+
+	rec = btd_device_get_record(device, uuid);
+	if (!rec)
+		return -EINVAL;
+
+	if (sdp_get_access_protos(rec, &protos) < 0)
+		return -EINVAL;
+
+	ch = sdp_get_proto_port(protos, RFCOMM_UUID);
+	sdp_list_foreach(protos, (sdp_list_func_t) sdp_list_free, NULL);
+	sdp_list_free(protos, NULL);
+
+	if (ch < 1 || ch > 30) {
+		error("Channel out of range: %d", ch);
+		return -EINVAL;
+	}
+
+	adapter_get_address(adapter, &src);
+	device_get_address(device, &dst);
+
+	return sap_device_register(connection, path, &src, &dst, uuid, ch);
+}
+
+static void sap_client_remove(struct btd_device *device)
+{
+	const gchar *path = device_get_path(device);
+
+	DBG("path %s", path);
+
+	sap_device_unregister(path);
+}
+
+
+static int client_probe(struct btd_device *device, GSList *uuids)
+{
+	while (uuids) {
+		sap_client_probe(device, uuids->data);
+		uuids = uuids->next;
+	}
+
+	return 0;
+}
+
+static void client_remove(struct btd_device *device)
+{
+	return sap_client_remove(device);
+}
+
+static struct btd_device_driver sap_client_driver = {
+	.name	= "sap-client",
+	.uuids	= BTD_UUIDS(SAP_UUID_STR),
+	.probe	= client_probe,
+	.remove	= client_remove,
+};
+
+int sap_manager_init(DBusConnection *conn)
+{
+	connection = dbus_connection_ref(conn);
+
+	btd_register_device_driver(&sap_client_driver);
+
+	return 0;
+}
+
+void sap_manager_exit(void)
+{
+	btd_unregister_device_driver(&sap_client_driver);
+
+	dbus_connection_unref(connection);
+	connection = NULL;
+}
diff --git a/sap/manager.h b/sap/manager.h
new file mode 100644
index 0000000..f9162a3
--- /dev/null
+++ b/sap/manager.h
@@ -0,0 +1,26 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2009 Intel Corporation
+ *  Copyright (C) 2004-2009  Marcel Holtmann <marcel@holtmann.org>
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+int sap_manager_init(DBusConnection *conn);
+void sap_manager_exit(void);
-- 
1.5.6.3

Re: SBC big endian issues?

[Brad Midgley]

Siarhei

> Also an interesting observation is that gstreamer is very slow. Using
> gst-launch to encode audio file to SBC is something like 1.5 times slower
> than sbcenc utility when benchmarked on x86 system. I wonder if
> something could be done about this.

I noticed something similar back in 3.x. On an xscale, it went from
about 15% cpu usage under libalsa to 50% using gst-launch.

Is there a best practices for gstreamer that the plugin is not following?

-- 
Brad Midgley

Re: SBC big endian issues?

[Siarhei Siamashka]

On Monday 19 January 2009 17:02:03 ext Brad Midgley wrote:
> > Also an interesting observation is that gstreamer is very slow. Using
> > gst-launch to encode audio file to SBC is something like 1.5 times slower
> > than sbcenc utility when benchmarked on x86 system. I wonder if
> > something could be done about this.
>
> I noticed something similar back in 3.x. On an xscale, it went from
> about 15% cpu usage under libalsa to 50% using gst-launch.

By the way, if that XScale chip is IWMMXT capable, it can benefit from SIMD
optimizations too. This should probably reduce CPU usage way below 15%.

Moreover, IWMMXT is essentially an MMX clone for ARM, so x86 MMX code can
be directly converted to it.

> Is there a best practices for gstreamer that the plugin is not following?

I'm not quite familiar with gstreamer yet, but these performance issues might
be really worth investigating...


Best regards,
Siarhei Siamashka

Re: SBC big endian issues?

[Siarhei Siamashka]

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

On Wednesday 07 January 2009 14:40:01 ext Siarhei Siamashka wrote:
> On Monday 05 January 2009 21:26:17 ext Brad Midgley wrote:
> > The copy is inefficient but it would be even better if we didn't have
> > to do it at all. I was investigating zero copy here and came up with a
> > patch but it was too complicated to be accepted.
>
> Yes, it is possible to reduce the number of data copies. Having
> both 'sbc_encoder_state->X' and 'sbc_frame->pcm_sample' arrays
> is obviously redundant and only one of them should remain.
>
> But eliminating any kind of data copies completely is hardly possible. The
> encoder needs to always have data for the 72 or 36 previous samples, so
> they need to be stored somewhere between the calls to frame encode
> function. And frame encode function will probably get data in small chunks
> if having low latency is desired. So at least this part of data will need
> to be moved around. Additionally, SIMD optimizations require input data
> permutation, so they can't work directly with the input buffer.
>
> Preserving the input samples 'history' is currently achieved by having
> 'sbc_encoder_state->X' array which works as some kind of ring buffer.
> If this buffer had infinite size, we would not have to duplicate
> information in the lower and higher halves of this buffer. But this is of
> course not possible due to the need to keep memory use reasonable and also
> in order to efficiently use data cache. Anyway, increasing
> 'sbc_encoder_state->X' size to some reasonable value may help to reduce
> extra overhead. One of the variants is to have space in X buffer for all
> the input data of a frame plus these 72/36 samples from the previous frame,
> copy the previous 72/36 samples to it, and then perform "endian conversion
> + channels deinterleaving + do SIMD permutation" in one pass directly into
> the X buffer from the buffer provided by the user at the entry of the frame
> packing function. Increasing X buffer more may allow to copy the previous
> 72/36 samples only once per 2 frames, once per 3 frames or whatever.
>
> This is not complicated at all, but is indeed a bit intrusive in the sense
> that it will touch quite a large part of code. But we are ready to do it
> now, am I right?

The abovementioned optimization is implemented in the attached experimental
patch. It makes sbcenc ~20% faster, which is a really nice improvement.

> > The messy part here is we let the caller specify the byte order of the
> > array. It would simplify a lot to standardize on host endian. I don't
> > remember what the reasoning was against this.

Yeah, this is one of the things that can simplify code and make maintenance
easier.

OK, before I submit a final/committable version of this patch, some things may
need to be discussed/decided:

1. Alignment of the input data. Currently data is passed in as a void *
pointer, which does not imply any alignment requirements. It would be nice if
the input data would be guaranteed to be accessible as  int16_t elements
(16-bit sound samples). On the platforms with strict alignment (older ARM
cores), unaligned memory accesses are not supported. Current sbcenc utility
uses 'unsigned char' buffer to read the data from file and pass it to the
encoder. With this setup, the compiler is theoretically allowed to allocate
this buffer on uneven address, causing the troubles if we interpret it as
(int16_t *) buffer in the encoder. I suggest changing sbc_encode/sbc_decode
function prototypes to int16_t * data type for the buffers with audio samples
and fixing sbcenc utility and other clients (gstreamer/alsa) which may
potentially have alignment problems.

2. Is support for non-simd variant of the analysis filter useful enough to
keep it (sbc_analyze_4b_4s/sbc_analyze_4b_8s functions)? They have
a slight advantage over the simd variant in the sense that they only require a
single table with coefficients (as opposed to two tables for simd). Also it is
possible to implement non-simd input data handling as a simple memory
copy (no permutation needed) or even directly work on some part of the input
data (if the input data is large enough, some copying is still needed to carry
on data between sbc_encode calls). But in the case of having simd/multimedia
extension support, simd variant becomes clearly preferable. Most cpus
have some sort of simd supported nowadays and this is interesting for some
really low end or very old cpu. Is keeping this code worth extra maintenance
burden?

3. Status of endian conversion in the SBC codec. Dropping endian conversion
functionality in SBC and relaying it to the client application will make SBC
code simpler. On the other hand, built-in endian conversion is a bit faster
if we get data in inherently non-native byte order (loaded from big endian
data format on little endian cpu for example). Is it worth extra complexity?

Comments are very much welcome.


Best regards,
Siarhei Siamashka

[-- Attachment #2: sbc-fast-processing-of-input-data.diff --]
[-- Type: text/x-diff, Size: 19004 bytes --]

diff --git a/sbc/sbc.c b/sbc/sbc.c
index 827b731..2750cc4 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -657,14 +657,11 @@ static int sbc_analyze_audio(struct sbc_encoder_state *state,
 		for (ch = 0; ch < frame->channels; ch++)
 			for (blk = 0; blk < frame->blocks; blk += 4) {
 				state->sbc_analyze_4b_4s(
-					&frame->pcm_sample[ch][blk * 4],
-					&state->X[ch][state->position[ch]],
+					&state->X[ch][state->position +
+							48 - blk * 4],
 					frame->sb_sample_f[blk][ch],
 					frame->sb_sample_f[blk + 1][ch] -
 					frame->sb_sample_f[blk][ch]);
-				state->position[ch] -= 16;
-				if (state->position[ch] < 0)
-					state->position[ch] = 64 - 16;
 			}
 		return frame->blocks * 4;
 
@@ -672,14 +669,11 @@ static int sbc_analyze_audio(struct sbc_encoder_state *state,
 		for (ch = 0; ch < frame->channels; ch++)
 			for (blk = 0; blk < frame->blocks; blk += 4) {
 				state->sbc_analyze_4b_8s(
-					&frame->pcm_sample[ch][blk * 8],
-					&state->X[ch][state->position[ch]],
+					&state->X[ch][state->position +
+							96 - blk * 8],
 					frame->sb_sample_f[blk][ch],
 					frame->sb_sample_f[blk + 1][ch] -
 					frame->sb_sample_f[blk][ch]);
-				state->position[ch] -= 32;
-				if (state->position[ch] < 0)
-					state->position[ch] = 128 - 32;
 			}
 		return frame->blocks * 8;
 
@@ -918,8 +912,7 @@ static void sbc_encoder_init(struct sbc_encoder_state *state,
 				const struct sbc_frame *frame)
 {
 	memset(&state->X, 0, sizeof(state->X));
-	state->subbands = frame->subbands;
-	state->position[0] = state->position[1] = 12 * frame->subbands;
+	state->position = 0;
 
 	sbc_init_primitives(state);
 }
@@ -1043,12 +1036,22 @@ int sbc_encode(sbc_t *sbc, void *input, int input_len, void *output,
 		int output_len, int *written)
 {
 	struct sbc_priv *priv;
-	char *ptr;
-	int i, ch, framelen, samples;
+	int framelen, samples;
+#if __BYTE_ORDER == __LITTLE_ENDIAN
+	int swap_endian = (sbc->endian != SBC_LE);
+#elif __BYTE_ORDER == __BIG_ENDIAN
+	int swap_endian = (sbc->endian != SBC_BE);
+#else
+#error "Unknown byte order"
+#endif
 
 	if (!sbc && !input)
 		return -EIO;
 
+	/* input buffer must be 2 bytes aligned (alignment for audio samples) */
+	if ((uintptr_t) input & 1)
+		return -EIO;
+
 	priv = sbc->priv;
 
 	if (written)
@@ -1079,19 +1082,22 @@ int sbc_encode(sbc_t *sbc, void *input, int input_len, void *output,
 	if (!output || output_len < priv->frame.length)
 		return -ENOSPC;
 
-	ptr = input;
-
-	for (i = 0; i < priv->frame.subbands * priv->frame.blocks; i++) {
-		for (ch = 0; ch < priv->frame.channels; ch++) {
-			int16_t s;
-			if (sbc->endian == SBC_BE)
-				s = (ptr[0] & 0xff) << 8 | (ptr[1] & 0xff);
-			else
-				s = (ptr[0] & 0xff) | (ptr[1] & 0xff) << 8;
-			ptr += 2;
-			priv->frame.pcm_sample[ch][i] = s;
-		}
-	}
+	if (priv->frame.subbands == 4)
+		priv->enc_state.position =
+			priv->enc_state.sbc_encoder_process_input_4s(
+				priv->enc_state.position,
+				(int16_t *) input, priv->enc_state.X,
+				priv->frame.subbands * priv->frame.blocks,
+				priv->frame.channels,
+				swap_endian);
+	else
+		priv->enc_state.position =
+			priv->enc_state.sbc_encoder_process_input_8s(
+				priv->enc_state.position,
+				(int16_t *) input, priv->enc_state.X,
+				priv->frame.subbands * priv->frame.blocks,
+				priv->frame.channels,
+				swap_endian);
 
 	samples = sbc_analyze_audio(&priv->enc_state, &priv->frame);
 
diff --git a/sbc/sbc_primitives.c b/sbc/sbc_primitives.c
index e3a7764..45bcccc 100644
--- a/sbc/sbc_primitives.c
+++ b/sbc/sbc_primitives.c
@@ -25,6 +25,8 @@
 
 #include <stdint.h>
 #include <limits.h>
+#include <byteswap.h>
+#include <string.h>
 #include "sbc.h"
 #include "sbc_math.h"
 #include "sbc_tables.h"
@@ -33,6 +35,8 @@
 #include "sbc_primitives_mmx.h"
 #include "sbc_primitives_neon.h"
 
+#if 0
+
 /*
  * A standard C code of analysis filter.
  */
@@ -176,6 +180,8 @@ static void sbc_analyze_4b_8s(int16_t *pcm, int16_t *x,
 	sbc_analyze_eight(x, out);
 }
 
+#endif
+
 /*
  * A reference C code of analysis filter with SIMD-friendly tables
  * reordering and code layout. This code can be used to develop platform
@@ -312,28 +318,9 @@ static inline void sbc_analyze_eight_simd(const int16_t *in, int32_t *out,
 			(SBC_COS_TABLE_FIXED8_SCALE - SCALE_OUT_BITS);
 }
 
-static inline void sbc_analyze_4b_4s_simd(int16_t *pcm, int16_t *x,
-						int32_t *out, int out_stride)
+static inline void sbc_analyze_4b_4s_simd(int16_t *x, int32_t *out,
+							int out_stride)
 {
-	/* Fetch audio samples and do input data reordering for SIMD */
-	x[64] = x[0]  = pcm[8 + 7];
-	x[65] = x[1]  = pcm[8 + 3];
-	x[66] = x[2]  = pcm[8 + 6];
-	x[67] = x[3]  = pcm[8 + 4];
-	x[68] = x[4]  = pcm[8 + 0];
-	x[69] = x[5]  = pcm[8 + 2];
-	x[70] = x[6]  = pcm[8 + 1];
-	x[71] = x[7]  = pcm[8 + 5];
-
-	x[72] = x[8]  = pcm[0 + 7];
-	x[73] = x[9]  = pcm[0 + 3];
-	x[74] = x[10] = pcm[0 + 6];
-	x[75] = x[11] = pcm[0 + 4];
-	x[76] = x[12] = pcm[0 + 0];
-	x[77] = x[13] = pcm[0 + 2];
-	x[78] = x[14] = pcm[0 + 1];
-	x[79] = x[15] = pcm[0 + 5];
-
 	/* Analyze blocks */
 	sbc_analyze_four_simd(x + 12, out, analysis_consts_fixed4_simd_odd);
 	out += out_stride;
@@ -344,44 +331,9 @@ static inline void sbc_analyze_4b_4s_simd(int16_t *pcm, int16_t *x,
 	sbc_analyze_four_simd(x + 0, out, analysis_consts_fixed4_simd_even);
 }
 
-static inline void sbc_analyze_4b_8s_simd(int16_t *pcm, int16_t *x,
-					  int32_t *out, int out_stride)
+static inline void sbc_analyze_4b_8s_simd(int16_t *x, int32_t *out,
+							int out_stride)
 {
-	/* Fetch audio samples and do input data reordering for SIMD */
-	x[128] = x[0]  = pcm[16 + 15];
-	x[129] = x[1]  = pcm[16 + 7];
-	x[130] = x[2]  = pcm[16 + 14];
-	x[131] = x[3]  = pcm[16 + 8];
-	x[132] = x[4]  = pcm[16 + 13];
-	x[133] = x[5]  = pcm[16 + 9];
-	x[134] = x[6]  = pcm[16 + 12];
-	x[135] = x[7]  = pcm[16 + 10];
-	x[136] = x[8]  = pcm[16 + 11];
-	x[137] = x[9]  = pcm[16 + 3];
-	x[138] = x[10] = pcm[16 + 6];
-	x[139] = x[11] = pcm[16 + 0];
-	x[140] = x[12] = pcm[16 + 5];
-	x[141] = x[13] = pcm[16 + 1];
-	x[142] = x[14] = pcm[16 + 4];
-	x[143] = x[15] = pcm[16 + 2];
-
-	x[144] = x[16] = pcm[0 + 15];
-	x[145] = x[17] = pcm[0 + 7];
-	x[146] = x[18] = pcm[0 + 14];
-	x[147] = x[19] = pcm[0 + 8];
-	x[148] = x[20] = pcm[0 + 13];
-	x[149] = x[21] = pcm[0 + 9];
-	x[150] = x[22] = pcm[0 + 12];
-	x[151] = x[23] = pcm[0 + 10];
-	x[152] = x[24] = pcm[0 + 11];
-	x[153] = x[25] = pcm[0 + 3];
-	x[154] = x[26] = pcm[0 + 6];
-	x[155] = x[27] = pcm[0 + 0];
-	x[156] = x[28] = pcm[0 + 5];
-	x[157] = x[29] = pcm[0 + 1];
-	x[158] = x[30] = pcm[0 + 4];
-	x[159] = x[31] = pcm[0 + 2];
-
 	/* Analyze blocks */
 	sbc_analyze_eight_simd(x + 24, out, analysis_consts_fixed8_simd_odd);
 	out += out_stride;
@@ -392,14 +344,187 @@ static inline void sbc_analyze_4b_8s_simd(int16_t *pcm, int16_t *x,
 	sbc_analyze_eight_simd(x + 0, out, analysis_consts_fixed8_simd_even);
 }
 
+#ifdef __GNUC__
+#define SBC_ALWAYS_INLINE __attribute__((always_inline))
+#else
+#define SBC_ALWAYS_INLINE inline
+#endif
+
+/*
+ * Input a new portion of data to encode into X buffer
+ */
+static SBC_ALWAYS_INLINE int sbc_encoder_process_input_s4_internal(
+	int position,
+	int16_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
+	int nsamples, int nchannels, int swap_endian)
+{
+	/* handle X buffer wraparound */
+	if (position < nsamples) {
+		if (nchannels > 0)
+			memcpy(&X[0][SBC_X_BUFFER_SIZE - 36], &X[0][position],
+							36 * sizeof(int16_t));
+		if (nchannels > 1)
+			memcpy(&X[1][SBC_X_BUFFER_SIZE - 36], &X[1][position],
+							36 * sizeof(int16_t));
+		position = SBC_X_BUFFER_SIZE - 36;
+	}
+
+	#define PCM(i) (swap_endian ? bswap_16(pcm[i]) : pcm[i])
+	/* copy/permutate audio samples */
+	while ((nsamples -= 8) >= 0) {
+		position -= 8;
+		if (nchannels > 0) {
+			int16_t *x = &X[0][position];
+			x[0]  = PCM(0 + 7 * nchannels);
+			x[1]  = PCM(0 + 3 * nchannels);
+			x[2]  = PCM(0 + 6 * nchannels);
+			x[3]  = PCM(0 + 4 * nchannels);
+			x[4]  = PCM(0 + 0 * nchannels);
+			x[5]  = PCM(0 + 2 * nchannels);
+			x[6]  = PCM(0 + 1 * nchannels);
+			x[7]  = PCM(0 + 5 * nchannels);
+		}
+		if (nchannels > 1) {
+			int16_t *x = &X[1][position];
+			x[0]  = PCM(1 + 7 * nchannels);
+			x[1]  = PCM(1 + 3 * nchannels);
+			x[2]  = PCM(1 + 6 * nchannels);
+			x[3]  = PCM(1 + 4 * nchannels);
+			x[4]  = PCM(1 + 0 * nchannels);
+			x[5]  = PCM(1 + 2 * nchannels);
+			x[6]  = PCM(1 + 1 * nchannels);
+			x[7]  = PCM(1 + 5 * nchannels);
+		}
+		pcm += 8 * nchannels;
+	}
+	#undef PCM
+
+	return position;
+}
+
+static SBC_ALWAYS_INLINE int sbc_encoder_process_input_s8_internal(
+	int position,
+	int16_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
+	int nsamples, int nchannels, int swap_endian)
+{
+	/* handle X buffer wraparound */
+	if (position < nsamples) {
+		if (nchannels > 0)
+			memcpy(&X[0][SBC_X_BUFFER_SIZE - 72], &X[0][position],
+							72 * sizeof(int16_t));
+		if (nchannels > 1)
+			memcpy(&X[1][SBC_X_BUFFER_SIZE - 72], &X[1][position],
+							72 * sizeof(int16_t));
+		position = SBC_X_BUFFER_SIZE - 72;
+	}
+
+	#define PCM(i) (swap_endian ? bswap_16(pcm[i]) : pcm[i])
+	/* copy/permutate audio samples */
+	while ((nsamples -= 16) >= 0) {
+		position -= 16;
+		if (nchannels > 0) {
+			int16_t *x = &X[0][position];
+			x[0]  = PCM(0 + 15 * nchannels);
+			x[1]  = PCM(0 + 7 * nchannels);
+			x[2]  = PCM(0 + 14 * nchannels);
+			x[3]  = PCM(0 + 8 * nchannels);
+			x[4]  = PCM(0 + 13 * nchannels);
+			x[5]  = PCM(0 + 9 * nchannels);
+			x[6]  = PCM(0 + 12 * nchannels);
+			x[7]  = PCM(0 + 10 * nchannels);
+			x[8]  = PCM(0 + 11 * nchannels);
+			x[9]  = PCM(0 + 3 * nchannels);
+			x[10] = PCM(0 + 6 * nchannels);
+			x[11] = PCM(0 + 0 * nchannels);
+			x[12] = PCM(0 + 5 * nchannels);
+			x[13] = PCM(0 + 1 * nchannels);
+			x[14] = PCM(0 + 4 * nchannels);
+			x[15] = PCM(0 + 2 * nchannels);
+		}
+		if (nchannels > 1) {
+			int16_t *x = &X[1][position];
+			x[0]  = PCM(1 + 15 * nchannels);
+			x[1]  = PCM(1 + 7 * nchannels);
+			x[2]  = PCM(1 + 14 * nchannels);
+			x[3]  = PCM(1 + 8 * nchannels);
+			x[4]  = PCM(1 + 13 * nchannels);
+			x[5]  = PCM(1 + 9 * nchannels);
+			x[6]  = PCM(1 + 12 * nchannels);
+			x[7]  = PCM(1 + 10 * nchannels);
+			x[8]  = PCM(1 + 11 * nchannels);
+			x[9]  = PCM(1 + 3 * nchannels);
+			x[10] = PCM(1 + 6 * nchannels);
+			x[11] = PCM(1 + 0 * nchannels);
+			x[12] = PCM(1 + 5 * nchannels);
+			x[13] = PCM(1 + 1 * nchannels);
+			x[14] = PCM(1 + 4 * nchannels);
+			x[15] = PCM(1 + 2 * nchannels);
+		}
+		pcm += 16 * nchannels;
+	}
+	#undef PCM
+
+	return position;
+}
+
+static int sbc_encoder_process_input_4s(int position,
+		int16_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
+		int nsamples, int nchannels, int swap_endian)
+{
+	if (nchannels > 1) {
+		if (swap_endian) {
+			return sbc_encoder_process_input_s4_internal(
+				position, pcm, X, nsamples, 2, 1);
+		} else {
+			return sbc_encoder_process_input_s4_internal(
+				position, pcm, X, nsamples, 2, 0);
+		}
+	} else {
+		if (swap_endian) {
+			return sbc_encoder_process_input_s4_internal(
+				position, pcm, X, nsamples, 1, 1);
+		} else {
+			return sbc_encoder_process_input_s4_internal(
+				position, pcm, X, nsamples, 1, 0);
+		}
+	}
+}
+
+static int sbc_encoder_process_input_8s(int position,
+		int16_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
+		int nsamples, int nchannels, int swap_endian)
+{
+	if (nchannels > 1) {
+		if (swap_endian) {
+			return sbc_encoder_process_input_s8_internal(
+				position, pcm, X, nsamples, 2, 1);
+		} else {
+			return sbc_encoder_process_input_s8_internal(
+				position, pcm, X, nsamples, 2, 0);
+		}
+	} else {
+		if (swap_endian) {
+			return sbc_encoder_process_input_s8_internal(
+				position, pcm, X, nsamples, 1, 1);
+		} else {
+			return sbc_encoder_process_input_s8_internal(
+				position, pcm, X, nsamples, 1, 0);
+		}
+	}
+}
+
 /*
  * Detect CPU features and setup function pointers
  */
 void sbc_init_primitives(struct sbc_encoder_state *state)
 {
 	/* Default implementation for analyze functions */
-	state->sbc_analyze_4b_4s = sbc_analyze_4b_4s;
-	state->sbc_analyze_4b_8s = sbc_analyze_4b_8s;
+	state->sbc_analyze_4b_4s = sbc_analyze_4b_4s_simd;
+	state->sbc_analyze_4b_8s = sbc_analyze_4b_8s_simd;
+
+	/* Default implementation for input reordering / deinterleaving */
+	state->sbc_encoder_process_input_4s = sbc_encoder_process_input_4s;
+	state->sbc_encoder_process_input_8s = sbc_encoder_process_input_8s;
 
 	/* X86/AMD64 optimizations */
 #ifdef SBC_BUILD_WITH_MMX_SUPPORT
diff --git a/sbc/sbc_primitives.h b/sbc/sbc_primitives.h
index 91b72ee..71b08f1 100644
--- a/sbc/sbc_primitives.h
+++ b/sbc/sbc_primitives.h
@@ -27,19 +27,23 @@
 #define __SBC_PRIMITIVES_H
 
 #define SCALE_OUT_BITS 15
+#define SBC_X_BUFFER_SIZE 328
 
 struct sbc_encoder_state {
-	int subbands;
-	int position[2];
-	int16_t SBC_ALIGNED X[2][256];
+	int position;
+	int16_t SBC_ALIGNED X[2][SBC_X_BUFFER_SIZE];
 	/* Polyphase analysis filter for 4 subbands configuration,
 	 * it handles 4 blocks at once */
-	void (*sbc_analyze_4b_4s)(int16_t *pcm, int16_t *x,
-					int32_t *out, int out_stride);
+	void (*sbc_analyze_4b_4s)(int16_t *x, int32_t *out, int out_stride);
 	/* Polyphase analysis filter for 8 subbands configuration,
 	 * it handles 4 blocks at once */
-	void (*sbc_analyze_4b_8s)(int16_t *pcm, int16_t *x,
-					int32_t *out, int out_stride);
+	void (*sbc_analyze_4b_8s)(int16_t *x, int32_t *out, int out_stride);
+	int (*sbc_encoder_process_input_4s)(int position,
+			int16_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
+			int nsamples, int nchannels, int swap_endian);
+	int (*sbc_encoder_process_input_8s)(int position,
+			int16_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
+			int nsamples, int nchannels, int swap_endian);
 };
 
 /*
diff --git a/sbc/sbc_primitives_mmx.c b/sbc/sbc_primitives_mmx.c
index 972e813..7db4af7 100644
--- a/sbc/sbc_primitives_mmx.c
+++ b/sbc/sbc_primitives_mmx.c
@@ -245,28 +245,9 @@ static inline void sbc_analyze_eight_mmx(const int16_t *in, int32_t *out,
 		: "memory");
 }
 
-static inline void sbc_analyze_4b_4s_mmx(int16_t *pcm, int16_t *x,
-						int32_t *out, int out_stride)
+static inline void sbc_analyze_4b_4s_mmx(int16_t *x, int32_t *out,
+						int out_stride)
 {
-	/* Fetch audio samples and do input data reordering for SIMD */
-	x[64] = x[0]  = pcm[8 + 7];
-	x[65] = x[1]  = pcm[8 + 3];
-	x[66] = x[2]  = pcm[8 + 6];
-	x[67] = x[3]  = pcm[8 + 4];
-	x[68] = x[4]  = pcm[8 + 0];
-	x[69] = x[5]  = pcm[8 + 2];
-	x[70] = x[6]  = pcm[8 + 1];
-	x[71] = x[7]  = pcm[8 + 5];
-
-	x[72] = x[8]  = pcm[0 + 7];
-	x[73] = x[9]  = pcm[0 + 3];
-	x[74] = x[10] = pcm[0 + 6];
-	x[75] = x[11] = pcm[0 + 4];
-	x[76] = x[12] = pcm[0 + 0];
-	x[77] = x[13] = pcm[0 + 2];
-	x[78] = x[14] = pcm[0 + 1];
-	x[79] = x[15] = pcm[0 + 5];
-
 	/* Analyze blocks */
 	sbc_analyze_four_mmx(x + 12, out, analysis_consts_fixed4_simd_odd);
 	out += out_stride;
@@ -279,44 +260,9 @@ static inline void sbc_analyze_4b_4s_mmx(int16_t *pcm, int16_t *x,
 	asm volatile ("emms\n");
 }
 
-static inline void sbc_analyze_4b_8s_mmx(int16_t *pcm, int16_t *x,
-						int32_t *out, int out_stride)
+static inline void sbc_analyze_4b_8s_mmx(int16_t *x, int32_t *out,
+						int out_stride)
 {
-	/* Fetch audio samples and do input data reordering for SIMD */
-	x[128] = x[0]  = pcm[16 + 15];
-	x[129] = x[1]  = pcm[16 + 7];
-	x[130] = x[2]  = pcm[16 + 14];
-	x[131] = x[3]  = pcm[16 + 8];
-	x[132] = x[4]  = pcm[16 + 13];
-	x[133] = x[5]  = pcm[16 + 9];
-	x[134] = x[6]  = pcm[16 + 12];
-	x[135] = x[7]  = pcm[16 + 10];
-	x[136] = x[8]  = pcm[16 + 11];
-	x[137] = x[9]  = pcm[16 + 3];
-	x[138] = x[10] = pcm[16 + 6];
-	x[139] = x[11] = pcm[16 + 0];
-	x[140] = x[12] = pcm[16 + 5];
-	x[141] = x[13] = pcm[16 + 1];
-	x[142] = x[14] = pcm[16 + 4];
-	x[143] = x[15] = pcm[16 + 2];
-
-	x[144] = x[16] = pcm[0 + 15];
-	x[145] = x[17] = pcm[0 + 7];
-	x[146] = x[18] = pcm[0 + 14];
-	x[147] = x[19] = pcm[0 + 8];
-	x[148] = x[20] = pcm[0 + 13];
-	x[149] = x[21] = pcm[0 + 9];
-	x[150] = x[22] = pcm[0 + 12];
-	x[151] = x[23] = pcm[0 + 10];
-	x[152] = x[24] = pcm[0 + 11];
-	x[153] = x[25] = pcm[0 + 3];
-	x[154] = x[26] = pcm[0 + 6];
-	x[155] = x[27] = pcm[0 + 0];
-	x[156] = x[28] = pcm[0 + 5];
-	x[157] = x[29] = pcm[0 + 1];
-	x[158] = x[30] = pcm[0 + 4];
-	x[159] = x[31] = pcm[0 + 2];
-
 	/* Analyze blocks */
 	sbc_analyze_eight_mmx(x + 24, out, analysis_consts_fixed8_simd_odd);
 	out += out_stride;
diff --git a/sbc/sbc_primitives_neon.c b/sbc/sbc_primitives_neon.c
index 7589a98..d9c12f9 100644
--- a/sbc/sbc_primitives_neon.c
+++ b/sbc/sbc_primitives_neon.c
@@ -210,28 +210,9 @@ static inline void _sbc_analyze_eight_neon(const int16_t *in, int32_t *out,
 			"d18", "d19");
 }
 
-static inline void sbc_analyze_4b_4s_neon(int16_t *pcm, int16_t *x,
+static inline void sbc_analyze_4b_4s_neon(int16_t *x,
 						int32_t *out, int out_stride)
 {
-	/* Fetch audio samples and do input data reordering for SIMD */
-	x[64] = x[0]  = pcm[8 + 7];
-	x[65] = x[1]  = pcm[8 + 3];
-	x[66] = x[2]  = pcm[8 + 6];
-	x[67] = x[3]  = pcm[8 + 4];
-	x[68] = x[4]  = pcm[8 + 0];
-	x[69] = x[5]  = pcm[8 + 2];
-	x[70] = x[6]  = pcm[8 + 1];
-	x[71] = x[7]  = pcm[8 + 5];
-
-	x[72] = x[8]  = pcm[0 + 7];
-	x[73] = x[9]  = pcm[0 + 3];
-	x[74] = x[10] = pcm[0 + 6];
-	x[75] = x[11] = pcm[0 + 4];
-	x[76] = x[12] = pcm[0 + 0];
-	x[77] = x[13] = pcm[0 + 2];
-	x[78] = x[14] = pcm[0 + 1];
-	x[79] = x[15] = pcm[0 + 5];
-
 	/* Analyze blocks */
 	_sbc_analyze_four_neon(x + 12, out, analysis_consts_fixed4_simd_odd);
 	out += out_stride;
@@ -242,44 +223,9 @@ static inline void sbc_analyze_4b_4s_neon(int16_t *pcm, int16_t *x,
 	_sbc_analyze_four_neon(x + 0, out, analysis_consts_fixed4_simd_even);
 }
 
-static inline void sbc_analyze_4b_8s_neon(int16_t *pcm, int16_t *x,
+static inline void sbc_analyze_4b_8s_neon(int16_t *x,
 						int32_t *out, int out_stride)
 {
-	/* Fetch audio samples and do input data reordering for SIMD */
-	x[128] = x[0]  = pcm[16 + 15];
-	x[129] = x[1]  = pcm[16 + 7];
-	x[130] = x[2]  = pcm[16 + 14];
-	x[131] = x[3]  = pcm[16 + 8];
-	x[132] = x[4]  = pcm[16 + 13];
-	x[133] = x[5]  = pcm[16 + 9];
-	x[134] = x[6]  = pcm[16 + 12];
-	x[135] = x[7]  = pcm[16 + 10];
-	x[136] = x[8]  = pcm[16 + 11];
-	x[137] = x[9]  = pcm[16 + 3];
-	x[138] = x[10] = pcm[16 + 6];
-	x[139] = x[11] = pcm[16 + 0];
-	x[140] = x[12] = pcm[16 + 5];
-	x[141] = x[13] = pcm[16 + 1];
-	x[142] = x[14] = pcm[16 + 4];
-	x[143] = x[15] = pcm[16 + 2];
-
-	x[144] = x[16] = pcm[0 + 15];
-	x[145] = x[17] = pcm[0 + 7];
-	x[146] = x[18] = pcm[0 + 14];
-	x[147] = x[19] = pcm[0 + 8];
-	x[148] = x[20] = pcm[0 + 13];
-	x[149] = x[21] = pcm[0 + 9];
-	x[150] = x[22] = pcm[0 + 12];
-	x[151] = x[23] = pcm[0 + 10];
-	x[152] = x[24] = pcm[0 + 11];
-	x[153] = x[25] = pcm[0 + 3];
-	x[154] = x[26] = pcm[0 + 6];
-	x[155] = x[27] = pcm[0 + 0];
-	x[156] = x[28] = pcm[0 + 5];
-	x[157] = x[29] = pcm[0 + 1];
-	x[158] = x[30] = pcm[0 + 4];
-	x[159] = x[31] = pcm[0 + 2];
-
 	/* Analyze blocks */
 	_sbc_analyze_eight_neon(x + 24, out, analysis_consts_fixed8_simd_odd);
 	out += out_stride;