Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#7286 - correct DC-bias for output signals (dsp, test_codec)

Attached to Project: Rockbox
Opened by Andree Buschmann (Buschel) - Sunday, 10 June 2007, 18:49 GMT+2
Last edited by Thom Johansen (preglow) - Wednesday, 29 August 2007, 16:33 GMT+2
Task Type Patches
Category Music playback
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This patch corrects the DC-bias of the PCM-signal ouput. I came around this issue when working on accuracy comparison between rockbox and reference decoder for musepack.
In trunk all signals will have DC-bias of -1/65536 which is cause by truncation of samples with more than 16 significant bits. Solution is to add 1<<(scale-1) before doing the >>scale shift.
The patch is available for apps/plugins/test_codec.c and apps/dsp.c. For this reason the patch is not available for coldfire targets as these use assembler code and not the changed c-code.
   dc_bias_dsp.patch (1.4 KiB) 
 dsp.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

   dc_bias_testcodec.patch (1.8 KiB) 
 test_codec.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

This task depends upon

Closed by  Thom Johansen (preglow)
Wednesday, 29 August 2007, 16:33 GMT+2
Reason for closing:  Accepted
Comment by Michael Sevakis (MikeS) - Thursday, 14 June 2007, 01:01 GMT+2
Actually I did have coldfire in emac signed integer rounding mode at the final output stage (it uses emac to scale and perform clipping at once) but really don't think this tiny DC bias is much of a concern for playback so I just used signed int mode. All that's needed it to set the rounding flag back again and I guess it's for free anyway. Also, I was getting a head for implementing a lot of ARM dsp in assembly to see if it helps things which with careful attention to avoid memory stalls, it should.
Comment by Andree Buschmann (Buschel) - Thursday, 14 June 2007, 08:10 GMT+2
When comparing a reference decoded signal with the rockbox-output (unpatched), you will see, that there are two issues
a) DC-offset by small amount
b) white quantization noise at about -93dB average.

When using the DC-offset (which is nothing but rounding instead of truncating), the DC-offset has gone and the quantization
noise compared to reference decoder is about -107dB in average.

As all audio outputs are influenced, and if we're talking of high precision audio output, we should correct this last stage.
Comment by Michael Sevakis (MikeS) - Thursday, 14 June 2007, 09:34 GMT+2
On coldfire, it's multiplying by 2^(16-scale) to cause anything > 0x7fff or < -0x8000 to get saturated by hardware and then taking the most significant 16 bits for the output sample. As a result hardware rounding won't help for it (I just realized). But, it's just a matter of initializing the accumulators with 2^15 to have it perform essentially (2^(16-scale) * decoder_sample + 2^15) / (2^16) = output_sample. I think I reasoned that out correctly but I'm tired. :P
Comment by Andree Buschmann (Buschel) - Thursday, 14 June 2007, 17:55 GMT+2
As far as I understand the Coldfire solution, initializing accumulators with 2^15 is exactly what we need here.
Can you add such solution to this patch?
Comment by Michael Sevakis (MikeS) - Friday, 15 June 2007, 04:00 GMT+2
This adds the solution to the Colfire assembly in the output routines. No runtime comparisons performed however.
   dc_bias_dsp2.patch (8.3 KiB) 
 dsp.c    |   11 +++++++----
 dsp_cf.S |   40 ++++++++++++++++++++++++++++++----------
 2 files changed, 37 insertions(+), 14 deletions(-)

Comment by Andree Buschmann (Buschel) - Tuesday, 19 June 2007, 20:52 GMT+2
Merged all patches and diff'ed against current trunk (rev xyz). Now the final output stage (either dsp or test_codec) use same and correct rounding instead of simple truncation for all targets. This will give another few dB's in accuray and lower the quantization noise.
No runtime comparisons performed, nevertheless any effect in runtime should at least be irrelevant (for stereo, 44.1kHz -> additional 88200 adds/sec).
   dc_bias_v1.patch (10.1 KiB) 
 dsp.c                |   11 +++++++----
 dsp_cf.S             |   40 ++++++++++++++++++++++++++++++----------
 plugins/test_codec.c |   13 +++++++------
 3 files changed, 44 insertions(+), 20 deletions(-)

Comment by Andree Buschmann (Buschel) - Thursday, 02 August 2007, 20:22 GMT+2
Sync'ed against 14147
Comment by Andree Buschmann (Buschel) - Thursday, 02 August 2007, 20:23 GMT+2
Oops, now with patch file. Sync'ed against 14147
   dc_bias_v2.patch (10.1 KiB) 
 dsp.c                |   11 +++++++----
 dsp_cf.S             |   40 ++++++++++++++++++++++++++++++----------
 plugins/test_codec.c |   13 +++++++------
 3 files changed, 44 insertions(+), 20 deletions(-)

Comment by Matt M (Chesteta) - Wednesday, 08 August 2007, 07:56 GMT+2
I still recieve an error saying

$ patch -p0 < dc_bias_v2.patch
patching file apps/dsp_cf.S
patching file apps/plugins/test_codec.c
patching file apps/dsp.c
patch unexpectedly ends in middle of line

IDK what to do about this...
Comment by Andree Buschmann (Buschel) - Thursday, 09 August 2007, 21:21 GMT+2
Corrected. New line was missing at the end of file.
   dc_bias_v3.patch (10.1 KiB) 
 dsp.c                |   11 +++++++----
 dsp_cf.S             |   40 ++++++++++++++++++++++++++++++----------
 plugins/test_codec.c |   13 +++++++------
 3 files changed, 44 insertions(+), 20 deletions(-)

Comment by Thom Johansen (preglow) - Friday, 24 August 2007, 12:37 GMT+2
Looks to me like the dithering routine already has bias applied, and now gets it applied twice. Or do you see some issue here I don't see? I'll commit this soon.
Comment by Michael Sevakis (MikeS) - Saturday, 25 August 2007, 06:06 GMT+2
Dithering routines remove bias because they spead the quantization errors across samples, correct? I think I agree that the dither output shouldn't have additional rounding.
Comment by Andree Buschmann (Buschel) - Saturday, 25 August 2007, 12:02 GMT+2
Reviewed the changes again. Now I am also not too sure about the correctness of adding the dc_bias while dithering. The error-feedback is calculated from "output &= ~mask" (which is "(output>>scale)<<scale"). The additional dc_bias would therefor lead to false error-feedback.
Should have taken a deeper look into it before submitting this patch...
Comment by Thom Johansen (preglow) - Tuesday, 28 August 2007, 22:59 GMT+2
Any new thoughts on the correctness of the dithering routine? If not, I'll just remove the extra bias in dithering and commit.
Comment by Andree Buschmann (Buschel) - Wednesday, 29 August 2007, 09:13 GMT+2
There are no new thoughts. So, commit without extra bias in dithering.

Loading...