11
\$\begingroup\$

Runs smoothly, however valgrind is showing "possibly lost: 544 bytes in 2 blocks". This is my first time doing a lot of things here so I might be making multiple mistakes.

Please let me know if anything needs to be fixed, or if I should just do something completely different for whatever reason.

/* protosynth
* 
* Throughout the source code, "frequency" refers to a Hz value, 
* and "pitch" refers to a numeric musical note value with 0 representing C0, 12 for C1, etc..
* 
* compiled with:
* gcc -Wall protosynth.c -o protosynth `sdl2-config --cflags --libs` -lm
*/

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include "SDL.h"

const double ChromaticRatio = 1.059463094359295264562;
const double Tao = 6.283185307179586476925;

Uint32 sampleRate = 48000;
Uint32  frameRate =    60;
Uint32 floatStreamLength = 1024;// must be a power of two, decrease to allow for a lower syncCompensationFactor to allow for lower latency, increase to reduce risk of underrun
Uint32 samplesPerFrame; // = sampleRate/frameRate;
Uint32 msPerFrame; // = 1000/frameRate;
double practicallySilent = 0.001;

Uint32 audioBufferLength = 48000;// must be a multiple of samplesPerFrame (auto adjusted upwards if not)
float *audioBuffer;

SDL_atomic_t audioCallbackLeftOff;
Sint32 audioMainLeftOff;
Uint8 audioMainAccumulator;

SDL_AudioDeviceID AudioDevice;
SDL_AudioSpec audioSpec;

SDL_Event event;
SDL_bool running = SDL_TRUE;


typedef struct {
  float *waveform;
  Uint32 waveformLength;
  double volume;        // multiplied
  double pan;           // 0 to 1: all the way left to all the way right
  double frequency;     // Hz
  double phase;         // 0 to 1
} voice;

/*                     _    
                      | |   
  ___ _ __   ___  __ _| | __
/ __| '_ \ / _ \/ _` | |/ /
\__ \ |_) |  __/ (_| |   < 
|___/ .__/ \___|\__,_|_|\_\
    | |                    
    |_|                    
*/
void speak(voice *v) {
  float sample;
  Uint32 sourceIndex;
  double phaseIncrement = v->frequency/sampleRate;
  Uint32 i;
  if (v->volume > practicallySilent) {
    for (i=0; (i+1)<samplesPerFrame; i+=2) {

      v->phase += phaseIncrement;
      if (v->phase > 1) v->phase -= 1;

      sourceIndex = v->phase*v->waveformLength;
      sample = v->waveform[sourceIndex]*v->volume;

      audioBuffer[audioMainLeftOff+i] += sample*(1-v->pan); //left channel
      audioBuffer[audioMainLeftOff+i+1] += sample*v->pan;   //right channel
    }
  }
  else {
    for (i=0; i<samplesPerFrame; i+=1)
      audioBuffer[audioMainLeftOff+i] = 0;
  }
  audioMainAccumulator++;
}

double getFrequency(double pitch) {
  return pow(ChromaticRatio, pitch-57)*440;
}
int getWaveformLength(double pitch) {
  return sampleRate / getFrequency(pitch)+0.5f;
}

void buildSineWave(float *data, Uint32 length) {
  Uint32 i;
  for (i=0; i < length; i++)
    data[i] = sin( i*(Tao/length) );
}

void logSpec(SDL_AudioSpec *as) {
  printf(
    " freq______%5d\n"
    " format____%5d\n"
    " channels__%5d\n"
    " silence___%5d\n"
    " samples___%5d\n"
    " size______%5d\n\n",
    (int) as->freq,
    (int) as->format,
    (int) as->channels,
    (int) as->silence,
    (int) as->samples,
    (int) as->size
  );
}

void logVoice(voice *v) {
  printf(
    " waveformLength__%d\n"
    " volume__________%f\n"
    " pan_____________%f\n"
    " frequency_______%f\n"
    " phase___________%f\n",
    v->waveformLength,
    v->volume,
    v->pan,
    v->frequency,
    v->phase
  );
}

void logWavedata(float *floatStream, Uint32 floatStreamLength, Uint32 increment) {
  printf("\n\nwaveform data:\n\n");
  Uint32 i=0;
  for (i=0; i<floatStreamLength; i+=increment)
    printf("%4d:%2.16f\n", i, floatStream[i]);
  printf("\n\n");
}

/*                _ _        _____      _ _ _                _    
                | (_)      / ____|    | | | |              | |   
  __ _ _   _  __| |_  ___ | |     __ _| | | |__   __ _  ___| | __
  / _` | | | |/ _` | |/ _ \| |    / _` | | | '_ \ / _` |/ __| |/ /
| (_| | |_| | (_| | | (_) | |___| (_| | | | |_) | (_| | (__|   < 
  \__,_|\__,_|\__,_|_|\___/ \_____\__,_|_|_|_.__/ \__,_|\___|_|\_\
*/
void audioCallback(void *unused, Uint8 *byteStream, int byteStreamLength) {
  float* floatStream = (float*) byteStream;

  Sint32 localAudioCallbackLeftOff = SDL_AtomicGet(&audioCallbackLeftOff);

  Uint32 i;
  for (i=0; i<floatStreamLength; i++) {

    floatStream[i] = audioBuffer[localAudioCallbackLeftOff];

    localAudioCallbackLeftOff++;
    if ( localAudioCallbackLeftOff == audioBufferLength )
      localAudioCallbackLeftOff = 0;
  }
  //printf("localAudioCallbackLeftOff__%5d\n", localAudioCallbackLeftOff);

  SDL_AtomicSet(&audioCallbackLeftOff, localAudioCallbackLeftOff);
}

/*_       _ _   
(_)     (_) |  
  _ _ __  _| |_ 
| | '_ \| | __|
| | | | | | |_ 
|_|_| |_|_|\__|
*/
int init() {
  SDL_Init(SDL_INIT_AUDIO | SDL_INIT_TIMER);

  SDL_AudioSpec want;

  SDL_zero(want);// btw, I have no idea what this is...

  want.freq = sampleRate;
  want.format = AUDIO_F32;
  want.channels = 2;
  want.samples = floatStreamLength;
  want.callback = audioCallback;

  AudioDevice = SDL_OpenAudioDevice(NULL, 0, &want, &audioSpec, SDL_AUDIO_ALLOW_FORMAT_CHANGE);

  if (AudioDevice == 0) {
    printf("\nFailed to open audio: %s\n", SDL_GetError());
    return 1;
  }

  printf("want:\n");
  logSpec(&want);
  printf("audioSpec:\n");
  logSpec(&audioSpec);

  if (audioSpec.format != want.format) {
    printf("\nCouldn't get Float32 audio format.\n");
    return 2;
  }

  sampleRate = audioSpec.freq;
  floatStreamLength = audioSpec.size/4;
  samplesPerFrame = sampleRate/frameRate;
  msPerFrame = 1000/frameRate;
  audioMainLeftOff = samplesPerFrame*8;
  SDL_AtomicSet(&audioCallbackLeftOff, 0);

  if (audioBufferLength % samplesPerFrame)
    audioBufferLength += samplesPerFrame-(audioBufferLength % samplesPerFrame);
  audioBuffer = malloc( sizeof(float)*audioBufferLength );

  return 0;
}
int onExit() {
  SDL_CloseAudioDevice(AudioDevice);
  //free(audioBuffer);//not necessary?
  SDL_Quit();
  return 0;
}

/*                _       
                (_)      
  _ __ ___   __ _ _ _ __  
| '_ ` _ \ / _` | | '_ \ 
| | | | | | (_| | | | | |
|_| |_| |_|\__,_|_|_| |_|
*/
int main(int argc, char *argv[]) {

  float  syncCompensationFactor = 0.0016;// decrease to reduce risk of collision, increase to lower latency
  Sint32 mainAudioLead;
  Uint32 i;

  voice testVoiceA;
  voice testVoiceB;
  voice testVoiceC;
  testVoiceA.volume = 1;
  testVoiceB.volume = 1;
  testVoiceC.volume = 1;
  testVoiceA.pan = 0.5;
  testVoiceB.pan = 0;
  testVoiceC.pan = 1;
  testVoiceA.phase = 0;
  testVoiceB.phase = 0;
  testVoiceC.phase = 0;
  testVoiceA.frequency = getFrequency(45);// A3
  testVoiceB.frequency = getFrequency(49);// C#4
  testVoiceC.frequency = getFrequency(52);// E4
  Uint16 C0waveformLength = getWaveformLength(0);
  testVoiceA.waveformLength = C0waveformLength;
  testVoiceB.waveformLength = C0waveformLength;
  testVoiceC.waveformLength = C0waveformLength;
  float sineWave[C0waveformLength];
  buildSineWave(sineWave, C0waveformLength);
  testVoiceA.waveform = sineWave;
  testVoiceB.waveform = sineWave;
  testVoiceC.waveform = sineWave;

  //logVoice(&testVoiceA);
  //logWavedata(testVoiceA.waveform, testVoiceA.waveformLength, 10);

  if ( init() ) return 1;

  SDL_Delay(42);// let the tubes warm up

  SDL_PauseAudioDevice(AudioDevice, 0);// unpause audio.

  while (running) {
    while( SDL_PollEvent( &event ) != 0 ) {
      if( event.type == SDL_QUIT ) { 
        running = SDL_FALSE;
      } 
    }
    for (i=0; i<samplesPerFrame; i++) audioBuffer[audioMainLeftOff+i] = 0;

    //printf("audioMainLeftOff___________%5d\n", audioMainLeftOff);

    speak(&testVoiceA);
    speak(&testVoiceB);
    speak(&testVoiceC);

    if (audioMainAccumulator > 1) {
      for (i=0; i<samplesPerFrame; i++) {
        audioBuffer[audioMainLeftOff+i] /= audioMainAccumulator;
      }
    }
    audioMainAccumulator = 0;

    audioMainLeftOff += samplesPerFrame;
    if (audioMainLeftOff == audioBufferLength) audioMainLeftOff = 0;

    mainAudioLead = audioMainLeftOff - SDL_AtomicGet(&audioCallbackLeftOff);
    if (mainAudioLead < 0) mainAudioLead += audioBufferLength;
    //printf("mainAudioLead:%5d\n", mainAudioLead);
    if (mainAudioLead < floatStreamLength) printf("An audio collision may have occured!\n");
    SDL_Delay( mainAudioLead*syncCompensationFactor );
  }
  onExit();
  return 0;
}

EDIT:

After doing some more research on my valgrind errors, I came to the conclusion that there wasn't much I could do other than suppress them. Here is my suppression file:

{
  <from SDL_TimerInit>
  Memcheck:Leak
  match-leak-kinds: possible
  fun:calloc
  fun:allocate_dtv
  fun:_dl_allocate_tls
  fun:pthread_create@@GLIBC_2.2.5
  fun:SDL_SYS_CreateThread
  fun:SDL_CreateThread
  fun:SDL_TimerInit
  fun:SDL_InitSubSystem
  fun:init
  fun:main
}
{
  <from SDL_AudioInit>
  Memcheck:Leak
  match-leak-kinds: possible
  fun:calloc
  fun:allocate_dtv
  fun:_dl_allocate_tls
  fun:pthread_create@@GLIBC_2.2.5
  fun:pa_thread_new
  fun:pa_threaded_mainloop_start
  fun:pa_simple_new
  fun:PULSEAUDIO_Init
  fun:SDL_AudioInit
  fun:SDL_InitSubSystem
  fun:init
  fun:main
}
\$\endgroup\$
  • 3
    \$\begingroup\$ ASCII art comments? That just repeat the names of the functions? Argh, my eyes... \$\endgroup\$ – Ant Feb 6 '14 at 18:11
  • \$\begingroup\$ haha, sorry, Ant. Those are just to make it easier to navigate until I break it down into multiple files. Then I won't need them. \$\endgroup\$ – Duovarious Feb 6 '14 at 18:58
10
\$\begingroup\$

Things you did well

  • Nicely formatted, easy to read.

  • Use of typedef with structures.

Things you could improve

Preprocessor:

  • Since SDL.h isn't one of your own pre-defined header files, you should be searching for it in directories pre-designated by the compiler (since that is where it should be stored).

    #include <SDL/SDL.h>
    

    In the C standard, §6.10.2, paragraphs 2 to 4 state:

    • A preprocessing directive of the form

      #include <h-char-sequence> new-line
      

      searches a sequence of implementation-defined places for a header identified uniquely by the specified sequence between the < and > delimiters, and causes the replacement of that directive by the entire contents of the header. How the places are specified or the header identified is implementation-defined.

    • A preprocessing directive of the form

      #include "q-char-sequence" new-line
      

      causes the replacement of that directive by the entire contents of the source file identified by the specified sequence between the " delimiters. The named source file is searched for in an implementation-defined manner. If this search is not supported, or if the search fails, the directive is reprocessed as if it read

      #include <h-char-sequence> new-line
      

      with the identical contained sequence (including > characters, if any) from the original directive.

    • A preprocessing directive of the form

      #include pp-tokens new-line
      

      (that does not match one of the two previous forms) is permitted. The preprocessing tokens after include in the directive are processed just as in normal text. (Each identifier currently defined as a macro name is replaced by its replacement list of preprocessing tokens.) The directive resulting after all replacements shall match one of the two previous forms. The method by which a sequence of preprocessing tokens between a < and a > preprocessing token pair or a pair of " characters is combined into a single header name preprocessing token is implementation-defined.

    Definitions:

    • h-char: any member of the source character set except the new-line character and >

    • q-char: any member of the source character set except the new-line character and "

    Don't forget to include -lSDL with gcc to link your code to the SDL library.

Variables/Initialization:

  • Tao is simple 2π, as you have defined in your code.

    const double Tao = 6.283185307179586476925;
    

    However, π is a mathematically defined constant in math.h. Since you are already using that header, you should utilize the predefined constant.

    const double TAO = 2 * M_PI;
    

Memory:

  • You allocate memory to audioBuffer(), but then never free() it,

    audioBuffer = malloc( sizeof(float)*audioBufferLength );
    // free(audioBuffer); //not necessary?
    

    This would be my guess as to what valgrind is whining about. You should always have freed all memory that you have allocated before you exit your program; we want to avoid memory leaks.

    Now to adress your comment as to whether or not that line of code is necessary, since you are exiting your program anyways. It depends on the operating system. The majority of modern (and all major) operating systems will free memory not freed by the program when it ends.

    Relying on this is bad practice and it is better to free() it explicitly. The issue isn't just that your code looks bad. You may decide you want to integrate your small program into a larger, long running one. Then a while later you have to spend hours tracking down memory leaks.

    Relying on a feature of an operating system also makes the code less portable.

Syntax/Styling:

  • Right now you are using Uint32 to represent an unsigned 32 bit integer, but uint32_t is the type that's defined by the C standard.

    uint32_t sampleRate = 48000;
    
  • Define i within your for loops, not outside.(C99)

    for (uint32_t i = 0; (i+1) < samplesPerFrame; i += 2)
    
  • typedef structs typically have a capitalized name by standard conventions.

    typedef struct 
    {
      float *waveform;
      Uint32 waveformLength;
      double volume;        // multiplied
      double pan;           // 0 to 1: all the way left to all the way right
      double frequency;     // Hz
      double phase;         // 0 to 1
    } Voice;
    
  • Declare all of your parameters as void when you don't take in any arguments.

    int init(void)
    
  • You aren't using the parameters specified in main().

    int main(int argc, char *argv[])
    

    Declare them as void if you aren't going to use them.

    int main(void)
    
  • Use puts() instead of printf() when you aren't formatting your output.

    printf("\n\nwaveform data:\n\n");
    
    puts("Waveform data: ");
    
  • Remove != 0 in some of your conditional tests for maximum C-ness.

Comments:

  • Your ASCII art comments... hurt my eyes.

    /*                _ _        _____      _ _ _                _    
                     | (_)      / ____|    | | | |              | |   
       __ _ _   _  __| |_  ___ | |     __ _| | | |__   __ _  ___| | __
      / _` | | | |/ _` | |/ _ \| |    / _` | | | '_ \ / _` |/ __| |/ /
     | (_| | |_| | (_| | | (_) | |___| (_| | | | |_) | (_| | (__|   < 
      \__,_|\__,_|\__,_|_|\___/ \_____\__,_|_|_|_.__/ \__,_|\___|_|\_\
    */
    

    You said in the comments that: "Those are just to make it easier to navigate until I break it down into multiple files. Then I won't need them."

    Let me suggest another alternative that you can keep around: documenting your code with Doxygen. Replacing your ASCII art comments with documentation of your methods will make it easier to navigate, and serve the very important purpose of stating why/how you programmed something a certain way.

    I've taken an example from one of my previous questions to use here.

    /**
     * @fn static void json_fillToken(JsonToken *token, JsonType type, int start, int end)
     * @brief Fills token type and boundaries.
     * @param token
     * @param type
     * @param start
     * @param end
     */
    static void json_fillToken(JsonToken *token, JsonType type, int start, int end)
    {
        token->type = type;
        token->start = start;
        token->end = end;
        token->size = 0;
    }
    
  • Remove old commented out code.

    //logVoice(&testVoiceA);
    //logWavedata(testVoiceA.waveform, testVoiceA.waveformLength, 10);
    

    It serves almost no purpose, and makes your code look cluttered.

  • Besides your ASCII art comments and your old commented out code, you have only a few other comments throughout your source code. See this blog post here as to why and how you should comment throughout your code.

Exiting:

  • You have a function dedicated to termination, and you call it right before you close down your program.

    int onExit() {
      SDL_CloseAudioDevice(AudioDevice);
      //free(audioBuffer);//not necessary?
      SDL_Quit();
      return 0;
    }
    

    I think you could make great use of the atexit() function in your code. The atexit() function registers a function to be called at normal program termination. Though if you decide to use this, you may want to rename onExit() to something such as cleanup() or something similar.

    int main(void)
    {
        ...
        atexit(cleanup);
        return 0;
    }
    
\$\endgroup\$
  • 3
    \$\begingroup\$ However, π is a mathematically defined constant in math.h. Not according to the standard, it's not. It's a very common extension, but it's not guaranteed to be defined. \$\endgroup\$ – Corbin Mar 18 '14 at 4:31
  • \$\begingroup\$ Thanks! I went ahead and made a lot of the changes you suggested. The free(audioBuffer) line wasn't the source of the valgrind error, but I've uncommented it anyway for the other reasons you mentioned, and added more info regarding this in my original post. Also, <SDL/SDL.h> didn't work, but <SDL.h> did. \$\endgroup\$ – Duovarious Mar 18 '14 at 7:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.