[PATCH v3] sbc: detect when bitpool has changed

[PATCH v3] sbc: detect when bitpool has changed

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>

A2DP spec allow bitpool changes midstream which is why sbc configuration
has a range of values for bitpool that the encoder can use and decoder
must support.

Bitpool changes do not affect the state of encoder/decoder so they don't
need to be reinitialize when this happens, so the impact is fairly small,
what it does change is the frame length so encoders may change the
bitpool to use the link more efficiently.
---
 sbc/sbc.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/sbc/sbc.c b/sbc/sbc.c
index a6391ae..77fcc5d 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -978,6 +978,9 @@ ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
 
 		priv->frame.codesize = sbc_get_codesize(sbc);
 		priv->frame.length = framelen;
+	} else if (priv->frame.bitpool != sbc->bitpool) {
+		priv->frame.length = framelen;
+		sbc->bitpool = priv->frame.bitpool;
 	}
 
 	if (!output)
@@ -1050,6 +1053,9 @@ ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len,
 
 		sbc_encoder_init(&priv->enc_state, &priv->frame);
 		priv->init = 1;
+	} else if (priv->frame.bitpool != sbc->bitpool) {
+		priv->frame.length = sbc_get_frame_length(sbc);
+		priv->frame.bitpool = sbc->bitpool;
 	}
 
 	/* input must be large enough to encode a complete frame */
@@ -1120,7 +1126,7 @@ size_t sbc_get_frame_length(sbc_t *sbc)
 	struct sbc_priv *priv;
 
 	priv = sbc->priv;
-	if (priv->init)
+	if (priv->init && priv->frame.bitpool == sbc->bitpool)
 		return priv->frame.length;
 
 	subbands = sbc->subbands ? 8 : 4;
-- 
1.7.1

SBC loudness bug?

[Christian Hoene]

Hello,

Mr. Hagen Hentschel was telling me that the BlueZ SBC implementation still has an bug. As compared to Bluetooth's reference implementation the signal is twotimes too silent. We told me, that in the last step of calculating the PCM samples a 1 bit right shift was done too much.

A correct implementation for C# can be found at http://sourceforge.net/projects/sbcsharp/

Sorry, due to time limits I cannot provide you with a patch...

With best regards,

 Christian Hoene

Re: SBC loudness bug?

[Siarhei Siamashka]

[-- Attachment #1: Type: Text/Plain, Size: 2735 bytes --]

On Wednesday 22 December 2010 16:14:16 Christian Hoene wrote:
> Hello,
> 
> Mr. Hagen Hentschel was telling me that the BlueZ SBC implementation still
> has an bug.

Yes, unfortunately bluez sbc *decoder* is still as bad as it used to be. There
easily may be more than one bug in it.

I feel somewhat responsible for not bringing sbc decoder into a better shape
earlier. But at that time, only properly working and fast sbc *encoder* was
needed for A2DP bluetooth stereo headsets support in Nokia N900. Moreover, I
had run out of allocated time to this task (due to actually having to fix some
bluez sbc encoder bugs in addition to doing just performance optimizations) and
had to wrap up this activity even before all the planned ARM NEON optimizations
had been implemented.

Fortunately I had some spare time on the last summer vacation to spend on bluez
sbc codec. It was a tough choice deciding between either fixing the decoder or
adding missing ARM NEON optimizations to the encoder. But in the end I decided
to get at least one thing done right (the encoder), especially considering that
it happens to be referred from several places as an example of ARM NEON
optimizations. And I just did not feel right about having clearly incomplete
stuff there :) So the encoder got more optimizations [1][2], and the decoder
remained the way it was (it is still in my low priority TODO list).

> As compared to Bluetooth's reference implementation the signal
> is twotimes too silent. We told me, that in the last step of calculating
> the PCM samples a 1 bit right shift was done too much.
> 
> A correct implementation for C# can be found at
> http://sourceforge.net/projects/sbcsharp/
> 
> Sorry, due to time limits I cannot provide you with a patch...

Even without looking at the changes (after all, it's not so easy without a
patch), I guess it's more like a band aid. The whole decoder needs to be
seriously reworked. At least dropping the current synthesis filter
completely and reimplementing it straight according to A2DP spec is
necessary. We can't verify whether the current filter is correct, and also
its low precision is likely masking the other bugs and deviations from the
spec. I had some notes about it earlier in [3].

I have no idea when this all might be done unless somebody steps up for this
task (Hagen?). Or unless I myself somehow get an opportunity to work on fixing
sbc decoder full time for at least a week (next summer vacation?).


1. http://marc.info/?l=linux-bluetooth&m=127781938327437&w=2
2. http://marc.info/?l=linux-bluetooth&m=127807359119577&w=2
3. http://marc.info/?l=linux-bluetooth&m=126106163120842&w=2

-- 
Best regards,
Siarhei Siamashka

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3] sbc: detect when bitpool has changed

[Brian Gix]

Hi Luiz,

On Wed, 2010-12-22 at 11:35 +0200, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
> 
> A2DP spec allow bitpool changes midstream which is why sbc configuration
> has a range of values for bitpool that the encoder can use and decoder
> must support.
> 
> Bitpool changes do not affect the state of encoder/decoder so they don't
> need to be reinitialize when this happens, so the impact is fairly small,
> what it does change is the frame length so encoders may change the
> bitpool to use the link more efficiently.
> ---
>  sbc/sbc.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/sbc/sbc.c b/sbc/sbc.c
> index a6391ae..77fcc5d 100644
> --- a/sbc/sbc.c
> +++ b/sbc/sbc.c
> @@ -978,6 +978,9 @@ ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
>  
>  		priv->frame.codesize = sbc_get_codesize(sbc);
>  		priv->frame.length = framelen;
> +	} else if (priv->frame.bitpool != sbc->bitpool) {
> +		priv->frame.length = framelen;
> +		sbc->bitpool = priv->frame.bitpool;
>  	}
>  
>  	if (!output)
> @@ -1050,6 +1053,9 @@ ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len,
>  
>  		sbc_encoder_init(&priv->enc_state, &priv->frame);
>  		priv->init = 1;
> +	} else if (priv->frame.bitpool != sbc->bitpool) {
> +		priv->frame.length = sbc_get_frame_length(sbc);
> +		priv->frame.bitpool = sbc->bitpool;
>  	}
>  
>  	/* input must be large enough to encode a complete frame */
> @@ -1120,7 +1126,7 @@ size_t sbc_get_frame_length(sbc_t *sbc)
>  	struct sbc_priv *priv;
>  
>  	priv = sbc->priv;
> -	if (priv->init)
> +	if (priv->init && priv->frame.bitpool == sbc->bitpool)
>  		return priv->frame.length;
>  
>  	subbands = sbc->subbands ? 8 : 4;

I think this patch is correct.

-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH v3] sbc: detect when bitpool has changed

[Johan Hedberg]

Hi Luiz,

On Wed, Dec 22, 2010, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
> 
> A2DP spec allow bitpool changes midstream which is why sbc configuration
> has a range of values for bitpool that the encoder can use and decoder
> must support.
> 
> Bitpool changes do not affect the state of encoder/decoder so they don't
> need to be reinitialize when this happens, so the impact is fairly small,
> what it does change is the frame length so encoders may change the
> bitpool to use the link more efficiently.
> ---
>  sbc/sbc.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)

The patch has been pushed upstream. Thanks. Could you also make sure
that it gets applied to PulseAudio's copy of libsbc?

Johan