From: Siarhei Siamashka <[email protected]>
The "(((audio_sample << 1) | 1) << frame->scale_factor[ch][sb])"
part of expression
"frame->sb_sample[blk][ch][sb] =
(((audio_sample << 1) | 1) << frame->scale_factor[ch][sb]) /
levels[ch][sb] - (1 << frame->scale_factor[ch][sb])"
in "sbc_unpack_frame" function can sometimes overflow 32-bit signed int.
This problem can be reproduced by first using bitpool 128 and encoding
some random noise data, and then feeding it to sbc decoder. The obvious
thing to do would be to change "audio_sample" variable type to uint32_t.
However the problem is a little bit more complicated. According
to the section "12.6.2 Scale Factors" of A2DP spec:
scalefactor[ch][sb] = pow(2.0, (scale_factor[ch][sb] + 1))
And according to "12.6.4 Reconstruction of the Subband Samples":
sb_sample[blk][ch][sb] = scalefactor[ch][sb] *
((audio_sample[blk][ch][sb]*2.0+1.0) / levels[ch][sb]-1.0);
Hence the current code for calculating "sb_sample[blk][ch][sb]" is
not quite correct, because it loses one least significant bit of
sample data and passes twice smaller sample values to the synthesis
filter (the filter also deviates from the spec to compensate this).
This all has quite a noticeable impact on audio quality. Moreover,
it makes sense to keep a few extra bits of precision here in order
to minimize rounding errors. So the proposed patch introduces a new
SBCDEC_FIXED_EXTRA_BITS constant and uses uint64_t data type
for intermediate calculations in order to safeguard against
overflows. This patch intentionally addresses only the quality
issue, but performance can be also improved later (like replacing
division with multiplication by reciprocal).
Test for the difference of sbc encoding/decoding roundtrip vs.
the original audio file for joint stereo, bitpool 128, 8 subbands
and http://media.xiph.org/sintel/sintel-master-st.flac sample
demonstrates some quality improvement:
=== before ===
--- comparing original / sbc_encoder.exe + sbcdec ---
stddev: 4.64 PSNR: 82.97 bytes:170495708/170496000
=== after ===
--- comparing original / sbc_encoder.exe + sbcdec ---
stddev: 1.95 PSNR: 90.50 bytes:170495708/170496000
---
sbc/sbc.c | 11 +++++++----
sbc/sbc_tables.h | 6 ++++--
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/sbc/sbc.c b/sbc/sbc.c
index 77fcc5d..13f87c0 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -383,7 +383,7 @@ static int sbc_unpack_frame(const uint8_t *data, struct sbc_frame *frame,
int crc_pos = 0;
int32_t temp;
- int audio_sample;
+ uint32_t audio_sample;
int ch, sb, blk, bit; /* channel, subband, block and bit standard
counters */
int bits[2][8]; /* bits distribution */
@@ -494,6 +494,9 @@ static int sbc_unpack_frame(const uint8_t *data, struct sbc_frame *frame,
for (ch = 0; ch < frame->channels; ch++) {
for (sb = 0; sb < frame->subbands; sb++) {
if (levels[ch][sb] > 0) {
+ uint32_t shift =
+ frame->scale_factor[ch][sb] +
+ 1 + SBCDEC_FIXED_EXTRA_BITS;
audio_sample = 0;
for (bit = 0; bit < bits[ch][sb]; bit++) {
if (consumed > len * 8)
@@ -505,9 +508,9 @@ static int sbc_unpack_frame(const uint8_t *data, struct sbc_frame *frame,
consumed++;
}
- frame->sb_sample[blk][ch][sb] =
- (((audio_sample << 1) | 1) << frame->scale_factor[ch][sb]) /
- levels[ch][sb] - (1 << frame->scale_factor[ch][sb]);
+ frame->sb_sample[blk][ch][sb] = (int32_t)
+ (((((uint64_t)audio_sample << 1) | 1) << shift) /
+ levels[ch][sb]) - (1 << shift);
} else
frame->sb_sample[blk][ch][sb] = 0;
}
diff --git a/sbc/sbc_tables.h b/sbc/sbc_tables.h
index 28c0d54..25e24e6 100644
--- a/sbc/sbc_tables.h
+++ b/sbc/sbc_tables.h
@@ -40,11 +40,13 @@ static const int sbc_offset8[4][8] = {
{ -4, 0, 0, 0, 0, 0, 1, 2 }
};
+/* extra bits of precision for the synthesis filter input data */
+#define SBCDEC_FIXED_EXTRA_BITS 2
#define SS4(val) ASR(val, SCALE_SPROTO4_TBL)
#define SS8(val) ASR(val, SCALE_SPROTO8_TBL)
-#define SN4(val) ASR(val, SCALE_NPROTO4_TBL)
-#define SN8(val) ASR(val, SCALE_NPROTO8_TBL)
+#define SN4(val) ASR(val, SCALE_NPROTO4_TBL + 1 + SBCDEC_FIXED_EXTRA_BITS)
+#define SN8(val) ASR(val, SCALE_NPROTO8_TBL + 1 + SBCDEC_FIXED_EXTRA_BITS)
static const int32_t sbc_proto_4_40m0[] = {
SS4(0x00000000), SS4(0xffa6982f), SS4(0xfba93848), SS4(0x0456c7b8),
--
1.7.3.4
Hi Siarhei,
On Mon, Oct 17, 2011, Siarhei Siamashka wrote:
> The "(((audio_sample << 1) | 1) << frame->scale_factor[ch][sb])"
> part of expression
> "frame->sb_sample[blk][ch][sb] =
> (((audio_sample << 1) | 1) << frame->scale_factor[ch][sb]) /
> levels[ch][sb] - (1 << frame->scale_factor[ch][sb])"
> in "sbc_unpack_frame" function can sometimes overflow 32-bit signed int.
> This problem can be reproduced by first using bitpool 128 and encoding
> some random noise data, and then feeding it to sbc decoder. The obvious
> thing to do would be to change "audio_sample" variable type to uint32_t.
>
> However the problem is a little bit more complicated. According
> to the section "12.6.2 Scale Factors" of A2DP spec:
> scalefactor[ch][sb] = pow(2.0, (scale_factor[ch][sb] + 1))
>
> And according to "12.6.4 Reconstruction of the Subband Samples":
> sb_sample[blk][ch][sb] = scalefactor[ch][sb] *
> ((audio_sample[blk][ch][sb]*2.0+1.0) / levels[ch][sb]-1.0);
>
> Hence the current code for calculating "sb_sample[blk][ch][sb]" is
> not quite correct, because it loses one least significant bit of
> sample data and passes twice smaller sample values to the synthesis
> filter (the filter also deviates from the spec to compensate this).
> This all has quite a noticeable impact on audio quality. Moreover,
> it makes sense to keep a few extra bits of precision here in order
> to minimize rounding errors. So the proposed patch introduces a new
> SBCDEC_FIXED_EXTRA_BITS constant and uses uint64_t data type
> for intermediate calculations in order to safeguard against
> overflows. This patch intentionally addresses only the quality
> issue, but performance can be also improved later (like replacing
> division with multiplication by reciprocal).
>
> Test for the difference of sbc encoding/decoding roundtrip vs.
> the original audio file for joint stereo, bitpool 128, 8 subbands
> and http://media.xiph.org/sintel/sintel-master-st.flac sample
> demonstrates some quality improvement:
>
> === before ===
> --- comparing original / sbc_encoder.exe + sbcdec ---
> stddev: 4.64 PSNR: 82.97 bytes:170495708/170496000
> === after ===
> --- comparing original / sbc_encoder.exe + sbcdec ---
> stddev: 1.95 PSNR: 90.50 bytes:170495708/170496000
> ---
> sbc/sbc.c | 11 +++++++----
> sbc/sbc_tables.h | 6 ++++--
> 2 files changed, 11 insertions(+), 6 deletions(-)
Applied. Thanks!
Additionally I pushed another patch to try to reduce the massive
indentation in the nested for-loops. In general whenever you've got the
following kind of pattern in a loop:
if (some condition) {
lots
of
stuff
...
} else
just_a_single_statement;
You can avoid indentation for the larger branch by changing this to:
if (inverted condition) {
just_a_single_statement;
continue;
}
lots
of
stuff
...
And if you just have a long if-branch without an else part at all it
becomes even simpler:
if (inverted condition)
continue;
<what used to be within a long if-branch>
Johan