Thread: My code should make sine sound. But instead, it makes noises...

wavfile: A Simple Sound Library


I copied exact code form above website and ran the program but it' not working...

It is supposed to make sine wave but instead it's making noise sound...


Here's the full code,


wavfile.h

Code:

#ifndef WAVFILE_H
#define WAVFILE_H


#include <stdio.h>
#include <inttypes.h>


FILE * wavfile_open( const char *filename );
void wavfile_write( FILE *file, short data[], int length );
void wavfile_close( FILE * file );


#define WAVFILE_SAMPLES_PER_SECOND 44100


#endif

wavfile.c

Code:

#include "wavfile.h"


#include <stdio.h>
#include <stdlib.h>
#include <string.h>


struct wavfile_header {
    char    riff_tag[4];
    int    riff_length;
    char    wave_tag[4];
    char    fmt_tag[4];
    int    fmt_length;
    short    audio_format;
    short    num_channels;
    int    sample_rate;
    int    byte_rate;
    short    block_align;
    short    bits_per_sample;
    char    data_tag[4];
    int    data_length;
};


FILE * wavfile_open( const char *filename )
{
    struct wavfile_header header;


    int samples_per_second = WAVFILE_SAMPLES_PER_SECOND;
    int bits_per_sample = 16;


    strncpy(header.riff_tag,"RIFF",4);
    strncpy(header.wave_tag,"WAVE",4);
    strncpy(header.fmt_tag,"fmt ",4);
    strncpy(header.data_tag,"data",4);


    header.riff_length = 0;
    header.fmt_length = 16;
    header.audio_format = 1;
    header.num_channels = 1;
    header.sample_rate = samples_per_second;
    header.byte_rate = samples_per_second*(bits_per_sample/8);
    header.block_align = bits_per_sample/8;
    header.bits_per_sample = bits_per_sample;
    header.data_length = 0;


    FILE * file = fopen(filename,"w+");
    if(!file) return 0;


    fwrite(&header,sizeof(header),1,file);


    fflush(file);


    return file;


}


void wavfile_write( FILE *file, short data[], int length )
{
    fwrite(data,sizeof(short),length,file);
}


void wavfile_close( FILE *file )
{
    int file_length = ftell(file);


    int data_length = file_length - sizeof(struct wavfile_header);
    fseek(file,sizeof(struct wavfile_header) - sizeof(int),SEEK_SET);
    fwrite(&data_length,sizeof(data_length),1,file);


    int riff_length = file_length - 8;
    fseek(file,4,SEEK_SET);
    fwrite(&riff_length,sizeof(riff_length),1,file);


    fclose(file);
}

sine.c

Code:

#include <stdio.h>
#define _USE_MATH_DEFINES
#include <math.h>
#include <stdlib.h>
#include <time.h>
#include <string.h>
#include <errno.h>


#include "wavfile.h"


const int NUM_SAMPLES = WAVFILE_SAMPLES_PER_SECOND*2;


int main()
{
    short waveform[NUM_SAMPLES];
    double frequency = 440.0;
    int volume = 32000;
    int length = NUM_SAMPLES;


    int i;
    for(i=0;i<length;i++) {
        double t = (double) i / WAVFILE_SAMPLES_PER_SECOND;
        waveform[i] = volume*sin(frequency*t*2*M_PI);
    }


    FILE * f = wavfile_open("sound.wav");
    if(!f) {
        printf("couldn't open sound.wav for writing: %s",strerror(errno));
        return 1;
    }


    wavfile_write(f,waveform,length);
    wavfile_close(f);


    return 0;
}

Could you guys run the code and tell me if it makes sine sound?


To compile on gcc,

Code:

gcc -std=c11 sine.c -o sine.exe -lm wavfile.c

If you compile it right, it does.

Compile using

Code:

gcc -Wall -Wextra -std=c99 -c wavfile.c
gcc -Wall -Wextra -std=c99 -c sine.c
gcc -Wall -Wextra -std=c99 sine.o wavfile.o -lm -o sine

and run

Code:

./sine
aplay sound.wav

or

Code:

./sine
paplay sound.wav

first uses ALSA player, the second PulseAudio player; one or both should be installed on every Linux distribution by default.

So I guess it doesn't work on Windows. Or does it? I tried your method but it still makes noise. Also, is c99 better than c11? it seems like lots of poeple use c99...

Originally Posted by yj1214

So I guess it doesn't work on Windows.

Have you tried opening the sound.wav in a sound editor, like Audacity perhaps? I don't have Windows, so I cannot help you there.

Originally Posted by yj1214

Also, is c99 better than c11?

If you use GCC in 2015, the C99 support is more complete than C11, that's all.

For many, a lot of the stuff in C11 feels like a sidestep or a marketing ploy; the step from C99 to C11 was disappointing compared to the step from ANSI C/C89 to C99. The atomics in C11 are very useful, but portable compiler-specific alternatives already exist (__sync built-ins), so there is very little in C11 that is useful for a serious C programmer that is not in C99 already.

Yes, I tried on Audacity and it's still noise wave, not sine wave...

If I find an answer, I'll post here...

Just noticed that your wavfile library uses a structure to describe the RIFF WAV header. That's stupid, because structure packing is nontrivial.

Could you check if the generated sound.wav on your machine is 176,444 bytes long? That sizeof (struct wavfile_header) == 44 (in e.g. wavfile_open())?

Originally Posted by yj1214

I think my sound.wav is 177,084 bytes long...do I need to change some kind of settings in order to play the sound? thanks.

Add

Code:

    fprintf(stderr, "sizeof (struct wavfile_header) = %d\n", (int)sizeof (struct wavfile_header));
    fflush(stderr);

to your wavfile_open() function, and compile and run the program.

If it reports anything other than

sizeof (struct wavfile_header) = 44

the problem is that the code uses a structure to describe a file header, but does not take into account structure packing. You could try modifying the structure definition into:

Code:

#include <stdint.h>

struct wavfile_header {
    char     riff_tag[4];
    uint32_t riff_length;
    char     wave_tag[4];
    char     fmt_tag[4];
    uint32_t fmt_length;
    uint16_t audio_format;
    uint16_t num_channels;
    uint32_t sample_rate;
    uint32_t byte_rate;
    uint16_t block_align;
    uint16_t bits_per_sample;
    char     data_tag[4];
    uint32_t data_length;
} __attribute__((packed));

but it is kinda hacky, relying on GCC's structure attributes (here, pack as tightly as possible, no padding). Other compilers support the same in other ways, e.g. #pragma pack, and so on, so it's not portable, and falls outside standard C.

In standard C, you cannot just assume you can use a structure to describe a specific data structure, because C allows the compiler to align and add padding within the structure. Telling the compiler to not do that is nonstandard, and varies a bit between compilers, so it's definitely hacky. Just because many people do this this way, does not make it right.

The correct approach, overall, would be to define the header as a simple 44-byte buffer, and use accessor functions to set the 16- and 32-bit low-endian unsigned integers, and 4-character identifiers, in it, preferably in a portable fashion.

Just to give you a hint, I'd start with wavfile.h that looks more like:

Code:

#ifndef WAVFILE_H
#define WAVFILE_H
#include <stdio.h>

typedef struct {
    FILE *out;
    uint32_t length;
    unsigned char header[44];
} wavfile;

int wavfile_create(wavfile *const w, const char *const filename,
                 const unsigned int sample_rate);

int wavfile_write(wavfile *const w, const void *const data, const size_t samples);

int wavfile_close(wavfile *const w);

#endif /* WAVFILE_H */

The idea is that the wavfile structure maintains all needed information on the file, including the stream handle.

In wavfile_c, we'd have

Code:

static void set_u16(wavfile *const w, const int offset, const unsigned int value)
{
    if (w && offset >= 0 && offset <= 42) {
        w->header[offset + 0] = value & 255U;
        w->header[offset + 1] = (value >> 8) & 255U;
    }
}

static void set_u32(wavfile *const w, const int offset, const unsigned int value)
{
    if (w && offset >= 0 && offset <= 40) {
        w->header[offset + 0] = value & 255U;
        w->header[offset + 1] = (value >> 8) & 255U;
        w->header[offset + 2] = (value >> 16) & 255U;
        w->header[offset + 3] = (value >> 24) & 255U;
    }
}

static void set_id4(wavfile *const w, const int offset, const char value[4])
{
    if (w && offset >= 0 && offset <= 40) {
        w->header[offset + 0] = (unsigned char)(value[0]);
        w->header[offset + 1] = (unsigned char)(value[1]);
        w->header[offset + 2] = (unsigned char)(value[2]);
        w->header[offset + 3] = (unsigned char)(value[3]);
    }
}

#define SET_RIFF_TAG(w) set_id4(w, 0, "RIFF")
#define SET_RIFF_LEN(w, len) set_u32(w, 4, len)
#define SET_WAVE_TAG(w) set_id4(w, 8, "WAVE")
#define SET_FMT_TAG(w) set_id4(w, 12, "fmt ")
#define SET_FMT_LEN(w, len) set_u32(w, 16, len)
#define SET_AUDIO_FORMAT(w, fmt) set_u16(w, 20, fmt)
#define SET_NUM_CHANNELS(w, num) set_u16(w, 22, num)
#define SET_SAMPLE_RATE(w, rate) set_u32(w, 24, rate)
#define SET_BYTE_RATE(w, rate) set_u32(w, 28, rate)
#define SET_BLOCK_ALIGN(w, al) set_u16(w, 32, al)
#define SET_BITS_PER_SAMPLE(w, bits) set_u16(w, 34, bits)
#define SET_DATA_TAG(w) set_id4(w, 36, "data")
#define SET_DATA_LENGTH(w, len) set_u32(w, 40, len)

Gaaargh. I forgot you're in Windows, right?

You need to open the file in "binary" mode, so that your C library won't mangle certain byte values. Modify line 52,

Code:

FILE * file = fopen(filename,"w+");

into

Code:

FILE * file = fopen(filename,"w+b");

The manglement includes replacing each '\n' with '\r\n' -- that is, each byte 10 with two bytes, 13 and 10. That explains the larger file size.

(Mumble mumble I'm stupid for not noticing that immediately; it's not like this is the first time, or even the twentieth time, I've encountered this, and should know better mumble mumble)

Originally Posted by Nominal Animal

In standard C, you cannot just assume you can use a structure to describe a specific data structure, because C allows the compiler to align and add padding within the structure. Telling the compiler to not do that is nonstandard, and varies a bit between compilers, so it's definitely hacky. Just because many people do this this way, does not make it right.

The correct approach, overall, would be to define the header as a simple 44-byte buffer, and use accessor functions to set the 16- and 32-bit low-endian unsigned integers, and 4-character identifiers, in it, preferably in a portable fashion.

I recently encountered this issue, when toying with the BMP file format. During my research, I found sample code that put header information into a struct, and used pragma pack to squeeze out the padding, before writing it to a binary file.

I liked the organization provided by the struct, but not the pragma, so my solution was to create a function that simply wrote each member individually to the binary file.

In your opinion, was this a sub-optimal solution, compared to the one you offered here (putting all bytes into a single buffer)?

There is probably more code involved with my solution (including multiple writes instead of just one), but I also retain the organizational aspects of the struct, which comes in handy when specific data from the header is needed elsewhere in the program.

Originally Posted by Matticus

my solution was to create a function that simply wrote each member individually to the binary file.

It's not a bad approach. I use the same approach occasionally with quick-and-dirty tests I tend to discard or rewrite later on. (Especially when testing support for a new file format.)

There are two minor downsides to that approach:

• Byte order with multi-byte values
• Function and system call overhead

Byte order is the main point. While most desktop operating systems are little-endian, some ARM ones used in tablets and such are big-endian. So, if you use Linux, it's quite possible that one of your future Linux machines will be little-endian. I myself have various strategies to deal with this in high performance computing (portable binary file formats with minimum write overhead), so this is kind of important to me in practice. For others, byte order issues are not something they often encounter in real life. (However, it's not something that is easy to "fix" afterwards. You can get the code to work by using type-specific macros to convert it on the fly, but it gets really ugly real fast.)

Function and system call overhead is much smaller issue. Using fwrite(), consecutive calls are usually internally buffered, and the overhead is very small. Using low-level POSIX write(), the data is immediately handed off to the kernel, which means a small additional overhead. If this is just part of the header -- say, a couple of dozen calls, for a largeish file --, it does not matter, because the overhead is within the noise of the overall time taken. The approach does not work well if you were to save each pixel data using multiple writes, because the overhead becomes significant.

For one-off tests, I have used putchar()/putc()/fputc() to save the individual bytes, to overcome the byte order issues. It's slow, but not as slow as one might think -- I think it is still faster than Python I/O.

Originally Posted by Matticus

In your opinion, was this a sub-optimal solution, compared to the one you offered here (putting all bytes into a single buffer)?

I don't know. If your code is clean and maintainable, and you don't need to wait for the file save to complete, it's close enough.

In practice, my file handles tend to contain the pertinent information twice: once as easily-used variables, once as a header buffer. (If the header is emitted only once, and not modified afterwards, then the header buffer is usually local to the file creating function.)

Having the header structure described is very useful, and makes code maintenance easier. Bugs occur even in file format descriptions, and you sometimes have to work around them by tweaking the file format a bit. It is important, and that's why I listed the macros. Your code has the explicit write-to-files that describe the same thing.

Now that I look back, usually I've used a function to generate (and save) the header buffer, based on variables stored in the file handle. That way, the file format specifics are handled in one place, and much easier to maintain. For WAV, it might look something like

Code:

#define HEADER_LEN 44

static int wav_header(const wavfile *const w, unsigned char *const header)
{
    if (!w || !header)
        return EINVAL;

    /* 0..3: RIFF tag. */
    header[0] = 'R';
    header[1] = 'I';
    header[2] = 'F';
    header[3] = 'F';

    /* 4..7: RIFF length. Total file size - 8. */
    header[4] = ...

    /* 8..11: WAVE tag. */
    header[8] = 'W';
    header[9] = 'A';
    header[10] = 'V';
    header[11] = 'E';

    /* ... */

    return 0;
}

Because you need all format-related variables in the handle, you can use the same approach to save files in different formats. However, that does not exclude writing the header components separately.

For example, I sometimes test image processing or image-generating functions using the PNM format (PPM, to be specific). It has a very simple format:

'P' '6' <LWS>+ width <LWS>+ height <LWS>+ maxval <LWS> pixeldata

where <LWS> is whitespace (ASCII '\t', '\r', '\n', ' '); width, height, and maxval are decimal number strings, and pixeldata defines width×height binary pixels, starting at top left, each row at a time, with no padding.

If maxval < 256, then each pixel is represented by three bytes: red component first, then green, then blue. None of them may exceed maxval (so if maxval is 55 = 0x37, then \x37\x37\x37 refers to white).

If maxval > 255, each pixel is represented by six bytes. The first two define the red component, the next two the green component, and the next two the blue component. Of each pair, the most significant byte is first (big endian). (In other words, \x01\x00 is 256, whereas \x00\x01 is 1.)

It is especially nice, because it allows for 16 bits per component. No, I don't have that kind of displays, but it does help with further processing. I do sometimes use A2R10G10B10 format (with the two A bits actually mask bits), giving me 2³⁰ colors, but I typically end up generating two or more normal 8-bit-per-color images from that to see the details my display cannot represent. (You could do HDR image processing this way, perhaps even 32-bit Y16Cr8Cb8 pixels internally, converted to R16G16B16 on output?)

However, note that for more than 8 bits per component, the byte order is big endian, and my development machine is currently little-endian. What I do, then, is convert a scan line at a time, when saving. Doing it pixel-by-pixel (or color component by component) would be pretty slow (noticeable only if you generate lots of images), but doing the calculations into a one-scan buffer works out just fine for me. It's not optimal either, but it is darn robust, and fast enough.

To get some perspective on how I view things, my getpixel-setpixel prototypes are usually

Code:

void setpixel(image *const img, const int x, const int y, const color c);
color getpixel(const image *const img, const int x, const int y, const color outside);

That is, I do check the coordinates. So, speed is not really my main objective; my main concern is getting the programs work robustly, and be maintainable. I want to be able to trust my code.

Because of that, my opinion really depends on the *exact* code you wrote. If it looks clear and easily maintainable, and isn't so slow you notice it, it's good enough.