Rockbox

Tasklist

FS#11511 - use codeclib's mdct in wmapro

Attached to Project: Rockbox
Opened by MohamedTarek (mtarek16) - Saturday, 31 July 2010, 12:42 GMT
Last edited by MohamedTarek (mtarek16) - Wednesday, 04 August 2010, 23:16 GMT
Task Type Patches
Category Codecs
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.6
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This patch modifies wma pro to use codeclib's mdct.
The input has to be scaled down by (block_size - 3). The output of the decoder when using codeclib's mdct had an opposite phase compared to the output of ffmpeg (or svn). so the patch compensated for that by switching the sign of the coeffs while extracting from the bitstream.

The output sounds good to me and the waveform looks good on audacity, but of course this is not enough analysis. I'd be grateful if someone could analyze it more and tell whether it's ok to commit or if the error would be a blocker.

This task depends upon

Closed by  MohamedTarek (mtarek16)
Wednesday, 04 August 2010, 23:16 GMT
Reason for closing:  Accepted
Additional comments about closing:  Committed in r27701
Comment by MohamedTarek (mtarek16) - Saturday, 31 July 2010, 13:52 GMT
I tested the patch on r27402 ... compared to how wmapro performs against ffmpeg with the current mdct and codeclib's, the error is not bad and I think I'll commit it, but I'll just wait a day or two for comments.
Comment by Andree Buschmann (Buschel) - Saturday, 31 July 2010, 17:27 GMT
2 comments and 1 question from my side.

Question: Is there any impact to speed?

Comment 1: You should add a comment to the "false" inversion of the sign at all places where you will change it. The comment should point out that this is mandatory to use another mdct implementation.

Comment 2: I am not sure whether it is a good idea to shift quant by (nbits-3). If I understand right nbits is 7..12, quant itself is derived from quant_tab[] which contains quite small values which you might shift to zero. It would be better imho if you would shift the result of the vector multiplication. In this case you would drop the fast asm implementation. Is there any other possibility or do you experience overflows within the mdct itself? If not, an idea could be to use wmapro_window() to scale down the pcm signal. You could simply do this by pre-shifting the window tables, as nbits describes a dedicated window size and window table.

Edit: If I read the irc logs correct, the maximum sample diff between the svn wav output and this patches output is 662. If this is true, the difference is _massive_. It might be not audible due to masking effects, but the standard quality gate for sample diff is +/- 1 (= -90dB noise). 662 equals -34 dB.
Comment by Andree Buschmann (Buschel) - Saturday, 31 July 2010, 19:27 GMT
Forget comment 2. This will not work as the mdct will overflow.

But nevertheless the attached patch uses another approach that should keep the precision of the output (not tested though). It does use a different scaling right at the beginning of the decoding process. Normally we shift decoded values by <<17. Now we shift by <<(17-nbits+3). As nbits-3 is always >17 we do not loose any precision in this step.
Comment by Andree Buschmann (Buschel) - Saturday, 31 July 2010, 20:37 GMT
v03:
- removes wmapro's own mdct from the build
- removes a now unused 32 KB array which will be a benefit for low memory targets
Comment by Andree Buschmann (Buschel) - Saturday, 31 July 2010, 21:14 GMT
v04 is sync'ed against r27644.

Decoding speed for wmapro_173k.wma on PP5022: 28.6 MHz (svn r27644: 29.9 MHz)
Comment by Andree Buschmann (Buschel) - Monday, 02 August 2010, 20:51 GMT
I have attached 2 png's with screenshots. Those show the diff-files of (svn-output) - (patched-output) with a amplification of +40 dB and with a x-scale that shows samples. As you can see the diff v04 clearly shows burst 2k-spikes and some noise-burst-like spikes with a distance of ~13,000 samples. The second diff shows the effect of a larger fract part: the 2k-spikes are reduced, the other noise-burst-spikes stay. All were measured with the file wmapro_173k.wma.

Question: Is maybe the svn-version itself buggy and adds such spikes and high noise-floor? In the attached diffs you cannot see whether svn or the patched version introduces the errors. Did you compare svn and patched version against a reference decoder?

Edit: the third file shows the diff for the decoder with larger fract part. Left channel is the diff, scaled by +40 dB, the right channel shows the signal. As you can see the noise-bursts in the diff are correlated to the signal itself.
Comment by MohamedTarek (mtarek16) - Tuesday, 03 August 2010, 23:09 GMT
With r27687, and then applying patches wmapro_mdct_v04 and wmapro_v08, the error is now quite nice : http://imagebin.ca/view/e_ixUuQC.html (max = 6e-5, mean = 7e-7 [absolute] ) . The error is measured against the original ffmpeg floating point decoder.
I still want to get rid of those spikes, but I think it can go in like this now, opinions ?

EDIT:
I attached a picture for a correct difference. The previous image showed the absolute error, in the attached image, it's clear the spikes are only in the negative the side, maybe there's some clipping (or use unsigned in place of signed) going on somewhere ?
Anyway now the mean error is 5.8e-8 max is 3e-5.
   diff3.jpg (78.4 KiB)
Comment by Andree Buschmann (Buschel) - Wednesday, 04 August 2010, 05:47 GMT
Great! The difference is now in the range of +/-1 (3e-5). The max difference is only -2 (6e-5). As the noise-burst spikes disappeared this means that the the former svn implementation had a bug. Therefor the new patched implementation is better than svn anyway. The few "spikes" of -2 will not be relevant.

I say: We should submit wmapro_v08 first, but need to either change the Coldfire asm in vector_fixmul_scalar() before submission or disable the relevant asm section for Coldfire asm. Afterwards wmapro_mdct_v04 can be submitted and we're fine :o)

Loading...