Opened 12 years ago

Closed 12 years ago

Last modified 8 months ago

#8659 closed patch

Optimisation for sound rate conversion code

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

diff (11.2 KB) - added by SF/robinwatts 12 years ago.
Diff to rate controller code
diff.2 (11.2 KB) - added by SF/robinwatts 12 years ago.
Revised version
SoundRateDiffs.txt (7.6 KB) - added by SF/robinwatts 12 years ago.
Sound Rate Diffs v3

Download all attachments as: .zip

Change History (21)

Changed 12 years ago by SF/robinwatts

Attachment: diff added

Diff to rate controller code

comment:1 Changed 12 years ago by fingolfin

Owner: set to fingolfin

comment:2 Changed 12 years ago by fingolfin

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

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

Changed 12 years ago by SF/robinwatts

Attachment: diff.2 added

Revised version

comment:4 Changed 12 years ago by SF/robinwatts

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

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

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

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

Sorry, I meant GP2X not DS! (Though it should apply to the DS too I'd have thought).

comment:9 Changed 12 years ago by fingolfin

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

Status: newpending

comment:11 Changed 12 years ago by SF/robinwatts

Status: pendingnew

comment:12 Changed 12 years ago by SF/robinwatts

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

Oops. Bad diffs. cygwin access problems. sorry.

Changed 12 years ago by SF/robinwatts

Attachment: SoundRateDiffs.txt added

Sound Rate Diffs v3

comment:14 Changed 12 years ago by SF/robinwatts

OK. Better diffs, tested to compile and work for WinCE.

File Added: SoundRateDiffs.txt

comment:15 Changed 12 years ago by fingolfin

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:16 Changed 12 years ago by sev-

Looks good to me as well. Please, commit it.

comment:17 Changed 12 years ago by fingolfin

Status: newclosed

comment:18 Changed 8 months ago by digitall

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