WAVE to FLAC encoder in C

This is a .wav to .flac encoder that I wrote a little while ago. The only method that is really called is encode(), which takes in a .wav file, converts it to FLAC, and stores it in the .flac file that is taken in as a parameter.

I would prefer suggestions on how to improve the method, decrease run-time, or cut down on the length of the code. Any other suggestions are acceptable though.

The header file (encode.h):

#include 

int encode(const char *wavfile, const char *flacfile);
static void progress_callback(const FLAC__StreamEncoder *encoder, FLAC__uint64 bytes_written, FLAC__uint64 samples_written, unsigned frames_written, unsigned total_frames_estimate, void *client_data);

The encoding program:

```
#include
#include
#include
#include "encode.h"

#define READSIZE 1020

static unsigned totalSamples = 0; / can use a 32-bit number due to WAVE size limitations /
static FLAC__byte buffer[READSIZE/samples/ 2/bytes_per_sample/ 2/channels/]; / we read the WAVE data into here /
static FLAC__int32 pcm[READSIZE/samples/ 2/channels*/];

int encode(const char wavfile, const char flacfile)
{
FLAC__bool ok = true;
FLAC__StreamEncoder *encoder = 0;
FLAC__StreamEncoderInitStatus initStatus;
FILE *fin;
unsigned sampleRate = 0;
unsigned channels = 0;
unsigned bps = 0;

if((fin = fopen(wavfile, "rb")) == NULL)
{
fprintf(stderr, "ERROR: opening %s for output\n", wavfile);
return 1;
}

/ read wav header and validate it /
if (fread(buffer, 1, 44, fin) != 44 || memcmp(buffer, "RIFF", 4) || memcmp(buffer + 8, "WAVEfmt \020\000\000\000\001\000\002\000", 16) ||memcmp(buffer + 32, "\004\000\020\000data", 8))
{
fprintf(stderr, "ERROR: invalid/unsupported WAVE file, only 16bps stereo WAVE in canonical form allowed\n");
fclose(fin);
return 1;
}
sampleRate = ((((((unsigned)buffer[27] READSIZE ? (size_t)READSIZE : (size_t)left);
if (fr

These two statement could be implemented by the same subroutine:

sampleRate = ((((((unsigned)buffer[27] << 8) | buffer[26]) << 8) | buffer[25]) << 8) | buffer[24];
totalSamples = (((((((unsigned)buffer[43] << 8) | buffer[42]) << 8) | buffer[41]) << 8) | buffer[40]) / 4;

For example:

unsigned read4bytes(FLAC__byte* ptr)
{
    return ((((((unsigned)*(ptr+3) << 8) | *(ptr+2)) << 8) | *(ptr+1)) << 8) | *(ptr);
}

and

sampleRate = read4bytes(buffer + 24);
totalSamples = read4bytes(buffer + 40);

You're using ok to skip processing until you call fclose at the end. Instead all that could be a subroutine ...

fin = fopen(wavfile, "rb");
rc = encodeFile(fin, flacfile);
fclose(fin);

... and then instead of having ok in encodeFile, you can return when (as soon as) you detect an error.

You currently don't fprintf a specific error message if a function call such as FLAC__stream_encoder_set_verify fails.

I don't know why READSIZE value is 1020.

channels and bps could be set to 2 and 16 at the start of the function; or they could be const or macros like READFILE is.

For the second subroutine, I had something like the following in mind (untested code ahead).

• By eliminating the ok variable I eliminate if statements in the body of the code.

• As soon as encodeFile detects an error, it prints an error and immediately returns.

• The calling routine which opened the file is responsible for closing the file; there's now only one fclose(fin) statement.

I did something similar (i.e. make a subroutine) to wrap the lifetime of the allocated encoder object.

#include 
#include 
#include 
#include "encode.h"

#define READSIZE 1020

static unsigned totalSamples = 0; /* can use a 32-bit number due to WAVE size limitations */
static FLAC__byte buffer[READSIZE/*samples*/ * 2/*bytes_per_sample*/ * 2/*channels*/]; /* we read the WAVE data into here */
static FLAC__int32 pcm[READSIZE/*samples*/ * 2/*channels*/];

int encodeFile2(FILE *fin, FLAC__StreamEncoder *encoder, const char *flacfile)
{
    unsigned sampleRate = ((((((unsigned)buffer[27] READSIZE ? (size_t)READSIZE : (size_t)left);
        if (fread(buffer, channels * (bps / 8), need, fin) != need) 
        {
            return fprintf(stderr, "ERROR: reading from WAVE file\n");
        }
        /* convert the packed little-endian 16-bit PCM samples from WAVE into an interleaved FLAC__int32 buffer for libFLAC */
        size_t i;
        for(i = 0; i < need*channels; i++)
        {
            /* inefficient but simple and works on big- or little-endian machines */
            pcm[i] = (FLAC__int32)(((FLAC__int16)(FLAC__int8)buffer[2 * i + 1] << 8) | (FLAC__int16)buffer[2 * i]);
        }
        /* feed samples to encoder */
        if (FLAC__stream_encoder_process_interleaved(encoder, pcm, need))
        {
            return fprintf(stderr, "ERROR: processing WAVE file: %s\n", FLAC__StreamEncoderStateString[FLAC__stream_encoder_get_state(encoder)]);
        }
        left -= need;
    }

    if (!FLAC__stream_encoder_finish(encoder))
    {
        return fprintf(stderr, "ERROR: finishing encoder: %s\n", FLAC__StreamEncoderStateString[FLAC__stream_encoder_get_state(encoder)]);
    }

    fprintf(stdout, "Succeeded");
    return 0;
}
int encodeFile1(FILE *fin, const char *flacfile)
{
    /* read wav header and validate it */
    /* before allocating the encoder */
    if (fread(buffer, 1, 44, fin) != 44 || memcmp(buffer, "RIFF", 4) || memcmp(buffer + 8, "WAVEfmt \020\000\000\000\001\000\002\000", 16) ||memcmp(buffer + 32, "\004\000\020\000data", 8))
    {
        return fprintf(stderr, "ERROR: invalid/unsupported WAVE file, only 16bps stereo WAVE in canonical form allowed\n");
    }

    /* allocate the encoder */
    FLAC__StreamEncoder *encoder;
    if((encoder = FLAC__stream_encoder_new()) == NULL) 
    {
        return fprintf(stderr, "ERROR: allocating encoder\n");
    }

    /* use the encoder */
    rc = encodeFile2(fin, encoder, flacfile);

    /* deallocate the encoder */
    FLAC__stream_encoder_delete(encoder);
    return rc;
}
int encode(const char *wavfile, const char *flacfile)
{
    FILE *fin;
    int rc;
    if((fin = fopen(wavfile, "rb")) == NULL)
    {
        fprintf(stderr, "ERROR: opening %s for output\n", wavfile);
        return 1;
    }
    rc = encodeFile1(fin, flacfile);
    fclose(fin);
    return (rc) ? 1 : 0;
}

I also changed to location where local variables are defined: instead of defining them at the top of the function, they're now not defined until they're initialized (which you seemed to be doing already, though inconsistently, in your OP; the ability to do so is newer-style C).

The above changes are stylistic:

• Shorter and simpler

• Easier to verify (by inspection) that allocated resources are closed/freed.

As for performance, the first rule is to use run-time profiler (don't try to guess what's wrong, and don't bother to performance-optimize code which isn't a performance bo

StackExchange Code Review Q#38944, answer score: 4