[PATCH] sbc: fix for overflow bug in quantization code

[PATCH] sbc: fix for overflow bug in quantization code

[Siarhei Siamashka]

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

Hello all,

This simple patch resolves one more SBC encoding quality issue. Now SBC
audio encoding quality should be fine in almost all cases after applying it
together with a new 8 subband filter.

The patch conflicts with the bitstream packing optimization, but I hope that
after all the encoding quality problems are fixed, we can get back to it.


Best regards,
Siarhei Siamashka

[-- Attachment #2: 0001-sbc-fix-for-overflow-bug-in-quantization-code.patch --]
[-- Type: text/x-diff, Size: 1194 bytes --]

>From 12088ac2053709bf89c3c84d47aef46b7f2da475 Mon Sep 17 00:00:00 2001
From: Siarhei Siamashka <siarhei.siamashka@nokia.com>
Date: Wed, 17 Dec 2008 22:32:11 +0200
Subject: [PATCH] sbc: fix for overflow bug in quantization code

The result of multiplication does not always fit into 32-bits. Using 64-bit
calculations helps to avoid overflows and sound quality problems in encoded
audio. Overflows are more likely to show up when using high values for
bitpool setting.

Signed-off-by: Siarhei Siamashka <siarhei.siamashka@nokia.com>
---
 sbc/sbc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sbc/sbc.c b/sbc/sbc.c
index 7072673..6624921 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -1085,7 +1085,7 @@ static int sbc_pack_frame(uint8_t *data, struct sbc_frame *frame, size_t len)
 			for (sb = 0; sb < frame->subbands; sb++) {
 				if (levels[ch][sb] > 0) {
 					audio_sample =
-						(uint16_t) ((((frame->sb_sample_f[blk][ch][sb]*levels[ch][sb]) >>
+						(uint16_t) (((((int64_t)frame->sb_sample_f[blk][ch][sb]*levels[ch][sb]) >>
 									(frame->scale_factor[ch][sb] + 1)) +
 								levels[ch][sb]) >> 1);
 					audio_sample <<= 16 - bits[ch][sb];
-- 
1.5.6.5

Re: [PATCH] sbc: fix for overflow bug in quantization code

[Marcel Holtmann]

Hi Siarhei,

> This simple patch resolves one more SBC encoding quality issue. Now SBC
> audio encoding quality should be fine in almost all cases after applying it
> together with a new 8 subband filter.
> 
> The patch conflicts with the bitstream packing optimization, but I hope that
> after all the encoding quality problems are fixed, we can get back to it.

both of your patches have been applied and pushed upstream. Please keep
developing against the upstream tree.

Regards

Marcel

Re: [PATCH] sbc: fix for overflow bug in quantization code

[Siarhei Siamashka]

On Friday 19 December 2008 00:59:20 ext Marcel Holtmann wrote:
> Hi Siarhei,
>
> > This simple patch resolves one more SBC encoding quality issue. Now SBC
> > audio encoding quality should be fine in almost all cases after applying
> > it together with a new 8 subband filter.
> >
> > The patch conflicts with the bitstream packing optimization, but I hope
> > that after all the encoding quality problems are fixed, we can get back
> > to it.
>
> both of your patches have been applied and pushed upstream. Please keep
> developing against the upstream tree.

Thanks.

-- 
Best regards,
Siarhei Siamashka

Re: [PATCH] sbc: fix for overflow bug in quantization code

[Luiz Augusto von Dentz]

Hi Siarhei, Jaska,

So what about the subband filters fixes and 16 bit fixed point, do you
thing it can be done? I left this out since the last patch was only
dealing with 8 subband and was 32 bit.

--=20
Luiz Augusto von Dentz
Engenheiro de Computa=E7=E3o

Re: [PATCH] sbc: fix for overflow bug in quantization code

[Brad Midgley]

Luiz,

> So what about the subband filters fixes and 16 bit fixed point, do you
> thing it can be done? I left this out since the last patch was only
> dealing with 8 subband and was 32 bit.

FYI, I made some testing changes some time ago from 32-bit to 16-bit
integers that didn't improve performance on arm at all... we shouldn't
sacrifice quality for no real improvement in performance.

The story may be different on a mips etc core, but I think we should
realize real performance improvements in order to justify lowering
fidelity.

Brad

Re: [PATCH] sbc: fix for overflow bug in quantization code

[Siarhei Siamashka]

On Saturday 20 December 2008 00:51:52 ext Brad Midgley wrote:
> Luiz,
>
> > So what about the subband filters fixes and 16 bit fixed point, do you
> > thing it can be done? I left this out since the last patch was only
> > dealing with 8 subband and was 32 bit.
>
> FYI, I made some testing changes some time ago from 32-bit to 16-bit
> integers that didn't improve performance on arm at all...

There are a few things to note:
1. Subband filter, while using a noticeable amount of CPU, is not the only
bottleneck in SBC encoder. Other parts of code also need to be improved in
order to see a major overall performance improvement as a result of
subband filter optimization
2. If you want to benefit from 16-bit math on ARM, you need to compile the
code with right -mcpu/-march settings. Fast single cycle 16-bit multiplication
instructions were introduced on ARM with armv5te instruction set and ARM9E
core. If you compile the code for armv4, you will hardly see any improvements
at all. Also current code explicitly uses 32-bit multiply-accumulate
instruction in inline assembly macro, preventing the compiler from using
16-bit instructions even for the cases where it could.

Performance improvement of using 16-bit multiplication instructions on ARM for
the subband filter function should be really high, if done right.

> we shouldn't sacrifice quality for no real improvement in performance.

With the patch from http://marc.info/?l=linux-bluetooth&m=122972547004294&w=2
anyone can already estimate the quality difference between using 16-bit fixed
point and 32-bit fixed point.

If we take http://media.xiph.org/BBB/BigBuckBunny-stereo.flac , convert it to
au format and use in testing, we get the following results (encoding was done
with bluez sbc encoder, decoding was done with some conformant reference
win32 decoder binary, original file was compared with the decoded result using
tiny_psnr tool).

First we can try to crank up bitrate to the very maximum and encode the file
with the following command line:
./sbcenc -j -S -b 255 BigBuckBunny-stereo.au >BigBuckBunny-stereo.sbc

bluez 16-bit fixed point:
stddev:    2.55 PSNR: 88.16 bytes:114491016/114491308

bluez 32-bit fixed point:
stddev:    1.09 PSNR: 95.56 bytes:114491016/114491308

reference encoder:
stddev:    1.09 PSNR: 95.56 bytes:114491016/114491308

Looks like a major difference and 16-bit fixed point looks bad in comparison,
right? But this is the artificial case when the compressed file is even bigger
than the original.

If we try to use more realistic settings similar to the recommended high
quality settings from SBC specification (Table 4.7):
./sbcenc -j -S -b 51 BigBuckBunny-stereo.au >BigBuckBunny-stereo.sbc

bluez 16-bit fixed point:
stddev:   43.82 PSNR: 63.48 bytes:114491016/114491308

bluez 32-bit fixed point:
stddev:   43.78 PSNR: 63.49 bytes:114491016/114491308

reference encoder:
stddev:   43.37 PSNR: 63.57 bytes:114491016/114491308

Now as you see, effects of having not very precise calculations are completely
eclipsed by the quantization and compression artefacts. I think that the 
difference is really negligible.

If anybody wants to help, results of blind ABX tests with a real A2DP headset
are very much welcome. But I suspect that nobody will be able to tell the
difference between 16-bit and 32-bit version.

PS. I still wonder why there is a loss when compared to reference encoder.
32-bit fixed point version should be even more precise than single precision
floating point. Maybe there could be another minor bug in the code, or it is
just a random deviation and there could be a win for other audio files.

> The story may be different on a mips etc core, but I think we should
> realize real performance improvements in order to justify lowering
> fidelity.

I don't know anything about mips at all.

-- 
Best regards,
Siarhei Siamashka

Re: [PATCH] sbc: fix for overflow bug in quantization code

[Marcel Holtmann]

Hi Siarhei,

> > > So what about the subband filters fixes and 16 bit fixed point, do you
> > > thing it can be done? I left this out since the last patch was only
> > > dealing with 8 subband and was 32 bit.
> >
> > FYI, I made some testing changes some time ago from 32-bit to 16-bit
> > integers that didn't improve performance on arm at all...
> 
> There are a few things to note:
> 1. Subband filter, while using a noticeable amount of CPU, is not the only
> bottleneck in SBC encoder. Other parts of code also need to be improved in
> order to see a major overall performance improvement as a result of
> subband filter optimization
> 2. If you want to benefit from 16-bit math on ARM, you need to compile the
> code with right -mcpu/-march settings. Fast single cycle 16-bit multiplication
> instructions were introduced on ARM with armv5te instruction set and ARM9E
> core. If you compile the code for armv4, you will hardly see any improvements
> at all. Also current code explicitly uses 32-bit multiply-accumulate
> instruction in inline assembly macro, preventing the compiler from using
> 16-bit instructions even for the cases where it could.
> 
> Performance improvement of using 16-bit multiplication instructions on ARM for
> the subband filter function should be really high, if done right.
> 
> > we shouldn't sacrifice quality for no real improvement in performance.
> 
> With the patch from http://marc.info/?l=linux-bluetooth&m=122972547004294&w=2
> anyone can already estimate the quality difference between using 16-bit fixed
> point and 32-bit fixed point.
> 
> If we take http://media.xiph.org/BBB/BigBuckBunny-stereo.flac , convert it to
> au format and use in testing, we get the following results (encoding was done
> with bluez sbc encoder, decoding was done with some conformant reference
> win32 decoder binary, original file was compared with the decoded result using
> tiny_psnr tool).
> 
> First we can try to crank up bitrate to the very maximum and encode the file
> with the following command line:
> ./sbcenc -j -S -b 255 BigBuckBunny-stereo.au >BigBuckBunny-stereo.sbc
> 
> bluez 16-bit fixed point:
> stddev:    2.55 PSNR: 88.16 bytes:114491016/114491308
> 
> bluez 32-bit fixed point:
> stddev:    1.09 PSNR: 95.56 bytes:114491016/114491308
> 
> reference encoder:
> stddev:    1.09 PSNR: 95.56 bytes:114491016/114491308
> 
> Looks like a major difference and 16-bit fixed point looks bad in comparison,
> right? But this is the artificial case when the compressed file is even bigger
> than the original.
> 
> If we try to use more realistic settings similar to the recommended high
> quality settings from SBC specification (Table 4.7):
> ./sbcenc -j -S -b 51 BigBuckBunny-stereo.au >BigBuckBunny-stereo.sbc
> 
> bluez 16-bit fixed point:
> stddev:   43.82 PSNR: 63.48 bytes:114491016/114491308
> 
> bluez 32-bit fixed point:
> stddev:   43.78 PSNR: 63.49 bytes:114491016/114491308
> 
> reference encoder:
> stddev:   43.37 PSNR: 63.57 bytes:114491016/114491308
> 
> Now as you see, effects of having not very precise calculations are completely
> eclipsed by the quantization and compression artefacts. I think that the 
> difference is really negligible.
> 
> If anybody wants to help, results of blind ABX tests with a real A2DP headset
> are very much welcome. But I suspect that nobody will be able to tell the
> difference between 16-bit and 32-bit version.
> 
> PS. I still wonder why there is a loss when compared to reference encoder.
> 32-bit fixed point version should be even more precise than single precision
> floating point. Maybe there could be another minor bug in the code, or it is
> just a random deviation and there could be a win for other audio files.

so it seems that best is to go for 16-bit fixed point and get us some
nice autoconf/automake magic to set the right GCC option to get the best
optimization. If you guys tell which options are needed for which
platform, I can easily make the autoconf/automake magic happen.

Personally I like the idea to let the compiler do the optimization for
the specific target CPU. That keeps the code simple.

Regards

Marcel

Re: [PATCH] sbc: fix for overflow bug in quantization code

[Luiz Augusto von Dentz]

Hi,

If I remember it correctly we announce our bitpool range 2-64 bits, so
if we could maintain a good quality in this scenarios there is no
reason to not adopt 16 bit.

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

Re: [PATCH] sbc: fix for overflow bug in quantization code

[Siarhei Siamashka]

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

On Monday 22 December 2008 13:37:53 ext Siarhei Siamashka wrote:
[...]
> If we try to use more realistic settings similar to the recommended high
> quality settings from SBC specification (Table 4.7):
> ./sbcenc -j -S -b 51 BigBuckBunny-stereo.au >BigBuckBunny-stereo.sbc
>
> bluez 16-bit fixed point:
> stddev:   43.82 PSNR: 63.48 bytes:114491016/114491308
>
> bluez 32-bit fixed point:
> stddev:   43.78 PSNR: 63.49 bytes:114491016/114491308
>
> reference encoder:
> stddev:   43.37 PSNR: 63.57 bytes:114491016/114491308
[...]
> PS. I still wonder why there is a loss when compared to reference encoder.
> 32-bit fixed point version should be even more precise than single
> precision floating point. Maybe there could be another minor bug in the
> code, or it is just a random deviation and there could be a win for other
> audio files.

Found what's the matter. It's a problem in subbands selection criteria for
joint-stereo. The following patch fixes it.

Best regards,
Siarhei Siamashka

[-- Attachment #2: 0001-Fixed-subbands-selection-for-joint-stereo-in-SBC-enc.patch --]
[-- Type: text/x-diff, Size: 1312 bytes --]

>From c1e98a7130377c126b6cdf93812016eab9151729 Mon Sep 17 00:00:00 2001
From: Siarhei Siamashka <siarhei.siamashka@nokia.com>
Date: Sat, 27 Dec 2008 19:36:14 +0200
Subject: [PATCH] Fixed subbands selection for joint-stereo in SBC encoder

Signed-off-by: Siarhei Siamashka <siarhei.siamashka@nokia.com>
---
 sbc/sbc.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sbc/sbc.c b/sbc/sbc.c
index a3a3ac1..d3dcd9a 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -1051,15 +1051,15 @@ static int sbc_pack_frame(uint8_t *data, struct sbc_frame *frame, size_t len)
 			}
 
 			/* decide whether to join this subband */
-			if ((scalefactor[0][sb] + scalefactor[1][sb]) >
-					(scalefactor_j[0] + scalefactor_j[1]) ) {
+			if ((frame->scale_factor[0][sb] +
+					frame->scale_factor[1][sb]) >
+					(scale_factor_j[0] +
+					scale_factor_j[1])) {
 				/* use joint stereo for this subband */
 				joint |= 1 << (frame->subbands - 1 - sb);
 				frame->joint |= 1 << sb;
 				frame->scale_factor[0][sb] = scale_factor_j[0];
 				frame->scale_factor[1][sb] = scale_factor_j[1];
-				scalefactor[0][sb] = scalefactor_j[0];
-				scalefactor[1][sb] = scalefactor_j[1];
 				for (blk = 0; blk < frame->blocks; blk++) {
 					frame->sb_sample_f[blk][0][sb] =
 							sb_sample_j[blk][0];
-- 
1.5.6.5

Re: [PATCH] sbc: fix for overflow bug in quantization code

[Marcel Holtmann]

Hi Siarhei,

> > If we try to use more realistic settings similar to the recommended high
> > quality settings from SBC specification (Table 4.7):
> > ./sbcenc -j -S -b 51 BigBuckBunny-stereo.au >BigBuckBunny-stereo.sbc
> >
> > bluez 16-bit fixed point:
> > stddev:   43.82 PSNR: 63.48 bytes:114491016/114491308
> >
> > bluez 32-bit fixed point:
> > stddev:   43.78 PSNR: 63.49 bytes:114491016/114491308
> >
> > reference encoder:
> > stddev:   43.37 PSNR: 63.57 bytes:114491016/114491308
> [...]
> > PS. I still wonder why there is a loss when compared to reference encoder.
> > 32-bit fixed point version should be even more precise than single
> > precision floating point. Maybe there could be another minor bug in the
> > code, or it is just a random deviation and there could be a win for other
> > audio files.
> 
> Found what's the matter. It's a problem in subbands selection criteria for
> joint-stereo. The following patch fixes it.

patch has been applied and pushed upstream. In the future, please leave
the Signed-off-by line out of it. That one is only a requirement for
kernel code and I never required it for BlueZ.

Regards

Marcel