#8661 closed patch
ARM: ARM asm versions of sound rate conversion/mixing code
| Reported by: | SF/robinwatts | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Audio |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
This patch includes ARM assembly versions of the sound rate conversion/mixing code. The code is based on the optimised version of the C++ code submitted in patch 1717419 (without the separate loops for the 'no volume' case as it didn't actually end up saving anything).
This code has been tested on WinCE as much as I am able to - not all the cases are exercised by the games I have. I have posted a copy of an executable built with this code in on my webspace and invited people in the WinCE/PocketPC port forum to test it with their games.
The code conforms to the most 'rigourous' ATPCS standards that I know (except for returning to thumb mode), so should hopefully be useful for other ports too (Symbian, DS, PSP?), but I can't test this.
I *believe* that this patch conforms to your coding standards - if you spot anything I've messed up then I'll gladly attempt to fix it.
Rather than pollute your existing rate.cpp file with nasty ifdefs, I have put the revised version of the C++ in rate_arm.cpp and the assembly into rate_arm_asm.s. I hope this meets your standards of neatness.
Thanks,
Robin
Ticket imported from: #1721826. Ticket imported from: patches/766.
Attachments (3)
Change History (24)
by , 17 years ago
comment:1 by , 17 years ago
Of course, your module.mk patch is not correct. I.e. it should have something in manner:
ifndef ARM MODULE_OBS += rate.o else MODULE_OBS += rate_arm.o \ rate_arm_asm.o endif
Still there are inconsistencies in open braces, especially with else clauses. See SimpleRateConverter template implementation, flow() template, etc
Would be nice if you extend Max's comment in rate_arm.cpp with info about purpose of this file. The comment starts with "The code in this file is based on code..."
Your patch has some debug output leftovers. Look for fprintf(stderr.
Proper emty SVN keywords look like $URL$ and $Id$, see rate_arm_asm.s header
For some reason your sound/rate_arm_asm.s has executable bit set. But perhaps it is because you're under Windows.
comment:2 by , 17 years ago
Very efficient code.
Obviously I cannot review the validity of the entire patch, but it seems to run alright (Robin has a test build up in the forums).
The assembly code relies on the specific layout of structs which are defined in rate_arm.cpp so, if these are going to change in the future, the code will need some marginal changes.
The other tricky point is the c++ call, which is commented fine.
The module.mk can be worked around by directly including the arm specific files from the backend Makefile (also since other ports supposedly defin the ARM symbol, it may not be a good idea to include the rate converter across all arm based ports until explicitly tested).
comment:3 by , 17 years ago
New patch, that hopefully addresses sev's comments.
Knakos may well be right about it being best to include the ARM specific parts from the WinCE specific makefile, but I haven't done that, because I am not entirely sure how!
The assembly code does rely on the layout of structs - but those structs are only used in rate_arm.cpp, so people tweaking other ports shouldn't break this by accident.
If you spot anything else I've done wrong, please just say and I'll try to fix it.
File Added: diff
comment:4 by , 17 years ago
Robin,
Thanks for the updated patch. This also seems to work well on the GP2X port after a quick test as do a number of your other patches. I will do further testing later but would like to include this with the GP2X port.
Rather then consider adding these to the port specific makefiles could we consider a way of defining these ARM specific tweaks in a way that would not be used by default in ports that define ARM or __ARM__?
Maybe something as simple as ARM_OPT until these patches are well tested then moved to ARM? That way porters can turn on and off these features at build time and evaluate them rather then them getting lost in the WinCE port/backend.
Considering the number of ARM powered ScummVM backends it seems rude not to exploit these bits of code as much as possible.
John
comment:5 by , 17 years ago
Robin: You're right, we have to exclude rate.cpp from the build and include rate_arm.cpp. This has to be done at the module.mk level.
As John suggests, we can use another symbol (cryptically named OPT :-) ) to flag includion/exclusion.
comment:6 by , 17 years ago
Great! If any of the patches can help other ports that's all to the good.
I'm only set up here to build/test for WinCE stuff at the moment, but if any of the patches need to be prodded slightly for other ports please just tell me and I'll gladly do what I can to help.
Using a separate define (or maybe several defines?) seems a sensible way to go: On the offchance we find that some of the patches work on some platform and some don't, maybe we should consider using names like ARM_SOUND_RATE rather than just ARM_OPT ?
comment:7 by , 17 years ago
ARM_SOUND_RATE sounds far more sane :). ARM_OPT was just an of the cuff idea.
comment:8 by , 17 years ago
Isn't the DS port also using ARM? Wonder if this patch works there, too? Agent-q?
comment:9 by , 17 years ago
| Owner: | set to |
|---|
comment:10 by , 17 years ago
So it seems this patch likely won't make it into 0.10.0 anymore, as require work is not yet done :-( ?
At least it appears the following still have to be done: * Use "ARM_SOUND_RATE" * Finish the integration into the build system ?!? * Adapt the legal header in the new files to our new standard
comment:11 by , 17 years ago
Sorry, but these changes most likely won't make it into 0.10.0. There are enough critical issues affecting the build at the moment to delay the release past the release date without looking at this.
While the additions made here look great, I have to prioritise issues which prevent games from running before looking at this.
Sorry.
comment:12 by , 17 years ago
| Owner: | changed from to |
|---|
comment:13 by , 17 years ago
So, anotherguest, would the Symbian port potentially be able to benefit from this, too? Some Symbian devices at least use ARM CPUs, right?
As for the PSP, that uses a MIPS R4000 processor, so won't be affected by this.
comment:14 by , 17 years ago
I've applied this patch to my local copy of the code, and it seems to work perfectly with the DS port.
comment:15 by , 17 years ago
Great. So if somebody made the requests adjustments, we could apply this to the trunk right away (we already branched 0.10.0, so that would be safe to do).
comment:17 by , 17 years ago
The latest patch uses a define USE_ARM_SOUND_ASM to select the use of the ARM assembler.
I enable this by default for the WinCE build.
by , 17 years ago
| Attachment: | ARM_ASM_Sound_Diffs.txt added |
|---|
ARM asm version of sound rate conversion/mixing code v4
comment:19 by , 17 years ago
Excellent, added to the repository. I hope that other porters (agentq, djwillis) will be able to use this in their ports, too. Please make & commit any required changes to your resp. build systems.
comment:20 by , 17 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → closed |
comment:21 by , 5 years ago
| Component: | → Audio |
|---|
ARM asm version of sound rate conversion/mixing code