HiveBrain v1.2.0
Get Started
← Back to all entries
patterncModerate

Play some sine waves with SDL2

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
sinewithwavesplaysomesdl2

Problem

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
#include
#include
#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;

/* _
| |
___ _ __ ___ __ _| | __
/ __| '_ \ / _ \/ _
| |/ /
\__ \ |_) | __/ (_| | frequency/sampleRate;
Uint32 i;
if (v->volume > practicallySilent) {
for (i=0; (i+1)phase += phaseIncrement;
if (v->phase > 1) v->phase -= 1;

sourceIndex = v->phase*v->waveformLength;
sam

Solution

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 


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



-
A preprocessing directive of the form

#include  new-line




searches a sequence of implementation-defined places for a header identified uniquely by the specified sequence between the ` 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  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 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.

``
/* _ _ _____ _ _ _ _
| (_) / ____| | | | | | |
_

Code Snippets

#include <SDL/SDL.h>
#include <h-char-sequence> new-line
#include "q-char-sequence" new-line
#include <h-char-sequence> new-line
#include pp-tokens new-line

Context

StackExchange Code Review Q#41086, answer score: 12

Revisions (0)

No revisions yet.