izzyreal / mpc

mpc2000xl emulator static library
GNU General Public License v3.0
72 stars 8 forks source link

Verify whether clipping is handled correctly #173

Closed izzyreal closed 1 year ago

izzyreal commented 1 year ago

Write some unit tests to check if clipping is handled correctly. Consider the below image

Zooming in on the first needle:

image

and the second:

image

This waveform is the result of a user sampling via a loopback device and using VMPC2000XL's Normalize. It is unclear at what stage the needles were introduced. But I've never paid a lot of attention to float and int ranges of sample data and how to handle clipping, so it's possible something in VMPC2000XL's sample data processing chain is off.

Consider the full chain and write some unit tests to verify the sample data is as intended after each stage of the chain. In the above use case that chain would be:

  1. Record sound via mpc::audiomidi::SoundRecorder
  2. Normalize the sound via mpc::lcdgui::screens::window::EditSoundScreen. See else if (edit == 8) in EditSoundScreen.cpp for the normalization algorithm.
  3. Save the sound as an SND file. This happens via mpc::file::sndwriter::SndWriter.
izzyreal commented 1 year ago

Sounds like VMPC2000XL's conversion from internal 32 bit float representation to 16 bit int does not clip like it should, resulting in integer overflow. That would explain the above needles.

Moreover, at the moment this conversion from 32 bit float to 16 bit int currently only happens when saving the sound. We should also do this conversion in SoundRecorder and MonitorInputAdapter, and immediately convert it back to 32 bit again. This conversion algorithm should be the exact inverse of the ones used when loading WAV or SND files.

izzyreal commented 1 year ago

@brownounces I just pushed 5fcd998aa1f9c788bb32dbd6b105f6f7682be0d7 and it should clip all your sampled input. Persisting your recorded sounds to disk should now yield the correct result. "Correct" means that, instead of the needles with integer-overflowed values, you will simply get clipped values without any polarity inversion.

I did a quick test by shouting into the mic, I confirmed that the 32-bit values can fall outside the [ -1, 1 ] range, and with 5fcd998aa1f9c788bb32dbd6b105f6f7682be0d7 these values will be clamped to fit within that range.

In theory you should now not be able to reproduce the issue anymore. Would be great if you could confirm!

brownounces commented 1 year ago

@izzyreal Did I make a post here that was deleted? I could have sworn I made one yesterday, lol.

My test on the latest CMake build was inconclusive; it seems that recording a clipped sample was fixed, but normalizing + saving the sample was not -- perhaps the NORMALIZE feature is still corrupting the data somehow. I've sent you an e-mail containing a file called 'TESTS.7z' to demonstrate.

In my experience, the offending sample does not introduce an artifact upon saving and loading it IF it is not normalized. Normalizing the sample alone also does not impart the artifact until it is saved and reloaded; you can test this with the 'TEST' folder I sent you.

  1. Load the .APS in 'TEST'
  2. Press pad 1
  3. You will hear the first chop (sound2) contains no artifact
  4. Go to TRIM and NORMALIZE the sample
  5. The sample will have been correctly normalized without artifacting
  6. Save the .APS and reload it
  7. The sample now contains an overflow artifact; also as demonstrated by the 'TESTN' folder
izzyreal commented 1 year ago

Right, this morning I started realising I didn't fix the problem fully. I'm clamping the 32 bit floats to [-1, 1], but my conversion to 16-bit will still yield needles for any floats that are exactly 1. Working on a fix as we speak.

izzyreal commented 1 year ago

Resolved in 2f9c2207d2d3cb8a0bb991cc4e58dbe333d49d83.