Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#11498 - Speed optimization to libwmapro

Attached to Project: Rockbox
Opened by Andree Buschmann (Buschel) - Monday, 26 July 2010, 08:26 GMT+1
Last edited by MohamedTarek (mtarek16) - Thursday, 05 August 2010, 20:56 GMT+1
Task Type Patches
Category Codecs
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Release 3.6
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This flyspray contains speed optimizations to libwmapro.

v01 introduces asm routines for multiplications. Furthermore it adds fixmul31 and fixmul16 as faster routines can be used for those.

Decoding speed for wmapro_173k.wma on PP5022: 43.1 MHz (svn: 49.6 MHz)

Test on Coldfire is needed.
   wmaprov_v01.patch (7.1 KiB) 
 apps/codecs/libwmapro/wmaprodec.c   |    6 -
 apps/codecs/libwmapro/wmapro_math.h |  147 +++++++++++++++++++++++++++++++++---
 2 files changed, 140 insertions(+), 13 deletions(-)

This task depends upon

Closed by  MohamedTarek (mtarek16)
Thursday, 05 August 2010, 20:56 GMT+1
Reason for closing:  Accepted
Additional comments about closing:  Committed in r27703.
Comment by Nils Wallménius (nls) - Monday, 26 July 2010, 13:36 GMT+1
For coldfire it probably makes sense to create two variants of vector_fixmul_scalar for the two different shifts used (16 and 24) since the fixmulshift function is pretty slow with 2 branches (othoh those branches could be eliminated here anyway since the shift is known to be less than 31).

edit: the coldifre fixmul32 function in libwma/wmafixed.h is doing the same as the fixmul16 in this patch but is 2 cycles faster and uses one register less
Comment by Andree Buschmann (Buschel) - Monday, 26 July 2010, 14:11 GMT+1
I did not see there are only used two variant of vector_fixmul_scalar(). You are right, we should definately define a 24 and a 16 bitshift variant of either vector_fixmul_scalar().

I will change Coldfire's fixmul16() to your proposed implementation. Is this also valid for other codecs implementations (e.g. atrac, mpc)? Or does this faster implementation use any knowledge about the codec's fixed point representation (e.g. 14 bits fract part)...
Comment by Nils Wallménius (nls) - Monday, 26 July 2010, 16:50 GMT+1
it does what the one in this patch does, takes the lower 16 bits of the high half of the 64bit result and make them the high 16 bits of the 32 bit final result and the high 16 bits of the lower half and make them the lower 16 bits of the final 32 bit result or (int32_t)((int64_t)x*y>>16), but you are right that it will not work for shifts other than 16.
Comment by Andree Buschmann (Buschel) - Monday, 26 July 2010, 20:21 GMT+1
Define fixmul24(), define two specific variants of vector_fixmul_scalar() (vector_fixmul_scalar_16 and vector_fixmul_scalar_24). Speed is same as measured with v01.
   wmapro_v02.patch (10.6 KiB) 
 apps/codecs/libwmapro/wmaprodec.c   |   24 ++--
 apps/codecs/libwmapro/wmapro_math.h |  200 +++++++++++++++++++++++++++++++++---
 2 files changed, 199 insertions(+), 25 deletions(-)

Comment by Andree Buschmann (Buschel) - Monday, 26 July 2010, 20:46 GMT+1
Define IRAM macros for different CPU types. Use IRAM for <WMAProDecodeCtx.tmp> and as many <WMAProChannelCtx.out> as fit to IRAM of the specific model.

Decoding speed for wmapro_173k.wma on PP5022: 37.5 MHz (svn: 49.6 MHz)
   wmapro_v03.patch (15.3 KiB) 
 apps/codecs/libwmapro/wmaprodec.c   |   50 +++++----
 apps/codecs/libwmapro/wmapro_math.h |  200 +++++++++++++++++++++++++++++++++---
 apps/codecs/libwmapro/wmaprodec.h   |   19 +++
 3 files changed, 239 insertions(+), 30 deletions(-)

Comment by Andree Buschmann (Buschel) - Monday, 26 July 2010, 21:56 GMT+1
Differ between
a) models with large IRAM -> put <WMAProDecodeCtx.tmp> to IRAM.
b) models with normal IRAM -> cannot put <WMAProDecodeCtx.tmp> to IRAM, but move several window tables to IRAM as second best option.

Edit: Histogram of window length for all rockbox wmapro samples is:
len 128 = 1600 calls
len 256 = 370 calls
len 512 = 370 calls
len 1024 = 960 calls
len 2048 = 5700 calls
len 4096 = 0 calls

Decoding speed for wmapro_173k.wma on PP5022: 37.3 MHz (svn: 49.6 MHz)

Edit 2:
mcf5249 wmapro_173k.wma; svn: 179.40MHz 69.22% realtime; wmapro_v04.patch: 117.63MHz 105.57% realtime
   wmapro_v04.patch (17.8 KiB) 
 apps/codecs/libwmapro/mdct_tables.c |    9 -
 apps/codecs/libwmapro/wmaprodec.c   |   52 ++++++---
 apps/codecs/libwmapro/wmapro_math.h |  200 +++++++++++++++++++++++++++++++++---
 apps/codecs/libwmapro/wmaprodec.h   |   26 ++++
 4 files changed, 252 insertions(+), 35 deletions(-)

Comment by MohamedTarek (mtarek16) - Monday, 26 July 2010, 23:18 GMT+1
This patch produces non interleaved output from wma pro decoder. Works directly on the buffers in wmaprodec.c instead of copying the data.
   wmapro_noninter.patch (6.2 KiB) 
 apps/codecs/wmapro.c              |   16 +++---------
 apps/codecs/libwmapro/wmaprodec.c |   49 ++++++++++++++++++--------------------
 apps/codecs/libwmapro/wmaprodec.h |    2 -
 3 files changed, 30 insertions(+), 37 deletions(-)

Comment by Andree Buschmann (Buschel) - Monday, 26 July 2010, 23:37 GMT+1
Minor change. We do not need vector_fixmul_scalar_16(), use vector_fixmul_scalar_24() instead. vector_fixmul_scalar_16() was only used to scale with sqrt(2), we can use higher precision for this rare use case.
   wmapro_v05.patch (17.2 KiB) 
 apps/codecs/libwmapro/mdct_tables.c |    9 -
 apps/codecs/libwmapro/wmaprodec.c   |   43 +++++---
 apps/codecs/libwmapro/wmapro_math.h |  192 +++++++++++++++++++++++++++++++++---
 apps/codecs/libwmapro/wmaprodec.h   |   26 ++++
 4 files changed, 240 insertions(+), 30 deletions(-)

Comment by Andree Buschmann (Buschel) - Monday, 26 July 2010, 23:43 GMT+1
Submitted wmapro_v05 with r27582.

Edit:
Submitted non-interleaving patch with r27583.
Comment by Andree Buschmann (Buschel) - Wednesday, 28 July 2010, 08:05 GMT+1
Add some comments and unroll loop in vector_fixmul_scalar(). Next step is to add some arm asm for vector_fixmul_scalar() and maybe vector_fixmul_window().

Decoding speed for wmapro_173k.wma on PP5022: 35.6 MHz (svn r27589: 35.9 MHz)
   wmapro_v06.patch (1.7 KiB) 
 apps/codecs/libwmapro/wmapro_math.h |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Comment by Andree Buschmann (Buschel) - Wednesday, 28 July 2010, 20:19 GMT+1
v06 submitted with r27595.
Comment by Andree Buschmann (Buschel) - Monday, 02 August 2010, 07:57 GMT+1
v07: Approach to use larger fract part for mdct and windowing. This preserves higher precision. The larger fract part is introduced via using fixmul16 instead of fixmul24 in the vector_fixmul_scalar() function. The asm for Coldfire has not been changed yet, especially Coldfire will speedup a bit through this change as well.

ToDo: Somebody to change the Coldfire asm and some cleanup of this change.
   wmapro_v07.patch (3.5 KiB) 
 apps/codecs/wmapro.c                |    2 +-
 apps/codecs/libwmapro/wmaprodec.c   |    5 +++--
 apps/codecs/libwmapro/wmapro_math.h |   24 ++++++++++++------------
 3 files changed, 16 insertions(+), 15 deletions(-)

Comment by Nils Wallménius (nls) - Monday, 02 August 2010, 11:06 GMT+1
Doing this with the current coldfire fixmul16 asm is pretty trivial but it think there's a better way: switching the emac to integer mode because we can then get the result with only one multiplication instead of the 2 used in the fixmul16 function, otoh, if this multiplication overflows 48 bits that will not work very well i think, do you know if this can overflow into the top 16 bits of the 64 bit result?
Comment by Andree Buschmann (Buschel) - Monday, 02 August 2010, 12:05 GMT+1
If would overflow into the upper 16 bits the result after fixmul16 would overflow anyway. The deeper I think about this change the less convinced I am it will not internally overflow...

1) Requantized values were seen to be <600 * (1<<shift), shift's worst case is for 128-size window -> shift = 17 - (8-3) = 12. => 600*(1<<12) = ~(1<<22)
2) quant (the scaling) was seen to go up to idx 106 => 10^(156/20) = ~(1<<26)
3) This results in (1<<22)*(1<<26) = (1<<48)

quite tight....
Comment by Andree Buschmann (Buschel) - Monday, 02 August 2010, 19:59 GMT+1
Some cleanup. Still the fixmul16-implementation must be merged to the Coldfire asm of vector_fixmul_scalar().
   wmapro_v08.patch (4 KiB) 
 apps/codecs/wmapro.c                |    2 +-
 apps/codecs/libwmapro/wmaprodec.c   |    6 +++---
 apps/codecs/libwmapro/wmapro_math.h |   24 ++++++++++++------------
 apps/codecs/libwmapro/wma.h         |    1 +
 4 files changed, 17 insertions(+), 16 deletions(-)

Comment by MohamedTarek (mtarek16) - Thursday, 05 August 2010, 01:27 GMT+1
v08 was committed in r27703. Thanks !
Task left open till coldfire vector_fixmul_scalar is done.
Comment by MohamedTarek (mtarek16) - Thursday, 05 August 2010, 07:52 GMT+1
here are my takes at the CF asm of fixmul_scalar : http://www.pastie.org/1076754
Comment by Nils Wallménius (nls) - Thursday, 05 August 2010, 12:57 GMT+1
Hi, i tested a version similar to your version 1 and also another approach switching the emac to unsigned integer mode and using that to get the lower half of the result but both turned out to be slower than the c loop using the fixmul16 macro so i think we can leave this as it is and close this task.
edit: my idea above about using the extension word of the emac to the the full result with one multiplication can not work.
Comment by MohamedTarek (mtarek16) - Thursday, 05 August 2010, 15:00 GMT+1
Is version 2 correct then ? If so, it would be faster than the c loop.
Comment by Nils Wallménius (nls) - Thursday, 05 August 2010, 15:20 GMT+1
no version 2 is not correct, we cannot get away with doing only one multiply because the multiplier yields only a 40 bit result, i was confused and thought it gave a 48 bit result but that is not the case
Comment by MohamedTarek (mtarek16) - Thursday, 05 August 2010, 16:06 GMT+1
just one thing iiuc, you're talking about integer mode, version 2 is intended to be in fractional mode.
Comment by MohamedTarek (mtarek16) - Thursday, 05 August 2010, 20:43 GMT+1
It's wrong .. totally wrong. I'll just remove the coldfire part.

Loading...