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 -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
