Discussion:
[RFC/PATCH] sbc: new filtering function for 8 band fixed point encoding
(too old to reply)
Jaska Uimonen
2008-11-28 13:35:37 UTC
Permalink
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.
Marcel Holtmann
2008-11-28 14:18:52 UTC
Permalink
Hi Jaska,
Post by Jaska Uimonen
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
Jelle de Jong
2008-11-28 14:24:59 UTC
Permalink
Post by Marcel Holtmann
Hi Jaska,
Post by Jaska Uimonen
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
Jaska Uimonen
2008-11-28 15:20:06 UTC
Permalink
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
Post by Jelle de Jong
Post by Marcel Holtmann
Hi Jaska,
Post by Jaska Uimonen
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
David Sainty
2008-11-28 18:13:08 UTC
Permalink
Post by Jaska Uimonen
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 ?
Jaska Uimonen
2008-11-28 15:14:35 UTC
Permalink
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
Post by Marcel Holtmann
Hi Jaska,
Post by Jaska Uimonen
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
Jim Carter
2008-12-02 20:15:20 UTC
Permalink
Post by Jaska Uimonen
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.
Post by Jaska Uimonen
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: jimc-***@public.gmane.org http://www.math.ucla.edu/~jimc (q.v. for PGP key)
Siarhei Siamashka
2008-12-12 17:14:48 UTC
Permalink
Post by Jaska Uimonen
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.
+/*
+ *
+ * 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
Brad Midgley
2008-12-12 19:19:20 UTC
Permalink
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
Post by Siarhei Siamashka
Post by Jaska Uimonen
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.
+/*
+ *
+ * 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
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Brad Midgley
Siarhei Siamashka
2008-12-15 12:54:19 UTC
Permalink
Post by Brad Midgley
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.
Post by Brad Midgley
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
Brad Midgley
2008-12-15 15:16:58 UTC
Permalink
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 :)
Post by Siarhei Siamashka
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
Siarhei Siamashka
2008-12-16 22:37:48 UTC
Permalink
Post by Brad Midgley
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).
Post by Brad Midgley
Post by Siarhei Siamashka
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
Jaska Uimonen
2008-12-17 08:16:34 UTC
Permalink
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
Post by Siarhei Siamashka
Post by Brad Midgley
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).
Post by Brad Midgley
Post by Siarhei Siamashka
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 Siamashka
2008-12-19 22:12:08 UTC
Permalink
Post by Siarhei Siamashka
Post by Brad Midgley
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).
Post by Brad Midgley
Post by Siarhei Siamashka
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
Siarhei Siamashka
2008-12-22 23:30:43 UTC
Permalink
On Saturday 20 December 2008 00:12:08 ext Siarhei Siamashka wrote:
[...]
Post by Siarhei Siamashka
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
Marcel Holtmann
2008-12-23 01:00:15 UTC
Permalink
Hi Siarhei,
Post by Siarhei Siamashka
Post by Siarhei Siamashka
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
J***@public.gmane.org
2008-12-23 08:20:25 UTC
Permalink
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. The problem
With this modification is that it also reduces the
Readability of the code.

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.

Br,
Jaska
-----Original Message-----
Sent: 23 December, 2008 03:00
To: Siamashka Siarhei (Nokia-D/Helsinki)
Cc: ext Brad Midgley; Uimonen Jaska (Nokia-D/Helsinki);
Subject: Re: [RFC/PATCH] sbc: new filtering function for 8
band fixed point encoding
Hi Siarhei,
Post by Siarhei Siamashka
Post by Siarhei Siamashka
We had a talk with Jaska Uimonen here, and now I'm kind of
delegated
Post by Siarhei Siamashka
Post by Siarhei Siamashka
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
Post by Siarhei Siamashka
Post by Siarhei Siamashka
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
Post by Siarhei Siamashka
Post by Siarhei Siamashka
to git repository? If I just continue adding changes to
the patch in
Post by Siarhei Siamashka
Post by Siarhei Siamashka
order to make it a faster, it will be also not so obvious
to see how
Post by Siarhei Siamashka
Post by Siarhei Siamashka
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
Post by Siarhei Siamashka
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
Siarhei Siamashka
2008-12-23 11:14:14 UTC
Permalink
Post by J***@public.gmane.org
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.
Post by J***@public.gmane.org
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.
Post by J***@public.gmane.org
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
Siarhei Siamashka
2008-12-23 10:45:14 UTC
Permalink
Post by Marcel Holtmann
Hi Siarhei,
Post by Siarhei Siamashka
Post by Siarhei Siamashka
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.
Post by Marcel Holtmann
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.
Post by Marcel Holtmann
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.
Post by Marcel Holtmann
+ 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.
Post by Marcel Holtmann
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
Marcel Holtmann
2008-12-23 11:48:08 UTC
Permalink
Hi Siarhei,
Post by Siarhei Siamashka
Post by Marcel Holtmann
Post by Siarhei Siamashka
Post by Siarhei Siamashka
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.
Post by Siarhei Siamashka
Post by Marcel Holtmann
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.
Post by Siarhei Siamashka
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
Marcel Holtmann
2008-12-29 10:00:37 UTC
Permalink
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
Marcel Holtmann
2008-12-29 12:03:01 UTC
Permalink
Hi Christian,
Post by Marcel Holtmann
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
Marcel Holtmann
2008-12-29 12:41:00 UTC
Permalink
Hi Christian,
Post by Marcel Holtmann
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 ;)
Post by Marcel Holtmann
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.
Post by Marcel Holtmann
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
Marcel Holtmann
2008-12-29 13:17:35 UTC
Permalink
Hi Christian,
Post by Marcel Holtmann
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
Siarhei Siamashka
2008-12-29 11:06:02 UTC
Permalink
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
Marcel Holtmann
2008-12-29 12:04:29 UTC
Permalink
Hi Siarhei,
Post by Siarhei Siamashka
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.
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
Siarhei Siamashka
2008-12-29 14:36:15 UTC
Permalink
Post by Marcel Holtmann
Hi Siarhei,
Post by Siarhei Siamashka
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.
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
Siarhei Siamashka
2008-12-29 15:04:03 UTC
Permalink
Post by Siarhei Siamashka
Post by Marcel Holtmann
Hi Siarhei,
Post by Siarhei Siamashka
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.
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 :)
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.
./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
./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
[...]
Post by Siarhei Siamashka
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

Siarhei Siamashka
2008-12-29 10:46:22 UTC
Permalink
On Tuesday 23 December 2008 12:45:14 ext Siarhei Siamashka wrote:

[coding style problems description skipped]
Post by Siarhei Siamashka
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
Marcel Holtmann
2008-12-29 11:56:11 UTC
Permalink
Hi Siarhei,
Post by Siarhei Siamashka
Post by Siarhei Siamashka
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
Continue reading on narkive:
Loading...