2008-12-17 20:43:27

by Siarhei Siamashka

[permalink] [raw]
Subject: [PATCH] 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


Attachments:
(No filename) (384.00 B)
0001-sbc-fix-for-overflow-bug-in-quantization-code.patch (1.17 kB)
Download all attachments

2008-12-29 10:33:50

by Marcel Holtmann

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

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



2008-12-29 10:22:00

by Siarhei Siamashka

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

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


Attachments:
(No filename) (993.00 B)
0001-Fixed-subbands-selection-for-joint-stereo-in-SBC-enc.patch (1.28 kB)
Download all attachments

2008-12-22 17:50:51

by Luiz Augusto von Dentz

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

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

2008-12-22 12:29:03

by Marcel Holtmann

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

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



2008-12-22 11:37:53

by Siarhei Siamashka

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

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

2008-12-19 22:51:52

by Brad Midgley

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

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

2008-12-19 22:23:00

by Luiz Augusto von Dentz

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

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

2008-12-19 21:25:46

by Siarhei Siamashka

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

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

2008-12-18 22:59:20

by Marcel Holtmann

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

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