blocksds / sdk

Main BlocksDS SDK repository
https://blocksds.github.io/docs/
164 stars 11 forks source link

Audio timer value calculated incorrectly #42

Open AntonioND opened 1 year ago

AntonioND commented 1 year ago

Issue reported originally by @Gericom:

https://github.com/devkitPro/libnds/blob/89ac6cbbf51b70b7ce5679d3a8bd621b9fb910e6/include/nds/arm7/audio.h#L47

In audio.h a rounded version of the audio timer frequency is used, while it should actually be BUS_CLOCK/2 (16756991). This causes issues with streaming music if you run a timer that should have exactly the same frequency to keep track of blocks in a ring buffer as it will run out of sync. (a tempoarly solution is using this bad calculation on arm9 as well, but that's very lame)

Follow-up:

Maybe adding proper rounding would help a bit. Something like (16756991 + ((rate + 1) >> 1)) / rate. That way it is always the closest value it can be at least.

asiekierka commented 1 year ago

I recall there were issues with fixing that, but not the details involved.

asiekierka commented 1 year ago

Some research:

>>> 16756991 / int(16777216 / 22050)
22048.672368421052
>>> 16756991 / int(16756991 / 22050)
22077.722002635048

In the former case (current), a 22050Hz timer would run at 22048Hz (a little bit slower); in the latter case (after the fix), a 22050Hz timer would run at 22077Hz (a larger bit faster).

Adding proper rounding fixes this:

>>> 16756991 / int((16756991 + ((22050 + 1) >> 1)) / 22050)
22048.672368421052

It also leads to more accurate values in some cases:

>>> 16756991 / int(16777216 / 240)
239.71090766039626
>>> 16756991 / int((16756991 + ((240 + 1) >> 1)) / 240)
239.99929820541098

However, such a change still requires fairly extensive testing, IMO.

asiekierka commented 5 months ago

The bug aspect of this has been somewhat fixed; however, the issue of "creating an in-sync ring buffer between ARM7 and ARM9" remains open. Perhaps a helper calculator tool (like mtheall's VRAM allocation tool) would be the right approach here?

AntonioND commented 5 months ago

Yeah, that's a good idea.