sbc: fix for overflow bug in quantization code

PATCH In review Siarhei Siamashka 4 participants 10 messages Dec 17, 2008 – Dec 29, 2008 Bluetooth drivers

lore

Share & tools

lore

Siarhei Siamashka

author

2008-12-17 20:43 UTC #0 lore raw

PATCH 1/1 sbc: fix for overflow bug in quantization code

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

Quoted 1 line

From 12088ac2053709bf89c3c84d47aef46b7f2da475 Mon Sep 17 00:00:00 2001

From: Siarhei Siamashka <[email protected]>
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 <[email protected]>

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

sbc/sbc.c +1 -1

index 7072673..6624921 100644
@@ -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];

Signature · 1 line

1.5.6.5

Marcel Holtmann

maintainer

2008-12-18 22:59 UTC #1 lore raw

Hi Siarhei,

Quoted 6 lines

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

Siarhei Siamashka

author

2008-12-19 21:25 UTC #2 lore raw

Quoted 6 lines · On Friday 19 December 2008 00:59:20 ext Marcel Holtmann wrote:

Hi Siarhei,
Quoted 4 lines
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.

Quoted 6 lines

Quoted 3 lines
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.

Signature · 2 lines

Best regards,
Siarhei Siamashka

Luiz Augusto von Dentz

maintainer

2008-12-19 22:23 UTC #3 lore raw

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

Brad Midgley

2008-12-19 22:51 UTC #4 lore raw

Luiz,

Quoted 3 lines

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

Siarhei Siamashka

author

2008-12-22 11:37 UTC #5 lore raw

Quoted 8 lines · On Saturday 20 December 2008 00:51:52 ext Brad Midgley wrote:

Luiz,
Quoted 3 lines
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.

Quoted 1 line

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.

Quoted 3 lines

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.

Signature · 2 lines

Best regards,
Siarhei Siamashka

Marcel Holtmann

maintainer

2008-12-22 12:29 UTC #6 lore raw

Hi Siarhei,

Quoted 69 lines

Quoted 6 lines
Quoted 3 lines
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.
Quoted 1 line
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.

Quoted 8 lines

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

Luiz Augusto von Dentz

maintainer

2008-12-22 17:50 UTC #7 lore raw

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.

Signature · 2 lines

Luiz Augusto von Dentz
Engenheiro de Computação

Siarhei Siamashka

author

2008-12-29 10:22 UTC #8 lore raw

In reply to Siarhei Siamashka

On Monday 22 December 2008 13:37:53 ext Siarhei Siamashka wrote:
[...]

Quoted 12 lines

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

[...]

Quoted 5 lines

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

Quoted 1 line

From c1e98a7130377c126b6cdf93812016eab9151729 Mon Sep 17 00:00:00 2001

From: Siarhei Siamashka <[email protected]>
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 <[email protected]>

---
 sbc/sbc.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

sbc/sbc.c +4 -4

index a3a3ac1..d3dcd9a 100644
@@ -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];

Signature · 1 line

1.5.6.5

Marcel Holtmann

maintainer

2008-12-29 10:33 UTC #9 lore raw

Hi Siarhei,

Quoted 13 lines

Quoted 12 lines
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
[...]

Quoted 8 lines

Quoted 5 lines
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