#8659 closed patch
Optimisation for sound rate conversion code
| Reported by: | SF/robinwatts | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Audio |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
This patch introduces various optimisations in the sound rate conversion code. Although inspired mainly by the needs of the WinCE port, the optimisations in here apply to all platforms.
The first change is to the "LinearRateController" class; we amalgamate the opos and ipos members by storing "opos-ipos" in opos. This means one less member to have to keep reading/writing, and tests simply become against zero.
Next, we remove the loop for reading samples and replace it with a simple if. (This is partly a personal taste thing, and partly down to my distrust of compilers; I trust a compiler to remove an if when the templated variable says it's never needed much more than I trust it to remove a loop).
The logic forming the "out" values is also simplified a bit - again in an effort to ease the compilers task when optimising for the different instances of the template.
The next change is to both the "LinearRateController" and "CopyRateController" classes; we spot the case where vol_l and vol_r are both full, and handle this in a separate loop. In this case we can avoid the per sample multiplication/division.
The final change is to introduce a new rate control method that is used when the inrate is a direct multiple of the output rate. This "SimpleRateController" does exactly what the "LinearRateController" did, but dispenses with the need to interpolate.
The SimpleRateController is basically a copy of the LinearRateController code with the interpolation logic removed, and a few simple optimisations done.
My tests seem to indicate that the most games (DOTT, BS1, MI1, DIG, STS) either play audio at the intended rate, or an exact multiple of it, and a significant amount of that time appears to be at full volume.
Only BASS (out of the games I have tested) needs rate interpolation, so it makes sense to relieve the processor of that burden when it is unnecessary.
I hope this all makes sense and adequately describes the changes. If not, please let me know.
Robin
Ticket imported from: #1717419. Ticket imported from: patches/764.
Attachments (3)
Change History (21)
by , 18 years ago
comment:1 by , 18 years ago
| Owner: | set to |
|---|
comment:2 by , 18 years ago
I am still on the road, so I can't properly review this, will do when I am back home in 1-2 weeks. In the meantime, a guick glance revealed that this patch does not conform to our Code Formatting Conventions <http://wiki.scummvm.org/index.php/Code_Formatting_Conventions>.
comment:3 by , 18 years ago
Let me note here that in an effort to conserve cpu, the ce port sets the mixing rate to match the rate dictated by each game. This is a hint toward the core to skip any audio interpolation process and use raw output. Moreover, this has been the case for patch #[ 1595026 ] engine init/shutdown OSystem methods:
https://sourceforge.net/tracker/?func=detail&aid=1595026&group_id=37116&atid=418822
comment:4 by , 18 years ago
Here is a new version of the patch, hopefully that conforms with the coding standard. My apologies for getting it wrong before - if there are still problems, please just say and I'll do my best to fix them.
File Added: diff
comment:5 by , 18 years ago
To respond to the comment about the WinCE port setting the mixing rate to match the rate dictated by each game... the debugging output I got from DOTT, BS1, SS etc suggested that although many of the rate conversion instantiations were indeed "CopyRate" ones (i.e. direct copies, no interpolation done), there were times when the "SimpleRate" ones were invoked - presumably when a game tries to mix samples at different speeds?
comment:6 by , 17 years ago
(Small side remark, not very important: Somewhat nicer/more descriptive filenames would be helpful for future patches :-)
As it is I'd be interested to know: Did you test the two changes separately? I.e.: Does "copyrate" alone (w/o full volume detection) show a measurable perfomance improvement? Does the "max volume" detection alone cause a measurable speed improvement? Does the combined patch show a measureable improvement over either of the two?
All in all I am in favor of this patch. But it would be helpful to have some numbers to justify these specializations, which after all increase the code size quite a bit.
comment:7 by , 17 years ago
I haven't got any timing figures for this stuff at all, I'm afraid.
It was done initially for the WinCE port, so was aimed at simplifying the code in ways that experience has taught me would make life easier for ARM processors. I have since ARM coded these functions, and submitted them as a separate patch (number 1721826).
The optimisations definately *did* make for more efficient ARM code in the end, although the "max volume" one turned out to be a red herring and was removed in the final version.
I can't even give exact figures for how much difference the ARM code version makes (partly because my device seems to run everything full speed anyway, and partly because I never figured out a decent way of timing this stuff!. I am informed by John Willis, that it does make a significant difference to the performance of the DS port though.
The existence of the ARM version means that the immediate need for these optimisations has diminished somewhat - whether they are still of use or not depends now upon the needs of the non ARM ports. I suspect it's much less important when running on a PC, for instance.
Personally, I'd like to take these changes on because efficiency is never a bad thing - but I might be tempted to strip the "max volume" stuff out first to reduce the size overhead. But all that is based on gut feeling and no solid evidence. I'm quire prepared to bow to the wisdom of people that know more about the range of places it will be used.
comment:8 by , 17 years ago
Sorry, I meant GP2X not DS! (Though it should apply to the DS too I'd have thought).
comment:9 by , 17 years ago
Well, how about submitting a new revision of the patch sans the max volume code path, then I can review it again and possible check it in, maybe even for 0.10.0 -- if we do it *now* :-)
comment:10 by , 17 years ago
| Status: | new → pending |
|---|
comment:11 by , 17 years ago
| Status: | pending → new |
|---|
comment:12 by , 17 years ago
Revised Sound Rate Diffs - maximum volume path stripped out.
Untested, as the WinCE build doesn't currently use this. Will rejig the build, test and post a comment here.
File Added: SoundRateDiffs.txt
comment:14 by , 17 years ago
OK. Better diffs, tested to compile and work for WinCE.
File Added: SoundRateDiffs.txt
comment:15 by , 17 years ago
Looks good. A tiny detail, though: I used st_sample_t tmp0, tmp1; instead of st_sample_t tmp[2]; on purpose: Not all compilers (most notably, certain versions of GCC) are able to properly optimize the latter, due to the nasty aliasing rules of the C(++) standard, while virtually all can correctly optimize code involving the former. So, at least over here, the former results in slightly better code.
Other than that it looks good to me.
comment:17 by , 17 years ago
| Status: | new → closed |
|---|
comment:18 by , 6 years ago
| Component: | → Audio |
|---|
Diff to rate controller code