Opened 12 years ago

Closed 12 years ago

Last modified 8 months ago

#8661 closed patch

ARM: ARM asm versions of sound rate conversion/mixing code

Reported by: SF/robinwatts Owned by: fingolfin
Priority: normal Component: Audio
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)

diff (31.5 KB) - added by SF/robinwatts 12 years ago.
ARM asm version of sound rate conversion/mixing code
diff.2 (44.0 KB) - added by SF/robinwatts 12 years ago.
Revised patch
ARM_ASM_Sound_Diffs.txt (44.7 KB) - added by SF/robinwatts 12 years ago.
ARM asm version of sound rate conversion/mixing code v4

Download all attachments as: .zip

Change History (24)

Changed 12 years ago by SF/robinwatts

Attachment: diff added

ARM asm version of sound rate conversion/mixing code

comment:1 Changed 12 years ago by sev-

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 Changed 12 years ago by SF/knakos

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).

Changed 12 years ago by SF/robinwatts

Attachment: diff.2 added

Revised patch

comment:3 Changed 12 years ago by SF/robinwatts

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 Changed 12 years ago by djwillis

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 Changed 12 years ago by SF/knakos

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 Changed 12 years ago by SF/robinwatts

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 Changed 12 years ago by djwillis

ARM_SOUND_RATE sounds far more sane :).
ARM_OPT was just an of the cuff idea.

comment:8 Changed 12 years ago by fingolfin

Isn't the DS port also using ARM? Wonder if this patch works there, too? Agent-q?

comment:9 Changed 12 years ago by fingolfin

Owner: set to agent-q

comment:10 Changed 12 years ago by fingolfin

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 Changed 12 years ago by agent-q

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 Changed 12 years ago by fingolfin

Owner: changed from agent-q to anotherguest

comment:13 Changed 12 years ago by fingolfin

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 Changed 12 years ago by agent-q

I've applied this patch to my local copy of the code, and it seems to work perfectly with the DS port.

comment:15 Changed 12 years ago by fingolfin

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:16 Changed 12 years ago by SF/robinwatts

File Added: ARM_ASM_Sound_Diffs.txt

comment:17 Changed 12 years ago by SF/robinwatts

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.

Changed 12 years ago by SF/robinwatts

Attachment: ARM_ASM_Sound_Diffs.txt added

ARM asm version of sound rate conversion/mixing code v4

comment:18 Changed 12 years ago by SF/robinwatts

oops, no... Better patch.
File Added: ARM_ASM_Sound_Diffs.txt

comment:19 Changed 12 years ago by fingolfin

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 Changed 12 years ago by fingolfin

Owner: changed from anotherguest to fingolfin
Status: newclosed

comment:21 Changed 8 months ago by digitall

Component: Audio
Note: See TracTickets for help on using tickets.