Hi All,
I did some testing on the current 8 band fixed point
encoding and it seems to attenuate frequencies below 800Hz
and above 18kHz. There might be some other stuff happening
also, because at least to me the bass seemed to lack some
"definition".
I didn't quite understand how the current tables are calculated
and how the filtering works so I wrote a new filtering function
and calculated new filter tables for it. It is written
using 16 bit fixed point without any platform specific optimizations.
I only unrolled some loops etc. I tried to follow the
flow chart in MPEG-1 annex c.
With this new filtering the low and high frequencies are there, but
I haven't done any more thorough testing. At least it sounds
a little bit better to my ears :)
br,
Jaska Uimonen
P.S. I could have done some discussion with the list members
about the current implementation, but I kind of got carried away.
Sorry about that.
Jaska Uimonen wrote:
> Hi,
>
> I did the git clone this morning and made the patch on top of that.
>
> Johan already came to tell me that I have whitespace instead
> of tabs in the changed parts. Sorry about that, I'll fix it.
> He said it will take 1 minutes to fix that but it was
> at least 1/2 hour with emacs :)
>
> br,
> Jaska
>
>
M-x tabify ?
Hi,
I did the git clone this morning and made the patch on top of that.
Johan already came to tell me that I have whitespace instead
of tabs in the changed parts. Sorry about that, I'll fix it.
He said it will take 1 minutes to fix that but it was
at least 1/2 hour with emacs :)
br,
Jaska
On Fri, 2008-11-28 at 15:24 +0100, ext Jelle de Jong wrote:
> Marcel Holtmann wrote:
> > Hi Jaska,
> >
> >> I did some testing on the current 8 band fixed point
> >> encoding and it seems to attenuate frequencies below 800Hz
> >> and above 18kHz. There might be some other stuff happening
> >> also, because at least to me the bass seemed to lack some
> >> "definition".
> >>
> >> I didn't quite understand how the current tables are calculated
> >> and how the filtering works so I wrote a new filtering function
> >> and calculated new filter tables for it. It is written
> >> using 16 bit fixed point without any platform specific optimizations.
> >> I only unrolled some loops etc. I tried to follow the
> >> flow chart in MPEG-1 annex c.
> >>
> >> With this new filtering the low and high frequencies are there, but
> >> I haven't done any more thorough testing. At least it sounds
> >> a little bit better to my ears :)
> >
> > thanks for looking at it. I am seriously lost when it comes to audio
> > codecs and my ears normally don't count for much.
> >
> > So do you think we should throw all away any you start over providing a
> > correct implementation with fixed point integer and then we start
> > optimizing step by step (while testing against SBC conformance) or how
> > should we continue. For sure we have to fix our codec.
> >
> > Regards
> >
> > Marcel
>
> Hi Jaska, Thank you so much for improving the codecs :-D it was on my
> long standing wish list. You want to receive a few Dutch stroopwafels
> for your efforts :-D
>
> Is the patch you provided working against the latest git? Marcel would
> you be willing to review the patch for code style, hidden issues etcetera.
>
> I would love to test this patch this weekend and apply it to the latest git.
>
> I am an heavy stereo bluetooth user and will notice glitches and quality
> distortions on my bluetooth speakers and headsets.
>
> Best regards,
>
> Jelle
Hi,
So in general I would just like to hear comments
about the quality on this one. So does it fix the
quality issues or is there still some problems.
If someone just has the measurement system ready and would
be able to run some sound through... This is all
16 bit fixed point and we have an option to go to 32 bit
if necessary.
About the optimizations... I myself use a vectorizing
compiler, which optimizes this kind of C-code very
well. So we could keep the "reference" fix point
in the code base (if it actually works now...) and
then do optimizations to different platform if
one wishes. But I'm really open to how you would like
to do it.
br,
Jaska
On Fri, 2008-11-28 at 15:18 +0100, ext Marcel Holtmann wrote:
> Hi Jaska,
>
> > I did some testing on the current 8 band fixed point
> > encoding and it seems to attenuate frequencies below 800Hz
> > and above 18kHz. There might be some other stuff happening
> > also, because at least to me the bass seemed to lack some
> > "definition".
> >
> > I didn't quite understand how the current tables are calculated
> > and how the filtering works so I wrote a new filtering function
> > and calculated new filter tables for it. It is written
> > using 16 bit fixed point without any platform specific optimizations.
> > I only unrolled some loops etc. I tried to follow the
> > flow chart in MPEG-1 annex c.
> >
> > With this new filtering the low and high frequencies are there, but
> > I haven't done any more thorough testing. At least it sounds
> > a little bit better to my ears :)
>
> thanks for looking at it. I am seriously lost when it comes to audio
> codecs and my ears normally don't count for much.
>
> So do you think we should throw all away any you start over providing a
> correct implementation with fixed point integer and then we start
> optimizing step by step (while testing against SBC conformance) or how
> should we continue. For sure we have to fix our codec.
>
> Regards
>
> Marcel
>
>
Marcel Holtmann wrote:
> Hi Jaska,
>
>> I did some testing on the current 8 band fixed point
>> encoding and it seems to attenuate frequencies below 800Hz
>> and above 18kHz. There might be some other stuff happening
>> also, because at least to me the bass seemed to lack some
>> "definition".
>>
>> I didn't quite understand how the current tables are calculated
>> and how the filtering works so I wrote a new filtering function
>> and calculated new filter tables for it. It is written
>> using 16 bit fixed point without any platform specific optimizations.
>> I only unrolled some loops etc. I tried to follow the
>> flow chart in MPEG-1 annex c.
>>
>> With this new filtering the low and high frequencies are there, but
>> I haven't done any more thorough testing. At least it sounds
>> a little bit better to my ears :)
>
> thanks for looking at it. I am seriously lost when it comes to audio
> codecs and my ears normally don't count for much.
>
> So do you think we should throw all away any you start over providing a
> correct implementation with fixed point integer and then we start
> optimizing step by step (while testing against SBC conformance) or how
> should we continue. For sure we have to fix our codec.
>
> Regards
>
> Marcel
Hi Jaska, Thank you so much for improving the codecs :-D it was on my
long standing wish list. You want to receive a few Dutch stroopwafels
for your efforts :-D
Is the patch you provided working against the latest git? Marcel would
you be willing to review the patch for code style, hidden issues etcetera.
I would love to test this patch this weekend and apply it to the latest git.
I am an heavy stereo bluetooth user and will notice glitches and quality
distortions on my bluetooth speakers and headsets.
Best regards,
Jelle
Hi Jaska,
> I did some testing on the current 8 band fixed point
> encoding and it seems to attenuate frequencies below 800Hz
> and above 18kHz. There might be some other stuff happening
> also, because at least to me the bass seemed to lack some
> "definition".
>
> I didn't quite understand how the current tables are calculated
> and how the filtering works so I wrote a new filtering function
> and calculated new filter tables for it. It is written
> using 16 bit fixed point without any platform specific optimizations.
> I only unrolled some loops etc. I tried to follow the
> flow chart in MPEG-1 annex c.
>
> With this new filtering the low and high frequencies are there, but
> I haven't done any more thorough testing. At least it sounds
> a little bit better to my ears :)
thanks for looking at it. I am seriously lost when it comes to audio
codecs and my ears normally don't count for much.
So do you think we should throw all away any you start over providing a
correct implementation with fixed point integer and then we start
optimizing step by step (while testing against SBC conformance) or how
should we continue. For sure we have to fix our codec.
Regards
Marcel
On Monday 29 December 2008 16:36:15 ext Siarhei Siamashka wrote:
> On Monday 29 December 2008 14:04:29 ext Marcel Holtmann wrote:
> > Hi Siarhei,
> >
> > > > I wrote this script to make some tests on the SBC decoder and encoder using
> > > > the recommended testing procedure with the reference bitstreams, the
> > > > reference codec and PEAQ.
> > > > http://net.cs.uni-tuebingen.de/html/nexgenvoip/html/
> > > > I got little bit confused with all the latest patches and whether they are
> > > > included or not. Just send me an email on which patch you like to have
> > > > tested. Running the tests just takes half an hour; me to answer my emails
> > > > maybe a bit longer.
> > >
> > > Please try the following patch (apply it to the latest git):
> > > http://marc.info/?l=linux-bluetooth&m=123054787830678&w=2
> > >
> > > You can try the patch "as is", and also with SBC_HIGH_PRECISION define
> > > uncommented. High precision mode is naturally more likely to pass the
> > > conformance tests.
> > >
> > > I used my own script for testing with 'tiny_psnr' tool for comparing original file
> > > before encoding and the final decoded result (it measures standard deviation
> > > and PSNR). It would be interesting to see how our results correlate.
> >
> > can you open source tiny_psnr an we merge it into the BlueZ source?
>
> It is already open source. This very simple tool is used in ffmpeg project
> regression tests (and is part of ffmpeg distribution). It does not do
> anything extraordinary, but just analyzes the difference between several
> audio files and gets standard deviation and PSNR statistics. I just did not
> feel like reinventing the wheel and used it for estimating quality :)
>
> I only use the following patch on top of it (it can do automatic shift detection):
> http://article.gmane.org/gmane.comp.video.ffmpeg.devel/74597
> I find this patch quite useful (especially as it turns out for SBC), but ffmpeg
> developers do not think so...
>
> A crude ruby script (sorry, I don't speak shell scripting language) is attached.
> I have been using it for testing audio quality.
>
> For example, here is the test of a high precision sbcenc build (bitpool=255):
>
> ./sbc_encode_test.rb BigBuckBunny-stereo.flac
> [2, 48000]
> ["-j -S -s8 -B16 -b255", "-j -l16 -n8 -r1569000"]
> --- comparing original / sbcenc + sbcdec ---
> stddev: 108.67 PSNR: 55.60 bytes:114519261/114520000
>
> --- comparing original / sbcenc + sbc_decoder.exe ---
> stddev: 1.09 PSNR: 95.56 bytes:114519260/114520000
>
> --- comparing original / sbc_encoder.exe + sbc_decoder.exe ---
> stddev: 1.09 PSNR: 95.56 bytes:114519260/114520000
>
> --- comparing sbcenc + sbc_decoder.exe / sbc_encoder.exe + sbc_decoder.exe
> stddev: 0.01 PSNR:130.99 bytes:114519552/114519552
>
> Test of a standard sbcenc build (bitpool=255):
>
> ./sbc_encode_test.rb BigBuckBunny-stereo.flac
> [2, 48000]
> ["-j -S -s8 -B16 -b255", "-j -l16 -n8 -r1569000"]
> --- comparing original / sbcenc + sbcdec ---
> stddev: 108.71 PSNR: 55.59 bytes:114519261/114520000
>
> --- comparing original / sbcenc + sbc_decoder.exe ---
> stddev: 2.07 PSNR: 89.98 bytes:114519260/114520000
>
> --- comparing original / sbc_encoder.exe + sbc_decoder.exe ---
> stddev: 1.09 PSNR: 95.56 bytes:114519260/114520000
>
> --- comparing sbcenc + sbc_decoder.exe / sbc_encoder.exe + sbc_decoder.exe
> stddev: 1.77 PSNR: 91.34 bytes:114519552/114519552
[...]
> By the way, 'sbcdec' seems to introduce quite a noticeable distortion and
> is orders of magnitude less precise than the encoder.
Please disregard this statement. I only tried to add support for sbcdec as the
last minute change (I did not pay any attention to the decoder audio quality
before today) and forgot to change its output to a normal WAV format
before comparing files. Sorry about that. A fixed script is attached and
updated results are below.
Test of a high precision sbcenc build (bitpool=255):
./sbc_encode_test.rb BigBuckBunny-stereo.flac
[2, 48000]
["-j -S -s8 -B16 -b255", "-j -l16 -n8 -r1569000"]
--- comparing original / sbcenc + sbcdec ---
stddev: 4.49 PSNR: 83.26 bytes:114519260/114520000
--- comparing original / sbcenc + sbc_decoder.exe ---
stddev: 1.09 PSNR: 95.56 bytes:114519260/114520000
--- comparing original / sbc_encoder.exe + sbc_decoder.exe ---
stddev: 1.09 PSNR: 95.56 bytes:114519260/114520000
--- comparing sbcenc + sbc_decoder.exe / sbc_encoder.exe + sbc_decoder.exe
stddev: 0.01 PSNR:130.99 bytes:114519552/114519552
Test of a standard sbcenc build (bitpool=255):
./sbc_encode_test.rb BigBuckBunny-stereo.flac
[2, 48000]
["-j -S -s8 -B16 -b255", "-j -l16 -n8 -r1569000"]
--- comparing original / sbcenc + sbcdec ---
stddev: 4.85 PSNR: 82.60 bytes:114519260/114520000
--- comparing original / sbcenc + sbc_decoder.exe ---
stddev: 2.07 PSNR: 89.98 bytes:114519260/114520000
--- comparing original / sbc_encoder.exe + sbc_decoder.exe ---
stddev: 1.09 PSNR: 95.56 bytes:114519260/114520000
--- comparing sbcenc + sbc_decoder.exe / sbc_encoder.exe + sbc_decoder.exe
stddev: 1.77 PSNR: 91.34 bytes:114519552/114519552
Still the sbcdec seems to be the major contributor to quality loss in all cases.
Best regards,
Siarhei Siamashka
On Monday 29 December 2008 14:04:29 ext Marcel Holtmann wrote:
> Hi Siarhei,
>
> > > I wrote this script to make some tests on the SBC decoder and encoder using
> > > the recommended testing procedure with the reference bitstreams, the
> > > reference codec and PEAQ.
> > > http://net.cs.uni-tuebingen.de/html/nexgenvoip/html/
> > > I got little bit confused with all the latest patches and whether they are
> > > included or not. Just send me an email on which patch you like to have
> > > tested. Running the tests just takes half an hour; me to answer my emails
> > > maybe a bit longer.
> >
> > Please try the following patch (apply it to the latest git):
> > http://marc.info/?l=linux-bluetooth&m=123054787830678&w=2
> >
> > You can try the patch "as is", and also with SBC_HIGH_PRECISION define
> > uncommented. High precision mode is naturally more likely to pass the
> > conformance tests.
> >
> > I used my own script for testing with 'tiny_psnr' tool for comparing original file
> > before encoding and the final decoded result (it measures standard deviation
> > and PSNR). It would be interesting to see how our results correlate.
>
> can you open source tiny_psnr an we merge it into the BlueZ source?
It is already open source. This very simple tool is used in ffmpeg project
regression tests (and is part of ffmpeg distribution). It does not do
anything extraordinary, but just analyzes the difference between several
audio files and gets standard deviation and PSNR statistics. I just did not
feel like reinventing the wheel and used it for estimating quality :)
I only use the following patch on top of it (it can do automatic shift detection):
http://article.gmane.org/gmane.comp.video.ffmpeg.devel/74597
I find this patch quite useful (especially as it turns out for SBC), but ffmpeg
developers do not think so...
A crude ruby script (sorry, I don't speak shell scripting language) is attached.
I have been using it for testing audio quality.
For example, here is the test of a high precision sbcenc build (bitpool=255):
./sbc_encode_test.rb BigBuckBunny-stereo.flac
[2, 48000]
["-j -S -s8 -B16 -b255", "-j -l16 -n8 -r1569000"]
--- comparing original / sbcenc + sbcdec ---
stddev: 108.67 PSNR: 55.60 bytes:114519261/114520000
--- comparing original / sbcenc + sbc_decoder.exe ---
stddev: 1.09 PSNR: 95.56 bytes:114519260/114520000
--- comparing original / sbc_encoder.exe + sbc_decoder.exe ---
stddev: 1.09 PSNR: 95.56 bytes:114519260/114520000
--- comparing sbcenc + sbc_decoder.exe / sbc_encoder.exe + sbc_decoder.exe
stddev: 0.01 PSNR:130.99 bytes:114519552/114519552
Test of a standard sbcenc build (bitpool=255):
./sbc_encode_test.rb BigBuckBunny-stereo.flac
[2, 48000]
["-j -S -s8 -B16 -b255", "-j -l16 -n8 -r1569000"]
--- comparing original / sbcenc + sbcdec ---
stddev: 108.71 PSNR: 55.59 bytes:114519261/114520000
--- comparing original / sbcenc + sbc_decoder.exe ---
stddev: 2.07 PSNR: 89.98 bytes:114519260/114520000
--- comparing original / sbc_encoder.exe + sbc_decoder.exe ---
stddev: 1.09 PSNR: 95.56 bytes:114519260/114520000
--- comparing sbcenc + sbc_decoder.exe / sbc_encoder.exe + sbc_decoder.exe
stddev: 1.77 PSNR: 91.34 bytes:114519552/114519552
Having high bitrate is useful for detecting bugs in analysis filter because
its contribution to quality loss is more visible in this configuration and
other factors play lesser role.
I tested a lot of various files and settings configurations. According to this
script, the quality of high precision sbcenc build matches the quality of
reference encoder pretty well. Standard 16-bit fixed point sbcenc build
introduces a very minor quality loss, noticeable only at extremely high
bitrates.
By the way, 'sbcdec' seems to introduce quite a noticeable distortion and
is orders of magnitude less precise than the encoder.
Of course it would be intertesting to see if the quality estimation done by
using tiny_psnr is adequate and if it can replace a 6000 EUR tool for this
particular purpose.
Best regards,
Siarhei Siamashka
Hi Christian,
> > The official files from the Bluetooth SIG and I don't know which options
> > would make them encode correctly. Just calling the reference encoder
> > with the filename makes it run into a busy loop. With wine and natively
> > running on Windows XP.
>
> No, the 28 official files from the Bluetooth SIG are already compressed. Use
> the decoder to get the WAV files (or click on my web page on the links in
> the first table).
there are also *.wav files in one of the test specification zip files.
Regards
Marcel
Hi Marcel,
> The official files from the Bluetooth SIG and I don't know which options
> would make them encode correctly. Just calling the reference encoder
> with the filename makes it run into a busy loop. With wine and natively
> running on Windows XP.
No, the 28 official files from the Bluetooth SIG are already compressed. Use
the decoder to get the WAV files (or click on my web page on the links in
the first table).
For testing the encoder, any kind of wav file can be considered. Usually, I
run my tests with about 20 test wav files used by the ITU-R. For my web
page, I only considered one stereo sound file (any other will do, too).
Greetings
Christian
Hi Christian,
> > I really prefer if we have an open source solution available to
> > everybody for the SBC conformance testing. If sbctester.c is not good
> > enough than we have to write one.
>
> Sbctester.c is perfect for the decoder. For the encoder, we might also take
> it or write something better to replace PEAQ.
I am all for that since we need to be able to pass SBC conformance with
pure open source tools. Otherwise it becomes painful for some people.
> BTW; then I will have spent 6000 EUR on buying one PEAQ license in vain ;-)
I know multiple better ways of spending that money ;)
> > I also don't mind using the reference
> > encoder/decoder and the reference files. Using wine is not optimal, but
> > I don't really mind it.
>
> Fine. But can it be added to the repository? Or shall a description be given
> on how to download it from the Bluetooth SIG web page.
No we can't add it. I would have to write something up on how to
download it and how testing is suppose to work.
> > Another question that I have is how you use the sbc_enc_test_01.wav and
> > sbc_enc_test_02.wav files with Windows reference encoder? It turns into
> > a busy loop for me.
>
> Which file do are you referring too? I usually compress the original.wav
> file (on the top of the second table). Which line options do you use for the
> encoder?
The official files from the Bluetooth SIG and I don't know which options
would make them encode correctly. Just calling the reference encoder
with the filename makes it run into a busy loop. With wine and natively
running on Windows XP.
Regards
Marcel
Hello Marcel,
> I really prefer if we have an open source solution available to
> everybody for the SBC conformance testing. If sbctester.c is not good
> enough than we have to write one.
Sbctester.c is perfect for the decoder. For the encoder, we might also take
it or write something better to replace PEAQ.
BTW; then I will have spent 6000 EUR on buying one PEAQ license in vain ;-)
> I also don't mind using the reference
> encoder/decoder and the reference files. Using wine is not optimal, but
> I don't really mind it.
Fine. But can it be added to the repository? Or shall a description be given
on how to download it from the Bluetooth SIG web page.
>
> Another question that I have is how you use the sbc_enc_test_01.wav and
> sbc_enc_test_02.wav files with Windows reference encoder? It turns into
> a busy loop for me.
Which file do are you referring too? I usually compress the original.wav
file (on the top of the second table). Which line options do you use for the
encoder?
Greetings
Christian
Hi Siarhei,
> > I wrote this script to make some tests on the SBC decoder and encoder using
> > the recommended testing procedure with the reference bitstreams, the
> > reference codec and PEAQ.
> > http://net.cs.uni-tuebingen.de/html/nexgenvoip/html/
> > I got little bit confused with all the latest patches and whether they are
> > included or not. Just send me an email on which patch you like to have
> > tested. Running the tests just takes half an hour; me to answer my emails
> > maybe a bit longer.
>
> Please try the following patch (apply it to the latest git):
> http://marc.info/?l=linux-bluetooth&m=123054787830678&w=2
>
> You can try the patch "as is", and also with SBC_HIGH_PRECISION define
> uncommented. High precision mode is naturally more likely to pass the
> conformance tests.
>
> I used my own script for testing with 'tiny_psnr' tool for comparing original file
> before encoding and the final decoded result (it measures standard deviation
> and PSNR). It would be interesting to see how our results correlate.
can you open source tiny_psnr an we merge it into the BlueZ source?
Regards
Marcel
Hi Christian,
> > I leave the patches to the guys that actually know what they are doing.
> >
> > So I like the test script, but I would really prefer if we can use
> > sbctester.c program to verify the results. What do you think?
>
> The sbctester.c works fine for the decoder and is used in the testing
> script. The testing of the encoder might also work with test procedure
> implemented in sbctester.c. However, the official Bluetooth testing spec
> requires that
> "9.2.2.1 SBC Encoder ... [E2] The subjective quality (measured by
> standardized way or by objective testing
> methods, see [6]and [7]) shall be equivalent to that of reference encoder."
> with " [6] Rec. ITU-R BS.1116-1, "METHODS FOR THE SUBJECTIVE ASSESSMENT OF
> SMALL IMPAIRMENTS IN AUDIO SYSTEMS INCLUDING MULTICHANNEL SOUND SYSTEMS",
> 1994-1997
> [7] Rec. ITU-R BS.1387-1, "METHOD FOR OBJECTIVE MEASUREMENTS OF PERCEIVED
> AUDIO QUALITY", 2001".
> Until the encoder works fine, we should use BS.1387-1 (PEAQ). If all errors
> have been fixed, then sbctester.c might can be used instead of ITU-R
> BS.1387-1. However, then, we might still need the reference implementations
> or some pretranslated wav and sbc files.
I really prefer if we have an open source solution available to
everybody for the SBC conformance testing. If sbctester.c is not good
enough than we have to write one. I also don't mind using the reference
encoder/decoder and the reference files. Using wine is not optimal, but
I don't really mind it.
Another question that I have is how you use the sbc_enc_test_01.wav and
sbc_enc_test_02.wav files with Windows reference encoder? It turns into
a busy loop for me.
Regards
Marcel
Hi Siarhei,
> > Right now I'm also doublechecking correctness of the code to ensure that
> > there are no overflows of other problems related to audio quality.
>
> Here is (hopefully) the final revision of patch. I tested it with all the possible files
> that I could find or generate myself and it seems to work fine in all cases.
>
> The code was reverted to use fabs macro. After all, looks like it is the right
> way to handle scale factors and adheres to the specification. The weird
> effects on sound quality related to the use of fabs and the modified macro
> which was tried as a replacement have been apparently the side effects of
> other bugs.
>
> Also I have to admit that the change from
> http://marc.info/?l=linux-bluetooth&m=122946440507726&w=2
> is not needed and the original patch was in fact correct. The output of
> quantizer is strictly a positive number (unless there are bugs or overflows).
> This was also changed back.
>
> Rounding for the values in the final analysis function output was removed
> (we already keep a lot of extra bits in output, so it does not matter for
> precision). Also the change of SCALE_OUT_BITS to 15 has a nice feature
> that the shifting of output values is not needed at all for 16-bit version
> (and naturally rounding is not needed here too), this should be good for
> performance.
I applied your patch to the upstream tree. Thanks for your work. I know
like to get feedback from other people if this helps us with the SBC
conformance testing and the audio quality.
Regards
Marcel
On Monday 29 December 2008 11:16:23 ext Christian Hoene wrote:
> Hello all,
>
> Merry Christmas!
>
> I wrote this script to make some tests on the SBC decoder and encoder using
> the recommended testing procedure with the reference bitstreams, the
> reference codec and PEAQ.
> http://net.cs.uni-tuebingen.de/html/nexgenvoip/html/
> I got little bit confused with all the latest patches and whether they are
> included or not. Just send me an email on which patch you like to have
> tested. Running the tests just takes half an hour; me to answer my emails
> maybe a bit longer.
Please try the following patch (apply it to the latest git):
http://marc.info/?l=linux-bluetooth&m=123054787830678&w=2
You can try the patch "as is", and also with SBC_HIGH_PRECISION define
uncommented. High precision mode is naturally more likely to pass the
conformance tests.
I used my own script for testing with 'tiny_psnr' tool for comparing original file
before encoding and the final decoded result (it measures standard deviation
and PSNR). It would be interesting to see how our results correlate.
According to my tests, encoder should now be OK with this patch. Decoder
is definitely in a better shape than the current encoder from git. But the
decoder also seems to have bugs which degrade quality significantly
in some cases.
Best regards,
Siarhei Siamashka
Hello Marcel,
> I leave the patches to the guys that actually know what they are doing.
>
> So I like the test script, but I would really prefer if we can use
> sbctester.c program to verify the results. What do you think?
The sbctester.c works fine for the decoder and is used in the testing
script. The testing of the encoder might also work with test procedure
implemented in sbctester.c. However, the official Bluetooth testing spec
requires that
"9.2.2.1 SBC Encoder ... [E2] The subjective quality (measured by
standardized way or by objective testing
methods, see [6]and [7]) shall be equivalent to that of reference encoder."
with " [6] Rec. ITU-R BS.1116-1, "METHODS FOR THE SUBJECTIVE ASSESSMENT OF
SMALL IMPAIRMENTS IN AUDIO SYSTEMS INCLUDING MULTICHANNEL SOUND SYSTEMS",
1994-1997
[7] Rec. ITU-R BS.1387-1, "METHOD FOR OBJECTIVE MEASUREMENTS OF PERCEIVED
AUDIO QUALITY", 2001".
Until the encoder works fine, we should use BS.1387-1 (PEAQ). If all errors
have been fixed, then sbctester.c might can be used instead of ITU-R
BS.1387-1. However, then, we might still need the reference implementations
or some pretranslated wav and sbc files.
Until the encoder is bug-fixed, it makes sense to work with this
intermediate solution.
With best regards
Christian
On Tuesday 23 December 2008 12:45:14 ext Siarhei Siamashka wrote:
[coding style problems description skipped]
> OK, I'll try to fix these.
> Right now I'm also doublechecking correctness of the code to ensure that
> there are no overflows of other problems related to audio quality.
Here is (hopefully) the final revision of patch. I tested it with all the possible files
that I could find or generate myself and it seems to work fine in all cases.
The code was reverted to use fabs macro. After all, looks like it is the right
way to handle scale factors and adheres to the specification. The weird
effects on sound quality related to the use of fabs and the modified macro
which was tried as a replacement have been apparently the side effects of
other bugs.
Also I have to admit that the change from
http://marc.info/?l=linux-bluetooth&m=122946440507726&w=2
is not needed and the original patch was in fact correct. The output of
quantizer is strictly a positive number (unless there are bugs or overflows).
This was also changed back.
Rounding for the values in the final analysis function output was removed
(we already keep a lot of extra bits in output, so it does not matter for
precision). Also the change of SCALE_OUT_BITS to 15 has a nice feature
that the shifting of output values is not needed at all for 16-bit version
(and naturally rounding is not needed here too), this should be good for
performance.
Best regards,
Siarhei Siamashka
Hi Christian,
> I wrote this script to make some tests on the SBC decoder and encoder using
> the recommended testing procedure with the reference bitstreams, the
> reference codec and PEAQ.
> http://net.cs.uni-tuebingen.de/html/nexgenvoip/html/
> I got little bit confused with all the latest patches and whether they are
> included or not. Just send me an email on which patch you like to have
> tested. Running the tests just takes half an hour; me to answer my emails
> maybe a bit longer.
I leave the patches to the guys that actually know what they are doing.
So I like the test script, but I would really prefer if we can use
sbctester.c program to verify the results. What do you think?
Regards
Marcel
Hello all,
Merry Christmas!
I wrote this script to make some tests on the SBC decoder and encoder using
the recommended testing procedure with the reference bitstreams, the
reference codec and PEAQ.
http://net.cs.uni-tuebingen.de/html/nexgenvoip/html/
I got little bit confused with all the latest patches and whether they are
included or not. Just send me an email on which patch you like to have
tested. Running the tests just takes half an hour; me to answer my emails
maybe a bit longer.
Greetings
Christian
Hi Siarhei,
> > > > We had a talk with Jaska Uimonen here, and now I'm kind of delegated to
> > > > finish the work on this filtering function for SBC encoder (including
> > > > the final addition of ARM assembly optimizations). He provided me with
> > > > his last variant of code, which contains some more optimizations to
> > > > reduce the number of operations and also loops unrolling. I will add
> > > > his changes to the patch on next iteration.
> > > >
> > > > Now the question is how to best integrate a fixed filtering function to
> > > > git repository? If I just continue adding changes to the patch in order
> > > > to make it a faster, it will be also not so obvious to see how we got
> > > > to these code transformations just from the commit log.
> > >
> > > Next iteration done. Added support for 4 subbands, number of arithmetic
> > > operations reduced (but without loop unrolling for better code
> > > readability), precision improved for both 16-bit and 32-bit fixed point,
> > > 'neginv' macro is now more portable and faster. The rest is in the code
> > > comments.
> >
> > I don't mind having patches as attachment, but this makes it hard to
> > review and comment on them.
>
> I thought that as long as the attachments are plain text and 'suggest
> automatic display' property is set, there should not be much problems
> reviewing them.
>
> I'm sorry for my incompetence in this part, but what do you recommend for
> posting patches? A link to some guide is sufficient.
>
> Well, it is good to always learn some new things.
the kernel contains good documentation about how to send inline patches.
However as I said, I don't mind that much since I can easily apply them,
but for commenting on patches inline is easier.
> > Especially when it comes to stuff like
> > coding style (since I have no ideas about the rest).
> >
> > + t1[0] = t1[1] = t1[2] = t1[3] =
> > + (FIXED_A)1 << (SBC_PROTO_FIXED4_SCALE-1);
> >
> > Should be like this: (FIXED_A) 1 << (SBC_PROTO_FIXED4_SCALE - 1);
>
> Do you have a reference to the coding style standard guide? This
> particular
> requirement for casts and a space seems quite unusual and I have never
> seen
> it before.
It is the kernel coding style. Check Greg KH's paper from OLS some years
ago.
> OK, I'll try to fix these. Getting rid of some spaces was done to try fitting
> some lines into 80 characters (that's also not always achieved yet).
This depends on the code. In normal code you could use continue and
break a lot, but within SBC this might not generate good binaries.
Don't try to omit whitespaces. Just don't.
Regards
Marcel
On Tuesday 23 December 2008 10:20:25 Uimonen Jaska (Nokia-D/Helsinki) wrote:
> Hi Guys,
>
> As Siarhei said, we had a talk last week about
> The optimizations to the code.
>
> I added the modifications to the code, which reduce
> The amount of operations quite a lot.
Yes, thank you very much for doing this part of work, I included these
modifications into the last revision of my patch. And of course, your
initial patch to fix the SBC quality issues was invaluable. I hope that you
will not be forgotten to be credited properly when the filtering functions
are complete and committed.
> The problem
> With this modification is that it also reduces the
> Readability of the code.
Yes, that's one of the reasons why I'm trying to post different revisions
here, so all the history of modifications can be tracked if somebody is
interested.
> So because of the redundancy in the cosine transform
> it is possible to reduce the number of variables
> and operations in the preceding filtering.
> This is very hard to explain with comments in the code
> (so you would write something like "here should be t[12], but
> The table goes only to 8 because of redundancy in the following
> Cosine transform). To really make the stuff
> Understandable one should write some small
> Wikipage or extensive comments to the code.
>
> So this is the path the previous code also took, but
> At some point there was a mistake. I really don't still
> Get how the anamatrix stuff was calculated, but as
> You see it takes time to reverse engineer :)
>
> But I suggest me and Siarhei clean the code internally
> And try really hard to follow the conventions. It starts
> To get little bit messy, because we both have many
> Versions of the code.
I think that my last revision of patch is more or less complete and needs
only minor tweaks and cosmetic changes. It's not too obfuscated yet, and
the logic still can be seen (hopefully).
I intentionally decided not to include loops unrolling part as it actually
makes code harder to optimize further (for example add SIMD optimizations).
The idea is to have some kind of "reference" implementation, which is
reasonably compact and simple and also configurable for having high
precision mode (very useful for testing purposes).
Implementations, optimized for various platforms can be derived from it:
1. Platforms where multiplications are expensive and you want to avoid as many
of them as possible (reintroduce 'anamatrix' table, have shifts and additions
instead of multiplications, etc)
2. Platforms where the total number of operations is desired to be kept at
minimum (tables need to have unused and duplicate elements removed in
order to reduce the number of operations for loading constants)
3. Platforms where SIMD optimizations are possible (the code and tables should
have a regular structure, suitable for vectorizing, some table elements may
have to be be reordered)
As one can see, these platform specific optimizations have somewhat
contradictory requirements. I tried to arrange the code in such a way, that
any of such further optimizations can be easily introduced based on it.
--
Best regards,
Siarhei Siamashka
On Tuesday 23 December 2008 03:00:15 ext Marcel Holtmann wrote:
> Hi Siarhei,
>
> > > We had a talk with Jaska Uimonen here, and now I'm kind of delegated to
> > > finish the work on this filtering function for SBC encoder (including
> > > the final addition of ARM assembly optimizations). He provided me with
> > > his last variant of code, which contains some more optimizations to
> > > reduce the number of operations and also loops unrolling. I will add
> > > his changes to the patch on next iteration.
> > >
> > > Now the question is how to best integrate a fixed filtering function to
> > > git repository? If I just continue adding changes to the patch in order
> > > to make it a faster, it will be also not so obvious to see how we got
> > > to these code transformations just from the commit log.
> >
> > Next iteration done. Added support for 4 subbands, number of arithmetic
> > operations reduced (but without loop unrolling for better code
> > readability), precision improved for both 16-bit and 32-bit fixed point,
> > 'neginv' macro is now more portable and faster. The rest is in the code
> > comments.
>
> I don't mind having patches as attachment, but this makes it hard to
> review and comment on them.
I thought that as long as the attachments are plain text and 'suggest
automatic display' property is set, there should not be much problems
reviewing them.
I'm sorry for my incompetence in this part, but what do you recommend for
posting patches? A link to some guide is sufficient.
Well, it is good to always learn some new things.
> Especially when it comes to stuff like
> coding style (since I have no ideas about the rest).
>
> + t1[0] = t1[1] = t1[2] = t1[3] =
> + (FIXED_A)1 << (SBC_PROTO_FIXED4_SCALE-1);
>
> Should be like this: (FIXED_A) 1 << (SBC_PROTO_FIXED4_SCALE - 1);
Do you have a reference to the coding style standard guide? This particular
requirement for casts and a space seems quite unusual and I have never seen
it before.
> Also do you need the (FIXED_A) cast?
Yes, it's good to have it for better portability. We need to make sure that
this bit is never shifted outside of the range of int type (constants have int
type by default). FIXED_A type can be 64-bit for high precision enabled build.
Even if SBC_FIXED8_EXTRA_BITS constant is currently set so that there is no
overflow, this kind of precaution is better to have in order not to have any
kind of unexpected nasty effect when experimenting with tuning various shift
values.
>
> + for (hop = 0; hop < 40; hop += 8) {
> + t1[0] += (FIXED_A)in[hop] * _sbc_proto_fixed4[hop];
>
> Same here. There has to be a space after every case.
>
> + t1[i] = (FIXED_A)1 <<
> (SBC_COS_TABLE_FIXED4_SCALE-1-SCALE_OUT_BITS);
>
> And between every operation there has to be a space: SCALE - 1 - SCALE.
> In this case I would actually put the - 1 at the end, but that is pure
> cosmetics and not a coding style violation.
This (SBC_COS_TABLE_FIXED4_SCALE-1-SCALE_OUT_BITS) expression can be
interpreted as ((SBC_COS_TABLE_FIXED4_SCALE - 1) - SCALE_OUT_BITS).
Putting -1 at the end is kind of obfuscation.
> Please fix all of these. There at least 8 or so.
>
> +#define neginv(x) ((-2 >> 1 == -1) ? \
> + ((((int32_t)(x)) >> 31) ^ (int32_t)(x)) : \
> + ((x >= 0) ? (x) : -(x)-1))
>
> Space after cast. Space before and after operator.
>
> +#ifdef SBC_HIGH_PRECISION
> +# define FIXED_A int64_t /* data type for fixed point accumulator */
> +# define FIXED_T int32_t /* data type for fixed point constants */
> +# define SBC_FIXED8_EXTRA_BITS 16
> +#else
> +# define FIXED_A int32_t /* data type for fixed point accumulator */
> +# define FIXED_T int16_t /* data type for fixed point constants */
> +# define SBC_FIXED8_EXTRA_BITS 0
> +#endif
>
> No space between # and define. I know that this is meant to improve
> readability, but I don't see it.
OK, I'll try to fix these. Getting rid of some spaces was done to try fitting
some lines into 80 characters (that's also not always achieved yet).
Right now I'm also doublechecking correctness of the code to ensure that there
are no overflows of other problems related to audio quality.
--
Best regards,
Siarhei Siamashka
Hi Guys,
As Siarhei said, we had a talk last week about=20
The optimizations to the code.=20
I added the modifications to the code, which reduce=20
The amount of operations quite a lot. The problem=20
With this modification is that it also reduces the=20
Readability of the code.=20
So because of the redundancy in the cosine transform=20
it is possible to reduce the number of variables=20
and operations in the preceding filtering.=20
This is very hard to explain with comments in the code=20
(so you would write something like "here should be t[12], but=20
The table goes only to 8 because of redundancy in the following=20
Cosine transform). To really make the stuff=20
Understandable one should write some small=20
Wikipage or extensive comments to the code.=20
So this is the path the previous code also took, but=20
At some point there was a mistake. I really don't still=20
Get how the anamatrix stuff was calculated, but as=20
You see it takes time to reverse engineer :)=20
But I suggest me and Siarhei clean the code internally=20
And try really hard to follow the conventions. It starts=20
To get little bit messy, because we both have many=20
Versions of the code.
Br,
Jaska
>-----Original Message-----
>From: ext Marcel Holtmann [mailto:[email protected]]=20
>Sent: 23 December, 2008 03:00
>To: Siamashka Siarhei (Nokia-D/Helsinki)
>Cc: ext Brad Midgley; Uimonen Jaska (Nokia-D/Helsinki);=20
>[email protected]
>Subject: Re: [RFC/PATCH] sbc: new filtering function for 8=20
>band fixed point encoding
>
>Hi Siarhei,
>
>> > We had a talk with Jaska Uimonen here, and now I'm kind of=20
>delegated=20
>> > to finish the work on this filtering function for SBC encoder=20
>> > (including the final addition of ARM assembly optimizations). He=20
>> > provided me with his last variant of code, which contains=20
>some more=20
>> > optimizations to reduce the number of operations and also loops=20
>> > unrolling. I will add his changes to the patch on next iteration.
>> >
>> > Now the question is how to best integrate a fixed=20
>filtering function=20
>> > to git repository? If I just continue adding changes to=20
>the patch in=20
>> > order to make it a faster, it will be also not so obvious=20
>to see how=20
>> > we got to these code transformations just from the commit log.
>>=20
>> Next iteration done. Added support for 4 subbands, number of=20
>> arithmetic operations reduced (but without loop unrolling for better=20
>> code readability), precision improved for both 16-bit and=20
>32-bit fixed=20
>> point, 'neginv' macro is now more portable and faster. The=20
>rest is in the code comments.
>
>I don't mind having patches as attachment, but this makes it=20
>hard to review and comment on them. Especially when it comes=20
>to stuff like coding style (since I have no ideas about the rest).
>
>+ t1[0] =3D t1[1] =3D t1[2] =3D t1[3] =3D
>+ (FIXED_A)1 << (SBC_PROTO_FIXED4_SCALE-1);
>
>Should be like this: (FIXED_A) 1 << (SBC_PROTO_FIXED4_SCALE - 1);
>
>Also do you need the (FIXED_A) cast?
>
>+ for (hop =3D 0; hop < 40; hop +=3D 8) {
>+ t1[0] +=3D (FIXED_A)in[hop] * _sbc_proto_fixed4[hop];
>
>Same here. There has to be a space after every case.
>
>+ t1[i] =3D (FIXED_A)1 <<=20
>+ (SBC_COS_TABLE_FIXED4_SCALE-1-SCALE_OUT_BITS);
>
>And between every operation there has to be a space: SCALE - 1 - SCALE.
>In this case I would actually put the - 1 at the end, but that=20
>is pure cosmetics and not a coding style violation.
>
>Please fix all of these. There at least 8 or so.
>
>+#define neginv(x) ((-2 >> 1 =3D=3D -1) ? \
>+ ((((int32_t)(x)) >> 31) ^ (int32_t)(x)) : \
>+ ((x >=3D 0) ? (x) : -(x)-1))
>
>Space after cast. Space before and after operator.
>
>+#ifdef SBC_HIGH_PRECISION
>+# define FIXED_A int64_t /* data type for fixed point=20
>accumulator */ #=20
>+define FIXED_T int32_t /* data type for fixed point constants */ #=20
>+define SBC_FIXED8_EXTRA_BITS 16 #else # define FIXED_A=20
>int32_t /* data=20
>+type for fixed point accumulator */ # define FIXED_T int16_t /* data=20
>+type for fixed point constants */ # define SBC_FIXED8_EXTRA_BITS 0=20
>+#endif
>
>No space between # and define. I know that this is meant to=20
>improve readability, but I don't see it.
>
>Regards
>
>Marcel
>
>
>
Hi Siarhei,
> > We had a talk with Jaska Uimonen here, and now I'm kind of delegated to
> > finish the work on this filtering function for SBC encoder (including the
> > final addition of ARM assembly optimizations). He provided me with his
> > last variant of code, which contains some more optimizations to reduce the
> > number of operations and also loops unrolling. I will add his changes to
> > the patch on next iteration.
> >
> > Now the question is how to best integrate a fixed filtering function to git
> > repository? If I just continue adding changes to the patch in order to make
> > it a faster, it will be also not so obvious to see how we got to these code
> > transformations just from the commit log.
>
> Next iteration done. Added support for 4 subbands, number of arithmetic
> operations reduced (but without loop unrolling for better code readability),
> precision improved for both 16-bit and 32-bit fixed point, 'neginv' macro is
> now more portable and faster. The rest is in the code comments.
I don't mind having patches as attachment, but this makes it hard to
review and comment on them. Especially when it comes to stuff like
coding style (since I have no ideas about the rest).
+ t1[0] = t1[1] = t1[2] = t1[3] =
+ (FIXED_A)1 << (SBC_PROTO_FIXED4_SCALE-1);
Should be like this: (FIXED_A) 1 << (SBC_PROTO_FIXED4_SCALE - 1);
Also do you need the (FIXED_A) cast?
+ for (hop = 0; hop < 40; hop += 8) {
+ t1[0] += (FIXED_A)in[hop] * _sbc_proto_fixed4[hop];
Same here. There has to be a space after every case.
+ t1[i] = (FIXED_A)1 << (SBC_COS_TABLE_FIXED4_SCALE-1-SCALE_OUT_BITS);
And between every operation there has to be a space: SCALE - 1 - SCALE.
In this case I would actually put the - 1 at the end, but that is pure
cosmetics and not a coding style violation.
Please fix all of these. There at least 8 or so.
+#define neginv(x) ((-2 >> 1 == -1) ? \
+ ((((int32_t)(x)) >> 31) ^ (int32_t)(x)) : \
+ ((x >= 0) ? (x) : -(x)-1))
Space after cast. Space before and after operator.
+#ifdef SBC_HIGH_PRECISION
+# define FIXED_A int64_t /* data type for fixed point accumulator */
+# define FIXED_T int32_t /* data type for fixed point constants */
+# define SBC_FIXED8_EXTRA_BITS 16
+#else
+# define FIXED_A int32_t /* data type for fixed point accumulator */
+# define FIXED_T int16_t /* data type for fixed point constants */
+# define SBC_FIXED8_EXTRA_BITS 0
+#endif
No space between # and define. I know that this is meant to improve
readability, but I don't see it.
Regards
Marcel
On Saturday 20 December 2008 00:12:08 ext Siarhei Siamashka wrote:
[...]
> We had a talk with Jaska Uimonen here, and now I'm kind of delegated to
> finish the work on this filtering function for SBC encoder (including the
> final addition of ARM assembly optimizations). He provided me with his
> last variant of code, which contains some more optimizations to reduce the
> number of operations and also loops unrolling. I will add his changes to
> the patch on next iteration.
>
> Now the question is how to best integrate a fixed filtering function to git
> repository? If I just continue adding changes to the patch in order to make
> it a faster, it will be also not so obvious to see how we got to these code
> transformations just from the commit log.
Next iteration done. Added support for 4 subbands, number of arithmetic
operations reduced (but without loop unrolling for better code readability),
precision improved for both 16-bit and 32-bit fixed point, 'neginv' macro is
now more portable and faster. The rest is in the code comments.
Best regards,
Siarhei Siamashka
On Wednesday 17 December 2008 00:37:48 ext Siarhei Siamashka wrote:
> On Monday 15 December 2008 17:16:58 ext Brad Midgley wrote:
> > I like your idea of using a macro with the original floating point
> > tables, as long as we know it is done at compile time, not runtime :)
>
> What about something like this modification to Jaska's patch? It contains
> floating point constants wrapped into a macro.
>
> This version is using 16-bit multiplications only (additional natural
> change would be just to convert 'sbc_encoder_state->' to int16_t because it
> does not need to be int32_t), which is good for performance for the
> platforms with fast 16-bit integer multiplication. But it is also flexible
> enough to be changed to use 32x32->64 multiplications just by replacing
> FIXED_A and FIXED_T types to int64_t and int32_t respectively (for better
> precision or experiments with conformance testing).
>
> > > Can anybody try to remember/explain what transformations were applied
> > > to the existing fixed point implementation?
> >
> > it was done by several people and the only record we have is in cvs.
> > (part of it is in the old btsco project's cvs)
>
> Regarding the code optimizations. Looking at the tables, It can be seen
> that 'cos_table_fixed_8[0+hop]' is always equal to
> 'cos_table_fixed_8[8+hop]'. The same is true for 'cos_table_fixed_8[1+hop]'
> and 'cos_table_fixed_8[7+hop]' So it is possible to join 't1[0] + t1[8]',
> 't1[1]+ t1[7]' and the other such pairs, effectively halving the number of
> counters. This looks very much like the optimization that was applied to
> the current fixed point code :)
>
> But now it would be very interesting to see if the conformance tests pass
> rate is better with the new filtering function.
Here is one more attempt at improving filtering function. Now I tried to get
the best possible audio quality (using 32-bit fixed point).
16-bit version of filtering function can be enabled by just commenting
out '#define SBC_HIGH_PRECISION' line
The improvements include fixing a problem in scalefactors processing code.
Here we don't want to use the absolute value just because it is possible to
encode more negative values than positive values with the same number
of bits:
- while (scalefactor[ch][sb] < fabs(frame->sb_sample_f[blk][ch][sb])) {
+ while ((scalefactor[ch][sb] << SCALE_OUT_BITS) <=
neginv(frame->sb_sample_f[blk][ch][sb])) {
Another quality improvement is achieved by keeping more bits in the output of
filtering function, thus avoiding unnecessary precision loss on quantizing
stage.
Both of these changes also naturally improve audio quality for the 16-bit
variant.
We had a talk with Jaska Uimonen here, and now I'm kind of delegated to finish
the work on this filtering function for SBC encoder (including the final
addition of ARM assembly optimizations). He provided me with his last variant
of code, which contains some more optimizations to reduce the number of
operations and also loops unrolling. I will add his changes to the patch on
next iteration.
Now the question is how to best integrate a fixed filtering function to git
repository? If I just continue adding changes to the patch in order to make it
a faster, it will be also not so obvious to see how we got to these code
transformations just from the commit log.
I intentionally keep posting work-in-progress variants just to keep track of
the history at least in this mailing list archive :)
As always, feedback is very much welcome.
Best regards,
Siarhei Siamashka
Hello All,
Sorry about the silence, I was away from the office for
couple of weeks.
Anyway here is the original patch modified to suit the
bluez coding conventions. There might still be something
(I'm quite bad with these issues...)
I also added Siarhei's rounding stuff to the filter
tables. Good point Siarhei!
Concerning Brad's comments I think the calculation
of tables should now be quite clear. There are
comments on the tables how to produce them in Octave and
the filtering function is very basic fixed point Q15
calculation.
I agree that including the original float values to the
code would probably be a good idea. Then we could just
use some macro at compile time to modify them to suitable
Q format and original values would always be there to
be viewed and compared to.
I would still be careful about the optimization of the
filtering, so that we don't obfuscate the code too much.
The filter and cosine table are quite small so we should
consider how much we save in computation. Anyway if the savings
are significant let's just add them there.
br,
Jaska Uimonen
On Wed, 2008-12-17 at 00:37 +0200, Siarhei Siamashka wrote:
> On Monday 15 December 2008 17:16:58 ext Brad Midgley wrote:
> > I like your idea of using a macro with the original floating point
> > tables, as long as we know it is done at compile time, not runtime :)
>
> What about something like this modification to Jaska's patch? It contains
> floating point constants wrapped into a macro.
>
> This version is using 16-bit multiplications only (additional natural change
> would be just to convert 'sbc_encoder_state->' to int16_t because it does not
> need to be int32_t), which is good for performance for the platforms with fast
> 16-bit integer multiplication. But it is also flexible enough to be changed to
> use 32x32->64 multiplications just by replacing FIXED_A and FIXED_T types
> to int64_t and int32_t respectively (for better precision or experiments with
> conformance testing).
>
> > > Can anybody try to remember/explain what transformations were applied to
> > > the existing fixed point implementation?
> >
> > it was done by several people and the only record we have is in cvs.
> > (part of it is in the old btsco project's cvs)
>
> Regarding the code optimizations. Looking at the tables, It can be seen that
> 'cos_table_fixed_8[0+hop]' is always equal to 'cos_table_fixed_8[8+hop]'.
> The same is true for 'cos_table_fixed_8[1+hop]' and 'cos_table_fixed_8[7+hop]'
> So it is possible to join 't1[0] + t1[8]', 't1[1]+ t1[7]' and the other such
> pairs, effectively halving the number of counters. This looks very much like
> the optimization that was applied to the current fixed point code :)
>
> But now it would be very interesting to see if the conformance tests pass
> rate is better with the new filtering function.
>
>
> Best regards,
> Siarhei Siamashka
On Monday 15 December 2008 17:16:58 ext Brad Midgley wrote:
> I like your idea of using a macro with the original floating point
> tables, as long as we know it is done at compile time, not runtime :)
What about something like this modification to Jaska's patch? It contains
floating point constants wrapped into a macro.
This version is using 16-bit multiplications only (additional natural change
would be just to convert 'sbc_encoder_state->' to int16_t because it does not
need to be int32_t), which is good for performance for the platforms with fast
16-bit integer multiplication. But it is also flexible enough to be changed to
use 32x32->64 multiplications just by replacing FIXED_A and FIXED_T types
to int64_t and int32_t respectively (for better precision or experiments with
conformance testing).
> > Can anybody try to remember/explain what transformations were applied to
> > the existing fixed point implementation?
>
> it was done by several people and the only record we have is in cvs.
> (part of it is in the old btsco project's cvs)
Regarding the code optimizations. Looking at the tables, It can be seen that
'cos_table_fixed_8[0+hop]' is always equal to 'cos_table_fixed_8[8+hop]'.
The same is true for 'cos_table_fixed_8[1+hop]' and 'cos_table_fixed_8[7+hop]'
So it is possible to join 't1[0] + t1[8]', 't1[1]+ t1[7]' and the other such
pairs, effectively halving the number of counters. This looks very much like
the optimization that was applied to the current fixed point code :)
But now it would be very interesting to see if the conformance tests pass
rate is better with the new filtering function.
Best regards,
Siarhei Siamashka
Siarhei
I like your idea of using a macro with the original floating point
tables, as long as we know it is done at compile time, not runtime :)
> Can anybody try to remember/explain what transformations were applied to
> the existing fixed point implementation?
it was done by several people and the only record we have is in cvs.
(part of it is in the old btsco project's cvs)
--
Brad Midgley
On Friday 12 December 2008 21:19:20 ext Brad Midgley wrote:
> Guys
>
> One mistake we made was not keeping track of what functions we were
> applying to the published tables as we worked things to look less and
> less like the pseudocode in the spec. So if you start again from one
> of the tables in the spec, keep a comment like
>
> /* ourtable(x) = (int32)(table1(x) << 16) */
This part can be probably done just by having a macro, something like
#define F_TO_Q15(x) (int16_t)((x>0) ? ((x)*(1<<15)+0.5) : ((x)*(1<<15)-0.5))
And then using plain floating point numbers from the SBC specification in the
table, wrapped into this macro. Though I wonder if it is possible to use such
conditional expression in the static table initializer list with all versions
of gcc/other compilers.
> Which in this case would mean that in ourtable we've shifted the
> original table1 float value left 16 bits and truncated it to an int32.
>
> I also combined tables or split tables to simplify our loop logic or
> eliminate operations; an explanation in a comment would have been
> appropriate.
Yes, any transformations or simplifications should be extensively commented.
So that it will be always possible to reproduce them or verify their
correctness.
Can anybody try to remember/explain what transformations were applied to
the existing fixed point implementation?
--
Best regards,
Siarhei Siamashka
Guys
One mistake we made was not keeping track of what functions we were
applying to the published tables as we worked things to look less and
less like the pseudocode in the spec. So if you start again from one
of the tables in the spec, keep a comment like
/* ourtable(x) = (int32)(table1(x) << 16) */
Which in this case would mean that in ourtable we've shifted the
original table1 float value left 16 bits and truncated it to an int32.
I also combined tables or split tables to simplify our loop logic or
eliminate operations; an explanation in a comment would have been
appropriate.
Brad
On Fri, Dec 12, 2008 at 10:14 AM, Siarhei Siamashka
<[email protected]> wrote:
> On Fri, 28 Nov 2008, Jaska Uimonen wrote:
>> I did some testing on the current 8 band fixed point
>> encoding and it seems to attenuate frequencies below 800Hz
>> and above 18kHz. There might be some other stuff happening
>> also, because at least to me the bass seemed to lack some
>> "definition".
>> I didn't quite understand how the current tables are calculated
>> and how the filtering works so I wrote a new filtering function
>> and calculated new filter tables for it. It is written
>> using 16 bit fixed point without any platform specific optimizations.
>> I only unrolled some loops etc. I tried to follow the
>> flow chart in MPEG-1 annex c.
>
>> +/*
>> + * to produce this Q15 format table:
>> + *
>> + * Get the filter coeffs from the spec and multiply them by 2^15.
>> + */
>> +static const signed short _sbc_proto_fixed8[80] = {
>> + 0, 5, 11, 18, 26, 37, 48, 58, 65, 68,
>> + 65, 52, 29, -5, -54, -114, 185, 263, 342, 417,
>> + 480, 521, 531, 501, 423, 290, 95, -161, -479, -855,
>> + -1280, -1742, 2228, 2719, 3197, 3643, 4039, 4366, 4612, 4764,
>> + 4815, 4764, 4612, 4366, 4039, 3643, 3197, 2719, -2228, -1742,
>> + -1280, -855, -479, -161, 95, 290, 423, 501, 531, 521,
>> + 480, 417, 342, 263, -185, -114, -54, -5, 29, 52,
>> + 65, 68, 65, 58, 48, 37, 26, 18, 11, 5
>> +};
>
> Just remembered to check this. Precision and audio quality should be a bit
> better if the original floating values are not truncated, but rounded when put
> into tables.
>
> For example, the fifth element is 26 in _sbc_proto_fixed8 table, but it was
> 26.998194372608 after multiplication and before conversion to integer.
> Using 27 would have been a bit more correct here.
>
> --
> Best regards,
> Siarhei Siamashka
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Brad Midgley
On Fri, 28 Nov 2008, Jaska Uimonen wrote:
> I did some testing on the current 8 band fixed point
> encoding and it seems to attenuate frequencies below 800Hz
> and above 18kHz. There might be some other stuff happening
> also, because at least to me the bass seemed to lack some
> "definition".
> I didn't quite understand how the current tables are calculated
> and how the filtering works so I wrote a new filtering function
> and calculated new filter tables for it. It is written
> using 16 bit fixed point without any platform specific optimizations.
> I only unrolled some loops etc. I tried to follow the
> flow chart in MPEG-1 annex c.
> +/*
> + * to produce this Q15 format table:
> + *
> + * Get the filter coeffs from the spec and multiply them by 2^15.
> + */
> +static const signed short _sbc_proto_fixed8[80] = {
> + 0, 5, 11, 18, 26, 37, 48, 58, 65, 68,
> + 65, 52, 29, -5, -54, -114, 185, 263, 342, 417,
> + 480, 521, 531, 501, 423, 290, 95, -161, -479, -855,
> + -1280, -1742, 2228, 2719, 3197, 3643, 4039, 4366, 4612, 4764,
> + 4815, 4764, 4612, 4366, 4039, 3643, 3197, 2719, -2228, -1742,
> + -1280, -855, -479, -161, 95, 290, 423, 501, 531, 521,
> + 480, 417, 342, 263, -185, -114, -54, -5, 29, 52,
> + 65, 68, 65, 58, 48, 37, 26, 18, 11, 5
> +};
Just remembered to check this. Precision and audio quality should be a bit
better if the original floating values are not truncated, but rounded when put
into tables.
For example, the fifth element is 26 in _sbc_proto_fixed8 table, but it was
26.998194372608 after multiplication and before conversion to integer.
Using 27 would have been a bit more correct here.
--
Best regards,
Siarhei Siamashka
On Fri, 28 Nov 2008, Jaska Uimonen wrote:
> I did some testing on the current 8 band fixed point
> encoding and it seems to attenuate frequencies below 800Hz
> and above 18kHz. There might be some other stuff happening
> also, because at least to me the bass seemed to lack some
> "definition".
I have the same subjective impression: on (inexpensive) Motorola HT-820
phones, A2DP is noticeably anemic compared to the same phones with the
provided wire.
> I didn't quite understand how the current tables are calculated
> and how the filtering works so I wrote a new filtering function
> and calculated new filter tables for it. It is written
> using 16 bit fixed point without any platform specific optimizations.
> I only unrolled some loops etc. I tried to follow the
> flow chart in MPEG-1 annex c.
This is very exciting! But it's been about 1.5 years since I read the A2DP
spec so I can't remember: does the headphone require standard band
breakpoints? Or are the breakpoints implicit in the encoding, so if you
adjust them to make better use of the Bluetooth bandwidth, the headphone
will automatically go along?
James F. Carter Voice 310 825 2897 FAX 310 206 6673
UCLA-Mathnet; 6115 MSA; 405 Hilgard Ave.; Los Angeles, CA, USA 90095-1555
Email: [email protected] http://www.math.ucla.edu/~jimc (q.v. for PGP key)
Hallo Marcel,
Good news. We can use the sbctester for conformance testing of the encoder
using the procedure in my testing.sh script.
http://net.cs.uni-tuebingen.de/html/nexgenvoip/html/testing.sh
Only for minor optimizations PEAQ might be a benefit.
Greetings
Christian
> -----Original Message-----
> From: Marcel Holtmann [mailto:[email protected]]
> Sent: Monday, December 29, 2008 2:18 PM
> To: [email protected]
> Cc: [email protected]
> Subject: RE: Testing SBC filtering functions
>
> Hi Christian,
>
> > > The official files from the Bluetooth SIG and I don't know which
options
> > > would make them encode correctly. Just calling the reference encoder
> > > with the filename makes it run into a busy loop. With wine and
natively
> > > running on Windows XP.
> >
> > No, the 28 official files from the Bluetooth SIG are already compressed.
Use
> > the decoder to get the WAV files (or click on my web page on the links
in
> > the first table).
>
> there are also *.wav files in one of the test specification zip files.
>
> Regards
>
> Marcel