Audio corruption

[Ian]

I know this has been talked about for years .. but most of the time I test with sound disabled it makes no sense unless you are hitting 60fps.

In debug mode, in visual studio the sound is perfect. Well it sounds as it should. But this is what the release build sounds like when compiled in visual studio 2017 ..

https://www.youtube.com/watch?v=mcinDcGD6GA

The audio is absolutely destroyed. I don't think the compiler is at fault here.. My best guess is there are some kind of illegal memory writes happening, and it's not an issue in debug mode due to extra memory padding. Could be that or some variable initialisation issue ..

[model123]

Hello Developers

I load the project into Visual Studio 2017 community
Update the platform tool to v141
Building solutions or supermodel with release x64

The sound is not broken

Google Translation

[Bart]

Also works fine with gcc. Does MSVC support linking debug and release modules? We could probably zero in on the offending code through a process of elimination. My money is on either the archaic MPEG decoder or the SCSP code.

We could probably run it through an address sanitizer, too, but I'm not sure if a sufficiently new version of gcc is available for Windows and I no longer have access to Linux systems.

[Ian]

Does MSVC support linking debug and release modules?

I'm not sure. But virtua fighter sounds fine, except a little extra reverb which probably shouldn't be there. I think its just the mp2 decoding

[Bart]

If there is a newer, GPL-compatible MPEG decoder we should probably swap it out (although it might not be too hard to identify the really gross parts of this one and fix them -- I think gcc produces a lot of compiler warnings that could be investigated). Does MAME have one? For years, they couldn't do MPEG music do to patent issues but I think the MPEG patents may have expired by now.

[Ian]

something like this ?
https://github.com/lieff/minimp3

Seems almost impossible to find any decent decoders that only decode mp2 for a possibly smaller implementation.

There is this one ..
https://keyj.emphy.de/kjmp2/

But at the bottom

Hi KeyJ, I tried using your library and it’s really simple to use and integrate.

But I’ve run into an issue regarding playback quality. On a sample sound I use, the decoder produces audible artifacts. This is visible both with the mp2play.exe and when decoding internally within my program.

and his reply

Coder: Yes, kjmp2 is very noisy – the SNR may be as low as 20 dB. I’m not sure whether this is strictly a result of its simplicity – it might just be that I have a bug somewhere or my fixed-point computations are sub-optimal. Also, there’s no noise shaping of any kind.

So probably not a good implementation to use

[Ian]

Well I had a few spare hours so I had a go at rewriting the audio code for mpeg decoding ..

Code: Select all

#define MINIMP3_IMPLEMENTATION
#include "minimp3.h"
#include <type_traits>
#include "MpegAudio.h"

struct Decoder
{
   mp3dec_t         mp3d;
   mp3dec_frame_info_t   info;
   uint8_t*         buffer;
   size_t            size, pos;
   bool            loop;
   bool            stopped;
   int               numSamples;
   int               pcmPos;
   short            pcm[MINIMP3_MAX_SAMPLES_PER_FRAME];
};

static Decoder dec = { 0 };

void MpegDec::SetMpegMemory(uint8_t *data, int length, bool loop)
{
   mp3dec_init(&dec.mp3d);

   dec.buffer      = data;
   dec.size      = length;
   dec.pos         = 0;
   dec.numSamples   = 0;
   dec.pcmPos      = 0;
   dec.loop      = loop;
   dec.stopped      = false;
}

int MpegDec::GetMpegPosition()
{
   return dec.pos;
}

static void FlushBuffer(int16_t*& left, int16_t*& right, int& numStereoSamples)
{
   int numChans = dec.info.channels;

   int &i = dec.pcmPos;

   for (; i < (dec.numSamples * numChans) && numStereoSamples; i += numChans) {
      *left++ = dec.pcm[i];
      *right++ = dec.pcm[i + numChans - 1];
      numStereoSamples--;
   }
}

// might need to copy some silence to end the buffer if we aren't looping
static void EndWithSilence(int16_t*& left, int16_t*& right, int& numStereoSamples)
{
   while (numStereoSamples)
   {
      *left++ = 0;
      *right++ = 0;
      numStereoSamples--;
   }
}

static bool EndOfBuffer()
{
   return dec.pos >= dec.size;
}

void MpegDec::StopMpeg()
{
   dec.stopped = true;
}

void MpegDec::DecodeMpegAudio(int16_t* left, int16_t* right, int numStereoSamples)
{
   // if we are stopped return silence
   if (dec.stopped || !dec.buffer) {
      EndWithSilence(left, right, numStereoSamples);
   }

   // copy any left over samples first
   FlushBuffer(left, right, numStereoSamples);

   while (numStereoSamples) {

      dec.numSamples = mp3dec_decode_frame(
         &dec.mp3d,
         dec.buffer + dec.pos,
         dec.size - dec.pos,
         dec.pcm,
         &dec.info);

      dec.pos += dec.info.frame_bytes;
      dec.pcmPos = 0;   // reset pos

      FlushBuffer(left, right, numStereoSamples);

      // check end of buffer handling
      if (EndOfBuffer()) {
         if (dec.loop) {
            dec.pos = 0;
         }
         else {
            EndWithSilence(left, right, numStereoSamples);
         }
      }

   }

}

header file

Code: Select all

#ifndef _MPEG_AUDIO_H_
#define _MPEG_AUDIO_H_

#include <stdint.h>

namespace MpegDec
{
   void   SetMpegMemory(uint8_t *data, int length, bool loop);
   int      GetMpegPosition();
   void   DecodeMpegAudio(int16_t* left, int16_t* right, int numStereoSamples);
   void   StopMpeg();
}

#endif

Basically it works .. in release mode too without crazy errors.

[Bart]

Whoa. You wrote this from scratch?

[Ian]

Yeah, well it's not much code really. The grunt work is being done by minimp3 or this thing
https://github.com/lieff/minimp3

It needs some tidying up and testing really, but it means we can rip out a lot of this old crazy mpeg code which seems to cause such issues depending on the compiler
One thing I did notice is the left/right chans seem swapped with my code. I assume? that the channel order is left right interleaved from mp3 ..

But the original code looks like this

for (i = 0; i < outpos; i++)
{
outputs[1][i] = *get++;
outputs[0][i] = *get++;
}

Which seems kind of the inverse to what I would expect. Either our left/right are swapped somewhere or something. Maybe I should check with the hardware as to which is correct

[Bart]

Nice! Looks like the license is GPL compatible, too!