Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AudioGeneratorMOD - scratches/noise due to integer overflow #474

Closed
softhack007 opened this issue Dec 29, 2021 · 5 comments
Closed

AudioGeneratorMOD - scratches/noise due to integer overflow #474

softhack007 opened this issue Dec 29, 2021 · 5 comments

Comments

@softhack007
Copy link
Contributor

Hi,

Thanks for creating this audio library, which even allowed me to dig out some "amiga MODs" and play them on a micro-controller :-)

Actually I have noticed some "over-saturation" noise in the MOD generator, especially when several channels are playing at high volume. It seems to me that the following code is having a 15-bit overflow:

int16_t sumL;
int16_t sumR;


// Upscale to BITDEPTH
out <<= BITDEPTH - 8;
// Channel volume
out = out * Mixer.channelVolume[channel] >> 6;
// Channel panning
sumL += out * min(128 - Mixer.channelPanning[channel], 64) >> 6;
sumR += out * min(Mixer.channelPanning[channel], 64) >> 6;
}

Probably the overflow is caused by the "16-bit up-scaling" that was not present in the original stellarplayer.
Changing sumL, sumR and out to int32_t fixes the problem.

Maybe I can make a PR over the weekend.

Cheers, Frank.

@softhack007 softhack007 changed the title AudioGeneratorMOD - scartches/noise due to integer overflow AudioGeneratorMOD - scratches/noise due to integer overflow Dec 29, 2021
@earlephilhower
Copy link
Owner

There was a bug in older versions of the MOD lib where I was off-by-one in shifting the audio output from fixed point to integer, causing overflow that way and bad sounding mods, but I haven't noticed any other problems with it, so good ears on you!

Maybe I can make a PR over the weekend.

That would be great, especially since you not only found the problem but have a fix!

@softhack007
Copy link
Contributor Author

softhack007 commented Jan 1, 2022

update - after spending a few hours of debugging:

  1. the mixer code from stellarplayer is a f****** buggy piece of c***. Probably not even the original author could explain why it works 90% of the time. And yes, with some fixes it seems to work also for BITDEPTH = 16 (instead of 15).

  2. int16 vs int32 is only a partial solution. The code below shifts samples into "unsigned" ranges. Then - I'm using AudioOutputI2S with internal DAC - this operation is repeated in the output driver, leading to crazy artefacts. BTW, i think this also explains why you did not experience the same "scratching" that i heared...

    // Fill the sound buffer with unsigned values
    sample[AudioOutput::LEFTCHANNEL] = sumL + (1 << (BITDEPTH - 1));
    sample[AudioOutput::RIGHTCHANNEL] = sumR + (1 << (BITDEPTH - 1));
    }

  3. I am playing MODs from SPIFFS. Guess that something's wrong in the FatBuffer code, as GetSample sometimes computes stupidly large values (-> noise). Not sure what happens exactly. A workaround is to test sumL and sumR against INT16_MIN / INT16_MAX, and not updating sample outputs when clipping would occur (this ignores "bad" samples by repeating the last sample).

  4. Initially I was re-using the same instance of "AudioGeneratorMOD" in my scetch, using mod->stop() and mod->begin() to play another MOD file. This lead to "cracking" when the next MOD file starts playing. My workaround is to delete AudioGeneratorMOD, and then creating a new instance for the next file to play. Maybe something you could look into ...

  5. Guru Meditation when playing MODs with more than 4 channels - due to "CHANNELS = 4" in the .h file. After changing to "CHANNELS = 8", playback works also for 8CHAN; however I'm not sure if this is the best solution, because it increases the memory footprint. What do you think?

I'll create a pull request tomorrow or monday to fix (or work around) some of the problems - actually i need some time to clear out all my extra debugging code.

@softhack007
Copy link
Contributor Author

softhack007 commented Jan 1, 2022

PS: below some MODs for reproducing problems:

And some stranger things for the guru

@earlephilhower
Copy link
Owner

Hey, good work digging into this. I would not be so harsh on Stellaplayer...he wrote it for a tiny microcontroller with way less RAM and MIPS than we have, and it works pretty well in many cases. And I'm sure that I busted some things as I ported to the ESP systems!

Anyway, if you have a Linux machine, or WSL on Windows, it is easy to build and test changes to the MOD player. Look at the tests/host directory. make mod will build a sample player that runs on the host and writes a WAV file out. It's how I check for memory bugs/etc., but it also makes it much easier to test/debug any changes you're looking at.

softhack007 added a commit to softhack007/ESP8266Audio that referenced this issue Jan 3, 2022
- do channel mixing in 32bit, to avoid noise from over/underflow
- write audio output as "signed int" (not unsigned)
- add int16 saturation check ("clipping")
@softhack007
Copy link
Contributor Author

Thanks for the tests/host tip, that's a lot better 👍

You are right about stellarplayer - the code is a bit "fragile", however it turned out that it works basically. Maybe I just experienced a high WTF/min (https://www.osnews.com/story/19266/wtfsm/) when trying to understand it ;-)

earlephilhower pushed a commit that referenced this issue Jan 9, 2022
* Fix a tricky bug that causes delays in playback

... this bug was already present in stellaplayer. Obviously "effectNumber" must be checked, because "effectParameterX" just holds the "upper nibble" of effectParameter

* fix for issue #474

- do channel mixing in 32bit, to avoid noise from over/underflow
- write audio output as "signed int" (not unsigned)
- add int16 saturation check ("clipping")

* avoid crash when MOD file has to many CHANNELS

- avoid guru meditation, by logging an error and refusing playback in case MOD.numberOfChannels > CHANNELS
- correct small typo

* internal DAC: avoid possible overflow in conversion int16 -> uint16

* update constants

.. we can do 16bit and 8 channels, so let's do it ;-)

This adds some debugging code to better understand what is going on inside the mixer code (AudioGeneratorMOD::GetSample). And it also prints out some usefull information from the MOD file.

Only active when do_MIXER_DEBUG is defined.

* MOD generator: 12 bits of "real" resolution (instead of 10)

With this change, we gain 2 additional bits of "real resolution" from the sample interpolation step.
Still need some testing to be sure that nothing get "lost" at the same time.

* limit to 4 channels on ESP8266

As discussed in #479 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants